[libcamera-devel] [PATCH v2 1/4] libcamera: camera_manager: Move private implementation to internal

[Kieran Bingham]

Quoting Jacopo Mondi (2023-05-13 15:45:07)
> Hi Kieran
> 
> On Thu, May 11, 2023 at 11:58:30PM +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.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_manager.h | 66 +++++++++++++++++++++
> >  include/libcamera/internal/meson.build      |  1 +
> >  src/libcamera/camera_manager.cpp            | 49 ++-------------
> >  3 files changed, 71 insertions(+), 45 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..4bba3ac2eb29
> > --- /dev/null
> > +++ b/include/libcamera/internal/camera_manager.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Ideas on Board Oy.
> > + *
> > + * camera_manager.h - Camera manager private data
> > + */
> > +#ifndef __LIBCAMERA_INTERNAL_CAMERA_MANAGER_H__
> > +#define __LIBCAMERA_INTERNAL_CAMERA_MANAGER_H__
> 
> #pragma once ?

 Yes :-) Not sure how I missed that.

> 
> > +
> > +#include <map>
> > +
> > +#include <libcamera/base/mutex.h>
> > +#include <libcamera/base/thread.h>
> > +
> > +#include <libcamera/camera_manager.h>
> > +
> > +#include "libcamera/internal/ipa_manager.h"
> > +#include "libcamera/internal/process.h"
> > +
> > +namespace libcamera {
> > +
> > +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 */
> > +
> > +#endif // __LIBCAMERA_INTERNAL_CAMERA_MANAGER_H__
> > 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',
> >      '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..b95284ba5a80 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -9,20 +9,19 @@
> >
> >  #include <map>
> >
> > -#include <libcamera/camera.h>
> > -
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/mutex.h>
> >  #include <libcamera/base/thread.h>
> >  #include <libcamera/base/utils.h>
> >
> > +#include <libcamera/camera.h>
> > +
> > +#include "libcamera/internal/camera_manager.h"
> 
> I would include this one first to make sure it is self-contained

Hrm ... the public one already is. I guess it makes sense to have both
up at the top?


> 
> >  #include "libcamera/internal/device_enumerator.h"
> > -#include "libcamera/internal/ipa_manager.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> > -#include "libcamera/internal/pruocess.h"
> >
> >  /**
> > - * \file camera_manager.h
> > + * \file libcamera/camera_manager.h
> 
> is this needed ?

Yes, otherwise doxygen complains.

> 
> All minors:
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks


> 
> Thanks
>   j
> 
> 
> >   * \brief The camera manager
> >   */
> >
> > @@ -33,46 +32,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)
> >  {
> > --
> > 2.34.1
> >