[libcamera-devel] [RFC PATCH 09/17] libcamera: pipeline_handler: Move CameraData members to Camera::Private
Jacopo Mondi
jacopo at jmondi.org
Thu Jul 29 23:11:07 CEST 2021
Hi Laurent
On Fri, Jul 23, 2021 at 07:00:28AM +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>
> ---
> Documentation/Doxyfile.in | 1 -
> include/libcamera/internal/camera.h | 8 +++
> include/libcamera/internal/pipeline_handler.h | 5 +-
> src/libcamera/camera.cpp | 60 ++++++++++++++++++-
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> src/libcamera/pipeline_handler.cpp | 45 +++++---------
> 6 files changed, 83 insertions(+), 38 deletions(-)
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index aee5c9fd7e59..b8777b0c4522 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_;
> + 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_;
> 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..72a42dbeb3d3 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,50 @@ 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 for 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, and shall not be modified afterwards.
Well, that's not true, but I understand this is a copy of the existing
documentation...
> + */
> +
> +/**
> + * \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 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
It might happen later in the series, but the Camera is now calling the
pipeline handler while it could actually just call its private class.
> {
> - 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_);
> + }
> +
> cameraData_[camera.get()] = std::move(data);
> cameras_.push_back(camera);
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list