[libcamera-devel] [RFC PATCH 03/17] libcamera: base: class: Don't pass Extensible pointer to Private constructor
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Jul 24 08:55:17 CEST 2021
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 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,
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
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list