[libcamera-devel] [RFC PATCH 08/10] libcamera: v4l2_videodevice: Create color-format planes in CreateBuffer()
Tomasz Figa
tfiga at chromium.org
Thu Aug 26 07:14:45 CEST 2021
On Sat, Aug 21, 2021 at 2:34 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Aug 20, 2021 at 07:32:54PM +0900, Hirokazu Honda wrote:
> > On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart wrote:
> > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote:
> > > > V4L2VideDevice::CreateBuffer() creates the same number of
> > >
> > > s/CreateBuffer/createBuffer/
> > >
> > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2
> > > > format single is single-planar format, the created number of
> > > > FrameBuffer::Planes is 1.
> > > > It should rather create the same number of FrameBuffer::Planes as the
> > > > color format planes.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > > include/libcamera/internal/v4l2_videodevice.h | 4 ++-
> > > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++--
> > > > 2 files changed, 29 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > > index e767ec84..d0aeb614 100644
> > > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > > @@ -136,11 +136,13 @@ private:
> > > > private:
> > > > struct Plane {
> > > > Plane(const FrameBuffer::Plane &plane)
> > > > - : fd(plane.fd.fd()), length(plane.length)
> > > > + : fd(plane.fd.fd()), offset(plane.offset),
> > > > + length(plane.length)
> > > > {
> > > > }
> > > >
> > > > int fd;
> > > > + unsigned int offset;
> > > > unsigned int length;
> > > > };
> > > >
> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > index ce60dff6..549418c8 100644
> > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > @@ -25,6 +25,7 @@
> > > >
> > > > #include <libcamera/file_descriptor.h>
> > > >
> > > > +#include "libcamera/internal/formats.h"
> > > > #include "libcamera/internal/media_device.h"
> > > > #include "libcamera/internal/media_object.h"
> > > >
> > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
> > > >
> > > > for (unsigned int i = 0; i < planes.size(); i++)
> > > > if (planes_[i].fd != planes[i].fd.fd() ||
> > > > + planes_[i].offset != planes_[i].offset ||
> > > > planes_[i].length != planes[i].length)
> > > > return false;
> > > > return true;
> > > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > >
> > > It would be good to capture the fact that the FrameBuffer planes now
> > > match the colour planes and not the V4L2 planes. This function is
> > > private and isn't documented, so we can't handle it here, and should
> > > instead document this in V4L2VideoDevice::allocateBuffers() and
> > > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that
> > > state
> > >
> > > * The number of planes and the plane sizes for the allocation are determined
> > > * by the currently active format on the device as set by setFormat().
> > >
> > > It could be extended to
> > >
> > > * The number of planes and their offsets and sizes are determined by the
> > > * currently active format on the device as set by setFormat(). They do not map
> > > * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> > > * instance, if the active format is formats::NV12, the allocated FrameBuffer
> > > * instances will have two planes, for the luma and chroma components,
> > > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> > > * V4L2_PIX_FMT_NV12M.
> > >
> > > >
> > > > FrameBuffer::Plane plane;
> > > > plane.fd = std::move(fd);
> > > > - plane.length = multiPlanar ?
> > > > - buf.m.planes[nplane].length : buf.length;
> > > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0;
> > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
> > >
> > > The data_offset field defined by V4L2 is, unfortunately, not the offset
> > > in the dmabuf, but the offset of the data from the start of the plane.
> > > It's used by codec drivers to indicate the size of the header stored at
> > > the beginning of the plane. The data_offset is actually contained in the
> > > length.
> > >
> > > I think we can ignore the data offset reported by V4L2, until the V4L2
> > > API is expanded with a "real" data offset. plane.offset can be hardcoded
> > > to 0. The change to operator== could also probably be dropped.
> >
> > Yeah, I have faced the problem in developing chromium video accelerated codecs.
> > Although I think setting offset to 0 is not correct either.. but no
> > way of avoiding problem here.
>
> Let's revisit this part of the code once V4L2 gets proper support for
> offsets :-)
>
We'll probably have to carry a downstream patch to handle them
initially... Would it be possible to at least make it easy to do that
until proper V4L2 support gets merged?
(v4l2_ext API is being worked on, but last time I checked it was stuck
in review, without any comments...)
> > > >
> > > > planes.push_back(std::move(plane));
> > > > }
> > > >
> > > > + V4L2DeviceFormat format{};
> > > > + ret = getFormat(&format);
> > > > + if (ret < 0) {
> > > > + LOG(V4L2, Error)
> > > > + << "Failed to get format: " << strerror(-ret);
> > > > + return nullptr;
> > > > + }
> > > > +
> > >
> > > A comment to explain what's going on would be useful.
> > >
> > > > + const auto &info = PixelFormatInfo::info(format.fourcc);
> > > > + if (info.isValid() && info.numPlanes() != numPlanes) {
> > >
> > > If info isn't valid, should we return nullptr ?
> > >
> > > > + ASSERT(numPlanes == 1u);
> > > > + planes.resize(info.numPlanes());
> > > > + const FileDescriptor &fd = planes[0].fd;
> > > > + size_t offset = 0;
> > > > + for (size_t i = 0; i < info.numPlanes(); ++i) {
> > > > + planes[i].fd = fd;
> > > > + planes[i].offset = offset;
> > > > + planes[i].length = info.frameSize(format.size);
> > >
> > > Isn't it the plane size that we need here, not the full frame size ?
> > > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format,
> > > will return the total size of the image, including both colour planes.
> > >
> > > There's also one change missing. You're addressing the buffer allocation
> > > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has
> > > two planes, we need to translate from two colour planes to one V4L2
> > > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used.
> >
> > I am confused thanks to various "Plane" structures.
> > In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it?
> > FrameMetadata::Plane is used to fill other v4l2_buffer variables.
> > What shall I fix in queueBuffer()?
>
> A FrameBuffer containing V4L2_PIX_FMT_NV12 should have two colour
> planes, with the same dmabuf for both:
>
> planes[0].fd = dmabuf;
> planes[0].offset = 0;
> planes[0].length = stride * height;
> planes[1].fd = dmabuf;
> planes[1].offset = stride * height;
> planes[1].length = stride * height / 2;
>
> This is because V4L2_PIX_FMT_NV12 is contiguous in memory, unlike
> V4L2_PIX_FMT_NV12M. When queing such a buffer to the V4L2VideoDevice,
> V4L2 will expect a single buffer plane, with
>
> v4l2Planes[0].m.fd = dmabuf;
>
> Additionally, for output buffers, bytesused and length also need to be
> set, but we may not need to handle that right away.
>
> V4L2VideoDevice thus needs, for V4L2_PIX_FMT_NV12, to translate from the
> two colour planes in the FrameBuffer to the single V4L2 buffer plane.
> The translation is fairly easy, as it only requires reducing the number
> of planes (but the number of V4L2 buffer planes for V4L2_PIX_FMT_NV12
> isn't available anywhere, so we will need to add this information
> somewhere). We however needs to verify that the FrameBuffer is suitable
> for V4L2, and check that
>
> - planes[0].fd == planes[1].fd
planes[0].fd != planes[1].fd doesn't always imply that they refer to a
different DMA-buf. I'd skip this check (or just emit a warning) and
rely on checking whether the offset matches.
Generally the two common patterns in V4L2 are:
1) single plane with 1 DMA-buf for all color planes
2) multiplane with 1 DMA-buf for _each_ color plane, with zero offset
within each
I haven't seen any other case in practice, so it should be safe to
simply check based on the offset. The ext API will solve this by
moving the DMA-buf and offset check to the kernel.
Best regards,
Tomasz
> - planes[0].offset == 0
> - planes[1].offset == planes[0].length
>
> and possibly also check the length. If the buffer isn't suitable,
> V4L2VideoDevice::queueBuffer() should return an error.
>
> > > > + offset += planes[i].length;
> > > > + }
> > > > + }
> > > > +
> > > > return std::make_unique<FrameBuffer>(std::move(planes));
> > > > }
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list