[RFC,v1] libcamera: request: Split "Pending" state into two

25280 diff mbox series

The "Pending" state at the moment is used for two very distinct scenarios:
when the application alone is managing the request, and when the request
is being processed. This duality makes it harder to track the true state of
a request and consequently it is harder to diagnose misuses.

So split the "Pending" state into the "Idle" and "InProgress" states to
make it explicit when a camera is managing the request. This improves
the detection of request double queueing, but since the status flag is
not updated atomically, it cannot detect all cases.

The name "Pending" is not kept because any application depending on it
has to be adjusted, so it is better to break the API as well. This is also
an ABI break.

Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 .../internal/tracepoints/request_enums.tp     |  7 ++--
 include/libcamera/request.h                   |  3 +-
 src/libcamera/camera.cpp                      |  2 +-
 src/libcamera/pipeline_handler.cpp            |  7 +++-
 src/libcamera/request.cpp                     | 34 ++++++++++++-------
 src/py/libcamera/py_main.cpp                  |  3 +-
 6 files changed, 36 insertions(+), 20 deletions(-)

On 2025-12-01 18:14, Barnabás Pőcze wrote:
> The "Pending" state at the moment is used for two very distinct scenarios:
> when the application alone is managing the request, and when the request
> is being processed. This duality makes it harder to track the true state of
> a request and consequently it is harder to diagnose misuses.
> 
> So split the "Pending" state into the "Idle" and "InProgress" states to
> make it explicit when a camera is managing the request. This improves
> the detection of request double queueing, but since the status flag is
> not updated atomically, it cannot detect all cases.
> 
> The name "Pending" is not kept because any application depending on it
> has to be adjusted, so it is better to break the API as well. This is also
> an ABI break.
> 
> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  .../internal/tracepoints/request_enums.tp     |  7 ++--
>  include/libcamera/request.h                   |  3 +-
>  src/libcamera/camera.cpp                      |  2 +-
>  src/libcamera/pipeline_handler.cpp            |  7 +++-
>  src/libcamera/request.cpp                     | 34 ++++++++++++-------
>  src/py/libcamera/py_main.cpp                  |  3 +-
>  6 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
> index bcbd1aaa2c..e9a5a78fac 100644
> --- a/include/libcamera/internal/tracepoints/request_enums.tp
> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
>  	libcamera,
>  	request_status,
>  	TP_ENUM_VALUES(
> -		ctf_enum_value("RequestPending", 0)
> -		ctf_enum_value("RequestComplete", 1)
> -		ctf_enum_value("RequestCancelled", 2)
> +		ctf_enum_value("RequestIdle", 0)
> +		ctf_enum_value("RequestInProgress", 1)
> +		ctf_enum_value("RequestComplete", 2)
> +		ctf_enum_value("RequestCancelled", 3)
>  	)
>  )
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 0c5939f7b3..1e7a5ca807 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -32,7 +32,8 @@ class Request : public Extensible
>  
>  public:
>  	enum Status {
> -		RequestPending,
> +		RequestIdle,
> +		RequestInProgress,
>  		RequestComplete,
>  		RequestCancelled,
>  	};
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 2e1e146a25..b6273f6c29 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
>  		return -EXDEV;
>  	}
>  
> -	if (request->status() != Request::RequestPending) {
> +	if (request->status() != Request::RequestIdle) {
>  		LOG(Camera, Error) << request->toString() << " is not valid";
>  		return -EINVAL;
>  	}
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 5c469e5bad..417b90245a 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
>  {
>  	LIBCAMERA_TRACEPOINT(request_queue, request);
>  
> +	ASSERT(request->status() == Request::RequestIdle);
> +
>  	Camera *camera = request->_d()->camera();
>  	Camera::Private *data = camera->_d();
>  	data->waitingRequests_.push(request);
> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>  			break;
>  
>  		Request *request = data->waitingRequests_.front();
> +		ASSERT(request->status() == Request::RequestInProgress);
>  		if (!request->_d()->prepared_)
>  			break;
>  
> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
>  
>  	while (!data->queuedRequests_.empty()) {
>  		Request *req = data->queuedRequests_.front();
> -		if (req->status() == Request::RequestPending)
> +		if (req->status() == Request::RequestInProgress)
>  			break;
>  
> +		ASSERT(req->status() == Request::RequestCancelled ||
> +		       req->status() == Request::RequestComplete);
>  		ASSERT(!req->hasPendingBuffers());
>  		data->queuedRequests_.pop_front();
>  		camera->requestComplete(req);
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 60565f5984..89ad0d7c8d 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -121,7 +121,7 @@ void Request::Private::complete()
>  {
>  	Request *request = _o<Request>();
>  
> -	ASSERT(request->status() == RequestPending);
> +	ASSERT(request->status() == RequestInProgress);
>  	ASSERT(!hasPendingBuffers());
>  
>  	request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> @@ -159,7 +159,7 @@ void Request::Private::cancel()
>  	LIBCAMERA_TRACEPOINT(request_cancel, this);
>  
>  	Request *request = _o<Request>();
> -	ASSERT(request->status() == RequestPending);
> +	ASSERT(request->status() == RequestInProgress);
>  
>  	doCancelRequest();
>  }
> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
>   */
>  void Request::Private::prepare(std::chrono::milliseconds timeout)
>  {
> +	_o<Request>()->status_ = RequestInProgress;
> +

My understanding is that RequestInProgress should be the status as soon
as the queueRequest() return successfully. Otherwise, it is again hard
to differentiate between:
1. RequestIdle (application managing the request)
2. RequestIdle (request queued by application, and is in internal
libcamera waiting queue), what happens if application requeues the same
request here?

Do we need additional status to differentiate between the two?

RequestIdle
RequestQueued
RequestInProgress ?


>  	/* Create and connect one notifier for each synchronization fence. */
>  	for (FrameBuffer *buffer : pending_) {
>  		const Fence *fence = buffer->_d()->fence();
> @@ -309,8 +311,10 @@ void Request::Private::timeout()
>  /**
>   * \enum Request::Status
>   * Request completion status
> - * \var Request::RequestPending
> - * The request hasn't completed yet
> + * \var Request::RequestIdle
> + * The request is idle
> + * \var Request::RequestInProgress
> + * The request is being processed
>   * \var Request::RequestComplete
>   * The request has completed
>   * \var Request::RequestCancelled
> @@ -354,7 +358,7 @@ void Request::Private::timeout()
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
>  	: Extensible(std::make_unique<Private>(camera)),
> -	  cookie_(cookie), status_(RequestPending)
> +	  cookie_(cookie), status_(RequestIdle)
>  {
>  	controls_ = new ControlList(controls::controls,
>  				    camera->_d()->validator());
> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
>  {
>  	LIBCAMERA_TRACEPOINT(request_reuse, this);
>  
> +	ASSERT(status_ == RequestIdle ||
> +	       status_ == RequestComplete ||
> +	       status_ == RequestCancelled);
> +
>  	_d()->reset();
>  
>  	if (flags & ReuseBuffers) {
> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
>  		bufferMap_.clear();
>  	}
>  
> -	status_ = RequestPending;
> +	status_ = RequestIdle;
>  
>  	controls_->clear();
>  	metadata_->clear();
> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
>   * \fn Request::status()
>   * \brief Retrieve the request completion status
>   *
> - * The request status indicates whether the request has completed successfully
> - * or with an error. When requests are created and before they complete the
> - * request status is set to RequestPending, and is updated at completion time
> - * to RequestComplete. If a request is cancelled at capture stop before it has
> - * completed, its status is set to RequestCancelled.
> + * The request status indicates whether the request has completed successfully or with
> + * an error. When requests are created the request status is set to RequestIdle; when
> + * they are queued, the status is set to RequestInProgress. Then it is updated at
> + * completion time to RequestComplete. If a request is cancelled at capture stop before
> + * it has completed, its status is set to RequestCancelled.
>   *
>   * \return The request completion status
>   */
> @@ -607,8 +615,8 @@ std::string Request::toString() const
>   */
>  std::ostream &operator<<(std::ostream &out, const Request &r)
>  {
> -	/* Pending, Completed, Cancelled(X). */
> -	static const char *statuses = "PCX";
> +	/* Idle, InProgress(P), Completed, Cancelled(X). */
> +	static const char statuses[] = "IPCX";
>  
>  	/* Example Output: Request(55:P:1/2:6523524) */
>  	out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index a983ea75c3..d12f00a6f5 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
>  		.def("__str__", &Request::toString);
>  
>  	pyRequestStatus
> -		.value("Pending", Request::RequestPending)
> +		.value("Idle", Request::RequestIdle)
> +		.value("InProgress", Request::RequestInProgress)
>  		.value("Complete", Request::RequestComplete)
>  		.value("Cancelled", Request::RequestCancelled);

2025. 12. 01. 15:42 keltezéssel, Umang Jain írta:
> On 2025-12-01 18:14, Barnabás Pőcze wrote:
>> The "Pending" state at the moment is used for two very distinct scenarios:
>> when the application alone is managing the request, and when the request
>> is being processed. This duality makes it harder to track the true state of
>> a request and consequently it is harder to diagnose misuses.
>>
>> So split the "Pending" state into the "Idle" and "InProgress" states to
>> make it explicit when a camera is managing the request. This improves
>> the detection of request double queueing, but since the status flag is
>> not updated atomically, it cannot detect all cases.
>>
>> The name "Pending" is not kept because any application depending on it
>> has to be adjusted, so it is better to break the API as well. This is also
>> an ABI break.
>>
>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   .../internal/tracepoints/request_enums.tp     |  7 ++--
>>   include/libcamera/request.h                   |  3 +-
>>   src/libcamera/camera.cpp                      |  2 +-
>>   src/libcamera/pipeline_handler.cpp            |  7 +++-
>>   src/libcamera/request.cpp                     | 34 ++++++++++++-------
>>   src/py/libcamera/py_main.cpp                  |  3 +-
>>   6 files changed, 36 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
>> index bcbd1aaa2c..e9a5a78fac 100644
>> --- a/include/libcamera/internal/tracepoints/request_enums.tp
>> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
>> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
>>   	libcamera,
>>   	request_status,
>>   	TP_ENUM_VALUES(
>> -		ctf_enum_value("RequestPending", 0)
>> -		ctf_enum_value("RequestComplete", 1)
>> -		ctf_enum_value("RequestCancelled", 2)
>> +		ctf_enum_value("RequestIdle", 0)
>> +		ctf_enum_value("RequestInProgress", 1)
>> +		ctf_enum_value("RequestComplete", 2)
>> +		ctf_enum_value("RequestCancelled", 3)
>>   	)
>>   )
>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>> index 0c5939f7b3..1e7a5ca807 100644
>> --- a/include/libcamera/request.h
>> +++ b/include/libcamera/request.h
>> @@ -32,7 +32,8 @@ class Request : public Extensible
>>   
>>   public:
>>   	enum Status {
>> -		RequestPending,
>> +		RequestIdle,
>> +		RequestInProgress,
>>   		RequestComplete,
>>   		RequestCancelled,
>>   	};
>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>> index 2e1e146a25..b6273f6c29 100644
>> --- a/src/libcamera/camera.cpp
>> +++ b/src/libcamera/camera.cpp
>> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
>>   		return -EXDEV;
>>   	}
>>   
>> -	if (request->status() != Request::RequestPending) {
>> +	if (request->status() != Request::RequestIdle) {
>>   		LOG(Camera, Error) << request->toString() << " is not valid";
>>   		return -EINVAL;
>>   	}
>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> index 5c469e5bad..417b90245a 100644
>> --- a/src/libcamera/pipeline_handler.cpp
>> +++ b/src/libcamera/pipeline_handler.cpp
>> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
>>   {
>>   	LIBCAMERA_TRACEPOINT(request_queue, request);
>>   
>> +	ASSERT(request->status() == Request::RequestIdle);
>> +
>>   	Camera *camera = request->_d()->camera();
>>   	Camera::Private *data = camera->_d();
>>   	data->waitingRequests_.push(request);
>> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>>   			break;
>>   
>>   		Request *request = data->waitingRequests_.front();
>> +		ASSERT(request->status() == Request::RequestInProgress);
>>   		if (!request->_d()->prepared_)
>>   			break;
>>   
>> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
>>   
>>   	while (!data->queuedRequests_.empty()) {
>>   		Request *req = data->queuedRequests_.front();
>> -		if (req->status() == Request::RequestPending)
>> +		if (req->status() == Request::RequestInProgress)
>>   			break;
>>   
>> +		ASSERT(req->status() == Request::RequestCancelled ||
>> +		       req->status() == Request::RequestComplete);
>>   		ASSERT(!req->hasPendingBuffers());
>>   		data->queuedRequests_.pop_front();
>>   		camera->requestComplete(req);
>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>> index 60565f5984..89ad0d7c8d 100644
>> --- a/src/libcamera/request.cpp
>> +++ b/src/libcamera/request.cpp
>> @@ -121,7 +121,7 @@ void Request::Private::complete()
>>   {
>>   	Request *request = _o<Request>();
>>   
>> -	ASSERT(request->status() == RequestPending);
>> +	ASSERT(request->status() == RequestInProgress);
>>   	ASSERT(!hasPendingBuffers());
>>   
>>   	request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
>> @@ -159,7 +159,7 @@ void Request::Private::cancel()
>>   	LIBCAMERA_TRACEPOINT(request_cancel, this);
>>   
>>   	Request *request = _o<Request>();
>> -	ASSERT(request->status() == RequestPending);
>> +	ASSERT(request->status() == RequestInProgress);
>>   
>>   	doCancelRequest();
>>   }
>> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
>>    */
>>   void Request::Private::prepare(std::chrono::milliseconds timeout)
>>   {
>> +	_o<Request>()->status_ = RequestInProgress;
>> +
> 
> My understanding is that RequestInProgress should be the status as soon
> as the queueRequest() return successfully. Otherwise, it is again hard
> to differentiate between:
> 1. RequestIdle (application managing the request)
> 2. RequestIdle (request queued by application, and is in internal
> libcamera waiting queue), what happens if application requeues the same
> request here?
> 
> Do we need additional status to differentiate between the two?
> 
> RequestIdle
> RequestQueued
> RequestInProgress ?
> 

PipelineHandler::queueRequest() "immediately" calls "prepare()" on the request,
so the state transition will happen "quickly", so this does not seem that problematic
to me at the moment. And we should probably limit the number of "internal" states exposed
in the public interface.

But something more atomic would be better because there is indeed a small time window
between `Camera::queueRequest()` returning and `Request::Private::prepare()` running.
(And this say nothing about two parallel `Camera::queueRequest()` calls.)
`Request::status_` is also not accessed atomically, which is also an issue.

So there are problems, I mostly just wanted to start some kind of discussion about this.

Some options:

   * keep `RequestPending` as is, add an internal `std::atomic_bool queued` to `Request::Private`
     to distinguish the two "pending" states;
   * add "Idle" and "InProgress" states as proposed here, but additionally make `Request::status_` atomic.

In both cases `Camera::queueRequest()` would need to be changed to do an atomic CAS
to check and transition into the new state.


Regards,
Barnabás Pőcze



> 
>>   	/* Create and connect one notifier for each synchronization fence. */
>>   	for (FrameBuffer *buffer : pending_) {
>>   		const Fence *fence = buffer->_d()->fence();
>> @@ -309,8 +311,10 @@ void Request::Private::timeout()
>>   /**
>>    * \enum Request::Status
>>    * Request completion status
>> - * \var Request::RequestPending
>> - * The request hasn't completed yet
>> + * \var Request::RequestIdle
>> + * The request is idle
>> + * \var Request::RequestInProgress
>> + * The request is being processed
>>    * \var Request::RequestComplete
>>    * The request has completed
>>    * \var Request::RequestCancelled
>> @@ -354,7 +358,7 @@ void Request::Private::timeout()
>>    */
>>   Request::Request(Camera *camera, uint64_t cookie)
>>   	: Extensible(std::make_unique<Private>(camera)),
>> -	  cookie_(cookie), status_(RequestPending)
>> +	  cookie_(cookie), status_(RequestIdle)
>>   {
>>   	controls_ = new ControlList(controls::controls,
>>   				    camera->_d()->validator());
>> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
>>   {
>>   	LIBCAMERA_TRACEPOINT(request_reuse, this);
>>   
>> +	ASSERT(status_ == RequestIdle ||
>> +	       status_ == RequestComplete ||
>> +	       status_ == RequestCancelled);
>> +
>>   	_d()->reset();
>>   
>>   	if (flags & ReuseBuffers) {
>> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
>>   		bufferMap_.clear();
>>   	}
>>   
>> -	status_ = RequestPending;
>> +	status_ = RequestIdle;
>>   
>>   	controls_->clear();
>>   	metadata_->clear();
>> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
>>    * \fn Request::status()
>>    * \brief Retrieve the request completion status
>>    *
>> - * The request status indicates whether the request has completed successfully
>> - * or with an error. When requests are created and before they complete the
>> - * request status is set to RequestPending, and is updated at completion time
>> - * to RequestComplete. If a request is cancelled at capture stop before it has
>> - * completed, its status is set to RequestCancelled.
>> + * The request status indicates whether the request has completed successfully or with
>> + * an error. When requests are created the request status is set to RequestIdle; when
>> + * they are queued, the status is set to RequestInProgress. Then it is updated at
>> + * completion time to RequestComplete. If a request is cancelled at capture stop before
>> + * it has completed, its status is set to RequestCancelled.
>>    *
>>    * \return The request completion status
>>    */
>> @@ -607,8 +615,8 @@ std::string Request::toString() const
>>    */
>>   std::ostream &operator<<(std::ostream &out, const Request &r)
>>   {
>> -	/* Pending, Completed, Cancelled(X). */
>> -	static const char *statuses = "PCX";
>> +	/* Idle, InProgress(P), Completed, Cancelled(X). */
>> +	static const char statuses[] = "IPCX";
>>   
>>   	/* Example Output: Request(55:P:1/2:6523524) */
>>   	out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>> index a983ea75c3..d12f00a6f5 100644
>> --- a/src/py/libcamera/py_main.cpp
>> +++ b/src/py/libcamera/py_main.cpp
>> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
>>   		.def("__str__", &Request::toString);
>>   
>>   	pyRequestStatus
>> -		.value("Pending", Request::RequestPending)
>> +		.value("Idle", Request::RequestIdle)
>> +		.value("InProgress", Request::RequestInProgress)
>>   		.value("Complete", Request::RequestComplete)
>>   		.value("Cancelled", Request::RequestCancelled);

On Mon, Dec 01, 2025 at 03:56:39PM +0100, Barnabás Pőcze wrote:
> 2025. 12. 01. 15:42 keltezéssel, Umang Jain írta:
> > On 2025-12-01 18:14, Barnabás Pőcze wrote:
> >> The "Pending" state at the moment is used for two very distinct scenarios:
> >> when the application alone is managing the request, and when the request
> >> is being processed. This duality makes it harder to track the true state of
> >> a request and consequently it is harder to diagnose misuses.
> >>
> >> So split the "Pending" state into the "Idle" and "InProgress" states to
> >> make it explicit when a camera is managing the request. This improves
> >> the detection of request double queueing, but since the status flag is
> >> not updated atomically, it cannot detect all cases.
> >>
> >> The name "Pending" is not kept because any application depending on it
> >> has to be adjusted, so it is better to break the API as well. This is also
> >> an ABI break.
> >>
> >> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   .../internal/tracepoints/request_enums.tp     |  7 ++--
> >>   include/libcamera/request.h                   |  3 +-
> >>   src/libcamera/camera.cpp                      |  2 +-
> >>   src/libcamera/pipeline_handler.cpp            |  7 +++-
> >>   src/libcamera/request.cpp                     | 34 ++++++++++++-------
> >>   src/py/libcamera/py_main.cpp                  |  3 +-
> >>   6 files changed, 36 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
> >> index bcbd1aaa2c..e9a5a78fac 100644
> >> --- a/include/libcamera/internal/tracepoints/request_enums.tp
> >> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
> >> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
> >>   	libcamera,
> >>   	request_status,
> >>   	TP_ENUM_VALUES(
> >> -		ctf_enum_value("RequestPending", 0)
> >> -		ctf_enum_value("RequestComplete", 1)
> >> -		ctf_enum_value("RequestCancelled", 2)
> >> +		ctf_enum_value("RequestIdle", 0)
> >> +		ctf_enum_value("RequestInProgress", 1)
> >> +		ctf_enum_value("RequestComplete", 2)
> >> +		ctf_enum_value("RequestCancelled", 3)
> >>   	)
> >>   )
> >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> >> index 0c5939f7b3..1e7a5ca807 100644
> >> --- a/include/libcamera/request.h
> >> +++ b/include/libcamera/request.h
> >> @@ -32,7 +32,8 @@ class Request : public Extensible
> >>   
> >>   public:
> >>   	enum Status {
> >> -		RequestPending,
> >> +		RequestIdle,
> >> +		RequestInProgress,
> >>   		RequestComplete,
> >>   		RequestCancelled,
> >>   	};
> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> >> index 2e1e146a25..b6273f6c29 100644
> >> --- a/src/libcamera/camera.cpp
> >> +++ b/src/libcamera/camera.cpp
> >> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
> >>   		return -EXDEV;
> >>   	}
> >>   
> >> -	if (request->status() != Request::RequestPending) {
> >> +	if (request->status() != Request::RequestIdle) {
> >>   		LOG(Camera, Error) << request->toString() << " is not valid";
> >>   		return -EINVAL;
> >>   	}
> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> index 5c469e5bad..417b90245a 100644
> >> --- a/src/libcamera/pipeline_handler.cpp
> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
> >>   {
> >>   	LIBCAMERA_TRACEPOINT(request_queue, request);
> >>   
> >> +	ASSERT(request->status() == Request::RequestIdle);
> >> +
> >>   	Camera *camera = request->_d()->camera();
> >>   	Camera::Private *data = camera->_d();
> >>   	data->waitingRequests_.push(request);
> >> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> >>   			break;
> >>   
> >>   		Request *request = data->waitingRequests_.front();
> >> +		ASSERT(request->status() == Request::RequestInProgress);
> >>   		if (!request->_d()->prepared_)
> >>   			break;
> >>   
> >> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
> >>   
> >>   	while (!data->queuedRequests_.empty()) {
> >>   		Request *req = data->queuedRequests_.front();
> >> -		if (req->status() == Request::RequestPending)
> >> +		if (req->status() == Request::RequestInProgress)
> >>   			break;
> >>   
> >> +		ASSERT(req->status() == Request::RequestCancelled ||
> >> +		       req->status() == Request::RequestComplete);
> >>   		ASSERT(!req->hasPendingBuffers());
> >>   		data->queuedRequests_.pop_front();
> >>   		camera->requestComplete(req);
> >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> >> index 60565f5984..89ad0d7c8d 100644
> >> --- a/src/libcamera/request.cpp
> >> +++ b/src/libcamera/request.cpp
> >> @@ -121,7 +121,7 @@ void Request::Private::complete()
> >>   {
> >>   	Request *request = _o<Request>();
> >>   
> >> -	ASSERT(request->status() == RequestPending);
> >> +	ASSERT(request->status() == RequestInProgress);
> >>   	ASSERT(!hasPendingBuffers());
> >>   
> >>   	request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> >> @@ -159,7 +159,7 @@ void Request::Private::cancel()
> >>   	LIBCAMERA_TRACEPOINT(request_cancel, this);
> >>   
> >>   	Request *request = _o<Request>();
> >> -	ASSERT(request->status() == RequestPending);
> >> +	ASSERT(request->status() == RequestInProgress);
> >>   
> >>   	doCancelRequest();
> >>   }
> >> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
> >>    */
> >>   void Request::Private::prepare(std::chrono::milliseconds timeout)
> >>   {
> >> +	_o<Request>()->status_ = RequestInProgress;
> >> +
> > 
> > My understanding is that RequestInProgress should be the status as soon
> > as the queueRequest() return successfully. Otherwise, it is again hard
> > to differentiate between:
> > 1. RequestIdle (application managing the request)
> > 2. RequestIdle (request queued by application, and is in internal
> > libcamera waiting queue), what happens if application requeues the same
> > request here?
> > 
> > Do we need additional status to differentiate between the two?
> > 
> > RequestIdle
> > RequestQueued
> > RequestInProgress ?
> 
> PipelineHandler::queueRequest() "immediately" calls "prepare()" on the request,
> so the state transition will happen "quickly", so this does not seem that problematic
> to me at the moment. And we should probably limit the number of "internal" states exposed
> in the public interface.
> 
> But something more atomic would be better because there is indeed a small time window
> between `Camera::queueRequest()` returning and `Request::Private::prepare()` running.
> (And this say nothing about two parallel `Camera::queueRequest()` calls.)
> `Request::status_` is also not accessed atomically, which is also an issue.
> 
> So there are problems, I mostly just wanted to start some kind of discussion about this.
> 
> Some options:
> 
>    * keep `RequestPending` as is, add an internal `std::atomic_bool queued` to `Request::Private`
>      to distinguish the two "pending" states;
>    * add "Idle" and "InProgress" states as proposed here, but additionally make `Request::status_` atomic.

Feel free to explore splitting the state to only expose to applications
the information they need, and keeping the rest internal. The current
states already feel a bit dirty, with RequestComplete and
RequestCancelled both indicating completed requests, so there could be
room for improvement (I'm not entirely certain alternatives would be
clearly better though).

> In both cases `Camera::queueRequest()` would need to be changed to do an atomic CAS
> to check and transition into the new state.

I don't think you need to spend too much effort trying to prevent two
concurrent queueRequest() calls using the same request. If it's really
easy to do I won't object, but it's a clear application bug that I would
personally consider as deserving as much empathy as
queueRequest(reinterpret_cast<Request *>(0xdeadbeef));

> >>   	/* Create and connect one notifier for each synchronization fence. */
> >>   	for (FrameBuffer *buffer : pending_) {
> >>   		const Fence *fence = buffer->_d()->fence();
> >> @@ -309,8 +311,10 @@ void Request::Private::timeout()
> >>   /**
> >>    * \enum Request::Status
> >>    * Request completion status
> >> - * \var Request::RequestPending
> >> - * The request hasn't completed yet
> >> + * \var Request::RequestIdle
> >> + * The request is idle
> >> + * \var Request::RequestInProgress
> >> + * The request is being processed
> >>    * \var Request::RequestComplete
> >>    * The request has completed
> >>    * \var Request::RequestCancelled
> >> @@ -354,7 +358,7 @@ void Request::Private::timeout()
> >>    */
> >>   Request::Request(Camera *camera, uint64_t cookie)
> >>   	: Extensible(std::make_unique<Private>(camera)),
> >> -	  cookie_(cookie), status_(RequestPending)
> >> +	  cookie_(cookie), status_(RequestIdle)
> >>   {
> >>   	controls_ = new ControlList(controls::controls,
> >>   				    camera->_d()->validator());
> >> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
> >>   {
> >>   	LIBCAMERA_TRACEPOINT(request_reuse, this);
> >>   
> >> +	ASSERT(status_ == RequestIdle ||
> >> +	       status_ == RequestComplete ||
> >> +	       status_ == RequestCancelled);
> >> +
> >>   	_d()->reset();
> >>   
> >>   	if (flags & ReuseBuffers) {
> >> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
> >>   		bufferMap_.clear();
> >>   	}
> >>   
> >> -	status_ = RequestPending;
> >> +	status_ = RequestIdle;
> >>   
> >>   	controls_->clear();
> >>   	metadata_->clear();
> >> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
> >>    * \fn Request::status()
> >>    * \brief Retrieve the request completion status
> >>    *
> >> - * The request status indicates whether the request has completed successfully
> >> - * or with an error. When requests are created and before they complete the
> >> - * request status is set to RequestPending, and is updated at completion time
> >> - * to RequestComplete. If a request is cancelled at capture stop before it has
> >> - * completed, its status is set to RequestCancelled.
> >> + * The request status indicates whether the request has completed successfully or with
> >> + * an error. When requests are created the request status is set to RequestIdle; when
> >> + * they are queued, the status is set to RequestInProgress. Then it is updated at
> >> + * completion time to RequestComplete. If a request is cancelled at capture stop before
> >> + * it has completed, its status is set to RequestCancelled.
> >>    *
> >>    * \return The request completion status
> >>    */
> >> @@ -607,8 +615,8 @@ std::string Request::toString() const
> >>    */
> >>   std::ostream &operator<<(std::ostream &out, const Request &r)
> >>   {
> >> -	/* Pending, Completed, Cancelled(X). */
> >> -	static const char *statuses = "PCX";
> >> +	/* Idle, InProgress(P), Completed, Cancelled(X). */
> >> +	static const char statuses[] = "IPCX";
> >>   
> >>   	/* Example Output: Request(55:P:1/2:6523524) */
> >>   	out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
> >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> >> index a983ea75c3..d12f00a6f5 100644
> >> --- a/src/py/libcamera/py_main.cpp
> >> +++ b/src/py/libcamera/py_main.cpp
> >> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
> >>   		.def("__str__", &Request::toString);
> >>   
> >>   	pyRequestStatus
> >> -		.value("Pending", Request::RequestPending)
> >> +		.value("Idle", Request::RequestIdle)
> >> +		.value("InProgress", Request::RequestInProgress)
> >>   		.value("Complete", Request::RequestComplete)
> >>   		.value("Cancelled", Request::RequestCancelled);

2025. 12. 01. 16:34 keltezéssel, Laurent Pinchart írta:
> On Mon, Dec 01, 2025 at 03:56:39PM +0100, Barnabás Pőcze wrote:
>> 2025. 12. 01. 15:42 keltezéssel, Umang Jain írta:
>>> On 2025-12-01 18:14, Barnabás Pőcze wrote:
>>>> The "Pending" state at the moment is used for two very distinct scenarios:
>>>> when the application alone is managing the request, and when the request
>>>> is being processed. This duality makes it harder to track the true state of
>>>> a request and consequently it is harder to diagnose misuses.
>>>>
>>>> So split the "Pending" state into the "Idle" and "InProgress" states to
>>>> make it explicit when a camera is managing the request. This improves
>>>> the detection of request double queueing, but since the status flag is
>>>> not updated atomically, it cannot detect all cases.
>>>>
>>>> The name "Pending" is not kept because any application depending on it
>>>> has to be adjusted, so it is better to break the API as well. This is also
>>>> an ABI break.
>>>>
>>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    .../internal/tracepoints/request_enums.tp     |  7 ++--
>>>>    include/libcamera/request.h                   |  3 +-
>>>>    src/libcamera/camera.cpp                      |  2 +-
>>>>    src/libcamera/pipeline_handler.cpp            |  7 +++-
>>>>    src/libcamera/request.cpp                     | 34 ++++++++++++-------
>>>>    src/py/libcamera/py_main.cpp                  |  3 +-
>>>>    6 files changed, 36 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
>>>> index bcbd1aaa2c..e9a5a78fac 100644
>>>> --- a/include/libcamera/internal/tracepoints/request_enums.tp
>>>> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
>>>> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
>>>>    	libcamera,
>>>>    	request_status,
>>>>    	TP_ENUM_VALUES(
>>>> -		ctf_enum_value("RequestPending", 0)
>>>> -		ctf_enum_value("RequestComplete", 1)
>>>> -		ctf_enum_value("RequestCancelled", 2)
>>>> +		ctf_enum_value("RequestIdle", 0)
>>>> +		ctf_enum_value("RequestInProgress", 1)
>>>> +		ctf_enum_value("RequestComplete", 2)
>>>> +		ctf_enum_value("RequestCancelled", 3)
>>>>    	)
>>>>    )
>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>>>> index 0c5939f7b3..1e7a5ca807 100644
>>>> --- a/include/libcamera/request.h
>>>> +++ b/include/libcamera/request.h
>>>> @@ -32,7 +32,8 @@ class Request : public Extensible
>>>>    
>>>>    public:
>>>>    	enum Status {
>>>> -		RequestPending,
>>>> +		RequestIdle,
>>>> +		RequestInProgress,
>>>>    		RequestComplete,
>>>>    		RequestCancelled,
>>>>    	};
>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>>> index 2e1e146a25..b6273f6c29 100644
>>>> --- a/src/libcamera/camera.cpp
>>>> +++ b/src/libcamera/camera.cpp
>>>> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
>>>>    		return -EXDEV;
>>>>    	}
>>>>    
>>>> -	if (request->status() != Request::RequestPending) {
>>>> +	if (request->status() != Request::RequestIdle) {
>>>>    		LOG(Camera, Error) << request->toString() << " is not valid";
>>>>    		return -EINVAL;
>>>>    	}
>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>>> index 5c469e5bad..417b90245a 100644
>>>> --- a/src/libcamera/pipeline_handler.cpp
>>>> +++ b/src/libcamera/pipeline_handler.cpp
>>>> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
>>>>    {
>>>>    	LIBCAMERA_TRACEPOINT(request_queue, request);
>>>>    
>>>> +	ASSERT(request->status() == Request::RequestIdle);
>>>> +
>>>>    	Camera *camera = request->_d()->camera();
>>>>    	Camera::Private *data = camera->_d();
>>>>    	data->waitingRequests_.push(request);
>>>> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>>>>    			break;
>>>>    
>>>>    		Request *request = data->waitingRequests_.front();
>>>> +		ASSERT(request->status() == Request::RequestInProgress);
>>>>    		if (!request->_d()->prepared_)
>>>>    			break;
>>>>    
>>>> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
>>>>    
>>>>    	while (!data->queuedRequests_.empty()) {
>>>>    		Request *req = data->queuedRequests_.front();
>>>> -		if (req->status() == Request::RequestPending)
>>>> +		if (req->status() == Request::RequestInProgress)
>>>>    			break;
>>>>    
>>>> +		ASSERT(req->status() == Request::RequestCancelled ||
>>>> +		       req->status() == Request::RequestComplete);
>>>>    		ASSERT(!req->hasPendingBuffers());
>>>>    		data->queuedRequests_.pop_front();
>>>>    		camera->requestComplete(req);
>>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>>>> index 60565f5984..89ad0d7c8d 100644
>>>> --- a/src/libcamera/request.cpp
>>>> +++ b/src/libcamera/request.cpp
>>>> @@ -121,7 +121,7 @@ void Request::Private::complete()
>>>>    {
>>>>    	Request *request = _o<Request>();
>>>>    
>>>> -	ASSERT(request->status() == RequestPending);
>>>> +	ASSERT(request->status() == RequestInProgress);
>>>>    	ASSERT(!hasPendingBuffers());
>>>>    
>>>>    	request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
>>>> @@ -159,7 +159,7 @@ void Request::Private::cancel()
>>>>    	LIBCAMERA_TRACEPOINT(request_cancel, this);
>>>>    
>>>>    	Request *request = _o<Request>();
>>>> -	ASSERT(request->status() == RequestPending);
>>>> +	ASSERT(request->status() == RequestInProgress);
>>>>    
>>>>    	doCancelRequest();
>>>>    }
>>>> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
>>>>     */
>>>>    void Request::Private::prepare(std::chrono::milliseconds timeout)
>>>>    {
>>>> +	_o<Request>()->status_ = RequestInProgress;
>>>> +
>>>
>>> My understanding is that RequestInProgress should be the status as soon
>>> as the queueRequest() return successfully. Otherwise, it is again hard
>>> to differentiate between:
>>> 1. RequestIdle (application managing the request)
>>> 2. RequestIdle (request queued by application, and is in internal
>>> libcamera waiting queue), what happens if application requeues the same
>>> request here?
>>>
>>> Do we need additional status to differentiate between the two?
>>>
>>> RequestIdle
>>> RequestQueued
>>> RequestInProgress ?
>>
>> PipelineHandler::queueRequest() "immediately" calls "prepare()" on the request,
>> so the state transition will happen "quickly", so this does not seem that problematic
>> to me at the moment. And we should probably limit the number of "internal" states exposed
>> in the public interface.
>>
>> But something more atomic would be better because there is indeed a small time window
>> between `Camera::queueRequest()` returning and `Request::Private::prepare()` running.
>> (And this say nothing about two parallel `Camera::queueRequest()` calls.)
>> `Request::status_` is also not accessed atomically, which is also an issue.
>>
>> So there are problems, I mostly just wanted to start some kind of discussion about this.
>>
>> Some options:
>>
>>     * keep `RequestPending` as is, add an internal `std::atomic_bool queued` to `Request::Private`
>>       to distinguish the two "pending" states;
>>     * add "Idle" and "InProgress" states as proposed here, but additionally make `Request::status_` atomic.
> 
> Feel free to explore splitting the state to only expose to applications
> the information they need, and keeping the rest internal. The current
> states already feel a bit dirty, with RequestComplete and
> RequestCancelled both indicating completed requests, so there could be
> room for improvement (I'm not entirely certain alternatives would be
> clearly better though).
> 
>> In both cases `Camera::queueRequest()` would need to be changed to do an atomic CAS
>> to check and transition into the new state.
> 
> I don't think you need to spend too much effort trying to prevent two
> concurrent queueRequest() calls using the same request. If it's really
> easy to do I won't object, but it's a clear application bug that I would
> personally consider as deserving as much empathy as
> queueRequest(reinterpret_cast<Request *>(0xdeadbeef));

Hmm, maybe it's because I myself have run into this issue, but I
feel quite differently about this. I think this should be robustly
diagnosed.


> 
>>>>    	/* Create and connect one notifier for each synchronization fence. */
>>>>    	for (FrameBuffer *buffer : pending_) {
>>>>    		const Fence *fence = buffer->_d()->fence();
>>>> @@ -309,8 +311,10 @@ void Request::Private::timeout()
>>>>    /**
>>>>     * \enum Request::Status
>>>>     * Request completion status
>>>> - * \var Request::RequestPending
>>>> - * The request hasn't completed yet
>>>> + * \var Request::RequestIdle
>>>> + * The request is idle
>>>> + * \var Request::RequestInProgress
>>>> + * The request is being processed
>>>>     * \var Request::RequestComplete
>>>>     * The request has completed
>>>>     * \var Request::RequestCancelled
>>>> @@ -354,7 +358,7 @@ void Request::Private::timeout()
>>>>     */
>>>>    Request::Request(Camera *camera, uint64_t cookie)
>>>>    	: Extensible(std::make_unique<Private>(camera)),
>>>> -	  cookie_(cookie), status_(RequestPending)
>>>> +	  cookie_(cookie), status_(RequestIdle)
>>>>    {
>>>>    	controls_ = new ControlList(controls::controls,
>>>>    				    camera->_d()->validator());
>>>> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
>>>>    {
>>>>    	LIBCAMERA_TRACEPOINT(request_reuse, this);
>>>>    
>>>> +	ASSERT(status_ == RequestIdle ||
>>>> +	       status_ == RequestComplete ||
>>>> +	       status_ == RequestCancelled);
>>>> +
>>>>    	_d()->reset();
>>>>    
>>>>    	if (flags & ReuseBuffers) {
>>>> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
>>>>    		bufferMap_.clear();
>>>>    	}
>>>>    
>>>> -	status_ = RequestPending;
>>>> +	status_ = RequestIdle;
>>>>    
>>>>    	controls_->clear();
>>>>    	metadata_->clear();
>>>> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
>>>>     * \fn Request::status()
>>>>     * \brief Retrieve the request completion status
>>>>     *
>>>> - * The request status indicates whether the request has completed successfully
>>>> - * or with an error. When requests are created and before they complete the
>>>> - * request status is set to RequestPending, and is updated at completion time
>>>> - * to RequestComplete. If a request is cancelled at capture stop before it has
>>>> - * completed, its status is set to RequestCancelled.
>>>> + * The request status indicates whether the request has completed successfully or with
>>>> + * an error. When requests are created the request status is set to RequestIdle; when
>>>> + * they are queued, the status is set to RequestInProgress. Then it is updated at
>>>> + * completion time to RequestComplete. If a request is cancelled at capture stop before
>>>> + * it has completed, its status is set to RequestCancelled.
>>>>     *
>>>>     * \return The request completion status
>>>>     */
>>>> @@ -607,8 +615,8 @@ std::string Request::toString() const
>>>>     */
>>>>    std::ostream &operator<<(std::ostream &out, const Request &r)
>>>>    {
>>>> -	/* Pending, Completed, Cancelled(X). */
>>>> -	static const char *statuses = "PCX";
>>>> +	/* Idle, InProgress(P), Completed, Cancelled(X). */
>>>> +	static const char statuses[] = "IPCX";
>>>>    
>>>>    	/* Example Output: Request(55:P:1/2:6523524) */
>>>>    	out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
>>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>>>> index a983ea75c3..d12f00a6f5 100644
>>>> --- a/src/py/libcamera/py_main.cpp
>>>> +++ b/src/py/libcamera/py_main.cpp
>>>> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
>>>>    		.def("__str__", &Request::toString);
>>>>    
>>>>    	pyRequestStatus
>>>> -		.value("Pending", Request::RequestPending)
>>>> +		.value("Idle", Request::RequestIdle)
>>>> +		.value("InProgress", Request::RequestInProgress)
>>>>    		.value("Complete", Request::RequestComplete)
>>>>    		.value("Cancelled", Request::RequestCancelled);
>

Quoting Barnabás Pőcze (2025-12-02 12:26:51)
> 2025. 12. 01. 16:34 keltezéssel, Laurent Pinchart írta:
> > On Mon, Dec 01, 2025 at 03:56:39PM +0100, Barnabás Pőcze wrote:
> >> 2025. 12. 01. 15:42 keltezéssel, Umang Jain írta:
> >>> On 2025-12-01 18:14, Barnabás Pőcze wrote:
> >>>> The "Pending" state at the moment is used for two very distinct scenarios:
> >>>> when the application alone is managing the request, and when the request
> >>>> is being processed. This duality makes it harder to track the true state of
> >>>> a request and consequently it is harder to diagnose misuses.
> >>>>
> >>>> So split the "Pending" state into the "Idle" and "InProgress" states to
> >>>> make it explicit when a camera is managing the request. This improves
> >>>> the detection of request double queueing, but since the status flag is
> >>>> not updated atomically, it cannot detect all cases.
> >>>>
> >>>> The name "Pending" is not kept because any application depending on it
> >>>> has to be adjusted, so it is better to break the API as well. This is also
> >>>> an ABI break.
> >>>>
> >>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>> ---
> >>>>    .../internal/tracepoints/request_enums.tp     |  7 ++--
> >>>>    include/libcamera/request.h                   |  3 +-
> >>>>    src/libcamera/camera.cpp                      |  2 +-
> >>>>    src/libcamera/pipeline_handler.cpp            |  7 +++-
> >>>>    src/libcamera/request.cpp                     | 34 ++++++++++++-------
> >>>>    src/py/libcamera/py_main.cpp                  |  3 +-
> >>>>    6 files changed, 36 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
> >>>> index bcbd1aaa2c..e9a5a78fac 100644
> >>>> --- a/include/libcamera/internal/tracepoints/request_enums.tp
> >>>> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
> >>>> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
> >>>>            libcamera,
> >>>>            request_status,
> >>>>            TP_ENUM_VALUES(
> >>>> -          ctf_enum_value("RequestPending", 0)
> >>>> -          ctf_enum_value("RequestComplete", 1)
> >>>> -          ctf_enum_value("RequestCancelled", 2)
> >>>> +          ctf_enum_value("RequestIdle", 0)
> >>>> +          ctf_enum_value("RequestInProgress", 1)
> >>>> +          ctf_enum_value("RequestComplete", 2)
> >>>> +          ctf_enum_value("RequestCancelled", 3)
> >>>>            )
> >>>>    )
> >>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> >>>> index 0c5939f7b3..1e7a5ca807 100644
> >>>> --- a/include/libcamera/request.h
> >>>> +++ b/include/libcamera/request.h
> >>>> @@ -32,7 +32,8 @@ class Request : public Extensible
> >>>>    
> >>>>    public:
> >>>>            enum Status {
> >>>> -          RequestPending,
> >>>> +          RequestIdle,
> >>>> +          RequestInProgress,
> >>>>                    RequestComplete,
> >>>>                    RequestCancelled,
> >>>>            };
> >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> >>>> index 2e1e146a25..b6273f6c29 100644
> >>>> --- a/src/libcamera/camera.cpp
> >>>> +++ b/src/libcamera/camera.cpp
> >>>> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
> >>>>                    return -EXDEV;
> >>>>            }
> >>>>    
> >>>> -  if (request->status() != Request::RequestPending) {
> >>>> +  if (request->status() != Request::RequestIdle) {
> >>>>                    LOG(Camera, Error) << request->toString() << " is not valid";
> >>>>                    return -EINVAL;
> >>>>            }
> >>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >>>> index 5c469e5bad..417b90245a 100644
> >>>> --- a/src/libcamera/pipeline_handler.cpp
> >>>> +++ b/src/libcamera/pipeline_handler.cpp
> >>>> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
> >>>>    {
> >>>>            LIBCAMERA_TRACEPOINT(request_queue, request);
> >>>>    
> >>>> +  ASSERT(request->status() == Request::RequestIdle);
> >>>> +
> >>>>            Camera *camera = request->_d()->camera();
> >>>>            Camera::Private *data = camera->_d();
> >>>>            data->waitingRequests_.push(request);
> >>>> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> >>>>                            break;
> >>>>    
> >>>>                    Request *request = data->waitingRequests_.front();
> >>>> +          ASSERT(request->status() == Request::RequestInProgress);
> >>>>                    if (!request->_d()->prepared_)
> >>>>                            break;
> >>>>    
> >>>> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
> >>>>    
> >>>>            while (!data->queuedRequests_.empty()) {
> >>>>                    Request *req = data->queuedRequests_.front();
> >>>> -          if (req->status() == Request::RequestPending)
> >>>> +          if (req->status() == Request::RequestInProgress)
> >>>>                            break;
> >>>>    
> >>>> +          ASSERT(req->status() == Request::RequestCancelled ||
> >>>> +                 req->status() == Request::RequestComplete);
> >>>>                    ASSERT(!req->hasPendingBuffers());
> >>>>                    data->queuedRequests_.pop_front();
> >>>>                    camera->requestComplete(req);
> >>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> >>>> index 60565f5984..89ad0d7c8d 100644
> >>>> --- a/src/libcamera/request.cpp
> >>>> +++ b/src/libcamera/request.cpp
> >>>> @@ -121,7 +121,7 @@ void Request::Private::complete()
> >>>>    {
> >>>>            Request *request = _o<Request>();
> >>>>    
> >>>> -  ASSERT(request->status() == RequestPending);
> >>>> +  ASSERT(request->status() == RequestInProgress);
> >>>>            ASSERT(!hasPendingBuffers());
> >>>>    
> >>>>            request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> >>>> @@ -159,7 +159,7 @@ void Request::Private::cancel()
> >>>>            LIBCAMERA_TRACEPOINT(request_cancel, this);
> >>>>    
> >>>>            Request *request = _o<Request>();
> >>>> -  ASSERT(request->status() == RequestPending);
> >>>> +  ASSERT(request->status() == RequestInProgress);
> >>>>    
> >>>>            doCancelRequest();
> >>>>    }
> >>>> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
> >>>>     */
> >>>>    void Request::Private::prepare(std::chrono::milliseconds timeout)
> >>>>    {
> >>>> +  _o<Request>()->status_ = RequestInProgress;
> >>>> +
> >>>
> >>> My understanding is that RequestInProgress should be the status as soon
> >>> as the queueRequest() return successfully. Otherwise, it is again hard
> >>> to differentiate between:
> >>> 1. RequestIdle (application managing the request)
> >>> 2. RequestIdle (request queued by application, and is in internal
> >>> libcamera waiting queue), what happens if application requeues the same
> >>> request here?
> >>>
> >>> Do we need additional status to differentiate between the two?
> >>>
> >>> RequestIdle
> >>> RequestQueued
> >>> RequestInProgress ?
> >>
> >> PipelineHandler::queueRequest() "immediately" calls "prepare()" on the request,
> >> so the state transition will happen "quickly", so this does not seem that problematic
> >> to me at the moment. And we should probably limit the number of "internal" states exposed
> >> in the public interface.
> >>
> >> But something more atomic would be better because there is indeed a small time window
> >> between `Camera::queueRequest()` returning and `Request::Private::prepare()` running.
> >> (And this say nothing about two parallel `Camera::queueRequest()` calls.)
> >> `Request::status_` is also not accessed atomically, which is also an issue.
> >>
> >> So there are problems, I mostly just wanted to start some kind of discussion about this.
> >>
> >> Some options:
> >>
> >>     * keep `RequestPending` as is, add an internal `std::atomic_bool queued` to `Request::Private`
> >>       to distinguish the two "pending" states;
> >>     * add "Idle" and "InProgress" states as proposed here, but additionally make `Request::status_` atomic.
> > 
> > Feel free to explore splitting the state to only expose to applications
> > the information they need, and keeping the rest internal. The current
> > states already feel a bit dirty, with RequestComplete and
> > RequestCancelled both indicating completed requests, so there could be
> > room for improvement (I'm not entirely certain alternatives would be
> > clearly better though).
> > 
> >> In both cases `Camera::queueRequest()` would need to be changed to do an atomic CAS
> >> to check and transition into the new state.
> > 
> > I don't think you need to spend too much effort trying to prevent two
> > concurrent queueRequest() calls using the same request. If it's really
> > easy to do I won't object, but it's a clear application bug that I would
> > personally consider as deserving as much empathy as
> > queueRequest(reinterpret_cast<Request *>(0xdeadbeef));
> 
> Hmm, maybe it's because I myself have run into this issue, but I
> feel quite differently about this. I think this should be robustly
> diagnosed.

I'm also going to be vocally on the side of *We should do better to
support applications here*.

This reminds me of the Request Canary I proposed
https://patchwork.libcamera.org/patch/18226/ - Which I /still/ put on
customer branches and it *has* caught and prevented bugs in those
developments.


Yes, applications can do bad things, and they can double free or
introduce use after free errors. And in C scope - perhaps those can
easily be detected with asan or such.

Here /we/ are the framework - and /we/ provide the interfaces. 

If we put the shotgun in the hands of the developer, when they point it
at their feet, I think we have a responsibilty to tell them what will
happen when they pull the trigger.

--
Kieran



> >>>>            /* Create and connect one notifier for each synchronization fence. */
> >>>>            for (FrameBuffer *buffer : pending_) {
> >>>>                    const Fence *fence = buffer->_d()->fence();
> >>>> @@ -309,8 +311,10 @@ void Request::Private::timeout()
> >>>>    /**
> >>>>     * \enum Request::Status
> >>>>     * Request completion status
> >>>> - * \var Request::RequestPending
> >>>> - * The request hasn't completed yet
> >>>> + * \var Request::RequestIdle
> >>>> + * The request is idle
> >>>> + * \var Request::RequestInProgress
> >>>> + * The request is being processed
> >>>>     * \var Request::RequestComplete
> >>>>     * The request has completed
> >>>>     * \var Request::RequestCancelled
> >>>> @@ -354,7 +358,7 @@ void Request::Private::timeout()
> >>>>     */
> >>>>    Request::Request(Camera *camera, uint64_t cookie)
> >>>>            : Extensible(std::make_unique<Private>(camera)),
> >>>> -    cookie_(cookie), status_(RequestPending)
> >>>> +    cookie_(cookie), status_(RequestIdle)
> >>>>    {
> >>>>            controls_ = new ControlList(controls::controls,
> >>>>                                        camera->_d()->validator());
> >>>> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
> >>>>    {
> >>>>            LIBCAMERA_TRACEPOINT(request_reuse, this);
> >>>>    
> >>>> +  ASSERT(status_ == RequestIdle ||
> >>>> +         status_ == RequestComplete ||
> >>>> +         status_ == RequestCancelled);
> >>>> +
> >>>>            _d()->reset();
> >>>>    
> >>>>            if (flags & ReuseBuffers) {
> >>>> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
> >>>>                    bufferMap_.clear();
> >>>>            }
> >>>>    
> >>>> -  status_ = RequestPending;
> >>>> +  status_ = RequestIdle;
> >>>>    
> >>>>            controls_->clear();
> >>>>            metadata_->clear();
> >>>> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
> >>>>     * \fn Request::status()
> >>>>     * \brief Retrieve the request completion status
> >>>>     *
> >>>> - * The request status indicates whether the request has completed successfully
> >>>> - * or with an error. When requests are created and before they complete the
> >>>> - * request status is set to RequestPending, and is updated at completion time
> >>>> - * to RequestComplete. If a request is cancelled at capture stop before it has
> >>>> - * completed, its status is set to RequestCancelled.
> >>>> + * The request status indicates whether the request has completed successfully or with
> >>>> + * an error. When requests are created the request status is set to RequestIdle; when
> >>>> + * they are queued, the status is set to RequestInProgress. Then it is updated at
> >>>> + * completion time to RequestComplete. If a request is cancelled at capture stop before
> >>>> + * it has completed, its status is set to RequestCancelled.
> >>>>     *
> >>>>     * \return The request completion status
> >>>>     */
> >>>> @@ -607,8 +615,8 @@ std::string Request::toString() const
> >>>>     */
> >>>>    std::ostream &operator<<(std::ostream &out, const Request &r)
> >>>>    {
> >>>> -  /* Pending, Completed, Cancelled(X). */
> >>>> -  static const char *statuses = "PCX";
> >>>> +  /* Idle, InProgress(P), Completed, Cancelled(X). */
> >>>> +  static const char statuses[] = "IPCX";
> >>>>    
> >>>>            /* Example Output: Request(55:P:1/2:6523524) */
> >>>>            out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
> >>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> >>>> index a983ea75c3..d12f00a6f5 100644
> >>>> --- a/src/py/libcamera/py_main.cpp
> >>>> +++ b/src/py/libcamera/py_main.cpp
> >>>> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
> >>>>                    .def("__str__", &Request::toString);
> >>>>    
> >>>>            pyRequestStatus
> >>>> -          .value("Pending", Request::RequestPending)
> >>>> +          .value("Idle", Request::RequestIdle)
> >>>> +          .value("InProgress", Request::RequestInProgress)
> >>>>                    .value("Complete", Request::RequestComplete)
> >>>>                    .value("Cancelled", Request::RequestCancelled);
> > 
>

On Tue, Dec 02, 2025 at 12:55:05PM +0000, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2025-12-02 12:26:51)
> > 2025. 12. 01. 16:34 keltezéssel, Laurent Pinchart írta:
> > > On Mon, Dec 01, 2025 at 03:56:39PM +0100, Barnabás Pőcze wrote:
> > >> 2025. 12. 01. 15:42 keltezéssel, Umang Jain írta:
> > >>> On 2025-12-01 18:14, Barnabás Pőcze wrote:
> > >>>> The "Pending" state at the moment is used for two very distinct scenarios:
> > >>>> when the application alone is managing the request, and when the request
> > >>>> is being processed. This duality makes it harder to track the true state of
> > >>>> a request and consequently it is harder to diagnose misuses.
> > >>>>
> > >>>> So split the "Pending" state into the "Idle" and "InProgress" states to
> > >>>> make it explicit when a camera is managing the request. This improves
> > >>>> the detection of request double queueing, but since the status flag is
> > >>>> not updated atomically, it cannot detect all cases.
> > >>>>
> > >>>> The name "Pending" is not kept because any application depending on it
> > >>>> has to be adjusted, so it is better to break the API as well. This is also
> > >>>> an ABI break.
> > >>>>
> > >>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
> > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > >>>> ---
> > >>>>    .../internal/tracepoints/request_enums.tp     |  7 ++--
> > >>>>    include/libcamera/request.h                   |  3 +-
> > >>>>    src/libcamera/camera.cpp                      |  2 +-
> > >>>>    src/libcamera/pipeline_handler.cpp            |  7 +++-
> > >>>>    src/libcamera/request.cpp                     | 34 ++++++++++++-------
> > >>>>    src/py/libcamera/py_main.cpp                  |  3 +-
> > >>>>    6 files changed, 36 insertions(+), 20 deletions(-)
> > >>>>
> > >>>> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
> > >>>> index bcbd1aaa2c..e9a5a78fac 100644
> > >>>> --- a/include/libcamera/internal/tracepoints/request_enums.tp
> > >>>> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
> > >>>> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
> > >>>>            libcamera,
> > >>>>            request_status,
> > >>>>            TP_ENUM_VALUES(
> > >>>> -          ctf_enum_value("RequestPending", 0)
> > >>>> -          ctf_enum_value("RequestComplete", 1)
> > >>>> -          ctf_enum_value("RequestCancelled", 2)
> > >>>> +          ctf_enum_value("RequestIdle", 0)
> > >>>> +          ctf_enum_value("RequestInProgress", 1)
> > >>>> +          ctf_enum_value("RequestComplete", 2)
> > >>>> +          ctf_enum_value("RequestCancelled", 3)
> > >>>>            )
> > >>>>    )
> > >>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > >>>> index 0c5939f7b3..1e7a5ca807 100644
> > >>>> --- a/include/libcamera/request.h
> > >>>> +++ b/include/libcamera/request.h
> > >>>> @@ -32,7 +32,8 @@ class Request : public Extensible
> > >>>>    
> > >>>>    public:
> > >>>>            enum Status {
> > >>>> -          RequestPending,
> > >>>> +          RequestIdle,
> > >>>> +          RequestInProgress,
> > >>>>                    RequestComplete,
> > >>>>                    RequestCancelled,
> > >>>>            };
> > >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > >>>> index 2e1e146a25..b6273f6c29 100644
> > >>>> --- a/src/libcamera/camera.cpp
> > >>>> +++ b/src/libcamera/camera.cpp
> > >>>> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
> > >>>>                    return -EXDEV;
> > >>>>            }
> > >>>>    
> > >>>> -  if (request->status() != Request::RequestPending) {
> > >>>> +  if (request->status() != Request::RequestIdle) {
> > >>>>                    LOG(Camera, Error) << request->toString() << " is not valid";
> > >>>>                    return -EINVAL;
> > >>>>            }
> > >>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > >>>> index 5c469e5bad..417b90245a 100644
> > >>>> --- a/src/libcamera/pipeline_handler.cpp
> > >>>> +++ b/src/libcamera/pipeline_handler.cpp
> > >>>> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
> > >>>>    {
> > >>>>            LIBCAMERA_TRACEPOINT(request_queue, request);
> > >>>>    
> > >>>> +  ASSERT(request->status() == Request::RequestIdle);
> > >>>> +
> > >>>>            Camera *camera = request->_d()->camera();
> > >>>>            Camera::Private *data = camera->_d();
> > >>>>            data->waitingRequests_.push(request);
> > >>>> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> > >>>>                            break;
> > >>>>    
> > >>>>                    Request *request = data->waitingRequests_.front();
> > >>>> +          ASSERT(request->status() == Request::RequestInProgress);
> > >>>>                    if (!request->_d()->prepared_)
> > >>>>                            break;
> > >>>>    
> > >>>> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
> > >>>>    
> > >>>>            while (!data->queuedRequests_.empty()) {
> > >>>>                    Request *req = data->queuedRequests_.front();
> > >>>> -          if (req->status() == Request::RequestPending)
> > >>>> +          if (req->status() == Request::RequestInProgress)
> > >>>>                            break;
> > >>>>    
> > >>>> +          ASSERT(req->status() == Request::RequestCancelled ||
> > >>>> +                 req->status() == Request::RequestComplete);
> > >>>>                    ASSERT(!req->hasPendingBuffers());
> > >>>>                    data->queuedRequests_.pop_front();
> > >>>>                    camera->requestComplete(req);
> > >>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > >>>> index 60565f5984..89ad0d7c8d 100644
> > >>>> --- a/src/libcamera/request.cpp
> > >>>> +++ b/src/libcamera/request.cpp
> > >>>> @@ -121,7 +121,7 @@ void Request::Private::complete()
> > >>>>    {
> > >>>>            Request *request = _o<Request>();
> > >>>>    
> > >>>> -  ASSERT(request->status() == RequestPending);
> > >>>> +  ASSERT(request->status() == RequestInProgress);
> > >>>>            ASSERT(!hasPendingBuffers());
> > >>>>    
> > >>>>            request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> > >>>> @@ -159,7 +159,7 @@ void Request::Private::cancel()
> > >>>>            LIBCAMERA_TRACEPOINT(request_cancel, this);
> > >>>>    
> > >>>>            Request *request = _o<Request>();
> > >>>> -  ASSERT(request->status() == RequestPending);
> > >>>> +  ASSERT(request->status() == RequestInProgress);
> > >>>>    
> > >>>>            doCancelRequest();
> > >>>>    }
> > >>>> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
> > >>>>     */
> > >>>>    void Request::Private::prepare(std::chrono::milliseconds timeout)
> > >>>>    {
> > >>>> +  _o<Request>()->status_ = RequestInProgress;
> > >>>> +
> > >>>
> > >>> My understanding is that RequestInProgress should be the status as soon
> > >>> as the queueRequest() return successfully. Otherwise, it is again hard
> > >>> to differentiate between:
> > >>> 1. RequestIdle (application managing the request)
> > >>> 2. RequestIdle (request queued by application, and is in internal
> > >>> libcamera waiting queue), what happens if application requeues the same
> > >>> request here?
> > >>>
> > >>> Do we need additional status to differentiate between the two?
> > >>>
> > >>> RequestIdle
> > >>> RequestQueued
> > >>> RequestInProgress ?
> > >>
> > >> PipelineHandler::queueRequest() "immediately" calls "prepare()" on the request,
> > >> so the state transition will happen "quickly", so this does not seem that problematic
> > >> to me at the moment. And we should probably limit the number of "internal" states exposed
> > >> in the public interface.
> > >>
> > >> But something more atomic would be better because there is indeed a small time window
> > >> between `Camera::queueRequest()` returning and `Request::Private::prepare()` running.
> > >> (And this say nothing about two parallel `Camera::queueRequest()` calls.)
> > >> `Request::status_` is also not accessed atomically, which is also an issue.
> > >>
> > >> So there are problems, I mostly just wanted to start some kind of discussion about this.
> > >>
> > >> Some options:
> > >>
> > >>     * keep `RequestPending` as is, add an internal `std::atomic_bool queued` to `Request::Private`
> > >>       to distinguish the two "pending" states;
> > >>     * add "Idle" and "InProgress" states as proposed here, but additionally make `Request::status_` atomic.
> > > 
> > > Feel free to explore splitting the state to only expose to applications
> > > the information they need, and keeping the rest internal. The current
> > > states already feel a bit dirty, with RequestComplete and
> > > RequestCancelled both indicating completed requests, so there could be
> > > room for improvement (I'm not entirely certain alternatives would be
> > > clearly better though).
> > > 
> > >> In both cases `Camera::queueRequest()` would need to be changed to do an atomic CAS
> > >> to check and transition into the new state.
> > > 
> > > I don't think you need to spend too much effort trying to prevent two
> > > concurrent queueRequest() calls using the same request. If it's really
> > > easy to do I won't object, but it's a clear application bug that I would
> > > personally consider as deserving as much empathy as
> > > queueRequest(reinterpret_cast<Request *>(0xdeadbeef));
> > 
> > Hmm, maybe it's because I myself have run into this issue, but I
> > feel quite differently about this. I think this should be robustly
> > diagnosed.
> 
> I'm also going to be vocally on the side of *We should do better to
> support applications here*.
> 
> This reminds me of the Request Canary I proposed
> https://patchwork.libcamera.org/patch/18226/ - Which I /still/ put on
> customer branches and it *has* caught and prevented bugs in those
> developments.
> 
> 
> Yes, applications can do bad things, and they can double free or
> introduce use after free errors. And in C scope - perhaps those can
> easily be detected with asan or such.
> 
> Here /we/ are the framework - and /we/ provide the interfaces. 
> 
> If we put the shotgun in the hands of the developer, when they point it
> at their feet, I think we have a responsibilty to tell them what will
> happen when they pull the trigger.

As I said in my previous e-mail, adding safeguards to report when an
already queued request is queued is a good thing. Safeguards against
*concurrent* queueRequest() calls from different threads *with the same
request* is what I said seems overkill. Barnabás, which one of the two
did you mean ?

> > >>>>            /* Create and connect one notifier for each synchronization fence. */
> > >>>>            for (FrameBuffer *buffer : pending_) {
> > >>>>                    const Fence *fence = buffer->_d()->fence();
> > >>>> @@ -309,8 +311,10 @@ void Request::Private::timeout()
> > >>>>    /**
> > >>>>     * \enum Request::Status
> > >>>>     * Request completion status
> > >>>> - * \var Request::RequestPending
> > >>>> - * The request hasn't completed yet
> > >>>> + * \var Request::RequestIdle
> > >>>> + * The request is idle
> > >>>> + * \var Request::RequestInProgress
> > >>>> + * The request is being processed
> > >>>>     * \var Request::RequestComplete
> > >>>>     * The request has completed
> > >>>>     * \var Request::RequestCancelled
> > >>>> @@ -354,7 +358,7 @@ void Request::Private::timeout()
> > >>>>     */
> > >>>>    Request::Request(Camera *camera, uint64_t cookie)
> > >>>>            : Extensible(std::make_unique<Private>(camera)),
> > >>>> -    cookie_(cookie), status_(RequestPending)
> > >>>> +    cookie_(cookie), status_(RequestIdle)
> > >>>>    {
> > >>>>            controls_ = new ControlList(controls::controls,
> > >>>>                                        camera->_d()->validator());
> > >>>> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
> > >>>>    {
> > >>>>            LIBCAMERA_TRACEPOINT(request_reuse, this);
> > >>>>    
> > >>>> +  ASSERT(status_ == RequestIdle ||
> > >>>> +         status_ == RequestComplete ||
> > >>>> +         status_ == RequestCancelled);
> > >>>> +
> > >>>>            _d()->reset();
> > >>>>    
> > >>>>            if (flags & ReuseBuffers) {
> > >>>> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
> > >>>>                    bufferMap_.clear();
> > >>>>            }
> > >>>>    
> > >>>> -  status_ = RequestPending;
> > >>>> +  status_ = RequestIdle;
> > >>>>    
> > >>>>            controls_->clear();
> > >>>>            metadata_->clear();
> > >>>> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
> > >>>>     * \fn Request::status()
> > >>>>     * \brief Retrieve the request completion status
> > >>>>     *
> > >>>> - * The request status indicates whether the request has completed successfully
> > >>>> - * or with an error. When requests are created and before they complete the
> > >>>> - * request status is set to RequestPending, and is updated at completion time
> > >>>> - * to RequestComplete. If a request is cancelled at capture stop before it has
> > >>>> - * completed, its status is set to RequestCancelled.
> > >>>> + * The request status indicates whether the request has completed successfully or with
> > >>>> + * an error. When requests are created the request status is set to RequestIdle; when
> > >>>> + * they are queued, the status is set to RequestInProgress. Then it is updated at
> > >>>> + * completion time to RequestComplete. If a request is cancelled at capture stop before
> > >>>> + * it has completed, its status is set to RequestCancelled.
> > >>>>     *
> > >>>>     * \return The request completion status
> > >>>>     */
> > >>>> @@ -607,8 +615,8 @@ std::string Request::toString() const
> > >>>>     */
> > >>>>    std::ostream &operator<<(std::ostream &out, const Request &r)
> > >>>>    {
> > >>>> -  /* Pending, Completed, Cancelled(X). */
> > >>>> -  static const char *statuses = "PCX";
> > >>>> +  /* Idle, InProgress(P), Completed, Cancelled(X). */
> > >>>> +  static const char statuses[] = "IPCX";
> > >>>>    
> > >>>>            /* Example Output: Request(55:P:1/2:6523524) */
> > >>>>            out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
> > >>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > >>>> index a983ea75c3..d12f00a6f5 100644
> > >>>> --- a/src/py/libcamera/py_main.cpp
> > >>>> +++ b/src/py/libcamera/py_main.cpp
> > >>>> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
> > >>>>                    .def("__str__", &Request::toString);
> > >>>>    
> > >>>>            pyRequestStatus
> > >>>> -          .value("Pending", Request::RequestPending)
> > >>>> +          .value("Idle", Request::RequestIdle)
> > >>>> +          .value("InProgress", Request::RequestInProgress)
> > >>>>                    .value("Complete", Request::RequestComplete)
> > >>>>                    .value("Cancelled", Request::RequestCancelled);

2025. 12. 02. 15:59 keltezéssel, Laurent Pinchart írta:
> On Tue, Dec 02, 2025 at 12:55:05PM +0000, Kieran Bingham wrote:
>> Quoting Barnabás Pőcze (2025-12-02 12:26:51)
>>> 2025. 12. 01. 16:34 keltezéssel, Laurent Pinchart írta:
>>>> On Mon, Dec 01, 2025 at 03:56:39PM +0100, Barnabás Pőcze wrote:
>>>>> 2025. 12. 01. 15:42 keltezéssel, Umang Jain írta:
>>>>>> On 2025-12-01 18:14, Barnabás Pőcze wrote:
>>>>>>> The "Pending" state at the moment is used for two very distinct scenarios:
>>>>>>> when the application alone is managing the request, and when the request
>>>>>>> is being processed. This duality makes it harder to track the true state of
>>>>>>> a request and consequently it is harder to diagnose misuses.
>>>>>>>
>>>>>>> So split the "Pending" state into the "Idle" and "InProgress" states to
>>>>>>> make it explicit when a camera is managing the request. This improves
>>>>>>> the detection of request double queueing, but since the status flag is
>>>>>>> not updated atomically, it cannot detect all cases.
>>>>>>>
>>>>>>> The name "Pending" is not kept because any application depending on it
>>>>>>> has to be adjusted, so it is better to break the API as well. This is also
>>>>>>> an ABI break.
>>>>>>>
>>>>>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>>>> ---
>>>>>>>     .../internal/tracepoints/request_enums.tp     |  7 ++--
>>>>>>>     include/libcamera/request.h                   |  3 +-
>>>>>>>     src/libcamera/camera.cpp                      |  2 +-
>>>>>>>     src/libcamera/pipeline_handler.cpp            |  7 +++-
>>>>>>>     src/libcamera/request.cpp                     | 34 ++++++++++++-------
>>>>>>>     src/py/libcamera/py_main.cpp                  |  3 +-
>>>>>>>     6 files changed, 36 insertions(+), 20 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
>>>>>>> index bcbd1aaa2c..e9a5a78fac 100644
>>>>>>> --- a/include/libcamera/internal/tracepoints/request_enums.tp
>>>>>>> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
>>>>>>> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
>>>>>>>             libcamera,
>>>>>>>             request_status,
>>>>>>>             TP_ENUM_VALUES(
>>>>>>> -          ctf_enum_value("RequestPending", 0)
>>>>>>> -          ctf_enum_value("RequestComplete", 1)
>>>>>>> -          ctf_enum_value("RequestCancelled", 2)
>>>>>>> +          ctf_enum_value("RequestIdle", 0)
>>>>>>> +          ctf_enum_value("RequestInProgress", 1)
>>>>>>> +          ctf_enum_value("RequestComplete", 2)
>>>>>>> +          ctf_enum_value("RequestCancelled", 3)
>>>>>>>             )
>>>>>>>     )
>>>>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>>>>>>> index 0c5939f7b3..1e7a5ca807 100644
>>>>>>> --- a/include/libcamera/request.h
>>>>>>> +++ b/include/libcamera/request.h
>>>>>>> @@ -32,7 +32,8 @@ class Request : public Extensible
>>>>>>>     
>>>>>>>     public:
>>>>>>>             enum Status {
>>>>>>> -          RequestPending,
>>>>>>> +          RequestIdle,
>>>>>>> +          RequestInProgress,
>>>>>>>                     RequestComplete,
>>>>>>>                     RequestCancelled,
>>>>>>>             };
>>>>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>>>>>> index 2e1e146a25..b6273f6c29 100644
>>>>>>> --- a/src/libcamera/camera.cpp
>>>>>>> +++ b/src/libcamera/camera.cpp
>>>>>>> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
>>>>>>>                     return -EXDEV;
>>>>>>>             }
>>>>>>>     
>>>>>>> -  if (request->status() != Request::RequestPending) {
>>>>>>> +  if (request->status() != Request::RequestIdle) {
>>>>>>>                     LOG(Camera, Error) << request->toString() << " is not valid";
>>>>>>>                     return -EINVAL;
>>>>>>>             }
>>>>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>>>>>> index 5c469e5bad..417b90245a 100644
>>>>>>> --- a/src/libcamera/pipeline_handler.cpp
>>>>>>> +++ b/src/libcamera/pipeline_handler.cpp
>>>>>>> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
>>>>>>>     {
>>>>>>>             LIBCAMERA_TRACEPOINT(request_queue, request);
>>>>>>>     
>>>>>>> +  ASSERT(request->status() == Request::RequestIdle);
>>>>>>> +
>>>>>>>             Camera *camera = request->_d()->camera();
>>>>>>>             Camera::Private *data = camera->_d();
>>>>>>>             data->waitingRequests_.push(request);
>>>>>>> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>>>>>>>                             break;
>>>>>>>     
>>>>>>>                     Request *request = data->waitingRequests_.front();
>>>>>>> +          ASSERT(request->status() == Request::RequestInProgress);
>>>>>>>                     if (!request->_d()->prepared_)
>>>>>>>                             break;
>>>>>>>     
>>>>>>> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
>>>>>>>     
>>>>>>>             while (!data->queuedRequests_.empty()) {
>>>>>>>                     Request *req = data->queuedRequests_.front();
>>>>>>> -          if (req->status() == Request::RequestPending)
>>>>>>> +          if (req->status() == Request::RequestInProgress)
>>>>>>>                             break;
>>>>>>>     
>>>>>>> +          ASSERT(req->status() == Request::RequestCancelled ||
>>>>>>> +                 req->status() == Request::RequestComplete);
>>>>>>>                     ASSERT(!req->hasPendingBuffers());
>>>>>>>                     data->queuedRequests_.pop_front();
>>>>>>>                     camera->requestComplete(req);
>>>>>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>>>>>>> index 60565f5984..89ad0d7c8d 100644
>>>>>>> --- a/src/libcamera/request.cpp
>>>>>>> +++ b/src/libcamera/request.cpp
>>>>>>> @@ -121,7 +121,7 @@ void Request::Private::complete()
>>>>>>>     {
>>>>>>>             Request *request = _o<Request>();
>>>>>>>     
>>>>>>> -  ASSERT(request->status() == RequestPending);
>>>>>>> +  ASSERT(request->status() == RequestInProgress);
>>>>>>>             ASSERT(!hasPendingBuffers());
>>>>>>>     
>>>>>>>             request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
>>>>>>> @@ -159,7 +159,7 @@ void Request::Private::cancel()
>>>>>>>             LIBCAMERA_TRACEPOINT(request_cancel, this);
>>>>>>>     
>>>>>>>             Request *request = _o<Request>();
>>>>>>> -  ASSERT(request->status() == RequestPending);
>>>>>>> +  ASSERT(request->status() == RequestInProgress);
>>>>>>>     
>>>>>>>             doCancelRequest();
>>>>>>>     }
>>>>>>> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
>>>>>>>      */
>>>>>>>     void Request::Private::prepare(std::chrono::milliseconds timeout)
>>>>>>>     {
>>>>>>> +  _o<Request>()->status_ = RequestInProgress;
>>>>>>> +
>>>>>>
>>>>>> My understanding is that RequestInProgress should be the status as soon
>>>>>> as the queueRequest() return successfully. Otherwise, it is again hard
>>>>>> to differentiate between:
>>>>>> 1. RequestIdle (application managing the request)
>>>>>> 2. RequestIdle (request queued by application, and is in internal
>>>>>> libcamera waiting queue), what happens if application requeues the same
>>>>>> request here?
>>>>>>
>>>>>> Do we need additional status to differentiate between the two?
>>>>>>
>>>>>> RequestIdle
>>>>>> RequestQueued
>>>>>> RequestInProgress ?
>>>>>
>>>>> PipelineHandler::queueRequest() "immediately" calls "prepare()" on the request,
>>>>> so the state transition will happen "quickly", so this does not seem that problematic
>>>>> to me at the moment. And we should probably limit the number of "internal" states exposed
>>>>> in the public interface.
>>>>>
>>>>> But something more atomic would be better because there is indeed a small time window
>>>>> between `Camera::queueRequest()` returning and `Request::Private::prepare()` running.
>>>>> (And this say nothing about two parallel `Camera::queueRequest()` calls.)
>>>>> `Request::status_` is also not accessed atomically, which is also an issue.
>>>>>
>>>>> So there are problems, I mostly just wanted to start some kind of discussion about this.
>>>>>
>>>>> Some options:
>>>>>
>>>>>      * keep `RequestPending` as is, add an internal `std::atomic_bool queued` to `Request::Private`
>>>>>        to distinguish the two "pending" states;
>>>>>      * add "Idle" and "InProgress" states as proposed here, but additionally make `Request::status_` atomic.
>>>>
>>>> Feel free to explore splitting the state to only expose to applications
>>>> the information they need, and keeping the rest internal. The current
>>>> states already feel a bit dirty, with RequestComplete and
>>>> RequestCancelled both indicating completed requests, so there could be
>>>> room for improvement (I'm not entirely certain alternatives would be
>>>> clearly better though).
>>>>
>>>>> In both cases `Camera::queueRequest()` would need to be changed to do an atomic CAS
>>>>> to check and transition into the new state.
>>>>
>>>> I don't think you need to spend too much effort trying to prevent two
>>>> concurrent queueRequest() calls using the same request. If it's really
>>>> easy to do I won't object, but it's a clear application bug that I would
>>>> personally consider as deserving as much empathy as
>>>> queueRequest(reinterpret_cast<Request *>(0xdeadbeef));
>>>
>>> Hmm, maybe it's because I myself have run into this issue, but I
>>> feel quite differently about this. I think this should be robustly
>>> diagnosed.
>>
>> I'm also going to be vocally on the side of *We should do better to
>> support applications here*.
>>
>> This reminds me of the Request Canary I proposed
>> https://patchwork.libcamera.org/patch/18226/ - Which I /still/ put on
>> customer branches and it *has* caught and prevented bugs in those
>> developments.
>>
>>
>> Yes, applications can do bad things, and they can double free or
>> introduce use after free errors. And in C scope - perhaps those can
>> easily be detected with asan or such.
>>
>> Here /we/ are the framework - and /we/ provide the interfaces.
>>
>> If we put the shotgun in the hands of the developer, when they point it
>> at their feet, I think we have a responsibilty to tell them what will
>> happen when they pull the trigger.
> 
> As I said in my previous e-mail, adding safeguards to report when an
> already queued request is queued is a good thing. Safeguards against
> *concurrent* queueRequest() calls from different threads *with the same
> request* is what I said seems overkill. Barnabás, which one of the two
> did you mean ?

I must have misunderstood then. I wasn't really making a distinction between the two
because it seems to me that the only real difference is whether an atomic cas is used
or not; at least in both approaches I tried. In any case I believe it can be expected
that most applications will use just the same thread to manage a camera, so the
concurrent case is probably a lot less likely. Nonetheless it seems to me that it
can be addressed relatively easily.


> 
>>>>>>>             /* Create and connect one notifier for each synchronization fence. */
>>>>>>>             for (FrameBuffer *buffer : pending_) {
>>>>>>>                     const Fence *fence = buffer->_d()->fence();
>>>>>>> @@ -309,8 +311,10 @@ void Request::Private::timeout()
>>>>>>>     /**
>>>>>>>      * \enum Request::Status
>>>>>>>      * Request completion status
>>>>>>> - * \var Request::RequestPending
>>>>>>> - * The request hasn't completed yet
>>>>>>> + * \var Request::RequestIdle
>>>>>>> + * The request is idle
>>>>>>> + * \var Request::RequestInProgress
>>>>>>> + * The request is being processed
>>>>>>>      * \var Request::RequestComplete
>>>>>>>      * The request has completed
>>>>>>>      * \var Request::RequestCancelled
>>>>>>> @@ -354,7 +358,7 @@ void Request::Private::timeout()
>>>>>>>      */
>>>>>>>     Request::Request(Camera *camera, uint64_t cookie)
>>>>>>>             : Extensible(std::make_unique<Private>(camera)),
>>>>>>> -    cookie_(cookie), status_(RequestPending)
>>>>>>> +    cookie_(cookie), status_(RequestIdle)
>>>>>>>     {
>>>>>>>             controls_ = new ControlList(controls::controls,
>>>>>>>                                         camera->_d()->validator());
>>>>>>> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
>>>>>>>     {
>>>>>>>             LIBCAMERA_TRACEPOINT(request_reuse, this);
>>>>>>>     
>>>>>>> +  ASSERT(status_ == RequestIdle ||
>>>>>>> +         status_ == RequestComplete ||
>>>>>>> +         status_ == RequestCancelled);
>>>>>>> +
>>>>>>>             _d()->reset();
>>>>>>>     
>>>>>>>             if (flags & ReuseBuffers) {
>>>>>>> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
>>>>>>>                     bufferMap_.clear();
>>>>>>>             }
>>>>>>>     
>>>>>>> -  status_ = RequestPending;
>>>>>>> +  status_ = RequestIdle;
>>>>>>>     
>>>>>>>             controls_->clear();
>>>>>>>             metadata_->clear();
>>>>>>> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
>>>>>>>      * \fn Request::status()
>>>>>>>      * \brief Retrieve the request completion status
>>>>>>>      *
>>>>>>> - * The request status indicates whether the request has completed successfully
>>>>>>> - * or with an error. When requests are created and before they complete the
>>>>>>> - * request status is set to RequestPending, and is updated at completion time
>>>>>>> - * to RequestComplete. If a request is cancelled at capture stop before it has
>>>>>>> - * completed, its status is set to RequestCancelled.
>>>>>>> + * The request status indicates whether the request has completed successfully or with
>>>>>>> + * an error. When requests are created the request status is set to RequestIdle; when
>>>>>>> + * they are queued, the status is set to RequestInProgress. Then it is updated at
>>>>>>> + * completion time to RequestComplete. If a request is cancelled at capture stop before
>>>>>>> + * it has completed, its status is set to RequestCancelled.
>>>>>>>      *
>>>>>>>      * \return The request completion status
>>>>>>>      */
>>>>>>> @@ -607,8 +615,8 @@ std::string Request::toString() const
>>>>>>>      */
>>>>>>>     std::ostream &operator<<(std::ostream &out, const Request &r)
>>>>>>>     {
>>>>>>> -  /* Pending, Completed, Cancelled(X). */
>>>>>>> -  static const char *statuses = "PCX";
>>>>>>> +  /* Idle, InProgress(P), Completed, Cancelled(X). */
>>>>>>> +  static const char statuses[] = "IPCX";
>>>>>>>     
>>>>>>>             /* Example Output: Request(55:P:1/2:6523524) */
>>>>>>>             out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
>>>>>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>>>>>>> index a983ea75c3..d12f00a6f5 100644
>>>>>>> --- a/src/py/libcamera/py_main.cpp
>>>>>>> +++ b/src/py/libcamera/py_main.cpp
>>>>>>> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
>>>>>>>                     .def("__str__", &Request::toString);
>>>>>>>     
>>>>>>>             pyRequestStatus
>>>>>>> -          .value("Pending", Request::RequestPending)
>>>>>>> +          .value("Idle", Request::RequestIdle)
>>>>>>> +          .value("InProgress", Request::RequestInProgress)
>>>>>>>                     .value("Complete", Request::RequestComplete)
>>>>>>>                     .value("Cancelled", Request::RequestCancelled);
>

On Tue, Dec 02, 2025 at 07:08:12PM +0100, Barnabás Pőcze wrote:
> 2025. 12. 02. 15:59 keltezéssel, Laurent Pinchart írta:
> > On Tue, Dec 02, 2025 at 12:55:05PM +0000, Kieran Bingham wrote:
> >> Quoting Barnabás Pőcze (2025-12-02 12:26:51)
> >>> 2025. 12. 01. 16:34 keltezéssel, Laurent Pinchart írta:
> >>>> On Mon, Dec 01, 2025 at 03:56:39PM +0100, Barnabás Pőcze wrote:
> >>>>> 2025. 12. 01. 15:42 keltezéssel, Umang Jain írta:
> >>>>>> On 2025-12-01 18:14, Barnabás Pőcze wrote:
> >>>>>>> The "Pending" state at the moment is used for two very distinct scenarios:
> >>>>>>> when the application alone is managing the request, and when the request
> >>>>>>> is being processed. This duality makes it harder to track the true state of
> >>>>>>> a request and consequently it is harder to diagnose misuses.
> >>>>>>>
> >>>>>>> So split the "Pending" state into the "Idle" and "InProgress" states to
> >>>>>>> make it explicit when a camera is managing the request. This improves
> >>>>>>> the detection of request double queueing, but since the status flag is
> >>>>>>> not updated atomically, it cannot detect all cases.
> >>>>>>>
> >>>>>>> The name "Pending" is not kept because any application depending on it
> >>>>>>> has to be adjusted, so it is better to break the API as well. This is also
> >>>>>>> an ABI break.
> >>>>>>>
> >>>>>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/issues/197
> >>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>>>>> ---
> >>>>>>>     .../internal/tracepoints/request_enums.tp     |  7 ++--
> >>>>>>>     include/libcamera/request.h                   |  3 +-
> >>>>>>>     src/libcamera/camera.cpp                      |  2 +-
> >>>>>>>     src/libcamera/pipeline_handler.cpp            |  7 +++-
> >>>>>>>     src/libcamera/request.cpp                     | 34 ++++++++++++-------
> >>>>>>>     src/py/libcamera/py_main.cpp                  |  3 +-
> >>>>>>>     6 files changed, 36 insertions(+), 20 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
> >>>>>>> index bcbd1aaa2c..e9a5a78fac 100644
> >>>>>>> --- a/include/libcamera/internal/tracepoints/request_enums.tp
> >>>>>>> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
> >>>>>>> @@ -9,8 +9,9 @@ TRACEPOINT_ENUM(
> >>>>>>>             libcamera,
> >>>>>>>             request_status,
> >>>>>>>             TP_ENUM_VALUES(
> >>>>>>> -          ctf_enum_value("RequestPending", 0)
> >>>>>>> -          ctf_enum_value("RequestComplete", 1)
> >>>>>>> -          ctf_enum_value("RequestCancelled", 2)
> >>>>>>> +          ctf_enum_value("RequestIdle", 0)
> >>>>>>> +          ctf_enum_value("RequestInProgress", 1)
> >>>>>>> +          ctf_enum_value("RequestComplete", 2)
> >>>>>>> +          ctf_enum_value("RequestCancelled", 3)
> >>>>>>>             )
> >>>>>>>     )
> >>>>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> >>>>>>> index 0c5939f7b3..1e7a5ca807 100644
> >>>>>>> --- a/include/libcamera/request.h
> >>>>>>> +++ b/include/libcamera/request.h
> >>>>>>> @@ -32,7 +32,8 @@ class Request : public Extensible
> >>>>>>>     
> >>>>>>>     public:
> >>>>>>>             enum Status {
> >>>>>>> -          RequestPending,
> >>>>>>> +          RequestIdle,
> >>>>>>> +          RequestInProgress,
> >>>>>>>                     RequestComplete,
> >>>>>>>                     RequestCancelled,
> >>>>>>>             };
> >>>>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> >>>>>>> index 2e1e146a25..b6273f6c29 100644
> >>>>>>> --- a/src/libcamera/camera.cpp
> >>>>>>> +++ b/src/libcamera/camera.cpp
> >>>>>>> @@ -1340,7 +1340,7 @@ int Camera::queueRequest(Request *request)
> >>>>>>>                     return -EXDEV;
> >>>>>>>             }
> >>>>>>>     
> >>>>>>> -  if (request->status() != Request::RequestPending) {
> >>>>>>> +  if (request->status() != Request::RequestIdle) {
> >>>>>>>                     LOG(Camera, Error) << request->toString() << " is not valid";
> >>>>>>>                     return -EINVAL;
> >>>>>>>             }
> >>>>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >>>>>>> index 5c469e5bad..417b90245a 100644
> >>>>>>> --- a/src/libcamera/pipeline_handler.cpp
> >>>>>>> +++ b/src/libcamera/pipeline_handler.cpp
> >>>>>>> @@ -470,6 +470,8 @@ void PipelineHandler::queueRequest(Request *request)
> >>>>>>>     {
> >>>>>>>             LIBCAMERA_TRACEPOINT(request_queue, request);
> >>>>>>>     
> >>>>>>> +  ASSERT(request->status() == Request::RequestIdle);
> >>>>>>> +
> >>>>>>>             Camera *camera = request->_d()->camera();
> >>>>>>>             Camera::Private *data = camera->_d();
> >>>>>>>             data->waitingRequests_.push(request);
> >>>>>>> @@ -514,6 +516,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> >>>>>>>                             break;
> >>>>>>>     
> >>>>>>>                     Request *request = data->waitingRequests_.front();
> >>>>>>> +          ASSERT(request->status() == Request::RequestInProgress);
> >>>>>>>                     if (!request->_d()->prepared_)
> >>>>>>>                             break;
> >>>>>>>     
> >>>>>>> @@ -591,9 +594,11 @@ void PipelineHandler::completeRequest(Request *request)
> >>>>>>>     
> >>>>>>>             while (!data->queuedRequests_.empty()) {
> >>>>>>>                     Request *req = data->queuedRequests_.front();
> >>>>>>> -          if (req->status() == Request::RequestPending)
> >>>>>>> +          if (req->status() == Request::RequestInProgress)
> >>>>>>>                             break;
> >>>>>>>     
> >>>>>>> +          ASSERT(req->status() == Request::RequestCancelled ||
> >>>>>>> +                 req->status() == Request::RequestComplete);
> >>>>>>>                     ASSERT(!req->hasPendingBuffers());
> >>>>>>>                     data->queuedRequests_.pop_front();
> >>>>>>>                     camera->requestComplete(req);
> >>>>>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> >>>>>>> index 60565f5984..89ad0d7c8d 100644
> >>>>>>> --- a/src/libcamera/request.cpp
> >>>>>>> +++ b/src/libcamera/request.cpp
> >>>>>>> @@ -121,7 +121,7 @@ void Request::Private::complete()
> >>>>>>>     {
> >>>>>>>             Request *request = _o<Request>();
> >>>>>>>     
> >>>>>>> -  ASSERT(request->status() == RequestPending);
> >>>>>>> +  ASSERT(request->status() == RequestInProgress);
> >>>>>>>             ASSERT(!hasPendingBuffers());
> >>>>>>>     
> >>>>>>>             request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> >>>>>>> @@ -159,7 +159,7 @@ void Request::Private::cancel()
> >>>>>>>             LIBCAMERA_TRACEPOINT(request_cancel, this);
> >>>>>>>     
> >>>>>>>             Request *request = _o<Request>();
> >>>>>>> -  ASSERT(request->status() == RequestPending);
> >>>>>>> +  ASSERT(request->status() == RequestInProgress);
> >>>>>>>     
> >>>>>>>             doCancelRequest();
> >>>>>>>     }
> >>>>>>> @@ -222,6 +222,8 @@ void Request::Private::emitPrepareCompleted()
> >>>>>>>      */
> >>>>>>>     void Request::Private::prepare(std::chrono::milliseconds timeout)
> >>>>>>>     {
> >>>>>>> +  _o<Request>()->status_ = RequestInProgress;
> >>>>>>> +
> >>>>>>
> >>>>>> My understanding is that RequestInProgress should be the status as soon
> >>>>>> as the queueRequest() return successfully. Otherwise, it is again hard
> >>>>>> to differentiate between:
> >>>>>> 1. RequestIdle (application managing the request)
> >>>>>> 2. RequestIdle (request queued by application, and is in internal
> >>>>>> libcamera waiting queue), what happens if application requeues the same
> >>>>>> request here?
> >>>>>>
> >>>>>> Do we need additional status to differentiate between the two?
> >>>>>>
> >>>>>> RequestIdle
> >>>>>> RequestQueued
> >>>>>> RequestInProgress ?
> >>>>>
> >>>>> PipelineHandler::queueRequest() "immediately" calls "prepare()" on the request,
> >>>>> so the state transition will happen "quickly", so this does not seem that problematic
> >>>>> to me at the moment. And we should probably limit the number of "internal" states exposed
> >>>>> in the public interface.
> >>>>>
> >>>>> But something more atomic would be better because there is indeed a small time window
> >>>>> between `Camera::queueRequest()` returning and `Request::Private::prepare()` running.
> >>>>> (And this say nothing about two parallel `Camera::queueRequest()` calls.)
> >>>>> `Request::status_` is also not accessed atomically, which is also an issue.
> >>>>>
> >>>>> So there are problems, I mostly just wanted to start some kind of discussion about this.
> >>>>>
> >>>>> Some options:
> >>>>>
> >>>>>      * keep `RequestPending` as is, add an internal `std::atomic_bool queued` to `Request::Private`
> >>>>>        to distinguish the two "pending" states;
> >>>>>      * add "Idle" and "InProgress" states as proposed here, but additionally make `Request::status_` atomic.
> >>>>
> >>>> Feel free to explore splitting the state to only expose to applications
> >>>> the information they need, and keeping the rest internal. The current
> >>>> states already feel a bit dirty, with RequestComplete and
> >>>> RequestCancelled both indicating completed requests, so there could be
> >>>> room for improvement (I'm not entirely certain alternatives would be
> >>>> clearly better though).
> >>>>
> >>>>> In both cases `Camera::queueRequest()` would need to be changed to do an atomic CAS
> >>>>> to check and transition into the new state.
> >>>>
> >>>> I don't think you need to spend too much effort trying to prevent two
> >>>> concurrent queueRequest() calls using the same request. If it's really
> >>>> easy to do I won't object, but it's a clear application bug that I would
> >>>> personally consider as deserving as much empathy as
> >>>> queueRequest(reinterpret_cast<Request *>(0xdeadbeef));
> >>>
> >>> Hmm, maybe it's because I myself have run into this issue, but I
> >>> feel quite differently about this. I think this should be robustly
> >>> diagnosed.
> >>
> >> I'm also going to be vocally on the side of *We should do better to
> >> support applications here*.
> >>
> >> This reminds me of the Request Canary I proposed
> >> https://patchwork.libcamera.org/patch/18226/ - Which I /still/ put on
> >> customer branches and it *has* caught and prevented bugs in those
> >> developments.
> >>
> >>
> >> Yes, applications can do bad things, and they can double free or
> >> introduce use after free errors. And in C scope - perhaps those can
> >> easily be detected with asan or such.
> >>
> >> Here /we/ are the framework - and /we/ provide the interfaces.
> >>
> >> If we put the shotgun in the hands of the developer, when they point it
> >> at their feet, I think we have a responsibilty to tell them what will
> >> happen when they pull the trigger.
> > 
> > As I said in my previous e-mail, adding safeguards to report when an
> > already queued request is queued is a good thing. Safeguards against
> > *concurrent* queueRequest() calls from different threads *with the same
> > request* is what I said seems overkill. Barnabás, which one of the two
> > did you mean ?
> 
> I must have misunderstood then. I wasn't really making a distinction between the two
> because it seems to me that the only real difference is whether an atomic cas is used
> or not; at least in both approaches I tried. In any case I believe it can be expected
> that most applications will use just the same thread to manage a camera, so the
> concurrent case is probably a lot less likely. Nonetheless it seems to me that it
> can be addressed relatively easily.

If it's easy, sure, it's good to have that check.

> >>>>>>>             /* Create and connect one notifier for each synchronization fence. */
> >>>>>>>             for (FrameBuffer *buffer : pending_) {
> >>>>>>>                     const Fence *fence = buffer->_d()->fence();
> >>>>>>> @@ -309,8 +311,10 @@ void Request::Private::timeout()
> >>>>>>>     /**
> >>>>>>>      * \enum Request::Status
> >>>>>>>      * Request completion status
> >>>>>>> - * \var Request::RequestPending
> >>>>>>> - * The request hasn't completed yet
> >>>>>>> + * \var Request::RequestIdle
> >>>>>>> + * The request is idle
> >>>>>>> + * \var Request::RequestInProgress
> >>>>>>> + * The request is being processed
> >>>>>>>      * \var Request::RequestComplete
> >>>>>>>      * The request has completed
> >>>>>>>      * \var Request::RequestCancelled
> >>>>>>> @@ -354,7 +358,7 @@ void Request::Private::timeout()
> >>>>>>>      */
> >>>>>>>     Request::Request(Camera *camera, uint64_t cookie)
> >>>>>>>             : Extensible(std::make_unique<Private>(camera)),
> >>>>>>> -    cookie_(cookie), status_(RequestPending)
> >>>>>>> +    cookie_(cookie), status_(RequestIdle)
> >>>>>>>     {
> >>>>>>>             controls_ = new ControlList(controls::controls,
> >>>>>>>                                         camera->_d()->validator());
> >>>>>>> @@ -391,6 +395,10 @@ void Request::reuse(ReuseFlag flags)
> >>>>>>>     {
> >>>>>>>             LIBCAMERA_TRACEPOINT(request_reuse, this);
> >>>>>>>     
> >>>>>>> +  ASSERT(status_ == RequestIdle ||
> >>>>>>> +         status_ == RequestComplete ||
> >>>>>>> +         status_ == RequestCancelled);
> >>>>>>> +
> >>>>>>>             _d()->reset();
> >>>>>>>     
> >>>>>>>             if (flags & ReuseBuffers) {
> >>>>>>> @@ -403,7 +411,7 @@ void Request::reuse(ReuseFlag flags)
> >>>>>>>                     bufferMap_.clear();
> >>>>>>>             }
> >>>>>>>     
> >>>>>>> -  status_ = RequestPending;
> >>>>>>> +  status_ = RequestIdle;
> >>>>>>>     
> >>>>>>>             controls_->clear();
> >>>>>>>             metadata_->clear();
> >>>>>>> @@ -563,11 +571,11 @@ uint32_t Request::sequence() const
> >>>>>>>      * \fn Request::status()
> >>>>>>>      * \brief Retrieve the request completion status
> >>>>>>>      *
> >>>>>>> - * The request status indicates whether the request has completed successfully
> >>>>>>> - * or with an error. When requests are created and before they complete the
> >>>>>>> - * request status is set to RequestPending, and is updated at completion time
> >>>>>>> - * to RequestComplete. If a request is cancelled at capture stop before it has
> >>>>>>> - * completed, its status is set to RequestCancelled.
> >>>>>>> + * The request status indicates whether the request has completed successfully or with
> >>>>>>> + * an error. When requests are created the request status is set to RequestIdle; when
> >>>>>>> + * they are queued, the status is set to RequestInProgress. Then it is updated at
> >>>>>>> + * completion time to RequestComplete. If a request is cancelled at capture stop before
> >>>>>>> + * it has completed, its status is set to RequestCancelled.
> >>>>>>>      *
> >>>>>>>      * \return The request completion status
> >>>>>>>      */
> >>>>>>> @@ -607,8 +615,8 @@ std::string Request::toString() const
> >>>>>>>      */
> >>>>>>>     std::ostream &operator<<(std::ostream &out, const Request &r)
> >>>>>>>     {
> >>>>>>> -  /* Pending, Completed, Cancelled(X). */
> >>>>>>> -  static const char *statuses = "PCX";
> >>>>>>> +  /* Idle, InProgress(P), Completed, Cancelled(X). */
> >>>>>>> +  static const char statuses[] = "IPCX";
> >>>>>>>     
> >>>>>>>             /* Example Output: Request(55:P:1/2:6523524) */
> >>>>>>>             out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
> >>>>>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> >>>>>>> index a983ea75c3..d12f00a6f5 100644
> >>>>>>> --- a/src/py/libcamera/py_main.cpp
> >>>>>>> +++ b/src/py/libcamera/py_main.cpp
> >>>>>>> @@ -486,7 +486,8 @@ PYBIND11_MODULE(_libcamera, m)
> >>>>>>>                     .def("__str__", &Request::toString);
> >>>>>>>     
> >>>>>>>             pyRequestStatus
> >>>>>>> -          .value("Pending", Request::RequestPending)
> >>>>>>> +          .value("Idle", Request::RequestIdle)
> >>>>>>> +          .value("InProgress", Request::RequestInProgress)
> >>>>>>>                     .value("Complete", Request::RequestComplete)
> >>>>>>>                     .value("Cancelled", Request::RequestCancelled);