[libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make FrameBuffer class Extensible
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jul 7 14:35:33 CEST 2021
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;
>>>
>
More information about the libcamera-devel
mailing list