| Message ID | 20250909131237.1344483-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi, Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : > The moment execution enters `Camera::queueRequest()`, an application must be > prepared to received the corresponding `requestCompleted` signal. However, > the gstreamer element is currently not prepared: it queues the request, > takes a lock, then inserts into a list. > > If the request completion handler acquires the lock right after the queueing, > then it will not find the object in the list that it expects. Even potentially > encountering an empty list, running into undefined behaviour when trying > to pop from it. > > Fix that by queueing the request after the lock protected insertion. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 > Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > > Doing it while holding the lock is not ideal, but the alternative is > getting a raw pointer to the `Request` object beforehand, but that does > not seem too desirable either. If we are worried calling into libcamera with uur own lock held, have you considered pushing it into the list before calling cam_->queueRequest() ? The camera object in libcamera is supposed to be threadsafe, and we should have strong ownership on the state and request wrap (if not some reffing is missing I guess). Nicolas p.s. the lock is dangerous if libcamera decides to callback from the caller thread instead of a separate thread. So calling with lock is implementation dependant. > > --- > src/gstreamer/gstlibcamerasrc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > b/src/gstreamer/gstlibcamerasrc.cpp > index 79a025a57..103dfbca2 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() > } > > GST_TRACE_OBJECT(src_, "Requesting buffers"); > - cam_->queueRequest(wrap->request_.get()); > > { > GLibLocker locker(&lock_); > + cam_->queueRequest(wrap->request_.get()); > queuedRequests_.push(std::move(wrap)); > } > > -- > 2.51.0
Hi 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: > Hi, > > Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : >> The moment execution enters `Camera::queueRequest()`, an application must be >> prepared to received the corresponding `requestCompleted` signal. However, >> the gstreamer element is currently not prepared: it queues the request, >> takes a lock, then inserts into a list. >> >> If the request completion handler acquires the lock right after the queueing, >> then it will not find the object in the list that it expects. Even potentially >> encountering an empty list, running into undefined behaviour when trying >> to pop from it. >> >> Fix that by queueing the request after the lock protected insertion. >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 >> Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> >> Doing it while holding the lock is not ideal, but the alternative is >> getting a raw pointer to the `Request` object beforehand, but that does >> not seem too desirable either. > > If we are worried calling into libcamera with uur own lock held, have you > considered pushing it into the list before calling cam_->queueRequest() ? The > camera object in libcamera is supposed to be threadsafe, and we should have > strong ownership on the state and request wrap (if not some reffing is missing I > guess). The `RequestWrap` pointer is moved into the container, so one would need to take a raw pointer to the `Request`. The fact that the request completions handler unconditionally pops the first element worries me a bit. Although if things have already gone out of sync there, it might not matter. > > Nicolas > > p.s. the lock is dangerous if libcamera decides to callback from the caller > thread instead of a separate thread. So calling with lock is implementation > dependant. That's true, but currently it is guaranteed. The application developer guide mentions it explicitly. Regards, Barnabás Pőcze > >> >> --- >> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >> b/src/gstreamer/gstlibcamerasrc.cpp >> index 79a025a57..103dfbca2 100644 >> --- a/src/gstreamer/gstlibcamerasrc.cpp >> +++ b/src/gstreamer/gstlibcamerasrc.cpp >> @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() >> } >> >> GST_TRACE_OBJECT(src_, "Requesting buffers"); >> - cam_->queueRequest(wrap->request_.get()); >> >> { >> GLibLocker locker(&lock_); >> + cam_->queueRequest(wrap->request_.get()); >> queuedRequests_.push(std::move(wrap)); >> } >> >> -- >> 2.51.0
Quoting Barnabás Pőcze (2025-09-10 15:01:15) > Hi > > 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: > > Hi, > > > > Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : > >> The moment execution enters `Camera::queueRequest()`, an application must be > >> prepared to received the corresponding `requestCompleted` signal. However, > >> the gstreamer element is currently not prepared: it queues the request, > >> takes a lock, then inserts into a list. > >> > >> If the request completion handler acquires the lock right after the queueing, > >> then it will not find the object in the list that it expects. Even potentially > >> encountering an empty list, running into undefined behaviour when trying > >> to pop from it. > >> > >> Fix that by queueing the request after the lock protected insertion. > >> > >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 > >> Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") Based on https://bugs.libcamera.org/show_bug.cgi?id=238#c18 I suspect we should try to get this into 0.6. > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> > >> Doing it while holding the lock is not ideal, but the alternative is > >> getting a raw pointer to the `Request` object beforehand, but that does > >> not seem too desirable either. > > > > If we are worried calling into libcamera with uur own lock held, have you > > considered pushing it into the list before calling cam_->queueRequest() ? The > > camera object in libcamera is supposed to be threadsafe, and we should have > > strong ownership on the state and request wrap (if not some reffing is missing I > > guess). > > The `RequestWrap` pointer is moved into the container, so one would need to take > a raw pointer to the `Request`. The fact that the request completions handler > unconditionally pops the first element worries me a bit. Although if things have > already gone out of sync there, it might not matter. > > > > > > Nicolas > > > > p.s. the lock is dangerous if libcamera decides to callback from the caller > > thread instead of a separate thread. So calling with lock is implementation > > dependant. > > That's true, but currently it is guaranteed. The application developer guide > mentions it explicitly. > Is there anything else required to progress this patch? any more specific concerns? -- Kieran > > Regards, > Barnabás Pőcze > > > > >> > >> --- > >> src/gstreamer/gstlibcamerasrc.cpp | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp > >> b/src/gstreamer/gstlibcamerasrc.cpp > >> index 79a025a57..103dfbca2 100644 > >> --- a/src/gstreamer/gstlibcamerasrc.cpp > >> +++ b/src/gstreamer/gstlibcamerasrc.cpp > >> @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() > >> } > >> > >> GST_TRACE_OBJECT(src_, "Requesting buffers"); > >> - cam_->queueRequest(wrap->request_.get()); > >> > >> { > >> GLibLocker locker(&lock_); > >> + cam_->queueRequest(wrap->request_.get()); > >> queuedRequests_.push(std::move(wrap)); > >> } > >> > >> -- > >> 2.51.0 >
2025. 10. 13. 13:40 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-09-10 15:01:15) >> Hi >> >> 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: >>> Hi, >>> >>> Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : >>>> The moment execution enters `Camera::queueRequest()`, an application must be >>>> prepared to received the corresponding `requestCompleted` signal. However, >>>> the gstreamer element is currently not prepared: it queues the request, >>>> takes a lock, then inserts into a list. >>>> >>>> If the request completion handler acquires the lock right after the queueing, >>>> then it will not find the object in the list that it expects. Even potentially >>>> encountering an empty list, running into undefined behaviour when trying >>>> to pop from it. >>>> >>>> Fix that by queueing the request after the lock protected insertion. >>>> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 >>>> Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") > > Based on https://bugs.libcamera.org/show_bug.cgi?id=238#c18 I suspect we > should try to get this into 0.6. > > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> >>>> Doing it while holding the lock is not ideal, but the alternative is >>>> getting a raw pointer to the `Request` object beforehand, but that does >>>> not seem too desirable either. >>> >>> If we are worried calling into libcamera with uur own lock held, have you >>> considered pushing it into the list before calling cam_->queueRequest() ? The >>> camera object in libcamera is supposed to be threadsafe, and we should have >>> strong ownership on the state and request wrap (if not some reffing is missing I >>> guess). >> >> The `RequestWrap` pointer is moved into the container, so one would need to take >> a raw pointer to the `Request`. The fact that the request completions handler >> unconditionally pops the first element worries me a bit. Although if things have >> already gone out of sync there, it might not matter. >> >> >>> >>> Nicolas >>> >>> p.s. the lock is dangerous if libcamera decides to callback from the caller >>> thread instead of a separate thread. So calling with lock is implementation >>> dependant. >> >> That's true, but currently it is guaranteed. The application developer guide >> mentions it explicitly. >> > > Is there anything else required to progress this patch? any more > specific concerns? From my point of view, there are no concerns, it can be merged as is. Before the mentioned commit (06bd05beced313) the queueRequest() and push() calls were done under the same lock, and the commit message also does not mention any correctness issues with that setup. Regards, Barnabás Pőcze > > -- > Kieran > > > > >> >> Regards, >> Barnabás Pőcze >> >>> >>>> >>>> --- >>>> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >>>> b/src/gstreamer/gstlibcamerasrc.cpp >>>> index 79a025a57..103dfbca2 100644 >>>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>>> @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() >>>> } >>>> >>>> GST_TRACE_OBJECT(src_, "Requesting buffers"); >>>> - cam_->queueRequest(wrap->request_.get()); >>>> >>>> { >>>> GLibLocker locker(&lock_); >>>> + cam_->queueRequest(wrap->request_.get()); >>>> queuedRequests_.push(std::move(wrap)); >>>> } >>>> >>>> -- >>>> 2.51.0 >>
2025. 10. 13. 14:19 keltezéssel, Barnabás Pőcze írta: > 2025. 10. 13. 13:40 keltezéssel, Kieran Bingham írta: >> Quoting Barnabás Pőcze (2025-09-10 15:01:15) >>> Hi >>> >>> 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: >>>> Hi, >>>> >>>> Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : >>>>> The moment execution enters `Camera::queueRequest()`, an application must be >>>>> prepared to received the corresponding `requestCompleted` signal. However, >>>>> the gstreamer element is currently not prepared: it queues the request, >>>>> takes a lock, then inserts into a list. >>>>> >>>>> If the request completion handler acquires the lock right after the queueing, >>>>> then it will not find the object in the list that it expects. Even potentially >>>>> encountering an empty list, running into undefined behaviour when trying >>>>> to pop from it. >>>>> >>>>> Fix that by queueing the request after the lock protected insertion. >>>>> >>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 >>>>> Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") >> >> Based on https://bugs.libcamera.org/show_bug.cgi?id=238#c18 I suspect we >> should try to get this into 0.6. >> >> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> >>>>> Doing it while holding the lock is not ideal, but the alternative is >>>>> getting a raw pointer to the `Request` object beforehand, but that does >>>>> not seem too desirable either. >>>> >>>> If we are worried calling into libcamera with uur own lock held, have you >>>> considered pushing it into the list before calling cam_->queueRequest() ? The >>>> camera object in libcamera is supposed to be threadsafe, and we should have >>>> strong ownership on the state and request wrap (if not some reffing is missing I >>>> guess). >>> >>> The `RequestWrap` pointer is moved into the container, so one would need to take >>> a raw pointer to the `Request`. The fact that the request completions handler >>> unconditionally pops the first element worries me a bit. Although if things have >>> already gone out of sync there, it might not matter. >>> >>> >>>> >>>> Nicolas >>>> >>>> p.s. the lock is dangerous if libcamera decides to callback from the caller >>>> thread instead of a separate thread. So calling with lock is implementation >>>> dependant. >>> >>> That's true, but currently it is guaranteed. The application developer guide >>> mentions it explicitly. >>> >> >> Is there anything else required to progress this patch? any more >> specific concerns? > > From my point of view, there are no concerns, it can be merged as is. > Before the mentioned commit (06bd05beced313) the queueRequest() and push() > calls were done under the same lock, and the commit message also does not > mention any correctness issues with that setup. Correction: I would like to slightly tweak the commit message. For example, the last sentence is not in sync with the code. > > > Regards, > Barnabás Pőcze > >> >> -- >> Kieran >> >> >> >> >>> >>> Regards, >>> Barnabás Pőcze >>> >>>> >>>>> >>>>> --- >>>>> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >>>>> b/src/gstreamer/gstlibcamerasrc.cpp >>>>> index 79a025a57..103dfbca2 100644 >>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>>>> @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() >>>>> } >>>>> >>>>> GST_TRACE_OBJECT(src_, "Requesting buffers"); >>>>> - cam_->queueRequest(wrap->request_.get()); >>>>> >>>>> { >>>>> GLibLocker locker(&lock_); >>>>> + cam_->queueRequest(wrap->request_.get()); >>>>> queuedRequests_.push(std::move(wrap)); >>>>> } >>>>> >>>>> -- >>>>> 2.51.0 >>> >
On 2025-10-13 17:49, Barnabás Pőcze wrote: > 2025. 10. 13. 13:40 keltezéssel, Kieran Bingham írta: >> Quoting Barnabás Pőcze (2025-09-10 15:01:15) >>> Hi >>> >>> 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: >>>> Hi, >>>> >>>> Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : >>>>> The moment execution enters `Camera::queueRequest()`, an application must be >>>>> prepared to received the corresponding `requestCompleted` signal. However, >>>>> the gstreamer element is currently not prepared: it queues the request, >>>>> takes a lock, then inserts into a list. >>>>> >>>>> If the request completion handler acquires the lock right after the queueing, >>>>> then it will not find the object in the list that it expects. Even potentially >>>>> encountering an empty list, running into undefined behaviour when trying >>>>> to pop from it. >>>>> >>>>> Fix that by queueing the request after the lock protected insertion. >>>>> >>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 >>>>> Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") >> >> Based on https://bugs.libcamera.org/show_bug.cgi?id=238#c18 I suspect we >> should try to get this into 0.6. >> >> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> >>>>> Doing it while holding the lock is not ideal, but the alternative is >>>>> getting a raw pointer to the `Request` object beforehand, but that does >>>>> not seem too desirable either. >>>> >>>> If we are worried calling into libcamera with uur own lock held, have you >>>> considered pushing it into the list before calling cam_->queueRequest() ? The >>>> camera object in libcamera is supposed to be threadsafe, and we should have >>>> strong ownership on the state and request wrap (if not some reffing is missing I >>>> guess). >>> >>> The `RequestWrap` pointer is moved into the container, so one would need to take >>> a raw pointer to the `Request`. The fact that the request completions handler >>> unconditionally pops the first element worries me a bit. Although if things have >>> already gone out of sync there, it might not matter. >>> >>> >>>> >>>> Nicolas >>>> >>>> p.s. the lock is dangerous if libcamera decides to callback from the caller >>>> thread instead of a separate thread. So calling with lock is implementation >>>> dependant. >>> >>> That's true, but currently it is guaranteed. The application developer guide >>> mentions it explicitly. >>> >> >> Is there anything else required to progress this patch? any more >> specific concerns? > > From my point of view, there are no concerns, it can be merged as is. > Before the mentioned commit (06bd05beced313) the queueRequest() and push() > calls were done under the same lock, and the commit message also does not > mention any correctness issues with that setup. I don't see any major objections here and ack the merge given it helps with the current bug. The existence of g_return_if_fail() in the request complete handler suggests that going out-of-sync is a programming error and should not occur. This patch aligns with that as well hence, Acked-by: Umang Jain <uajain@igalia.com> > > > Regards, > Barnabás Pőcze > >> >> -- >> Kieran >> >> >> >> >>> >>> Regards, >>> Barnabás Pőcze >>> >>>> >>>>> >>>>> --- >>>>> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >>>>> b/src/gstreamer/gstlibcamerasrc.cpp >>>>> index 79a025a57..103dfbca2 100644 >>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>>>> @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() >>>>> } >>>>> >>>>> GST_TRACE_OBJECT(src_, "Requesting buffers"); >>>>> - cam_->queueRequest(wrap->request_.get()); >>>>> >>>>> { >>>>> GLibLocker locker(&lock_); >>>>> + cam_->queueRequest(wrap->request_.get()); >>>>> queuedRequests_.push(std::move(wrap)); >>>>> } >>>>> >>>>> -- >>>>> 2.51.0 >>>
Hi Nicolas 2025. 09. 10. 16:01 keltezéssel, Barnabás Pőcze írta: > Hi > > 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: >> Hi, >> >> Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : >>> The moment execution enters `Camera::queueRequest()`, an application must be >>> prepared to received the corresponding `requestCompleted` signal. However, >>> the gstreamer element is currently not prepared: it queues the request, >>> takes a lock, then inserts into a list. >>> >>> If the request completion handler acquires the lock right after the queueing, >>> then it will not find the object in the list that it expects. Even potentially >>> encountering an empty list, running into undefined behaviour when trying >>> to pop from it. >>> >>> Fix that by queueing the request after the lock protected insertion. >>> >>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 >>> Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> --- >>> >>> Doing it while holding the lock is not ideal, but the alternative is >>> getting a raw pointer to the `Request` object beforehand, but that does >>> not seem too desirable either. >> >> If we are worried calling into libcamera with uur own lock held, have you >> considered pushing it into the list before calling cam_->queueRequest() ? The >> camera object in libcamera is supposed to be threadsafe, and we should have >> strong ownership on the state and request wrap (if not some reffing is missing I >> guess). > > The `RequestWrap` pointer is moved into the container, so one would need to take > a raw pointer to the `Request`. The fact that the request completions handler > unconditionally pops the first element worries me a bit. Although if things have > already gone out of sync there, it might not matter. > > >> >> Nicolas >> >> p.s. the lock is dangerous if libcamera decides to callback from the caller >> thread instead of a separate thread. So calling with lock is implementation >> dependant. > > That's true, but currently it is guaranteed. The application developer guide > mentions it explicitly. > > We would like to move forward with this change, and wondering if it looks acceptable to you and/or if you have any other concerns? Regards, Barnabás Pőcze > >> >>> >>> --- >>> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >>> b/src/gstreamer/gstlibcamerasrc.cpp >>> index 79a025a57..103dfbca2 100644 >>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>> @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() >>> } >>> >>> GST_TRACE_OBJECT(src_, "Requesting buffers"); >>> - cam_->queueRequest(wrap->request_.get()); >>> >>> { >>> GLibLocker locker(&lock_); >>> + cam_->queueRequest(wrap->request_.get()); >>> queuedRequests_.push(std::move(wrap)); >>> } >>> >>> -- >>> 2.51.0 >
Hi, Le jeudi 23 octobre 2025 à 16:04 +0200, Barnabás Pőcze a écrit : > Hi Nicolas > > 2025. 09. 10. 16:01 keltezéssel, Barnabás Pőcze írta: > > Hi > > > > 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: > > > Hi, > > > > > > Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : > > > > The moment execution enters `Camera::queueRequest()`, an application must be > > > > prepared to received the corresponding `requestCompleted` signal. However, > > > > the gstreamer element is currently not prepared: it queues the request, > > > > takes a lock, then inserts into a list. > > > > > > > > If the request completion handler acquires the lock right after the queueing, > > > > then it will not find the object in the list that it expects. Even potentially > > > > encountering an empty list, running into undefined behaviour when trying > > > > to pop from it. > > > > > > > > Fix that by queueing the request after the lock protected insertion. > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 > > > > Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > --- > > > > > > > > Doing it while holding the lock is not ideal, but the alternative is > > > > getting a raw pointer to the `Request` object beforehand, but that does > > > > not seem too desirable either. > > > > > > If we are worried calling into libcamera with uur own lock held, have you > > > considered pushing it into the list before calling cam_->queueRequest() ? The > > > camera object in libcamera is supposed to be threadsafe, and we should have > > > strong ownership on the state and request wrap (if not some reffing is missing I > > > guess). > > > > The `RequestWrap` pointer is moved into the container, so one would need to take > > a raw pointer to the `Request`. The fact that the request completions handler > > unconditionally pops the first element worries me a bit. Although if things have > > already gone out of sync there, it might not matter. > > > > > > > > > > Nicolas > > > > > > p.s. the lock is dangerous if libcamera decides to callback from the caller > > > thread instead of a separate thread. So calling with lock is implementation > > > dependant. > > > > That's true, but currently it is guaranteed. The application developer guide > > mentions it explicitly. > > > > > > We would like to move forward with this change, and wondering if it looks acceptable > to you and/or if you have any other concerns? I was mostly making suggestion to improve memory ownership, with some effort, the use of C++ implicit ref count is possible with shared pointer, but really its up to you. It seems no one is worried of calling into libcamera with lock held. Nicolas > > > Regards, > Barnabás Pőcze > > > > > > > > > > > > > > --- > > > > src/gstreamer/gstlibcamerasrc.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > > > b/src/gstreamer/gstlibcamerasrc.cpp > > > > index 79a025a57..103dfbca2 100644 > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > > @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() > > > > } > > > > > > > > GST_TRACE_OBJECT(src_, "Requesting buffers"); > > > > - cam_->queueRequest(wrap->request_.get()); > > > > > > > > { > > > > GLibLocker locker(&lock_); > > > > + cam_->queueRequest(wrap->request_.get()); > > > > queuedRequests_.push(std::move(wrap)); > > > > } > > > > > > > > -- > > > > 2.51.0 > >
2025. 11. 04. 19:02 keltezéssel, Nicolas Dufresne írta: > Hi, > > Le jeudi 23 octobre 2025 à 16:04 +0200, Barnabás Pőcze a écrit : >> Hi Nicolas >> >> 2025. 09. 10. 16:01 keltezéssel, Barnabás Pőcze írta: >>> Hi >>> >>> 2025. 09. 09. 16:34 keltezéssel, Nicolas Dufresne írta: >>>> Hi, >>>> >>>> Le mardi 09 septembre 2025 à 15:12 +0200, Barnabás Pőcze a écrit : >>>>> The moment execution enters `Camera::queueRequest()`, an application must be >>>>> prepared to received the corresponding `requestCompleted` signal. However, >>>>> the gstreamer element is currently not prepared: it queues the request, >>>>> takes a lock, then inserts into a list. >>>>> >>>>> If the request completion handler acquires the lock right after the queueing, >>>>> then it will not find the object in the list that it expects. Even potentially >>>>> encountering an empty list, running into undefined behaviour when trying >>>>> to pop from it. >>>>> >>>>> Fix that by queueing the request after the lock protected insertion. >>>>> >>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 >>>>> Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> >>>>> Doing it while holding the lock is not ideal, but the alternative is >>>>> getting a raw pointer to the `Request` object beforehand, but that does >>>>> not seem too desirable either. >>>> >>>> If we are worried calling into libcamera with uur own lock held, have you >>>> considered pushing it into the list before calling cam_->queueRequest() ? The >>>> camera object in libcamera is supposed to be threadsafe, and we should have >>>> strong ownership on the state and request wrap (if not some reffing is missing I >>>> guess). >>> >>> The `RequestWrap` pointer is moved into the container, so one would need to take >>> a raw pointer to the `Request`. The fact that the request completions handler >>> unconditionally pops the first element worries me a bit. Although if things have >>> already gone out of sync there, it might not matter. >>> >>> >>>> >>>> Nicolas >>>> >>>> p.s. the lock is dangerous if libcamera decides to callback from the caller >>>> thread instead of a separate thread. So calling with lock is implementation >>>> dependant. >>> >>> That's true, but currently it is guaranteed. The application developer guide >>> mentions it explicitly. >>> >>> >> >> We would like to move forward with this change, and wondering if it looks acceptable >> to you and/or if you have any other concerns? > > I was mostly making suggestion to improve memory ownership, with some effort, > the use of C++ implicit ref count is possible with shared pointer, but really > its up to you. It seems no one is worried of calling into libcamera with lock > held. The `queueRequest()` call can be placed in 3 different position wrt. the insertion into the `queuedRequests_` list. (a) before * currently implemented * not entirely correct: the request could complete before the insertion (b) at the same time (wrt. `lock_`): * this is proposed * addresses the "early" completion issue of (a) by doing the `queueRequest() under the lock, preventing the request completion handler from progressing too soon * not ideal because it calls into libcamera with the lock held (c) after * addresses the "early" completion issue of (a) by doing the `queueRequest()` after the mutex has been unlocked * not ideal because the list contains std::unique_ptr, so the ownership of the request is transferred to the container; a raw pointer to the request would be needed As far as I understand `gst_libcamera_src_task_run()` is the only caller, so `GstLibcameraSrcState::queueRequest()` only ever runs in a single thread, thus both (b) and (c) should be correct. As far as I gather you would prefer something that does not call into libcamera with `lock_` held, which leaves only (c). My concern with (c) is what happens if things go wrong. That is, if we get a raw pointer to the `Request`, can that be invalidated before `queueRequest()` time? I suppose things are already in a bad state at that point, so maybe it does not matter much. As a variant of (c), if I understand you correctly, std::shared_ptr could be used in the container. Is that the suggestion or am I misunderstanding? Regards, Barnabás Pőcze > > Nicolas > >> >> >> Regards, >> Barnabás Pőcze >> >>> >>>> >>>>> >>>>> --- >>>>> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >>>>> b/src/gstreamer/gstlibcamerasrc.cpp >>>>> index 79a025a57..103dfbca2 100644 >>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>>>> @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() >>>>> } >>>>> >>>>> GST_TRACE_OBJECT(src_, "Requesting buffers"); >>>>> - cam_->queueRequest(wrap->request_.get()); >>>>> >>>>> { >>>>> GLibLocker locker(&lock_); >>>>> + cam_->queueRequest(wrap->request_.get()); >>>>> queuedRequests_.push(std::move(wrap)); >>>>> } >>>>> >>>>> -- >>>>> 2.51.0 >>>
Hi, Le vendredi 21 novembre 2025 à 12:24 +0100, Barnabás Pőcze a écrit : > (c) after > * addresses the "early" completion issue of (a) by doing the `queueRequest()` > after the mutex has been unlocked > * not ideal because the list contains std::unique_ptr, so the ownership of the request > is transferred to the container; a raw pointer to the request would be needed [...] > > As a variant of (c), if I understand you correctly, std::shared_ptr could be used in > the container. Is that the suggestion or am I misunderstanding? This is what I initially proposed couple of weeks ago (you simply replied you can't its unique). This is all internal, it was made the most restrictive because when you don't know this is what you should do, but since it no longer applies, it can be changed. Perhaps my take for future libcamera C API, make sure asynchronous operation have a user_data and a destroy function for that user data. This way, we don't have to use lists, and don't endup in this ownership loop. Nicolas
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 79a025a57..103dfbca2 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -214,10 +214,10 @@ int GstLibcameraSrcState::queueRequest() } GST_TRACE_OBJECT(src_, "Requesting buffers"); - cam_->queueRequest(wrap->request_.get()); { GLibLocker locker(&lock_); + cam_->queueRequest(wrap->request_.get()); queuedRequests_.push(std::move(wrap)); }
The moment execution enters `Camera::queueRequest()`, an application must be prepared to received the corresponding `requestCompleted` signal. However, the gstreamer element is currently not prepared: it queues the request, takes a lock, then inserts into a list. If the request completion handler acquires the lock right after the queueing, then it will not find the object in the list that it expects. Even potentially encountering an empty list, running into undefined behaviour when trying to pop from it. Fix that by queueing the request after the lock protected insertion. Bug: https://bugs.libcamera.org/show_bug.cgi?id=238 Fixes: 06bd05beced313 ("gstreamer: Use dedicated lock for request queues") Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- Doing it while holding the lock is not ideal, but the alternative is getting a raw pointer to the `Request` object beforehand, but that does not seem too desirable either. --- src/gstreamer/gstlibcamerasrc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.51.0