[libcamera-devel] [RFC PATCH 08/10] libcamera: v4l2_videodevice: Create color-format planes in CreateBuffer()

[Hirokazu Honda]

Hi Laurent, thank you for reviewing.

On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> 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.

> >
> >               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()?

Best Regards,
-Hiro
> > +                     offset += planes[i].length;
> > +             }
> > +     }
> > +
> >       return std::make_unique<FrameBuffer>(std::move(planes));
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart