[libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make FrameBuffer class Extensible

[Jacopo Mondi]

Hi Laurent,

On Sun, Jul 11, 2021 at 05:38:28PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Wed, Jul 07, 2021 at 01:35:33PM +0100, Kieran Bingham wrote:
> > On 07/07/2021 12:52, Laurent Pinchart wrote:
> > > On Wed, Jul 07, 2021 at 09:41:40AM +0100, Kieran Bingham wrote:
> > >> 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.
> > >
> > > And that's also the whole point of the D-Pointer pattern, if we added
> > > FrameBuffer::setRequest() as a wrapper around _d()->setRequest(), it
> > > wouldn't make much sense :-)
> >
> > I get that, I'm not suggesting wrapping it to be available in the
> > 'public' methods of the class...
> >
> > >> Does _d convey the right meaning to everyone for that? Or would the
> > >> accessor be better named as
> > >>
> > >> 	buffer->internal()->setRequest(request) ?
> > >
> > > I'd like to keep the name as short as possible.
> >
> > What does _d actually mean?
> >
> > ->internal() I could understand.
> > ->private() I could understand (but it's a reserved keyword).
> >
> > If it was _p I could understand it was shorthand for either pointer or
> > private.
> >
> > But 'd' sounds like a random letter to me currently. it doesn't convey
> > the message "Access the internal object" in any way other than a magic
> > short letter.
> >
> > I.e. is there a distinction as to why it is a 'd' rather than a 'z'...
> >
> > https://en.wikipedia.org/wiki/Opaque_pointer tells me that it's simply
> > the pattern name ... so is _d data? or something else?
>
> It's the design pattern name. I'm not sure what it meant in the first
> place, it could be data.
>
> > >> 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"
> > >
> > > We could drop LIBCAMERA_D_PTR() as it's easy to type _d(). It was there
> > > originally to hide the <Private> template argument, but now that it's
> > > gone, I think it makes sense to drop the macro.
> >
> > Will that apply to the _o() usage as well? (it doesn't have to)
> >
> > (Hence why I asked earlier if _o() should be templates too, but they
> > aren't used outside of the ::Private)
>
> If we want to drop the LIBCAMERA_O_PTR() then I think we should define
> overriden version of the _o() function to avoid having to write
> _o<Public>(). Anyone has a preference ?
>

I don't, but I also don't really think it's worth tbh.

Whatever naming we chose, one has to read documentation to get what
'internal' 'external' 'outer' or 'private' means in the context of
Exensible. Even if the macro it's not required, LIBCAMERA_D_PTR()
makes explicit what the reader is dealing with, more than just d_().

> > >>>  	}
> > >>>
> > >>>  	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;
> > >>>
>
> --
> Regards,
>
> Laurent Pinchart