[libcamera-devel] [PATCH v6 3/7] android: camera_device: Refactor descriptor status and sendCaptureResults()

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Sat, Oct 23, 2021 at 04:02:58PM +0530, Umang Jain wrote:
> Currently, we use Camera3RequestDescriptor::Status to determine:
> - When the descriptor has been completely processed by HAL
> - Whether any errors were encountered, during its processing
> 
> Both of these are essential to know whether the descriptor is eligible
> to call process_capture_results() through sendCaptureResults().
> When a status(Success/Error) is set on the descriptor, it is ready to
> be sent back via sendCaptureResults(). However, this might lead to
> undesired results especially when sendCaptureResults() runs in a
> different thread (for e.g. stream's post-processor async completion
> slot).
> 
> This patch decouples the descriptor status (Success/Error) from the
> descriptor's completion status (pending or complete). The advantage
> of this is we can set the completion status when the descriptor has
> been processed fully by the layer and we can set the error status on
> the descriptor wherever an error is encountered, throughout the
> lifetime descriptor in the HAL layer.
> 
> While at it, introduce a wrapper completeDescriptor() around
> sendCaptureResults(). completeDescriptor() as the name suggests will
> mark the descriptor as complete, so it is ready to be sent back.
> The locking mechanism is moved from sendCaptureResults() to this wrapper
> since the intention is to use completeDescriptor() in place of existing
> sendCaptureResults() calls.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/camera_device.cpp  | 33 ++++++++++++++++++---------------
>  src/android/camera_device.h    |  1 +
>  src/android/camera_request.cpp |  2 +-
>  src/android/camera_request.h   |  7 ++++---
>  4 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 806b4090..f4d4fb09 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -982,13 +982,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	MutexLocker stateLock(stateMutex_);
>  
>  	if (state_ == State::Flushing) {
> -		abortRequest(descriptor.get());
> +		Camera3RequestDescriptor *rawDescriptor = descriptor.get();
> +		abortRequest(rawDescriptor);

Could we move the abortRequest() call just before completeDescriptor(),
to always operate with the descriptor already added to the queue ? It
should make no difference today, but consistent call patterns will make
it easier to understand the code and the possible race conditions.

>  		{
>  			MutexLocker descriptorsLock(descriptorsMutex_);
>  			descriptors_.push(std::move(descriptor));
>  		}
> -
> -		sendCaptureResults();
> +		completeDescriptor(rawDescriptor);
>  
>  		return 0;
>  	}
> @@ -1079,7 +1079,7 @@ void CameraDevice::requestComplete(Request *request)
>  				<< request->status();
>  
>  		abortRequest(descriptor);
> -		sendCaptureResults();
> +		completeDescriptor(descriptor);
>  
>  		return;
>  	}
> @@ -1117,6 +1117,7 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  
>  	/* Handle post-processing. */
> +	bool hasPostProcessingErrors = false;
>  	for (auto &buffer : descriptor->buffers_) {
>  		CameraStream *stream = buffer.stream;
>  
> @@ -1129,6 +1130,7 @@ void CameraDevice::requestComplete(Request *request)
>  			buffer.status = Camera3RequestDescriptor::Status::Error;
>  			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>  				    CAMERA3_MSG_ERROR_BUFFER);
> +			hasPostProcessingErrors = true;

You can set

			descriptor->status_ = Camera3RequestDescriptor::Status::Error;

here.

>  			continue;
>  		}
>  
> @@ -1143,29 +1145,32 @@ void CameraDevice::requestComplete(Request *request)
>  
>  		if (ret) {
>  			buffer.status = Camera3RequestDescriptor::Status::Error;
> +			hasPostProcessingErrors = true;

And here too.

>  			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>  				    CAMERA3_MSG_ERROR_BUFFER);
>  		}
>  	}
>  
> -	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> +	if (hasPostProcessingErrors)
> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;

And then drop this.

> +
> +	completeDescriptor(descriptor);
> +}
> +
> +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)
> +{
> +	MutexLocker lock(descriptorsMutex_);
> +	descriptor->completeDescriptor();
> +
>  	sendCaptureResults();
>  }
>  
>  void CameraDevice::sendCaptureResults()
>  {
> -	MutexLocker lock(descriptorsMutex_);
>  	while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
>  		auto descriptor = std::move(descriptors_.front());
>  		descriptors_.pop();
>  
> -		/*
> -		 * \todo Releasing and re-acquiring the critical section for
> -		 * every request completion (grain-locking) might have an
> -		 * impact on performance which should be measured.
> -		 */
> -		lock.unlock();
> -
>  		camera3_capture_result_t captureResult = {};
>  
>  		captureResult.frame_number = descriptor->frameNumber_;
> @@ -1201,8 +1206,6 @@ void CameraDevice::sendCaptureResults()
>  			captureResult.partial_result = 1;
>  
>  		callbacks_->process_capture_result(callbacks_, &captureResult);
> -
> -		lock.lock();
>  	}
>  }
>  
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 863cf414..e544f2bd 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -93,6 +93,7 @@ private:
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>  			 camera3_error_msg_code code) const;
>  	int processControls(Camera3RequestDescriptor *descriptor);
> +	void completeDescriptor(Camera3RequestDescriptor *descriptor);
>  	void sendCaptureResults();
>  	std::unique_ptr<CameraMetadata> getResultMetadata(
>  		const Camera3RequestDescriptor &descriptor) const;
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index faa85ada..16cf266f 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -36,7 +36,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>  			static_cast<CameraStream *>(buffer.stream->priv);
>  
>  		buffers_.push_back({ stream, buffer.buffer, nullptr,
> -				     buffer.acquire_fence, Status::Pending });
> +				     buffer.acquire_fence, Status::Success });
>  	}
>  
>  	/* Clone the controls associated with the camera3 request. */
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index 05dabf89..904886da 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -26,7 +26,6 @@ class Camera3RequestDescriptor
>  {
>  public:
>  	enum class Status {
> -		Pending,
>  		Success,
>  		Error,
>  	};
> @@ -43,7 +42,8 @@ public:
>  				 const camera3_capture_request_t *camera3Request);
>  	~Camera3RequestDescriptor();
>  
> -	bool isPending() const { return status_ == Status::Pending; }
> +	bool isPending() const { return !complete_; }
> +	void completeDescriptor() { complete_ = true; }

As the function is a member of the Camera3RequestDescriptor class, I
wouldn't repeat "descriptor" in the name. Given that the complete_
member is public, I'd actually drop the function and access complete_
directly, the same way we access status_. If you prefer an accessor, I'd
make the complete_ member private, but that's probably overkill for now
given that all other members are public.

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

>  	uint32_t frameNumber_ = 0;
>  
> @@ -53,7 +53,8 @@ public:
>  	std::unique_ptr<CaptureRequest> request_;
>  	std::unique_ptr<CameraMetadata> resultMetadata_;
>  
> -	Status status_ = Status::Pending;
> +	bool complete_ = false;
> +	Status status_ = Status::Success;
>  
>  private:
>  	LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)

-- 
Regards,

Laurent Pinchart