[libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make FrameBuffer class Extensible
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 12 15:03:50 CEST 2021
Hi Kieran,
On Mon, Jul 12, 2021 at 09:25:17AM +0100, Kieran Bingham wrote:
> On 12/07/2021 09:15, Jacopo Mondi wrote:
> > On Sun, Jul 11, 2021 at 05:38:28PM +0300, Laurent Pinchart wrote:
> >> 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 ;-)
That I think we all agree on :-)
> >>>>>> }
> >>>>>>
> >>>>>> 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