| Message ID | 20251022080011.97665-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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. */
Hi 2025. 10. 22. 18:48 keltezéssel, Stefan Klug írta: > 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 I think we should select one of https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern and use that so the issues are automatically closed. >>> >>> 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? I suppose you could have a `completeRequests()` function that does something like: completeRequests(auto &c) { while (!c.empty()) { req = c.front(); if (req->status() == Pending) break; c.pop_front(); camera->requestComplete(req); } } so mainly what `PipelineHandler::completeRequest()` does but on an arbitrary container of requests. Then `PipelineHandler::stop()` could cancel every waiting request, and complete all of them with the above function. Although this won't set a sequence number on the request. Reading the documentation of `Requests::sequence()` seems to suggest that libcamera should guarantee a proper request sequence number for every request that was successfully queued, which includes these waiting requests. Initially I also didn't particularly like the fact the these requests are added to `queuedRequests_`, but the more I look at it, the more I like the fact that these requests also go through the same "pipeline" as the other ones. And the proposed change is also quite short and simple. Nonetheless, I think it could still be argued that a future change could (easily?) break it by accident. Regards, Barnabás Pőcze > > Best regards, > Stefan > >> >> >>> } >>> >>> /* Make sure no requests are pending. */
On Thu, Oct 23, 2025 at 10:51:18AM +0200, Barnabás Pőcze wrote: > Hi > > 2025. 10. 22. 18:48 keltezéssel, Stefan Klug írta: > > 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 > > I think we should select one of https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern > and use that so the issues are automatically closed. I'd like that to go in commit message trailers, with a full URL. That doesn't match the default closing pattern. Apparently it can be customized, but not per repository. > >>> > >>> 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? > > I suppose you could have a `completeRequests()` function that does something like: > > completeRequests(auto &c) { > while (!c.empty()) { > req = c.front(); > if (req->status() == Pending) break; > c.pop_front(); > camera->requestComplete(req); > } > } > > so mainly what `PipelineHandler::completeRequest()` does but on an arbitrary > container of requests. Then `PipelineHandler::stop()` could cancel every waiting > request, and complete all of them with the above function. > > Although this won't set a sequence number on the request. Reading the documentation > of `Requests::sequence()` seems to suggest that libcamera should guarantee a proper > request sequence number for every request that was successfully queued, which includes > these waiting requests. Yes, we should always have a proper sequence number in requests. > Initially I also didn't particularly like the fact the these requests are added > to `queuedRequests_`, but the more I look at it, the more I like the fact that > these requests also go through the same "pipeline" as the other ones. And the > proposed change is also quite short and simple. Nonetheless, I think it could > still be argued that a future change could (easily?) break it by accident. Going through the same code path for completion of all requests would be my preferred option. > >>> } > >>> > >>> /* Make sure no requests are pending. */
Hi 2025. 10. 23. 12:56 keltezéssel, Laurent Pinchart írta: > On Thu, Oct 23, 2025 at 10:51:18AM +0200, Barnabás Pőcze wrote: >> Hi >> >> 2025. 10. 22. 18:48 keltezéssel, Stefan Klug írta: >>> 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 >> >> I think we should select one of https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern >> and use that so the issues are automatically closed. > > I'd like that to go in commit message trailers, with a full URL. That > doesn't match the default closing pattern. Apparently it can be > customized, but not per repository. Both `Fixes: $url` and `Closes: $url` works, so it seems the `:` is not an issue. Or the issue is that you would like to use a different word? > >>>>> >>>>> 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? >> >> I suppose you could have a `completeRequests()` function that does something like: >> >> completeRequests(auto &c) { >> while (!c.empty()) { >> req = c.front(); >> if (req->status() == Pending) break; >> c.pop_front(); >> camera->requestComplete(req); >> } >> } >> >> so mainly what `PipelineHandler::completeRequest()` does but on an arbitrary >> container of requests. Then `PipelineHandler::stop()` could cancel every waiting >> request, and complete all of them with the above function. >> >> Although this won't set a sequence number on the request. Reading the documentation >> of `Requests::sequence()` seems to suggest that libcamera should guarantee a proper >> request sequence number for every request that was successfully queued, which includes >> these waiting requests. > > Yes, we should always have a proper sequence number in requests. > >> Initially I also didn't particularly like the fact the these requests are added >> to `queuedRequests_`, but the more I look at it, the more I like the fact that >> these requests also go through the same "pipeline" as the other ones. And the >> proposed change is also quite short and simple. Nonetheless, I think it could >> still be argued that a future change could (easily?) break it by accident. > > Going through the same code path for completion of all requests would be > my preferred option. > >>>>> } >>>>> >>>>> /* Make sure no requests are pending. */ > > -- > Regards, > > Laurent Pinchart
On Mon, Oct 27, 2025 at 09:34:18AM +0100, Barnabás Pőcze wrote: > 2025. 10. 23. 12:56 keltezéssel, Laurent Pinchart írta: > > On Thu, Oct 23, 2025 at 10:51:18AM +0200, Barnabás Pőcze wrote: > >> 2025. 10. 22. 18:48 keltezéssel, Stefan Klug írta: > >>> Quoting uajain (2025-10-22 16:46:07) > >>>> 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 > >> > >> I think we should select one of https://docs.gitlab.com/user/project/issues/managing_issues/#default-closing-pattern > >> and use that so the issues are automatically closed. > > > > I'd like that to go in commit message trailers, with a full URL. That > > doesn't match the default closing pattern. Apparently it can be > > customized, but not per repository. > > Both `Fixes: $url` and `Closes: $url` works, so it seems the `:` is not an issue. > Or the issue is that you would like to use a different word? You're right, I had missed the (:?) in the regex. It's all fine then. Let's standardize on using a trailer and a full url. If we want to follow kernel conventions, it would be 'Closes:'. We can keep the Fixes: tag to reference a commit, as we do today. > >>>>> > >>>>> 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? > >> > >> I suppose you could have a `completeRequests()` function that does something like: > >> > >> completeRequests(auto &c) { > >> while (!c.empty()) { > >> req = c.front(); > >> if (req->status() == Pending) break; > >> c.pop_front(); > >> camera->requestComplete(req); > >> } > >> } > >> > >> so mainly what `PipelineHandler::completeRequest()` does but on an arbitrary > >> container of requests. Then `PipelineHandler::stop()` could cancel every waiting > >> request, and complete all of them with the above function. > >> > >> Although this won't set a sequence number on the request. Reading the documentation > >> of `Requests::sequence()` seems to suggest that libcamera should guarantee a proper > >> request sequence number for every request that was successfully queued, which includes > >> these waiting requests. > > > > Yes, we should always have a proper sequence number in requests. > > > >> Initially I also didn't particularly like the fact the these requests are added > >> to `queuedRequests_`, but the more I look at it, the more I like the fact that > >> these requests also go through the same "pipeline" as the other ones. And the > >> proposed change is also quite short and simple. Nonetheless, I think it could > >> still be argued that a future change could (easily?) break it by accident. > > > > Going through the same code path for completion of all requests would be > > my preferred option. > > > >>>>> } > >>>>> > >>>>> /* Make sure no requests are pending. */
Quoting Stefan Klug (2025-10-22 16:59:11) > 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); On first sight it looked hacky, but the more I looked at it it actually looks pretty good; it forces the canceled request to go through the same proper paths a regular canceled request would've taken were it not for the stop() call. I think the doQueueRequest() call itself is what makes this look hacky, but the comment covers it. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > } > > /* Make sure no requests are pending. */ > -- > 2.48.1 >
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); } /* Make sure no requests are pending. */
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(-)