From patchwork Mon Oct 10 14:49:23 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 17571 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id BA01DBD16B for ; Mon, 10 Oct 2022 14:49:32 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id ECDD362D66; Mon, 10 Oct 2022 16:49:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1665413370; bh=hI4jnDuI3ZXpJK91f58UAMLYfGVNUBW9V9mcGjwTvAg=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=rlRNuxK0hFNB9B0fK2lK2Pu4Uei3ZjbknBV1xa0uWxVHLyeH298qNDWecUPgv9wQ+ by5ovnJs+pCqEticvqpawFLpk0yDujwKa7yiIrcu3ldbKKmPZBFKrDDN5j26MMOReg iCuIE58OEcdpSov+OWqNmZKvz9WO9nmuve3Ns+LUTqGz4AKH31N2h4TUoohG1Gqfsy W/ANoexL/mK3A7nMQAKLyxUaWsOd15tV8FzptzaQjtcIgfstJSs2P+mA2RipS4NDc5 baxTFvnPAg0//w2xhu4ERZOuiQrapEuxxiIUvPalDYDz1M1vLN105tvPBsFspuXEbr RaJ+8+xVknQ8w== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8902762272 for ; Mon, 10 Oct 2022 16:49:29 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="cf3cJIQZ"; dkim-atps=neutral Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D62F1BB0 for ; Mon, 10 Oct 2022 16:49:28 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1665413369; bh=hI4jnDuI3ZXpJK91f58UAMLYfGVNUBW9V9mcGjwTvAg=; h=From:To:Subject:Date:From; b=cf3cJIQZiqjYC0xEE92w9Jv5MGOszoj+i+LxGp2PhZzomlCGTeghcEhSbb9K/qVUH IXKB4CT2uajojI3zkSyj7uON7ggXigsixcgJ5tHSxz8I4ZQ2ZDFmoS4OkBHQTC19+N EYqXA8NzaK8QpekcAsJISQl5R6uGkG7UEq3Vyiwc= To: libcamera-devel@lists.libcamera.org Date: Mon, 10 Oct 2022 17:49:23 +0300 Message-Id: <20221010144923.21949-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.35.1 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2.1 1/4] libcamera: framebuffer: Move remaining private data to Private class X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Laurent Pinchart via libcamera-devel From: Laurent Pinchart Reply-To: Laurent Pinchart Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 --- Changes since v2: - Fix compilation on Chrome OS --- 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 &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 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 &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 52e8c180e3db..0665c77b1377 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 acb2fa2b699d..956623df1f80 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 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 &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 955e150867ef..4d846f6be7fa 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); }