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

[Laurent Pinchart]

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 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.

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));
> > > > >  }
> > > > >

-- 
Regards,

Laurent Pinchart