[libcamera-devel] [PATCH 4/5] android: camera_worker: Notify fence wait failures

[Umang Jain]

Hi Jacopo,

On 9/6/21 7:31 PM, Jacopo Mondi wrote:
> The CameraDevice class stores one CaptureRequestDescriptor instance
> for each capture request queued to the Camera. Such descriptors are
> cleared when the Camera completes the Request in the
> CameraDevice::requestComplete() slot.
>
> The CameraDevice class relies on an internal worker to off-load
> waiting on synchronization fences, and does unconditionally store
> the descriptor in the descriptors_ class member map to handle its
> lifetime.
>
> If waiting on a fence fails, the created descriptor remains in the
> descriptors_ map until the next call to stop() or close(), as
> requestComplete() will never be called, and the camera
> service is never notified about the queueing failure.
>
> Fix that by allowing the CameraWorker to notify to the CameraDevice if
> waiting on a fence has failed, by installing a new requestQueueFailed
> Signal.
>
> The Signal is emitted by the CameraWorker internal worker if waiting on
> a fence has failed. The CameraDevice is augmented with a slot connected
> to the Signal that removes the descriptor from the map and notifies the
> camera framework about the failure.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>


Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

> ---
>   src/android/camera_device.cpp | 22 +++++++++++++++++++++-
>   src/android/camera_device.h   |  1 +
>   src/android/camera_worker.cpp | 10 ++++++----
>   src/android/camera_worker.h   |  9 +++++++--
>   4 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a0ea138d9499..0ce9b699bfae 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -244,7 +244,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>   	 * Create the CaptureRequest, stored as a unique_ptr<> to tie its
>   	 * lifetime to the descriptor.
>   	 */
> -	request_ = std::make_unique<CaptureRequest>(camera);
> +	request_ = std::make_unique<CaptureRequest>(camera, camera3Request);
>   }
>   
>   /*
> @@ -264,6 +264,7 @@ CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
>   	: id_(id), state_(State::Stopped), camera_(std::move(camera)),
>   	  facing_(CAMERA_FACING_FRONT), orientation_(0)
>   {
> +	worker_.requestQueueFailed.connect(this, &CameraDevice::requestQueueFailed);
>   	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
>   
>   	maker_ = "libcamera";
> @@ -1060,6 +1061,25 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   	return 0;
>   }
>   
> +void CameraDevice::requestQueueFailed(CaptureRequest *request)
> +{
> +	/*
> +	 * Remove the descriptor from the map if the worker has failed
> +	 * to process it and notify the camera service about the failure.
> +	 */
> +	MutexLocker descriptorsLock(descriptorsMutex_);
> +	auto it = descriptors_.find(request->cookie());
> +	if (it == descriptors_.end()) {
> +		LOG(HAL, Fatal)
> +			<< "Unknown request: " << request->cookie();
> +		return;
> +	}
> +
> +	descriptors_.extract(it);
> +
> +	abortRequest(request->camera3Request());
> +}
> +
>   void CameraDevice::requestComplete(Request *request)
>   {
>   	decltype(descriptors_)::node_type node;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 54c4cb9ab499..65456ecca7e3 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -83,6 +83,7 @@ private:
>   		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>   		CameraMetadata settings_;
>   		std::unique_ptr<CaptureRequest> request_;
> +		const camera3_capture_request_t *camera3Request_;
>   	};
>   
>   	enum class State {
> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> index 98dddd9eb13b..7f39fab01638 100644
> --- a/src/android/camera_worker.cpp
> +++ b/src/android/camera_worker.cpp
> @@ -27,8 +27,9 @@ LOG_DECLARE_CATEGORY(HAL)
>    * by the CameraWorker which queues it to the libcamera::Camera after handling
>    * fences.
>    */
> -CaptureRequest::CaptureRequest(libcamera::Camera *camera)
> -	: camera_(camera)
> +CaptureRequest::CaptureRequest(libcamera::Camera *camera,
> +			       const camera3_capture_request_t *camera3Request)
> +	: camera_(camera), camera3Request_(camera3Request)
>   {
>   	request_ = camera_->createRequest(reinterpret_cast<uint64_t>(this));
>   }
> @@ -77,7 +78,7 @@ void CameraWorker::queueRequest(CaptureRequest *request)
>   {
>   	/* Async process the request on the worker which runs its own thread. */
>   	worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,
> -			     request);
> +			     this, request);
>   }
>   
>   /*
> @@ -109,7 +110,7 @@ int CameraWorker::Worker::waitFence(int fence)
>   	return -errno;
>   }
>   
> -void CameraWorker::Worker::processRequest(CaptureRequest *request)
> +void CameraWorker::Worker::processRequest(CameraWorker *context, CaptureRequest *request)
>   {
>   	/* Wait on all fences before queuing the Request. */
>   	for (int fence : request->fences()) {
> @@ -121,6 +122,7 @@ void CameraWorker::Worker::processRequest(CaptureRequest *request)
>   		if (ret < 0) {
>   			LOG(HAL, Error) << "Failed waiting for fence: "
>   					<< fence << ": " << strerror(-ret);
> +			context->requestQueueFailed.emit(request);
>   			return;
>   		}
>   	}
> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> index 67ae50bd9288..86f20f741e54 100644
> --- a/src/android/camera_worker.h
> +++ b/src/android/camera_worker.h
> @@ -10,6 +10,7 @@
>   #include <memory>
>   
>   #include <libcamera/base/object.h>
> +#include <libcamera/base/signal.h>
>   #include <libcamera/base/thread.h>
>   
>   #include <libcamera/camera.h>
> @@ -22,7 +23,8 @@ class CameraDevice;
>   class CaptureRequest
>   {
>   public:
> -	CaptureRequest(libcamera::Camera *camera);
> +	CaptureRequest(libcamera::Camera *camera,
> +		       const camera3_capture_request_t *camera3Request);
>   
>   	const std::vector<int> &fences() const { return acquireFences_; }
>   	libcamera::ControlList &controls() { return request_->controls(); }
> @@ -31,6 +33,7 @@ public:
>   		return request_->metadata();
>   	}
>   	unsigned long cookie() const { return request_->cookie(); }
> +	const camera3_capture_request_t *camera3Request() const { return camera3Request_; }
>   
>   	void addBuffer(libcamera::Stream *stream,
>   		       libcamera::FrameBuffer *buffer, int fence);
> @@ -40,6 +43,7 @@ private:
>   	libcamera::Camera *camera_;
>   	std::vector<int> acquireFences_;
>   	std::unique_ptr<libcamera::Request> request_;
> +	const camera3_capture_request_t *camera3Request_;
>   };
>   
>   class CameraWorker : private libcamera::Thread
> @@ -51,6 +55,7 @@ public:
>   	void stop();
>   
>   	void queueRequest(CaptureRequest *request);
> +	libcamera::Signal<CaptureRequest *> requestQueueFailed;
>   
>   protected:
>   	void run() override;
> @@ -59,7 +64,7 @@ private:
>   	class Worker : public libcamera::Object
>   	{
>   	public:
> -		void processRequest(CaptureRequest *request);
> +		void processRequest(CameraWorker *context, CaptureRequest *request);
>   
>   	private:
>   		int waitFence(int fence);