[libcamera-devel] [PATCH v3 7/9] 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.
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 26 15:03:01 CEST 2021
Hi Hiro,
Thank you for the patch.
On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> include/libcamera/internal/v4l2_videodevice.h | 10 +-
> src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++----
> test/v4l2_videodevice/buffer_cache.cpp | 6 +-
> 3 files changed, 123 insertions(+), 34 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index e767ec84..bfda6726 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);
> ~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_;
> };
> @@ -242,6 +245,7 @@ private:
> FrameBuffer *dequeueBuffer();
>
> V4L2Capability caps_;
> + V4L2DeviceFormat format_;
>
> enum v4l2_buf_type bufferType_;
> enum v4l2_memory memoryType_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 2ff25af2..f5f8741a 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"
>
> @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)
> /**
> * \brief Create an empty cache with \a numEntries entries
> * \param[in] numEntries Number of entries to reserve in the cache
> + * \param[in] numPlanes Number of V4L2 buffer planes
> *
> - * 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);
> }
> @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> /**
> * \brief Create a pre-populated cache
> * \param[in] buffers Array of buffers to pre-populated with
> + * \param[in] numPlanes Number of V4L2 buffer planes
> *
> - * 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 +243,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 +264,53 @@ 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;
> }
>
> 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) {
> + /*
> + * planes_ is V4L2 single-planar format buffer. fds of
> + * FrameBuffer::Plane must be identical and the sum of plane
> + * size is the V4L2 buffer length.
> + */
> + 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 {
> + /* planes_ is V4L2 multi-planar format buffer. */
> + 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;
Is all of this actually needed ? What's the issue if we store, in the
cache entries, the colour planes from FrameBuffer instance of the V4L2
buffer planes ?
> }
>
> @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()
> << "Opened device " << caps_.bus_info() << ": "
> << caps_.driver() << ": " << caps_.card();
>
> + ret = getFormat(&format_);
> + if (ret) {
> + LOG(V4L2, Error) << "Failed to get format";
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
> << "Opened device " << caps_.bus_info() << ": "
> << caps_.driver() << ": " << caps_.card();
>
> + ret = getFormat(&format_);
> + if (ret) {
> + LOG(V4L2, Error) << "Failed to get format";
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
> */
> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
> {
> + int ret = 0;
> if (caps_.isMeta())
> - return trySetFormatMeta(format, true);
> + ret = trySetFormatMeta(format, true);
> else if (caps_.isMultiplanar())
> - return trySetFormatMultiplane(format, true);
> + ret = trySetFormatMultiplane(format, true);
> else
> - return trySetFormatSingleplane(format, true);
> + ret = trySetFormatSingleplane(format, true);
> +
> + /* Cache the set format on success. */
> + if (ret == 0)
> + format_ = *format;
> +
> + return ret;
How about
if (ret)
return ret;
format_ = *format;
return 0;
We usually return early on error, and handle the normal case in the top
scope of the function.
> }
>
> int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> @@ -1152,8 +1207,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
> @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
> if (ret < 0)
> return ret;
>
> - cache_ = new V4L2BufferCache(*buffers);
> + cache_ = new V4L2BufferCache(*buffers, format_.planesCount);
> memoryType_ = V4L2_MEMORY_MMAP;
>
> return ret;
> @@ -1190,8 +1250,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
> @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> * \todo Set the right offset once V4L2 API provides a way.
> */
> plane.offset = 0;
> - plane.length = multiPlanar ?
> - buf.m.planes[nplane].length : buf.length;
> + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
>
> planes.push_back(std::move(plane));
> }
>
> + const auto &info = PixelFormatInfo::info(format_.fourcc);
> + if (info.isValid() && info.numPlanes() != numPlanes) {
> + 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;
> +
> + /* \todo Take the V4L2 stride into account */
> + 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));
> }
>
> @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)
> 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;
I'm afraid the test now fails :-S Could you have a look at that ?
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list