libcamera: pipeline_handler: Fix requestComplete on waiting requests on stop

24719 diff mbox series

The requestComplete signal is not emitted when the camera is stopped and
the request is still in the waitingRequests_ queue. This was reported in
[1]. Fix that by calling doQueueRequest() on the waiting requests after
marking them as cancelled. This ensures that the request is not queued
to the device but gets added to the queuedRequests_ list. This list is
then iterated in completeRequest() and leads to the requestComplete
signal.

[1] https://gitlab.freedesktop.org/camera/libcamera/-/issues/281

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/pipeline_handler.cpp | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Hi Stefan,

On 2025-10-22 13:29, Stefan Klug wrote:
> The requestComplete signal is not emitted when the camera is stopped and
> the request is still in the waitingRequests_ queue. This was reported in
> [1]. Fix that by calling doQueueRequest() on the waiting requests after
> marking them as cancelled. This ensures that the request is not queued
> to the device but gets added to the queuedRequests_ list. This list is
> then iterated in completeRequest() and leads to the requestComplete
> signal.
> 
> [1] https://gitlab.freedesktop.org/camera/libcamera/-/issues/281
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/libcamera/pipeline_handler.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index e5f9e55c9783..3aa814069382 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -380,7 +380,15 @@ void PipelineHandler::stop(Camera *camera)
>  	while (!waitingRequests.empty()) {
>  		Request *request = waitingRequests.front();
>  		waitingRequests.pop();
> -		cancelRequest(request);
> +
> +		/*
> +		 * Cancel all requests by marking them as cancelled and calling
> +		 * doQueueRequest() instead of cancelRequest() to ensure the
> +		 * request is temporarily added to queuedRequests_ so it can
> +		 * then be properly completed in completeRequest().
> +		 */
> +		request->_d()->cancel();
> +		doQueueRequest(request);

Can't you simple call Camera::requestComplete() here? I don't like the
idea of putting cancelled requests (from waiting queue) in
queuedRequests_.

With that you will still be able to complete in-order, as at this point,
queuedRequests_ is definitely empty, if I am reading this right.


>  	}
>  
>  	/* Make sure no requests are pending. */

Hi Umang,

Thank you for the review.

Quoting uajain (2025-10-22 16:46:07)
> Hi Stefan,
> 
> On 2025-10-22 13:29, Stefan Klug wrote:
> > The requestComplete signal is not emitted when the camera is stopped and
> > the request is still in the waitingRequests_ queue. This was reported in
> > [1]. Fix that by calling doQueueRequest() on the waiting requests after
> > marking them as cancelled. This ensures that the request is not queued
> > to the device but gets added to the queuedRequests_ list. This list is
> > then iterated in completeRequest() and leads to the requestComplete
> > signal.
> > 
> > [1] https://gitlab.freedesktop.org/camera/libcamera/-/issues/281
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline_handler.cpp | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index e5f9e55c9783..3aa814069382 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -380,7 +380,15 @@ void PipelineHandler::stop(Camera *camera)
> >       while (!waitingRequests.empty()) {
> >               Request *request = waitingRequests.front();
> >               waitingRequests.pop();
> > -             cancelRequest(request);
> > +
> > +             /*
> > +              * Cancel all requests by marking them as cancelled and calling
> > +              * doQueueRequest() instead of cancelRequest() to ensure the
> > +              * request is temporarily added to queuedRequests_ so it can
> > +              * then be properly completed in completeRequest().
> > +              */
> > +             request->_d()->cancel();
> > +             doQueueRequest(request);
> 
> Can't you simple call Camera::requestComplete() here? I don't like the
> idea of putting cancelled requests (from waiting queue) in
> queuedRequests_.
> 
> With that you will still be able to complete in-order, as at this point,
> queuedRequests_ is definitely empty, if I am reading this right.

Yes, I was considering that, too. I think it would work. I thought it
would be nice if the request sequence would be filled and I didn't
really check if I need to do additional checks. I'm fine with either
case. Maybe there are other opinions?

Best regards,
Stefan

> 
> 
> >       }
> >  
> >       /* Make sure no requests are pending. */