| Message ID | 20240221174015.52958-2-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo On 21/02/2024 17:40, Jacopo Mondi wrote: > In order to allow each pipeline handler to create a Request::Private > derived class to store ancillary data associated with a capture request, > modify the way a Request is created by the PipelineHandler base class. > > Introduce a virtual PipelineHandler::createRequestDevice() that pipeline > handler implementation can optionally override to provide a custom > private data type to initialize the Request with. > > As we can't anymore create a Request without going through an active > camera, drop the queueRequest() checks from the Camera statemachine > test. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > include/libcamera/internal/pipeline_handler.h | 5 ++- > include/libcamera/request.h | 3 +- > src/libcamera/camera.cpp | 8 +--- > src/libcamera/pipeline_handler.cpp | 38 ++++++++++++++++--- > src/libcamera/request.cpp | 15 +++++--- > test/camera/statemachine.cpp | 12 ------ > 6 files changed, 50 insertions(+), 31 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index c96944f4ecc4..bbe74f22e5ae 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -21,6 +21,7 @@ > #include <libcamera/stream.h> > > #include "libcamera/internal/ipa_proxy.h" > +#include "libcamera/internal/request.h" > > namespace libcamera { > > @@ -59,7 +60,7 @@ public: > void stop(Camera *camera); > bool hasPendingRequests(const Camera *camera) const; > > - void registerRequest(Request *request); > + std::unique_ptr<Request> createRequest(Camera *camera, uint64_t cookie); > void queueRequest(Request *request); > > bool completeBuffer(Request *request, FrameBuffer *buffer); > @@ -74,6 +75,8 @@ protected: > void registerCamera(std::shared_ptr<Camera> camera); > void hotplugMediaDevice(MediaDevice *media); > > + virtual std::unique_ptr<Request> createRequestDevice(Camera *camera, > + uint64_t cookie); > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > virtual void stopDevice(Camera *camera) = 0; > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index dffde1536cad..e16e61a93873 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -45,9 +45,10 @@ public: > > using BufferMap = std::map<const Stream *, FrameBuffer *>; > > - Request(Camera *camera, uint64_t cookie = 0); > + Request(std::unique_ptr<Private> d, uint64_t cookie = 0); > ~Request(); > > + static std::unique_ptr<Request> create(std::unique_ptr<Private> d, uint64_t); > void reuse(ReuseFlag flags = Default); > > ControlList &controls() { return *controls_; } > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index a71dc933b911..b7053576fe42 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1236,12 +1236,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > if (ret < 0) > return nullptr; > > - std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie); > - > - /* Associate the request with the pipeline handler. */ > - d->pipe_->registerRequest(request.get()); > - > - return request; > + /* Create a Request from the pipeline handler. */ > + return d->pipe_->createRequest(this, cookie); > } > > /** > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 29e0c98a6db5..f2a8cdac0408 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -371,20 +371,25 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > } > > /** > - * \fn PipelineHandler::registerRequest() > - * \brief Register a request for use by the pipeline handler > - * \param[in] request The request to register > + * \fn PipelineHandler::createRequest() > + * \brief Create a request and register it for use by the pipeline handler > + * \param[in] camera The camera the Request belongs to > + * \param[in] cookie The Request unique identifier > * > - * This function is called when the request is created, and allows the pipeline > - * handler to perform any one-time initialization it requries for the request. > + * This function is called to create a Request by calling createRequestDevice() > + * which can be optionally provided by the PipelineHandler derived classes. > */ > -void PipelineHandler::registerRequest(Request *request) > +std::unique_ptr<Request> PipelineHandler::createRequest(Camera *camera, uint64_t cookie) > { > + std::unique_ptr<Request> request = createRequestDevice(camera, cookie); > + > /* > * Connect the request prepared signal to notify the pipeline handler > * when a request is ready to be processed. > */ > request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); > + > + return request; > } > > /** > @@ -462,6 +467,27 @@ void PipelineHandler::doQueueRequests() > } > } > > +/** > + * \brief Create a Request from the pipeline handler > + * \param[in] camera The camera the Request belongs to > + * \param[in] cookie The Request unique identifier > + * > + * A virtual function that PipelineHandler derived classes are free to override > + * in order to initialize a Request with a custom Request::Private derived > + * class. > + * > + * This is the base class implementation that use Request::Private to > + * initialize the Request. > + * > + * \return A unique pointer to a newly created Request > + */ > +std::unique_ptr<Request> > +PipelineHandler::createRequestDevice(Camera *camera, uint64_t cookie) > +{ > + auto d = std::make_unique<Request::Private>(camera); > + return Request::create(std::move(d), cookie); > +} > + > /** > * \fn PipelineHandler::queueRequestDevice() > * \brief Queue a request to the device > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 949c556fa437..d1051ad3d25e 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -336,7 +336,7 @@ void Request::Private::timeout() > > /** > * \brief Create a capture request for a camera > - * \param[in] camera The camera that creates the request > + * \param[in] d The request private data > * \param[in] cookie Opaque cookie for application use > * > * The \a cookie is stored in the request and is accessible through the > @@ -344,12 +344,17 @@ void Request::Private::timeout() > * the request to an external resource in the request completion handler, and is > * completely opaque to libcamera. > */ > -Request::Request(Camera *camera, uint64_t cookie) > - : Extensible(std::make_unique<Private>(camera)), > - cookie_(cookie), status_(RequestPending) > +std::unique_ptr<Request> Request::create(std::unique_ptr<Private> d, > + uint64_t cookie) > +{ > + return std::make_unique<Request>(std::move(d), cookie); > +} > + I'm sure this is just me not understanding c++ properly; but what's the reason for this function? Why not just use the class constructor? > +Request::Request(std::unique_ptr<Request::Private> d, uint64_t cookie) > + : Extensible(std::move(d)), cookie_(cookie), status_(RequestPending) > { > controls_ = new ControlList(controls::controls, > - camera->_d()->validator()); > + _d()->camera()->_d()->validator()); > > /** > * \todo Add a validator for metadata controls. > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp > index 9c2b0c6a7d99..5714061f88b5 100644 > --- a/test/camera/statemachine.cpp > +++ b/test/camera/statemachine.cpp > @@ -38,10 +38,6 @@ protected: > if (camera_->start() != -EACCES) > return TestFail; > > - Request request(camera_.get()); > - if (camera_->queueRequest(&request) != -EACCES) > - return TestFail; > - > /* Test operations which should pass. */ > if (camera_->release()) > return TestFail; > @@ -68,10 +64,6 @@ protected: > if (camera_->start() != -EACCES) > return TestFail; > > - Request request(camera_.get()); > - if (camera_->queueRequest(&request) != -EACCES) > - return TestFail; > - > /* Test operations which should pass. */ > if (camera_->stop()) > return TestFail; > @@ -95,10 +87,6 @@ protected: > if (camera_->acquire() != -EBUSY) > return TestFail; > > - Request request1(camera_.get()); > - if (camera_->queueRequest(&request1) != -EACCES) > - return TestFail; > - > /* Test operations which should pass. */ > std::unique_ptr<Request> request2 = camera_->createRequest(); > if (!request2)
Hi Dan On Tue, Mar 05, 2024 at 11:10:02AM +0000, Dan Scally wrote: > Hi Jacopo > > On 21/02/2024 17:40, Jacopo Mondi wrote: > > In order to allow each pipeline handler to create a Request::Private > > derived class to store ancillary data associated with a capture request, > > modify the way a Request is created by the PipelineHandler base class. > > > > Introduce a virtual PipelineHandler::createRequestDevice() that pipeline > > handler implementation can optionally override to provide a custom > > private data type to initialize the Request with. > > > > As we can't anymore create a Request without going through an active > > camera, drop the queueRequest() checks from the Camera statemachine > > test. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > include/libcamera/internal/pipeline_handler.h | 5 ++- > > include/libcamera/request.h | 3 +- > > src/libcamera/camera.cpp | 8 +--- > > src/libcamera/pipeline_handler.cpp | 38 ++++++++++++++++--- > > src/libcamera/request.cpp | 15 +++++--- > > test/camera/statemachine.cpp | 12 ------ > > 6 files changed, 50 insertions(+), 31 deletions(-) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index c96944f4ecc4..bbe74f22e5ae 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -21,6 +21,7 @@ > > #include <libcamera/stream.h> > > #include "libcamera/internal/ipa_proxy.h" > > +#include "libcamera/internal/request.h" > > namespace libcamera { > > @@ -59,7 +60,7 @@ public: > > void stop(Camera *camera); > > bool hasPendingRequests(const Camera *camera) const; > > - void registerRequest(Request *request); > > + std::unique_ptr<Request> createRequest(Camera *camera, uint64_t cookie); > > void queueRequest(Request *request); > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > @@ -74,6 +75,8 @@ protected: > > void registerCamera(std::shared_ptr<Camera> camera); > > void hotplugMediaDevice(MediaDevice *media); > > + virtual std::unique_ptr<Request> createRequestDevice(Camera *camera, > > + uint64_t cookie); > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > > virtual void stopDevice(Camera *camera) = 0; > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index dffde1536cad..e16e61a93873 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -45,9 +45,10 @@ public: > > using BufferMap = std::map<const Stream *, FrameBuffer *>; > > - Request(Camera *camera, uint64_t cookie = 0); > > + Request(std::unique_ptr<Private> d, uint64_t cookie = 0); > > ~Request(); > > + static std::unique_ptr<Request> create(std::unique_ptr<Private> d, uint64_t); > > void reuse(ReuseFlag flags = Default); > > ControlList &controls() { return *controls_; } > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index a71dc933b911..b7053576fe42 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -1236,12 +1236,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > > if (ret < 0) > > return nullptr; > > - std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie); > > - > > - /* Associate the request with the pipeline handler. */ > > - d->pipe_->registerRequest(request.get()); > > - > > - return request; > > + /* Create a Request from the pipeline handler. */ > > + return d->pipe_->createRequest(this, cookie); > > } > > /** > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 29e0c98a6db5..f2a8cdac0408 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -371,20 +371,25 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > > } > > /** > > - * \fn PipelineHandler::registerRequest() > > - * \brief Register a request for use by the pipeline handler > > - * \param[in] request The request to register > > + * \fn PipelineHandler::createRequest() > > + * \brief Create a request and register it for use by the pipeline handler > > + * \param[in] camera The camera the Request belongs to > > + * \param[in] cookie The Request unique identifier > > * > > - * This function is called when the request is created, and allows the pipeline > > - * handler to perform any one-time initialization it requries for the request. > > + * This function is called to create a Request by calling createRequestDevice() > > + * which can be optionally provided by the PipelineHandler derived classes. > > */ > > -void PipelineHandler::registerRequest(Request *request) > > +std::unique_ptr<Request> PipelineHandler::createRequest(Camera *camera, uint64_t cookie) > > { > > + std::unique_ptr<Request> request = createRequestDevice(camera, cookie); > > + > > /* > > * Connect the request prepared signal to notify the pipeline handler > > * when a request is ready to be processed. > > */ > > request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); > > + > > + return request; > > } > > /** > > @@ -462,6 +467,27 @@ void PipelineHandler::doQueueRequests() > > } > > } > > +/** > > + * \brief Create a Request from the pipeline handler > > + * \param[in] camera The camera the Request belongs to > > + * \param[in] cookie The Request unique identifier > > + * > > + * A virtual function that PipelineHandler derived classes are free to override > > + * in order to initialize a Request with a custom Request::Private derived > > + * class. > > + * > > + * This is the base class implementation that use Request::Private to > > + * initialize the Request. > > + * > > + * \return A unique pointer to a newly created Request > > + */ > > +std::unique_ptr<Request> > > +PipelineHandler::createRequestDevice(Camera *camera, uint64_t cookie) > > +{ > > + auto d = std::make_unique<Request::Private>(camera); > > + return Request::create(std::move(d), cookie); > > +} > > + > > /** > > * \fn PipelineHandler::queueRequestDevice() > > * \brief Queue a request to the device > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index 949c556fa437..d1051ad3d25e 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -336,7 +336,7 @@ void Request::Private::timeout() > > /** > > * \brief Create a capture request for a camera > > - * \param[in] camera The camera that creates the request > > + * \param[in] d The request private data > > * \param[in] cookie Opaque cookie for application use > > * > > * The \a cookie is stored in the request and is accessible through the > > @@ -344,12 +344,17 @@ void Request::Private::timeout() > > * the request to an external resource in the request completion handler, and is > > * completely opaque to libcamera. > > */ > > -Request::Request(Camera *camera, uint64_t cookie) > > - : Extensible(std::make_unique<Private>(camera)), > > - cookie_(cookie), status_(RequestPending) > > +std::unique_ptr<Request> Request::create(std::unique_ptr<Private> d, > > + uint64_t cookie) > > +{ > > + return std::make_unique<Request>(std::move(d), cookie); > > +} > > + > > > I'm sure this is just me not understanding c++ properly; but what's the > reason for this function? Why not just use the class constructor? > It's an helper, to easily create a unique_ptr<> from a Request. I think pipeline handlers can live without it std::unique_ptr<Request> PipelineHandlerRkISP1::createRequestDevice(Camera *camera, uint64_t cookie) { - auto request = std::make_unique<RkISP1Request>(camera); - return Request::create(std::move(request), cookie); + return std::make_unique<Request>(std::make_unique<RkISP1Request>(camera), + cookie); } It just feels nicer Also note that applications won't be able to use this function or neither the constructor has they don't have access to internal headers so they can't provide a Request::Private instance. > > +Request::Request(std::unique_ptr<Request::Private> d, uint64_t cookie) > > + : Extensible(std::move(d)), cookie_(cookie), status_(RequestPending) > > { > > controls_ = new ControlList(controls::controls, > > - camera->_d()->validator()); > > + _d()->camera()->_d()->validator()); > > /** > > * \todo Add a validator for metadata controls. > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp > > index 9c2b0c6a7d99..5714061f88b5 100644 > > --- a/test/camera/statemachine.cpp > > +++ b/test/camera/statemachine.cpp > > @@ -38,10 +38,6 @@ protected: > > if (camera_->start() != -EACCES) > > return TestFail; > > - Request request(camera_.get()); > > - if (camera_->queueRequest(&request) != -EACCES) > > - return TestFail; > > - > > /* Test operations which should pass. */ > > if (camera_->release()) > > return TestFail; > > @@ -68,10 +64,6 @@ protected: > > if (camera_->start() != -EACCES) > > return TestFail; > > - Request request(camera_.get()); > > - if (camera_->queueRequest(&request) != -EACCES) > > - return TestFail; > > - > > /* Test operations which should pass. */ > > if (camera_->stop()) > > return TestFail; > > @@ -95,10 +87,6 @@ protected: > > if (camera_->acquire() != -EBUSY) > > return TestFail; > > - Request request1(camera_.get()); > > - if (camera_->queueRequest(&request1) != -EACCES) > > - return TestFail; > > - > > /* Test operations which should pass. */ > > std::unique_ptr<Request> request2 = camera_->createRequest(); > > if (!request2)
Hi Jacopo On 05/03/2024 11:28, Jacopo Mondi wrote: > Hi Dan > > On Tue, Mar 05, 2024 at 11:10:02AM +0000, Dan Scally wrote: >> Hi Jacopo >> >> On 21/02/2024 17:40, Jacopo Mondi wrote: >>> In order to allow each pipeline handler to create a Request::Private >>> derived class to store ancillary data associated with a capture request, >>> modify the way a Request is created by the PipelineHandler base class. >>> >>> Introduce a virtual PipelineHandler::createRequestDevice() that pipeline >>> handler implementation can optionally override to provide a custom >>> private data type to initialize the Request with. >>> >>> As we can't anymore create a Request without going through an active >>> camera, drop the queueRequest() checks from the Camera statemachine >>> test. >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> --- >>> include/libcamera/internal/pipeline_handler.h | 5 ++- >>> include/libcamera/request.h | 3 +- >>> src/libcamera/camera.cpp | 8 +--- >>> src/libcamera/pipeline_handler.cpp | 38 ++++++++++++++++--- >>> src/libcamera/request.cpp | 15 +++++--- >>> test/camera/statemachine.cpp | 12 ------ >>> 6 files changed, 50 insertions(+), 31 deletions(-) >>> >>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >>> index c96944f4ecc4..bbe74f22e5ae 100644 >>> --- a/include/libcamera/internal/pipeline_handler.h >>> +++ b/include/libcamera/internal/pipeline_handler.h >>> @@ -21,6 +21,7 @@ >>> #include <libcamera/stream.h> >>> #include "libcamera/internal/ipa_proxy.h" >>> +#include "libcamera/internal/request.h" >>> namespace libcamera { >>> @@ -59,7 +60,7 @@ public: >>> void stop(Camera *camera); >>> bool hasPendingRequests(const Camera *camera) const; >>> - void registerRequest(Request *request); >>> + std::unique_ptr<Request> createRequest(Camera *camera, uint64_t cookie); >>> void queueRequest(Request *request); >>> bool completeBuffer(Request *request, FrameBuffer *buffer); >>> @@ -74,6 +75,8 @@ protected: >>> void registerCamera(std::shared_ptr<Camera> camera); >>> void hotplugMediaDevice(MediaDevice *media); >>> + virtual std::unique_ptr<Request> createRequestDevice(Camera *camera, >>> + uint64_t cookie); >>> virtual int queueRequestDevice(Camera *camera, Request *request) = 0; >>> virtual void stopDevice(Camera *camera) = 0; >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>> index dffde1536cad..e16e61a93873 100644 >>> --- a/include/libcamera/request.h >>> +++ b/include/libcamera/request.h >>> @@ -45,9 +45,10 @@ public: >>> using BufferMap = std::map<const Stream *, FrameBuffer *>; >>> - Request(Camera *camera, uint64_t cookie = 0); >>> + Request(std::unique_ptr<Private> d, uint64_t cookie = 0); >>> ~Request(); >>> + static std::unique_ptr<Request> create(std::unique_ptr<Private> d, uint64_t); >>> void reuse(ReuseFlag flags = Default); >>> ControlList &controls() { return *controls_; } >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>> index a71dc933b911..b7053576fe42 100644 >>> --- a/src/libcamera/camera.cpp >>> +++ b/src/libcamera/camera.cpp >>> @@ -1236,12 +1236,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) >>> if (ret < 0) >>> return nullptr; >>> - std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie); >>> - >>> - /* Associate the request with the pipeline handler. */ >>> - d->pipe_->registerRequest(request.get()); >>> - >>> - return request; >>> + /* Create a Request from the pipeline handler. */ >>> + return d->pipe_->createRequest(this, cookie); >>> } >>> /** >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >>> index 29e0c98a6db5..f2a8cdac0408 100644 >>> --- a/src/libcamera/pipeline_handler.cpp >>> +++ b/src/libcamera/pipeline_handler.cpp >>> @@ -371,20 +371,25 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const >>> } >>> /** >>> - * \fn PipelineHandler::registerRequest() >>> - * \brief Register a request for use by the pipeline handler >>> - * \param[in] request The request to register >>> + * \fn PipelineHandler::createRequest() >>> + * \brief Create a request and register it for use by the pipeline handler >>> + * \param[in] camera The camera the Request belongs to >>> + * \param[in] cookie The Request unique identifier >>> * >>> - * This function is called when the request is created, and allows the pipeline >>> - * handler to perform any one-time initialization it requries for the request. >>> + * This function is called to create a Request by calling createRequestDevice() >>> + * which can be optionally provided by the PipelineHandler derived classes. >>> */ >>> -void PipelineHandler::registerRequest(Request *request) >>> +std::unique_ptr<Request> PipelineHandler::createRequest(Camera *camera, uint64_t cookie) >>> { >>> + std::unique_ptr<Request> request = createRequestDevice(camera, cookie); >>> + >>> /* >>> * Connect the request prepared signal to notify the pipeline handler >>> * when a request is ready to be processed. >>> */ >>> request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); >>> + >>> + return request; >>> } >>> /** >>> @@ -462,6 +467,27 @@ void PipelineHandler::doQueueRequests() >>> } >>> } >>> +/** >>> + * \brief Create a Request from the pipeline handler >>> + * \param[in] camera The camera the Request belongs to >>> + * \param[in] cookie The Request unique identifier >>> + * >>> + * A virtual function that PipelineHandler derived classes are free to override >>> + * in order to initialize a Request with a custom Request::Private derived >>> + * class. >>> + * >>> + * This is the base class implementation that use Request::Private to >>> + * initialize the Request. >>> + * >>> + * \return A unique pointer to a newly created Request >>> + */ >>> +std::unique_ptr<Request> >>> +PipelineHandler::createRequestDevice(Camera *camera, uint64_t cookie) >>> +{ >>> + auto d = std::make_unique<Request::Private>(camera); >>> + return Request::create(std::move(d), cookie); >>> +} >>> + >>> /** >>> * \fn PipelineHandler::queueRequestDevice() >>> * \brief Queue a request to the device >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >>> index 949c556fa437..d1051ad3d25e 100644 >>> --- a/src/libcamera/request.cpp >>> +++ b/src/libcamera/request.cpp >>> @@ -336,7 +336,7 @@ void Request::Private::timeout() >>> /** >>> * \brief Create a capture request for a camera >>> - * \param[in] camera The camera that creates the request >>> + * \param[in] d The request private data >>> * \param[in] cookie Opaque cookie for application use >>> * >>> * The \a cookie is stored in the request and is accessible through the >>> @@ -344,12 +344,17 @@ void Request::Private::timeout() >>> * the request to an external resource in the request completion handler, and is >>> * completely opaque to libcamera. >>> */ >>> -Request::Request(Camera *camera, uint64_t cookie) >>> - : Extensible(std::make_unique<Private>(camera)), >>> - cookie_(cookie), status_(RequestPending) >>> +std::unique_ptr<Request> Request::create(std::unique_ptr<Private> d, >>> + uint64_t cookie) >>> +{ >>> + return std::make_unique<Request>(std::move(d), cookie); >>> +} >>> + >> >> I'm sure this is just me not understanding c++ properly; but what's the >> reason for this function? Why not just use the class constructor? >> > It's an helper, to easily create a unique_ptr<> from a Request. I > think pipeline handlers can live without it > > > std::unique_ptr<Request> PipelineHandlerRkISP1::createRequestDevice(Camera *camera, > uint64_t cookie) > { > - auto request = std::make_unique<RkISP1Request>(camera); > - return Request::create(std::move(request), cookie); > + return std::make_unique<Request>(std::make_unique<RkISP1Request>(camera), > + cookie); > } Yeah that's what I was expecting > It just feels nicer Fair enough, I was worried I wasn't understanding something! Anyway; I think the patch is good: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> and Tested-by: Daniel Scally <dan.scally@ideasonboard.com> > Also note that applications won't be able to use this function or neither the > constructor has they don't have access to internal headers so they > can't provide a Request::Private instance. > >>> +Request::Request(std::unique_ptr<Request::Private> d, uint64_t cookie) >>> + : Extensible(std::move(d)), cookie_(cookie), status_(RequestPending) >>> { >>> controls_ = new ControlList(controls::controls, >>> - camera->_d()->validator()); >>> + _d()->camera()->_d()->validator()); >>> /** >>> * \todo Add a validator for metadata controls. >>> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp >>> index 9c2b0c6a7d99..5714061f88b5 100644 >>> --- a/test/camera/statemachine.cpp >>> +++ b/test/camera/statemachine.cpp >>> @@ -38,10 +38,6 @@ protected: >>> if (camera_->start() != -EACCES) >>> return TestFail; >>> - Request request(camera_.get()); >>> - if (camera_->queueRequest(&request) != -EACCES) >>> - return TestFail; >>> - >>> /* Test operations which should pass. */ >>> if (camera_->release()) >>> return TestFail; >>> @@ -68,10 +64,6 @@ protected: >>> if (camera_->start() != -EACCES) >>> return TestFail; >>> - Request request(camera_.get()); >>> - if (camera_->queueRequest(&request) != -EACCES) >>> - return TestFail; >>> - >>> /* Test operations which should pass. */ >>> if (camera_->stop()) >>> return TestFail; >>> @@ -95,10 +87,6 @@ protected: >>> if (camera_->acquire() != -EBUSY) >>> return TestFail; >>> - Request request1(camera_.get()); >>> - if (camera_->queueRequest(&request1) != -EACCES) >>> - return TestFail; >>> - >>> /* Test operations which should pass. */ >>> std::unique_ptr<Request> request2 = camera_->createRequest(); >>> if (!request2)
Hi Jacopo, maybe it would be nice to mention in the commit message, that this breaks the ABI. (I know we don't really track this, but hey :-) On Tue, Mar 05, 2024 at 12:28:14PM +0100, Jacopo Mondi wrote: > Hi Dan > > On Tue, Mar 05, 2024 at 11:10:02AM +0000, Dan Scally wrote: > > Hi Jacopo > > > > On 21/02/2024 17:40, Jacopo Mondi wrote: > > > In order to allow each pipeline handler to create a Request::Private > > > derived class to store ancillary data associated with a capture request, > > > modify the way a Request is created by the PipelineHandler base class. > > > > > > Introduce a virtual PipelineHandler::createRequestDevice() that pipeline > > > handler implementation can optionally override to provide a custom > > > private data type to initialize the Request with. > > > > > > As we can't anymore create a Request without going through an active > > > camera, drop the queueRequest() checks from the Camera statemachine > > > test. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > include/libcamera/internal/pipeline_handler.h | 5 ++- > > > include/libcamera/request.h | 3 +- > > > src/libcamera/camera.cpp | 8 +--- > > > src/libcamera/pipeline_handler.cpp | 38 ++++++++++++++++--- > > > src/libcamera/request.cpp | 15 +++++--- > > > test/camera/statemachine.cpp | 12 ------ > > > 6 files changed, 50 insertions(+), 31 deletions(-) > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > index c96944f4ecc4..bbe74f22e5ae 100644 > > > --- a/include/libcamera/internal/pipeline_handler.h > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > @@ -21,6 +21,7 @@ > > > #include <libcamera/stream.h> > > > #include "libcamera/internal/ipa_proxy.h" > > > +#include "libcamera/internal/request.h" > > > namespace libcamera { > > > @@ -59,7 +60,7 @@ public: > > > void stop(Camera *camera); > > > bool hasPendingRequests(const Camera *camera) const; > > > - void registerRequest(Request *request); > > > + std::unique_ptr<Request> createRequest(Camera *camera, uint64_t cookie); > > > void queueRequest(Request *request); > > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > > @@ -74,6 +75,8 @@ protected: > > > void registerCamera(std::shared_ptr<Camera> camera); > > > void hotplugMediaDevice(MediaDevice *media); > > > + virtual std::unique_ptr<Request> createRequestDevice(Camera *camera, > > > + uint64_t cookie); > > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > > > virtual void stopDevice(Camera *camera) = 0; > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > index dffde1536cad..e16e61a93873 100644 > > > --- a/include/libcamera/request.h > > > +++ b/include/libcamera/request.h > > > @@ -45,9 +45,10 @@ public: > > > using BufferMap = std::map<const Stream *, FrameBuffer *>; > > > - Request(Camera *camera, uint64_t cookie = 0); > > > + Request(std::unique_ptr<Private> d, uint64_t cookie = 0); > > > ~Request(); > > > + static std::unique_ptr<Request> create(std::unique_ptr<Private> d, uint64_t); > > > void reuse(ReuseFlag flags = Default); > > > ControlList &controls() { return *controls_; } > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index a71dc933b911..b7053576fe42 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -1236,12 +1236,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > > > if (ret < 0) > > > return nullptr; > > > - std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie); > > > - > > > - /* Associate the request with the pipeline handler. */ > > > - d->pipe_->registerRequest(request.get()); > > > - > > > - return request; > > > + /* Create a Request from the pipeline handler. */ > > > + return d->pipe_->createRequest(this, cookie); > > > } > > > /** > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 29e0c98a6db5..f2a8cdac0408 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -371,20 +371,25 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > > > } > > > /** > > > - * \fn PipelineHandler::registerRequest() > > > - * \brief Register a request for use by the pipeline handler > > > - * \param[in] request The request to register > > > + * \fn PipelineHandler::createRequest() > > > + * \brief Create a request and register it for use by the pipeline handler > > > + * \param[in] camera The camera the Request belongs to > > > + * \param[in] cookie The Request unique identifier > > > * > > > - * This function is called when the request is created, and allows the pipeline > > > - * handler to perform any one-time initialization it requries for the request. > > > + * This function is called to create a Request by calling createRequestDevice() > > > + * which can be optionally provided by the PipelineHandler derived classes. > > > */ > > > -void PipelineHandler::registerRequest(Request *request) > > > +std::unique_ptr<Request> PipelineHandler::createRequest(Camera *camera, uint64_t cookie) > > > { > > > + std::unique_ptr<Request> request = createRequestDevice(camera, cookie); > > > + > > > /* > > > * Connect the request prepared signal to notify the pipeline handler > > > * when a request is ready to be processed. > > > */ > > > request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); > > > + > > > + return request; > > > } > > > /** > > > @@ -462,6 +467,27 @@ void PipelineHandler::doQueueRequests() > > > } > > > } > > > +/** > > > + * \brief Create a Request from the pipeline handler > > > + * \param[in] camera The camera the Request belongs to > > > + * \param[in] cookie The Request unique identifier > > > + * > > > + * A virtual function that PipelineHandler derived classes are free to override > > > + * in order to initialize a Request with a custom Request::Private derived > > > + * class. > > > + * > > > + * This is the base class implementation that use Request::Private to > > > + * initialize the Request. > > > + * > > > + * \return A unique pointer to a newly created Request > > > + */ > > > +std::unique_ptr<Request> > > > +PipelineHandler::createRequestDevice(Camera *camera, uint64_t cookie) > > > +{ > > > + auto d = std::make_unique<Request::Private>(camera); > > > + return Request::create(std::move(d), cookie); > > > +} > > > + > > > /** > > > * \fn PipelineHandler::queueRequestDevice() > > > * \brief Queue a request to the device > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > index 949c556fa437..d1051ad3d25e 100644 > > > --- a/src/libcamera/request.cpp > > > +++ b/src/libcamera/request.cpp > > > @@ -336,7 +336,7 @@ void Request::Private::timeout() > > > /** > > > * \brief Create a capture request for a camera > > > - * \param[in] camera The camera that creates the request > > > + * \param[in] d The request private data > > > * \param[in] cookie Opaque cookie for application use > > > * > > > * The \a cookie is stored in the request and is accessible through the > > > @@ -344,12 +344,17 @@ void Request::Private::timeout() > > > * the request to an external resource in the request completion handler, and is > > > * completely opaque to libcamera. > > > */ > > > -Request::Request(Camera *camera, uint64_t cookie) > > > - : Extensible(std::make_unique<Private>(camera)), > > > - cookie_(cookie), status_(RequestPending) > > > +std::unique_ptr<Request> Request::create(std::unique_ptr<Private> d, > > > + uint64_t cookie) > > > +{ > > > + return std::make_unique<Request>(std::move(d), cookie); > > > +} > > > + > > > > > > I'm sure this is just me not understanding c++ properly; but what's the > > reason for this function? Why not just use the class constructor? > > > > It's an helper, to easily create a unique_ptr<> from a Request. I > think pipeline handlers can live without it > > > std::unique_ptr<Request> PipelineHandlerRkISP1::createRequestDevice(Camera *camera, > uint64_t cookie) > { > - auto request = std::make_unique<RkISP1Request>(camera); > - return Request::create(std::move(request), cookie); > + return std::make_unique<Request>(std::make_unique<RkISP1Request>(camera), > + cookie); > } > > It just feels nicer > > Also note that applications won't be able to use this function or neither the > constructor has they don't have access to internal headers so they > can't provide a Request::Private instance. As this is on the public API and not much hassle to do manually in the PipelineHandler I suggest to remove that function. In the docs for the constructor we could add a note, that Requests can not be constructed directly but only through a camera. With or without that change: Reviewed-by: Stefan Klug<stefan.klug@ideasonboard.com> Cheers, Stefan > > > > +Request::Request(std::unique_ptr<Request::Private> d, uint64_t cookie) > > > + : Extensible(std::move(d)), cookie_(cookie), status_(RequestPending) > > > { > > > controls_ = new ControlList(controls::controls, > > > - camera->_d()->validator()); > > > + _d()->camera()->_d()->validator()); > > > /** > > > * \todo Add a validator for metadata controls. > > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp > > > index 9c2b0c6a7d99..5714061f88b5 100644 > > > --- a/test/camera/statemachine.cpp > > > +++ b/test/camera/statemachine.cpp > > > @@ -38,10 +38,6 @@ protected: > > > if (camera_->start() != -EACCES) > > > return TestFail; > > > - Request request(camera_.get()); > > > - if (camera_->queueRequest(&request) != -EACCES) > > > - return TestFail; > > > - > > > /* Test operations which should pass. */ > > > if (camera_->release()) > > > return TestFail; > > > @@ -68,10 +64,6 @@ protected: > > > if (camera_->start() != -EACCES) > > > return TestFail; > > > - Request request(camera_.get()); > > > - if (camera_->queueRequest(&request) != -EACCES) > > > - return TestFail; > > > - > > > /* Test operations which should pass. */ > > > if (camera_->stop()) > > > return TestFail; > > > @@ -95,10 +87,6 @@ protected: > > > if (camera_->acquire() != -EBUSY) > > > return TestFail; > > > - Request request1(camera_.get()); > > > - if (camera_->queueRequest(&request1) != -EACCES) > > > - return TestFail; > > > - > > > /* Test operations which should pass. */ > > > std::unique_ptr<Request> request2 = camera_->createRequest(); > > > if (!request2)
Hi Stefan thanks for review On Wed, Mar 06, 2024 at 04:42:03PM +0100, Stefan Klug wrote: > Hi Jacopo, > > maybe it would be nice to mention in the commit message, that this breaks the ABI. > (I know we don't really track this, but hey :-) Indeed this changes the public Request interface. Applications were not supposed to create a Request directly (however it seems to me it was possible, unfortunately) but they were meant to go through Camera::createRequest() and this series doesn't change that. > > On Tue, Mar 05, 2024 at 12:28:14PM +0100, Jacopo Mondi wrote: > > Hi Dan > > > > On Tue, Mar 05, 2024 at 11:10:02AM +0000, Dan Scally wrote: > > > Hi Jacopo > > > > > > On 21/02/2024 17:40, Jacopo Mondi wrote: > > > > In order to allow each pipeline handler to create a Request::Private > > > > derived class to store ancillary data associated with a capture request, > > > > modify the way a Request is created by the PipelineHandler base class. > > > > > > > > Introduce a virtual PipelineHandler::createRequestDevice() that pipeline > > > > handler implementation can optionally override to provide a custom > > > > private data type to initialize the Request with. > > > > > > > > As we can't anymore create a Request without going through an active > > > > camera, drop the queueRequest() checks from the Camera statemachine > > > > test. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > include/libcamera/internal/pipeline_handler.h | 5 ++- > > > > include/libcamera/request.h | 3 +- > > > > src/libcamera/camera.cpp | 8 +--- > > > > src/libcamera/pipeline_handler.cpp | 38 ++++++++++++++++--- > > > > src/libcamera/request.cpp | 15 +++++--- > > > > test/camera/statemachine.cpp | 12 ------ > > > > 6 files changed, 50 insertions(+), 31 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > > index c96944f4ecc4..bbe74f22e5ae 100644 > > > > --- a/include/libcamera/internal/pipeline_handler.h > > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > > @@ -21,6 +21,7 @@ > > > > #include <libcamera/stream.h> > > > > #include "libcamera/internal/ipa_proxy.h" > > > > +#include "libcamera/internal/request.h" > > > > namespace libcamera { > > > > @@ -59,7 +60,7 @@ public: > > > > void stop(Camera *camera); > > > > bool hasPendingRequests(const Camera *camera) const; > > > > - void registerRequest(Request *request); > > > > + std::unique_ptr<Request> createRequest(Camera *camera, uint64_t cookie); > > > > void queueRequest(Request *request); > > > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > > > @@ -74,6 +75,8 @@ protected: > > > > void registerCamera(std::shared_ptr<Camera> camera); > > > > void hotplugMediaDevice(MediaDevice *media); > > > > + virtual std::unique_ptr<Request> createRequestDevice(Camera *camera, > > > > + uint64_t cookie); > > > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > > > > virtual void stopDevice(Camera *camera) = 0; > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > > index dffde1536cad..e16e61a93873 100644 > > > > --- a/include/libcamera/request.h > > > > +++ b/include/libcamera/request.h > > > > @@ -45,9 +45,10 @@ public: > > > > using BufferMap = std::map<const Stream *, FrameBuffer *>; > > > > - Request(Camera *camera, uint64_t cookie = 0); > > > > + Request(std::unique_ptr<Private> d, uint64_t cookie = 0); > > > > ~Request(); > > > > + static std::unique_ptr<Request> create(std::unique_ptr<Private> d, uint64_t); > > > > void reuse(ReuseFlag flags = Default); > > > > ControlList &controls() { return *controls_; } > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > index a71dc933b911..b7053576fe42 100644 > > > > --- a/src/libcamera/camera.cpp > > > > +++ b/src/libcamera/camera.cpp > > > > @@ -1236,12 +1236,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > > > > if (ret < 0) > > > > return nullptr; > > > > - std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie); > > > > - > > > > - /* Associate the request with the pipeline handler. */ > > > > - d->pipe_->registerRequest(request.get()); > > > > - > > > > - return request; > > > > + /* Create a Request from the pipeline handler. */ > > > > + return d->pipe_->createRequest(this, cookie); > > > > } > > > > /** > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > index 29e0c98a6db5..f2a8cdac0408 100644 > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > @@ -371,20 +371,25 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > > > > } > > > > /** > > > > - * \fn PipelineHandler::registerRequest() > > > > - * \brief Register a request for use by the pipeline handler > > > > - * \param[in] request The request to register > > > > + * \fn PipelineHandler::createRequest() > > > > + * \brief Create a request and register it for use by the pipeline handler > > > > + * \param[in] camera The camera the Request belongs to > > > > + * \param[in] cookie The Request unique identifier > > > > * > > > > - * This function is called when the request is created, and allows the pipeline > > > > - * handler to perform any one-time initialization it requries for the request. > > > > + * This function is called to create a Request by calling createRequestDevice() > > > > + * which can be optionally provided by the PipelineHandler derived classes. > > > > */ > > > > -void PipelineHandler::registerRequest(Request *request) > > > > +std::unique_ptr<Request> PipelineHandler::createRequest(Camera *camera, uint64_t cookie) > > > > { > > > > + std::unique_ptr<Request> request = createRequestDevice(camera, cookie); > > > > + > > > > /* > > > > * Connect the request prepared signal to notify the pipeline handler > > > > * when a request is ready to be processed. > > > > */ > > > > request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); > > > > + > > > > + return request; > > > > } > > > > /** > > > > @@ -462,6 +467,27 @@ void PipelineHandler::doQueueRequests() > > > > } > > > > } > > > > +/** > > > > + * \brief Create a Request from the pipeline handler > > > > + * \param[in] camera The camera the Request belongs to > > > > + * \param[in] cookie The Request unique identifier > > > > + * > > > > + * A virtual function that PipelineHandler derived classes are free to override > > > > + * in order to initialize a Request with a custom Request::Private derived > > > > + * class. > > > > + * > > > > + * This is the base class implementation that use Request::Private to > > > > + * initialize the Request. > > > > + * > > > > + * \return A unique pointer to a newly created Request > > > > + */ > > > > +std::unique_ptr<Request> > > > > +PipelineHandler::createRequestDevice(Camera *camera, uint64_t cookie) > > > > +{ > > > > + auto d = std::make_unique<Request::Private>(camera); > > > > + return Request::create(std::move(d), cookie); > > > > +} > > > > + > > > > /** > > > > * \fn PipelineHandler::queueRequestDevice() > > > > * \brief Queue a request to the device > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > > index 949c556fa437..d1051ad3d25e 100644 > > > > --- a/src/libcamera/request.cpp > > > > +++ b/src/libcamera/request.cpp > > > > @@ -336,7 +336,7 @@ void Request::Private::timeout() > > > > /** > > > > * \brief Create a capture request for a camera > > > > - * \param[in] camera The camera that creates the request > > > > + * \param[in] d The request private data > > > > * \param[in] cookie Opaque cookie for application use > > > > * > > > > * The \a cookie is stored in the request and is accessible through the > > > > @@ -344,12 +344,17 @@ void Request::Private::timeout() > > > > * the request to an external resource in the request completion handler, and is > > > > * completely opaque to libcamera. > > > > */ > > > > -Request::Request(Camera *camera, uint64_t cookie) > > > > - : Extensible(std::make_unique<Private>(camera)), > > > > - cookie_(cookie), status_(RequestPending) > > > > +std::unique_ptr<Request> Request::create(std::unique_ptr<Private> d, > > > > + uint64_t cookie) > > > > +{ > > > > + return std::make_unique<Request>(std::move(d), cookie); > > > > +} > > > > + > > > > > > > > > I'm sure this is just me not understanding c++ properly; but what's the > > > reason for this function? Why not just use the class constructor? > > > > > > > It's an helper, to easily create a unique_ptr<> from a Request. I > > think pipeline handlers can live without it > > > > > > std::unique_ptr<Request> PipelineHandlerRkISP1::createRequestDevice(Camera *camera, > > uint64_t cookie) > > { > > - auto request = std::make_unique<RkISP1Request>(camera); > > - return Request::create(std::move(request), cookie); > > + return std::make_unique<Request>(std::make_unique<RkISP1Request>(camera), > > + cookie); > > } > > > > It just feels nicer > > > > Also note that applications won't be able to use this function or neither the > > constructor has they don't have access to internal headers so they > > can't provide a Request::Private instance. > > As this is on the public API and not much hassle to do manually in the > PipelineHandler I suggest to remove that function. In the docs for the > constructor we could add a note, that Requests can not be constructed > directly but only through a camera. > Please not that the existing Request constructor cannot be unsed anymore by applications (and as said, they shouldn't have had to in first place) because they cannot provide a Request::Private instance as it's not part of public headers. When it comes to the interface between the Camera and the PipelineHandler (this is were the new PipelineHandler::createRequest() is introduced in place of PipelineHandler::registerRequest()) it's purely internal to the library. > With or without that change: If most people is bothered by this I can indeed remove it. Please note it's the same pattern implemented by the Camera::create(). > > Reviewed-by: Stefan Klug<stefan.klug@ideasonboard.com> > > Cheers, > Stefan > > > > > > > +Request::Request(std::unique_ptr<Request::Private> d, uint64_t cookie) > > > > + : Extensible(std::move(d)), cookie_(cookie), status_(RequestPending) > > > > { > > > > controls_ = new ControlList(controls::controls, > > > > - camera->_d()->validator()); > > > > + _d()->camera()->_d()->validator()); > > > > /** > > > > * \todo Add a validator for metadata controls. > > > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp > > > > index 9c2b0c6a7d99..5714061f88b5 100644 > > > > --- a/test/camera/statemachine.cpp > > > > +++ b/test/camera/statemachine.cpp > > > > @@ -38,10 +38,6 @@ protected: > > > > if (camera_->start() != -EACCES) > > > > return TestFail; > > > > - Request request(camera_.get()); > > > > - if (camera_->queueRequest(&request) != -EACCES) > > > > - return TestFail; > > > > - > > > > /* Test operations which should pass. */ > > > > if (camera_->release()) > > > > return TestFail; > > > > @@ -68,10 +64,6 @@ protected: > > > > if (camera_->start() != -EACCES) > > > > return TestFail; > > > > - Request request(camera_.get()); > > > > - if (camera_->queueRequest(&request) != -EACCES) > > > > - return TestFail; > > > > - > > > > /* Test operations which should pass. */ > > > > if (camera_->stop()) > > > > return TestFail; > > > > @@ -95,10 +87,6 @@ protected: > > > > if (camera_->acquire() != -EBUSY) > > > > return TestFail; > > > > - Request request1(camera_.get()); > > > > - if (camera_->queueRequest(&request1) != -EACCES) > > > > - return TestFail; > > > > - > > > > /* Test operations which should pass. */ > > > > std::unique_ptr<Request> request2 = camera_->createRequest(); > > > > if (!request2)
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index c96944f4ecc4..bbe74f22e5ae 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -21,6 +21,7 @@ #include <libcamera/stream.h> #include "libcamera/internal/ipa_proxy.h" +#include "libcamera/internal/request.h" namespace libcamera { @@ -59,7 +60,7 @@ public: void stop(Camera *camera); bool hasPendingRequests(const Camera *camera) const; - void registerRequest(Request *request); + std::unique_ptr<Request> createRequest(Camera *camera, uint64_t cookie); void queueRequest(Request *request); bool completeBuffer(Request *request, FrameBuffer *buffer); @@ -74,6 +75,8 @@ protected: void registerCamera(std::shared_ptr<Camera> camera); void hotplugMediaDevice(MediaDevice *media); + virtual std::unique_ptr<Request> createRequestDevice(Camera *camera, + uint64_t cookie); virtual int queueRequestDevice(Camera *camera, Request *request) = 0; virtual void stopDevice(Camera *camera) = 0; diff --git a/include/libcamera/request.h b/include/libcamera/request.h index dffde1536cad..e16e61a93873 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -45,9 +45,10 @@ public: using BufferMap = std::map<const Stream *, FrameBuffer *>; - Request(Camera *camera, uint64_t cookie = 0); + Request(std::unique_ptr<Private> d, uint64_t cookie = 0); ~Request(); + static std::unique_ptr<Request> create(std::unique_ptr<Private> d, uint64_t); void reuse(ReuseFlag flags = Default); ControlList &controls() { return *controls_; } diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index a71dc933b911..b7053576fe42 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1236,12 +1236,8 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) if (ret < 0) return nullptr; - std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie); - - /* Associate the request with the pipeline handler. */ - d->pipe_->registerRequest(request.get()); - - return request; + /* Create a Request from the pipeline handler. */ + return d->pipe_->createRequest(this, cookie); } /** diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 29e0c98a6db5..f2a8cdac0408 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -371,20 +371,25 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const } /** - * \fn PipelineHandler::registerRequest() - * \brief Register a request for use by the pipeline handler - * \param[in] request The request to register + * \fn PipelineHandler::createRequest() + * \brief Create a request and register it for use by the pipeline handler + * \param[in] camera The camera the Request belongs to + * \param[in] cookie The Request unique identifier * - * This function is called when the request is created, and allows the pipeline - * handler to perform any one-time initialization it requries for the request. + * This function is called to create a Request by calling createRequestDevice() + * which can be optionally provided by the PipelineHandler derived classes. */ -void PipelineHandler::registerRequest(Request *request) +std::unique_ptr<Request> PipelineHandler::createRequest(Camera *camera, uint64_t cookie) { + std::unique_ptr<Request> request = createRequestDevice(camera, cookie); + /* * Connect the request prepared signal to notify the pipeline handler * when a request is ready to be processed. */ request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); + + return request; } /** @@ -462,6 +467,27 @@ void PipelineHandler::doQueueRequests() } } +/** + * \brief Create a Request from the pipeline handler + * \param[in] camera The camera the Request belongs to + * \param[in] cookie The Request unique identifier + * + * A virtual function that PipelineHandler derived classes are free to override + * in order to initialize a Request with a custom Request::Private derived + * class. + * + * This is the base class implementation that use Request::Private to + * initialize the Request. + * + * \return A unique pointer to a newly created Request + */ +std::unique_ptr<Request> +PipelineHandler::createRequestDevice(Camera *camera, uint64_t cookie) +{ + auto d = std::make_unique<Request::Private>(camera); + return Request::create(std::move(d), cookie); +} + /** * \fn PipelineHandler::queueRequestDevice() * \brief Queue a request to the device diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 949c556fa437..d1051ad3d25e 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -336,7 +336,7 @@ void Request::Private::timeout() /** * \brief Create a capture request for a camera - * \param[in] camera The camera that creates the request + * \param[in] d The request private data * \param[in] cookie Opaque cookie for application use * * The \a cookie is stored in the request and is accessible through the @@ -344,12 +344,17 @@ void Request::Private::timeout() * the request to an external resource in the request completion handler, and is * completely opaque to libcamera. */ -Request::Request(Camera *camera, uint64_t cookie) - : Extensible(std::make_unique<Private>(camera)), - cookie_(cookie), status_(RequestPending) +std::unique_ptr<Request> Request::create(std::unique_ptr<Private> d, + uint64_t cookie) +{ + return std::make_unique<Request>(std::move(d), cookie); +} + +Request::Request(std::unique_ptr<Request::Private> d, uint64_t cookie) + : Extensible(std::move(d)), cookie_(cookie), status_(RequestPending) { controls_ = new ControlList(controls::controls, - camera->_d()->validator()); + _d()->camera()->_d()->validator()); /** * \todo Add a validator for metadata controls. diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp index 9c2b0c6a7d99..5714061f88b5 100644 --- a/test/camera/statemachine.cpp +++ b/test/camera/statemachine.cpp @@ -38,10 +38,6 @@ protected: if (camera_->start() != -EACCES) return TestFail; - Request request(camera_.get()); - if (camera_->queueRequest(&request) != -EACCES) - return TestFail; - /* Test operations which should pass. */ if (camera_->release()) return TestFail; @@ -68,10 +64,6 @@ protected: if (camera_->start() != -EACCES) return TestFail; - Request request(camera_.get()); - if (camera_->queueRequest(&request) != -EACCES) - return TestFail; - /* Test operations which should pass. */ if (camera_->stop()) return TestFail; @@ -95,10 +87,6 @@ protected: if (camera_->acquire() != -EBUSY) return TestFail; - Request request1(camera_.get()); - if (camera_->queueRequest(&request1) != -EACCES) - return TestFail; - /* Test operations which should pass. */ std::unique_ptr<Request> request2 = camera_->createRequest(); if (!request2)
In order to allow each pipeline handler to create a Request::Private derived class to store ancillary data associated with a capture request, modify the way a Request is created by the PipelineHandler base class. Introduce a virtual PipelineHandler::createRequestDevice() that pipeline handler implementation can optionally override to provide a custom private data type to initialize the Request with. As we can't anymore create a Request without going through an active camera, drop the queueRequest() checks from the Camera statemachine test. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- include/libcamera/internal/pipeline_handler.h | 5 ++- include/libcamera/request.h | 3 +- src/libcamera/camera.cpp | 8 +--- src/libcamera/pipeline_handler.cpp | 38 ++++++++++++++++--- src/libcamera/request.cpp | 15 +++++--- test/camera/statemachine.cpp | 12 ------ 6 files changed, 50 insertions(+), 31 deletions(-)