[libcamera-devel,v2,3/3] libcamera: framebuffer: Make FrameBuffer class Extensible
diff mbox series

Message ID 20210711170359.300-4-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Make Framebuffer class Extensible
Related show

Commit Message

Laurent Pinchart July 11, 2021, 5:03 p.m. UTC 
Implement the D-Pointer design pattern in the FrameBuffer class to allow
changing internal data without affecting the public ABI.

Move the request_ field and the setRequest() function to the
FrameBuffer::Private class. This allows hiding the setRequest() function
from the public API, removing one todo item. More fields may be moved
later.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
Changes since v1:

- Drop extraneous semicolon
- Use _d() instead of LIBCAMERA_D_PTR()
---
 include/libcamera/framebuffer.h          |  8 +++---
 include/libcamera/internal/framebuffer.h | 13 +++++++++
 src/libcamera/framebuffer.cpp            | 34 ++++++++++++++----------
 src/libcamera/pipeline/ipu3/cio2.cpp     |  3 ++-
 src/libcamera/pipeline/ipu3/frames.cpp   |  5 ++--
 src/libcamera/request.cpp                |  7 ++---
 6 files changed, 46 insertions(+), 24 deletions(-)

Comments

Kieran Bingham July 12, 2021, 8:34 a.m. UTC | #1 
On 11/07/2021 18:03, Laurent Pinchart wrote:
> Implement the D-Pointer design pattern in the FrameBuffer class to allow
> changing internal data without affecting the public ABI.
> 
> Move the request_ field and the setRequest() function to the
> FrameBuffer::Private class. This allows hiding the setRequest() function
> from the public API, removing one todo item. More fields may be moved
> later.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
> Changes since v1:
> 
> - Drop extraneous semicolon
> - Use _d() instead of LIBCAMERA_D_PTR()
> ---
>  include/libcamera/framebuffer.h          |  8 +++---
>  include/libcamera/internal/framebuffer.h | 13 +++++++++
>  src/libcamera/framebuffer.cpp            | 34 ++++++++++++++----------
>  src/libcamera/pipeline/ipu3/cio2.cpp     |  3 ++-
>  src/libcamera/pipeline/ipu3/frames.cpp   |  5 ++--
>  src/libcamera/request.cpp                |  7 ++---
>  6 files changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index baf22a466907..28307890eac9 100644
> --- a/include/libcamera/framebuffer.h
> +++ b/include/libcamera/framebuffer.h
> @@ -35,8 +35,10 @@ struct FrameMetadata {
>  	std::vector<Plane> planes;
>  };
>  
> -class FrameBuffer final
> +class FrameBuffer final : public Extensible
>  {
> +	LIBCAMERA_DECLARE_PRIVATE()
> +
>  public:
>  	struct Plane {
>  		FileDescriptor fd;
> @@ -47,8 +49,7 @@ public:
>  
>  	const std::vector<Plane> &planes() const { return planes_; }
>  
> -	Request *request() const { return request_; }
> -	void setRequest(Request *request) { request_ = request; }
> +	Request *request() const;
>  	const FrameMetadata &metadata() const { return metadata_; }
>  
>  	unsigned int cookie() const { return cookie_; }
> @@ -63,7 +64,6 @@ private:
>  
>  	std::vector<Plane> planes_;
>  
> -	Request *request_;
>  	FrameMetadata metadata_;
>  
>  	unsigned int cookie_;
> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> index 0c76fc62af1d..a11e895d9b88 100644
> --- a/include/libcamera/internal/framebuffer.h
> +++ b/include/libcamera/internal/framebuffer.h
> @@ -47,6 +47,19 @@ public:
>  	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>  };
>  
> +class FrameBuffer::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
> +
> +public:
> +	Private(FrameBuffer *buffer);
> +
> +	void setRequest(Request *request) { request_ = request; }
> +
> +private:
> +	Request *request_;
> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__ */
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 4bde556c4013..40bf64b0a4fe 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -100,6 +100,21 @@ LOG_DEFINE_CATEGORY(Buffer)
>   * \brief Array of per-plane metadata
>   */
>  
> +FrameBuffer::Private::Private(FrameBuffer *buffer)
> +	: Extensible::Private(buffer), request_(nullptr)
> +{
> +}
> +
> +/**
> + * \fn FrameBuffer::Private::setRequest()
> + * \brief Set the request this buffer belongs to
> + * \param[in] request Request to set
> + *
> + * For buffers added to requests by applications, this method is called by
> + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline
> + * handlers, it is called by the pipeline handlers themselves.
> + */
> +
>  /**
>   * \class FrameBuffer
>   * \brief Frame buffer data and its associated dynamic metadata
> @@ -161,7 +176,7 @@ LOG_DEFINE_CATEGORY(Buffer)
>   * \param[in] cookie Cookie
>   */
>  FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> -	: planes_(planes), request_(nullptr), cookie_(cookie)
> +	: Extensible(new Private(this)), planes_(planes), cookie_(cookie)
>  {
>  }
>  
> @@ -172,7 +187,6 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
>   */
>  
>  /**
> - * \fn FrameBuffer::request()
>   * \brief Retrieve the request this buffer belongs to
>   *
>   * The intended callers of this method are buffer completion handlers that
> @@ -185,18 +199,10 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
>   * \return The Request the FrameBuffer belongs to, or nullptr if the buffer is
>   * not associated with a request
>   */
> -
> -/**
> - * \fn FrameBuffer::setRequest()
> - * \brief Set the request this buffer belongs to
> - * \param[in] request Request to set
> - *
> - * For buffers added to requests by applications, this method is called by
> - * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline
> - * handlers, it is called by the pipeline handlers themselves.
> - *
> - * \todo Shall be hidden from applications with a d-pointer design.
> - */
> +Request *FrameBuffer::request() const
> +{
> +	return _d()->request_;
> +}
>  
>  /**
>   * \fn FrameBuffer::metadata()
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 1be2cbcd5cac..1bcd580e251c 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -14,6 +14,7 @@
>  #include <libcamera/stream.h>
>  
>  #include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  
> @@ -278,7 +279,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  
>  		buffer = availableBuffers_.front();
>  		availableBuffers_.pop();
> -		buffer->setRequest(request);
> +		buffer->_d()->setRequest(request);
>  	}
>  
>  	int ret = output_->queueBuffer(buffer);
> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
> index ce5ccbf18e41..a4c3477cd9ef 100644
> --- a/src/libcamera/pipeline/ipu3/frames.cpp
> +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> @@ -10,6 +10,7 @@
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/request.h>
>  
> +#include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
> @@ -56,8 +57,8 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)
>  	FrameBuffer *paramBuffer = availableParamBuffers_.front();
>  	FrameBuffer *statBuffer = availableStatBuffers_.front();
>  
> -	paramBuffer->setRequest(request);
> -	statBuffer->setRequest(request);
> +	paramBuffer->_d()->setRequest(request);
> +	statBuffer->_d()->setRequest(request);
>  
>  	availableParamBuffers_.pop();
>  	availableStatBuffers_.pop();
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 5faf3c71ff02..c095c9f45b09 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -18,6 +18,7 @@
>  #include <libcamera/stream.h>
>  
>  #include "libcamera/internal/camera_controls.h"
> +#include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/tracepoints.h"
>  
>  /**
> @@ -121,7 +122,7 @@ void Request::reuse(ReuseFlag flags)
>  	if (flags & ReuseBuffers) {
>  		for (auto pair : bufferMap_) {
>  			FrameBuffer *buffer = pair.second;
> -			buffer->setRequest(this);
> +			buffer->_d()->setRequest(this);
>  			pending_.insert(buffer);
>  		}
>  	} else {
> @@ -191,7 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>  		return -EEXIST;
>  	}
>  
> -	buffer->setRequest(this);
> +	buffer->_d()->setRequest(this);
>  	pending_.insert(buffer);
>  	bufferMap_[stream] = buffer;
>  
> @@ -336,7 +337,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
>  
> -	buffer->setRequest(nullptr);
> +	buffer->_d()->setRequest(nullptr);
>  
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>  		cancelled_ = true;
>

Patch
diff mbox series 

diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
index baf22a466907..28307890eac9 100644
--- a/include/libcamera/framebuffer.h
+++ b/include/libcamera/framebuffer.h
@@ -35,8 +35,10 @@  struct FrameMetadata {
 	std::vector<Plane> planes;
 };
 
-class FrameBuffer final
+class FrameBuffer final : public Extensible
 {
+	LIBCAMERA_DECLARE_PRIVATE()
+
 public:
 	struct Plane {
 		FileDescriptor fd;
@@ -47,8 +49,7 @@  public:
 
 	const std::vector<Plane> &planes() const { return planes_; }
 
-	Request *request() const { return request_; }
-	void setRequest(Request *request) { request_ = request; }
+	Request *request() const;
 	const FrameMetadata &metadata() const { return metadata_; }
 
 	unsigned int cookie() const { return cookie_; }
@@ -63,7 +64,6 @@  private:
 
 	std::vector<Plane> planes_;
 
-	Request *request_;
 	FrameMetadata metadata_;
 
 	unsigned int cookie_;
diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
index 0c76fc62af1d..a11e895d9b88 100644
--- a/include/libcamera/internal/framebuffer.h
+++ b/include/libcamera/internal/framebuffer.h
@@ -47,6 +47,19 @@  public:
 	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
 };
 
+class FrameBuffer::Private : public Extensible::Private
+{
+	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
+
+public:
+	Private(FrameBuffer *buffer);
+
+	void setRequest(Request *request) { request_ = request; }
+
+private:
+	Request *request_;
+};
+
 } /* namespace libcamera */
 
 #endif /* __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__ */
diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
index 4bde556c4013..40bf64b0a4fe 100644
--- a/src/libcamera/framebuffer.cpp
+++ b/src/libcamera/framebuffer.cpp
@@ -100,6 +100,21 @@  LOG_DEFINE_CATEGORY(Buffer)
  * \brief Array of per-plane metadata
  */
 
+FrameBuffer::Private::Private(FrameBuffer *buffer)
+	: Extensible::Private(buffer), request_(nullptr)
+{
+}
+
+/**
+ * \fn FrameBuffer::Private::setRequest()
+ * \brief Set the request this buffer belongs to
+ * \param[in] request Request to set
+ *
+ * For buffers added to requests by applications, this method is called by
+ * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline
+ * handlers, it is called by the pipeline handlers themselves.
+ */
+
 /**
  * \class FrameBuffer
  * \brief Frame buffer data and its associated dynamic metadata
@@ -161,7 +176,7 @@  LOG_DEFINE_CATEGORY(Buffer)
  * \param[in] cookie Cookie
  */
 FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
-	: planes_(planes), request_(nullptr), cookie_(cookie)
+	: Extensible(new Private(this)), planes_(planes), cookie_(cookie)
 {
 }
 
@@ -172,7 +187,6 @@  FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
  */
 
 /**
- * \fn FrameBuffer::request()
  * \brief Retrieve the request this buffer belongs to
  *
  * The intended callers of this method are buffer completion handlers that
@@ -185,18 +199,10 @@  FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
  * \return The Request the FrameBuffer belongs to, or nullptr if the buffer is
  * not associated with a request
  */
-
-/**
- * \fn FrameBuffer::setRequest()
- * \brief Set the request this buffer belongs to
- * \param[in] request Request to set
- *
- * For buffers added to requests by applications, this method is called by
- * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline
- * handlers, it is called by the pipeline handlers themselves.
- *
- * \todo Shall be hidden from applications with a d-pointer design.
- */
+Request *FrameBuffer::request() const
+{
+	return _d()->request_;
+}
 
 /**
  * \fn FrameBuffer::metadata()
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index 1be2cbcd5cac..1bcd580e251c 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -14,6 +14,7 @@ 
 #include <libcamera/stream.h>
 
 #include "libcamera/internal/camera_sensor.h"
+#include "libcamera/internal/framebuffer.h"
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/v4l2_subdevice.h"
 
@@ -278,7 +279,7 @@  FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
 
 		buffer = availableBuffers_.front();
 		availableBuffers_.pop();
-		buffer->setRequest(request);
+		buffer->_d()->setRequest(request);
 	}
 
 	int ret = output_->queueBuffer(buffer);
diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
index ce5ccbf18e41..a4c3477cd9ef 100644
--- a/src/libcamera/pipeline/ipu3/frames.cpp
+++ b/src/libcamera/pipeline/ipu3/frames.cpp
@@ -10,6 +10,7 @@ 
 #include <libcamera/framebuffer.h>
 #include <libcamera/request.h>
 
+#include "libcamera/internal/framebuffer.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/v4l2_videodevice.h"
 
@@ -56,8 +57,8 @@  IPU3Frames::Info *IPU3Frames::create(Request *request)
 	FrameBuffer *paramBuffer = availableParamBuffers_.front();
 	FrameBuffer *statBuffer = availableStatBuffers_.front();
 
-	paramBuffer->setRequest(request);
-	statBuffer->setRequest(request);
+	paramBuffer->_d()->setRequest(request);
+	statBuffer->_d()->setRequest(request);
 
 	availableParamBuffers_.pop();
 	availableStatBuffers_.pop();
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index 5faf3c71ff02..c095c9f45b09 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -18,6 +18,7 @@ 
 #include <libcamera/stream.h>
 
 #include "libcamera/internal/camera_controls.h"
+#include "libcamera/internal/framebuffer.h"
 #include "libcamera/internal/tracepoints.h"
 
 /**
@@ -121,7 +122,7 @@  void Request::reuse(ReuseFlag flags)
 	if (flags & ReuseBuffers) {
 		for (auto pair : bufferMap_) {
 			FrameBuffer *buffer = pair.second;
-			buffer->setRequest(this);
+			buffer->_d()->setRequest(this);
 			pending_.insert(buffer);
 		}
 	} else {
@@ -191,7 +192,7 @@  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
 		return -EEXIST;
 	}
 
-	buffer->setRequest(this);
+	buffer->_d()->setRequest(this);
 	pending_.insert(buffer);
 	bufferMap_[stream] = buffer;
 
@@ -336,7 +337,7 @@  bool Request::completeBuffer(FrameBuffer *buffer)
 	int ret = pending_.erase(buffer);
 	ASSERT(ret == 1);
 
-	buffer->setRequest(nullptr);
+	buffer->_d()->setRequest(nullptr);
 
 	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
 		cancelled_ = true;