[RFC PATCH 2/4] libcamera: internal: Allow to limit the number of queued requests

[Sven Püschel]

Hi Jacopo,

On 5/30/25 11:02, Jacopo Mondi wrote:
> Hi Sven, Stefan
>
> On Tue, May 27, 2025 at 05:46:48PM +0200, Sven Püschel wrote:
>> Hi Stefan,
>>
>> On 5/26/25 23:42, Stefan Klug wrote:
>>> Allow pipeline handler classes to limit the maximum number of requests that
>>> get queued using queueRequestDevice().
>>>
>>> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
>>> ---
>>>    include/libcamera/internal/pipeline_handler.h | 5 +++++
>>>    src/libcamera/pipeline_handler.cpp            | 3 +++
>>>    2 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>> index dedc29e815fb..86b86d5971dc 100644
>>> --- a/include/libcamera/internal/pipeline_handler.h
>>> +++ b/include/libcamera/internal/pipeline_handler.h
>>> @@ -79,6 +79,11 @@ protected:
>>>    	virtual bool acquireDevice(Camera *camera);
>>>    	virtual void releaseDevice(Camera *camera);
>>> +	virtual unsigned int maxQueuedRequestsDevice() const
>>> +	{
>>> +		return std::numeric_limits<unsigned int>::max();
>>> +	}
>>> +
>> is there a benefit of having a virtual function instead of a variable
>> (initialized to max by default and potentially changed in the subclass
>> constructors)?
> Just for sake of discussion, should we go in the other direction and
> make this a pure virtual function in the base class to make sure every
> pipeline handler provides an implementation ?

Option 4: Additional constructor parameter which initializes a constant 
member variable ^-^

My reasons why I prefer a variable:

- A function returning a constant just looks weird to me (reminds me of 
https://xkcd.com/221/ )
- (for a constant) code can assume that it won't change during it's 
lifetime (thinking about cases like == maxQueueRequestsDevice() pointed 
out in another comment)
- If there is an actual use case requiring a variable number, it's not 
hard to change it later to a function


>
>>>    	CameraManager *manager_;
>>>    private:
>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>> index 14d8602e67d8..1853bca71371 100644
>>> --- a/src/libcamera/pipeline_handler.cpp
>>> +++ b/src/libcamera/pipeline_handler.cpp
>>> @@ -490,6 +490,9 @@ void PipelineHandler::doQueueRequests()
>>>    		Camera::Private *data = camera->_d();
>>>    		while (!data->waitingRequests_.empty()) {
>>> +			if (data->queuedRequests_.size() == maxQueuedRequestsDevice())
>>> +				break;
>>> +
>>>    			Request *request = data->waitingRequests_.front();
>>>    			if (!request->_d()->prepared_)
>>>    				break;