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 <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Tested-by: Naushir Patuck <naush@raspberrypi.com>

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<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 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<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 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<FrameBuffer::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 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<FrameBuffer::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 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<Plane> &planes, uint64_t cookie)
+	: planes_(planes), cookie_(cookie), request_(nullptr),
+	  isContiguous_(true)
 {
+	metadata_.planes_.resize(planes_.size());
 }
 
 /**
@@ -195,6 +202,12 @@ FrameBuffer::Private::~Private()
  */
 
 /**
+ * \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 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<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();
+
+	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);
 	}