| Message ID | 20250807072123.35523-1-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi, Le jeudi 07 août 2025 à 12:51 +0530, Umang Jain a écrit : > RequestWrap `wrap` is set only if completedRequests_ queue > is not empty, and if the `wrap` remains unset, -ENOBUFS is returned > by GstLibcameraSrcState::processRequest(). > > There is no need to set the `err` variable to -ENOBUFS, > by checking the completedRequests_ queue again. Hence, drop this > conditional check from the processRequest() hotpath. > > Signed-off-by: Umang Jain <uajain@igalia.com> I agree with the removal of the check, so for that: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Though, the commit message describe this as if there is no behaviour change. In practice, there is a behaviour change. Before the change, if the request was the last of the queue, it would process that request and return -ENOBUFS. While now, it will request that request and return 0. This change the behaviour of the switch: ret = state->processRequest(); switch (ret) { case 0: /* Another completed request is available, resume the task. */ doResume = true; break; case -EPIPE: gst_task_stop(self->task); return; case -ENOBUFS: default: break; With the effect that once it popped the last request, it will try once again before pausing the task. This task dynamic is known racy, https://bugs.libcamera.org/show_bug.cgi?id=201 I don't think this removal will completely fix the race, but it will likely reduce it. Can you track down the reproducer from bug 201 and see what implication it has ? Then please rewrite your commit message to properly describe the effect of your changes. Nicolas > --- > src/gstreamer/gstlibcamerasrc.cpp | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 79a025a5..3adb34f9 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -346,9 +346,6 @@ int GstLibcameraSrcState::processRequest() > wrap = std::move(completedRequests_.front()); > completedRequests_.pop(); > } > - > - if (completedRequests_.empty()) > - err = -ENOBUFS; > } > > if (!wrap)
Hi Nicolas On 2025-08-07 18:25, Nicolas Dufresne wrote: > Hi, > > Le jeudi 07 août 2025 à 12:51 +0530, Umang Jain a écrit : >> RequestWrap `wrap` is set only if completedRequests_ queue >> is not empty, and if the `wrap` remains unset, -ENOBUFS is returned >> by GstLibcameraSrcState::processRequest(). >> >> There is no need to set the `err` variable to -ENOBUFS, >> by checking the completedRequests_ queue again. Hence, drop this >> conditional check from the processRequest() hotpath. >> >> Signed-off-by: Umang Jain <uajain@igalia.com> > > I agree with the removal of the check, so for that: > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Though, the commit message describe this as if there is no behaviour change. In > practice, there is a behaviour change. Before the change, if the request was the > last of the queue, it would process that request and return -ENOBUFS. While now, > it will request that request and return 0. Yes, I am aware of that behaviour and I agree I should have done better job in the commit message. Removal the check will lead to one more iteration to pause the GstTask, but eventually it will be paused with -ENOBUFS, so from that pov, I implied that no behaviorable change as such. > > This change the behaviour of the switch: > > ret = state->processRequest(); > switch (ret) { > case 0: > /* Another completed request is available, resume the task. */ > doResume = true; > break; > > case -EPIPE: > gst_task_stop(self->task); > return; > > case -ENOBUFS: > default: > break; > > > With the effect that once it popped the last request, it will try once again > before pausing the task. This task dynamic is known racy, > > https://bugs.libcamera.org/show_bug.cgi?id=201 Oh, this was not on my radar. I will try to a look there. > > I don't think this removal will completely fix the race, but it will likely > reduce it. Can you track down the reproducer from bug 201 and see what > implication it has ? Then please rewrite your commit message to properly > describe the effect of your changes. Ack. > > Nicolas > >> --- >> src/gstreamer/gstlibcamerasrc.cpp | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp >> index 79a025a5..3adb34f9 100644 >> --- a/src/gstreamer/gstlibcamerasrc.cpp >> +++ b/src/gstreamer/gstlibcamerasrc.cpp >> @@ -346,9 +346,6 @@ int GstLibcameraSrcState::processRequest() >> wrap = std::move(completedRequests_.front()); >> completedRequests_.pop(); >> } >> - >> - if (completedRequests_.empty()) >> - err = -ENOBUFS; >> } >> >> if (!wrap)
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 79a025a5..3adb34f9 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -346,9 +346,6 @@ int GstLibcameraSrcState::processRequest() wrap = std::move(completedRequests_.front()); completedRequests_.pop(); } - - if (completedRequests_.empty()) - err = -ENOBUFS; } if (!wrap)
RequestWrap `wrap` is set only if completedRequests_ queue is not empty, and if the `wrap` remains unset, -ENOBUFS is returned by GstLibcameraSrcState::processRequest(). There is no need to set the `err` variable to -ENOBUFS, by checking the completedRequests_ queue again. Hence, drop this conditional check from the processRequest() hotpath. Signed-off-by: Umang Jain <uajain@igalia.com> --- src/gstreamer/gstlibcamerasrc.cpp | 3 --- 1 file changed, 3 deletions(-)