[libcamera-devel] [RFC PATCH v2 08/10] libcamera: v4l2_videodevice: Create color-format planes in createBuffer()
Hirokazu Honda
hiroh at chromium.org
Wed Aug 25 09:13:21 CEST 2021
Hi Laurent, thank you for reviewing.
On Tue, Aug 24, 2021 at 9:27 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> One more comment.
>
> On Tue, Aug 24, 2021 at 02:54:56AM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 23, 2021 at 10:12:19PM +0900, Hirokazu Honda wrote:
> > > V4L2VideDevice::createBuffer() creates the same number of
> > > 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 | 9 +-
> > > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++----
> > > test/v4l2_videodevice/buffer_cache.cpp | 6 +-
> > > 3 files changed, 128 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index e767ec84..69bb964a 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {
> > > class V4L2BufferCache
> > > {
> > > public:
> > > - V4L2BufferCache(unsigned int numEntries);
> > > - V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
> > > + V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);
> > > + V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > > + unsigned int numPlanes);
>
> I'm getting the following doxygen warnings:
>
> include/libcamera/internal/v4l2_videodevice.h:117: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) is not documented:
> parameter 'numPlanes'
> include/libcamera/internal/v4l2_videodevice.h:118: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(const std::vector< std::unique_ptr< FrameBuffer > > &buffers, unsigned int numPlanes) is not documented:
> parameter 'numPlanes'
>
> > > ~V4L2BufferCache();
> > >
> > > int get(const FrameBuffer &buffer);
> > > @@ -126,7 +127,8 @@ private:
> > > {
> > > public:
> > > Entry();
> > > - Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);
> > > + Entry(bool free, uint64_t lastUsed, unsigned int numPlanes,
> > > + const FrameBuffer &buffer);
> > >
> > > bool operator==(const FrameBuffer &buffer) const;
> > >
> > > @@ -149,6 +151,7 @@ private:
> > >
> > > std::atomic<uint64_t> lastUsedCounter_;
> > > std::vector<Entry> cache_;
> > > + unsigned int numPlanes_;
> > > /* \todo Expose the miss counter through an instrumentation API. */
> > > unsigned int missCounter_;
> > > };
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index ce60dff6..42b66bd3 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"
> > >
> > > @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2)
> > > * \brief Create an empty cache with \a numEntries entries
> > > * \param[in] numEntries Number of entries to reserve in the cache
> > > *
> > > - * Create a cache with \a numEntries entries all marked as unused. The entries
> > > - * will be populated as the cache is used. This is typically used to implement
> > > - * buffer import, with buffers added to the cache as they are queued.
> > > + * Create a cache with \a numEntries entries all marked as unused. The entry is
> > > + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the
> > > + * cache is used. This is typically used to implement buffer import, with
> > > + * buffers added to the cache as they are queued.
> > > */
> > > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> > > - : lastUsedCounter_(1), missCounter_(0)
> > > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)
> > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
> > > {
> > > cache_.resize(numEntries);
> > > }
> > > @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> > > * \brief Create a pre-populated cache
> > > * \param[in] buffers Array of buffers to pre-populated with
> > > *
> > > - * Create a cache pre-populated with \a buffers. This is typically used to
> > > - * implement buffer export, with all buffers added to the cache when they are
> > > - * allocated.
> > > + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes
> > > + * planes V4L2 buffer. This is typically used to implement buffer export, with
> > > + * all buffers added to the cache when they are allocated.
> > > */
> > > -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> > > - : lastUsedCounter_(1), missCounter_(0)
> > > +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > > + unsigned int numPlanes)
> > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
> > > {
> > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> > > cache_.emplace_back(true,
> > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> > > + numPlanes_,
> > > *buffer.get());
> > > }
> > >
> > > @@ -237,6 +241,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> > >
> > > cache_[use] = Entry(false,
> > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> > > + numPlanes_,
> > > buffer);
> > >
> > > return use;
> > > @@ -257,24 +262,47 @@ V4L2BufferCache::Entry::Entry()
> > > {
> > > }
> > >
> > > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
> > > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,
> > > + unsigned int numPlanes, const FrameBuffer &buffer)
> > > : free_(free), lastUsed_(lastUsed)
> > > {
> > > - for (const FrameBuffer::Plane &plane : buffer.planes())
> > > - planes_.emplace_back(plane);
> > > + ASSERT(numPlanes <= buffer.planes().size());
> > > + unsigned int length = 0;
> > > + for (const FrameBuffer::Plane &plane : buffer.planes()) {
> > > + ASSERT(plane.offset == length);
> > > + length += plane.length;
> > > +
> > > + if (planes_.size() < numPlanes)
> > > + planes_.emplace_back(plane);
> > > + }
> > > +
> > > + if (numPlanes == 1) {
> > > + planes_[0].length = length;
> > > + }
> >
> > No need for curly braces.
> >
> > > }
> > >
> > > bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
> > > {
> > > const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
> > >
> > > - if (planes_.size() != planes.size())
> > > + if (planes_.size() != planes.size() || planes_.size() == 1)
> > > return false;
> > > -
> > > - for (unsigned int i = 0; i < planes.size(); i++)
> > > - if (planes_[i].fd != planes[i].fd.fd() ||
> > > - planes_[i].length != planes[i].length)
> > > + if (planes_.size() == 1) {
> > > + unsigned int length = 0;
> > > + for (unsigned int i = 0; i < planes.size(); i++) {
> > > + if (planes_[0].fd != planes[i].fd.fd())
> > > + return false;
> > > + length += planes[i].length;
> > > + }
> > > + if (length != planes_[0].length)
> > > return false;
> > > + } else {
> > > + for (unsigned int i = 0; i < planes.size(); i++)
> > > + if (planes_[i].fd != planes[i].fd.fd() ||
> > > + planes_[i].length != planes[i].length)
> > > + return false;
> > > + }
> > > +
> > > return true;
> > > }
> >
> > I'm not sure to understand why all this is needed.
> >
I will add a comment to the next patch series.
> > >
> > > @@ -1152,8 +1180,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
> > > * successful return the driver's internal buffer management is initialized in
> > > * MMAP mode, and the video device is ready to accept queueBuffer() calls.
> > > *
> > > - * 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().
> > > + * 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.
> > > *
> > > * Buffers allocated with this function shall later be free with
> > > * releaseBuffers(). If buffers have already been allocated with
> > > @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
> > > int V4L2VideoDevice::allocateBuffers(unsigned int count,
> > > std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > {
> > > - int ret = createBuffers(count, buffers);
> > > + V4L2DeviceFormat format{};
> > > + int ret = getFormat(&format);
> > > + if (ret < 0) {
> > > + LOG(V4L2, Error)
> > > + << "Failed to get format: " << strerror(-ret);
> > > + return ret;
> > > + }
> >
> > Instead of calling getFormat() here and in two places below, how about
> > caching the format in setFormat() ?
> >
> > > +
> > > + ret = createBuffers(count, buffers);
> > > if (ret < 0)
> > > return ret;
> > >
> > > - cache_ = new V4L2BufferCache(*buffers);
> > > + cache_ = new V4L2BufferCache(*buffers, format.planesCount);
> > > memoryType_ = V4L2_MEMORY_MMAP;
> > >
> > > return ret;
> > > @@ -1190,8 +1231,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
> > > * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
> > > * importers.
> > > *
> > > - * 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().
> > > + * 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.
> > > *
> > > * Multiple independent sets of buffers can be allocated with multiple calls to
> > > * this function. Device-specific limitations may apply regarding the minimum
> > > @@ -1283,12 +1329,49 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > >
> > > FrameBuffer::Plane plane;
> > > plane.fd = std::move(fd);
> > > - plane.length = multiPlanar ?
> > > - buf.m.planes[nplane].length : buf.length;
> > > + /*
> > > + * V4L2 API doesn't provide dmabuf offset information of plane.
> > > + * Set 0 as a placeholder offset.
> > > + * \todo Set the right offset once V4L2 API provides a way.
> > > + */
> > > + plane.offset = 0;
> > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
> > >
> > > planes.push_back(std::move(plane));
> > > }
> > >
> > > + /*
> > > + * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar
> > > + * format, overwrite FrameBuffer::Plane to make it color format planes.
> > > + * */
> > > + V4L2DeviceFormat format{};
> > > + ret = getFormat(&format);
> > > + if (ret < 0) {
> > > + LOG(V4L2, Error)
> > > + << "Failed to get format: " << strerror(-ret);
> > > + return nullptr;
> > > + }
> > > +
> > > + const auto &info = PixelFormatInfo::info(format.fourcc);
> > > + if (info.isValid() && info.numPlanes() != numPlanes) {
> >
> > If info isn't valid, should we return nullptr ?
createBuffer() is called for non pixel format buffer for example,
/dev/video10[33:out]: Invalid format: 0x0-ip3p
I need to check by info.isValid() to filter out non pixel formats.
-Hiro
> >
> > > + ASSERT(numPlanes == 1u);
> > > + const size_t numColorPlanes = info.numPlanes();
> > > + planes.resize(numColorPlanes);
> > > + const FileDescriptor &fd = planes[0].fd;
> > > + size_t offset = 0;
> > > + for (size_t i = 0; i < numColorPlanes; ++i) {
> > > + planes[i].fd = fd;
> > > + planes[i].offset = offset;
> > > +
> >
> > Could you please add a
> >
> > /* \todo Take the V4L2 stride into account */
> >
> > comment ? No need to address this now, but it will need to be done at
> > some point.
> >
> > > + const unsigned int vertSubSample =
> > > + info.planes[i].verticalSubSampling;
> > > + planes[i].length =
> > > + info.stride(format.size.width, i, 1u) *
> > > + ((format.size.height + vertSubSample - 1) / vertSubSample);
> > > + offset += planes[i].length;
> > > + }
> > > + }
> > > +
> > > return std::make_unique<FrameBuffer>(std::move(planes));
> > > }
> > >
> > > @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count)
> > >
> > > memoryType_ = V4L2_MEMORY_DMABUF;
> > >
> > > - int ret = requestBuffers(count, V4L2_MEMORY_DMABUF);
> > > + V4L2DeviceFormat format{};
> > > + int ret = getFormat(&format);
> > > + if (ret < 0) {
> > > + LOG(V4L2, Error)
> > > + << "Failed to get format: " << strerror(-ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = requestBuffers(count, V4L2_MEMORY_DMABUF);
> > > if (ret)
> > > return ret;
> > >
> > > - cache_ = new V4L2BufferCache(count);
> > > + cache_ = new V4L2BufferCache(count, format.planesCount);
> > >
> > > LOG(V4L2, Debug) << "Prepared to import " << count << " buffers";
> > >
> > > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> > > index b3f2bec1..48f748bc 100644
> > > --- a/test/v4l2_videodevice/buffer_cache.cpp
> > > +++ b/test/v4l2_videodevice/buffer_cache.cpp
> > > @@ -166,7 +166,7 @@ public:
> > > * Test cache of same size as there are buffers, the cache is
> > > * created from a list of buffers and will be pre-populated.
> > > */
> > > - V4L2BufferCache cacheFromBuffers(buffers);
> > > + V4L2BufferCache cacheFromBuffers(buffers, 1u);
> > >
> > > if (testSequential(&cacheFromBuffers, buffers) != TestPass)
> > > return TestFail;
> > > @@ -181,7 +181,7 @@ public:
> > > * Test cache of same size as there are buffers, the cache is
> > > * not pre-populated.
> > > */
> > > - V4L2BufferCache cacheFromNumbers(numBuffers);
> > > + V4L2BufferCache cacheFromNumbers(numBuffers, 1u);
> > >
> > > if (testSequential(&cacheFromNumbers, buffers) != TestPass)
> > > return TestFail;
> > > @@ -196,7 +196,7 @@ public:
> > > * Test cache half the size of number of buffers used, the cache
> > > * is not pre-populated.
> > > */
> > > - V4L2BufferCache cacheHalf(numBuffers / 2);
> > > + V4L2BufferCache cacheHalf(numBuffers / 2, 1u);
> > >
> > > if (testRandom(&cacheHalf, buffers) != TestPass)
> > > return TestFail;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list