[libcamera-devel] [RFC PATCH v1 10/12] libcamera: v4l2_videodevice: Coalesce planes when queuing buffer
Hirokazu Honda
hiroh at chromium.org
Fri Sep 3 14:25:17 CEST 2021
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;
> >
More information about the libcamera-devel
mailing list