[PATCH 1/5] libcamera: Allow pipeline to provide a Private request

Dan Scally dan.scally at ideasonboard.com
Tue Mar 5 12:40:31 CET 2024


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 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);
>   }
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 at ideasonboard.com>

and

Tested-by: Daniel Scally <dan.scally at 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)


More information about the libcamera-devel mailing list