[libcamera-devel] [RFC PATCH v1 11/12] libcamera: v4l2_videodevice: Split planes when dequeuing buffer

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Fri, Sep 3, 2021 at 3:31 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Thu, Sep 02, 2021 at 12:17:31PM +0100, Kieran Bingham wrote:
> > On 02/09/2021 05:23, Laurent Pinchart wrote:
> > > When dequeueing a buffer from a V4L2VideoDevice, the number of planes in
> > > the FrameBuffer may not match the number of V4L2 buffer planes if the
> > > PixelFormat is multi-planar (has multiple colour planes) and the V4L2
> > > format is single-planar (has a single buffer plane). In this case, we
> > > need to split the single V4L2 buffer plane into FrameBuffer planes. Do
> > > so, and add checks to reject invalid V4L2 buffers in case of a driver
> > > issue.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/libcamera/v4l2_videodevice.cpp | 54 ++++++++++++++++++++++++------
> > >  1 file changed, 43 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 79cb792117d5..b80b9038a914 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -7,6 +7,7 @@
> > >
> > >  #include "libcamera/internal/v4l2_videodevice.h"
> > >
> > > +#include <algorithm>
> > >  #include <array>
> > >  #include <fcntl.h>
> > >  #include <iomanip>
> > > @@ -1637,18 +1638,49 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >     buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> > >                                 + buf.timestamp.tv_usec * 1000ULL;
> > >
> > > -   if (multiPlanar) {
> > > -           if (buf.length > buffer->metadata_.planes().size()) {
> > > -                   LOG(V4L2, Error)
> > > -                           << "Invalid number of planes (" << buf.length
> > > -                           << " != " << buffer->metadata_.planes().size() << ")";
> > > -                   return nullptr;
> > > +   if (V4L2_TYPE_IS_CAPTURE(buf.type)) {
> >
> > Should we swap this around and return the buffer early to lower the
> > indents below?
> >
> >  if (!V4L2_TYPE_IS_CAPTURE(buf.type)) {
> >       /* Output buffer is completed */
> >       return buffer;
> >  }
>
> Yes, good point.
>
> > > +           unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
> > > +           FrameMetadata &metadata = buffer->metadata_;
> > > +
> > > +           if (numV4l2Planes != metadata.planes().size()) {

If my comment to 10/12 is correct, could you implement this like
So I would implement as follows.
if (multiPlanar) {
   ...
} else {
  if (numV4l2Planes != metadata.planes.size()) {
    ...
  } else {
     ...
}

Thanks,
-Hiro

> > > +                   /*
> > > +                    * If we have a multi-planar buffer with a V4L2
> > > +                    * single-planar format, split the V4L2 buffer across
> > > +                    * the buffer planes. Only the last plane may have less
> > > +                    * bytes used than its length.
> > > +                    */
> > > +                   if (numV4l2Planes != 1) {
> > > +                           LOG(V4L2, Error)
> > > +                                   << "Invalid number of planes (" << numV4l2Planes
> > > +                                   << " != " << metadata.planes().size() << ")";
> > > +                           return nullptr;
> > > +                   }
> > > +
> > > +                   unsigned int bytesused = multiPlanar ? planes[0].bytesused
> > > +                                          : buf.bytesused;
> > > +
> > > +                   for (auto [i, plane] : utils::enumerate(buffer->planes())) {
> > > +                           if (!bytesused) {
> > > +                                   LOG(V4L2, Error)
> > > +                                           << "Dequeued buffer is too small";
> >
> > Presumably if we got here - it's likely something (hardware) already
> > overwrote memory it likely didn't own?
>
> It likely means something bad happened, yes. It's a bit of defensive
> programming, to output a clear error message if the kernel does
> something bad, but also to possibly catch bugs in the implementation of
> this function.
>
> > > +                                   return nullptr;
> >
> > I'm still very weary of returning nullptrs on dequeue when a buffer is
> > removed from queuedBuffers_
> >
> >
> > Wouldn't it be better to return the buffer, but mark it as corrupted or
> > invalid or such ?
>
> Yes, I'll do that.
>
> > > +                           }
> > > +
> > > +                           metadata.planes()[i].bytesused =
> > > +                                   std::min(plane.length, bytesused);
> > > +                           bytesused -= metadata.planes()[i].bytesused;
> > > +                   }
> > > +           } else if (multiPlanar) {
> > > +                   /*
> > > +                    * If we use the multi-planar API, fill in the planes.
> > > +                    * The number of planes in the frame buffer and in the
> > > +                    * V4L2 buffer is guaranteed to be equal at this point.
> > > +                    */
> > > +                   for (unsigned int i = 0; i < numV4l2Planes; ++i)
> > > +                           metadata.planes()[i].bytesused = planes[i].bytesused;
> > > +           } else {
> > > +                   metadata.planes()[0].bytesused = buf.bytesused;
> > >             }
> > > -
> > > -           for (unsigned int nplane = 0; nplane < buf.length; nplane++)
> > > -                   buffer->metadata_.planes()[nplane].bytesused = planes[nplane].bytesused;
> > > -   } else {
> > > -           buffer->metadata_.planes()[0].bytesused = buf.bytesused;
> > >     }
> > >
> > >     return buffer;
> > >
>
> --
> Regards,
>
> Laurent Pinchart