[libcamera-devel] [RFC PATCH v1 10/12] libcamera: v4l2_videodevice: Coalesce planes when queuing buffer
Hirokazu Honda
hiroh at chromium.org
Fri Sep 3 16:46:46 CEST 2021
Hi Laurent,
On Fri, Sep 3, 2021 at 11:34 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Sep 03, 2021 at 09:25:17PM +0900, Hirokazu Honda wrote:
> > On Thu, Sep 2, 2021 at 8:25 PM Kieran Bingham 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.
>
> Agreed.
>
> > I would replace formatInfo->numPlanes() with planes.size().
>
This is the opposite. replace planes.size() with formatinfo_->numPlanes().
> Where ?
>
> > > 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
>
> multiPlanar indicates whether or not the driver uses the V4L2
> multi-planar API, that is, if it stores buffer data in buf.length and
> buf.bytesused, or in buf.m.planes[].length and buf.m.planes[].bytesused.
> Formats that use multiple V4L2 buffer planes (such as
> V4L2_PIX_FMT_NV12M) require the multi-planar API, but formats that use a
> single V4L2 buffer plane (such as V4L2_PIX_FMT_NV12) can use either.
>
> We are here under the context of the condition
>
> if (numV4l2Planes != planes.size())
>
> numV4l2Planes stores the number of V4L2 buffer planes, so for
> V4L2_PIX_FMT_NV12, it will be 1. planes.size(), for V4L2_PIX_FMT_NV12,
> will be 2. We can thus reach this point with V4L2_PIX_FMT_NV12, and that
> format can be used with both the single-planar API and the multi-planar
> API.
Ah, I got it.
Thanks
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
-Hiro
>
> > 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;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list