[1/5] libcamera: Allow pipeline to provide a Private request
diff mbox series

Message ID 20240221174015.52958-2-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Replace IPU3/RkISP1FrameInfo
Related show

Commit Message

Jacopo Mondi Feb. 21, 2024, 5:40 p.m. UTC
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(-)

Comments

Dan Scally March 5, 2024, 11:10 a.m. UTC | #1
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)
Jacopo Mondi March 5, 2024, 11:28 a.m. UTC | #2
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)
Dan Scally March 5, 2024, 11:40 a.m. UTC | #3
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)
Stefan Klug March 6, 2024, 3:42 p.m. UTC | #4
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)
Jacopo Mondi March 6, 2024, 4:05 p.m. UTC | #5
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)

Patch
diff mbox series

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)