[libcamera-devel] [PATCH v2 13/27] libcamera: v4l2_videodevice: Coalesce planes when queuing buffer

[Laurent Pinchart]

Hi Hiro,

On Mon, Sep 06, 2021 at 10:06:55PM +0900, Hirokazu Honda wrote:
> On Mon, Sep 6, 2021 at 11:01 AM Laurent Pinchart wrote:
> >
> > When queueing a buffer to 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 coalesce all FrameBuffer planes into a single V4L2 buffer plane.
> > Do so, and add validity checks to reject frame buffers that can't be
> > described using a single V4L2 buffer plane.
> >
> > This change prepares for proper multi-planar support, but isn't expected
> > to result in a change of behaviour with existing pipeline handlers, as
> > none of them queue an output buffer with multiple FrameBuffer planes or
> > use non-contiguous buffers for either capture or output.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > Changes since v1:
> >
> > - Make numV4l2Planes const
> > - Use format_.planesCount
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 67 +++++++++++++++++++++++++-----
> >  1 file changed, 57 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index c6c9263c49e9..625d5da40337 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -22,10 +22,12 @@
> >
> >  #include <libcamera/base/event_notifier.h>
> >  #include <libcamera/base/log.h>
> > +#include <libcamera/base/utils.h>
> >
> >  #include <libcamera/file_descriptor.h>
> >
> >  #include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/framebuffer.h"
> >  #include "libcamera/internal/media_device.h"
> >  #include "libcamera/internal/media_object.h"
> >
> > @@ -1496,10 +1498,20 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >
> >         bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> >         const std::vector<FrameBuffer::Plane> &planes = buffer->planes();
> > +       const unsigned int numV4l2Planes = format_.planesCount;
> > +
> > +       /*
> > +        * If the frame buffer has multiple planes and the V4L2 format requires
> > +        * contiguous planes, ensure that's the case.
> > +        */
> > +       if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
> > +               LOG(V4L2, Error) << "Device format requires contiguous buffer";
> > +               return -EINVAL;
> > +       }
> 
> nit: shall we also check planes.size() >= numV4l2Planes?

It's a good idea, I'll do so.

> >
> >         if (buf.memory == V4L2_MEMORY_DMABUF) {
> >                 if (multiPlanar) {
> > -                       for (unsigned int p = 0; p < planes.size(); ++p)
> > +                       for (unsigned int p = 0; p < numV4l2Planes; ++p)
> >                                 v4l2Planes[p].m.fd = planes[p].fd.fd();
> >                 } else {
> >                         buf.m.fd = planes[0].fd.fd();
> > @@ -1507,23 +1519,58 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >         }
> >
> >         if (multiPlanar) {
> > -               buf.length = planes.size();
> > +               buf.length = numV4l2Planes;
> >                 buf.m.planes = v4l2Planes;
> >         }
> >
> >         if (V4L2_TYPE_IS_OUTPUT(buf.type)) {
> >                 const FrameMetadata &metadata = buffer->metadata();
> >
> > -               if (multiPlanar) {
> > -                       unsigned int nplane = 0;
> > -                       for (const FrameMetadata::Plane &plane : metadata.planes) {
> > -                               v4l2Planes[nplane].bytesused = plane.bytesused;
> > -                               v4l2Planes[nplane].length = buffer->planes()[nplane].length;
> > -                               nplane++;
> > +               if (numV4l2Planes != planes.size()) {
> > +                       /*
> > +                        * If we have a multi-planar buffer with a V4L2
> > +                        * single-planar format, coalesce all planes. The length
> > +                        * and number of bytes used may only differ in the last
> > +                        * plane as any other situation can't be represented.
> > +                        */
> > +                       unsigned int bytesused = 0;
> > +                       unsigned int length = 0;
> > +
> > +                       for (auto [i, plane] : utils::enumerate(planes)) {
> > +                               bytesused += metadata.planes[i].bytesused;
> > +                               length += plane.length;
> > +
> > +                               if (i != planes.size() - 1 && bytesused != length) {
> > +                                       LOG(V4L2, Error)
> > +                                               << "Holes in multi-planar buffer not supported";
> > +                                       return -EINVAL;
> > +                               }
> > +                       }
> > +
> > +                       if (multiPlanar) {
> > +                               v4l2Planes[0].bytesused = bytesused;
> > +                               v4l2Planes[0].length = length;
> > +                       } else {
> > +                               buf.bytesused = bytesused;
> > +                               buf.length = length;
> > +                       }
> > +               } 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 (auto [i, plane] : utils::enumerate(planes)) {
> > +                               v4l2Planes[i].bytesused = metadata.planes[i].bytesused;
> > +                               v4l2Planes[i].length = plane.length;
> >                         }
> >                 } else {
> > -                       if (metadata.planes.size())
> > -                               buf.bytesused = metadata.planes[0].bytesused;
> > +                       /*
> > +                        * Single-planar API with a single plane in the buffer
> > +                        * is trivial to handle.
> > +                        */
> > +                       buf.bytesused = metadata.planes[0].bytesused;
> > +                       buf.length = planes[0].length;
> >                 }
> >
> >                 buf.sequence = metadata.sequence;

-- 
Regards,

Laurent Pinchart