[libcamera-devel] [RFC PATCH 08/10] libcamera: v4l2_videodevice: Create color-format planes in CreateBuffer()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 20 19:34:44 CEST 2021
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 :-)
> > >
> > > 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].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