| Message ID | 20210723040036.32346-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2021-07-23 07:00:22 +0300, Laurent Pinchart wrote: > The Extensible and Extensible::Private classes contain pointers to each > other. These pointers are initialized in the respective class's > constructor, by passing a pointer to the other class to each > constructor. This particular construct reduces the flexibility of the > Extensible pattern, as the Private class instance has to be allocated > and constructed in the members initializer list of the Extensible > class's constructor. It is thus impossible to perform any operation on > the Private class between its construction and the construction of the > Extensible class, or to subclass the Private class without subclassing > the Extensible class. > > To make the design pattern more flexible, don't pass the pointer to the > Extensible class to the Private class's constructor, but initialize the > pointer manually in the Extensible class's constructor. This requires a > const_cast as the o_ member of the Private class is const. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/class.h | 3 ++- > include/libcamera/internal/framebuffer.h | 2 +- > src/android/camera_hal_config.cpp | 7 +++---- > src/android/mm/cros_camera_buffer.cpp | 4 ++-- > src/android/mm/generic_camera_buffer.cpp | 3 +-- > src/libcamera/base/class.cpp | 6 +++--- > src/libcamera/camera.cpp | 10 +++++----- > src/libcamera/camera_manager.cpp | 8 ++++---- > src/libcamera/framebuffer.cpp | 6 +++--- > 9 files changed, 24 insertions(+), 25 deletions(-) > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > index 9c7f0f2e6e27..c9d9cd949ca1 100644 > --- a/include/libcamera/base/class.h > +++ b/include/libcamera/base/class.h > @@ -64,7 +64,7 @@ public: > class Private > { > public: > - Private(Extensible *o); > + Private(); > virtual ~Private(); > > #ifndef __DOXYGEN__ > @@ -82,6 +82,7 @@ public: > #endif > > private: > + friend class Extensible; I don't like friends, but here it makes sens. I would add a comment here explaining why the friend statement is needed so it can be removed in the future if someone can think of a different design pattern. With this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Extensible *const o_; > }; > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h > index a11e895d9b88..8c187adf70c7 100644 > --- a/include/libcamera/internal/framebuffer.h > +++ b/include/libcamera/internal/framebuffer.h > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > public: > - Private(FrameBuffer *buffer); > + Private(); > > void setRequest(Request *request) { request_ = request; } > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > index 833cf4ba98a9..bbbb1410b52c 100644 > --- a/src/android/camera_hal_config.cpp > +++ b/src/android/camera_hal_config.cpp > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private > LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig) > > public: > - Private(CameraHalConfig *halConfig); > + Private(); > > int parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras); > > @@ -50,8 +50,7 @@ private: > std::map<std::string, CameraConfigData> *cameras_; > }; > > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig) > - : Extensible::Private(halConfig) > +CameraHalConfig::Private::Private() > { > } > > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh, > } > > CameraHalConfig::CameraHalConfig() > - : Extensible(new Private(this)), exists_(false), valid_(false) > + : Extensible(new Private()), exists_(false), valid_(false) > { > parseConfigurationFile(); > } > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > index bb55b95e3a39..437502fb8276 100644 > --- a/src/android/mm/cros_camera_buffer.cpp > +++ b/src/android/mm/cros_camera_buffer.cpp > @@ -47,8 +47,8 @@ private: > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > buffer_handle_t camera3Buffer, > [[maybe_unused]] int flags) > - : Extensible::Private(cameraBuffer), handle_(camera3Buffer), > - numPlanes_(0), valid_(false), registered_(false) > + : handle_(camera3Buffer), numPlanes_(0), valid_(false), > + registered_(false) > { > bufferManager_ = cros::CameraBufferManager::GetInstance(); > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp > index 166be36efd5b..2a4b77ea5236 100644 > --- a/src/android/mm/generic_camera_buffer.cpp > +++ b/src/android/mm/generic_camera_buffer.cpp > @@ -34,9 +34,8 @@ public: > size_t jpegBufferSize(size_t maxJpegBufferSize) const; > }; > > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > buffer_handle_t camera3Buffer, int flags) > - : Extensible::Private(cameraBuffer) > { > maps_.reserve(camera3Buffer->numFds); > error_ = 0; > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp > index d4f0ac64ad48..d24043c20e55 100644 > --- a/src/libcamera/base/class.cpp > +++ b/src/libcamera/base/class.cpp > @@ -151,6 +151,7 @@ namespace libcamera { > Extensible::Extensible(Extensible::Private *d) > : d_(d) > { > + *const_cast<Extensible **>(&d_->o_) = this; > } > > /** > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d) > > /** > * \brief Construct an instance of an Extensible class private data > - * \param[in] o Pointer to the public class object > */ > -Extensible::Private::Private(Extensible *o) > - : o_(o) > +Extensible::Private::Private() > + : o_(nullptr) > { > } > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index c8858e71d105..c126b49290ce 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -344,7 +344,7 @@ public: > CameraRunning, > }; > > - Private(Camera *camera, PipelineHandler *pipe, const std::string &id, > + Private(PipelineHandler *pipe, const std::string &id, > const std::set<Stream *> &streams); > ~Private(); > > @@ -368,11 +368,11 @@ private: > std::atomic<State> state_; > }; > > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe, > +Camera::Private::Private(PipelineHandler *pipe, > const std::string &id, > const std::set<Stream *> &streams) > - : Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id), > - streams_(streams), disconnected_(false), state_(CameraAvailable) > + : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > + disconnected_(false), state_(CameraAvailable) > { > } > > @@ -632,7 +632,7 @@ const std::string &Camera::id() const > > Camera::Camera(PipelineHandler *pipe, const std::string &id, > const std::set<Stream *> &streams) > - : Extensible(new Private(this, pipe, id, streams)) > + : Extensible(new Private(pipe, id, streams)) > { > } > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 1c79308ad4b5..384a574f2baa 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > public: > - Private(CameraManager *cm); > + Private(); > > int start(); > void addCamera(std::shared_ptr<Camera> camera, > @@ -74,8 +74,8 @@ private: > ProcessManager processManager_; > }; > > -CameraManager::Private::Private(CameraManager *cm) > - : Extensible::Private(cm), initialized_(false) > +CameraManager::Private::Private() > + : initialized_(false) > { > } > > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera) > CameraManager *CameraManager::self_ = nullptr; > > CameraManager::CameraManager() > - : Extensible(new CameraManager::Private(this)) > + : Extensible(new CameraManager::Private()) > { > if (self_) > LOG(Camera, Fatal) > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > index 40bf64b0a4fe..48d14b31f68d 100644 > --- a/src/libcamera/framebuffer.cpp > +++ b/src/libcamera/framebuffer.cpp > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer) > * \brief Array of per-plane metadata > */ > > -FrameBuffer::Private::Private(FrameBuffer *buffer) > - : Extensible::Private(buffer), request_(nullptr) > +FrameBuffer::Private::Private() > + : request_(nullptr) > { > } > > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer) > * \param[in] cookie Cookie > */ > FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > - : Extensible(new Private(this)), planes_(planes), cookie_(cookie) > + : Extensible(new Private()), planes_(planes), cookie_(cookie) > { > } > > -- > Regards, > > Laurent Pinchart >
Hi Niklas, On Sat, Jul 24, 2021 at 08:55:17AM +0200, Niklas Söderlund wrote: > On 2021-07-23 07:00:22 +0300, Laurent Pinchart wrote: > > The Extensible and Extensible::Private classes contain pointers to each > > other. These pointers are initialized in the respective class's > > constructor, by passing a pointer to the other class to each > > constructor. This particular construct reduces the flexibility of the > > Extensible pattern, as the Private class instance has to be allocated > > and constructed in the members initializer list of the Extensible > > class's constructor. It is thus impossible to perform any operation on > > the Private class between its construction and the construction of the > > Extensible class, or to subclass the Private class without subclassing > > the Extensible class. > > > > To make the design pattern more flexible, don't pass the pointer to the > > Extensible class to the Private class's constructor, but initialize the > > pointer manually in the Extensible class's constructor. This requires a > > const_cast as the o_ member of the Private class is const. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/class.h | 3 ++- > > include/libcamera/internal/framebuffer.h | 2 +- > > src/android/camera_hal_config.cpp | 7 +++---- > > src/android/mm/cros_camera_buffer.cpp | 4 ++-- > > src/android/mm/generic_camera_buffer.cpp | 3 +-- > > src/libcamera/base/class.cpp | 6 +++--- > > src/libcamera/camera.cpp | 10 +++++----- > > src/libcamera/camera_manager.cpp | 8 ++++---- > > src/libcamera/framebuffer.cpp | 6 +++--- > > 9 files changed, 24 insertions(+), 25 deletions(-) > > > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > > index 9c7f0f2e6e27..c9d9cd949ca1 100644 > > --- a/include/libcamera/base/class.h > > +++ b/include/libcamera/base/class.h > > @@ -64,7 +64,7 @@ public: > > class Private > > { > > public: > > - Private(Extensible *o); > > + Private(); > > virtual ~Private(); > > > > #ifndef __DOXYGEN__ > > @@ -82,6 +82,7 @@ public: > > #endif > > > > private: > > + friend class Extensible; > > I don't like friends, but here it makes sens. I would add a comment here > explaining why the friend statement is needed so it can be removed in > the future if someone can think of a different design pattern. With this > fixed, I'll add the following comment: /* To initialize o_ from Extensible. */ friend class Extensible; Extensible *const o_; > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > Extensible *const o_; > > }; > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h > > index a11e895d9b88..8c187adf70c7 100644 > > --- a/include/libcamera/internal/framebuffer.h > > +++ b/include/libcamera/internal/framebuffer.h > > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private > > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > public: > > - Private(FrameBuffer *buffer); > > + Private(); > > > > void setRequest(Request *request) { request_ = request; } > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > index 833cf4ba98a9..bbbb1410b52c 100644 > > --- a/src/android/camera_hal_config.cpp > > +++ b/src/android/camera_hal_config.cpp > > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private > > LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig) > > > > public: > > - Private(CameraHalConfig *halConfig); > > + Private(); > > > > int parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras); > > > > @@ -50,8 +50,7 @@ private: > > std::map<std::string, CameraConfigData> *cameras_; > > }; > > > > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig) > > - : Extensible::Private(halConfig) > > +CameraHalConfig::Private::Private() > > { > > } > > > > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh, > > } > > > > CameraHalConfig::CameraHalConfig() > > - : Extensible(new Private(this)), exists_(false), valid_(false) > > + : Extensible(new Private()), exists_(false), valid_(false) > > { > > parseConfigurationFile(); > > } > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > > index bb55b95e3a39..437502fb8276 100644 > > --- a/src/android/mm/cros_camera_buffer.cpp > > +++ b/src/android/mm/cros_camera_buffer.cpp > > @@ -47,8 +47,8 @@ private: > > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > > buffer_handle_t camera3Buffer, > > [[maybe_unused]] int flags) > > - : Extensible::Private(cameraBuffer), handle_(camera3Buffer), > > - numPlanes_(0), valid_(false), registered_(false) > > + : handle_(camera3Buffer), numPlanes_(0), valid_(false), > > + registered_(false) > > { > > bufferManager_ = cros::CameraBufferManager::GetInstance(); > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp > > index 166be36efd5b..2a4b77ea5236 100644 > > --- a/src/android/mm/generic_camera_buffer.cpp > > +++ b/src/android/mm/generic_camera_buffer.cpp > > @@ -34,9 +34,8 @@ public: > > size_t jpegBufferSize(size_t maxJpegBufferSize) const; > > }; > > > > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > > buffer_handle_t camera3Buffer, int flags) > > - : Extensible::Private(cameraBuffer) > > { > > maps_.reserve(camera3Buffer->numFds); > > error_ = 0; > > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp > > index d4f0ac64ad48..d24043c20e55 100644 > > --- a/src/libcamera/base/class.cpp > > +++ b/src/libcamera/base/class.cpp > > @@ -151,6 +151,7 @@ namespace libcamera { > > Extensible::Extensible(Extensible::Private *d) > > : d_(d) > > { > > + *const_cast<Extensible **>(&d_->o_) = this; > > } > > > > /** > > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d) > > > > /** > > * \brief Construct an instance of an Extensible class private data > > - * \param[in] o Pointer to the public class object > > */ > > -Extensible::Private::Private(Extensible *o) > > - : o_(o) > > +Extensible::Private::Private() > > + : o_(nullptr) > > { > > } > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index c8858e71d105..c126b49290ce 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -344,7 +344,7 @@ public: > > CameraRunning, > > }; > > > > - Private(Camera *camera, PipelineHandler *pipe, const std::string &id, > > + Private(PipelineHandler *pipe, const std::string &id, > > const std::set<Stream *> &streams); > > ~Private(); > > > > @@ -368,11 +368,11 @@ private: > > std::atomic<State> state_; > > }; > > > > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe, > > +Camera::Private::Private(PipelineHandler *pipe, > > const std::string &id, > > const std::set<Stream *> &streams) > > - : Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id), > > - streams_(streams), disconnected_(false), state_(CameraAvailable) > > + : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > > + disconnected_(false), state_(CameraAvailable) > > { > > } > > > > @@ -632,7 +632,7 @@ const std::string &Camera::id() const > > > > Camera::Camera(PipelineHandler *pipe, const std::string &id, > > const std::set<Stream *> &streams) > > - : Extensible(new Private(this, pipe, id, streams)) > > + : Extensible(new Private(pipe, id, streams)) > > { > > } > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index 1c79308ad4b5..384a574f2baa 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread > > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > > > public: > > - Private(CameraManager *cm); > > + Private(); > > > > int start(); > > void addCamera(std::shared_ptr<Camera> camera, > > @@ -74,8 +74,8 @@ private: > > ProcessManager processManager_; > > }; > > > > -CameraManager::Private::Private(CameraManager *cm) > > - : Extensible::Private(cm), initialized_(false) > > +CameraManager::Private::Private() > > + : initialized_(false) > > { > > } > > > > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera) > > CameraManager *CameraManager::self_ = nullptr; > > > > CameraManager::CameraManager() > > - : Extensible(new CameraManager::Private(this)) > > + : Extensible(new CameraManager::Private()) > > { > > if (self_) > > LOG(Camera, Fatal) > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > > index 40bf64b0a4fe..48d14b31f68d 100644 > > --- a/src/libcamera/framebuffer.cpp > > +++ b/src/libcamera/framebuffer.cpp > > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer) > > * \brief Array of per-plane metadata > > */ > > > > -FrameBuffer::Private::Private(FrameBuffer *buffer) > > - : Extensible::Private(buffer), request_(nullptr) > > +FrameBuffer::Private::Private() > > + : request_(nullptr) > > { > > } > > > > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer) > > * \param[in] cookie Cookie > > */ > > FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > - : Extensible(new Private(this)), planes_(planes), cookie_(cookie) > > + : Extensible(new Private()), planes_(planes), cookie_(cookie) > > { > > } > >
Hi Laurent, On Fri, Jul 23, 2021 at 07:00:22AM +0300, Laurent Pinchart wrote: > The Extensible and Extensible::Private classes contain pointers to each > other. These pointers are initialized in the respective class's > constructor, by passing a pointer to the other class to each > constructor. This particular construct reduces the flexibility of the > Extensible pattern, as the Private class instance has to be allocated > and constructed in the members initializer list of the Extensible > class's constructor. It is thus impossible to perform any operation on > the Private class between its construction and the construction of the > Extensible class, or to subclass the Private class without subclassing Am I wrong or the main obstacle here is that is impossible to construct Private instances seprately from Public ones ? > the Extensible class. > > To make the design pattern more flexible, don't pass the pointer to the > Extensible class to the Private class's constructor, but initialize the > pointer manually in the Extensible class's constructor. This requires a > const_cast as the o_ member of the Private class is const. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/class.h | 3 ++- > include/libcamera/internal/framebuffer.h | 2 +- > src/android/camera_hal_config.cpp | 7 +++---- > src/android/mm/cros_camera_buffer.cpp | 4 ++-- > src/android/mm/generic_camera_buffer.cpp | 3 +-- > src/libcamera/base/class.cpp | 6 +++--- > src/libcamera/camera.cpp | 10 +++++----- > src/libcamera/camera_manager.cpp | 8 ++++---- > src/libcamera/framebuffer.cpp | 6 +++--- > 9 files changed, 24 insertions(+), 25 deletions(-) > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > index 9c7f0f2e6e27..c9d9cd949ca1 100644 > --- a/include/libcamera/base/class.h > +++ b/include/libcamera/base/class.h > @@ -64,7 +64,7 @@ public: > class Private > { > public: > - Private(Extensible *o); > + Private(); > virtual ~Private(); > > #ifndef __DOXYGEN__ > @@ -82,6 +82,7 @@ public: > #endif > > private: > + friend class Extensible; > Extensible *const o_; > }; > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h > index a11e895d9b88..8c187adf70c7 100644 > --- a/include/libcamera/internal/framebuffer.h > +++ b/include/libcamera/internal/framebuffer.h > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > public: > - Private(FrameBuffer *buffer); > + Private(); > > void setRequest(Request *request) { request_ = request; } > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > index 833cf4ba98a9..bbbb1410b52c 100644 > --- a/src/android/camera_hal_config.cpp > +++ b/src/android/camera_hal_config.cpp > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private > LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig) > > public: > - Private(CameraHalConfig *halConfig); > + Private(); > > int parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras); > > @@ -50,8 +50,7 @@ private: > std::map<std::string, CameraConfigData> *cameras_; > }; > > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig) > - : Extensible::Private(halConfig) > +CameraHalConfig::Private::Private() > { > } > > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh, > } > > CameraHalConfig::CameraHalConfig() > - : Extensible(new Private(this)), exists_(false), valid_(false) > + : Extensible(new Private()), exists_(false), valid_(false) > { > parseConfigurationFile(); > } > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > index bb55b95e3a39..437502fb8276 100644 > --- a/src/android/mm/cros_camera_buffer.cpp > +++ b/src/android/mm/cros_camera_buffer.cpp > @@ -47,8 +47,8 @@ private: > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > buffer_handle_t camera3Buffer, > [[maybe_unused]] int flags) > - : Extensible::Private(cameraBuffer), handle_(camera3Buffer), > - numPlanes_(0), valid_(false), registered_(false) > + : handle_(camera3Buffer), numPlanes_(0), valid_(false), > + registered_(false) > { > bufferManager_ = cros::CameraBufferManager::GetInstance(); > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp > index 166be36efd5b..2a4b77ea5236 100644 > --- a/src/android/mm/generic_camera_buffer.cpp > +++ b/src/android/mm/generic_camera_buffer.cpp > @@ -34,9 +34,8 @@ public: > size_t jpegBufferSize(size_t maxJpegBufferSize) const; > }; > > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > buffer_handle_t camera3Buffer, int flags) > - : Extensible::Private(cameraBuffer) > { > maps_.reserve(camera3Buffer->numFds); > error_ = 0; > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp > index d4f0ac64ad48..d24043c20e55 100644 > --- a/src/libcamera/base/class.cpp > +++ b/src/libcamera/base/class.cpp > @@ -151,6 +151,7 @@ namespace libcamera { > Extensible::Extensible(Extensible::Private *d) > : d_(d) > { > + *const_cast<Extensible **>(&d_->o_) = this; Do you think there's much value in Extensible *const o_; as it is private and only Extensible can access it ? It will save the const_cast<> here (but according to my compiler will require a new one in the "T * _o()" function, so Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > } > > /** > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d) > > /** > * \brief Construct an instance of an Extensible class private data > - * \param[in] o Pointer to the public class object > */ > -Extensible::Private::Private(Extensible *o) > - : o_(o) > +Extensible::Private::Private() > + : o_(nullptr) > { > } > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index c8858e71d105..c126b49290ce 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -344,7 +344,7 @@ public: > CameraRunning, > }; > > - Private(Camera *camera, PipelineHandler *pipe, const std::string &id, > + Private(PipelineHandler *pipe, const std::string &id, > const std::set<Stream *> &streams); > ~Private(); > > @@ -368,11 +368,11 @@ private: > std::atomic<State> state_; > }; > > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe, > +Camera::Private::Private(PipelineHandler *pipe, > const std::string &id, > const std::set<Stream *> &streams) > - : Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id), > - streams_(streams), disconnected_(false), state_(CameraAvailable) > + : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > + disconnected_(false), state_(CameraAvailable) > { > } > > @@ -632,7 +632,7 @@ const std::string &Camera::id() const > > Camera::Camera(PipelineHandler *pipe, const std::string &id, > const std::set<Stream *> &streams) > - : Extensible(new Private(this, pipe, id, streams)) > + : Extensible(new Private(pipe, id, streams)) > { > } > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 1c79308ad4b5..384a574f2baa 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > public: > - Private(CameraManager *cm); > + Private(); > > int start(); > void addCamera(std::shared_ptr<Camera> camera, > @@ -74,8 +74,8 @@ private: > ProcessManager processManager_; > }; > > -CameraManager::Private::Private(CameraManager *cm) > - : Extensible::Private(cm), initialized_(false) > +CameraManager::Private::Private() > + : initialized_(false) > { > } > > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera) > CameraManager *CameraManager::self_ = nullptr; > > CameraManager::CameraManager() > - : Extensible(new CameraManager::Private(this)) > + : Extensible(new CameraManager::Private()) > { > if (self_) > LOG(Camera, Fatal) > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > index 40bf64b0a4fe..48d14b31f68d 100644 > --- a/src/libcamera/framebuffer.cpp > +++ b/src/libcamera/framebuffer.cpp > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer) > * \brief Array of per-plane metadata > */ > > -FrameBuffer::Private::Private(FrameBuffer *buffer) > - : Extensible::Private(buffer), request_(nullptr) > +FrameBuffer::Private::Private() > + : request_(nullptr) > { > } > > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer) > * \param[in] cookie Cookie > */ > FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > - : Extensible(new Private(this)), planes_(planes), cookie_(cookie) > + : Extensible(new Private()), planes_(planes), cookie_(cookie) > { > } > > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Thu, Jul 29, 2021 at 10:23:18PM +0200, Jacopo Mondi wrote: > On Fri, Jul 23, 2021 at 07:00:22AM +0300, Laurent Pinchart wrote: > > The Extensible and Extensible::Private classes contain pointers to each > > other. These pointers are initialized in the respective class's > > constructor, by passing a pointer to the other class to each > > constructor. This particular construct reduces the flexibility of the > > Extensible pattern, as the Private class instance has to be allocated > > and constructed in the members initializer list of the Extensible > > class's constructor. It is thus impossible to perform any operation on > > the Private class between its construction and the construction of the > > Extensible class, or to subclass the Private class without subclassing > > Am I wrong or the main obstacle here is that is impossible to > construct Private instances seprately from Public ones ? Yes, before this patch both the public and private classes have to be constructed together, with the constructor of the public class using, in its member initializer list, : Extensible(new Private(this)) The very tight coupling makes it impossible to use the Private class to store private data ahead of construction of the public class. > > the Extensible class. > > > > To make the design pattern more flexible, don't pass the pointer to the > > Extensible class to the Private class's constructor, but initialize the > > pointer manually in the Extensible class's constructor. This requires a > > const_cast as the o_ member of the Private class is const. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/class.h | 3 ++- > > include/libcamera/internal/framebuffer.h | 2 +- > > src/android/camera_hal_config.cpp | 7 +++---- > > src/android/mm/cros_camera_buffer.cpp | 4 ++-- > > src/android/mm/generic_camera_buffer.cpp | 3 +-- > > src/libcamera/base/class.cpp | 6 +++--- > > src/libcamera/camera.cpp | 10 +++++----- > > src/libcamera/camera_manager.cpp | 8 ++++---- > > src/libcamera/framebuffer.cpp | 6 +++--- > > 9 files changed, 24 insertions(+), 25 deletions(-) > > > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > > index 9c7f0f2e6e27..c9d9cd949ca1 100644 > > --- a/include/libcamera/base/class.h > > +++ b/include/libcamera/base/class.h > > @@ -64,7 +64,7 @@ public: > > class Private > > { > > public: > > - Private(Extensible *o); > > + Private(); > > virtual ~Private(); > > > > #ifndef __DOXYGEN__ > > @@ -82,6 +82,7 @@ public: > > #endif > > > > private: > > + friend class Extensible; > > Extensible *const o_; > > }; > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h > > index a11e895d9b88..8c187adf70c7 100644 > > --- a/include/libcamera/internal/framebuffer.h > > +++ b/include/libcamera/internal/framebuffer.h > > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private > > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > public: > > - Private(FrameBuffer *buffer); > > + Private(); > > > > void setRequest(Request *request) { request_ = request; } > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > index 833cf4ba98a9..bbbb1410b52c 100644 > > --- a/src/android/camera_hal_config.cpp > > +++ b/src/android/camera_hal_config.cpp > > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private > > LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig) > > > > public: > > - Private(CameraHalConfig *halConfig); > > + Private(); > > > > int parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras); > > > > @@ -50,8 +50,7 @@ private: > > std::map<std::string, CameraConfigData> *cameras_; > > }; > > > > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig) > > - : Extensible::Private(halConfig) > > +CameraHalConfig::Private::Private() > > { > > } > > > > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh, > > } > > > > CameraHalConfig::CameraHalConfig() > > - : Extensible(new Private(this)), exists_(false), valid_(false) > > + : Extensible(new Private()), exists_(false), valid_(false) > > { > > parseConfigurationFile(); > > } > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > > index bb55b95e3a39..437502fb8276 100644 > > --- a/src/android/mm/cros_camera_buffer.cpp > > +++ b/src/android/mm/cros_camera_buffer.cpp > > @@ -47,8 +47,8 @@ private: > > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > > buffer_handle_t camera3Buffer, > > [[maybe_unused]] int flags) > > - : Extensible::Private(cameraBuffer), handle_(camera3Buffer), > > - numPlanes_(0), valid_(false), registered_(false) > > + : handle_(camera3Buffer), numPlanes_(0), valid_(false), > > + registered_(false) > > { > > bufferManager_ = cros::CameraBufferManager::GetInstance(); > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp > > index 166be36efd5b..2a4b77ea5236 100644 > > --- a/src/android/mm/generic_camera_buffer.cpp > > +++ b/src/android/mm/generic_camera_buffer.cpp > > @@ -34,9 +34,8 @@ public: > > size_t jpegBufferSize(size_t maxJpegBufferSize) const; > > }; > > > > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > > buffer_handle_t camera3Buffer, int flags) > > - : Extensible::Private(cameraBuffer) > > { > > maps_.reserve(camera3Buffer->numFds); > > error_ = 0; > > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp > > index d4f0ac64ad48..d24043c20e55 100644 > > --- a/src/libcamera/base/class.cpp > > +++ b/src/libcamera/base/class.cpp > > @@ -151,6 +151,7 @@ namespace libcamera { > > Extensible::Extensible(Extensible::Private *d) > > : d_(d) > > { > > + *const_cast<Extensible **>(&d_->o_) = this; > > Do you think there's much value in > Extensible *const o_; > > as it is private and only Extensible can access it ? Do you mean much value in keeping it const ? Or much value in having it at all ? If it's the latter, I'm not sure to understand the question. If it's the former, I've considered dropping the const qualifier, but it's nice to leverage the compiler to ensure the pointer isn't modified when it shouldn't. > It will save the const_cast<> here (but according to my compiler will > require a new one in the "T * _o()" function, so > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > } > > > > /** > > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d) > > > > /** > > * \brief Construct an instance of an Extensible class private data > > - * \param[in] o Pointer to the public class object > > */ > > -Extensible::Private::Private(Extensible *o) > > - : o_(o) > > +Extensible::Private::Private() > > + : o_(nullptr) > > { > > } > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index c8858e71d105..c126b49290ce 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -344,7 +344,7 @@ public: > > CameraRunning, > > }; > > > > - Private(Camera *camera, PipelineHandler *pipe, const std::string &id, > > + Private(PipelineHandler *pipe, const std::string &id, > > const std::set<Stream *> &streams); > > ~Private(); > > > > @@ -368,11 +368,11 @@ private: > > std::atomic<State> state_; > > }; > > > > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe, > > +Camera::Private::Private(PipelineHandler *pipe, > > const std::string &id, > > const std::set<Stream *> &streams) > > - : Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id), > > - streams_(streams), disconnected_(false), state_(CameraAvailable) > > + : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > > + disconnected_(false), state_(CameraAvailable) > > { > > } > > > > @@ -632,7 +632,7 @@ const std::string &Camera::id() const > > > > Camera::Camera(PipelineHandler *pipe, const std::string &id, > > const std::set<Stream *> &streams) > > - : Extensible(new Private(this, pipe, id, streams)) > > + : Extensible(new Private(pipe, id, streams)) > > { > > } > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index 1c79308ad4b5..384a574f2baa 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread > > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > > > public: > > - Private(CameraManager *cm); > > + Private(); > > > > int start(); > > void addCamera(std::shared_ptr<Camera> camera, > > @@ -74,8 +74,8 @@ private: > > ProcessManager processManager_; > > }; > > > > -CameraManager::Private::Private(CameraManager *cm) > > - : Extensible::Private(cm), initialized_(false) > > +CameraManager::Private::Private() > > + : initialized_(false) > > { > > } > > > > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera) > > CameraManager *CameraManager::self_ = nullptr; > > > > CameraManager::CameraManager() > > - : Extensible(new CameraManager::Private(this)) > > + : Extensible(new CameraManager::Private()) > > { > > if (self_) > > LOG(Camera, Fatal) > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > > index 40bf64b0a4fe..48d14b31f68d 100644 > > --- a/src/libcamera/framebuffer.cpp > > +++ b/src/libcamera/framebuffer.cpp > > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer) > > * \brief Array of per-plane metadata > > */ > > > > -FrameBuffer::Private::Private(FrameBuffer *buffer) > > - : Extensible::Private(buffer), request_(nullptr) > > +FrameBuffer::Private::Private() > > + : request_(nullptr) > > { > > } > > > > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer) > > * \param[in] cookie Cookie > > */ > > FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > - : Extensible(new Private(this)), planes_(planes), cookie_(cookie) > > + : Extensible(new Private()), planes_(planes), cookie_(cookie) > > { > > } > >
Hi Laurent, On Fri, Jul 30, 2021 at 05:33:21AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Jul 29, 2021 at 10:23:18PM +0200, Jacopo Mondi wrote: > > On Fri, Jul 23, 2021 at 07:00:22AM +0300, Laurent Pinchart wrote: > > > The Extensible and Extensible::Private classes contain pointers to each > > > other. These pointers are initialized in the respective class's > > > constructor, by passing a pointer to the other class to each > > > constructor. This particular construct reduces the flexibility of the > > > Extensible pattern, as the Private class instance has to be allocated > > > and constructed in the members initializer list of the Extensible > > > class's constructor. It is thus impossible to perform any operation on > > > the Private class between its construction and the construction of the > > > Extensible class, or to subclass the Private class without subclassing > > > > Am I wrong or the main obstacle here is that is impossible to > > construct Private instances seprately from Public ones ? > > Yes, before this patch both the public and private classes have to be > constructed together, with the constructor of the public class using, in > its member initializer list, > > : Extensible(new Private(this)) > > The very tight coupling makes it impossible to use the Private class to > store private data ahead of construction of the public class. > > > > the Extensible class. > > > > > > To make the design pattern more flexible, don't pass the pointer to the > > > Extensible class to the Private class's constructor, but initialize the > > > pointer manually in the Extensible class's constructor. This requires a > > > const_cast as the o_ member of the Private class is const. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/base/class.h | 3 ++- > > > include/libcamera/internal/framebuffer.h | 2 +- > > > src/android/camera_hal_config.cpp | 7 +++---- > > > src/android/mm/cros_camera_buffer.cpp | 4 ++-- > > > src/android/mm/generic_camera_buffer.cpp | 3 +-- > > > src/libcamera/base/class.cpp | 6 +++--- > > > src/libcamera/camera.cpp | 10 +++++----- > > > src/libcamera/camera_manager.cpp | 8 ++++---- > > > src/libcamera/framebuffer.cpp | 6 +++--- > > > 9 files changed, 24 insertions(+), 25 deletions(-) > > > > > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > > > index 9c7f0f2e6e27..c9d9cd949ca1 100644 > > > --- a/include/libcamera/base/class.h > > > +++ b/include/libcamera/base/class.h > > > @@ -64,7 +64,7 @@ public: > > > class Private > > > { > > > public: > > > - Private(Extensible *o); > > > + Private(); > > > virtual ~Private(); > > > > > > #ifndef __DOXYGEN__ > > > @@ -82,6 +82,7 @@ public: > > > #endif > > > > > > private: > > > + friend class Extensible; > > > Extensible *const o_; > > > }; > > > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h > > > index a11e895d9b88..8c187adf70c7 100644 > > > --- a/include/libcamera/internal/framebuffer.h > > > +++ b/include/libcamera/internal/framebuffer.h > > > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private > > > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) > > > > > > public: > > > - Private(FrameBuffer *buffer); > > > + Private(); > > > > > > void setRequest(Request *request) { request_ = request; } > > > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > > index 833cf4ba98a9..bbbb1410b52c 100644 > > > --- a/src/android/camera_hal_config.cpp > > > +++ b/src/android/camera_hal_config.cpp > > > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private > > > LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig) > > > > > > public: > > > - Private(CameraHalConfig *halConfig); > > > + Private(); > > > > > > int parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras); > > > > > > @@ -50,8 +50,7 @@ private: > > > std::map<std::string, CameraConfigData> *cameras_; > > > }; > > > > > > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig) > > > - : Extensible::Private(halConfig) > > > +CameraHalConfig::Private::Private() > > > { > > > } > > > > > > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh, > > > } > > > > > > CameraHalConfig::CameraHalConfig() > > > - : Extensible(new Private(this)), exists_(false), valid_(false) > > > + : Extensible(new Private()), exists_(false), valid_(false) > > > { > > > parseConfigurationFile(); > > > } > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > > > index bb55b95e3a39..437502fb8276 100644 > > > --- a/src/android/mm/cros_camera_buffer.cpp > > > +++ b/src/android/mm/cros_camera_buffer.cpp > > > @@ -47,8 +47,8 @@ private: > > > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > > > buffer_handle_t camera3Buffer, > > > [[maybe_unused]] int flags) > > > - : Extensible::Private(cameraBuffer), handle_(camera3Buffer), > > > - numPlanes_(0), valid_(false), registered_(false) > > > + : handle_(camera3Buffer), numPlanes_(0), valid_(false), > > > + registered_(false) > > > { > > > bufferManager_ = cros::CameraBufferManager::GetInstance(); > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp > > > index 166be36efd5b..2a4b77ea5236 100644 > > > --- a/src/android/mm/generic_camera_buffer.cpp > > > +++ b/src/android/mm/generic_camera_buffer.cpp > > > @@ -34,9 +34,8 @@ public: > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const; > > > }; > > > > > > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, > > > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > > > buffer_handle_t camera3Buffer, int flags) > > > - : Extensible::Private(cameraBuffer) > > > { > > > maps_.reserve(camera3Buffer->numFds); > > > error_ = 0; > > > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp > > > index d4f0ac64ad48..d24043c20e55 100644 > > > --- a/src/libcamera/base/class.cpp > > > +++ b/src/libcamera/base/class.cpp > > > @@ -151,6 +151,7 @@ namespace libcamera { > > > Extensible::Extensible(Extensible::Private *d) > > > : d_(d) > > > { > > > + *const_cast<Extensible **>(&d_->o_) = this; > > > > Do you think there's much value in > > Extensible *const o_; > > > > as it is private and only Extensible can access it ? > > Do you mean much value in keeping it const ? Or much value in having it > at all ? If it's the latter, I'm not sure to understand the question. If > it's the former, I've considered dropping the const qualifier, but it's > nice to leverage the compiler to ensure the pointer isn't modified when > it shouldn't. I meant the const qualifier :) Only the Extensible derived class could access o_ and as the newly introduced const_cast shows, they can if they really want to. But ofc it makes it harder to do so, so I guess it has value. > > > It will save the const_cast<> here (but according to my compiler will > > require a new one in the "T * _o()" function, so > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > } > > > > > > /** > > > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d) > > > > > > /** > > > * \brief Construct an instance of an Extensible class private data > > > - * \param[in] o Pointer to the public class object > > > */ > > > -Extensible::Private::Private(Extensible *o) > > > - : o_(o) > > > +Extensible::Private::Private() > > > + : o_(nullptr) > > > { > > > } > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index c8858e71d105..c126b49290ce 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -344,7 +344,7 @@ public: > > > CameraRunning, > > > }; > > > > > > - Private(Camera *camera, PipelineHandler *pipe, const std::string &id, > > > + Private(PipelineHandler *pipe, const std::string &id, > > > const std::set<Stream *> &streams); > > > ~Private(); > > > > > > @@ -368,11 +368,11 @@ private: > > > std::atomic<State> state_; > > > }; > > > > > > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe, > > > +Camera::Private::Private(PipelineHandler *pipe, > > > const std::string &id, > > > const std::set<Stream *> &streams) > > > - : Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id), > > > - streams_(streams), disconnected_(false), state_(CameraAvailable) > > > + : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > > > + disconnected_(false), state_(CameraAvailable) > > > { > > > } > > > > > > @@ -632,7 +632,7 @@ const std::string &Camera::id() const > > > > > > Camera::Camera(PipelineHandler *pipe, const std::string &id, > > > const std::set<Stream *> &streams) > > > - : Extensible(new Private(this, pipe, id, streams)) > > > + : Extensible(new Private(pipe, id, streams)) > > > { > > > } > > > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > > index 1c79308ad4b5..384a574f2baa 100644 > > > --- a/src/libcamera/camera_manager.cpp > > > +++ b/src/libcamera/camera_manager.cpp > > > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread > > > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > > > > > public: > > > - Private(CameraManager *cm); > > > + Private(); > > > > > > int start(); > > > void addCamera(std::shared_ptr<Camera> camera, > > > @@ -74,8 +74,8 @@ private: > > > ProcessManager processManager_; > > > }; > > > > > > -CameraManager::Private::Private(CameraManager *cm) > > > - : Extensible::Private(cm), initialized_(false) > > > +CameraManager::Private::Private() > > > + : initialized_(false) > > > { > > > } > > > > > > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera) > > > CameraManager *CameraManager::self_ = nullptr; > > > > > > CameraManager::CameraManager() > > > - : Extensible(new CameraManager::Private(this)) > > > + : Extensible(new CameraManager::Private()) > > > { > > > if (self_) > > > LOG(Camera, Fatal) > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > > > index 40bf64b0a4fe..48d14b31f68d 100644 > > > --- a/src/libcamera/framebuffer.cpp > > > +++ b/src/libcamera/framebuffer.cpp > > > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer) > > > * \brief Array of per-plane metadata > > > */ > > > > > > -FrameBuffer::Private::Private(FrameBuffer *buffer) > > > - : Extensible::Private(buffer), request_(nullptr) > > > +FrameBuffer::Private::Private() > > > + : request_(nullptr) > > > { > > > } > > > > > > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer) > > > * \param[in] cookie Cookie > > > */ > > > FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > > - : Extensible(new Private(this)), planes_(planes), cookie_(cookie) > > > + : Extensible(new Private()), planes_(planes), cookie_(cookie) > > > { > > > } > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h index 9c7f0f2e6e27..c9d9cd949ca1 100644 --- a/include/libcamera/base/class.h +++ b/include/libcamera/base/class.h @@ -64,7 +64,7 @@ public: class Private { public: - Private(Extensible *o); + Private(); virtual ~Private(); #ifndef __DOXYGEN__ @@ -82,6 +82,7 @@ public: #endif private: + friend class Extensible; Extensible *const o_; }; diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h index a11e895d9b88..8c187adf70c7 100644 --- a/include/libcamera/internal/framebuffer.h +++ b/include/libcamera/internal/framebuffer.h @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private LIBCAMERA_DECLARE_PUBLIC(FrameBuffer) public: - Private(FrameBuffer *buffer); + Private(); void setRequest(Request *request) { request_ = request; } diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp index 833cf4ba98a9..bbbb1410b52c 100644 --- a/src/android/camera_hal_config.cpp +++ b/src/android/camera_hal_config.cpp @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig) public: - Private(CameraHalConfig *halConfig); + Private(); int parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras); @@ -50,8 +50,7 @@ private: std::map<std::string, CameraConfigData> *cameras_; }; -CameraHalConfig::Private::Private(CameraHalConfig *halConfig) - : Extensible::Private(halConfig) +CameraHalConfig::Private::Private() { } @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh, } CameraHalConfig::CameraHalConfig() - : Extensible(new Private(this)), exists_(false), valid_(false) + : Extensible(new Private()), exists_(false), valid_(false) { parseConfigurationFile(); } diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp index bb55b95e3a39..437502fb8276 100644 --- a/src/android/mm/cros_camera_buffer.cpp +++ b/src/android/mm/cros_camera_buffer.cpp @@ -47,8 +47,8 @@ private: CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer, [[maybe_unused]] int flags) - : Extensible::Private(cameraBuffer), handle_(camera3Buffer), - numPlanes_(0), valid_(false), registered_(false) + : handle_(camera3Buffer), numPlanes_(0), valid_(false), + registered_(false) { bufferManager_ = cros::CameraBufferManager::GetInstance(); diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp index 166be36efd5b..2a4b77ea5236 100644 --- a/src/android/mm/generic_camera_buffer.cpp +++ b/src/android/mm/generic_camera_buffer.cpp @@ -34,9 +34,8 @@ public: size_t jpegBufferSize(size_t maxJpegBufferSize) const; }; -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer, +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer, int flags) - : Extensible::Private(cameraBuffer) { maps_.reserve(camera3Buffer->numFds); error_ = 0; diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp index d4f0ac64ad48..d24043c20e55 100644 --- a/src/libcamera/base/class.cpp +++ b/src/libcamera/base/class.cpp @@ -151,6 +151,7 @@ namespace libcamera { Extensible::Extensible(Extensible::Private *d) : d_(d) { + *const_cast<Extensible **>(&d_->o_) = this; } /** @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d) /** * \brief Construct an instance of an Extensible class private data - * \param[in] o Pointer to the public class object */ -Extensible::Private::Private(Extensible *o) - : o_(o) +Extensible::Private::Private() + : o_(nullptr) { } diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index c8858e71d105..c126b49290ce 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -344,7 +344,7 @@ public: CameraRunning, }; - Private(Camera *camera, PipelineHandler *pipe, const std::string &id, + Private(PipelineHandler *pipe, const std::string &id, const std::set<Stream *> &streams); ~Private(); @@ -368,11 +368,11 @@ private: std::atomic<State> state_; }; -Camera::Private::Private(Camera *camera, PipelineHandler *pipe, +Camera::Private::Private(PipelineHandler *pipe, const std::string &id, const std::set<Stream *> &streams) - : Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id), - streams_(streams), disconnected_(false), state_(CameraAvailable) + : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), + disconnected_(false), state_(CameraAvailable) { } @@ -632,7 +632,7 @@ const std::string &Camera::id() const Camera::Camera(PipelineHandler *pipe, const std::string &id, const std::set<Stream *> &streams) - : Extensible(new Private(this, pipe, id, streams)) + : Extensible(new Private(pipe, id, streams)) { } diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 1c79308ad4b5..384a574f2baa 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread LIBCAMERA_DECLARE_PUBLIC(CameraManager) public: - Private(CameraManager *cm); + Private(); int start(); void addCamera(std::shared_ptr<Camera> camera, @@ -74,8 +74,8 @@ private: ProcessManager processManager_; }; -CameraManager::Private::Private(CameraManager *cm) - : Extensible::Private(cm), initialized_(false) +CameraManager::Private::Private() + : initialized_(false) { } @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera) CameraManager *CameraManager::self_ = nullptr; CameraManager::CameraManager() - : Extensible(new CameraManager::Private(this)) + : Extensible(new CameraManager::Private()) { if (self_) LOG(Camera, Fatal) diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp index 40bf64b0a4fe..48d14b31f68d 100644 --- a/src/libcamera/framebuffer.cpp +++ b/src/libcamera/framebuffer.cpp @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer) * \brief Array of per-plane metadata */ -FrameBuffer::Private::Private(FrameBuffer *buffer) - : Extensible::Private(buffer), request_(nullptr) +FrameBuffer::Private::Private() + : request_(nullptr) { } @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer) * \param[in] cookie Cookie */ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) - : Extensible(new Private(this)), planes_(planes), cookie_(cookie) + : Extensible(new Private()), planes_(planes), cookie_(cookie) { }
The Extensible and Extensible::Private classes contain pointers to each other. These pointers are initialized in the respective class's constructor, by passing a pointer to the other class to each constructor. This particular construct reduces the flexibility of the Extensible pattern, as the Private class instance has to be allocated and constructed in the members initializer list of the Extensible class's constructor. It is thus impossible to perform any operation on the Private class between its construction and the construction of the Extensible class, or to subclass the Private class without subclassing the Extensible class. To make the design pattern more flexible, don't pass the pointer to the Extensible class to the Private class's constructor, but initialize the pointer manually in the Extensible class's constructor. This requires a const_cast as the o_ member of the Private class is const. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/base/class.h | 3 ++- include/libcamera/internal/framebuffer.h | 2 +- src/android/camera_hal_config.cpp | 7 +++---- src/android/mm/cros_camera_buffer.cpp | 4 ++-- src/android/mm/generic_camera_buffer.cpp | 3 +-- src/libcamera/base/class.cpp | 6 +++--- src/libcamera/camera.cpp | 10 +++++----- src/libcamera/camera_manager.cpp | 8 ++++---- src/libcamera/framebuffer.cpp | 6 +++--- 9 files changed, 24 insertions(+), 25 deletions(-)