[libcamera-devel] [PATCH v2 02/11] libcamera: pipeline_handler: Move CameraData members to Camera::Private
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 11 19:31:46 CEST 2021
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 ?
> > > + */
> > > +
> > > /**
> > > * \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)
>
> Good point. Will fix.
>
> > > + */
> > > +
> > > +/**
> > > + * \var Camera::Private::queuedRequests_
> > > + * \brief The list of queued and not yet completed request
> >
> > requests
>
> I'll fix this and the other issues below in a separate patch, to avoid
> hiding the changes in the move.
>
> > > + *
> > > + * 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() ?
>
> 11/11 in this series, as you've noticed :-)
>
> > > }
> > >
> > > /**
> > > @@ -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