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

21558 diff mbox series

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 request must be also canceled, which
requires introducing a little PipelineHandler::cancelRequest helper in
order to be able to access the private cancel() method.

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@redhat.com>
---
 include/libcamera/internal/pipeline_handler.h |  1 +
 src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
 src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
 3 files changed, 31 insertions(+), 7 deletions(-)

Milan Zamazal <mzamazal@redhat.com> writes:

> 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 request must be also canceled, which
> requires introducing a little PipelineHandler::cancelRequest helper in
> order to be able to access the private cancel() method.
>
> 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@redhat.com>

Changes in v2:
- The request is additionally canceled.
- New helper method PipelineHandler::cancelRequest() introduced.

Robert, could you please test v2?

> ---
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>  3 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 0d3808036..fb28a18d0 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -60,6 +60,7 @@ public:
>  
>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>  	void completeRequest(Request *request);
> +	void cancelRequest(Request *request);
>  
>  	std::string configurationFile(const std::string &subdir,
>  				      const std::string &name) const;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
> +		}
> +		conversionQueue_.pop();
> +	}
> +}
> +
>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>  {
>  	swIsp_->processStats(frame, bufferId,
> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>  
>  	data->conversionBuffers_.clear();
> +	data->clearIncompleteRequests();
>  
>  	releasePipeline(data);
>  }
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index e59404691..c9cb11f0f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>  	while (!waitingRequests_.empty()) {
>  		Request *request = waitingRequests_.front();
>  		waitingRequests_.pop();
> -
> -		request->_d()->cancel();
> -		completeRequest(request);
> +		cancelRequest(request);
>  	}
>  
>  	/* Make sure no requests are pending. */
> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>  	}
>  
>  	int ret = queueRequestDevice(camera, request);
> -	if (ret) {
> -		request->_d()->cancel();
> -		completeRequest(request);
> -	}
> +	if (ret)
> +		cancelRequest(request);
>  }
>  
>  /**
> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>  	}
>  }
>  
> +/**
> + * \brief Cancel request and signal its completion
> + * \param[in] request The request to cancel
> + *
> + * This function cancels the request in addition to its completion. The same
> + * rules as for completeRequest() apply.
> + */
> +void PipelineHandler::cancelRequest(Request *request)
> +{
> +	request->_d()->cancel();
> +	completeRequest(request);
> +}
> +
>  /**
>   * \brief Retrieve the absolute path to a platform configuration file
>   * \param[in] subdir The pipeline handler specific subdirectory name

Hi, thanks for the patch!

On 09.10.24 10:39, Milan Zamazal wrote:
> Milan Zamazal <mzamazal@redhat.com> writes:
>
>> 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 request must be also canceled, which
>> requires introducing a little PipelineHandler::cancelRequest helper in
>> order to be able to access the private cancel() method.
>>
>> 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@redhat.com>
> Changes in v2:
> - The request is additionally canceled.
> - New helper method PipelineHandler::cancelRequest() introduced.
>
> Robert, could you please test v2?

I gave it a quick go for 5 minutes - generally it seems to work great, 
however trying enough quick camera switching I still run into the assert.

So probably, on top of this, we'll need something implementing what 
Kieran suggested:

 > (Though I think we should reject any incoming requests as soon as we 
hit stop())

>> ---
>>   include/libcamera/internal/pipeline_handler.h |  1 +
>>   src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>>   src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>>   3 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> index 0d3808036..fb28a18d0 100644
>> --- a/include/libcamera/internal/pipeline_handler.h
>> +++ b/include/libcamera/internal/pipeline_handler.h
>> @@ -60,6 +60,7 @@ public:
>>   
>>   	bool completeBuffer(Request *request, FrameBuffer *buffer);
>>   	void completeRequest(Request *request);
>> +	void cancelRequest(Request *request);
>>   
>>   	std::string configurationFile(const std::string &subdir,
>>   				      const std::string &name) const;
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>> +		}
>> +		conversionQueue_.pop();
>> +	}
>> +}
>> +
>>   void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>   {
>>   	swIsp_->processStats(frame, bufferId,
>> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>   	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>   
>>   	data->conversionBuffers_.clear();
>> +	data->clearIncompleteRequests();
>>   
>>   	releasePipeline(data);
>>   }
>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> index e59404691..c9cb11f0f 100644
>> --- a/src/libcamera/pipeline_handler.cpp
>> +++ b/src/libcamera/pipeline_handler.cpp
>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>>   	while (!waitingRequests_.empty()) {
>>   		Request *request = waitingRequests_.front();
>>   		waitingRequests_.pop();
>> -
>> -		request->_d()->cancel();
>> -		completeRequest(request);
>> +		cancelRequest(request);
>>   	}
>>   
>>   	/* Make sure no requests are pending. */
>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>>   	}
>>   
>>   	int ret = queueRequestDevice(camera, request);
>> -	if (ret) {
>> -		request->_d()->cancel();
>> -		completeRequest(request);
>> -	}
>> +	if (ret)
>> +		cancelRequest(request);
>>   }
>>   
>>   /**
>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>>   	}
>>   }
>>   
>> +/**
>> + * \brief Cancel request and signal its completion
>> + * \param[in] request The request to cancel

Small typo here


>> + *
>> + * This function cancels the request in addition to its completion. The same
>> + * rules as for completeRequest() apply.
>> + */
>> +void PipelineHandler::cancelRequest(Request *request)
>> +{
>> +	request->_d()->cancel();
>> +	completeRequest(request);
>> +}
>> +
>>   /**
>>    * \brief Retrieve the absolute path to a platform configuration file
>>    * \param[in] subdir The pipeline handler specific subdirectory name

Quoting Robert Mader (2024-10-09 10:41:00)
> Hi, thanks for the patch!
> 
> On 09.10.24 10:39, Milan Zamazal wrote:
> > Milan Zamazal <mzamazal@redhat.com> writes:
> >
> >> 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 request must be also canceled, which
> >> requires introducing a little PipelineHandler::cancelRequest helper in
> >> order to be able to access the private cancel() method.
> >>
> >> 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@redhat.com>
> > Changes in v2:
> > - The request is additionally canceled.
> > - New helper method PipelineHandler::cancelRequest() introduced.
> >
> > Robert, could you please test v2?
> 
> I gave it a quick go for 5 minutes - generally it seems to work great, 
> however trying enough quick camera switching I still run into the assert.
> 
> So probably, on top of this, we'll need something implementing what 
> Kieran suggested:
> 
>  > (Though I think we should reject any incoming requests as soon as we 
> hit stop())

Sorry - there I was saying that should /already/ be happening. The state
machine should put the camera in to a stopping state where new incoming
requests will be rejected...

https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1401

Unless this is still somehow racy ;-(

--
Kieran


> 
> >> ---
> >>   include/libcamera/internal/pipeline_handler.h |  1 +
> >>   src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
> >>   src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
> >>   3 files changed, 31 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> >> index 0d3808036..fb28a18d0 100644
> >> --- a/include/libcamera/internal/pipeline_handler.h
> >> +++ b/include/libcamera/internal/pipeline_handler.h
> >> @@ -60,6 +60,7 @@ public:
> >>   
> >>      bool completeBuffer(Request *request, FrameBuffer *buffer);
> >>      void completeRequest(Request *request);
> >> +    void cancelRequest(Request *request);
> >>   
> >>      std::string configurationFile(const std::string &subdir,
> >>                                    const std::string &name) const;
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
> >> +            }
> >> +            conversionQueue_.pop();
> >> +    }
> >> +}
> >> +
> >>   void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> >>   {
> >>      swIsp_->processStats(frame, bufferId,
> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> >>      video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
> >>   
> >>      data->conversionBuffers_.clear();
> >> +    data->clearIncompleteRequests();
> >>   
> >>      releasePipeline(data);
> >>   }
> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> index e59404691..c9cb11f0f 100644
> >> --- a/src/libcamera/pipeline_handler.cpp
> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
> >>      while (!waitingRequests_.empty()) {
> >>              Request *request = waitingRequests_.front();
> >>              waitingRequests_.pop();
> >> -
> >> -            request->_d()->cancel();
> >> -            completeRequest(request);
> >> +            cancelRequest(request);
> >>      }
> >>   
> >>      /* Make sure no requests are pending. */
> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
> >>      }
> >>   
> >>      int ret = queueRequestDevice(camera, request);
> >> -    if (ret) {
> >> -            request->_d()->cancel();
> >> -            completeRequest(request);
> >> -    }
> >> +    if (ret)
> >> +            cancelRequest(request);
> >>   }
> >>   
> >>   /**
> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
> >>      }
> >>   }
> >>   
> >> +/**
> >> + * \brief Cancel request and signal its completion
> >> + * \param[in] request The request to cancel
> 
> Small typo here
> 
> 
> >> + *
> >> + * This function cancels the request in addition to its completion. The same
> >> + * rules as for completeRequest() apply.
> >> + */
> >> +void PipelineHandler::cancelRequest(Request *request)
> >> +{
> >> +    request->_d()->cancel();
> >> +    completeRequest(request);
> >> +}
> >> +
> >>   /**
> >>    * \brief Retrieve the absolute path to a platform configuration file
> >>    * \param[in] subdir The pipeline handler specific subdirectory name
> 
> -- 
> Robert Mader
> Consultant Software Developer
> 
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718
>

Quoting Milan Zamazal (2024-10-09 09:35:56)
> 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 request must be also canceled, which
> requires introducing a little PipelineHandler::cancelRequest helper in
> order to be able to access the private cancel() method.
> 
> 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@redhat.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>  3 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 0d3808036..fb28a18d0 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -60,6 +60,7 @@ public:
>  
>         bool completeBuffer(Request *request, FrameBuffer *buffer);
>         void completeRequest(Request *request);
> +       void cancelRequest(Request *request);
>  
>         std::string configurationFile(const std::string &subdir,
>                                       const std::string &name) const;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
> +               }
> +               conversionQueue_.pop();
> +       }
> +}
> +
>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>  {
>         swIsp_->processStats(frame, bufferId,
> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>         video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>  
>         data->conversionBuffers_.clear();
> +       data->clearIncompleteRequests();

Do the requests have any existing reference to anything in teh
conversionBuffers_ that would need to make sure we cancel/clear before
releasing the conversionBuffers_? 

Probably not - but just checking.

>  
>         releasePipeline(data);
>  }
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index e59404691..c9cb11f0f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp

Sorry to be a pain on nitpicking - but the changes to pipeline_handler
are core - not SoftISP and the changes below will function standalone,
so this should be a separate patch.

Then the softISP can use it!

Something like "libcamera: pipeline_handler: Provide cancelRequest"

is definitely something I want to be visible in the changelogs, and
clear that other pipeline handlers can/should use it.

Once split to two - I'd probably say this already goes in the right
direction to get merged even if there is possible still another race to
investigate on top...

--
Kieran

> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>         while (!waitingRequests_.empty()) {
>                 Request *request = waitingRequests_.front();
>                 waitingRequests_.pop();
> -
> -               request->_d()->cancel();
> -               completeRequest(request);
> +               cancelRequest(request);
>         }
>  
>         /* Make sure no requests are pending. */
> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>         }
>  
>         int ret = queueRequestDevice(camera, request);
> -       if (ret) {
> -               request->_d()->cancel();
> -               completeRequest(request);
> -       }
> +       if (ret)
> +               cancelRequest(request);
>  }
>  
>  /**
> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>         }
>  }
>  
> +/**
> + * \brief Cancel request and signal its completion
> + * \param[in] request The request to cancel
> + *
> + * This function cancels the request in addition to its completion. The same
> + * rules as for completeRequest() apply.
> + */
> +void PipelineHandler::cancelRequest(Request *request)
> +{
> +       request->_d()->cancel();
> +       completeRequest(request);
> +}
> +
>  /**
>   * \brief Retrieve the absolute path to a platform configuration file
>   * \param[in] subdir The pipeline handler specific subdirectory name
> -- 
> 2.44.1
>

Hi Robert,

thank you for testing and review.

Robert Mader <robert.mader@collabora.com> writes:

> Hi, thanks for the patch!
>
> On 09.10.24 10:39, Milan Zamazal wrote:
>> Milan Zamazal <mzamazal@redhat.com> writes:
>>
>>> 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 request must be also canceled, which
>>> requires introducing a little PipelineHandler::cancelRequest helper in
>>> order to be able to access the private cancel() method.
>>>
>>> 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@redhat.com>
>> Changes in v2:
>> - The request is additionally canceled.
>> - New helper method PipelineHandler::cancelRequest() introduced.
>>
>> Robert, could you please test v2?
>
> I gave it a quick go for 5 minutes - generally it seems to work great, however trying enough quick camera
> switching I still run into the assert.
>
> So probably, on top of this, we'll need something implementing what Kieran suggested:
>
>> (Though I think we should reject any incoming requests as soon as we hit stop())
>
>>> ---
>>>   include/libcamera/internal/pipeline_handler.h |  1 +
>>>   src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>>>   src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>>>   3 files changed, 31 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>> index 0d3808036..fb28a18d0 100644
>>> --- a/include/libcamera/internal/pipeline_handler.h
>>> +++ b/include/libcamera/internal/pipeline_handler.h
>>> @@ -60,6 +60,7 @@ public:
>>>     	bool completeBuffer(Request *request, FrameBuffer *buffer);
>>>   	void completeRequest(Request *request);
>>> +	void cancelRequest(Request *request);
>>>     	std::string configurationFile(const std::string &subdir,
>>>   				      const std::string &name) const;
>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>>> +		}
>>> +		conversionQueue_.pop();
>>> +	}
>>> +}
>>> +
>>>   void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>>   {
>>>   	swIsp_->processStats(frame, bufferId,
>>> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>>   	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>>     	data->conversionBuffers_.clear();
>>> +	data->clearIncompleteRequests();
>>>     	releasePipeline(data);
>>>   }
>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>> index e59404691..c9cb11f0f 100644
>>> --- a/src/libcamera/pipeline_handler.cpp
>>> +++ b/src/libcamera/pipeline_handler.cpp
>>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>>>   	while (!waitingRequests_.empty()) {
>>>   		Request *request = waitingRequests_.front();
>>>   		waitingRequests_.pop();
>>> -
>>> -		request->_d()->cancel();
>>> -		completeRequest(request);
>>> +		cancelRequest(request);
>>>   	}
>>>     	/* Make sure no requests are pending. */
>>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>>>   	}
>>>     	int ret = queueRequestDevice(camera, request);
>>> -	if (ret) {
>>> -		request->_d()->cancel();
>>> -		completeRequest(request);
>>> -	}
>>> +	if (ret)
>>> +		cancelRequest(request);
>>>   }
>>>     /**
>>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>>>   	}
>>>   }
>>>   +/**
>>> + * \brief Cancel request and signal its completion
>>> + * \param[in] request The request to cancel
>
> Small typo here

Sorry, I can't spot the typo, what typo exactly?

>>> + *
>>> + * This function cancels the request in addition to its completion. The same
>>> + * rules as for completeRequest() apply.
>>> + */
>>> +void PipelineHandler::cancelRequest(Request *request)
>>> +{
>>> +	request->_d()->cancel();
>>> +	completeRequest(request);
>>> +}
>>> +
>>>   /**
>>>    * \brief Retrieve the absolute path to a platform configuration file
>>>    * \param[in] subdir The pipeline handler specific subdirectory name

On 09.10.24 16:05, Milan Zamazal wrote:
> Hi Robert,
>
> thank you for testing and review.
>
> Robert Mader <robert.mader@collabora.com> writes:
>
>> Hi, thanks for the patch!
>>
>> On 09.10.24 10:39, Milan Zamazal wrote:
>>> Milan Zamazal <mzamazal@redhat.com> writes:
>>>
>>>> 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 request must be also canceled, which
>>>> requires introducing a little PipelineHandler::cancelRequest helper in
>>>> order to be able to access the private cancel() method.
>>>>
>>>> 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@redhat.com>
>>> Changes in v2:
>>> - The request is additionally canceled.
>>> - New helper method PipelineHandler::cancelRequest() introduced.
>>>
>>> Robert, could you please test v2?
>> I gave it a quick go for 5 minutes - generally it seems to work great, however trying enough quick camera
>> switching I still run into the assert.
>>
>> So probably, on top of this, we'll need something implementing what Kieran suggested:
>>
>>> (Though I think we should reject any incoming requests as soon as we hit stop())
>>>> ---
>>>>    include/libcamera/internal/pipeline_handler.h |  1 +
>>>>    src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>>>>    src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>>>>    3 files changed, 31 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>>> index 0d3808036..fb28a18d0 100644
>>>> --- a/include/libcamera/internal/pipeline_handler.h
>>>> +++ b/include/libcamera/internal/pipeline_handler.h
>>>> @@ -60,6 +60,7 @@ public:
>>>>      	bool completeBuffer(Request *request, FrameBuffer *buffer);
>>>>    	void completeRequest(Request *request);
>>>> +	void cancelRequest(Request *request);
>>>>      	std::string configurationFile(const std::string &subdir,
>>>>    				      const std::string &name) const;
>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>>>> +		}
>>>> +		conversionQueue_.pop();
>>>> +	}
>>>> +}
>>>> +
>>>>    void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>>>    {
>>>>    	swIsp_->processStats(frame, bufferId,
>>>> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>>>    	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>>>      	data->conversionBuffers_.clear();
>>>> +	data->clearIncompleteRequests();
>>>>      	releasePipeline(data);
>>>>    }
>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>>> index e59404691..c9cb11f0f 100644
>>>> --- a/src/libcamera/pipeline_handler.cpp
>>>> +++ b/src/libcamera/pipeline_handler.cpp
>>>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>>>>    	while (!waitingRequests_.empty()) {
>>>>    		Request *request = waitingRequests_.front();
>>>>    		waitingRequests_.pop();
>>>> -
>>>> -		request->_d()->cancel();
>>>> -		completeRequest(request);
>>>> +		cancelRequest(request);
>>>>    	}
>>>>      	/* Make sure no requests are pending. */
>>>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>>>>    	}
>>>>      	int ret = queueRequestDevice(camera, request);
>>>> -	if (ret) {
>>>> -		request->_d()->cancel();
>>>> -		completeRequest(request);
>>>> -	}
>>>> +	if (ret)
>>>> +		cancelRequest(request);
>>>>    }
>>>>      /**
>>>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>>>>    	}
>>>>    }
>>>>    +/**
>>>> + * \brief Cancel request and signal its completion
>>>> + * \param[in] request The request to cancel
>> Small typo here
> Sorry, I can't spot the typo, what typo exactly?

Urgh, never mind, I though the "request The request.." part was a typo - 
but that's the arg name 🤦

>
>>>> + *
>>>> + * This function cancels the request in addition to its completion. The same
>>>> + * rules as for completeRequest() apply.
>>>> + */
>>>> +void PipelineHandler::cancelRequest(Request *request)
>>>> +{
>>>> +	request->_d()->cancel();
>>>> +	completeRequest(request);
>>>> +}
>>>> +
>>>>    /**
>>>>     * \brief Retrieve the absolute path to a platform configuration file
>>>>     * \param[in] subdir The pipeline handler specific subdirectory name

On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
> requires introducing a little PipelineHandler::cancelRequest helper in
> order to be able to access the private cancel() method.
> 
> 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@redhat.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>  3 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 0d3808036..fb28a18d0 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -60,6 +60,7 @@ public:
>  
>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>  	void completeRequest(Request *request);
> +	void cancelRequest(Request *request);
>  
>  	std::string configurationFile(const std::string &subdir,
>  				      const std::string &name) const;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);

Aren't you cancelling the same request multiple times ?

> +		}
> +		conversionQueue_.pop();
> +	}
> +}
> +
>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>  {
>  	swIsp_->processStats(frame, bufferId,
> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>  
>  	data->conversionBuffers_.clear();
> +	data->clearIncompleteRequests();
>  
>  	releasePipeline(data);
>  }
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index e59404691..c9cb11f0f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>  	while (!waitingRequests_.empty()) {
>  		Request *request = waitingRequests_.front();
>  		waitingRequests_.pop();
> -
> -		request->_d()->cancel();
> -		completeRequest(request);
> +		cancelRequest(request);
>  	}
>  
>  	/* Make sure no requests are pending. */
> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>  	}
>  
>  	int ret = queueRequestDevice(camera, request);
> -	if (ret) {
> -		request->_d()->cancel();
> -		completeRequest(request);
> -	}
> +	if (ret)
> +		cancelRequest(request);
>  }
>  
>  /**
> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>  	}
>  }
>  
> +/**
> + * \brief Cancel request and signal its completion
> + * \param[in] request The request to cancel
> + *
> + * This function cancels the request in addition to its completion. The same
> + * rules as for completeRequest() apply.
> + */
> +void PipelineHandler::cancelRequest(Request *request)
> +{
> +	request->_d()->cancel();
> +	completeRequest(request);
> +}
> +
>  /**
>   * \brief Retrieve the absolute path to a platform configuration file
>   * \param[in] subdir The pipeline handler specific subdirectory name

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

> Quoting Milan Zamazal (2024-10-09 09:35:56)
>> 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 request must be also canceled, which
>> requires introducing a little PipelineHandler::cancelRequest helper in
>> order to be able to access the private cancel() method.
>> 
>> 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@redhat.com>
>> ---
>>  include/libcamera/internal/pipeline_handler.h |  1 +
>>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>>  3 files changed, 31 insertions(+), 7 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> index 0d3808036..fb28a18d0 100644
>> --- a/include/libcamera/internal/pipeline_handler.h
>> +++ b/include/libcamera/internal/pipeline_handler.h
>> @@ -60,6 +60,7 @@ public:
>>  
>>         bool completeBuffer(Request *request, FrameBuffer *buffer);
>>         void completeRequest(Request *request);
>> +       void cancelRequest(Request *request);
>>  
>>         std::string configurationFile(const std::string &subdir,
>>                                       const std::string &name) const;
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>> +               }
>> +               conversionQueue_.pop();
>> +       }
>> +}
>> +
>>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>  {
>>         swIsp_->processStats(frame, bufferId,
>> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>         video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>  
>>         data->conversionBuffers_.clear();
>> +       data->clearIncompleteRequests();
>
> Do the requests have any existing reference to anything in teh
> conversionBuffers_ that would need to make sure we cancel/clear before
> releasing the conversionBuffers_? 

I don't think so -- conversionBuffers_ contains buffers obtained from
V4L2VideoDevice while clearIncompleteRequests examines conversionQueue_,
which contains buffers from requests.  I.e. they are input x output
buffers if I understand the things correctly.

I can swap the two lines to avoid any doubts or future mistakes.

> Probably not - but just checking.
>
>>  
>>         releasePipeline(data);
>>  }
>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> index e59404691..c9cb11f0f 100644
>> --- a/src/libcamera/pipeline_handler.cpp
>> +++ b/src/libcamera/pipeline_handler.cpp
>
> Sorry to be a pain on nitpicking - but the changes to pipeline_handler
> are core - not SoftISP and the changes below will function standalone,
> so this should be a separate patch.
>
> Then the softISP can use it!
>
> Something like "libcamera: pipeline_handler: Provide cancelRequest"
>
> is definitely something I want to be visible in the changelogs, and
> clear that other pipeline handlers can/should use it.

Yes, I'll split it.

> Once split to two - I'd probably say this already goes in the right
> direction to get merged even if there is possible still another race to
> investigate on top...

Good deal. :-)

> --
> Kieran
>
>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>>         while (!waitingRequests_.empty()) {
>>                 Request *request = waitingRequests_.front();
>>                 waitingRequests_.pop();
>> -
>> -               request->_d()->cancel();
>> -               completeRequest(request);
>> +               cancelRequest(request);
>>         }
>>  
>>         /* Make sure no requests are pending. */
>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>>         }
>>  
>>         int ret = queueRequestDevice(camera, request);
>> -       if (ret) {
>> -               request->_d()->cancel();
>> -               completeRequest(request);
>> -       }
>> +       if (ret)
>> +               cancelRequest(request);
>>  }
>>  
>>  /**
>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>>         }
>>  }
>>  
>> +/**
>> + * \brief Cancel request and signal its completion
>> + * \param[in] request The request to cancel
>> + *
>> + * This function cancels the request in addition to its completion. The same
>> + * rules as for completeRequest() apply.
>> + */
>> +void PipelineHandler::cancelRequest(Request *request)
>> +{
>> +       request->_d()->cancel();
>> +       completeRequest(request);
>> +}
>> +
>>  /**
>>   * \brief Retrieve the absolute path to a platform configuration file
>>   * \param[in] subdir The pipeline handler specific subdirectory name
>> -- 
>> 2.44.1
>>

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
>> requires introducing a little PipelineHandler::cancelRequest helper in
>> order to be able to access the private cancel() method.
>> 
>> 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@redhat.com>
>> ---
>>  include/libcamera/internal/pipeline_handler.h |  1 +
>>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>>  3 files changed, 31 insertions(+), 7 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> index 0d3808036..fb28a18d0 100644
>> --- a/include/libcamera/internal/pipeline_handler.h
>> +++ b/include/libcamera/internal/pipeline_handler.h
>> @@ -60,6 +60,7 @@ public:
>>  
>>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>>  	void completeRequest(Request *request);
>> +	void cancelRequest(Request *request);
>>  
>>  	std::string configurationFile(const std::string &subdir,
>>  				      const std::string &name) const;
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>
> Aren't you cancelling the same request multiple times ?

Possibly yes.  I'll add a check for RequestPending status.

>> +		}
>> +		conversionQueue_.pop();
>> +	}
>> +}
>> +
>>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>  {
>>  	swIsp_->processStats(frame, bufferId,
>> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>  
>>  	data->conversionBuffers_.clear();
>> +	data->clearIncompleteRequests();
>>  
>>  	releasePipeline(data);
>>  }
>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> index e59404691..c9cb11f0f 100644
>> --- a/src/libcamera/pipeline_handler.cpp
>> +++ b/src/libcamera/pipeline_handler.cpp
>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>>  	while (!waitingRequests_.empty()) {
>>  		Request *request = waitingRequests_.front();
>>  		waitingRequests_.pop();
>> -
>> -		request->_d()->cancel();
>> -		completeRequest(request);
>> +		cancelRequest(request);
>>  	}
>>  
>>  	/* Make sure no requests are pending. */
>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>>  	}
>>  
>>  	int ret = queueRequestDevice(camera, request);
>> -	if (ret) {
>> -		request->_d()->cancel();
>> -		completeRequest(request);
>> -	}
>> +	if (ret)
>> +		cancelRequest(request);
>>  }
>>  
>>  /**
>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>>  	}
>>  }
>>  
>> +/**
>> + * \brief Cancel request and signal its completion
>> + * \param[in] request The request to cancel
>> + *
>> + * This function cancels the request in addition to its completion. The same
>> + * rules as for completeRequest() apply.
>> + */
>> +void PipelineHandler::cancelRequest(Request *request)
>> +{
>> +	request->_d()->cancel();
>> +	completeRequest(request);
>> +}
>> +
>>  /**
>>   * \brief Retrieve the absolute path to a platform configuration file
>>   * \param[in] subdir The pipeline handler specific subdirectory name

On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
> Laurent Pinchart writes:
> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
> >> requires introducing a little PipelineHandler::cancelRequest helper in
> >> order to be able to access the private cancel() method.
> >> 
> >> 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@redhat.com>
> >> ---
> >>  include/libcamera/internal/pipeline_handler.h |  1 +
> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
> >>  3 files changed, 31 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> >> index 0d3808036..fb28a18d0 100644
> >> --- a/include/libcamera/internal/pipeline_handler.h
> >> +++ b/include/libcamera/internal/pipeline_handler.h
> >> @@ -60,6 +60,7 @@ public:
> >>  
> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
> >>  	void completeRequest(Request *request);
> >> +	void cancelRequest(Request *request);
> >>  
> >>  	std::string configurationFile(const std::string &subdir,
> >>  				      const std::string &name) const;
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
> >
> > Aren't you cancelling the same request multiple times ?
> 
> Possibly yes.  I'll add a check for RequestPending status.

How about modifying conversionQueue_ to store instances of a structure
that contain a Request pointer in addition to the std::map ?

> >> +		}
> >> +		conversionQueue_.pop();
> >> +	}
> >> +}
> >> +
> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> >>  {
> >>  	swIsp_->processStats(frame, bufferId,
> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
> >>  
> >>  	data->conversionBuffers_.clear();
> >> +	data->clearIncompleteRequests();
> >>  
> >>  	releasePipeline(data);
> >>  }
> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> index e59404691..c9cb11f0f 100644
> >> --- a/src/libcamera/pipeline_handler.cpp
> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
> >>  	while (!waitingRequests_.empty()) {
> >>  		Request *request = waitingRequests_.front();
> >>  		waitingRequests_.pop();
> >> -
> >> -		request->_d()->cancel();
> >> -		completeRequest(request);
> >> +		cancelRequest(request);
> >>  	}
> >>  
> >>  	/* Make sure no requests are pending. */
> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
> >>  	}
> >>  
> >>  	int ret = queueRequestDevice(camera, request);
> >> -	if (ret) {
> >> -		request->_d()->cancel();
> >> -		completeRequest(request);
> >> -	}
> >> +	if (ret)
> >> +		cancelRequest(request);
> >>  }
> >>  
> >>  /**
> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
> >>  	}
> >>  }
> >>  
> >> +/**
> >> + * \brief Cancel request and signal its completion
> >> + * \param[in] request The request to cancel
> >> + *
> >> + * This function cancels the request in addition to its completion. The same
> >> + * rules as for completeRequest() apply.
> >> + */
> >> +void PipelineHandler::cancelRequest(Request *request)
> >> +{
> >> +	request->_d()->cancel();
> >> +	completeRequest(request);
> >> +}
> >> +
> >>  /**
> >>   * \brief Retrieve the absolute path to a platform configuration file
> >>   * \param[in] subdir The pipeline handler specific subdirectory name
>

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart writes:
>> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
>> >> requires introducing a little PipelineHandler::cancelRequest helper in
>> >> order to be able to access the private cancel() method.
>> >> 
>> >> 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@redhat.com>
>> >> ---
>> >>  include/libcamera/internal/pipeline_handler.h |  1 +
>> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>> >>  3 files changed, 31 insertions(+), 7 deletions(-)
>> >> 
>> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> >> index 0d3808036..fb28a18d0 100644
>> >> --- a/include/libcamera/internal/pipeline_handler.h
>> >> +++ b/include/libcamera/internal/pipeline_handler.h
>> >> @@ -60,6 +60,7 @@ public:
>> >>  
>> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>> >>  	void completeRequest(Request *request);
>> >> +	void cancelRequest(Request *request);
>> >>  
>> >>  	std::string configurationFile(const std::string &subdir,
>> >>  				      const std::string &name) const;
>> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>> >
>> > Aren't you cancelling the same request multiple times ?
>> 
>> Possibly yes.  I'll add a check for RequestPending status.
>
> How about modifying conversionQueue_ to store instances of a structure
> that contain a Request pointer in addition to the std::map ?

I prefer keeping data structures simple.  What would be the advantage
worth of maintaining the extra pointer?  I'd be inclined to have it if
it served more purposes, but is it worth just for the cleanup?

>> >> +		}
>> >> +		conversionQueue_.pop();
>> >> +	}
>> >> +}
>> >> +
>> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>> >>  {
>> >>  	swIsp_->processStats(frame, bufferId,
>> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>> >>  
>> >>  	data->conversionBuffers_.clear();
>> >> +	data->clearIncompleteRequests();
>> >>  
>> >>  	releasePipeline(data);
>> >>  }
>> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> >> index e59404691..c9cb11f0f 100644
>> >> --- a/src/libcamera/pipeline_handler.cpp
>> >> +++ b/src/libcamera/pipeline_handler.cpp
>> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>> >>  	while (!waitingRequests_.empty()) {
>> >>  		Request *request = waitingRequests_.front();
>> >>  		waitingRequests_.pop();
>> >> -
>> >> -		request->_d()->cancel();
>> >> -		completeRequest(request);
>> >> +		cancelRequest(request);
>> >>  	}
>> >>  
>> >>  	/* Make sure no requests are pending. */
>> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>> >>  	}
>> >>  
>> >>  	int ret = queueRequestDevice(camera, request);
>> >> -	if (ret) {
>> >> -		request->_d()->cancel();
>> >> -		completeRequest(request);
>> >> -	}
>> >> +	if (ret)
>> >> +		cancelRequest(request);
>> >>  }
>> >>  
>> >>  /**
>> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>> >>  	}
>> >>  }
>> >>  
>> >> +/**
>> >> + * \brief Cancel request and signal its completion
>> >> + * \param[in] request The request to cancel
>> >> + *
>> >> + * This function cancels the request in addition to its completion. The same
>> >> + * rules as for completeRequest() apply.
>> >> + */
>> >> +void PipelineHandler::cancelRequest(Request *request)
>> >> +{
>> >> +	request->_d()->cancel();
>> >> +	completeRequest(request);
>> >> +}
>> >> +
>> >>  /**
>> >>   * \brief Retrieve the absolute path to a platform configuration file
>> >>   * \param[in] subdir The pipeline handler specific subdirectory name
>>

Hi


2024. október 9., szerda 13:17 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Robert Mader (2024-10-09 10:41:00)
> > Hi, thanks for the patch!
> >
> > On 09.10.24 10:39, Milan Zamazal wrote:
> > > Milan Zamazal <mzamazal@redhat.com> writes:
> > >
> > >> 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 request must be also canceled, which
> > >> requires introducing a little PipelineHandler::cancelRequest helper in
> > >> order to be able to access the private cancel() method.
> > >>
> > >> 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@redhat.com>
> > > Changes in v2:
> > > - The request is additionally canceled.
> > > - New helper method PipelineHandler::cancelRequest() introduced.
> > >
> > > Robert, could you please test v2?
> >
> > I gave it a quick go for 5 minutes - generally it seems to work great,
> > however trying enough quick camera switching I still run into the assert.
> >
> > So probably, on top of this, we'll need something implementing what
> > Kieran suggested:
> >
> >  > (Though I think we should reject any incoming requests as soon as we
> > hit stop())
> 
> Sorry - there I was saying that should /already/ be happening. The state
> machine should put the camera in to a stopping state where new incoming
> requests will be rejected...
> 
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1401
> 
> Unless this is still somehow racy ;-(

It seems to me that one thread calling Camera::stop() and another thread calling
Camera::queueRequest() can cause this issue. Specifically, if the thread executing
Camera::stop() is stopped e.g. after `LOG(Camera, Debug) << "Stopping capture";`,
then the thread running Camera::queueRequest() can progress and queue the request
without anything stopping it. Afterwards, if the thread running Camera::stop() continues
such that the request hasn't been completed, then the assertion will be hit
unless PipelineHandler::stopDevice() does something with the requests. At least
that what I can see, it is possible that I have overlooked something.

Slightly unrelated, but I think the state transitions of the Camera should be done
with atomic compare exchange operations, otherwise multiple threads could successfully
call e.g. Camera::stop() concurrently.


Regards,
Barnabás Pőcze

> 
> --
> Kieran
> 
> 
> >
> > >> ---
> > >>   include/libcamera/internal/pipeline_handler.h |  1 +
> > >>   src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
> > >>   src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
> > >>   3 files changed, 31 insertions(+), 7 deletions(-)
> > >>
> > >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > >> index 0d3808036..fb28a18d0 100644
> > >> --- a/include/libcamera/internal/pipeline_handler.h
> > >> +++ b/include/libcamera/internal/pipeline_handler.h
> > >> @@ -60,6 +60,7 @@ public:
> > >>
> > >>      bool completeBuffer(Request *request, FrameBuffer *buffer);
> > >>      void completeRequest(Request *request);
> > >> +    void cancelRequest(Request *request);
> > >>
> > >>      std::string configurationFile(const std::string &subdir,
> > >>                                    const std::string &name) const;
> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
> > >> +            }
> > >> +            conversionQueue_.pop();
> > >> +    }
> > >> +}
> > >> +
> > >>   void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> > >>   {
> > >>      swIsp_->processStats(frame, bufferId,
> > >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> > >>      video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
> > >>
> > >>      data->conversionBuffers_.clear();
> > >> +    data->clearIncompleteRequests();
> > >>
> > >>      releasePipeline(data);
> > >>   }
> > >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > >> index e59404691..c9cb11f0f 100644
> > >> --- a/src/libcamera/pipeline_handler.cpp
> > >> +++ b/src/libcamera/pipeline_handler.cpp
> > >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
> > >>      while (!waitingRequests_.empty()) {
> > >>              Request *request = waitingRequests_.front();
> > >>              waitingRequests_.pop();
> > >> -
> > >> -            request->_d()->cancel();
> > >> -            completeRequest(request);
> > >> +            cancelRequest(request);
> > >>      }
> > >>
> > >>      /* Make sure no requests are pending. */
> > >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
> > >>      }
> > >>
> > >>      int ret = queueRequestDevice(camera, request);
> > >> -    if (ret) {
> > >> -            request->_d()->cancel();
> > >> -            completeRequest(request);
> > >> -    }
> > >> +    if (ret)
> > >> +            cancelRequest(request);
> > >>   }
> > >>
> > >>   /**
> > >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
> > >>      }
> > >>   }
> > >>
> > >> +/**
> > >> + * \brief Cancel request and signal its completion
> > >> + * \param[in] request The request to cancel
> >
> > Small typo here
> >
> >
> > >> + *
> > >> + * This function cancels the request in addition to its completion. The same
> > >> + * rules as for completeRequest() apply.
> > >> + */
> > >> +void PipelineHandler::cancelRequest(Request *request)
> > >> +{
> > >> +    request->_d()->cancel();
> > >> +    completeRequest(request);
> > >> +}
> > >> +
> > >>   /**
> > >>    * \brief Retrieve the absolute path to a platform configuration file
> > >>    * \param[in] subdir The pipeline handler specific subdirectory name
> >
> > --
> > Robert Mader
> > Consultant Software Developer
> >
> > Collabora Ltd.
> > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> > Registered in England & Wales, no. 5513718
> >

On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:
> Laurent Pinchart writes:
> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
> >> Laurent Pinchart writes:
> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
> >> >> requires introducing a little PipelineHandler::cancelRequest helper in
> >> >> order to be able to access the private cancel() method.
> >> >> 
> >> >> 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@redhat.com>
> >> >> ---
> >> >>  include/libcamera/internal/pipeline_handler.h |  1 +
> >> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
> >> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
> >> >>  3 files changed, 31 insertions(+), 7 deletions(-)
> >> >> 
> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> >> >> index 0d3808036..fb28a18d0 100644
> >> >> --- a/include/libcamera/internal/pipeline_handler.h
> >> >> +++ b/include/libcamera/internal/pipeline_handler.h
> >> >> @@ -60,6 +60,7 @@ public:
> >> >>  
> >> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
> >> >>  	void completeRequest(Request *request);
> >> >> +	void cancelRequest(Request *request);
> >> >>  
> >> >>  	std::string configurationFile(const std::string &subdir,
> >> >>  				      const std::string &name) const;
> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
> >> >
> >> > Aren't you cancelling the same request multiple times ?
> >> 
> >> Possibly yes.  I'll add a check for RequestPending status.
> >
> > How about modifying conversionQueue_ to store instances of a structure
> > that contain a Request pointer in addition to the std::map ?
> 
> I prefer keeping data structures simple.  What would be the advantage
> worth of maintaining the extra pointer?  I'd be inclined to have it if
> it served more purposes, but is it worth just for the cleanup?

As far as I understand, the conversionQueue_ contains a map of streams
to output buffers for one request. If you look at
SimpleCameraData::bufferReady(), you'll see, in the
!FrameMetadata::FrameSuccess path at the top of the function,

		Request *request = nullptr;
		for (auto &item : conversionQueue_.front()) {
			FrameBuffer *outputBuffer = item.second;
			request = outputBuffer->request();
			pipe->completeBuffer(request, outputBuffer);
		}
		conversionQueue_.pop();

		if (request)
			pipe->completeRequest(request);

All buffers need to be completed individually, but the request needs to
be completed once only.

All the output buffers in the map relate to the same request, so I think
it makes sense to store the request pointer separately in the queue for
easy access. Alternatively, you could have a code construct similar to
the above, getting the request pointer from the buffer, and calling
completeRequest() once only. It would be nice if we could share more
code, replacing the above construct with something shared by the cancel
path.

> >> >> +		}
> >> >> +		conversionQueue_.pop();
> >> >> +	}
> >> >> +}
> >> >> +
> >> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> >> >>  {
> >> >>  	swIsp_->processStats(frame, bufferId,
> >> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> >> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
> >> >>  
> >> >>  	data->conversionBuffers_.clear();
> >> >> +	data->clearIncompleteRequests();
> >> >>  
> >> >>  	releasePipeline(data);
> >> >>  }
> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> >> index e59404691..c9cb11f0f 100644
> >> >> --- a/src/libcamera/pipeline_handler.cpp
> >> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
> >> >>  	while (!waitingRequests_.empty()) {
> >> >>  		Request *request = waitingRequests_.front();
> >> >>  		waitingRequests_.pop();
> >> >> -
> >> >> -		request->_d()->cancel();
> >> >> -		completeRequest(request);
> >> >> +		cancelRequest(request);
> >> >>  	}
> >> >>  
> >> >>  	/* Make sure no requests are pending. */
> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
> >> >>  	}
> >> >>  
> >> >>  	int ret = queueRequestDevice(camera, request);
> >> >> -	if (ret) {
> >> >> -		request->_d()->cancel();
> >> >> -		completeRequest(request);
> >> >> -	}
> >> >> +	if (ret)
> >> >> +		cancelRequest(request);
> >> >>  }
> >> >>  
> >> >>  /**
> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
> >> >>  	}
> >> >>  }
> >> >>  
> >> >> +/**
> >> >> + * \brief Cancel request and signal its completion
> >> >> + * \param[in] request The request to cancel
> >> >> + *
> >> >> + * This function cancels the request in addition to its completion. The same
> >> >> + * rules as for completeRequest() apply.
> >> >> + */
> >> >> +void PipelineHandler::cancelRequest(Request *request)
> >> >> +{
> >> >> +	request->_d()->cancel();
> >> >> +	completeRequest(request);
> >> >> +}
> >> >> +
> >> >>  /**
> >> >>   * \brief Retrieve the absolute path to a platform configuration file
> >> >>   * \param[in] subdir The pipeline handler specific subdirectory name

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart writes:
>> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
>
>> >> Laurent Pinchart writes:
>> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
>> >> >> requires introducing a little PipelineHandler::cancelRequest helper in
>> >> >> order to be able to access the private cancel() method.
>> >> >> 
>> >> >> 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@redhat.com>
>> >> >> ---
>> >> >>  include/libcamera/internal/pipeline_handler.h |  1 +
>> >> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>> >> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>> >> >>  3 files changed, 31 insertions(+), 7 deletions(-)
>> >> >> 
>> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> >> >> index 0d3808036..fb28a18d0 100644
>> >> >> --- a/include/libcamera/internal/pipeline_handler.h
>> >> >> +++ b/include/libcamera/internal/pipeline_handler.h
>> >> >> @@ -60,6 +60,7 @@ public:
>> >> >>  
>> >> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>> >> >>  	void completeRequest(Request *request);
>> >> >> +	void cancelRequest(Request *request);
>> >> >>  
>> >> >>  	std::string configurationFile(const std::string &subdir,
>> >> >>  				      const std::string &name) const;
>> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>> >> >
>> >> > Aren't you cancelling the same request multiple times ?
>> >> 
>> >> Possibly yes.  I'll add a check for RequestPending status.
>> >
>> > How about modifying conversionQueue_ to store instances of a structure
>> > that contain a Request pointer in addition to the std::map ?
>> 
>> I prefer keeping data structures simple.  What would be the advantage
>> worth of maintaining the extra pointer?  I'd be inclined to have it if
>> it served more purposes, but is it worth just for the cleanup?
>
> As far as I understand, the conversionQueue_ contains a map of streams
> to output buffers for one request. If you look at
> SimpleCameraData::bufferReady(), you'll see, in the
> !FrameMetadata::FrameSuccess path at the top of the function,
>
> 		Request *request = nullptr;
> 		for (auto &item : conversionQueue_.front()) {
> 			FrameBuffer *outputBuffer = item.second;
> 			request = outputBuffer->request();
> 			pipe->completeBuffer(request, outputBuffer);
> 		}
> 		conversionQueue_.pop();
>
> 		if (request)
> 			pipe->completeRequest(request);
>
> All buffers need to be completed individually, but the request needs to
> be completed once only.

But it is completed only if:

- There is at least one output buffer
- AND the buffer refers to the given request.

Those look like reasonable assumptions but I'm not sure there is nothing
subtle behind.  You authored the code structure above when adding
support for multiple streams so can you confirm that something like

  std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_;

  ...

  Request *request = conversionQueue_.front().first;
  for (auto &item : conversionQueue_.front().second)
  	pipe->completeBuffer(request, item.second);
  pipe->completeRequest(request);
  conversionQueue_.pop();

would be equivalent under the right assumptions?

> All the output buffers in the map relate to the same request, so I think
> it makes sense to store the request pointer separately in the queue for
> easy access. Alternatively, you could have a code construct similar to
> the above, getting the request pointer from the buffer, and calling
> completeRequest() once only. It would be nice if we could share more
> code, replacing the above construct with something shared by the cancel
> path.
>
>> >> >> +		}
>> >> >> +		conversionQueue_.pop();
>> >> >> +	}
>> >> >> +}
>> >> >> +
>> >> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>> >> >>  {
>> >> >>  	swIsp_->processStats(frame, bufferId,
>> >> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>> >> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>> >> >>  
>> >> >>  	data->conversionBuffers_.clear();
>> >> >> +	data->clearIncompleteRequests();
>> >> >>  
>> >> >>  	releasePipeline(data);
>> >> >>  }
>> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> >> >> index e59404691..c9cb11f0f 100644
>> >> >> --- a/src/libcamera/pipeline_handler.cpp
>> >> >> +++ b/src/libcamera/pipeline_handler.cpp
>> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>> >> >>  	while (!waitingRequests_.empty()) {
>> >> >>  		Request *request = waitingRequests_.front();
>> >> >>  		waitingRequests_.pop();
>> >> >> -
>> >> >> -		request->_d()->cancel();
>> >> >> -		completeRequest(request);
>> >> >> +		cancelRequest(request);
>> >> >>  	}
>> >> >>  
>> >> >>  	/* Make sure no requests are pending. */
>> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>> >> >>  	}
>> >> >>  
>> >> >>  	int ret = queueRequestDevice(camera, request);
>> >> >> -	if (ret) {
>> >> >> -		request->_d()->cancel();
>> >> >> -		completeRequest(request);
>> >> >> -	}
>> >> >> +	if (ret)
>> >> >> +		cancelRequest(request);
>> >> >>  }
>> >> >>  
>> >> >>  /**
>> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>> >> >>  	}
>> >> >>  }
>> >> >>  
>> >> >> +/**
>> >> >> + * \brief Cancel request and signal its completion
>> >> >> + * \param[in] request The request to cancel
>> >> >> + *
>> >> >> + * This function cancels the request in addition to its completion. The same
>> >> >> + * rules as for completeRequest() apply.
>> >> >> + */
>> >> >> +void PipelineHandler::cancelRequest(Request *request)
>> >> >> +{
>> >> >> +	request->_d()->cancel();
>> >> >> +	completeRequest(request);
>> >> >> +}
>> >> >> +
>> >> >>  /**
>> >> >>   * \brief Retrieve the absolute path to a platform configuration file
>> >> >>   * \param[in] subdir The pipeline handler specific subdirectory name

Milan Zamazal <mzamazal@redhat.com> writes:

> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:
>>> Laurent Pinchart writes:
>>> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
>>
>>> >> Laurent Pinchart writes:
>>> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
>>> >> >> requires introducing a little PipelineHandler::cancelRequest helper in
>>> >> >> order to be able to access the private cancel() method.
>>> >> >> 
>>> >> >> 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@redhat.com>
>>> >> >> ---
>>> >> >>  include/libcamera/internal/pipeline_handler.h |  1 +
>>> >> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>>> >> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>>> >> >>  3 files changed, 31 insertions(+), 7 deletions(-)
>>> >> >> 
>>> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>> >> >> index 0d3808036..fb28a18d0 100644
>>> >> >> --- a/include/libcamera/internal/pipeline_handler.h
>>> >> >> +++ b/include/libcamera/internal/pipeline_handler.h
>>> >> >> @@ -60,6 +60,7 @@ public:
>>> >> >>  
>>> >> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>>> >> >>  	void completeRequest(Request *request);
>>> >> >> +	void cancelRequest(Request *request);
>>> >> >>  
>>> >> >>  	std::string configurationFile(const std::string &subdir,
>>> >> >>  				      const std::string &name) const;
>>> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>>> >> >
>>> >> > Aren't you cancelling the same request multiple times ?
>>> >> 
>>> >> Possibly yes.  I'll add a check for RequestPending status.
>>> >
>>> > How about modifying conversionQueue_ to store instances of a structure
>>> > that contain a Request pointer in addition to the std::map ?
>>> 
>>> I prefer keeping data structures simple.  What would be the advantage
>>> worth of maintaining the extra pointer?  I'd be inclined to have it if
>>> it served more purposes, but is it worth just for the cleanup?
>>
>> As far as I understand, the conversionQueue_ contains a map of streams
>> to output buffers for one request. If you look at
>> SimpleCameraData::bufferReady(), you'll see, in the
>> !FrameMetadata::FrameSuccess path at the top of the function,
>>
>> 		Request *request = nullptr;
>> 		for (auto &item : conversionQueue_.front()) {
>> 			FrameBuffer *outputBuffer = item.second;
>> 			request = outputBuffer->request();
>> 			pipe->completeBuffer(request, outputBuffer);
>> 		}
>> 		conversionQueue_.pop();
>>
>> 		if (request)
>> 			pipe->completeRequest(request);
>>
>> All buffers need to be completed individually, but the request needs to
>> be completed once only.
>
> But it is completed only if:
>
> - There is at least one output buffer
> - AND the buffer refers to the given request.
>
> Those look like reasonable assumptions but I'm not sure there is nothing
> subtle behind.  You authored the code structure above when adding
> support for multiple streams so can you confirm that something like
>
>   std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_;
>
>   ...
>
>   Request *request = conversionQueue_.front().first;
>   for (auto &item : conversionQueue_.front().second)
>   	pipe->completeBuffer(request, item.second);
>   pipe->completeRequest(request);
>   conversionQueue_.pop();
>
> would be equivalent under the right assumptions?

I take the silence as yes, so posted v4 with this change.

>> All the output buffers in the map relate to the same request, so I think
>> it makes sense to store the request pointer separately in the queue for
>> easy access. Alternatively, you could have a code construct similar to
>> the above, getting the request pointer from the buffer, and calling
>> completeRequest() once only. It would be nice if we could share more
>> code, replacing the above construct with something shared by the cancel
>> path.
>>
>>> >> >> +		}
>>> >> >> +		conversionQueue_.pop();
>>> >> >> +	}
>>> >> >> +}
>>> >> >> +
>>> >> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>> >> >>  {
>>> >> >>  	swIsp_->processStats(frame, bufferId,
>>> >> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>> >> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>> >> >>  
>>> >> >>  	data->conversionBuffers_.clear();
>>> >> >> +	data->clearIncompleteRequests();
>>> >> >>  
>>> >> >>  	releasePipeline(data);
>>> >> >>  }
>>> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>> >> >> index e59404691..c9cb11f0f 100644
>>> >> >> --- a/src/libcamera/pipeline_handler.cpp
>>> >> >> +++ b/src/libcamera/pipeline_handler.cpp
>>> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>>> >> >>  	while (!waitingRequests_.empty()) {
>>> >> >>  		Request *request = waitingRequests_.front();
>>> >> >>  		waitingRequests_.pop();
>>> >> >> -
>>> >> >> -		request->_d()->cancel();
>>> >> >> -		completeRequest(request);
>>> >> >> +		cancelRequest(request);
>>> >> >>  	}
>>> >> >>  
>>> >> >>  	/* Make sure no requests are pending. */
>>> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>>> >> >>  	}
>>> >> >>  
>>> >> >>  	int ret = queueRequestDevice(camera, request);
>>> >> >> -	if (ret) {
>>> >> >> -		request->_d()->cancel();
>>> >> >> -		completeRequest(request);
>>> >> >> -	}
>>> >> >> +	if (ret)
>>> >> >> +		cancelRequest(request);
>>> >> >>  }
>>> >> >>  
>>> >> >>  /**
>>> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>>> >> >>  	}
>>> >> >>  }
>>> >> >>  
>>> >> >> +/**
>>> >> >> + * \brief Cancel request and signal its completion
>>> >> >> + * \param[in] request The request to cancel
>>> >> >> + *
>>> >> >> + * This function cancels the request in addition to its completion. The same
>>> >> >> + * rules as for completeRequest() apply.
>>> >> >> + */
>>> >> >> +void PipelineHandler::cancelRequest(Request *request)
>>> >> >> +{
>>> >> >> +	request->_d()->cancel();
>>> >> >> +	completeRequest(request);
>>> >> >> +}
>>> >> >> +
>>> >> >>  /**
>>> >> >>   * \brief Retrieve the absolute path to a platform configuration file
>>> >> >>   * \param[in] subdir The pipeline handler specific subdirectory name

Hi Milan,

Discussing this fix today made me realized I forgot to reply to this
e-mail. Sorry about that.

On Fri, Oct 11, 2024 at 04:09:30PM +0200, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:
> >> Laurent Pinchart writes:
> >> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
> >> >> Laurent Pinchart writes:
> >> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
> >> >> >> requires introducing a little PipelineHandler::cancelRequest helper in
> >> >> >> order to be able to access the private cancel() method.
> >> >> >> 
> >> >> >> 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@redhat.com>
> >> >> >> ---
> >> >> >>  include/libcamera/internal/pipeline_handler.h |  1 +
> >> >> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
> >> >> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
> >> >> >>  3 files changed, 31 insertions(+), 7 deletions(-)
> >> >> >> 
> >> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> >> >> >> index 0d3808036..fb28a18d0 100644
> >> >> >> --- a/include/libcamera/internal/pipeline_handler.h
> >> >> >> +++ b/include/libcamera/internal/pipeline_handler.h
> >> >> >> @@ -60,6 +60,7 @@ public:
> >> >> >>  
> >> >> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
> >> >> >>  	void completeRequest(Request *request);
> >> >> >> +	void cancelRequest(Request *request);
> >> >> >>  
> >> >> >>  	std::string configurationFile(const std::string &subdir,
> >> >> >>  				      const std::string &name) const;
> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
> >> >> >
> >> >> > Aren't you cancelling the same request multiple times ?
> >> >> 
> >> >> Possibly yes.  I'll add a check for RequestPending status.
> >> >
> >> > How about modifying conversionQueue_ to store instances of a structure
> >> > that contain a Request pointer in addition to the std::map ?
> >> 
> >> I prefer keeping data structures simple.  What would be the advantage
> >> worth of maintaining the extra pointer?  I'd be inclined to have it if
> >> it served more purposes, but is it worth just for the cleanup?
> >
> > As far as I understand, the conversionQueue_ contains a map of streams
> > to output buffers for one request. If you look at
> > SimpleCameraData::bufferReady(), you'll see, in the
> > !FrameMetadata::FrameSuccess path at the top of the function,
> >
> > 		Request *request = nullptr;
> > 		for (auto &item : conversionQueue_.front()) {
> > 			FrameBuffer *outputBuffer = item.second;
> > 			request = outputBuffer->request();
> > 			pipe->completeBuffer(request, outputBuffer);
> > 		}
> > 		conversionQueue_.pop();
> >
> > 		if (request)
> > 			pipe->completeRequest(request);
> >
> > All buffers need to be completed individually, but the request needs to
> > be completed once only.
> 
> But it is completed only if:
> 
> - There is at least one output buffer
> - AND the buffer refers to the given request.
> 
> Those look like reasonable assumptions but I'm not sure there is nothing
> subtle behind.  You authored the code structure above when adding
> support for multiple streams so can you confirm that something like
> 
>   std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_;
> 
>   ...
> 
>   Request *request = conversionQueue_.front().first;
>   for (auto &item : conversionQueue_.front().second)
>   	pipe->completeBuffer(request, item.second);
>   pipe->completeRequest(request);
>   conversionQueue_.pop();
> 
> would be equivalent under the right assumptions?

My assumption (please tell me if I'm wrong) is that an entry in the
conversion queue corresponds to one request, and holds a map of streams
to frame buffers for that particular request. We have code that needs to
deal with those buffers, and also code that needs to deal with the
request. Currently, the request is retrieved from the buffer. In the
case of SimpleCameraData::bufferReady(), we use the request from the
last buffer in the map, but any buffer would do, as they all belong to
the same request.

I find this code confusing, and I believe we should explicitly store the
request pointer in the conversion queue entry, and retrieve it from
there, instead of retrieving it from the buffer. It would make the code
clearer. Your code snippet above looks fine, althouh I would probably
create a new structure to hold the request pointer and map:

	struct ConversionJob {
		Request *request;
		std::map<const Stream *, FrameBuffer *> buffers;
	};

	std::queue<ConversionJob> conversionQueue_;

(we can bikeshed the ConversionJob name). The code could then look like

 	Request *request = nullptr;
	const ConversionJob &job = conversionQueue_.front();

 	for (auto &[stream, outputBuffer] : job)
 		pipe->completeBuffer(request, outputBuffer);

	pipe->completeRequest(job.request);
 	conversionQueue_.pop();

which I think is much more readable.

> > All the output buffers in the map relate to the same request, so I think
> > it makes sense to store the request pointer separately in the queue for
> > easy access. Alternatively, you could have a code construct similar to
> > the above, getting the request pointer from the buffer, and calling
> > completeRequest() once only. It would be nice if we could share more
> > code, replacing the above construct with something shared by the cancel
> > path.
> >
> >> >> >> +		}
> >> >> >> +		conversionQueue_.pop();
> >> >> >> +	}
> >> >> >> +}
> >> >> >> +
> >> >> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> >> >> >>  {
> >> >> >>  	swIsp_->processStats(frame, bufferId,
> >> >> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> >> >> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
> >> >> >>  
> >> >> >>  	data->conversionBuffers_.clear();
> >> >> >> +	data->clearIncompleteRequests();
> >> >> >>  
> >> >> >>  	releasePipeline(data);
> >> >> >>  }
> >> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> >> >> index e59404691..c9cb11f0f 100644
> >> >> >> --- a/src/libcamera/pipeline_handler.cpp
> >> >> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
> >> >> >>  	while (!waitingRequests_.empty()) {
> >> >> >>  		Request *request = waitingRequests_.front();
> >> >> >>  		waitingRequests_.pop();
> >> >> >> -
> >> >> >> -		request->_d()->cancel();
> >> >> >> -		completeRequest(request);
> >> >> >> +		cancelRequest(request);
> >> >> >>  	}
> >> >> >>  
> >> >> >>  	/* Make sure no requests are pending. */
> >> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
> >> >> >>  	}
> >> >> >>  
> >> >> >>  	int ret = queueRequestDevice(camera, request);
> >> >> >> -	if (ret) {
> >> >> >> -		request->_d()->cancel();
> >> >> >> -		completeRequest(request);
> >> >> >> -	}
> >> >> >> +	if (ret)
> >> >> >> +		cancelRequest(request);
> >> >> >>  }
> >> >> >>  
> >> >> >>  /**
> >> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
> >> >> >>  	}
> >> >> >>  }
> >> >> >>  
> >> >> >> +/**
> >> >> >> + * \brief Cancel request and signal its completion
> >> >> >> + * \param[in] request The request to cancel
> >> >> >> + *
> >> >> >> + * This function cancels the request in addition to its completion. The same
> >> >> >> + * rules as for completeRequest() apply.
> >> >> >> + */
> >> >> >> +void PipelineHandler::cancelRequest(Request *request)
> >> >> >> +{
> >> >> >> +	request->_d()->cancel();
> >> >> >> +	completeRequest(request);
> >> >> >> +}
> >> >> >> +
> >> >> >>  /**
> >> >> >>   * \brief Retrieve the absolute path to a platform configuration file
> >> >> >>   * \param[in] subdir The pipeline handler specific subdirectory name

Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Milan,
>
> Discussing this fix today made me realized I forgot to reply to this
> e-mail. Sorry about that.
>
> On Fri, Oct 11, 2024 at 04:09:30PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>> > On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:
>> >> Laurent Pinchart writes:
>> >> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
>> >> >> Laurent Pinchart writes:
>> >> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, 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 request must be also canceled, which
>> >> >> >> requires introducing a little PipelineHandler::cancelRequest helper in
>> >> >> >> order to be able to access the private cancel() method.
>> >> >> >> 
>> >> >> >> 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@redhat.com>
>> >> >> >> ---
>> >> >> >>  include/libcamera/internal/pipeline_handler.h |  1 +
>> >> >> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>> >> >> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>> >> >> >>  3 files changed, 31 insertions(+), 7 deletions(-)
>> >> >> >> 
>> >> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> >> >> >> index 0d3808036..fb28a18d0 100644
>> >> >> >> --- a/include/libcamera/internal/pipeline_handler.h
>> >> >> >> +++ b/include/libcamera/internal/pipeline_handler.h
>> >> >> >> @@ -60,6 +60,7 @@ public:
>> >> >> >>  
>> >> >> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>> >> >> >>  	void completeRequest(Request *request);
>> >> >> >> +	void cancelRequest(Request *request);
>> >> >> >>  
>> >> >> >>  	std::string configurationFile(const std::string &subdir,
>> >> >> >>  				      const std::string &name) const;
>> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> >> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);
>> >> >> >
>> >> >> > Aren't you cancelling the same request multiple times ?
>> >> >> 
>> >> >> Possibly yes.  I'll add a check for RequestPending status.
>> >> >
>> >> > How about modifying conversionQueue_ to store instances of a structure
>> >> > that contain a Request pointer in addition to the std::map ?
>> >> 
>> >> I prefer keeping data structures simple.  What would be the advantage
>> >> worth of maintaining the extra pointer?  I'd be inclined to have it if
>> >> it served more purposes, but is it worth just for the cleanup?
>> >
>> > As far as I understand, the conversionQueue_ contains a map of streams
>> > to output buffers for one request. If you look at
>> > SimpleCameraData::bufferReady(), you'll see, in the
>> > !FrameMetadata::FrameSuccess path at the top of the function,
>> >
>> > 		Request *request = nullptr;
>> > 		for (auto &item : conversionQueue_.front()) {
>> > 			FrameBuffer *outputBuffer = item.second;
>> > 			request = outputBuffer->request();
>> > 			pipe->completeBuffer(request, outputBuffer);
>> > 		}
>> > 		conversionQueue_.pop();
>> >
>> > 		if (request)
>> > 			pipe->completeRequest(request);
>> >
>> > All buffers need to be completed individually, but the request needs to
>> > be completed once only.
>> 
>> But it is completed only if:
>> 
>> - There is at least one output buffer
>> - AND the buffer refers to the given request.
>> 
>> Those look like reasonable assumptions but I'm not sure there is nothing
>> subtle behind.  You authored the code structure above when adding
>> support for multiple streams so can you confirm that something like
>> 
>>   std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_;
>> 
>>   ...
>> 
>>   Request *request = conversionQueue_.front().first;
>>   for (auto &item : conversionQueue_.front().second)
>>   	pipe->completeBuffer(request, item.second);
>>   pipe->completeRequest(request);
>>   conversionQueue_.pop();
>> 
>> would be equivalent under the right assumptions?
>
> My assumption (please tell me if I'm wrong) is that an entry in the
> conversion queue corresponds to one request, and holds a map of streams
> to frame buffers for that particular request. 

OK.

> We have code that needs to deal with those buffers, and also code that
> needs to deal with the request. Currently, the request is retrieved
> from the buffer. In the case of SimpleCameraData::bufferReady(), we
> use the request from the last buffer in the map, but any buffer would
> do, as they all belong to the same request.
>
> I find this code confusing, and I believe we should explicitly store the
> request pointer in the conversion queue entry, and retrieve it from
> there, instead of retrieving it from the buffer. It would make the code
> clearer. Your code snippet above looks fine, althouh I would probably
> create a new structure to hold the request pointer and map:
>
> 	struct ConversionJob {
> 		Request *request;
> 		std::map<const Stream *, FrameBuffer *> buffers;
> 	};
>
> 	std::queue<ConversionJob> conversionQueue_;
>
> (we can bikeshed the ConversionJob name). The code could then look like
>
>  	Request *request = nullptr;
> 	const ConversionJob &job = conversionQueue_.front();
>
>  	for (auto &[stream, outputBuffer] : job)
>  		pipe->completeBuffer(request, outputBuffer);
>
> 	pipe->completeRequest(job.request);
>  	conversionQueue_.pop();
>
> which I think is much more readable.

I think the eventual code in v6 is basically the same, minus naming and
omitting `request' variable.  And since it's based on a snippet you
wrote, I suppose it should be OK for you :-) but tell me in case any
further adjustments are needed.

>> > All the output buffers in the map relate to the same request, so I think
>> > it makes sense to store the request pointer separately in the queue for
>> > easy access. Alternatively, you could have a code construct similar to
>> > the above, getting the request pointer from the buffer, and calling
>> > completeRequest() once only. It would be nice if we could share more
>> > code, replacing the above construct with something shared by the cancel
>> > path.
>> >
>> >> >> >> +		}
>> >> >> >> +		conversionQueue_.pop();
>> >> >> >> +	}
>> >> >> >> +}
>> >> >> >> +
>> >> >> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>> >> >> >>  {
>> >> >> >>  	swIsp_->processStats(frame, bufferId,
>> >> >> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>> >> >> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>> >> >> >>  
>> >> >> >>  	data->conversionBuffers_.clear();
>> >> >> >> +	data->clearIncompleteRequests();
>> >> >> >>  
>> >> >> >>  	releasePipeline(data);
>> >> >> >>  }
>> >> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> >> >> >> index e59404691..c9cb11f0f 100644
>> >> >> >> --- a/src/libcamera/pipeline_handler.cpp
>> >> >> >> +++ b/src/libcamera/pipeline_handler.cpp
>> >> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>> >> >> >>  	while (!waitingRequests_.empty()) {
>> >> >> >>  		Request *request = waitingRequests_.front();
>> >> >> >>  		waitingRequests_.pop();
>> >> >> >> -
>> >> >> >> -		request->_d()->cancel();
>> >> >> >> -		completeRequest(request);
>> >> >> >> +		cancelRequest(request);
>> >> >> >>  	}
>> >> >> >>  
>> >> >> >>  	/* Make sure no requests are pending. */
>> >> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>> >> >> >>  	}
>> >> >> >>  
>> >> >> >>  	int ret = queueRequestDevice(camera, request);
>> >> >> >> -	if (ret) {
>> >> >> >> -		request->_d()->cancel();
>> >> >> >> -		completeRequest(request);
>> >> >> >> -	}
>> >> >> >> +	if (ret)
>> >> >> >> +		cancelRequest(request);
>> >> >> >>  }
>> >> >> >>  
>> >> >> >>  /**
>> >> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>> >> >> >>  	}
>> >> >> >>  }
>> >> >> >>  
>> >> >> >> +/**
>> >> >> >> + * \brief Cancel request and signal its completion
>> >> >> >> + * \param[in] request The request to cancel
>> >> >> >> + *
>> >> >> >> + * This function cancels the request in addition to its completion. The same
>> >> >> >> + * rules as for completeRequest() apply.
>> >> >> >> + */
>> >> >> >> +void PipelineHandler::cancelRequest(Request *request)
>> >> >> >> +{
>> >> >> >> +	request->_d()->cancel();
>> >> >> >> +	completeRequest(request);
>> >> >> >> +}
>> >> >> >> +
>> >> >> >>  /**
>> >> >> >>   * \brief Retrieve the absolute path to a platform configuration file
>> >> >> >>   * \param[in] subdir The pipeline handler specific subdirectory name