[PATCH v4 2/3] libcamera: simple: Track requests in conversionQueue_

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-10-18 10:27:15)
>> Simple pipeline retrieves the requests to complete from the
>> conversionQueue_.  This patch stores the requests in conversionQueue_
>
>> explicitly.  This explicit tracking is supposed to be preferred to
>> implicit retrieval and it simplifies the completion code a bit here and
>> in the followup patch that adds request cleanup on stop.
>> 
>> The change as implemented assumes that all the buffers in each of the
>> conversionQueue_ elements point to the same request, the one specified.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/libcamera/pipeline/simple/simple.cpp | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 3ddce71d3..a1339d87c 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -282,7 +282,7 @@ public:
>>         std::unique_ptr<DelayedControls> delayedCtrls_;
>>  
>>         std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
>> -       std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
>> +       std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_;
>>         bool useConversion_;
>>  
>>         std::unique_ptr<Converter> converter_;
>> @@ -813,16 +813,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>>                 if (conversionQueue_.empty())
>>                         return;
>>  
>> -               Request *request = nullptr;
>> -               for (auto &item : conversionQueue_.front()) {
>> -                       FrameBuffer *outputBuffer = item.second;
>> -                       request = outputBuffer->request();
>> -                       pipe->completeBuffer(request, outputBuffer);
>> -               }
>> +               Request *request = conversionQueue_.front().first;
>> +               for (auto &item : conversionQueue_.front().second)
>> +                       pipe->completeBuffer(request, item.second);
>
> I'll always have a personal dislike for std::pair<> because
> something.first and something.second just really doesn't tell me what
> it's supposed to be.
>
> Even more so with
> 	"auto &item : conversionQueue_.front().second"
>
>
>
>
>> +               pipe->completeRequest(request);
>>                 conversionQueue_.pop();
>>  
>> -               if (request)
>> -                       pipe->completeRequest(request);
>>                 return;
>>         }
>>  
>> @@ -838,7 +834,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>>  
>>         if (useConversion_ && !conversionQueue_.empty()) {
>>                 const std::map<const Stream *, FrameBuffer *> &outputs =
>> -                       conversionQueue_.front();
>> +                       conversionQueue_.front().second;
>>                 if (!outputs.empty()) {
>>                         FrameBuffer *outputBuffer = outputs.begin()->second;
>>                         if (outputBuffer)
>> @@ -862,7 +858,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>>                 }
>>  
>>                 if (converter_)
>> -                       converter_->queueBuffers(buffer, conversionQueue_.front());
>> +                       converter_->queueBuffers(buffer, conversionQueue_.front().second);
>>                 else
>>                         /*
>>                          * request->sequence() cannot be retrieved from `buffer' inside
>> @@ -870,7 +866,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>>                          * already here.
>>                          */
>>                         swIsp_->queueBuffers(request->sequence(), buffer,
>> -                                            conversionQueue_.front());
>> +                                            conversionQueue_.front().second);
>>  
>>                 conversionQueue_.pop();
>>                 return;
>> @@ -1434,7 +1430,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>>         }
>>  
>>         if (data->useConversion_) {
>> -               data->conversionQueue_.push(std::move(buffers));
>> +               data->conversionQueue_.push(std::make_pair(request, std::move(buffers)));
>
> As far as I can tell all the above looks correct, even with my dislike
> of std::pair - and it's /working/
>
> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> If there were a reason to respin, and std::pair could be more readable
> I'd be happier - but I can still do this I think:

Let's make you happier, this is a safe and easy change, I'll post v5
with it.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>>                 if (data->swIsp_)
>>                         data->swIsp_->queueRequest(request->sequence(), request->controls());
>>         }
>> -- 
>> 2.44.1
>>