[libcamera-devel] [PATCH v2 12/25] libcamera: v4l2_videodevice: Add FrameBuffer interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 8 02:01:44 CET 2020
Hi Niklas,
Thank you for the patch.
On Mon, Dec 30, 2019 at 01:04:57PM +0100, Niklas Söderlund wrote:
> Add a new interface in parallel with the existing Buffer implementation
> to also support FrameBuffer. The reason it's added in parallel is to aid
> in the migration from Buffer to FrameBuffer thru out libcamera. With this
s/thru out/throughout/
> change discrete parts of libcamera can be migrated and tested
> independently.
>
> As the new interface is added in parallel there are some oddities in
> this change which will be undone in a follow up patch once libcamera
> have migrated away from the Buffer interface.
>
> - There is a nasty hack in V4L2VideoDevice::bufferAvailable(). It is
> needed to allow both interfaces to exist and function at the same
> time. The idea is if buffers are allocated using the FrameBuffer
> interface V4L2VideoDevice::cache_ is set and we know to call the
> FrameBuffer 'buffer ready' signal, and likewise if it's not to call
> the Buffer variant.
>
> - There is some code duplication between the two interfaces as they aim
> to solve the same thing in slightly different ways. As all Buffer
> related code is soon to be removed no effort to create code sharing
> between them have been made.
>
> - Some function and variables which can't be distinguished by their
> argument types have been given a frameBuffer prefix instead of a
> buffer prefix. They are clearly documented in the code and will be
> renamed to the correct buffer prefix when the Buffer interface is
> removed.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Rename allocateBuffers() to exportBuffers()
> - Rename externalBuffers() to importBuffers()
> - Rename createBuffer() to createFrameBuffer()
> - Reworked createFrameBuffer() to take advantage of the FileDescriptor()
> - Fix up a lot of documentation and small code beautification fixes.
> - Merged all FrameBuffer interface additions into this patch, some where
> done in the really huge 27/30 patch.
> ---
> src/libcamera/include/v4l2_videodevice.h | 12 +
> src/libcamera/v4l2_videodevice.cpp | 312 ++++++++++++++++++++++-
> 2 files changed, 313 insertions(+), 11 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 9fc2082b2e7f7219..97a79e56c647e7a6 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -196,11 +196,17 @@ public:
>
> int exportBuffers(BufferPool *pool);
> int importBuffers(BufferPool *pool);
> + int exportBuffers(unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> + int importBuffers(unsigned int count);
> int releaseBuffers();
>
> int queueBuffer(Buffer *buffer);
> std::vector<std::unique_ptr<Buffer>> queueAllBuffers();
> Signal<Buffer *> bufferReady;
> + int queueBuffer(FrameBuffer *buffer);
> + /* todo: Rename to bufferReady when the Buffer version is removed */
s/todo:/todo/
here and below.
> + Signal<FrameBuffer *> frameBufferReady;
>
> int streamOn();
> int streamOff();
> @@ -231,10 +237,13 @@ private:
> int requestBuffers(unsigned int count);
> int createPlane(BufferMemory *buffer, unsigned int index,
> unsigned int plane, unsigned int length);
> + std::unique_ptr<FrameBuffer> createFrameBuffer(const struct v4l2_buffer &buf);
> int exportDmabufFd(unsigned int index, unsigned int plane);
>
> Buffer *dequeueBuffer();
> void bufferAvailable(EventNotifier *notifier);
> + /* todo: Rename to dequeueBuffer() when the Buffer version is removed */
> + FrameBuffer *dequeueFrameBuffer();
>
> V4L2Capability caps_;
>
> @@ -242,7 +251,10 @@ private:
> enum v4l2_memory memoryType_;
>
> BufferPool *bufferPool_;
> + V4L2BufferCache *cache_;
> std::map<unsigned int, Buffer *> queuedBuffers_;
> + /* todo: Rename to queuedBuffers_ when the Buffer version is removed */
> + std::map<unsigned int, FrameBuffer *> queuedFrameBuffers_;
>
> EventNotifier *fdEvent_;
> };
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index c9e17c44bfe17c3b..b380f08c1e880427 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -370,7 +370,8 @@ const std::string V4L2DeviceFormat::toString() const
> * \param[in] deviceNode The file-system path to the video device node
> */
> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> - : V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)
> + : V4L2Device(deviceNode), bufferPool_(nullptr), cache_(nullptr),
> + fdEvent_(nullptr)
> {
> /*
> * We default to an MMAP based CAPTURE video device, however this will
> @@ -1038,6 +1039,95 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool)
> return 0;
> }
>
> +/**
> + * \brief Allocate buffers from the video device
> + * \param[in] count Number of buffers to allocate
> + * \param[out] buffers Vector to store allocated buffers
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::exportBuffers(unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> + if (cache_) {
> + LOG(V4L2, Error) << "FrameBuffers already allocated";
We don't pluralize capitalized class names. This should be "Buffers",
"Framebuffers" or "FrameBuffer instances".
> + return -EINVAL;
> + }
> +
> + memoryType_ = V4L2_MEMORY_MMAP;
> +
> + int ret = requestBuffers(count);
> + if (ret < 0)
> + return ret;
> +
> + for (unsigned i = 0; i < count; ++i) {
> + struct v4l2_buffer buf = {};
> + struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> +
> + buf.index = i;
> + buf.type = bufferType_;
> + buf.memory = memoryType_;
> + buf.length = ARRAY_SIZE(planes);
> + buf.m.planes = planes;
> +
> + ret = ioctl(VIDIOC_QUERYBUF, &buf);
> + if (ret < 0) {
> + LOG(V4L2, Error)
> + << "Unable to query buffer " << i << ": "
> + << strerror(-ret);
> + goto err_buf;
> + }
> +
> + std::unique_ptr<FrameBuffer> buffer = createFrameBuffer(buf);
> + if (!buffer) {
> + LOG(V4L2, Error) << "Unable to create buffer";
> + ret = -EINVAL;
> + goto err_buf;
> + }
> +
> + buffers->push_back(std::move(buffer));
> + }
> +
> + cache_ = new V4L2BufferCache(*buffers);
> +
> + return count;
> +
> +err_buf:
> + requestBuffers(0);
> +
> + buffers->clear();
> +
> + return ret;
> +}
> +
> +std::unique_ptr<FrameBuffer> V4L2VideoDevice::createFrameBuffer(const struct v4l2_buffer &buf)
Maybe a line break after the return type ?
> +{
> + const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> + const unsigned int numPlanes = multiPlanar ? buf.length : 1;
> +
> + if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
> + LOG(V4L2, Error) << "Invalid number of planes";
> + return nullptr;
> + }
> +
> + std::vector<FrameBuffer::Plane> planes;
> + for (unsigned int nplane = 0; nplane < numPlanes; nplane++) {
> + int fd = exportDmabufFd(buf.index, nplane);
I think exportDmabufFd() should return a FileDescriptor, but that's out
of scope for this patch. We can perform additional optimizations on top
of this series.
There's an issue with returning FileDescriptor by value from
exportDmabufFd() (yes, I had to try it of course...): the copy
constructor is marked as explicit, and return performs an implicit copy.
With RVO the copy will be elided in practice, but the copy constructor
still have to be invocable, and there seems to be no syntax to
explicitly call the copy constructor in a return statement.
One option is to remove the explicit keyword from the copy constructor,
but the compiler will then not catch unwanted copies, leading to
expensive operations that are hard to catch during review. Another
option would be to turn FileDescriptor into a shared_ptr wrapping the
fd, allowing for cheap copies (which should still be avoided if
possible, but if one slips through it won't be a too big issue) while
still retaining RAII semantics. What's your opinion on this ?
> + if (fd < 0)
> + return nullptr;
> +
> + FrameBuffer::Plane plane;
> + plane.fd = FileDescriptor(fd);
> + plane.length = multiPlanar ?
> + buf.m.planes[nplane].length : buf.length;
> +
> + planes.push_back(plane);
std::move(plane) to avoid a copy ? Copying FileDescriptor is pretty
expensive.
> +
> + ::close(fd);
> + }
> +
> + return utils::make_unique<FrameBuffer>(planes);
Same here, std::move(planes).
> +}
> +
> int V4L2VideoDevice::exportDmabufFd(unsigned int index, unsigned int plane)
> {
> struct v4l2_exportbuffer expbuf = {};
> @@ -1058,14 +1148,41 @@ int V4L2VideoDevice::exportDmabufFd(unsigned int index, unsigned int plane)
> return expbuf.fd;
> }
>
> +/**
> + * \brief Prepare the device to import \a count buffers
> + * \param[in] count Number of buffers to prepare to import
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::importBuffers(unsigned int count)
> +{
> + if (cache_) {
> + LOG(V4L2, Error) << "FrameBuffers already allocated";
Same comment as above.
> + return -EINVAL;
> + }
> +
> + memoryType_ = V4L2_MEMORY_DMABUF;
> +
> + int ret = requestBuffers(count);
> + if (ret)
> + return ret;
> +
> + cache_ = new V4L2BufferCache(count);
> +
> + LOG(V4L2, Debug) << "Prepared to import " << count << " buffers";
> +
> + return 0;
> +}
> +
> /**
> * \brief Release all internally allocated buffers
> */
> int V4L2VideoDevice::releaseBuffers()
> {
> - LOG(V4L2, Debug) << "Releasing bufferPool";
> + LOG(V4L2, Debug) << "Releasing buffers";
>
> bufferPool_ = nullptr;
> + delete cache_;
> + cache_ = nullptr;
>
> return requestBuffers(0);
> }
> @@ -1079,6 +1196,8 @@ int V4L2VideoDevice::releaseBuffers()
> * will be processed by the device. Once the device has finished processing the
> * buffer, it will be available for dequeue.
> *
> + * The best available V4L2 buffer is picked for \a buffer using the V4L2 cache.
> + *
s/V4L2 cache/V4L2 buffer cache/
Doesn't this belong to the documentation of
queueBuffer(FrameBuffer *buffer) below ?
> * \return 0 on success or a negative error code otherwise
> */
> int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> @@ -1183,6 +1302,84 @@ std::vector<std::unique_ptr<Buffer>> V4L2VideoDevice::queueAllBuffers()
> return buffers;
> }
>
> +/**
> + * \brief Queue a buffer into the video device
s/into/to/
> + * \param[in] buffer The buffer to be queued
> + *
> + * For capture video devices the \a buffer will be filled with data by the
> + * device. For output video devices the \a buffer shall contain valid data and
> + * will be processed by the device. Once the device has finished processing the
> + * buffer, it will be available for dequeue.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> +{
> + struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> + struct v4l2_buffer buf = {};
> + int ret;
> +
> + ret = cache_->get(*buffer);
> + if (ret < 0)
> + return ret;
> +
> + buf.index = ret;
> + buf.type = bufferType_;
> + buf.memory = memoryType_;
> + buf.field = V4L2_FIELD_NONE;
> +
> + bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> + const std::vector<FrameBuffer::Plane> &planes = buffer->planes();
> +
> + if (buf.memory == V4L2_MEMORY_DMABUF) {
> + if (multiPlanar) {
> + for (unsigned int p = 0; p < planes.size(); ++p)
> + v4l2Planes[p].m.fd = planes[p].fd.fd();
> + } else {
> + buf.m.fd = planes[0].fd.fd();
> + }
> + }
> +
> + if (multiPlanar) {
> + buf.length = planes.size();
> + buf.m.planes = v4l2Planes;
> + }
> +
> + if (V4L2_TYPE_IS_OUTPUT(buf.type)) {
> + const FrameMetadata &metadata = buffer->metadata();
> +
> + if (multiPlanar) {
> + unsigned int nplane = 0;
> + for (const FrameMetadata::Plane &plane : metadata.planes)
> + v4l2Planes[nplane++].bytesused = plane.bytesused;
> + } else {
> + if (metadata.planes.size())
> + buf.bytesused = metadata.planes[0].bytesused;
> + }
> +
> + buf.sequence = metadata.sequence;
> + buf.timestamp.tv_sec = metadata.timestamp / 1000000000;
> + buf.timestamp.tv_usec = (metadata.timestamp / 1000) % 1000000;
> + }
> +
> + LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> +
> + ret = ioctl(VIDIOC_QBUF, &buf);
> + if (ret < 0) {
> + LOG(V4L2, Error)
> + << "Failed to queue buffer " << buf.index << ": "
> + << strerror(-ret);
> + return ret;
> + }
> +
> + if (queuedFrameBuffers_.empty())
> + fdEvent_->setEnabled(true);
> +
> + queuedFrameBuffers_[buf.index] = buffer;
> +
> + return 0;
> +}
> +
> /**
> * \brief Dequeue the next available buffer from the video device
> *
> @@ -1242,17 +1439,97 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> * When this slot is called, a Buffer has become available from the device, and
> * will be emitted through the bufferReady Signal.
> *
> - * For Capture video devices the Buffer will contain valid data.
> - * For Output video devices the Buffer can be considered empty.
> + * For Capture video devices the FrameBuffer will contain valid data.
> + * For Output video devices the FrameBuffer can be considered empty.
> */
> void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
> {
> - Buffer *buffer = dequeueBuffer();
> - if (!buffer)
> - return;
> + /**
I don't think this needs to be part of the generated documentation, /*
should be enough.
> + * This is a hack which allows both Buffer and FrameBuffer interfaces
> + * to work with the same code base. This allows different parts of
> + * libcamera to migrate to FrameBuffer in different patches.
> + *
> + * If we have a cache_ we know FrameBuffer is in use.
> + *
> + * \todo: Remove this hack when the Buffer interface is removed.
> + */
> + if (cache_) {
> + FrameBuffer *buffer = dequeueFrameBuffer();
> + if (!buffer)
> + return;
> +
> + /* Notify anyone listening to the device. */
> + frameBufferReady.emit(buffer);
> + } else {
> + Buffer *buffer = dequeueBuffer();
> + if (!buffer)
> + return;
> +
> + /* Notify anyone listening to the device. */
> + bufferReady.emit(buffer);
> + }
> +}
> +
> +/**
> + * \brief Dequeue the next available buffer from the video device
> + *
> + * This method dequeues the next available buffer from the device. If no buffer
> + * is available to be dequeued it will return nullptr immediately.
> + *
> + * \todo: Reanme to dequeueBuffer() once the FrameBuffer transition is complete
s/Reanme/Rename/
> + *
> + * \return A pointer to the dequeued buffer on success, or nullptr otherwise
> + */
> +FrameBuffer *V4L2VideoDevice::dequeueFrameBuffer()
> +{
> + struct v4l2_buffer buf = {};
> + struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> + int ret;
> +
> + buf.type = bufferType_;
> + buf.memory = memoryType_;
>
> - /* Notify anyone listening to the device. */
> - bufferReady.emit(buffer);
> + bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> +
> + if (multiPlanar) {
> + buf.length = VIDEO_MAX_PLANES;
> + buf.m.planes = planes;
> + }
> +
> + ret = ioctl(VIDIOC_DQBUF, &buf);
> + if (ret < 0) {
> + LOG(V4L2, Error)
> + << "Failed to dequeue buffer: " << strerror(-ret);
> + return nullptr;
> + }
> +
> + LOG(V4L2, Debug) << "Dequeuing buffer " << buf.index;
> +
> + cache_->put(buf.index);
> +
> + auto it = queuedFrameBuffers_.find(buf.index);
> + FrameBuffer *buffer = it->second;
> + queuedFrameBuffers_.erase(it);
> +
> + if (queuedFrameBuffers_.empty())
> + fdEvent_->setEnabled(false);
> +
> + buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR
> + ? FrameMetadata::FrameError
> + : FrameMetadata::FrameSuccess;
> + buffer->metadata_.sequence = buf.sequence;
> + buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> + + buf.timestamp.tv_usec * 1000ULL;
> +
> + buffer->metadata_.planes.clear();
> + if (multiPlanar && buf.length > 1) {
I think you should drop buf.length > 1. Quoting the V4L2 documentation
of v4l2_buffer::bytesused,
"For multiplanar formats this field is ignored and the planes pointer is
used instead."
So we always need to look at planes for multiplanar buffers, even when
using a single planar format.
> + for (unsigned int nplane = 0; nplane < buf.length; nplane++)
> + buffer->metadata_.planes.push_back({ planes[nplane].bytesused });
> + } else {
> + buffer->metadata_.planes.push_back({ buf.bytesused });
> + }
> +
> + return buffer;
> }
>
> /**
> @@ -1260,6 +1537,11 @@ void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
> * \brief A Signal emitted when a buffer completes
> */
>
> +/**
> + * \var V4L2VideoDevice::frameBufferReady
> + * \brief A Signal emitted when a framebuffer completes
> + */
> +
> /**
> * \brief Start the video stream
> * \return 0 on success or a negative error code otherwise
> @@ -1281,8 +1563,8 @@ int V4L2VideoDevice::streamOn()
> /**
> * \brief Stop the video stream
> *
> - * Buffers that are still queued when the video stream is stopped are
> - * immediately dequeued with their status set to Buffer::BufferError,
> + * FrameBuffers that are still queued when the video stream is stopped are
Please don't pluralize class names. I think you can keep "Buffers" here
as a generic term.
> + * immediately dequeued with their status set to BufferMetadata::BufferCancelled,
s/BufferMetadata::BufferCancelled/FrameMetadata::FrameCancelled/
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> * and the bufferReady signal is emitted for them. The order in which those
> * buffers are dequeued is not specified.
> *
> @@ -1309,7 +1591,15 @@ int V4L2VideoDevice::streamOff()
> bufferReady.emit(buffer);
> }
>
> + for (auto it : queuedFrameBuffers_) {
> + FrameBuffer *buffer = it.second;
> +
> + buffer->metadata_.status = FrameMetadata::FrameCancelled;
> + frameBufferReady.emit(buffer);
> + }
> +
> queuedBuffers_.clear();
> + queuedFrameBuffers_.clear();
> fdEvent_->setEnabled(false);
>
> return 0;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list