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

[Kieran Bingham]

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:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>                 if (data->swIsp_)
>                         data->swIsp_->queueRequest(request->sequence(), request->controls());
>         }
> -- 
> 2.44.1
>