[libcamera-devel] [PATCH v3 03/14] libcamera: pipeline_handler: Move CameraData members to Camera::Private

[Jacopo Mondi]

Hi Laurent,

On Thu, Aug 12, 2021 at 02:26:14AM +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 v2:
>
> - Include <list> where appropriate
> - Minor documentation update
>
> Changes since v1:
>
> - Add \todo comment for controlInfo_
> ---
>  Documentation/Doxyfile.in                     |  1 -
>  include/libcamera/internal/camera.h           |  9 +++
>  include/libcamera/internal/pipeline_handler.h |  6 +-
>  src/libcamera/camera.cpp                      | 65 ++++++++++++++++++-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +-
>  src/libcamera/pipeline_handler.cpp            | 45 +++++--------
>  6 files changed, 89 insertions(+), 39 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..1a08da0cedc4 100644
> --- a/include/libcamera/internal/camera.h
> +++ b/include/libcamera/internal/camera.h
> @@ -8,6 +8,7 @@
>  #define __LIBCAMERA_INTERNAL_CAMERA_H__
>
>  #include <atomic>
> +#include <list>
>  #include <memory>
>  #include <set>
>  #include <string>
> @@ -29,6 +30,14 @@ public:
>  	Private(PipelineHandler *pipe);
>  	~Private();
>
> +	PipelineHandler *pipe() { return pipe_.get(); }
> +
> +	std::list<Request *> queuedRequests_;
> +	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..24b0c5ca081c 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -7,7 +7,6 @@
>  #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__
>  #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__
>
> -#include <list>
>  #include <map>
>  #include <memory>
>  #include <set>
> @@ -39,18 +38,15 @@ class CameraData
>  {
>  public:
>  	explicit CameraData(PipelineHandler *pipe)
> -		: pipe_(pipe), requestSequence_(0)
> +		: pipe_(pipe)
>  	{
>  	}
>  	virtual ~CameraData() = default;
>
>  	PipelineHandler *pipe_;
> -	std::list<Request *> queuedRequests_;
>  	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 d9f6b784e0dc..4080da151af1 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().
> + */
> +
>  /**
>   * \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
> + * \return The pipeline handler that created this camera
> + */
> +
> +/**
> + * \var Camera::Private::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 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.
> + */
> +
> +/**
> + * \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
> + *
> + * 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 c2872289337b..cebd94b2c18b 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 1ab237c8052e..0e571d8981df 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_;
>  }
>
>  /**
> @@ -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_);

With the introduction of std::move() you would need <utility>

As this goes away in the next patches I think it's anyway ok

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

> +	}
> +
>  	cameraData_[camera.get()] = std::move(data);
>  	cameras_.push_back(camera);
>
> --
> Regards,
>
> Laurent Pinchart
>