| Message ID | 20210704232817.8205-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Jul 05, 2021 at 02:28:17AM +0300, 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@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_; > +}; > + Oh, I should have read the series in full before asking The "Private" is available to other internal classes as it gets defined in an internal header. Looks good Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> for 1/3 too Thanks j > } /* 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(); > > -/** > - * \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); > } > > 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 >
Hi Laurent, On 7/5/21 4:58 AM, 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@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@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(); > > -/** > - * \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); > } > > 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;
Hi Laurent, 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@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. Does _d convey the right meaning to everyone for that? Or would the accessor be better named as buffer->internal()->setRequest(request) ? 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" > } > > 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; >
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@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; > >
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@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; >>> >
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@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; > >>>
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@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_(). > > >>> } > > >>> > > >>> 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
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@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
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@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; > >>>>>>
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(); -/** - * \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); } 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;
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@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(-)