[libcamera-devel,RFC,4/5] WIP: libcamera: V4L2VideoDevice: Fix a bug in CreateBuffer()
diff mbox series

Message ID 20210811124015.2116188-5-hiroh@chromium.org
State Superseded
Headers show
Series
  • MappedFrameBuffer::maps() returns the plane address
Related show

Commit Message

Hirokazu Honda Aug. 11, 2021, 12:40 p.m. UTC
FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same
number of planes as v4l2 buffer planes. However, if the format is
a single planar format, the number of planes is one. FrameBuffer
should have the same number of planes as color format planes.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Aug. 11, 2021, 11:22 p.m. UTC | #1
Hi Hiro,

Thank you for the patch.

On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:
> FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same
> number of planes as v4l2 buffer planes. However, if the format is
> a single planar format, the number of planes is one. FrameBuffer
> should have the same number of planes as color format planes.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ce60dff6..e70076f3 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1269,7 +1269,6 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  
>  	const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
>  	const unsigned int numPlanes = multiPlanar ? buf.length : 1;
> -

Unrelated change ?

>  	if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
>  		LOG(V4L2, Error) << "Invalid number of planes";
>  		return nullptr;
> @@ -1289,6 +1288,20 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  		planes.push_back(std::move(plane));
>  	}
>  
> +

Extra blank line.

> +	V4L2DeviceFormat format{};

V4L2DeviceFormat initializes all members to 0 when constructed, you can
drop {}.

> +	ret = getFormat(&format);
> +	if (ret < 0) {
> +		LOG(V4L2, Error) << "Failed to get buffer format";
> +		return nullptr;
> +	}

I'd move the extra blank line from above here :-)

This being said, maybe we could cache the fourcc in V4L2VideoDevice
instead of querying it back from the kernel ?

> +	if (format.fourcc == V4L2_PIX_FMT_NV12) {
> +		planes.resize(2);
> +		planes[0].length = format.size.width * format.size.height;
> +		planes[1].fd = planes[0].fd;
> +		planes[1].length = format.size.width * ((format.size.height + 1) / 2);
> +	}

I don't think this is right. Well, it is, but at the same time, it isn't
:-)

At the moment, the number of planes in the FrameBuffer class matches the
number of "buffer planes" used by V4L2 (1 for NV12, as opposed to the
number of "format planes", which would be 2). I agree that we want to
move to "format planes" in the long term here, to avoid exposing the
V4L2 historical design mistake to applications ("long term" meaning "as
soon as possible" here). However, this requires adding an offset field
to the FrameBuffer::Plane structure, as otherwise we miss the
information to implement the calculation properly (or at least we miss
exposing the information explictly and rely on undocumented
implementation details).

There's no need, I think, to implement this as part of this series. We
can keep the calculation internal to MappedFrameBuffer as a first step,
and then address the issue in the FrameBuffer class. It may also be
possible to do it the other way around, and move to "format planes" in
FrameBuffer at the bottom of the series. I have a feeling that it may be
difficult to keep patches reasonable in size and avoid bisection
breakages in that case, but I may be wrong.

> +
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>
Hirokazu Honda Aug. 12, 2021, 5:18 a.m. UTC | #2
Hi Laurent, thank you for reviewing.

On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:
> > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same
> > number of planes as v4l2 buffer planes. However, if the format is
> > a single planar format, the number of planes is one. FrameBuffer
> > should have the same number of planes as color format planes.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index ce60dff6..e70076f3 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1269,7 +1269,6 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> >
> >       const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> >       const unsigned int numPlanes = multiPlanar ? buf.length : 1;
> > -
>
> Unrelated change ?
>
> >       if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
> >               LOG(V4L2, Error) << "Invalid number of planes";
> >               return nullptr;
> > @@ -1289,6 +1288,20 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> >               planes.push_back(std::move(plane));
> >       }
> >
> > +
>
> Extra blank line.
>
> > +     V4L2DeviceFormat format{};
>
> V4L2DeviceFormat initializes all members to 0 when constructed, you can
> drop {}.
>
> > +     ret = getFormat(&format);
> > +     if (ret < 0) {
> > +             LOG(V4L2, Error) << "Failed to get buffer format";
> > +             return nullptr;
> > +     }
>
> I'd move the extra blank line from above here :-)
>
> This being said, maybe we could cache the fourcc in V4L2VideoDevice
> instead of querying it back from the kernel ?
>
> > +     if (format.fourcc == V4L2_PIX_FMT_NV12) {
> > +             planes.resize(2);
> > +             planes[0].length = format.size.width * format.size.height;
> > +             planes[1].fd = planes[0].fd;
> > +             planes[1].length = format.size.width * ((format.size.height + 1) / 2);
> > +     }
>
> I don't think this is right. Well, it is, but at the same time, it isn't
> :-)
>
> At the moment, the number of planes in the FrameBuffer class matches the
> number of "buffer planes" used by V4L2 (1 for NV12, as opposed to the
> number of "format planes", which would be 2). I agree that we want to
> move to "format planes" in the long term here, to avoid exposing the
> V4L2 historical design mistake to applications ("long term" meaning "as
> soon as possible" here). However, this requires adding an offset field
> to the FrameBuffer::Plane structure, as otherwise we miss the
> information to implement the calculation properly (or at least we miss
> exposing the information explictly and rely on undocumented
> implementation details).
>

I agree. We need `offset`.

> There's no need, I think, to implement this as part of this series. We
> can keep the calculation internal to MappedFrameBuffer as a first step,
> and then address the issue in the FrameBuffer class. It may also be
> possible to do it the other way around, and move to "format planes" in
> FrameBuffer at the bottom of the series. I have a feeling that it may be
> difficult to keep patches reasonable in size and avoid bisection
> breakages in that case, but I may be wrong.
>

MappedFrameBuffer (or even FrameBuffer) doesn't have info about format.
In the single planar NV12 case, the created FrameBuffer::planes has one plane.
How can I workaround within MappedFrameBuffer while MappedFrameBuffer
couldn't recognize the number of format planes is two?
I also wonder if FrameBuffer::plane.fds are different in planes thanks
to exportDmabuf?

Best Regards,
-Hiro
> > +
> >       return std::make_unique<FrameBuffer>(std::move(planes));
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Aug. 12, 2021, 8:25 p.m. UTC | #3
Hi Hiro,

On Thu, Aug 12, 2021 at 02:18:11PM +0900, Hirokazu Honda wrote:
> On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart wrote:
> > On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:
> > > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same
> > > number of planes as v4l2 buffer planes. However, if the format is
> > > a single planar format, the number of planes is one. FrameBuffer
> > > should have the same number of planes as color format planes.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >  src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index ce60dff6..e70076f3 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1269,7 +1269,6 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > >
> > >       const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> > >       const unsigned int numPlanes = multiPlanar ? buf.length : 1;
> > > -
> >
> > Unrelated change ?
> >
> > >       if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
> > >               LOG(V4L2, Error) << "Invalid number of planes";
> > >               return nullptr;
> > > @@ -1289,6 +1288,20 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > >               planes.push_back(std::move(plane));
> > >       }
> > >
> > > +
> >
> > Extra blank line.
> >
> > > +     V4L2DeviceFormat format{};
> >
> > V4L2DeviceFormat initializes all members to 0 when constructed, you can
> > drop {}.
> >
> > > +     ret = getFormat(&format);
> > > +     if (ret < 0) {
> > > +             LOG(V4L2, Error) << "Failed to get buffer format";
> > > +             return nullptr;
> > > +     }
> >
> > I'd move the extra blank line from above here :-)
> >
> > This being said, maybe we could cache the fourcc in V4L2VideoDevice
> > instead of querying it back from the kernel ?
> >
> > > +     if (format.fourcc == V4L2_PIX_FMT_NV12) {
> > > +             planes.resize(2);
> > > +             planes[0].length = format.size.width * format.size.height;
> > > +             planes[1].fd = planes[0].fd;
> > > +             planes[1].length = format.size.width * ((format.size.height + 1) / 2);
> > > +     }
> >
> > I don't think this is right. Well, it is, but at the same time, it isn't
> > :-)
> >
> > At the moment, the number of planes in the FrameBuffer class matches the
> > number of "buffer planes" used by V4L2 (1 for NV12, as opposed to the
> > number of "format planes", which would be 2). I agree that we want to
> > move to "format planes" in the long term here, to avoid exposing the
> > V4L2 historical design mistake to applications ("long term" meaning "as
> > soon as possible" here). However, this requires adding an offset field
> > to the FrameBuffer::Plane structure, as otherwise we miss the
> > information to implement the calculation properly (or at least we miss
> > exposing the information explictly and rely on undocumented
> > implementation details).
> 
> I agree. We need `offset`.
> 
> > There's no need, I think, to implement this as part of this series. We
> > can keep the calculation internal to MappedFrameBuffer as a first step,
> > and then address the issue in the FrameBuffer class. It may also be
> > possible to do it the other way around, and move to "format planes" in
> > FrameBuffer at the bottom of the series. I have a feeling that it may be
> > difficult to keep patches reasonable in size and avoid bisection
> > breakages in that case, but I may be wrong.
> 
> MappedFrameBuffer (or even FrameBuffer) doesn't have info about format.
> In the single planar NV12 case, the created FrameBuffer::planes has one plane.
> How can I workaround within MappedFrameBuffer while MappedFrameBuffer
> couldn't recognize the number of format planes is two?

MappedFrameBuffer is indeed missing that information. One option could
be to then pass the format to the constructor. The other option is to go
ahead with the full change, and add the offset field to
FrameBuffer::Plane. If you decide to go with the latter (it's a bit more
work, but will bring much larger benefits), please let me know if I can
help you.

> I also wonder if FrameBuffer::plane.fds are different in planes thanks
> to exportDmabuf?

When using NV12M, we will get two buffer planes from V4L2, which will be
exported in two calls to exportDmabufFd(). With NV12, there will be a
single buffer plane, so a single fd. In both case, the V4L2VideoDevice
createBuffer() function will need to initialize two Plane entries in
FrameBuffer.

The NV12M case is easy. In the NV12 case it will be a bit more
difficult. Brainstorming a little bit, we can compare the number of
buffer planes reported by V4L2 and the number of planes reported by
PixelFormatInfo::numPlanes(). If they differ, we know we have to handle
the calculation internally.

> > > +
> > >       return std::make_unique<FrameBuffer>(std::move(planes));
> > >  }
> > >
Hirokazu Honda Aug. 13, 2021, 3:29 a.m. UTC | #4
Hi Laurent,


On Fri, Aug 13, 2021 at 5:25 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Thu, Aug 12, 2021 at 02:18:11PM +0900, Hirokazu Honda wrote:
> > On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart wrote:
> > > On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:
> > > > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same
> > > > number of planes as v4l2 buffer planes. However, if the format is
> > > > a single planar format, the number of planes is one. FrameBuffer
> > > > should have the same number of planes as color format planes.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > ---
> > > >  src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-
> > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > index ce60dff6..e70076f3 100644
> > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > @@ -1269,7 +1269,6 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > > >
> > > >       const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> > > >       const unsigned int numPlanes = multiPlanar ? buf.length : 1;
> > > > -
> > >
> > > Unrelated change ?
> > >
> > > >       if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
> > > >               LOG(V4L2, Error) << "Invalid number of planes";
> > > >               return nullptr;
> > > > @@ -1289,6 +1288,20 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > > >               planes.push_back(std::move(plane));
> > > >       }
> > > >
> > > > +
> > >
> > > Extra blank line.
> > >
> > > > +     V4L2DeviceFormat format{};
> > >
> > > V4L2DeviceFormat initializes all members to 0 when constructed, you can
> > > drop {}.
> > >
> > > > +     ret = getFormat(&format);
> > > > +     if (ret < 0) {
> > > > +             LOG(V4L2, Error) << "Failed to get buffer format";
> > > > +             return nullptr;
> > > > +     }
> > >
> > > I'd move the extra blank line from above here :-)
> > >
> > > This being said, maybe we could cache the fourcc in V4L2VideoDevice
> > > instead of querying it back from the kernel ?
> > >
> > > > +     if (format.fourcc == V4L2_PIX_FMT_NV12) {
> > > > +             planes.resize(2);
> > > > +             planes[0].length = format.size.width * format.size.height;
> > > > +             planes[1].fd = planes[0].fd;
> > > > +             planes[1].length = format.size.width * ((format.size.height + 1) / 2);
> > > > +     }
> > >
> > > I don't think this is right. Well, it is, but at the same time, it isn't
> > > :-)
> > >
> > > At the moment, the number of planes in the FrameBuffer class matches the
> > > number of "buffer planes" used by V4L2 (1 for NV12, as opposed to the
> > > number of "format planes", which would be 2). I agree that we want to
> > > move to "format planes" in the long term here, to avoid exposing the
> > > V4L2 historical design mistake to applications ("long term" meaning "as
> > > soon as possible" here). However, this requires adding an offset field
> > > to the FrameBuffer::Plane structure, as otherwise we miss the
> > > information to implement the calculation properly (or at least we miss
> > > exposing the information explictly and rely on undocumented
> > > implementation details).
> >
> > I agree. We need `offset`.
> >
> > > There's no need, I think, to implement this as part of this series. We
> > > can keep the calculation internal to MappedFrameBuffer as a first step,
> > > and then address the issue in the FrameBuffer class. It may also be
> > > possible to do it the other way around, and move to "format planes" in
> > > FrameBuffer at the bottom of the series. I have a feeling that it may be
> > > difficult to keep patches reasonable in size and avoid bisection
> > > breakages in that case, but I may be wrong.
> >
> > MappedFrameBuffer (or even FrameBuffer) doesn't have info about format.
> > In the single planar NV12 case, the created FrameBuffer::planes has one plane.
> > How can I workaround within MappedFrameBuffer while MappedFrameBuffer
> > couldn't recognize the number of format planes is two?
>
> MappedFrameBuffer is indeed missing that information. One option could
> be to then pass the format to the constructor. The other option is to go
> ahead with the full change, and add the offset field to
> FrameBuffer::Plane. If you decide to go with the latter (it's a bit more
> work, but will bring much larger benefits), please let me know if I can
> help you.
>

If we pass format info to MappedFrameBuffer, we need to carry the
format info in another way around.
I am going to introduce offset to FrameBuffer::Plane. It is the correct manner.
Once you acknowledge this, I will start working for this.

-Hiro
> > I also wonder if FrameBuffer::plane.fds are different in planes thanks
> > to exportDmabuf?
>
> When using NV12M, we will get two buffer planes from V4L2, which will be
> exported in two calls to exportDmabufFd(). With NV12, there will be a
> single buffer plane, so a single fd. In both case, the V4L2VideoDevice
> createBuffer() function will need to initialize two Plane entries in
> FrameBuffer.
>
> The NV12M case is easy. In the NV12 case it will be a bit more
> difficult. Brainstorming a little bit, we can compare the number of
> buffer planes reported by V4L2 and the number of planes reported by
> PixelFormatInfo::numPlanes(). If they differ, we know we have to handle
> the calculation internally.
>
> > > > +
> > > >       return std::make_unique<FrameBuffer>(std::move(planes));
> > > >  }
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Aug. 13, 2021, 3:43 a.m. UTC | #5
Hi Hiro,

On Fri, Aug 13, 2021 at 12:29:30PM +0900, Hirokazu Honda wrote:
> On Fri, Aug 13, 2021 at 5:25 AM Laurent Pinchart wrote:
> > On Thu, Aug 12, 2021 at 02:18:11PM +0900, Hirokazu Honda wrote:
> > > On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart wrote:
> > > > On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:
> > > > > FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same
> > > > > number of planes as v4l2 buffer planes. However, if the format is
> > > > > a single planar format, the number of planes is one. FrameBuffer
> > > > > should have the same number of planes as color format planes.
> > > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > ---
> > > > >  src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-
> > > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > > index ce60dff6..e70076f3 100644
> > > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > > @@ -1269,7 +1269,6 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > > > >
> > > > >       const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> > > > >       const unsigned int numPlanes = multiPlanar ? buf.length : 1;
> > > > > -
> > > >
> > > > Unrelated change ?
> > > >
> > > > >       if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
> > > > >               LOG(V4L2, Error) << "Invalid number of planes";
> > > > >               return nullptr;
> > > > > @@ -1289,6 +1288,20 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > > > >               planes.push_back(std::move(plane));
> > > > >       }
> > > > >
> > > > > +
> > > >
> > > > Extra blank line.
> > > >
> > > > > +     V4L2DeviceFormat format{};
> > > >
> > > > V4L2DeviceFormat initializes all members to 0 when constructed, you can
> > > > drop {}.
> > > >
> > > > > +     ret = getFormat(&format);
> > > > > +     if (ret < 0) {
> > > > > +             LOG(V4L2, Error) << "Failed to get buffer format";
> > > > > +             return nullptr;
> > > > > +     }
> > > >
> > > > I'd move the extra blank line from above here :-)
> > > >
> > > > This being said, maybe we could cache the fourcc in V4L2VideoDevice
> > > > instead of querying it back from the kernel ?
> > > >
> > > > > +     if (format.fourcc == V4L2_PIX_FMT_NV12) {
> > > > > +             planes.resize(2);
> > > > > +             planes[0].length = format.size.width * format.size.height;
> > > > > +             planes[1].fd = planes[0].fd;
> > > > > +             planes[1].length = format.size.width * ((format.size.height + 1) / 2);
> > > > > +     }
> > > >
> > > > I don't think this is right. Well, it is, but at the same time, it isn't
> > > > :-)
> > > >
> > > > At the moment, the number of planes in the FrameBuffer class matches the
> > > > number of "buffer planes" used by V4L2 (1 for NV12, as opposed to the
> > > > number of "format planes", which would be 2). I agree that we want to
> > > > move to "format planes" in the long term here, to avoid exposing the
> > > > V4L2 historical design mistake to applications ("long term" meaning "as
> > > > soon as possible" here). However, this requires adding an offset field
> > > > to the FrameBuffer::Plane structure, as otherwise we miss the
> > > > information to implement the calculation properly (or at least we miss
> > > > exposing the information explictly and rely on undocumented
> > > > implementation details).
> > >
> > > I agree. We need `offset`.
> > >
> > > > There's no need, I think, to implement this as part of this series. We
> > > > can keep the calculation internal to MappedFrameBuffer as a first step,
> > > > and then address the issue in the FrameBuffer class. It may also be
> > > > possible to do it the other way around, and move to "format planes" in
> > > > FrameBuffer at the bottom of the series. I have a feeling that it may be
> > > > difficult to keep patches reasonable in size and avoid bisection
> > > > breakages in that case, but I may be wrong.
> > >
> > > MappedFrameBuffer (or even FrameBuffer) doesn't have info about format.
> > > In the single planar NV12 case, the created FrameBuffer::planes has one plane.
> > > How can I workaround within MappedFrameBuffer while MappedFrameBuffer
> > > couldn't recognize the number of format planes is two?
> >
> > MappedFrameBuffer is indeed missing that information. One option could
> > be to then pass the format to the constructor. The other option is to go
> > ahead with the full change, and add the offset field to
> > FrameBuffer::Plane. If you decide to go with the latter (it's a bit more
> > work, but will bring much larger benefits), please let me know if I can
> > help you.
> >
> 
> If we pass format info to MappedFrameBuffer, we need to carry the
> format info in another way around.
> I am going to introduce offset to FrameBuffer::Plane. It is the correct manner.
> Once you acknowledge this, I will start working for this.

Ack :-)

Overall I think we have all the pieces we need to introduce this, but if
you run into any issue along the way, please feel free to report it and
we can discuss solutions.

> > > I also wonder if FrameBuffer::plane.fds are different in planes thanks
> > > to exportDmabuf?
> >
> > When using NV12M, we will get two buffer planes from V4L2, which will be
> > exported in two calls to exportDmabufFd(). With NV12, there will be a
> > single buffer plane, so a single fd. In both case, the V4L2VideoDevice
> > createBuffer() function will need to initialize two Plane entries in
> > FrameBuffer.
> >
> > The NV12M case is easy. In the NV12 case it will be a bit more
> > difficult. Brainstorming a little bit, we can compare the number of
> > buffer planes reported by V4L2 and the number of planes reported by
> > PixelFormatInfo::numPlanes(). If they differ, we know we have to handle
> > the calculation internally.
> >
> > > > > +
> > > > >       return std::make_unique<FrameBuffer>(std::move(planes));
> > > > >  }
> > > > >

Patch
diff mbox series

diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index ce60dff6..e70076f3 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -1269,7 +1269,6 @@  std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
 
 	const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
 	const unsigned int numPlanes = multiPlanar ? buf.length : 1;
-
 	if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
 		LOG(V4L2, Error) << "Invalid number of planes";
 		return nullptr;
@@ -1289,6 +1288,20 @@  std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
 		planes.push_back(std::move(plane));
 	}
 
+
+	V4L2DeviceFormat format{};
+	ret = getFormat(&format);
+	if (ret < 0) {
+		LOG(V4L2, Error) << "Failed to get buffer format";
+		return nullptr;
+	}
+	if (format.fourcc == V4L2_PIX_FMT_NV12) {
+		planes.resize(2);
+		planes[0].length = format.size.width * format.size.height;
+		planes[1].fd = planes[0].fd;
+		planes[1].length = format.size.width * ((format.size.height + 1) / 2);
+	}
+
 	return std::make_unique<FrameBuffer>(std::move(planes));
 }