[libcamera-devel] [RFC PATCH 4/5] WIP: libcamera: V4L2VideoDevice: Fix a bug in CreateBuffer()

[Hirokazu Honda]

Hi Laurent, thank you for reviewing.

On Thu, Aug 12, 2021 at 8:22 AM Laurent Pinchart
<laurent.pinchart at 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 at 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