[libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make FrameBuffer class Extensible
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jul 11 16:38:28 CEST 2021
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 ?
> >>> }
> >>>
> >>> 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