[libcamera-devel] [PATCH v3 2/5] libcamera: camera_manager: Move private implementation to internal

[Kieran Bingham]

Quoting Laurent Pinchart (2023-06-06 17:05:35)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Mon, May 15, 2023 at 01:45:47PM +0100, Kieran Bingham via libcamera-devel wrote:
> > The CameraManager makes use of the Extensible pattern to provide an
> > internal private implementation that is not exposed in the public API.
> > 
> > Move the Private declaration to an internal header to make it available
> > from other internal components in preperation for reducing the surface
> > area of the public interface of the Camera Manager.
> 
> s/Camera Manager/CameraManager/
> 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_manager.h | 64 +++++++++++++++++++++
> >  include/libcamera/internal/meson.build      |  1 +
> >  src/libcamera/camera_manager.cpp            | 50 ++--------------
> >  3 files changed, 69 insertions(+), 46 deletions(-)
> >  create mode 100644 include/libcamera/internal/camera_manager.h
> > 
> > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> > new file mode 100644
> > index 000000000000..05a1e4df8add
> > --- /dev/null
> > +++ b/include/libcamera/internal/camera_manager.h
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Ideas on Board Oy.
> > + *
> > + * camera_manager.h - Camera manager private data
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> 
> #include <memory>
> 
> for std::shared_ptr, std::unique_ptr and std::weak_ptr, and
> 
> #include <vector>
> 
> for std::vector.

Also #include <sys/types.h> for dev_t;

> 
> > +
> 
> #include <libcamera/base/class.h>
> 
> for Extensible::Private.
> 
> > +#include <libcamera/base/mutex.h>
> > +#include <libcamera/base/thread.h>
> > +
> > +#include <libcamera/camera_manager.h>
> 
> Should this go first to improve the self-contained header test coverage
> ?

clang-format really doesn't like that ;-) I don't think 'headers'
including a component header have any concept in the include handling
there.

But as the core camera_manager.cpp now includes this file first, indeed
I think it should come first to complete the chain.

> 
> > +
> > +#include "libcamera/internal/ipa_manager.h"
> > +#include "libcamera/internal/process.h"
> > +
> > +namespace libcamera {
> > +
> 
> class Camera;
> 
> > +class DeviceEnumerator;
> > +
> > +class CameraManager::Private : public Extensible::Private, public Thread
> > +{
> > +     LIBCAMERA_DECLARE_PUBLIC(CameraManager)
> > +
> > +public:
> > +     Private();
> > +
> > +     int start();
> > +     void addCamera(std::shared_ptr<Camera> camera,
> > +                    const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +     void removeCamera(Camera *camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +
> > +     /*
> > +      * This mutex protects
> > +      *
> > +      * - initialized_ and status_ during initialization
> > +      * - cameras_ and camerasByDevnum_ after initialization
> > +      */
> > +     mutable Mutex mutex_;
> > +     std::vector<std::shared_ptr<Camera>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > +     std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > +
> > +protected:
> > +     void run() override;
> > +
> > +private:
> > +     int init();
> > +     void createPipelineHandlers();
> > +     void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +
> > +     ConditionVariable cv_;
> > +     bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > +     int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > +
> > +     std::unique_ptr<DeviceEnumerator> enumerator_;
> > +
> > +     IPAManager ipaManager_;
> > +     ProcessManager processManager_;
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index d75088059996..0028ed0dc27f 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -13,6 +13,7 @@ libcamera_internal_headers = files([
> >      'bayer_format.h',
> >      'byte_stream_buffer.h',
> >      'camera.h',
> > +    'camera_manager.h',
> 
> Alphabetical order.
> 
> >      'camera_controls.h',
> >      'camera_lens.h',
> >      'camera_sensor.h',
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index c1edefdad160..d3c297b888d8 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -5,24 +5,22 @@
> >   * camera_manager.h - Camera management
> >   */
> >  
> > -#include <libcamera/camera_manager.h>
> > +#include "libcamera/internal/camera_manager.h"
> >  
> >  #include <map>
> 
> You can drop this.
> 
> >  
> > -#include <libcamera/camera.h>
> > -
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/mutex.h>
> >  #include <libcamera/base/thread.h>
> 
> And mutex.h and thread.h too.
> 
> >  #include <libcamera/base/utils.h>
> >  
> > +#include <libcamera/camera.h>
> > +
> >  #include "libcamera/internal/device_enumerator.h"
> > -#include "libcamera/internal/ipa_manager.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> > -#include "libcamera/internal/process.h"
> >  
> >  /**
> > - * \file camera_manager.h
> > + * \file libcamera/camera_manager.h
> 
> I think you need both, see framebuffer.cpp.

Ah, yes that's what I was missing.

> 
> >   * \brief The camera manager
> >   */
> >  
> > @@ -33,46 +31,6 @@ namespace libcamera {
> >  
> >  LOG_DEFINE_CATEGORY(Camera)
> >  
> > -class CameraManager::Private : public Extensible::Private, public Thread
> > -{
> > -     LIBCAMERA_DECLARE_PUBLIC(CameraManager)
> > -
> > -public:
> > -     Private();
> > -
> > -     int start();
> > -     void addCamera(std::shared_ptr<Camera> camera,
> > -                    const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > -     void removeCamera(Camera *camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > -
> > -     /*
> > -      * This mutex protects
> > -      *
> > -      * - initialized_ and status_ during initialization
> > -      * - cameras_ and camerasByDevnum_ after initialization
> > -      */
> > -     mutable Mutex mutex_;
> > -     std::vector<std::shared_ptr<Camera>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > -     std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > -
> > -protected:
> > -     void run() override;
> > -
> > -private:
> > -     int init();
> > -     void createPipelineHandlers();
> > -     void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
> > -
> > -     ConditionVariable cv_;
> > -     bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > -     int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> > -
> > -     std::unique_ptr<DeviceEnumerator> enumerator_;
> > -
> > -     IPAManager ipaManager_;
> > -     ProcessManager processManager_;
> > -};
> > -
> >  CameraManager::Private::Private()
> >       : initialized_(false)
> >  {
> 
> -- 
> Regards,
> 
> Laurent Pinchart