[libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make FrameBuffer class Extensible
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jul 7 10:41:40 CEST 2021
Hi Laurent,
On 05/07/2021 00:28, 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 at ideasonboard.com>
> ---
> 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, 47 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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
> */
> +Request *FrameBuffer::request() const
> +{
> + const Private *const d = LIBCAMERA_D_PTR();
so internally in a class with a Private, it uses LIBCAMERA_D_PTR(),
>
> -/**
> - * \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.
> - */
> + 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);
But anywhere else, is through the _d()?
Is LIBCAMERA_D_PTR now redundant, that even internally the access could
be done through _d()?
Though I find
buffer->_d()->setRequest(request);
A bit more ugly to read... and now the read has to know which parts of
the object are accessible through buffer-> and which ones have to be
indirected - but I guess that's not really an issue. Just an adjustment
required on the developers.
Does _d convey the right meaning to everyone for that? Or would the
accessor be better named as
buffer->internal()->setRequest(request) ?
I see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's
there to maintain the existing functionality anyway, but my question is
more on the "Do we want two ways to access the same data"
> }
>
> 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;
>
More information about the libcamera-devel
mailing list