[libcamera-devel] [PATCH v2 14/27] libcamera: v4l2_videodevice: Split planes when dequeuing buffer

[Laurent Pinchart]

Hi Hiro,

On Mon, Sep 06, 2021 at 10:16:47PM +0900, Hirokazu Honda wrote:
> On Mon, Sep 6, 2021 at 8:58 PM Laurent Pinchart wrote:
> > On Mon, Sep 06, 2021 at 09:22:56AM +0200, Jean-Michel Hautbois wrote:
> > > On 06/09/2021 04:00, 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.
> > >
> > > What checks are those exactly ? The test on the dequeued buffer size ?
> >
> > Yes, and the test on the number of V4L2 planes.
> >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Reduce indentation
> > > > ---
> > > >  src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++---
> > > >  1 file changed, 48 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > index 625d5da40337..0a7fbdb7e011 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>
> > > > @@ -1664,12 +1665,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > > >     buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> > > >                                 + buf.timestamp.tv_usec * 1000ULL;
> > > >
> > > > -   buffer->metadata_.planes.clear();
> > > > -   if (multiPlanar) {
> > > > -           for (unsigned int nplane = 0; nplane < buf.length; nplane++)
> > > > -                   buffer->metadata_.planes.push_back({ planes[nplane].bytesused });
> > > > +   if (V4L2_TYPE_IS_OUTPUT(buf.type))
> > > > +           return buffer;
> > > > +
> > > > +   unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
> > > > +   FrameMetadata &metadata = buffer->metadata_;
> > > > +   metadata.planes.clear();
> > > > +
> > > > +   if (numV4l2Planes != buffer->planes().size()) {
> > > > +           /*
> > > > +            * 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
> > > > +                           << " != " << buffer->planes().size() << ")";
> > > > +
> > > > +                   metadata.status = FrameMetadata::FrameError;
> > > > +                   return buffer;
> > > > +           }
> > > > +
> > > > +           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";
> > > > +
> > > > +                           metadata.status = FrameMetadata::FrameError;
> > > > +                           return buffer;
> > > > +                   }
> > > > +
> > > > +                   metadata.planes.push_back({ std::min(plane.length, bytesused) });
> > > > +                   bytesused -= metadata.planes.back().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.push_back({ planes[i].bytesused });
> > > >     } else {
> > > > -           buffer->metadata_.planes.push_back({ buf.bytesused });
> > > > +           metadata.planes.push_back({ buf.bytesused });
> > > >     }
> 
> I think the latter two cases can be just
> } else {
>   for (unsigned int i = 0; i < numV4l2Planes; ++i)
>       metadata.planes.push_back({ planes[i].bytesused });
> }

In the multi planar case, the bytesused value is retrieved from
planes[i].bytesused, while in the single planar case, it comes from
buf.bytesused.

> With the nit,
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> 
> > > >
> > > >     return buffer;

-- 
Regards,

Laurent Pinchart