[libcamera-devel] [PATCH 2/4] libcamera: framebuffer: Move remaining private data to Private class
Jacopo Mondi
jacopo at jmondi.org
Mon Oct 3 10:32:35 CEST 2022
Hi Laurent
On Sun, Oct 02, 2022 at 03:36:10AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/framebuffer.h | 19 ++----
> include/libcamera/internal/framebuffer.h | 10 +++-
> .../mm/cros_frame_buffer_allocator.cpp | 8 +--
> .../mm/generic_frame_buffer_allocator.cpp | 9 +--
> src/libcamera/framebuffer.cpp | 58 ++++++++++++-------
> src/libcamera/v4l2_videodevice.cpp | 20 ++++---
> 6 files changed, 71 insertions(+), 53 deletions(-)
>
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index 36b91d11ed79..695539993f96 100644
> --- a/include/libcamera/framebuffer.h
> +++ b/include/libcamera/framebuffer.h
> @@ -59,28 +59,19 @@ public:
> };
>
> FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> - FrameBuffer(std::unique_ptr<Private> d,
> - const std::vector<Plane> &planes, unsigned int cookie = 0);
> + FrameBuffer(std::unique_ptr<Private> d);
>
> - const std::vector<Plane> &planes() const { return planes_; }
> + const std::vector<Plane> &planes() const;
> Request *request() const;
> - const FrameMetadata &metadata() const { return metadata_; }
> + const FrameMetadata &metadata() const;
>
> - uint64_t cookie() const { return cookie_; }
> - void setCookie(uint64_t cookie) { cookie_ = cookie; }
> + uint64_t cookie() const;
> + void setCookie(uint64_t cookie);
>
> std::unique_ptr<Fence> releaseFence();
>
> private:
> LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)
> -
> - friend class V4L2VideoDevice; /* Needed to update metadata_. */
> -
> - std::vector<Plane> planes_;
> -
> - FrameMetadata metadata_;
> -
> - uint64_t cookie_;
> };
>
> } /* namespace libcamera */
> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> index 8a9cc98e22e3..1f42a4fcc865 100644
> --- a/include/libcamera/internal/framebuffer.h
> +++ b/include/libcamera/internal/framebuffer.h
> @@ -22,7 +22,7 @@ class FrameBuffer::Private : public Extensible::Private
> LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
>
> public:
> - Private();
> + Private(const std::vector<Plane> &planes, uint64_t cookie = 0);
> virtual ~Private();
>
> void setRequest(Request *request) { request_ = request; }
> @@ -31,9 +31,15 @@ public:
> Fence *fence() const { return fence_.get(); }
> void setFence(std::unique_ptr<Fence> fence) { fence_ = std::move(fence); }
>
> - void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; }
> + void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }
> +
> + FrameMetadata &metadata() { return metadata_; }
>
> private:
> + std::vector<Plane> planes_;
> + FrameMetadata metadata_;
> + uint64_t cookie_;
> +
> std::unique_ptr<Fence> fence_;
> Request *request_;
> bool isContiguous_;
> diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp
> index 52e8c180e3db..1e6f1ef55434 100644
> --- a/src/android/mm/cros_frame_buffer_allocator.cpp
> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp
> @@ -28,8 +28,9 @@ class CrosFrameBufferData : public FrameBuffer::Private
> LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
>
> public:
> - CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)
> - : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))
> + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle,
> + const std::vector<Plane> &planes)
> + : FrameBuffer::Private(planes), scopedHandle_(std::move(scopedHandle))
> {
> }
>
> @@ -81,8 +82,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
> }
>
> return std::make_unique<FrameBuffer>(
> - std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),
> - planes);
> + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle), planes));
> }
>
> PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
> index acb2fa2b699d..8ff4241f1506 100644
> --- a/src/android/mm/generic_frame_buffer_allocator.cpp
> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> @@ -32,8 +32,10 @@ class GenericFrameBufferData : public FrameBuffer::Private
>
> public:
> GenericFrameBufferData(struct alloc_device_t *allocDevice,
> - buffer_handle_t handle)
> - : allocDevice_(allocDevice), handle_(handle)
> + buffer_handle_t handle,
> + const std::vector<Plane> &planes)
> + : FrameBuffer::Private(planes), allocDevice_(allocDevice),
> + handle_(handle)
> {
> ASSERT(allocDevice_);
> ASSERT(handle_);
> @@ -136,8 +138,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
> }
>
> return std::make_unique<FrameBuffer>(
> - std::make_unique<GenericFrameBufferData>(allocDevice_, handle),
> - planes);
> + std::make_unique<GenericFrameBufferData>(allocDevice_, handle, planes));
> }
>
> PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 7be18560417c..5a7f3c0b5f9a 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -114,9 +114,16 @@ LOG_DEFINE_CATEGORY(Buffer)
> * pipeline handlers.
> */
>
> -FrameBuffer::Private::Private()
> - : request_(nullptr), isContiguous_(true)
> +/**
> + * \brief Construct a FrameBuffer::Private instance
> + * \param[in] planes The frame memory planes
> + * \param[in] cookie Cookie
> + */
> +FrameBuffer::Private::Private(const std::vector<Plane> &planes, uint64_t cookie)
> + : planes_(planes), cookie_(cookie), request_(nullptr),
> + isContiguous_(true)
> {
> + metadata_.planes_.resize(planes_.size());
> }
>
> /**
> @@ -194,6 +201,12 @@ FrameBuffer::Private::~Private()
> * indicate that the metadata is invalid.
> */
>
> +/**
> + * \fn FrameBuffer::Private::metadata()
> + * \brief Retrieve the dynamic metadata
> + * \return Dynamic metadata for the frame contained in the buffer
> + */
> +
> /**
> * \class FrameBuffer
> * \brief Frame buffer data and its associated dynamic metadata
> @@ -291,29 +304,22 @@ ino_t fileDescriptorInode(const SharedFD &fd)
> * \param[in] cookie Cookie
> */
> FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> - : FrameBuffer(std::make_unique<Private>(), planes, cookie)
> + : FrameBuffer(std::make_unique<Private>(planes, cookie))
> {
> }
>
> /**
> - * \brief Construct a FrameBuffer with an extensible private class and an array
> - * of planes
> + * \brief Construct a FrameBuffer with an extensible private class
> * \param[in] d The extensible private class
> - * \param[in] planes The frame memory planes
> - * \param[in] cookie Cookie
> */
> -FrameBuffer::FrameBuffer(std::unique_ptr<Private> d,
> - const std::vector<Plane> &planes,
> - unsigned int cookie)
> - : Extensible(std::move(d)), planes_(planes), cookie_(cookie)
> +FrameBuffer::FrameBuffer(std::unique_ptr<Private> d)
> + : Extensible(std::move(d))
> {
> - metadata_.planes_.resize(planes_.size());
> -
> unsigned int offset = 0;
> bool isContiguous = true;
> ino_t inode = 0;
>
> - for (const auto &plane : planes_) {
> + for (const auto &plane : _d()->planes_) {
> ASSERT(plane.offset != Plane::kInvalidOffset);
>
> if (plane.offset != offset) {
> @@ -325,9 +331,9 @@ FrameBuffer::FrameBuffer(std::unique_ptr<Private> d,
> * Two different dmabuf file descriptors may still refer to the
> * same dmabuf instance. Check this using inodes.
> */
> - if (plane.fd != planes_[0].fd) {
> + if (plane.fd != _d()->planes_[0].fd) {
> if (!inode)
> - inode = fileDescriptorInode(planes_[0].fd);
> + inode = fileDescriptorInode(_d()->planes_[0].fd);
> if (fileDescriptorInode(plane.fd) != inode) {
> isContiguous = false;
> break;
> @@ -344,10 +350,13 @@ FrameBuffer::FrameBuffer(std::unique_ptr<Private> d,
> }
>
> /**
> - * \fn FrameBuffer::planes()
> * \brief Retrieve the static plane descriptors
> * \return Array of plane descriptors
> */
> +const std::vector<FrameBuffer::Plane> &FrameBuffer::planes() const
> +{
> + return _d()->planes_;
> +}
>
> /**
> * \brief Retrieve the request this buffer belongs to
> @@ -368,13 +377,15 @@ Request *FrameBuffer::request() const
> }
>
> /**
> - * \fn FrameBuffer::metadata()
> * \brief Retrieve the dynamic metadata
> * \return Dynamic metadata for the frame contained in the buffer
> */
> +const FrameMetadata &FrameBuffer::metadata() const
> +{
> + return _d()->metadata_;
> +}
>
> /**
> - * \fn FrameBuffer::cookie()
> * \brief Retrieve the cookie
> *
> * The cookie belongs to the creator of the FrameBuffer, which controls its
> @@ -384,9 +395,12 @@ Request *FrameBuffer::request() const
> *
> * \return The cookie
> */
> +uint64_t FrameBuffer::cookie() const
> +{
> + return _d()->cookie_;
> +}
>
> /**
> - * \fn FrameBuffer::setCookie()
> * \brief Set the cookie
> * \param[in] cookie Cookie to set
> *
> @@ -395,6 +409,10 @@ Request *FrameBuffer::request() const
> * modify the cookie value of buffers they haven't created themselves. The
> * libcamera core never modifies the buffer cookie.
> */
> +void FrameBuffer::setCookie(uint64_t cookie)
> +{
> + _d()->cookie_ = cookie;
> +}
>
> /**
> * \brief Extract the Fence associated with this Framebuffer
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 1dbb839ed456..d25a785adeea 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1793,12 +1793,14 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_));
> }
>
> - 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;
> + FrameMetadata &metadata = buffer->_d()->metadata();
A very minor nit: in queueBuffer() we use the public FrameBuffer::metadata()
function. Might be nice to use the same pattern
That apart
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> +
> + metadata.status = buf.flags & V4L2_BUF_FLAG_ERROR
> + ? FrameMetadata::FrameError
> + : FrameMetadata::FrameSuccess;
> + metadata.sequence = buf.sequence;
> + metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> + + buf.timestamp.tv_usec * 1000ULL;
>
> if (V4L2_TYPE_IS_OUTPUT(buf.type))
> return buffer;
> @@ -1814,10 +1816,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> << buf.sequence << ")";
> firstFrame_ = buf.sequence;
> }
> - buffer->metadata_.sequence -= firstFrame_.value();
> + metadata.sequence -= firstFrame_.value();
>
> unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
> - FrameMetadata &metadata = buffer->metadata_;
>
> if (numV4l2Planes != buffer->planes().size()) {
> /*
> @@ -1943,9 +1944,10 @@ int V4L2VideoDevice::streamOff()
> /* Send back all queued buffers. */
> for (auto it : queuedBuffers_) {
> FrameBuffer *buffer = it.second;
> + FrameMetadata &metadata = buffer->_d()->metadata();
>
> cache_->put(it.first);
> - buffer->metadata_.status = FrameMetadata::FrameCancelled;
> + metadata.status = FrameMetadata::FrameCancelled;
> bufferReady.emit(buffer);
> }
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list