[libcamera-devel] [PATCH v2 02/11] libcamera: pipeline_handler: Move CameraData members to Camera::Private
Jacopo Mondi
jacopo at jmondi.org
Thu Aug 12 09:26:47 CEST 2021
On Wed, Aug 11, 2021 at 08:31:46PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Aug 11, 2021 at 08:24:12PM +0300, Laurent Pinchart wrote:
> > On Tue, Aug 10, 2021 at 03:31:37PM +0200, Jacopo Mondi wrote:
> > > On Thu, Aug 05, 2021 at 08:58:39PM +0300, Laurent Pinchart wrote:
> > > > With pipeline handlers now being able to subclass Camera::Private, start
> > > > the migration from CameraData to Camera::Private by moving the members
> > > > of the base CameraData class. The controlInfo_, properties_ and pipe_
> > > > members are duplicated for now, to allow migrating pipeline handlers one
> > > > by one.
> > > >
> > > > The Camera::Private class is now properly documented, don't exclude it
> > > > from documentation generation.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Add \todo comment for controlInfo_
> > > > ---
> > > > Documentation/Doxyfile.in | 1 -
> > > > include/libcamera/internal/camera.h | 8 +++
> > > > include/libcamera/internal/pipeline_handler.h | 5 +-
> > > > src/libcamera/camera.cpp | 65 ++++++++++++++++++-
> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > > > src/libcamera/pipeline_handler.cpp | 45 +++++--------
> > > > 6 files changed, 88 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > > > index fb321ad25f6f..e3b77428cd4f 100644
> > > > --- a/Documentation/Doxyfile.in
> > > > +++ b/Documentation/Doxyfile.in
> > > > @@ -882,7 +882,6 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \
> > > > libcamera::BoundMethodPack \
> > > > libcamera::BoundMethodPackBase \
> > > > libcamera::BoundMethodStatic \
> > > > - libcamera::Camera::Private \
> > > > libcamera::CameraManager::Private \
> > > > libcamera::SignalBase \
> > > > *::details \
> > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > > > index 9ec8321a9a21..44242332a52f 100644
> > > > --- a/include/libcamera/internal/camera.h
> > > > +++ b/include/libcamera/internal/camera.h
> > > > @@ -29,6 +29,14 @@ public:
> > > > Private(PipelineHandler *pipe);
> > > > ~Private();
> > > >
> > > > + PipelineHandler *pipe() { return pipe_.get(); }
> > > > +
> > > > + std::list<Request *> queuedRequests_;
> > >
> > > Do you need to include <list> ?
> >
> > Indeed.
> >
> > > > + ControlInfoMap controlInfo_;
> > > > + ControlList properties_;
> > > > +
> > > > + uint32_t requestSequence_;
> > > > +
> > > > private:
> > > > enum State {
> > > > CameraAvailable,
> > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > > index 9e2d65d6f2c5..86727f90bb65 100644
> > > > --- a/include/libcamera/internal/pipeline_handler.h
> > > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > > @@ -39,18 +39,15 @@ class CameraData
> > > > {
> > > > public:
> > > > explicit CameraData(PipelineHandler *pipe)
> > > > - : pipe_(pipe), requestSequence_(0)
> > > > + : pipe_(pipe)
> > > > {
> > > > }
> > > > virtual ~CameraData() = default;
> > > >
> > > > PipelineHandler *pipe_;
> > > > - std::list<Request *> queuedRequests_;
> > >
> > > and you can drop <list> from this file
> >
> > Indeed too :-)
> >
> > > > ControlInfoMap controlInfo_;
> > > > ControlList properties_;
> > > >
> > > > - uint32_t requestSequence_;
> > > > -
> > > > private:
> > > > LIBCAMERA_DISABLE_COPY(CameraData)
> > > > };
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index a5bb60c698bc..f0893f89a1b3 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -332,13 +332,25 @@ std::size_t CameraConfiguration::size() const
> > > > * \brief The vector of stream configurations
> > > > */
> > > >
> > > > +/**
> > > > + * \class Camera::Private
> > > > + * \brief Base class for camera private data
> > > > + *
> > > > + * The Camera::Private class stores all private data associated with a camera.
> > > > + * In addition to hiding core Camera data from the public API, it is expected to
> > > > + * be subclassed by pipeline handlers to store pipeline-specific data.
> > > > + *
> > > > + * Pipeline handlers can obtain the Camera::Private instance associated with a
> > > > + * camera by calling Camera::_d().
> > >
> > > it's maybe not necessary, as it's a rather C++ standard thing (or
> > > rather OOP in general) but mentioning that the pointer returned
> > > through _d() should be subclassed ?
>
> The documentation already states the the pipeline handler is expected to
> subclass Camera::Private in the previous paragraph, what do you think is
> missing here ?
>
I meant 'downcasted' :)
More information about the libcamera-devel
mailing list