[libcamera-devel] [RFC PATCH 03/17] libcamera: base: class: Don't pass Extensible pointer to Private constructor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jul 24 20:53:35 CEST 2021
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 at 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 at 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
More information about the libcamera-devel
mailing list