[libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make FrameBuffer class Extensible
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 7 13:52:22 CEST 2021
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 :-)
> 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.
> 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.
> > }
> >
> > 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