[libcamera-devel,v3,02/10] android: camera_device: Transform descriptors_ map to queue

13866 diff mbox series

The descriptors_ map hold Camera3RequestDescriptor(s) which are
per-capture request placed by the framework to libcamera HAL.
CameraDevice::requestComplete() looks for the descriptor for which the
camera request has been completed and removes it from the map.
However, this approach has its limitations going forwards.

In subsequent commits, the post-processing operation which happens
in requestComplete() synchronously, is going to be run asynchronously.
Therefore, instead of a map for descriptors, a queue makes more sense
going forwards and the framework expects capture results to be received
in the same order as they were queued. When the async processing is
completed, the descriptor queue is inspected to send back the capture
results and then de-queued. This helps to maintain the order of sending
back the capture results whilst preventing unnecessary complexity of
using a map.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
 src/android/camera_device.h   |  5 +-
 2 files changed, 48 insertions(+), 46 deletions(-)

Hi Umang,

Thank you for the patch.

On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
> The descriptors_ map hold Camera3RequestDescriptor(s) which are
> per-capture request placed by the framework to libcamera HAL.
> CameraDevice::requestComplete() looks for the descriptor for which the
> camera request has been completed and removes it from the map.
> However, this approach has its limitations going forwards.
> 
> In subsequent commits, the post-processing operation which happens
> in requestComplete() synchronously, is going to be run asynchronously.
> Therefore, instead of a map for descriptors, a queue makes more sense
> going forwards and the framework expects capture results to be received
> in the same order as they were queued. When the async processing is
> completed, the descriptor queue is inspected to send back the capture
> results and then de-queued. This helps to maintain the order of sending
> back the capture results whilst preventing unnecessary complexity of
> using a map.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
>  src/android/camera_device.h   |  5 +-
>  2 files changed, 48 insertions(+), 46 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f461e14c..0562c225 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * The descriptor and the associated memory reserved here are freed
>  	 * at request complete time.
>  	 */
> -	Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> +	std::unique_ptr<Camera3RequestDescriptor> descriptor =
> +		std::make_unique<Camera3RequestDescriptor>(camera_.get(),
> +							   camera3Request);
>  
>  	/*
>  	 * \todo The Android request model is incremental, settings passed in
> @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	if (camera3Request->settings)
>  		lastSettings_ = camera3Request->settings;
>  	else
> -		descriptor.settings_ = lastSettings_;
> +		descriptor->settings_ = lastSettings_;
>  
> -	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> -			<< " with " << descriptor.buffers_.size() << " streams";
> -	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> -		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> +	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> +			<< " with " << descriptor->buffers_.size() << " streams";
> +	for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> +		const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
>  		camera3_stream *camera3Stream = camera3Buffer.stream;
>  		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
>  
> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			buffer = createFrameBuffer(*camera3Buffer.buffer,
>  						   cameraStream->configuration().pixelFormat,
>  						   cameraStream->configuration().size);
> -			descriptor.frameBuffers_.emplace_back(buffer);
> +			descriptor->frameBuffers_.emplace_back(buffer);
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>  
> @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			return -ENOMEM;
>  		}
>  
> -		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> +		descriptor->request_->addBuffer(cameraStream->stream(), buffer,
>  						camera3Buffer.acquire_fence);
>  	}
>  
> @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * Translate controls from Android to libcamera and queue the request
>  	 * to the CameraWorker thread.
>  	 */
> -	int ret = processControls(&descriptor);
> +	int ret = processControls(descriptor.get());
>  	if (ret)
>  		return ret;
>  
> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		state_ = State::Running;
>  	}
>  
> -	worker_.queueRequest(descriptor.request_.get());
> +	worker_.queueRequest(descriptor->request_.get());
>  
>  	{
>  		MutexLocker descriptorsLock(descriptorsMutex_);
> -		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> +		descriptors_.push_back(std::move(descriptor));
>  	}

We have a race condition here, worker_.queueRequest() should go after
adding the request to the queue. Could you fix it in a patch on top ?

>  
>  	return 0;
> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  
>  void CameraDevice::requestComplete(Request *request)
>  {
> -	decltype(descriptors_)::node_type node;
> -	{
> -		MutexLocker descriptorsLock(descriptorsMutex_);
> -		auto it = descriptors_.find(request->cookie());
> -		if (it == descriptors_.end()) {
> -			/*
> -			 * \todo Clarify if the Camera has to be closed on
> -			 * ERROR_DEVICE and possibly demote the Fatal to simple
> -			 * Error.
> -			 */
> -			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> -			LOG(HAL, Fatal)
> -				<< "Unknown request: " << request->cookie();
> +	if (descriptors_.empty())
> +		return;
>  
> -			return;
> -		}
> +	Camera3RequestDescriptor *descriptor = descriptors_.front().get();

This needs to be protected by descriptorsMutex_.

	Camera3RequestDescriptor *descriptor;

	{
		MutexLocker descriptorsLock(descriptorsMutex_);
		descriptor = descriptors_.front().get();
	}

> +	if (descriptor->request_->cookie() != request->cookie()) {

This is correct as long as we handle post-processing synchronously.
Let's see how it evolves in subsequent patches.

> +		/*
> +		 * \todo Clarify if the Camera has to be closed on
> +		 * ERROR_DEVICE and possibly demote the Fatal to simple
> +		 * Error.
> +		 */
> +		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> +		LOG(HAL, Fatal)
> +			<< "Unknown request: " << request->cookie();

I'd change the message to

			<< "Out-of-order completion for request "
			<< request->cookie();

By the way, with the cookie containing a pointer, I think it would be
more readable in hex. Maybe a patch on top to use utils::hex() ?

>  
> -		node = descriptors_.extract(it);
> +		return;
>  	}
> -	Camera3RequestDescriptor &descriptor = node.mapped();
>  
>  	/*
>  	 * Prepare the capture result for the Android camera stack.
> @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
>  	 * post-processing/compression fails.
>  	 */
>  	camera3_capture_result_t captureResult = {};
> -	captureResult.frame_number = descriptor.frameNumber_;
> -	captureResult.num_output_buffers = descriptor.buffers_.size();
> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> +	captureResult.frame_number = descriptor->frameNumber_;
> +	captureResult.num_output_buffers = descriptor->buffers_.size();
> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>  		buffer.acquire_fence = -1;
>  		buffer.release_fence = -1;
>  		buffer.status = CAMERA3_BUFFER_STATUS_OK;
>  	}
> -	captureResult.output_buffers = descriptor.buffers_.data();
> +	captureResult.output_buffers = descriptor->buffers_.data();
>  	captureResult.partial_result = 1;
>  
>  	/*
> @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
>  				<< " not successfully completed: "
>  				<< request->status();
>  
> -		notifyError(descriptor.frameNumber_, nullptr,
> +		notifyError(descriptor->frameNumber_, nullptr,
>  			    CAMERA3_MSG_ERROR_REQUEST);
>  
>  		captureResult.partial_result = 0;
> -		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> +		for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
>  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>  		callbacks_->process_capture_result(callbacks_, &captureResult);
>  
> +		descriptors_.pop_front();

I'm slightly concerned that in some paths we could complete the request
but forget to remove it from the queue. Maybe wrapping
callbacks_->process_capture_result() and descriptors_.pop_front() in a
function would be good. Let's see how it looks like with the whole
series applied.

>  		return;
>  	}
>  
> @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
>  	 */
>  	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
>  							 .get(controls::SensorTimestamp));
> -	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> +	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
>  
>  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> -			<< descriptor.buffers_.size() << " streams";
> +			<< descriptor->buffers_.size() << " streams";
>  
>  	/*
>  	 * Generate the metadata associated with the captured buffers.
> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
>  	 */
>  	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
>  	if (!resultMetadata) {
> -		notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> +		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>  
>  		/* The camera framework expects an empty metadata pack on error. */
>  		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
>  	}
>  
>  	/* Handle any JPEG compression. */
> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>  		CameraStream *cameraStream =
>  			static_cast<CameraStream *>(buffer.stream->priv);
>  
> @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
>  		if (!src) {
>  			LOG(HAL, Error) << "Failed to find a source stream buffer";
>  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> -			notifyError(descriptor.frameNumber_, buffer.stream,
> +			notifyError(descriptor->frameNumber_, buffer.stream,
>  				    CAMERA3_MSG_ERROR_BUFFER);
>  			continue;
>  		}
>  
>  		int ret = cameraStream->process(*src, *buffer.buffer,
> -						descriptor.settings_,
> +						descriptor->settings_,
>  						resultMetadata.get());
>  		/*
>  		 * Return the FrameBuffer to the CameraStream now that we're
> @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
>  
>  		if (ret) {
>  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> -			notifyError(descriptor.frameNumber_, buffer.stream,
> +			notifyError(descriptor->frameNumber_, buffer.stream,
>  				    CAMERA3_MSG_ERROR_BUFFER);
>  		}
>  	}
>  
>  	captureResult.result = resultMetadata->get();
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
> +
> +	descriptors_.pop_front();
>  }
>  
>  std::string CameraDevice::logPrefix() const
> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>   * Produce a set of fixed result metadata.
>   */
>  std::unique_ptr<CameraMetadata>
> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
>  {
> -	const ControlList &metadata = descriptor.request_->metadata();
> -	const CameraMetadata &settings = descriptor.settings_;
> +	const ControlList &metadata = descriptor->request_->metadata();
> +	const CameraMetadata &settings = descriptor->settings_;
>  	camera_metadata_ro_entry_t entry;
>  	bool found;
>  
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index a5576927..9c895b25 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -7,6 +7,7 @@
>  #ifndef __ANDROID_CAMERA_DEVICE_H__
>  #define __ANDROID_CAMERA_DEVICE_H__
>  
> +#include <deque>
>  #include <map>
>  #include <memory>
>  #include <mutex>
> @@ -103,7 +104,7 @@ private:
>  			 camera3_error_msg_code code);
>  	int processControls(Camera3RequestDescriptor *descriptor);
>  	std::unique_ptr<CameraMetadata> getResultMetadata(
> -		const Camera3RequestDescriptor &descriptor) const;
> +		const Camera3RequestDescriptor *descriptor) const;
>  
>  	unsigned int id_;
>  	camera3_device_t camera3Device_;
> @@ -123,7 +124,7 @@ private:
>  	std::vector<CameraStream> streams_;
>  
>  	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> -	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> +	std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;

Could we use a std::queue instead ? It will be implemented on top of a
std::deque, so there will be no change in performances, but it gives us
the semantics we need (essentially, a FIFO).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	std::string maker_;
>  	std::string model_;

Hi Laurent,

Thanks for the review.

On 9/21/21 12:36 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
>> The descriptors_ map hold Camera3RequestDescriptor(s) which are
>> per-capture request placed by the framework to libcamera HAL.
>> CameraDevice::requestComplete() looks for the descriptor for which the
>> camera request has been completed and removes it from the map.
>> However, this approach has its limitations going forwards.
>>
>> In subsequent commits, the post-processing operation which happens
>> in requestComplete() synchronously, is going to be run asynchronously.
>> Therefore, instead of a map for descriptors, a queue makes more sense
>> going forwards and the framework expects capture results to be received
>> in the same order as they were queued. When the async processing is
>> completed, the descriptor queue is inspected to send back the capture
>> results and then de-queued. This helps to maintain the order of sending
>> back the capture results whilst preventing unnecessary complexity of
>> using a map.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
>>   src/android/camera_device.h   |  5 +-
>>   2 files changed, 48 insertions(+), 46 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index f461e14c..0562c225 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   	 * The descriptor and the associated memory reserved here are freed
>>   	 * at request complete time.
>>   	 */
>> -	Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
>> +	std::unique_ptr<Camera3RequestDescriptor> descriptor =
>> +		std::make_unique<Camera3RequestDescriptor>(camera_.get(),
>> +							   camera3Request);
>>   
>>   	/*
>>   	 * \todo The Android request model is incremental, settings passed in
>> @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   	if (camera3Request->settings)
>>   		lastSettings_ = camera3Request->settings;
>>   	else
>> -		descriptor.settings_ = lastSettings_;
>> +		descriptor->settings_ = lastSettings_;
>>   
>> -	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
>> -			<< " with " << descriptor.buffers_.size() << " streams";
>> -	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
>> -		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
>> +	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
>> +			<< " with " << descriptor->buffers_.size() << " streams";
>> +	for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
>> +		const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
>>   		camera3_stream *camera3Stream = camera3Buffer.stream;
>>   		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
>>   
>> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   			buffer = createFrameBuffer(*camera3Buffer.buffer,
>>   						   cameraStream->configuration().pixelFormat,
>>   						   cameraStream->configuration().size);
>> -			descriptor.frameBuffers_.emplace_back(buffer);
>> +			descriptor->frameBuffers_.emplace_back(buffer);
>>   			LOG(HAL, Debug) << ss.str() << " (direct)";
>>   			break;
>>   
>> @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   			return -ENOMEM;
>>   		}
>>   
>> -		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
>> +		descriptor->request_->addBuffer(cameraStream->stream(), buffer,
>>   						camera3Buffer.acquire_fence);
>>   	}
>>   
>> @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   	 * Translate controls from Android to libcamera and queue the request
>>   	 * to the CameraWorker thread.
>>   	 */
>> -	int ret = processControls(&descriptor);
>> +	int ret = processControls(descriptor.get());
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   		state_ = State::Running;
>>   	}
>>   
>> -	worker_.queueRequest(descriptor.request_.get());
>> +	worker_.queueRequest(descriptor->request_.get());
>>   
>>   	{
>>   		MutexLocker descriptorsLock(descriptorsMutex_);
>> -		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
>> +		descriptors_.push_back(std::move(descriptor));
>>   	}
> We have a race condition here, worker_.queueRequest() should go after
> adding the request to the queue. Could you fix it in a patch on top ?


Do you mean the race condition is existing already, with the 
descriptors_ map (that has been removed from this patch)?

Yes, I can introduce a patch before this one, that fixes the race first 
in the map itself. Is my understanding correct?

>
>>   
>>   	return 0;
>> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   
>>   void CameraDevice::requestComplete(Request *request)
>>   {
>> -	decltype(descriptors_)::node_type node;
>> -	{
>> -		MutexLocker descriptorsLock(descriptorsMutex_);
>> -		auto it = descriptors_.find(request->cookie());
>> -		if (it == descriptors_.end()) {
>> -			/*
>> -			 * \todo Clarify if the Camera has to be closed on
>> -			 * ERROR_DEVICE and possibly demote the Fatal to simple
>> -			 * Error.
>> -			 */
>> -			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
>> -			LOG(HAL, Fatal)
>> -				<< "Unknown request: " << request->cookie();
>> +	if (descriptors_.empty())
>> +		return;
>>   
>> -			return;
>> -		}
>> +	Camera3RequestDescriptor *descriptor = descriptors_.front().get();
> This needs to be protected by descriptorsMutex_.
>
> 	Camera3RequestDescriptor *descriptor;
>
> 	{
> 		MutexLocker descriptorsLock(descriptorsMutex_);
> 		descriptor = descriptors_.front().get();
> 	}
>
>> +	if (descriptor->request_->cookie() != request->cookie()) {
> This is correct as long as we handle post-processing synchronously.
> Let's see how it evolves in subsequent patches.


Why not valid for async post-processing?

So this is requestComplete() function, invoked whenever a 
libcamera::Request is completed by libcamera::Camera. The completion is 
guaranteed to be done in order, right ? Later in this function, the 
post-processing shall happen (sync or async).

>
>> +		/*
>> +		 * \todo Clarify if the Camera has to be closed on
>> +		 * ERROR_DEVICE and possibly demote the Fatal to simple
>> +		 * Error.
>> +		 */
>> +		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
>> +		LOG(HAL, Fatal)
>> +			<< "Unknown request: " << request->cookie();
> I'd change the message to
>
> 			<< "Out-of-order completion for request "
> 			<< request->cookie();
>
> By the way, with the cookie containing a pointer, I think it would be
> more readable in hex. Maybe a patch on top to use utils::hex() ?


Makes sense, I'll double check

>
>>   
>> -		node = descriptors_.extract(it);
>> +		return;
>>   	}
>> -	Camera3RequestDescriptor &descriptor = node.mapped();
>>   
>>   	/*
>>   	 * Prepare the capture result for the Android camera stack.
>> @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
>>   	 * post-processing/compression fails.
>>   	 */
>>   	camera3_capture_result_t captureResult = {};
>> -	captureResult.frame_number = descriptor.frameNumber_;
>> -	captureResult.num_output_buffers = descriptor.buffers_.size();
>> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>> +	captureResult.frame_number = descriptor->frameNumber_;
>> +	captureResult.num_output_buffers = descriptor->buffers_.size();
>> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>   		buffer.acquire_fence = -1;
>>   		buffer.release_fence = -1;
>>   		buffer.status = CAMERA3_BUFFER_STATUS_OK;
>>   	}
>> -	captureResult.output_buffers = descriptor.buffers_.data();
>> +	captureResult.output_buffers = descriptor->buffers_.data();
>>   	captureResult.partial_result = 1;
>>   
>>   	/*
>> @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
>>   				<< " not successfully completed: "
>>   				<< request->status();
>>   
>> -		notifyError(descriptor.frameNumber_, nullptr,
>> +		notifyError(descriptor->frameNumber_, nullptr,
>>   			    CAMERA3_MSG_ERROR_REQUEST);
>>   
>>   		captureResult.partial_result = 0;
>> -		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
>> +		for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
>>   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>   		callbacks_->process_capture_result(callbacks_, &captureResult);
>>   
>> +		descriptors_.pop_front();
> I'm slightly concerned that in some paths we could complete the request
> but forget to remove it from the queue. Maybe wrapping
> callbacks_->process_capture_result() and descriptors_.pop_front() in a
> function would be good. Let's see how it looks like with the whole
> series applied.


This is a good point and has been address already via 
sendCaptureResults() in subsequent patches.

However, this particular error path you have pointed out here, is the 
/only/ part where queue.front() is dropped, other than ofcoourse in 
sendCaptureResults(). hmm, I'll take a look if I can figure it out too 
and have a singular place of processsing the queue.

>
>>   		return;
>>   	}
>>   
>> @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
>>   	 */
>>   	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
>>   							 .get(controls::SensorTimestamp));
>> -	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
>> +	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
>>   
>>   	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
>> -			<< descriptor.buffers_.size() << " streams";
>> +			<< descriptor->buffers_.size() << " streams";
>>   
>>   	/*
>>   	 * Generate the metadata associated with the captured buffers.
>> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
>>   	 */
>>   	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
>>   	if (!resultMetadata) {
>> -		notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>> +		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>>   
>>   		/* The camera framework expects an empty metadata pack on error. */
>>   		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
>>   	}
>>   
>>   	/* Handle any JPEG compression. */
>> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>   		CameraStream *cameraStream =
>>   			static_cast<CameraStream *>(buffer.stream->priv);
>>   
>> @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
>>   		if (!src) {
>>   			LOG(HAL, Error) << "Failed to find a source stream buffer";
>>   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>> -			notifyError(descriptor.frameNumber_, buffer.stream,
>> +			notifyError(descriptor->frameNumber_, buffer.stream,
>>   				    CAMERA3_MSG_ERROR_BUFFER);
>>   			continue;
>>   		}
>>   
>>   		int ret = cameraStream->process(*src, *buffer.buffer,
>> -						descriptor.settings_,
>> +						descriptor->settings_,
>>   						resultMetadata.get());
>>   		/*
>>   		 * Return the FrameBuffer to the CameraStream now that we're
>> @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
>>   
>>   		if (ret) {
>>   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>> -			notifyError(descriptor.frameNumber_, buffer.stream,
>> +			notifyError(descriptor->frameNumber_, buffer.stream,
>>   				    CAMERA3_MSG_ERROR_BUFFER);
>>   		}
>>   	}
>>   
>>   	captureResult.result = resultMetadata->get();
>>   	callbacks_->process_capture_result(callbacks_, &captureResult);
>> +
>> +	descriptors_.pop_front();
>>   }
>>   
>>   std::string CameraDevice::logPrefix() const
>> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>>    * Produce a set of fixed result metadata.
>>    */
>>   std::unique_ptr<CameraMetadata>
>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
>>   {
>> -	const ControlList &metadata = descriptor.request_->metadata();
>> -	const CameraMetadata &settings = descriptor.settings_;
>> +	const ControlList &metadata = descriptor->request_->metadata();
>> +	const CameraMetadata &settings = descriptor->settings_;
>>   	camera_metadata_ro_entry_t entry;
>>   	bool found;
>>   
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index a5576927..9c895b25 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -7,6 +7,7 @@
>>   #ifndef __ANDROID_CAMERA_DEVICE_H__
>>   #define __ANDROID_CAMERA_DEVICE_H__
>>   
>> +#include <deque>
>>   #include <map>
>>   #include <memory>
>>   #include <mutex>
>> @@ -103,7 +104,7 @@ private:
>>   			 camera3_error_msg_code code);
>>   	int processControls(Camera3RequestDescriptor *descriptor);
>>   	std::unique_ptr<CameraMetadata> getResultMetadata(
>> -		const Camera3RequestDescriptor &descriptor) const;
>> +		const Camera3RequestDescriptor *descriptor) const;
>>   
>>   	unsigned int id_;
>>   	camera3_device_t camera3Device_;
>> @@ -123,7 +124,7 @@ private:
>>   	std::vector<CameraStream> streams_;
>>   
>>   	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
>> -	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
>> +	std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> Could we use a std::queue instead ? It will be implemented on top of a
> std::deque, so there will be no change in performances, but it gives us
> the semantics we need (essentially, a FIFO).


Yes, great! The deque is coming from earlier version where we need to 
iterate over the queue.

I see, no place as such in v3, where the queue is iterated upon, so we 
can surely use std::queue.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>>   
>>   	std::string maker_;
>>   	std::string model_;

Hi Umang, Laurent

On Mon, Sep 20, 2021 at 10:06:32PM +0300, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
> > The descriptors_ map hold Camera3RequestDescriptor(s) which are
> > per-capture request placed by the framework to libcamera HAL.
> > CameraDevice::requestComplete() looks for the descriptor for which the
> > camera request has been completed and removes it from the map.
> > However, this approach has its limitations going forwards.
> >
> > In subsequent commits, the post-processing operation which happens
> > in requestComplete() synchronously, is going to be run asynchronously.
> > Therefore, instead of a map for descriptors, a queue makes more sense
> > going forwards and the framework expects capture results to be received
> > in the same order as they were queued. When the async processing is
> > completed, the descriptor queue is inspected to send back the capture
> > results and then de-queued. This helps to maintain the order of sending
> > back the capture results whilst preventing unnecessary complexity of
> > using a map.
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
> >  src/android/camera_device.h   |  5 +-
> >  2 files changed, 48 insertions(+), 46 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index f461e14c..0562c225 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	 * The descriptor and the associated memory reserved here are freed
> >  	 * at request complete time.
> >  	 */
> > -	Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> > +	std::unique_ptr<Camera3RequestDescriptor> descriptor =
> > +		std::make_unique<Camera3RequestDescriptor>(camera_.get(),
> > +							   camera3Request);
> >
> >  	/*
> >  	 * \todo The Android request model is incremental, settings passed in
> > @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	if (camera3Request->settings)
> >  		lastSettings_ = camera3Request->settings;
> >  	else
> > -		descriptor.settings_ = lastSettings_;
> > +		descriptor->settings_ = lastSettings_;
> >
> > -	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> > -			<< " with " << descriptor.buffers_.size() << " streams";
> > -	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> > -		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> > +	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> > +			<< " with " << descriptor->buffers_.size() << " streams";
> > +	for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> > +		const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
> >  		camera3_stream *camera3Stream = camera3Buffer.stream;
> >  		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> >
> > @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  			buffer = createFrameBuffer(*camera3Buffer.buffer,
> >  						   cameraStream->configuration().pixelFormat,
> >  						   cameraStream->configuration().size);
> > -			descriptor.frameBuffers_.emplace_back(buffer);
> > +			descriptor->frameBuffers_.emplace_back(buffer);
> >  			LOG(HAL, Debug) << ss.str() << " (direct)";
> >  			break;
> >
> > @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  			return -ENOMEM;
> >  		}
> >
> > -		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> > +		descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> >  						camera3Buffer.acquire_fence);
> >  	}
> >
> > @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	 * Translate controls from Android to libcamera and queue the request
> >  	 * to the CameraWorker thread.
> >  	 */
> > -	int ret = processControls(&descriptor);
> > +	int ret = processControls(descriptor.get());
> >  	if (ret)
> >  		return ret;
> >
> > @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		state_ = State::Running;
> >  	}
> >
> > -	worker_.queueRequest(descriptor.request_.get());
> > +	worker_.queueRequest(descriptor->request_.get());
> >
> >  	{
> >  		MutexLocker descriptorsLock(descriptorsMutex_);
> > -		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> > +		descriptors_.push_back(std::move(descriptor));
> >  	}
>
> We have a race condition here, worker_.queueRequest() should go after
> adding the request to the queue. Could you fix it in a patch on top ?
>
> >
> >  	return 0;
> > @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >
> >  void CameraDevice::requestComplete(Request *request)
> >  {
> > -	decltype(descriptors_)::node_type node;
> > -	{
> > -		MutexLocker descriptorsLock(descriptorsMutex_);
> > -		auto it = descriptors_.find(request->cookie());
> > -		if (it == descriptors_.end()) {
> > -			/*
> > -			 * \todo Clarify if the Camera has to be closed on
> > -			 * ERROR_DEVICE and possibly demote the Fatal to simple
> > -			 * Error.
> > -			 */
> > -			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> > -			LOG(HAL, Fatal)
> > -				<< "Unknown request: " << request->cookie();
> > +	if (descriptors_.empty())
> > +		return;
> >
> > -			return;
> > -		}
> > +	Camera3RequestDescriptor *descriptor = descriptors_.front().get();
>
> This needs to be protected by descriptorsMutex_.
>
> 	Camera3RequestDescriptor *descriptor;
>
> 	{
> 		MutexLocker descriptorsLock(descriptorsMutex_);
> 		descriptor = descriptors_.front().get();
> 	}

Mmm, is this enough ?

We would get a reference to a unique_ptr<> stored in the global
descriptors_, which a concurrent call to configureStreams() (which
calls stop() and then descriptors_.clear()) or to close() (which again
calls stop()) would invalidate.

I think we should either:
1) Lock the whole requestComplete() function
   which is bad, as it could take a while. However if we'll get to a
   point where we can interrupt async processing could be acceptable

2) Extract the descriptor from the queue and handle its lifetime
   inside this function, which I'm not yet clear on what implies.

That said, if we're going to move descriptor away from the queue, we
could as well create a dedicated struct/class with a different queue
if it turns out to be simpler.

>
> > +	if (descriptor->request_->cookie() != request->cookie()) {
>
> This is correct as long as we handle post-processing synchronously.
> Let's see how it evolves in subsequent patches.
>
> > +		/*
> > +		 * \todo Clarify if the Camera has to be closed on
> > +		 * ERROR_DEVICE and possibly demote the Fatal to simple
> > +		 * Error.
> > +		 */
> > +		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> > +		LOG(HAL, Fatal)
> > +			<< "Unknown request: " << request->cookie();
>
> I'd change the message to
>
> 			<< "Out-of-order completion for request "
> 			<< request->cookie();
>
> By the way, with the cookie containing a pointer, I think it would be
> more readable in hex. Maybe a patch on top to use utils::hex() ?
>
> >
> > -		node = descriptors_.extract(it);
> > +		return;
> >  	}
> > -	Camera3RequestDescriptor &descriptor = node.mapped();
> >
> >  	/*
> >  	 * Prepare the capture result for the Android camera stack.
> > @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
> >  	 * post-processing/compression fails.
> >  	 */
> >  	camera3_capture_result_t captureResult = {};
> > -	captureResult.frame_number = descriptor.frameNumber_;
> > -	captureResult.num_output_buffers = descriptor.buffers_.size();
> > -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > +	captureResult.frame_number = descriptor->frameNumber_;
> > +	captureResult.num_output_buffers = descriptor->buffers_.size();
> > +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >  		buffer.acquire_fence = -1;
> >  		buffer.release_fence = -1;
> >  		buffer.status = CAMERA3_BUFFER_STATUS_OK;
> >  	}
> > -	captureResult.output_buffers = descriptor.buffers_.data();
> > +	captureResult.output_buffers = descriptor->buffers_.data();
> >  	captureResult.partial_result = 1;
> >
> >  	/*
> > @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
> >  				<< " not successfully completed: "
> >  				<< request->status();
> >
> > -		notifyError(descriptor.frameNumber_, nullptr,
> > +		notifyError(descriptor->frameNumber_, nullptr,
> >  			    CAMERA3_MSG_ERROR_REQUEST);
> >
> >  		captureResult.partial_result = 0;
> > -		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> > +		for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
> >  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >  		callbacks_->process_capture_result(callbacks_, &captureResult);
> >
> > +		descriptors_.pop_front();
>
> I'm slightly concerned that in some paths we could complete the request
> but forget to remove it from the queue. Maybe wrapping
> callbacks_->process_capture_result() and descriptors_.pop_front() in a
> function would be good. Let's see how it looks like with the whole
> series applied.
>
> >  		return;
> >  	}
> >
> > @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
> >  	 */
> >  	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> >  							 .get(controls::SensorTimestamp));
> > -	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> > +	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >
> >  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> > -			<< descriptor.buffers_.size() << " streams";
> > +			<< descriptor->buffers_.size() << " streams";
> >
> >  	/*
> >  	 * Generate the metadata associated with the captured buffers.
> > @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
> >  	 */
> >  	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> >  	if (!resultMetadata) {
> > -		notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> > +		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >
> >  		/* The camera framework expects an empty metadata pack on error. */
> >  		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> >  	}
> >
> >  	/* Handle any JPEG compression. */
> > -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >  		CameraStream *cameraStream =
> >  			static_cast<CameraStream *>(buffer.stream->priv);
> >
> > @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
> >  		if (!src) {
> >  			LOG(HAL, Error) << "Failed to find a source stream buffer";
> >  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > -			notifyError(descriptor.frameNumber_, buffer.stream,
> > +			notifyError(descriptor->frameNumber_, buffer.stream,
> >  				    CAMERA3_MSG_ERROR_BUFFER);
> >  			continue;
> >  		}
> >
> >  		int ret = cameraStream->process(*src, *buffer.buffer,
> > -						descriptor.settings_,
> > +						descriptor->settings_,
> >  						resultMetadata.get());
> >  		/*
> >  		 * Return the FrameBuffer to the CameraStream now that we're
> > @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
> >
> >  		if (ret) {
> >  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > -			notifyError(descriptor.frameNumber_, buffer.stream,
> > +			notifyError(descriptor->frameNumber_, buffer.stream,
> >  				    CAMERA3_MSG_ERROR_BUFFER);
> >  		}
> >  	}
> >
> >  	captureResult.result = resultMetadata->get();
> >  	callbacks_->process_capture_result(callbacks_, &captureResult);
> > +
> > +	descriptors_.pop_front();
> >  }
> >
> >  std::string CameraDevice::logPrefix() const
> > @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> >   * Produce a set of fixed result metadata.
> >   */
> >  std::unique_ptr<CameraMetadata>
> > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
> > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
> >  {
> > -	const ControlList &metadata = descriptor.request_->metadata();
> > -	const CameraMetadata &settings = descriptor.settings_;
> > +	const ControlList &metadata = descriptor->request_->metadata();
> > +	const CameraMetadata &settings = descriptor->settings_;
> >  	camera_metadata_ro_entry_t entry;
> >  	bool found;
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index a5576927..9c895b25 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __ANDROID_CAMERA_DEVICE_H__
> >  #define __ANDROID_CAMERA_DEVICE_H__
> >
> > +#include <deque>
> >  #include <map>
> >  #include <memory>
> >  #include <mutex>
> > @@ -103,7 +104,7 @@ private:
> >  			 camera3_error_msg_code code);
> >  	int processControls(Camera3RequestDescriptor *descriptor);
> >  	std::unique_ptr<CameraMetadata> getResultMetadata(
> > -		const Camera3RequestDescriptor &descriptor) const;
> > +		const Camera3RequestDescriptor *descriptor) const;
> >
> >  	unsigned int id_;
> >  	camera3_device_t camera3Device_;
> > @@ -123,7 +124,7 @@ private:
> >  	std::vector<CameraStream> streams_;
> >
> >  	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> > -	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> > +	std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
>
> Could we use a std::queue instead ? It will be implemented on top of a
> std::deque, so there will be no change in performances, but it gives us
> the semantics we need (essentially, a FIFO).
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >
> >  	std::string maker_;
> >  	std::string model_;
>
> --
> Regards,
>
> Laurent Pinchart

Hi Umang,

On Tue, Sep 21, 2021 at 09:56:22AM +0530, Umang Jain wrote:
> On 9/21/21 12:36 AM, Laurent Pinchart wrote:
> > On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
> >> The descriptors_ map hold Camera3RequestDescriptor(s) which are
> >> per-capture request placed by the framework to libcamera HAL.
> >> CameraDevice::requestComplete() looks for the descriptor for which the
> >> camera request has been completed and removes it from the map.
> >> However, this approach has its limitations going forwards.
> >>
> >> In subsequent commits, the post-processing operation which happens
> >> in requestComplete() synchronously, is going to be run asynchronously.
> >> Therefore, instead of a map for descriptors, a queue makes more sense
> >> going forwards and the framework expects capture results to be received
> >> in the same order as they were queued. When the async processing is
> >> completed, the descriptor queue is inspected to send back the capture
> >> results and then de-queued. This helps to maintain the order of sending
> >> back the capture results whilst preventing unnecessary complexity of
> >> using a map.
> >>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
> >>   src/android/camera_device.h   |  5 +-
> >>   2 files changed, 48 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index f461e14c..0562c225 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   	 * The descriptor and the associated memory reserved here are freed
> >>   	 * at request complete time.
> >>   	 */
> >> -	Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> >> +	std::unique_ptr<Camera3RequestDescriptor> descriptor =
> >> +		std::make_unique<Camera3RequestDescriptor>(camera_.get(),
> >> +							   camera3Request);
> >>   
> >>   	/*
> >>   	 * \todo The Android request model is incremental, settings passed in
> >> @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   	if (camera3Request->settings)
> >>   		lastSettings_ = camera3Request->settings;
> >>   	else
> >> -		descriptor.settings_ = lastSettings_;
> >> +		descriptor->settings_ = lastSettings_;
> >>   
> >> -	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> >> -			<< " with " << descriptor.buffers_.size() << " streams";
> >> -	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> >> -		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> >> +	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> >> +			<< " with " << descriptor->buffers_.size() << " streams";
> >> +	for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> >> +		const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
> >>   		camera3_stream *camera3Stream = camera3Buffer.stream;
> >>   		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> >>   
> >> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   			buffer = createFrameBuffer(*camera3Buffer.buffer,
> >>   						   cameraStream->configuration().pixelFormat,
> >>   						   cameraStream->configuration().size);
> >> -			descriptor.frameBuffers_.emplace_back(buffer);
> >> +			descriptor->frameBuffers_.emplace_back(buffer);
> >>   			LOG(HAL, Debug) << ss.str() << " (direct)";
> >>   			break;
> >>   
> >> @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   			return -ENOMEM;
> >>   		}
> >>   
> >> -		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> >> +		descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> >>   						camera3Buffer.acquire_fence);
> >>   	}
> >>   
> >> @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   	 * Translate controls from Android to libcamera and queue the request
> >>   	 * to the CameraWorker thread.
> >>   	 */
> >> -	int ret = processControls(&descriptor);
> >> +	int ret = processControls(descriptor.get());
> >>   	if (ret)
> >>   		return ret;
> >>   
> >> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   		state_ = State::Running;
> >>   	}
> >>   
> >> -	worker_.queueRequest(descriptor.request_.get());
> >> +	worker_.queueRequest(descriptor->request_.get());
> >>   
> >>   	{
> >>   		MutexLocker descriptorsLock(descriptorsMutex_);
> >> -		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> >> +		descriptors_.push_back(std::move(descriptor));
> >>   	}
> >
> > We have a race condition here, worker_.queueRequest() should go after
> > adding the request to the queue. Could you fix it in a patch on top ?
> 
> Do you mean the race condition is existing already, with the 
> descriptors_ map (that has been removed from this patch)?

Correct, it's already here.

> Yes, I can introduce a patch before this one, that fixes the race first 
> in the map itself. Is my understanding correct?

Sounds good to me. It should be a small patch.

> >>   
> >>   	return 0;
> >> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   
> >>   void CameraDevice::requestComplete(Request *request)
> >>   {
> >> -	decltype(descriptors_)::node_type node;
> >> -	{
> >> -		MutexLocker descriptorsLock(descriptorsMutex_);
> >> -		auto it = descriptors_.find(request->cookie());
> >> -		if (it == descriptors_.end()) {
> >> -			/*
> >> -			 * \todo Clarify if the Camera has to be closed on
> >> -			 * ERROR_DEVICE and possibly demote the Fatal to simple
> >> -			 * Error.
> >> -			 */
> >> -			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> >> -			LOG(HAL, Fatal)
> >> -				<< "Unknown request: " << request->cookie();
> >> +	if (descriptors_.empty())
> >> +		return;
> >>   
> >> -			return;
> >> -		}
> >> +	Camera3RequestDescriptor *descriptor = descriptors_.front().get();
> >
> > This needs to be protected by descriptorsMutex_.
> >
> > 	Camera3RequestDescriptor *descriptor;
> >
> > 	{
> > 		MutexLocker descriptorsLock(descriptorsMutex_);
> > 		descriptor = descriptors_.front().get();
> > 	}
> >
> >> +	if (descriptor->request_->cookie() != request->cookie()) {
> >
> > This is correct as long as we handle post-processing synchronously.
> > Let's see how it evolves in subsequent patches.
> 
> Why not valid for async post-processing?
> 
> So this is requestComplete() function, invoked whenever a 
> libcamera::Request is completed by libcamera::Camera. The completion is 
> guaranteed to be done in order, right ? Later in this function, the 
> post-processing shall happen (sync or async).

When we'll move to async post-processing, the request at the front of
the queue will be a request undergoing post-processing. libcamera may
signal completion of the next request before the post-processing is
complete, so the check will fail.

> >> +		/*
> >> +		 * \todo Clarify if the Camera has to be closed on
> >> +		 * ERROR_DEVICE and possibly demote the Fatal to simple
> >> +		 * Error.
> >> +		 */
> >> +		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> >> +		LOG(HAL, Fatal)
> >> +			<< "Unknown request: " << request->cookie();
> >
> > I'd change the message to
> >
> > 			<< "Out-of-order completion for request "
> > 			<< request->cookie();
> >
> > By the way, with the cookie containing a pointer, I think it would be
> > more readable in hex. Maybe a patch on top to use utils::hex() ?
> 
> Makes sense, I'll double check
> 
> >>   
> >> -		node = descriptors_.extract(it);
> >> +		return;
> >>   	}
> >> -	Camera3RequestDescriptor &descriptor = node.mapped();
> >>   
> >>   	/*
> >>   	 * Prepare the capture result for the Android camera stack.
> >> @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
> >>   	 * post-processing/compression fails.
> >>   	 */
> >>   	camera3_capture_result_t captureResult = {};
> >> -	captureResult.frame_number = descriptor.frameNumber_;
> >> -	captureResult.num_output_buffers = descriptor.buffers_.size();
> >> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> >> +	captureResult.frame_number = descriptor->frameNumber_;
> >> +	captureResult.num_output_buffers = descriptor->buffers_.size();
> >> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >>   		buffer.acquire_fence = -1;
> >>   		buffer.release_fence = -1;
> >>   		buffer.status = CAMERA3_BUFFER_STATUS_OK;
> >>   	}
> >> -	captureResult.output_buffers = descriptor.buffers_.data();
> >> +	captureResult.output_buffers = descriptor->buffers_.data();
> >>   	captureResult.partial_result = 1;
> >>   
> >>   	/*
> >> @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
> >>   				<< " not successfully completed: "
> >>   				<< request->status();
> >>   
> >> -		notifyError(descriptor.frameNumber_, nullptr,
> >> +		notifyError(descriptor->frameNumber_, nullptr,
> >>   			    CAMERA3_MSG_ERROR_REQUEST);
> >>   
> >>   		captureResult.partial_result = 0;
> >> -		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> >> +		for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
> >>   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>   		callbacks_->process_capture_result(callbacks_, &captureResult);
> >>   
> >> +		descriptors_.pop_front();
> >
> > I'm slightly concerned that in some paths we could complete the request
> > but forget to remove it from the queue. Maybe wrapping
> > callbacks_->process_capture_result() and descriptors_.pop_front() in a
> > function would be good. Let's see how it looks like with the whole
> > series applied.
> 
> This is a good point and has been address already via 
> sendCaptureResults() in subsequent patches.
> 
> However, this particular error path you have pointed out here, is the 
> /only/ part where queue.front() is dropped, other than ofcoourse in 
> sendCaptureResults(). hmm, I'll take a look if I can figure it out too 
> and have a singular place of processsing the queue.
> 
> >>   		return;
> >>   	}
> >>   
> >> @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
> >>   	 */
> >>   	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> >>   							 .get(controls::SensorTimestamp));
> >> -	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> >> +	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >>   
> >>   	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> >> -			<< descriptor.buffers_.size() << " streams";
> >> +			<< descriptor->buffers_.size() << " streams";
> >>   
> >>   	/*
> >>   	 * Generate the metadata associated with the captured buffers.
> >> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
> >>   	 */
> >>   	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> >>   	if (!resultMetadata) {
> >> -		notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >> +		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >>   
> >>   		/* The camera framework expects an empty metadata pack on error. */
> >>   		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> >>   	}
> >>   
> >>   	/* Handle any JPEG compression. */
> >> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> >> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >>   		CameraStream *cameraStream =
> >>   			static_cast<CameraStream *>(buffer.stream->priv);
> >>   
> >> @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
> >>   		if (!src) {
> >>   			LOG(HAL, Error) << "Failed to find a source stream buffer";
> >>   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >> -			notifyError(descriptor.frameNumber_, buffer.stream,
> >> +			notifyError(descriptor->frameNumber_, buffer.stream,
> >>   				    CAMERA3_MSG_ERROR_BUFFER);
> >>   			continue;
> >>   		}
> >>   
> >>   		int ret = cameraStream->process(*src, *buffer.buffer,
> >> -						descriptor.settings_,
> >> +						descriptor->settings_,
> >>   						resultMetadata.get());
> >>   		/*
> >>   		 * Return the FrameBuffer to the CameraStream now that we're
> >> @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
> >>   
> >>   		if (ret) {
> >>   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >> -			notifyError(descriptor.frameNumber_, buffer.stream,
> >> +			notifyError(descriptor->frameNumber_, buffer.stream,
> >>   				    CAMERA3_MSG_ERROR_BUFFER);
> >>   		}
> >>   	}
> >>   
> >>   	captureResult.result = resultMetadata->get();
> >>   	callbacks_->process_capture_result(callbacks_, &captureResult);
> >> +
> >> +	descriptors_.pop_front();
> >>   }
> >>   
> >>   std::string CameraDevice::logPrefix() const
> >> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> >>    * Produce a set of fixed result metadata.
> >>    */
> >>   std::unique_ptr<CameraMetadata>
> >> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
> >> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
> >>   {
> >> -	const ControlList &metadata = descriptor.request_->metadata();
> >> -	const CameraMetadata &settings = descriptor.settings_;
> >> +	const ControlList &metadata = descriptor->request_->metadata();
> >> +	const CameraMetadata &settings = descriptor->settings_;
> >>   	camera_metadata_ro_entry_t entry;
> >>   	bool found;
> >>   
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index a5576927..9c895b25 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -7,6 +7,7 @@
> >>   #ifndef __ANDROID_CAMERA_DEVICE_H__
> >>   #define __ANDROID_CAMERA_DEVICE_H__
> >>   
> >> +#include <deque>
> >>   #include <map>
> >>   #include <memory>
> >>   #include <mutex>
> >> @@ -103,7 +104,7 @@ private:
> >>   			 camera3_error_msg_code code);
> >>   	int processControls(Camera3RequestDescriptor *descriptor);
> >>   	std::unique_ptr<CameraMetadata> getResultMetadata(
> >> -		const Camera3RequestDescriptor &descriptor) const;
> >> +		const Camera3RequestDescriptor *descriptor) const;
> >>   
> >>   	unsigned int id_;
> >>   	camera3_device_t camera3Device_;
> >> @@ -123,7 +124,7 @@ private:
> >>   	std::vector<CameraStream> streams_;
> >>   
> >>   	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> >> -	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> >> +	std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> >
> > Could we use a std::queue instead ? It will be implemented on top of a
> > std::deque, so there will be no change in performances, but it gives us
> > the semantics we need (essentially, a FIFO).
> 
> Yes, great! The deque is coming from earlier version where we need to 
> iterate over the queue.
> 
> I see, no place as such in v3, where the queue is iterated upon, so we 
> can surely use std::queue.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> >>   	std::string maker_;
> >>   	std::string model_;

Hi Jacopo,

On Tue, Sep 21, 2021 at 01:05:36PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 20, 2021 at 10:06:32PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
> > > The descriptors_ map hold Camera3RequestDescriptor(s) which are
> > > per-capture request placed by the framework to libcamera HAL.
> > > CameraDevice::requestComplete() looks for the descriptor for which the
> > > camera request has been completed and removes it from the map.
> > > However, this approach has its limitations going forwards.
> > >
> > > In subsequent commits, the post-processing operation which happens
> > > in requestComplete() synchronously, is going to be run asynchronously.
> > > Therefore, instead of a map for descriptors, a queue makes more sense
> > > going forwards and the framework expects capture results to be received
> > > in the same order as they were queued. When the async processing is
> > > completed, the descriptor queue is inspected to send back the capture
> > > results and then de-queued. This helps to maintain the order of sending
> > > back the capture results whilst preventing unnecessary complexity of
> > > using a map.
> > >
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >  src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
> > >  src/android/camera_device.h   |  5 +-
> > >  2 files changed, 48 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index f461e14c..0562c225 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  	 * The descriptor and the associated memory reserved here are freed
> > >  	 * at request complete time.
> > >  	 */
> > > -	Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> > > +	std::unique_ptr<Camera3RequestDescriptor> descriptor =
> > > +		std::make_unique<Camera3RequestDescriptor>(camera_.get(),
> > > +							   camera3Request);
> > >
> > >  	/*
> > >  	 * \todo The Android request model is incremental, settings passed in
> > > @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  	if (camera3Request->settings)
> > >  		lastSettings_ = camera3Request->settings;
> > >  	else
> > > -		descriptor.settings_ = lastSettings_;
> > > +		descriptor->settings_ = lastSettings_;
> > >
> > > -	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> > > -			<< " with " << descriptor.buffers_.size() << " streams";
> > > -	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> > > -		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> > > +	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> > > +			<< " with " << descriptor->buffers_.size() << " streams";
> > > +	for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> > > +		const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
> > >  		camera3_stream *camera3Stream = camera3Buffer.stream;
> > >  		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> > >
> > > @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  			buffer = createFrameBuffer(*camera3Buffer.buffer,
> > >  						   cameraStream->configuration().pixelFormat,
> > >  						   cameraStream->configuration().size);
> > > -			descriptor.frameBuffers_.emplace_back(buffer);
> > > +			descriptor->frameBuffers_.emplace_back(buffer);
> > >  			LOG(HAL, Debug) << ss.str() << " (direct)";
> > >  			break;
> > >
> > > @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  			return -ENOMEM;
> > >  		}
> > >
> > > -		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> > > +		descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> > >  						camera3Buffer.acquire_fence);
> > >  	}
> > >
> > > @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  	 * Translate controls from Android to libcamera and queue the request
> > >  	 * to the CameraWorker thread.
> > >  	 */
> > > -	int ret = processControls(&descriptor);
> > > +	int ret = processControls(descriptor.get());
> > >  	if (ret)
> > >  		return ret;
> > >
> > > @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  		state_ = State::Running;
> > >  	}
> > >
> > > -	worker_.queueRequest(descriptor.request_.get());
> > > +	worker_.queueRequest(descriptor->request_.get());
> > >
> > >  	{
> > >  		MutexLocker descriptorsLock(descriptorsMutex_);
> > > -		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> > > +		descriptors_.push_back(std::move(descriptor));
> > >  	}
> >
> > We have a race condition here, worker_.queueRequest() should go after
> > adding the request to the queue. Could you fix it in a patch on top ?
> >
> > >
> > >  	return 0;
> > > @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >
> > >  void CameraDevice::requestComplete(Request *request)
> > >  {
> > > -	decltype(descriptors_)::node_type node;
> > > -	{
> > > -		MutexLocker descriptorsLock(descriptorsMutex_);
> > > -		auto it = descriptors_.find(request->cookie());
> > > -		if (it == descriptors_.end()) {
> > > -			/*
> > > -			 * \todo Clarify if the Camera has to be closed on
> > > -			 * ERROR_DEVICE and possibly demote the Fatal to simple
> > > -			 * Error.
> > > -			 */
> > > -			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> > > -			LOG(HAL, Fatal)
> > > -				<< "Unknown request: " << request->cookie();
> > > +	if (descriptors_.empty())
> > > +		return;
> > >
> > > -			return;
> > > -		}
> > > +	Camera3RequestDescriptor *descriptor = descriptors_.front().get();
> >
> > This needs to be protected by descriptorsMutex_.
> >
> > 	Camera3RequestDescriptor *descriptor;
> >
> > 	{
> > 		MutexLocker descriptorsLock(descriptorsMutex_);
> > 		descriptor = descriptors_.front().get();
> > 	}
> 
> Mmm, is this enough ?
> 
> We would get a reference to a unique_ptr<> stored in the global
> descriptors_, which a concurrent call to configureStreams() (which
> calls stop() and then descriptors_.clear()) or to close() (which again
> calls stop()) would invalidate.
> 
> I think we should either:
> 1) Lock the whole requestComplete() function
>    which is bad, as it could take a while. However if we'll get to a
>    point where we can interrupt async processing could be acceptable
> 
> 2) Extract the descriptor from the queue and handle its lifetime
>    inside this function, which I'm not yet clear on what implies.
> 
> That said, if we're going to move descriptor away from the queue, we
> could as well create a dedicated struct/class with a different queue
> if it turns out to be simpler.

As discussed offline, stop() (and flush()) needs to wait for all pending
requests to complete (possibly with an error status for the requests
that get cancelled) before we can proceed. Otherwise, if we reported to
the camera service that all requests are completed while some are
actually in flight, the camera service could reuse (or free) buffers
that we're writing to.

> > > +	if (descriptor->request_->cookie() != request->cookie()) {
> >
> > This is correct as long as we handle post-processing synchronously.
> > Let's see how it evolves in subsequent patches.
> >
> > > +		/*
> > > +		 * \todo Clarify if the Camera has to be closed on
> > > +		 * ERROR_DEVICE and possibly demote the Fatal to simple
> > > +		 * Error.
> > > +		 */
> > > +		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> > > +		LOG(HAL, Fatal)
> > > +			<< "Unknown request: " << request->cookie();
> >
> > I'd change the message to
> >
> > 			<< "Out-of-order completion for request "
> > 			<< request->cookie();
> >
> > By the way, with the cookie containing a pointer, I think it would be
> > more readable in hex. Maybe a patch on top to use utils::hex() ?
> >
> > >
> > > -		node = descriptors_.extract(it);
> > > +		return;
> > >  	}
> > > -	Camera3RequestDescriptor &descriptor = node.mapped();
> > >
> > >  	/*
> > >  	 * Prepare the capture result for the Android camera stack.
> > > @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
> > >  	 * post-processing/compression fails.
> > >  	 */
> > >  	camera3_capture_result_t captureResult = {};
> > > -	captureResult.frame_number = descriptor.frameNumber_;
> > > -	captureResult.num_output_buffers = descriptor.buffers_.size();
> > > -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > > +	captureResult.frame_number = descriptor->frameNumber_;
> > > +	captureResult.num_output_buffers = descriptor->buffers_.size();
> > > +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > >  		buffer.acquire_fence = -1;
> > >  		buffer.release_fence = -1;
> > >  		buffer.status = CAMERA3_BUFFER_STATUS_OK;
> > >  	}
> > > -	captureResult.output_buffers = descriptor.buffers_.data();
> > > +	captureResult.output_buffers = descriptor->buffers_.data();
> > >  	captureResult.partial_result = 1;
> > >
> > >  	/*
> > > @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
> > >  				<< " not successfully completed: "
> > >  				<< request->status();
> > >
> > > -		notifyError(descriptor.frameNumber_, nullptr,
> > > +		notifyError(descriptor->frameNumber_, nullptr,
> > >  			    CAMERA3_MSG_ERROR_REQUEST);
> > >
> > >  		captureResult.partial_result = 0;
> > > -		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> > > +		for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
> > >  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > >  		callbacks_->process_capture_result(callbacks_, &captureResult);
> > >
> > > +		descriptors_.pop_front();
> >
> > I'm slightly concerned that in some paths we could complete the request
> > but forget to remove it from the queue. Maybe wrapping
> > callbacks_->process_capture_result() and descriptors_.pop_front() in a
> > function would be good. Let's see how it looks like with the whole
> > series applied.
> >
> > >  		return;
> > >  	}
> > >
> > > @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
> > >  	 */
> > >  	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> > >  							 .get(controls::SensorTimestamp));
> > > -	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> > > +	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> > >
> > >  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> > > -			<< descriptor.buffers_.size() << " streams";
> > > +			<< descriptor->buffers_.size() << " streams";
> > >
> > >  	/*
> > >  	 * Generate the metadata associated with the captured buffers.
> > > @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
> > >  	 */
> > >  	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> > >  	if (!resultMetadata) {
> > > -		notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> > > +		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> > >
> > >  		/* The camera framework expects an empty metadata pack on error. */
> > >  		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> > >  	}
> > >
> > >  	/* Handle any JPEG compression. */
> > > -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > > +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > >  		CameraStream *cameraStream =
> > >  			static_cast<CameraStream *>(buffer.stream->priv);
> > >
> > > @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
> > >  		if (!src) {
> > >  			LOG(HAL, Error) << "Failed to find a source stream buffer";
> > >  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > > -			notifyError(descriptor.frameNumber_, buffer.stream,
> > > +			notifyError(descriptor->frameNumber_, buffer.stream,
> > >  				    CAMERA3_MSG_ERROR_BUFFER);
> > >  			continue;
> > >  		}
> > >
> > >  		int ret = cameraStream->process(*src, *buffer.buffer,
> > > -						descriptor.settings_,
> > > +						descriptor->settings_,
> > >  						resultMetadata.get());
> > >  		/*
> > >  		 * Return the FrameBuffer to the CameraStream now that we're
> > > @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
> > >
> > >  		if (ret) {
> > >  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > > -			notifyError(descriptor.frameNumber_, buffer.stream,
> > > +			notifyError(descriptor->frameNumber_, buffer.stream,
> > >  				    CAMERA3_MSG_ERROR_BUFFER);
> > >  		}
> > >  	}
> > >
> > >  	captureResult.result = resultMetadata->get();
> > >  	callbacks_->process_capture_result(callbacks_, &captureResult);
> > > +
> > > +	descriptors_.pop_front();
> > >  }
> > >
> > >  std::string CameraDevice::logPrefix() const
> > > @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> > >   * Produce a set of fixed result metadata.
> > >   */
> > >  std::unique_ptr<CameraMetadata>
> > > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
> > > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
> > >  {
> > > -	const ControlList &metadata = descriptor.request_->metadata();
> > > -	const CameraMetadata &settings = descriptor.settings_;
> > > +	const ControlList &metadata = descriptor->request_->metadata();
> > > +	const CameraMetadata &settings = descriptor->settings_;
> > >  	camera_metadata_ro_entry_t entry;
> > >  	bool found;
> > >
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index a5576927..9c895b25 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -7,6 +7,7 @@
> > >  #ifndef __ANDROID_CAMERA_DEVICE_H__
> > >  #define __ANDROID_CAMERA_DEVICE_H__
> > >
> > > +#include <deque>
> > >  #include <map>
> > >  #include <memory>
> > >  #include <mutex>
> > > @@ -103,7 +104,7 @@ private:
> > >  			 camera3_error_msg_code code);
> > >  	int processControls(Camera3RequestDescriptor *descriptor);
> > >  	std::unique_ptr<CameraMetadata> getResultMetadata(
> > > -		const Camera3RequestDescriptor &descriptor) const;
> > > +		const Camera3RequestDescriptor *descriptor) const;
> > >
> > >  	unsigned int id_;
> > >  	camera3_device_t camera3Device_;
> > > @@ -123,7 +124,7 @@ private:
> > >  	std::vector<CameraStream> streams_;
> > >
> > >  	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> > > -	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> > > +	std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> >
> > Could we use a std::queue instead ? It will be implemented on top of a
> > std::deque, so there will be no change in performances, but it gives us
> > the semantics we need (essentially, a FIFO).
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > >
> > >  	std::string maker_;
> > >  	std::string model_;

Hi Laurent,

Thanks for the review.

On 9/22/21 3:46 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Tue, Sep 21, 2021 at 09:56:22AM +0530, Umang Jain wrote:
>> On 9/21/21 12:36 AM, Laurent Pinchart wrote:
>>> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
>>>> The descriptors_ map hold Camera3RequestDescriptor(s) which are
>>>> per-capture request placed by the framework to libcamera HAL.
>>>> CameraDevice::requestComplete() looks for the descriptor for which the
>>>> camera request has been completed and removes it from the map.
>>>> However, this approach has its limitations going forwards.
>>>>
>>>> In subsequent commits, the post-processing operation which happens
>>>> in requestComplete() synchronously, is going to be run asynchronously.
>>>> Therefore, instead of a map for descriptors, a queue makes more sense
>>>> going forwards and the framework expects capture results to be received
>>>> in the same order as they were queued. When the async processing is
>>>> completed, the descriptor queue is inspected to send back the capture
>>>> results and then de-queued. This helps to maintain the order of sending
>>>> back the capture results whilst preventing unnecessary complexity of
>>>> using a map.
>>>>
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> ---
>>>>    src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
>>>>    src/android/camera_device.h   |  5 +-
>>>>    2 files changed, 48 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>>> index f461e14c..0562c225 100644
>>>> --- a/src/android/camera_device.cpp
>>>> +++ b/src/android/camera_device.cpp
>>>> @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    	 * The descriptor and the associated memory reserved here are freed
>>>>    	 * at request complete time.
>>>>    	 */
>>>> -	Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
>>>> +	std::unique_ptr<Camera3RequestDescriptor> descriptor =
>>>> +		std::make_unique<Camera3RequestDescriptor>(camera_.get(),
>>>> +							   camera3Request);
>>>>    
>>>>    	/*
>>>>    	 * \todo The Android request model is incremental, settings passed in
>>>> @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    	if (camera3Request->settings)
>>>>    		lastSettings_ = camera3Request->settings;
>>>>    	else
>>>> -		descriptor.settings_ = lastSettings_;
>>>> +		descriptor->settings_ = lastSettings_;
>>>>    
>>>> -	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
>>>> -			<< " with " << descriptor.buffers_.size() << " streams";
>>>> -	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
>>>> -		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
>>>> +	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
>>>> +			<< " with " << descriptor->buffers_.size() << " streams";
>>>> +	for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
>>>> +		const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
>>>>    		camera3_stream *camera3Stream = camera3Buffer.stream;
>>>>    		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
>>>>    
>>>> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    			buffer = createFrameBuffer(*camera3Buffer.buffer,
>>>>    						   cameraStream->configuration().pixelFormat,
>>>>    						   cameraStream->configuration().size);
>>>> -			descriptor.frameBuffers_.emplace_back(buffer);
>>>> +			descriptor->frameBuffers_.emplace_back(buffer);
>>>>    			LOG(HAL, Debug) << ss.str() << " (direct)";
>>>>    			break;
>>>>    
>>>> @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    			return -ENOMEM;
>>>>    		}
>>>>    
>>>> -		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
>>>> +		descriptor->request_->addBuffer(cameraStream->stream(), buffer,
>>>>    						camera3Buffer.acquire_fence);
>>>>    	}
>>>>    
>>>> @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    	 * Translate controls from Android to libcamera and queue the request
>>>>    	 * to the CameraWorker thread.
>>>>    	 */
>>>> -	int ret = processControls(&descriptor);
>>>> +	int ret = processControls(descriptor.get());
>>>>    	if (ret)
>>>>    		return ret;
>>>>    
>>>> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    		state_ = State::Running;
>>>>    	}
>>>>    
>>>> -	worker_.queueRequest(descriptor.request_.get());
>>>> +	worker_.queueRequest(descriptor->request_.get());
>>>>    
>>>>    	{
>>>>    		MutexLocker descriptorsLock(descriptorsMutex_);
>>>> -		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
>>>> +		descriptors_.push_back(std::move(descriptor));
>>>>    	}
>>> We have a race condition here, worker_.queueRequest() should go after
>>> adding the request to the queue. Could you fix it in a patch on top ?
>> Do you mean the race condition is existing already, with the
>> descriptors_ map (that has been removed from this patch)?
> Correct, it's already here.
>
>> Yes, I can introduce a patch before this one, that fixes the race first
>> in the map itself. Is my understanding correct?
> Sounds good to me. It should be a small patch.
>
>>>>    
>>>>    	return 0;
>>>> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    
>>>>    void CameraDevice::requestComplete(Request *request)
>>>>    {
>>>> -	decltype(descriptors_)::node_type node;
>>>> -	{
>>>> -		MutexLocker descriptorsLock(descriptorsMutex_);
>>>> -		auto it = descriptors_.find(request->cookie());
>>>> -		if (it == descriptors_.end()) {
>>>> -			/*
>>>> -			 * \todo Clarify if the Camera has to be closed on
>>>> -			 * ERROR_DEVICE and possibly demote the Fatal to simple
>>>> -			 * Error.
>>>> -			 */
>>>> -			notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
>>>> -			LOG(HAL, Fatal)
>>>> -				<< "Unknown request: " << request->cookie();
>>>> +	if (descriptors_.empty())
>>>> +		return;
>>>>    
>>>> -			return;
>>>> -		}
>>>> +	Camera3RequestDescriptor *descriptor = descriptors_.front().get();
>>> This needs to be protected by descriptorsMutex_.
>>>
>>> 	Camera3RequestDescriptor *descriptor;
>>>
>>> 	{
>>> 		MutexLocker descriptorsLock(descriptorsMutex_);
>>> 		descriptor = descriptors_.front().get();
>>> 	}
>>>
>>>> +	if (descriptor->request_->cookie() != request->cookie()) {
>>> This is correct as long as we handle post-processing synchronously.
>>> Let's see how it evolves in subsequent patches.
>> Why not valid for async post-processing?
>>
>> So this is requestComplete() function, invoked whenever a
>> libcamera::Request is completed by libcamera::Camera. The completion is
>> guaranteed to be done in order, right ? Later in this function, the
>> post-processing shall happen (sync or async).
> When we'll move to async post-processing, the request at the front of
> the queue will be a request undergoing post-processing. libcamera may
> signal completion of the next request before the post-processing is
> complete, so the check will fail.


Yes, you are right, my bad :S

>
>>>> +		/*
>>>> +		 * \todo Clarify if the Camera has to be closed on
>>>> +		 * ERROR_DEVICE and possibly demote the Fatal to simple
>>>> +		 * Error.
>>>> +		 */
>>>> +		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
>>>> +		LOG(HAL, Fatal)
>>>> +			<< "Unknown request: " << request->cookie();
>>> I'd change the message to
>>>
>>> 			<< "Out-of-order completion for request "
>>> 			<< request->cookie();
>>>
>>> By the way, with the cookie containing a pointer, I think it would be
>>> more readable in hex. Maybe a patch on top to use utils::hex() ?
>> Makes sense, I'll double check
>>
>>>>    
>>>> -		node = descriptors_.extract(it);
>>>> +		return;
>>>>    	}
>>>> -	Camera3RequestDescriptor &descriptor = node.mapped();
>>>>    
>>>>    	/*
>>>>    	 * Prepare the capture result for the Android camera stack.
>>>> @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
>>>>    	 * post-processing/compression fails.
>>>>    	 */
>>>>    	camera3_capture_result_t captureResult = {};
>>>> -	captureResult.frame_number = descriptor.frameNumber_;
>>>> -	captureResult.num_output_buffers = descriptor.buffers_.size();
>>>> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>>>> +	captureResult.frame_number = descriptor->frameNumber_;
>>>> +	captureResult.num_output_buffers = descriptor->buffers_.size();
>>>> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>>>    		buffer.acquire_fence = -1;
>>>>    		buffer.release_fence = -1;
>>>>    		buffer.status = CAMERA3_BUFFER_STATUS_OK;
>>>>    	}
>>>> -	captureResult.output_buffers = descriptor.buffers_.data();
>>>> +	captureResult.output_buffers = descriptor->buffers_.data();
>>>>    	captureResult.partial_result = 1;
>>>>    
>>>>    	/*
>>>> @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
>>>>    				<< " not successfully completed: "
>>>>    				<< request->status();
>>>>    
>>>> -		notifyError(descriptor.frameNumber_, nullptr,
>>>> +		notifyError(descriptor->frameNumber_, nullptr,
>>>>    			    CAMERA3_MSG_ERROR_REQUEST);
>>>>    
>>>>    		captureResult.partial_result = 0;
>>>> -		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
>>>> +		for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
>>>>    			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>>>    		callbacks_->process_capture_result(callbacks_, &captureResult);
>>>>    
>>>> +		descriptors_.pop_front();
>>> I'm slightly concerned that in some paths we could complete the request
>>> but forget to remove it from the queue. Maybe wrapping
>>> callbacks_->process_capture_result() and descriptors_.pop_front() in a
>>> function would be good. Let's see how it looks like with the whole
>>> series applied.
>> This is a good point and has been address already via
>> sendCaptureResults() in subsequent patches.
>>
>> However, this particular error path you have pointed out here, is the
>> /only/ part where queue.front() is dropped, other than ofcoourse in
>> sendCaptureResults(). hmm, I'll take a look if I can figure it out too
>> and have a singular place of processsing the queue.
>>
>>>>    		return;
>>>>    	}
>>>>    
>>>> @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
>>>>    	 */
>>>>    	uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
>>>>    							 .get(controls::SensorTimestamp));
>>>> -	notifyShutter(descriptor.frameNumber_, sensorTimestamp);
>>>> +	notifyShutter(descriptor->frameNumber_, sensorTimestamp);
>>>>    
>>>>    	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
>>>> -			<< descriptor.buffers_.size() << " streams";
>>>> +			<< descriptor->buffers_.size() << " streams";
>>>>    
>>>>    	/*
>>>>    	 * Generate the metadata associated with the captured buffers.
>>>> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
>>>>    	 */
>>>>    	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
>>>>    	if (!resultMetadata) {
>>>> -		notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>>>> +		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>>>>    
>>>>    		/* The camera framework expects an empty metadata pack on error. */
>>>>    		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
>>>>    	}
>>>>    
>>>>    	/* Handle any JPEG compression. */
>>>> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>>>> +	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>>>    		CameraStream *cameraStream =
>>>>    			static_cast<CameraStream *>(buffer.stream->priv);
>>>>    
>>>> @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
>>>>    		if (!src) {
>>>>    			LOG(HAL, Error) << "Failed to find a source stream buffer";
>>>>    			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>>> -			notifyError(descriptor.frameNumber_, buffer.stream,
>>>> +			notifyError(descriptor->frameNumber_, buffer.stream,
>>>>    				    CAMERA3_MSG_ERROR_BUFFER);
>>>>    			continue;
>>>>    		}
>>>>    
>>>>    		int ret = cameraStream->process(*src, *buffer.buffer,
>>>> -						descriptor.settings_,
>>>> +						descriptor->settings_,
>>>>    						resultMetadata.get());
>>>>    		/*
>>>>    		 * Return the FrameBuffer to the CameraStream now that we're
>>>> @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
>>>>    
>>>>    		if (ret) {
>>>>    			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>>> -			notifyError(descriptor.frameNumber_, buffer.stream,
>>>> +			notifyError(descriptor->frameNumber_, buffer.stream,
>>>>    				    CAMERA3_MSG_ERROR_BUFFER);
>>>>    		}
>>>>    	}
>>>>    
>>>>    	captureResult.result = resultMetadata->get();
>>>>    	callbacks_->process_capture_result(callbacks_, &captureResult);
>>>> +
>>>> +	descriptors_.pop_front();
>>>>    }
>>>>    
>>>>    std::string CameraDevice::logPrefix() const
>>>> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>>>>     * Produce a set of fixed result metadata.
>>>>     */
>>>>    std::unique_ptr<CameraMetadata>
>>>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
>>>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
>>>>    {
>>>> -	const ControlList &metadata = descriptor.request_->metadata();
>>>> -	const CameraMetadata &settings = descriptor.settings_;
>>>> +	const ControlList &metadata = descriptor->request_->metadata();
>>>> +	const CameraMetadata &settings = descriptor->settings_;
>>>>    	camera_metadata_ro_entry_t entry;
>>>>    	bool found;
>>>>    
>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>>> index a5576927..9c895b25 100644
>>>> --- a/src/android/camera_device.h
>>>> +++ b/src/android/camera_device.h
>>>> @@ -7,6 +7,7 @@
>>>>    #ifndef __ANDROID_CAMERA_DEVICE_H__
>>>>    #define __ANDROID_CAMERA_DEVICE_H__
>>>>    
>>>> +#include <deque>
>>>>    #include <map>
>>>>    #include <memory>
>>>>    #include <mutex>
>>>> @@ -103,7 +104,7 @@ private:
>>>>    			 camera3_error_msg_code code);
>>>>    	int processControls(Camera3RequestDescriptor *descriptor);
>>>>    	std::unique_ptr<CameraMetadata> getResultMetadata(
>>>> -		const Camera3RequestDescriptor &descriptor) const;
>>>> +		const Camera3RequestDescriptor *descriptor) const;
>>>>    
>>>>    	unsigned int id_;
>>>>    	camera3_device_t camera3Device_;
>>>> @@ -123,7 +124,7 @@ private:
>>>>    	std::vector<CameraStream> streams_;
>>>>    
>>>>    	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
>>>> -	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
>>>> +	std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
>>> Could we use a std::queue instead ? It will be implemented on top of a
>>> std::deque, so there will be no change in performances, but it gives us
>>> the semantics we need (essentially, a FIFO).
>> Yes, great! The deque is coming from earlier version where we need to
>> iterate over the queue.
>>
>> I see, no place as such in v3, where the queue is iterated upon, so we
>> can surely use std::queue.
>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>>>    	std::string maker_;
>>>>    	std::string model_;

Hi Umang, thank you for the patch.

On Wed, Sep 22, 2021 at 5:28 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Hi Laurent,
>
> Thanks for the review.
>
> On 9/22/21 3:46 AM, Laurent Pinchart wrote:
> > Hi Umang,
> >
> > On Tue, Sep 21, 2021 at 09:56:22AM +0530, Umang Jain wrote:
> >> On 9/21/21 12:36 AM, Laurent Pinchart wrote:
> >>> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
> >>>> The descriptors_ map hold Camera3RequestDescriptor(s) which are
> >>>> per-capture request placed by the framework to libcamera HAL.
> >>>> CameraDevice::requestComplete() looks for the descriptor for which the
> >>>> camera request has been completed and removes it from the map.
> >>>> However, this approach has its limitations going forwards.
> >>>>
> >>>> In subsequent commits, the post-processing operation which happens
> >>>> in requestComplete() synchronously, is going to be run asynchronously.
> >>>> Therefore, instead of a map for descriptors, a queue makes more sense
> >>>> going forwards and the framework expects capture results to be received
> >>>> in the same order as they were queued. When the async processing is
> >>>> completed, the descriptor queue is inspected to send back the capture
> >>>> results and then de-queued. This helps to maintain the order of sending
> >>>> back the capture results whilst preventing unnecessary complexity of
> >>>> using a map.
> >>>>
> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>>> ---
> >>>>    src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
> >>>>    src/android/camera_device.h   |  5 +-
> >>>>    2 files changed, 48 insertions(+), 46 deletions(-)
> >>>>
> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>>> index f461e14c..0562c225 100644
> >>>> --- a/src/android/camera_device.cpp
> >>>> +++ b/src/android/camera_device.cpp
> >>>> @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>             * The descriptor and the associated memory reserved here are freed
> >>>>             * at request complete time.
> >>>>             */
> >>>> -  Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> >>>> +  std::unique_ptr<Camera3RequestDescriptor> descriptor =
> >>>> +          std::make_unique<Camera3RequestDescriptor>(camera_.get(),
> >>>> +                                                     camera3Request);

nit: I would use auto here.

> >>>>
> >>>>            /*
> >>>>             * \todo The Android request model is incremental, settings passed in
> >>>> @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>            if (camera3Request->settings)
> >>>>                    lastSettings_ = camera3Request->settings;
> >>>>            else
> >>>> -          descriptor.settings_ = lastSettings_;
> >>>> +          descriptor->settings_ = lastSettings_;
> >>>>
> >>>> -  LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> >>>> -                  << " with " << descriptor.buffers_.size() << " streams";
> >>>> -  for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> >>>> -          const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> >>>> +  LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> >>>> +                  << " with " << descriptor->buffers_.size() << " streams";
> >>>> +  for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> >>>> +          const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
> >>>>                    camera3_stream *camera3Stream = camera3Buffer.stream;
> >>>>                    CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> >>>>
> >>>> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>                            buffer = createFrameBuffer(*camera3Buffer.buffer,
> >>>>                                                       cameraStream->configuration().pixelFormat,
> >>>>                                                       cameraStream->configuration().size);
> >>>> -                  descriptor.frameBuffers_.emplace_back(buffer);
> >>>> +                  descriptor->frameBuffers_.emplace_back(buffer);
> >>>>                            LOG(HAL, Debug) << ss.str() << " (direct)";
> >>>>                            break;
> >>>>
> >>>> @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>                            return -ENOMEM;
> >>>>                    }
> >>>>
> >>>> -          descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> >>>> +          descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> >>>>                                                    camera3Buffer.acquire_fence);
> >>>>            }
> >>>>
> >>>> @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>             * Translate controls from Android to libcamera and queue the request
> >>>>             * to the CameraWorker thread.
> >>>>             */
> >>>> -  int ret = processControls(&descriptor);
> >>>> +  int ret = processControls(descriptor.get());
> >>>>            if (ret)
> >>>>                    return ret;
> >>>>
> >>>> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>                    state_ = State::Running;
> >>>>            }
> >>>>
> >>>> -  worker_.queueRequest(descriptor.request_.get());
> >>>> +  worker_.queueRequest(descriptor->request_.get());
> >>>>
> >>>>            {
> >>>>                    MutexLocker descriptorsLock(descriptorsMutex_);
> >>>> -          descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> >>>> +          descriptors_.push_back(std::move(descriptor));
> >>>>            }
> >>> We have a race condition here, worker_.queueRequest() should go after
> >>> adding the request to the queue. Could you fix it in a patch on top ?
> >> Do you mean the race condition is existing already, with the
> >> descriptors_ map (that has been removed from this patch)?
> > Correct, it's already here.
> >
> >> Yes, I can introduce a patch before this one, that fixes the race first
> >> in the map itself. Is my understanding correct?
> > Sounds good to me. It should be a small patch.
> >
> >>>>
> >>>>            return 0;
> >>>> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>
> >>>>    void CameraDevice::requestComplete(Request *request)
> >>>>    {
> >>>> -  decltype(descriptors_)::node_type node;
> >>>> -  {
> >>>> -          MutexLocker descriptorsLock(descriptorsMutex_);
> >>>> -          auto it = descriptors_.find(request->cookie());
> >>>> -          if (it == descriptors_.end()) {
> >>>> -                  /*
> >>>> -                   * \todo Clarify if the Camera has to be closed on
> >>>> -                   * ERROR_DEVICE and possibly demote the Fatal to simple
> >>>> -                   * Error.
> >>>> -                   */
> >>>> -                  notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> >>>> -                  LOG(HAL, Fatal)
> >>>> -                          << "Unknown request: " << request->cookie();
> >>>> +  if (descriptors_.empty())
> >>>> +          return;
> >>>>
> >>>> -                  return;
> >>>> -          }
> >>>> +  Camera3RequestDescriptor *descriptor = descriptors_.front().get();
> >>> This needs to be protected by descriptorsMutex_.
> >>>
> >>>     Camera3RequestDescriptor *descriptor;
> >>>
> >>>     {
> >>>             MutexLocker descriptorsLock(descriptorsMutex_);
> >>>             descriptor = descriptors_.front().get();
> >>>     }
> >>>
> >>>> +  if (descriptor->request_->cookie() != request->cookie()) {
> >>> This is correct as long as we handle post-processing synchronously.
> >>> Let's see how it evolves in subsequent patches.
> >> Why not valid for async post-processing?
> >>
> >> So this is requestComplete() function, invoked whenever a
> >> libcamera::Request is completed by libcamera::Camera. The completion is
> >> guaranteed to be done in order, right ? Later in this function, the
> >> post-processing shall happen (sync or async).
> > When we'll move to async post-processing, the request at the front of
> > the queue will be a request undergoing post-processing. libcamera may
> > signal completion of the next request before the post-processing is
> > complete, so the check will fail.
>
>
> Yes, you are right, my bad :S
>
> >
> >>>> +          /*
> >>>> +           * \todo Clarify if the Camera has to be closed on
> >>>> +           * ERROR_DEVICE and possibly demote the Fatal to simple
> >>>> +           * Error.
> >>>> +           */
> >>>> +          notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> >>>> +          LOG(HAL, Fatal)
> >>>> +                  << "Unknown request: " << request->cookie();
> >>> I'd change the message to
> >>>
> >>>                     << "Out-of-order completion for request "
> >>>                     << request->cookie();
> >>>
> >>> By the way, with the cookie containing a pointer, I think it would be
> >>> more readable in hex. Maybe a patch on top to use utils::hex() ?
> >> Makes sense, I'll double check
> >>
> >>>>
> >>>> -          node = descriptors_.extract(it);
> >>>> +          return;
> >>>>            }
> >>>> -  Camera3RequestDescriptor &descriptor = node.mapped();
> >>>>
> >>>>            /*
> >>>>             * Prepare the capture result for the Android camera stack.
> >>>> @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
> >>>>             * post-processing/compression fails.
> >>>>             */
> >>>>            camera3_capture_result_t captureResult = {};
> >>>> -  captureResult.frame_number = descriptor.frameNumber_;
> >>>> -  captureResult.num_output_buffers = descriptor.buffers_.size();
> >>>> -  for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> >>>> +  captureResult.frame_number = descriptor->frameNumber_;
> >>>> +  captureResult.num_output_buffers = descriptor->buffers_.size();
> >>>> +  for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >>>>                    buffer.acquire_fence = -1;
> >>>>                    buffer.release_fence = -1;
> >>>>                    buffer.status = CAMERA3_BUFFER_STATUS_OK;
> >>>>            }
> >>>> -  captureResult.output_buffers = descriptor.buffers_.data();
> >>>> +  captureResult.output_buffers = descriptor->buffers_.data();
> >>>>            captureResult.partial_result = 1;
> >>>>
> >>>>            /*
> >>>> @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
> >>>>                                    << " not successfully completed: "
> >>>>                                    << request->status();
> >>>>
> >>>> -          notifyError(descriptor.frameNumber_, nullptr,
> >>>> +          notifyError(descriptor->frameNumber_, nullptr,
> >>>>                                CAMERA3_MSG_ERROR_REQUEST);
> >>>>
> >>>>                    captureResult.partial_result = 0;
> >>>> -          for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> >>>> +          for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
> >>>>                            buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>>                    callbacks_->process_capture_result(callbacks_, &captureResult);
> >>>>
> >>>> +          descriptors_.pop_front();
> >>> I'm slightly concerned that in some paths we could complete the request
> >>> but forget to remove it from the queue. Maybe wrapping
> >>> callbacks_->process_capture_result() and descriptors_.pop_front() in a
> >>> function would be good. Let's see how it looks like with the whole
> >>> series applied.
> >> This is a good point and has been address already via
> >> sendCaptureResults() in subsequent patches.
> >>
> >> However, this particular error path you have pointed out here, is the
> >> /only/ part where queue.front() is dropped, other than ofcoourse in
> >> sendCaptureResults(). hmm, I'll take a look if I can figure it out too
> >> and have a singular place of processsing the queue.
> >>
> >>>>                    return;
> >>>>            }
> >>>>
> >>>> @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
> >>>>             */
> >>>>            uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> >>>>                                                             .get(controls::SensorTimestamp));
> >>>> -  notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> >>>> +  notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >>>>
> >>>>            LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> >>>> -                  << descriptor.buffers_.size() << " streams";
> >>>> +                  << descriptor->buffers_.size() << " streams";
> >>>>
> >>>>            /*
> >>>>             * Generate the metadata associated with the captured buffers.
> >>>> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
> >>>>             */
> >>>>            std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> >>>>            if (!resultMetadata) {
> >>>> -          notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >>>> +          notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >>>>
> >>>>                    /* The camera framework expects an empty metadata pack on error. */
> >>>>                    resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> >>>>            }
> >>>>
> >>>>            /* Handle any JPEG compression. */
> >>>> -  for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> >>>> +  for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >>>>                    CameraStream *cameraStream =
> >>>>                            static_cast<CameraStream *>(buffer.stream->priv);
> >>>>
> >>>> @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
> >>>>                    if (!src) {
> >>>>                            LOG(HAL, Error) << "Failed to find a source stream buffer";
> >>>>                            buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> -                  notifyError(descriptor.frameNumber_, buffer.stream,
> >>>> +                  notifyError(descriptor->frameNumber_, buffer.stream,
> >>>>                                        CAMERA3_MSG_ERROR_BUFFER);
> >>>>                            continue;
> >>>>                    }
> >>>>
> >>>>                    int ret = cameraStream->process(*src, *buffer.buffer,
> >>>> -                                          descriptor.settings_,
> >>>> +                                          descriptor->settings_,
> >>>>                                                    resultMetadata.get());
> >>>>                    /*
> >>>>                     * Return the FrameBuffer to the CameraStream now that we're
> >>>> @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
> >>>>
> >>>>                    if (ret) {
> >>>>                            buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> -                  notifyError(descriptor.frameNumber_, buffer.stream,
> >>>> +                  notifyError(descriptor->frameNumber_, buffer.stream,
> >>>>                                        CAMERA3_MSG_ERROR_BUFFER);
> >>>>                    }
> >>>>            }
> >>>>
> >>>>            captureResult.result = resultMetadata->get();
> >>>>            callbacks_->process_capture_result(callbacks_, &captureResult);
> >>>> +
> >>>> +  descriptors_.pop_front();
> >>>>    }
> >>>>
> >>>>    std::string CameraDevice::logPrefix() const
> >>>> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> >>>>     * Produce a set of fixed result metadata.
> >>>>     */
> >>>>    std::unique_ptr<CameraMetadata>
> >>>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
> >>>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
> >>>>    {
> >>>> -  const ControlList &metadata = descriptor.request_->metadata();
> >>>> -  const CameraMetadata &settings = descriptor.settings_;
> >>>> +  const ControlList &metadata = descriptor->request_->metadata();
> >>>> +  const CameraMetadata &settings = descriptor->settings_;
> >>>>            camera_metadata_ro_entry_t entry;
> >>>>            bool found;
> >>>>
> >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >>>> index a5576927..9c895b25 100644
> >>>> --- a/src/android/camera_device.h
> >>>> +++ b/src/android/camera_device.h
> >>>> @@ -7,6 +7,7 @@
> >>>>    #ifndef __ANDROID_CAMERA_DEVICE_H__
> >>>>    #define __ANDROID_CAMERA_DEVICE_H__
> >>>>
> >>>> +#include <deque>
> >>>>    #include <map>
> >>>>    #include <memory>
> >>>>    #include <mutex>
> >>>> @@ -103,7 +104,7 @@ private:
> >>>>                             camera3_error_msg_code code);
> >>>>            int processControls(Camera3RequestDescriptor *descriptor);
> >>>>            std::unique_ptr<CameraMetadata> getResultMetadata(
> >>>> -          const Camera3RequestDescriptor &descriptor) const;
> >>>> +          const Camera3RequestDescriptor *descriptor) const;
> >>>>
> >>>>            unsigned int id_;
> >>>>            camera3_device_t camera3Device_;
> >>>> @@ -123,7 +124,7 @@ private:
> >>>>            std::vector<CameraStream> streams_;
> >>>>
> >>>>            libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> >>>> -  std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> >>>> +  std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> >>> Could we use a std::queue instead ? It will be implemented on top of a
> >>> std::deque, so there will be no change in performances, but it gives us
> >>> the semantics we need (essentially, a FIFO).
> >> Yes, great! The deque is coming from earlier version where we need to
> >> iterate over the queue.
> >>
> >> I see, no place as such in v3, where the queue is iterated upon, so we
> >> can surely use std::queue.
> >>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

All the problems I noticed have been pointed out by Laurent and Jacopo.
I look forward to the next version.

-Hiro
> >>>
> >>>>            std::string maker_;
> >>>>            std::string model_;