[libcamera-devel] [RFC PATCH v1 10/12] libcamera: v4l2_videodevice: Coalesce planes when queuing buffer

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Thu, Sep 2, 2021 at 8:25 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> On 02/09/2021 05:23, 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.
>
>
> Yikes ;-) lets see...
>
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 68 +++++++++++++++++++++++++-----
> >  1 file changed, 58 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 2d9a94c3c974..79cb792117d5 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"
> >
> > @@ -1468,10 +1470,21 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >
> >       bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> >       const std::vector<FrameBuffer::Plane> &planes = buffer->planes();
> > +     unsigned int numV4l2Planes = format_.fourcc == formatInfo_->v4l2Format[0]
> > +                                ? 1 : planes.size();

nit const unsigned int.
I would replace formatInfo->numPlanes() with planes.size().

>
> Aha, ok so that's how we recover the planar/multiplanar information I
> was concerned we had lost earlier.
>
> I wonder if we should set/cache numV4l2Planes_ when we set formatInfo_
> as they are so closely paired .. But it's a trivial operation here.
>
>
>
> > +
> > +     /*
> > +      * 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;
> > +     }
> >
> >       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();
> > @@ -1479,23 +1492,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) {
>
> Ah, i almost thought that was wrong, but it's
>    "If we're not the last plane, and the plane is not fully used..."
>
> So it's fine.
>
>
> > +                                     LOG(V4L2, Error)
> > +                                             << "Holes in multi-planar buffer not supported";
> > +                                     return -EINVAL;
> > +                             }
> > +                     }
> > +
> > +                     if (multiPlanar) {
> > +                             v4l2Planes[0].bytesused = bytesused;
> > +                             v4l2Planes[0].length = length;

I think multiPlanar must be false here.

v4l2 format | plane.size() | numV4L2Planes |     numV4L2Planes ==
planes.size()
multi          |         1        |      planes.size() |          true
multi          |         2+      |      planes.size() |           true
single        |         1        |       1                 |           true
single        |         2+        |       1                 |           false

So I would implement as follows.
if (multiPlanar) {
   ...
} else {
  if (numV4l2Planes != planes.size()) {
    ...
  } else {
     ...
  }
}


> > +                     } 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())
>
> Guaranteeing the planes are there is much nicer here.
>
> Phew, this patch 'looks' a lot more complex than it actually is,
>
> So I think ... this is fine.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > -                             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;
> >