[v1,6/6] libcamera: pipeline_handler: cancel waiting requests first

23692 diff mbox series

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

When stopping a camera, we may now find that there are requests
queued to the camera but not yet available in the pipeline handler.

These are "waitingReqeusts" which are not yet queued to the device.

While stopping a camera, we ask the pipeline to stop with stopDevice()
and then cancel any waiting requests after.

This however can lead to a race where having stopped the camera and
completed the requests, the pipeline may try to further consume requests
from the waiting lists.

Therefore empty the waitingRequests queue before stopping the device and
cancel the waitingRequests afterwards.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/pipeline_handler.cpp | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Quoting Stefan Klug (2025-06-30 09:11:21)
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> When stopping a camera, we may now find that there are requests
> queued to the camera but not yet available in the pipeline handler.
> 
> These are "waitingReqeusts" which are not yet queued to the device.
> 
> While stopping a camera, we ask the pipeline to stop with stopDevice()
> and then cancel any waiting requests after.
> 
> This however can lead to a race where having stopped the camera and
> completed the requests, the pipeline may try to further consume requests
> from the waiting lists.
> 
> Therefore empty the waitingRequests queue before stopping the device and
> cancel the waitingRequests afterwards.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

That's most of a rewrite - so I don't know who the author is now - but
indeed, based on previous review comments this looks good to me!


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

... But I'm reviewing your modifications not a patch I wrote ;-) 


> ---
>  src/libcamera/pipeline_handler.cpp | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 383c6ad0c4aa..207a2bae96d6 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -364,20 +364,28 @@ void PipelineHandler::unlockMediaDevices()
>   */
>  void PipelineHandler::stop(Camera *camera)
>  {
> +       /*
> +        * Take all waiting requests so that they are not requeued in response
> +        * to completeRequest() being called inside stopDevice(). Cancel them
> +        * after the device to keep them in order.
> +        */
> +       Camera::Private *data = camera->_d();
> +       std::queue<Request *> waitingRequests;
> +       waitingRequests.swap(data->waitingRequests_);
> +
>         /* Stop the pipeline handler and let the queued requests complete. */
>         stopDevice(camera);
>  
> -       Camera::Private *data = camera->_d();
> -
>         /* Cancel and signal as complete all waiting requests. */
> -       while (!data->waitingRequests_.empty()) {
> -               Request *request = data->waitingRequests_.front();
> -               data->waitingRequests_.pop();
> +       while (!waitingRequests.empty()) {
> +               Request *request = waitingRequests.front();
> +               waitingRequests.pop();
>                 cancelRequest(request);
>         }
>  
>         /* Make sure no requests are pending. */
>         ASSERT(data->queuedRequests_.empty());
> +       ASSERT(data->waitingRequests_.empty());
>  
>         data->requestSequence_ = 0;
>  }
> -- 
> 2.48.1
>