[libcamera-devel] [PATCH v2 02/11] libcamera: pipeline_handler: Move CameraData members to Camera::Private

[Laurent Pinchart]

Hi Jacopo,

On Thu, Aug 12, 2021 at 09:26:47AM +0200, Jacopo Mondi wrote:
> On Wed, Aug 11, 2021 at 08:31:46PM +0300, Laurent Pinchart wrote:
> > 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' :)

Ah yes of course :-) I think that's standard C++ indeed, and the
compiler will certainly remind anyone who doesn't do so, so I don't
think it needs to be explicitly specified. We have enough to document
already without teaching C++ to the reader :-)

-- 
Regards,

Laurent Pinchart