[PATCH] libcamera: software_isp: Clean up pending requests on stop

[Robert Mader]

Hey, thanks a lot for the patch!

I gave it a quick try on my Pixel 3a and while the revision here avoids 
the assertion most of the time and makes things much more stable, I 
still managed to trigger it with enough trying (4 buffers, 
starting/stopping/switching cameras in Gnome Snapshot). I haven't tried 
the suggestion from Kieran yet, but to me it looks like Stop() is 
currently still somehow racy - at least the way it's used by Pipewire.

Regards

On 08.10.24 17:09, Milan Zamazal wrote:
> PipelineHandler::stop() calls stopDevice() method to perform pipeline
> specific cleanup and then completes waiting requests.  If any queued
> requests remain, an assertion error is raised.
>
> Software ISP stores request buffers in
> SimpleCameraData::conversionQueue_ and queues them as V4L2 signals
> bufferReady.  stopDevice() cleanup forgets to clean up the buffers and
> their requests from conversionQueue_, possibly resulting in the
> assertion error.  This patch fixes the omission.
>
> The problem wasn't very visible when
> SimplePipelineHandler::kNumInternalBuffers (the number of buffers
> allocated in V4L2) was equal to the number of buffers exported from
> software ISP.  But when the number of the exported buffers was increased
> by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion
> error started pop up in some environments.  Increasing the number of the
> buffers much more, e.g. to 9, makes the problem very reproducible.
>
> Each pipeline uses its own mechanism to track the requests to clean up
> and it can't be excluded that similar omissions are present in other
> places.  But there is no obvious way to make a common cleanup for all
> the pipelines (except for doing it instead of raising the assertion
> error, which is probably undesirable, in order not to hide incomplete
> pipeline specific cleanups).
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>   src/libcamera/pipeline/simple/simple.cpp | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3..4e8504922 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -284,6 +284,7 @@ public:
>   	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
>   	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
>   	bool useConversion_;
> +	void clearIncompleteRequests();
>   
>   	std::unique_ptr<Converter> converter_;
>   	std::unique_ptr<SoftwareIsp> swIsp_;
> @@ -897,6 +898,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
>   		pipe->completeRequest(request);
>   }
>   
> +void SimpleCameraData::clearIncompleteRequests()
> +{
> +	while (!conversionQueue_.empty()) {
> +		for (auto &item : conversionQueue_.front()) {
> +			FrameBuffer *outputBuffer = item.second;
> +			Request *request = outputBuffer->request();
> +			pipe()->completeBuffer(request, outputBuffer);
> +			pipe()->completeRequest(request);
> +		}
> +		conversionQueue_.pop();
> +	}
> +}
> +
>   void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>   {
>   	swIsp_->processStats(frame, bufferId,
> @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>   	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>   
>   	data->conversionBuffers_.clear();
> +	data->clearIncompleteRequests();
>   
>   	releasePipeline(data);
>   }

-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718