[libcamera-devel] [RFC PATCH 08/10] libcamera: v4l2_videodevice: Create color-format planes in CreateBuffer()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 26 13:08:46 CEST 2021
Hi Tomasz,
On Thu, Aug 26, 2021 at 02:14:45PM +0900, Tomasz Figa wrote:
> On Sat, Aug 21, 2021 at 2:34 AM Laurent Pinchart wrote:
> > 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...)
If there's significant progress with this development we can carry a
downstream patch to avoid waiting for the API to be merged upstream, but
it indeed seems stuck. What would be your use case for such a downstream
patch though ? We would need drivers using the API to make it useful in
libcamera.
> > > > >
> > > > > 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.
We can check the inode number with fstat() (I've tested that by dupping
a dmabuf fd, and I get the same inode). It's probably overkill for now,
but will likely be done at some point. The reason why I'd like this
check to stay is to catch userspace that would give use different fds
(whether or not they reference the same dmabuf), and then decide how to
handle them.
> 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'd say that more than this being a common pattern, it's the only thing
that V4L2 supports upstream :-)
> 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.
>
> > - 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