[libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make FrameBuffer class Extensible
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jul 12 10:25:17 CEST 2021
Hi Jacopo/Laurent,
On 12/07/2021 09:15, Jacopo Mondi wrote:
> 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_().
>
Quoting my earlier statement:
>> more on the "Do we want two ways to access the same data"
The part of this patch I disliked is that we can now access the internal
data with either
blah->_d()->foo()
or
const mydata *d = LIBCAMERA_D_PTR();
d->foo();
which simply calls _d()
I don't mind if we keep LIBCAMERA_D_PTR as the style - but I'd prefer
consistency ...
As LIBCAMERA_D_PTR isn't really needed, that's why I asked if it was
redundant.
If using it would be more explicit - then it should be used externally too.
buffer->LIBCAMERA_D_PTR()->setRequest(request);
would technically still work - but it doesn't look right ;-)
>>>>>> }
>>>>>>
>>>>>> 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
More information about the libcamera-devel
mailing list