From c20d3f5575cbb73adc8ad3cf6baac817ce8bd119 Mon Sep 17 00:00:00 2001 From: Laurent Pinchart Date: Sun, 2 Oct 2022 02:21:16 +0300 Subject: libcamera: framebuffer: Move remaining private data to Private class Private members of the FrameBuffer class are split between FrameBuffer and FrameBuffer::Private. There was no real justification for this split, and keeping some members private in the FrameBuffer class causes multiple issues: - Future modifications of the FrameBuffer class without breaking the ABI may be more difficult. - Mutable access to members that should not be modified by applications require a friend statement, or going through the Private class. Move all remaining private members to the Private class to address the first issue, and add a Private::metadata() function to address the second problem. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Umang Jain Tested-by: Naushir Patuck --- include/libcamera/framebuffer.h | 19 ++------ include/libcamera/internal/framebuffer.h | 10 +++- src/android/mm/cros_frame_buffer_allocator.cpp | 8 ++-- src/android/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 36b91d11..69553999 100644 --- a/include/libcamera/framebuffer.h +++ b/include/libcamera/framebuffer.h @@ -59,28 +59,19 @@ public: }; FrameBuffer(const std::vector &planes, unsigned int cookie = 0); - FrameBuffer(std::unique_ptr d, - const std::vector &planes, unsigned int cookie = 0); + FrameBuffer(std::unique_ptr d); - const std::vector &planes() const { return planes_; } + const std::vector &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 releaseFence(); private: LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer) - - friend class V4L2VideoDevice; /* Needed to update metadata_. */ - - std::vector planes_; - - FrameMetadata metadata_; - - uint64_t cookie_; }; } /* namespace libcamera */ diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h index 8a9cc98e..1f42a4fc 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 &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_ = 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 planes_; + FrameMetadata metadata_; + uint64_t cookie_; + std::unique_ptr 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 52e8c180..0665c77b 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 &planes) + : FrameBuffer::Private(planes), scopedHandle_(std::move(scopedHandle)) { } @@ -81,8 +82,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat, } return std::make_unique( - std::make_unique(std::move(scopedHandle)), - planes); + std::make_unique(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 acb2fa2b..956623df 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 &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( - std::make_unique(allocDevice_, handle), - planes); + std::make_unique(allocDevice_, handle, planes)); } PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp index 7be18560..5a7f3c0b 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 &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 &planes, unsigned int cookie) - : FrameBuffer(std::make_unique(), planes, cookie) + : FrameBuffer(std::make_unique(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 d, - const std::vector &planes, - unsigned int cookie) - : Extensible(std::move(d)), planes_(planes), cookie_(cookie) +FrameBuffer::FrameBuffer(std::unique_ptr 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 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 d, } /** - * \fn FrameBuffer::planes() * \brief Retrieve the static plane descriptors * \return Array of plane descriptors */ +const std::vector &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 955e1508..4d846f6b 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1791,12 +1791,14 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() watchdog_.start(std::chrono::duration_cast(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(); + + 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; @@ -1812,10 +1814,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()) { /* @@ -1941,9 +1942,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); } -- cgit v1.2.1