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

[Hirokazu Honda]

Hi Laurent,


On Fri, Aug 13, 2021 at 5:25 AM Laurent Pinchart
<laurent.pinchart at 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 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?
>
> 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