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

[Umang Jain]

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 need 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 | 46 ++++++++++++++++++++++++++---------
 src/android/camera_device.h   | 15 +++++++++++-
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index b0b7f4fd..8e2d22c5 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
 	/* Clone the controls associated with the camera3 request. */
 	settings_ = CameraMetadata(camera3Request->settings);
 
+	status_ = Status::Pending;
+
 	/*
 	 * Create the CaptureRequest, stored as a unique_ptr<> to tie its
 	 * lifetime to the descriptor.
@@ -859,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)
 {
 	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;
@@ -877,7 +880,8 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)
 	}
 	result.output_buffers = resultBuffers.data();
 
-	callbacks_->process_capture_result(callbacks_, &result);
+	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
+	sendCaptureResults();
 }
 
 bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
@@ -1045,13 +1049,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 push the descriptor in the queue and abort
+	 * the request. 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);
+		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
+		{
+			MutexLocker descriptorsLock(descriptorsMutex_);
+			descriptors_.push_back(std::move(descriptor));
+		}
+		abortRequest(descriptors_.back().get(), camera3Request);
 		return 0;
 	}
 
@@ -1099,7 +1109,7 @@ void CameraDevice::requestComplete(Request *request)
 		return;
 	}
 
-	camera3_capture_result_t captureResult = {};
+	camera3_capture_result_t &captureResult = descriptor->captureResult_;
 	captureResult.frame_number = descriptor->frameNumber_;
 	captureResult.num_output_buffers = descriptor->buffers_.size();
 	captureResult.output_buffers = descriptor->buffers_.data();
@@ -1138,9 +1148,9 @@ void CameraDevice::requestComplete(Request *request)
 			buffer.acquire_fence = -1;
 			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
 		}
-		callbacks_->process_capture_result(callbacks_, &captureResult);
 
-		descriptors_.pop_front();
+		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
+		sendCaptureResults();
 		return;
 	}
 
@@ -1217,9 +1227,23 @@ void CameraDevice::requestComplete(Request *request)
 	captureResult.partial_result = 1;
 
 	captureResult.result = resultMetadata->get();
-	callbacks_->process_capture_result(callbacks_, &captureResult);
+	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
+	sendCaptureResults();
+}
 
-	descriptors_.pop_front();
+void CameraDevice::sendCaptureResults()
+{
+	MutexLocker lock(descriptorsMutex_);
+	while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
+		std::unique_ptr<Camera3RequestDescriptor> descriptor =
+			std::move(descriptors_.front());
+		descriptors_.pop_front();
+
+		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 5889a0e7..545cb9b4 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_ = {};
+		libcamera::FrameBuffer *internalBuffer_;
+		Status status_;
 	};
 
 	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;
 
-- 
2.31.1