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

[Kieran Bingham]

Hi Laurent,

On 07/07/2021 12:52, Laurent Pinchart wrote:
> Hi Kieran,
> 
> 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?




>> 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)



>>>  	}
>>>  
>>>  	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;
>>>
>