[libcamera-devel] [PATCH v2 3/3] android: camera_device: Send capture results by inspecting the queue

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Tue, Sep 28, 2021 at 09:55:36PM +0530, Umang Jain wrote:
> There is a possibility that an out-of-order completion of capture
> request happens by calling process_capture_result() directly on error
> paths. The framework expects that errors should be notified as soon as
> possible, but the request completion order should remain intact.
> An existing instance of this is abortRequest(), which sends the capture
> results on flushing state, without considering order-of-completion.
> 
> Since we have a queue of Camera3RequestDescriptor tracking each
> capture request placed by framework to libcamera HAL, we should be only
> sending back capture results from a single location, by inspecting
> the queue. As per the patch, this now happens in
> CameraDevice::sendCaptureResults().
> 
> Each descriptor is now equipped with its own status to denote whether
> the capture request is complete and ready to send back to the framework
> or needs to be waited upon. This ensures that the order of completion is
> respected for the requests.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 44 +++++++++++++++++++++++++----------
>  src/android/camera_device.h   | 15 +++++++++++-
>  2 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a3b8a549..83030366 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  	return 0;
>  }
>  
> -void CameraDevice::abortRequest(camera3_capture_request_t *request)
> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor,
> +				camera3_capture_request_t *request)

I don't think I've seen a reply to my review comment in v1:

Could this function take a Camera3RequestDescriptor pointer only ? It
should contain all the needed data. This can be done as a patch before
this one if desired, or here as it shouldn't be much extra work.

This function uses the num_output_buffers, frame_number and
output_buffers members of camera3_capture_request_t. Those are copied to
the Camera3RequestDescriptor buffers_ and frameNumber_ members which you
can use here.

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

>  {
>  	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>  
> -	camera3_capture_result_t result = {};
> +	camera3_capture_result_t &result = descriptor->captureResult_;
>  	result.num_output_buffers = request->num_output_buffers;
>  	result.frame_number = request->frame_number;
>  	result.partial_result = 0;
> @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)
>  	}
>  	result.output_buffers = resultBuffers.data();
>  
> -	callbacks_->process_capture_result(callbacks_, &result);
> +	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>  }
>  
>  bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> @@ -1052,13 +1053,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		return ret;
>  
>  	/*
> -	 * If flush is in progress abort the request. If the camera has been
> -	 * stopped we have to re-start it to be able to process the request.
> +	 * If flush is in progress abort the request and push the descriptor to
> +	 * the queue. If the camera has been stopped we have to re-start it to
> +	 * be able to process the request.
>  	 */
>  	MutexLocker stateLock(stateMutex_);
>  
>  	if (state_ == State::Flushing) {
> -		abortRequest(camera3Request);
> +		abortRequest(descriptors_.back().get(), camera3Request);
> +		{
> +			MutexLocker descriptorsLock(descriptorsMutex_);
> +			descriptors_.push(std::move(descriptor));
> +		}
> +		sendCaptureResults();
>  		return 0;
>  	}
>  
> @@ -1116,7 +1123,7 @@ void CameraDevice::requestComplete(Request *request)
>  	 * The buffer status is set to OK and later changed to ERROR if
>  	 * post-processing/compression fails.
>  	 */
> -	camera3_capture_result_t captureResult = {};
> +	camera3_capture_result_t &captureResult = descriptor->captureResult_;
>  	captureResult.frame_number = descriptor->frameNumber_;
>  	captureResult.num_output_buffers = descriptor->buffers_.size();
>  	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> @@ -1166,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request)
>  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>  		}
>  
> -		callbacks_->process_capture_result(callbacks_, &captureResult);
> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> +		sendCaptureResults();
>  
> -		descriptors_.pop();
>  		return;
>  	}
>  
> @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  
>  	captureResult.result = resultMetadata->get();
> -	callbacks_->process_capture_result(callbacks_, &captureResult);
> +	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> +	sendCaptureResults();
> +}
>  
> -	MutexLocker descriptorsLock(descriptorsMutex_);
> -	descriptors_.pop();
> +void CameraDevice::sendCaptureResults()
> +{
> +	MutexLocker lock(descriptorsMutex_);
> +	while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
> +		std::unique_ptr<Camera3RequestDescriptor> descriptor =
> +			std::move(descriptors_.front());
> +		descriptors_.pop();
> +
> +		lock.unlock();
> +		callbacks_->process_capture_result(callbacks_,
> +						   &descriptor->captureResult_);
> +		lock.lock();
> +	}
>  }
>  
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 9ec510d5..dbfa7431 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -74,17 +74,28 @@ private:
>  	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
>  
>  	struct Camera3RequestDescriptor {
> +		enum class Status {
> +			Pending,
> +			Success,
> +			Error,
> +		};
> +
>  		Camera3RequestDescriptor() = default;
>  		~Camera3RequestDescriptor() = default;
>  		Camera3RequestDescriptor(libcamera::Camera *camera,
>  					 const camera3_capture_request_t *camera3Request);
>  		Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
>  
> +		bool isPending() const { return status_ == Status::Pending; }
> +
>  		uint32_t frameNumber_ = 0;
>  		std::vector<camera3_stream_buffer_t> buffers_;
>  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>  		CameraMetadata settings_;
>  		std::unique_ptr<CaptureRequest> request_;
> +
> +		camera3_capture_result_t captureResult_ = {};
> +		Status status_ = Status::Pending;
>  	};
>  
>  	enum class State {
> @@ -99,12 +110,14 @@ private:
>  	createFrameBuffer(const buffer_handle_t camera3buffer,
>  			  libcamera::PixelFormat pixelFormat,
>  			  const libcamera::Size &size);
> -	void abortRequest(camera3_capture_request_t *request);
> +	void abortRequest(Camera3RequestDescriptor *descriptor,
> +			  camera3_capture_request_t *request);
>  	bool isValidRequest(camera3_capture_request_t *request) const;
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>  			 camera3_error_msg_code code);
>  	int processControls(Camera3RequestDescriptor *descriptor);
> +	void sendCaptureResults();
>  	std::unique_ptr<CameraMetadata> getResultMetadata(
>  		const Camera3RequestDescriptor &descriptor) const;
>  

-- 
Regards,

Laurent Pinchart