| Message ID | 20210811232625.17280-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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@jmondi.org> Thanks j > + } > + > cameraData_[camera.get()] = std::move(data); > cameras_.push_back(camera); > > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Thu, Aug 12, 2021 at 09:54:11AM +0200, Jacopo Mondi wrote: > 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@ideasonboard.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@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> This is related to a discussion we had on Tuesday I think. How far should we go when it comes to inclusion of headers ? One particular case that I'm not sure how to handle is if a .cpp file should include headers for classes that are used in the corresponding .h. For instance, foo.h #include <bar.h> class Foo { public: Bar bar_; }; There's a guarantee that bar.h is included by foo.h, so we don't necessarily have to include it in foo.cpp. This could also be extended to std::move(): if a class definition uses std::unique_ptr<>, there's a guarantee it includes <utility>, and we don't necessarily have to include it in the .cpp file. I wonder what the best practice would be in this case. This being said, here we use the move assignment operators of ControlInfoMap and ControlList, not std::unique_ptr<>, so we should still include <utility> if the PipelineHandler class didn't have other unique pointers. > As this goes away in the next patches I think it's anyway ok > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + } > > + > > cameraData_[camera.get()] = std::move(data); > > cameras_.push_back(camera); > >
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_); + } + cameraData_[camera.get()] = std::move(data); cameras_.push_back(camera);