[RFC,v1] gstreamer: Be prepared when queueing request

24298 diff mbox series

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

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