[PATCH 1/5] libcamera: Allow pipeline to provide a Private request
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Mar 6 17:05:02 CET 2024
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 at 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 at 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)
More information about the libcamera-devel
mailing list