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

[Hirokazu Honda]

Hi Laurent,

On Mon, Sep 6, 2021 at 11:01 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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?

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