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

[Jacopo Mondi]

Hi Laurent

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> ?

> +	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

>  	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 ?

> + */
> +
>  /**
>   * \brief Construct a Camera::Private instance
>   * \param[in] pipe The pipeline handler responsible for the camera device
>   */
>  Camera::Private::Private(PipelineHandler *pipe)
> -	: pipe_(pipe->shared_from_this()), disconnected_(false),
> -	  state_(CameraAvailable)
> +	: requestSequence_(0), pipe_(pipe->shared_from_this()),
> +	  disconnected_(false), state_(CameraAvailable)
>  {
>  }
>
> @@ -348,6 +360,55 @@ Camera::Private::~Private()
>  		LOG(Camera, Error) << "Removing camera while still in use";
>  }
>
> +/**
> + * \fn Camera::Private::pipe()
> + * \brief Retrieve the pipeline handler related to this camera

I admit if I were totally new to the thing I could wonder why a
pipeline handler has to call pipe() to get a pointer to itself :)

> + * \return The pipeline handler for this camera

I would rather say "The pipeline that created the camera" as it might
suggest each Camera could potentially have a different ph associated
(which is true in general, but not in the ph developer perspective)

> + */
> +
> +/**
> + * \var Camera::Private::queuedRequests_
> + * \brief The list of queued and not yet completed request

requests

> + *
> + * The list of queued request is used to track requests queued in order to

'queued' is repeated twice

> + * ensure completion of all requests when the pipeline handler is stopped.
> + *
> + * \sa PipelineHandler::queueRequest(), PipelineHandler::stop(),
> + * PipelineHandler::completeRequest()
> + */
> +
> +/**
> + * \var Camera::Private::controlInfo_
> + * \brief The set of controls supported by the camera
> + *
> + * The control information shall be initialised by the pipeline handler when
> + * creating the camera.
> + *
> + * \todo This member was initially meant to stay constant after the camera is
> + * created. Several pipeline handlers are already updating it when the camera
> + * is configured. Update the documentation accordingly, and possibly the API as
> + * well, when implementing official support for control info updates.

Thanks :)

> + */
> +
> +/**
> + * \var Camera::Private::properties_
> + * \brief The list of properties supported by the camera
> + *
> + * The list of camera properties shall be initialised by the pipeline handler
> + * when creating the camera, and shall not be modified afterwards.
> + */
> +
> +/**
> + * \var Camera::Private::requestSequence_
> + * \brief The queuing sequence of the request

The queueing sequence -number- ?

Ah well, the documentation was already there...

> + *
> + * When requests are queued, they are given a per-camera sequence number to
> + * facilitate debugging of internal request usage.
> + *
> + * The requestSequence_ tracks the number of requests queued to a camera
> + * over its lifetime.
> + */
> +
>  static const char *const camera_state_names[] = {
>  	"Available",
>  	"Acquired",
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4a8ac97d5ef0..cc279ce733ef 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -847,7 +847,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
>  		LOG(RkISP1, Warning)
>  			<< "Failed to stop parameters for " << camera->id();
>
> -	ASSERT(data->queuedRequests_.empty());
> +	ASSERT(camera->_d()->queuedRequests_.empty());
>  	data->frameInfo_.clear();
>
>  	freeBuffers(camera);
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index c9928d444b04..1f1de19d81c5 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -16,6 +16,7 @@
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/framebuffer.h>
>
> +#include "libcamera/internal/camera.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/tracepoints.h"
> @@ -71,17 +72,6 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * and remains valid until the instance is destroyed.
>   */
>
> -/**
> - * \var CameraData::queuedRequests_
> - * \brief The list of queued and not yet completed request
> - *
> - * The list of queued request is used to track requests queued in order to
> - * ensure completion of all requests when the pipeline handler is stopped.
> - *
> - * \sa PipelineHandler::queueRequest(), PipelineHandler::stop(),
> - * PipelineHandler::completeRequest()
> - */
> -
>  /**
>   * \var CameraData::controlInfo_
>   * \brief The set of controls supported by the camera
> @@ -98,17 +88,6 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * when creating the camera, and shall not be modified afterwards.
>   */
>
> -/**
> - * \var CameraData::requestSequence_
> - * \brief The queuing sequence of the request
> - *
> - * When requests are queued, they are given a per-camera sequence number to
> - * facilitate debugging of internal request usage.
> - *
> - * The requestSequence_ tracks the number of requests queued to a camera
> - * over its lifetime.
> - */
> -
>  /**
>   * \class PipelineHandler
>   * \brief Create and manage cameras based on a set of media devices
> @@ -254,8 +233,7 @@ void PipelineHandler::unlock()
>   */
>  const ControlInfoMap &PipelineHandler::controls(const Camera *camera) const
>  {
> -	const CameraData *data = cameraData(camera);
> -	return data->controlInfo_;
> +	return camera->_d()->controlInfo_;

I'm sorry if we discussed this already, I have a vague remembering of
that... but do Camera need to go through the PipelineHander base class
to do so as it has direct access to _d() ?

>  }
>
>  /**
> @@ -265,8 +243,7 @@ const ControlInfoMap &PipelineHandler::controls(const Camera *camera) const
>   */
>  const ControlList &PipelineHandler::properties(const Camera *camera) const
>  {
> -	const CameraData *data = cameraData(camera);
> -	return data->properties_;
> +	return camera->_d()->properties_;
>  }
>
>  /**
> @@ -380,8 +357,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const
>   */
>  bool PipelineHandler::hasPendingRequests(const Camera *camera) const
>  {
> -	const CameraData *data = cameraData(camera);
> -	return !data->queuedRequests_.empty();
> +	return !camera->_d()->queuedRequests_.empty();
>  }
>
>  /**
> @@ -406,7 +382,7 @@ void PipelineHandler::queueRequest(Request *request)
>  	LIBCAMERA_TRACEPOINT(request_queue, request);
>
>  	Camera *camera = request->camera_;
> -	CameraData *data = cameraData(camera);
> +	Camera::Private *data = camera->_d();
>  	data->queuedRequests_.push_back(request);
>
>  	request->sequence_ = data->requestSequence_++;
> @@ -479,7 +455,7 @@ void PipelineHandler::completeRequest(Request *request)
>
>  	request->complete();
>
> -	CameraData *data = cameraData(camera);
> +	Camera::Private *data = camera->_d();
>
>  	while (!data->queuedRequests_.empty()) {
>  		Request *req = data->queuedRequests_.front();
> @@ -507,6 +483,15 @@ void PipelineHandler::completeRequest(Request *request)
>  void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
>  				     std::unique_ptr<CameraData> data)
>  {
> +	/*
> +	 * To be removed once all pipeline handlers will have migrated from
> +	 * CameraData to Camera::Private.
> +	 */
> +	if (data) {
> +		camera->_d()->controlInfo_ = std::move(data->controlInfo_);
> +		camera->_d()->properties_ = std::move(data->properties_);
> +	}
> +
>  	cameraData_[camera.get()] = std::move(data);
>  	cameras_.push_back(camera);
>
> --
> Regards,
>
> Laurent Pinchart
>