[libcamera-devel] [PATCH 03/11] android: camera_device: Build capture_result dynamically

[Jacopo Mondi]

Hi Umang,

On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> The camera3_capture_result_t is only needed to convey capture results to
> the camera service through the process_capture_result() callback.
> There's no need to store it in the Camera3RequestDescriptor. Build it
> dynamically in CameraDevice::sendCaptureResults() instead.
>
> This requires storing the result metadata created in
> CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side
> effect of this change is that the request metadata lifetime will match
> the Camera3RequestDescriptor instead of being destroyed at the end of
> requestComplete(). This will be needed to support asynchronous
> post-processing, where the request completion will be signaled to the
> camera service asynchronously from requestComplete().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 43 ++++++++++++++++-------------------
>  src/android/camera_request.h  |  2 +-
>  2 files changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b4ab5da1..c6ae8930 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
>  {
>  	notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>
> -	camera3_capture_result_t &result = descriptor->captureResult_;
> -	result.num_output_buffers = descriptor->buffers_.size();
> -	result.frame_number = descriptor->frameNumber_;
> -	result.partial_result = 0;
> -
> -	std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
> -	for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
> -		buffer = descriptor->buffers_[i];
> -		buffer.release_fence = descriptor->buffers_[i].acquire_fence;
> +	for (auto &buffer : descriptor->buffers_) {
> +		buffer.release_fence = buffer.acquire_fence;
>  		buffer.acquire_fence = -1;
>  		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>  	}
> -	result.output_buffers = resultBuffers.data();
>
>  	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>  }
> @@ -1090,9 +1082,6 @@ 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 = descriptor->captureResult_;
> -	captureResult.frame_number = descriptor->frameNumber_;
> -	captureResult.num_output_buffers = descriptor->buffers_.size();
>  	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>  		CameraStream *cameraStream =
>  			static_cast<CameraStream *>(buffer.stream->priv);
> @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)
>  		buffer.release_fence = -1;
>  		buffer.status = CAMERA3_BUFFER_STATUS_OK;
>  	}
> -	captureResult.output_buffers = descriptor->buffers_.data();
> -	captureResult.partial_result = 1;
>
>  	/*
>  	 * If the Request has failed, abort the request by notifying the error
> @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)
>  		notifyError(descriptor->frameNumber_, nullptr,
>  			    CAMERA3_MSG_ERROR_REQUEST);
>
> -		captureResult.partial_result = 0;
>  		for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>  			/*
>  			 * Signal to the framework it has to handle fences that
> @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)
>  	 * Notify if the metadata generation has failed, but continue processing
>  	 * buffers and return an empty metadata pack.
>  	 */
> -	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor);
> -	if (!resultMetadata) {
> +	descriptor->resultMetadata_ = getResultMetadata(*descriptor);
> +	if (!descriptor->resultMetadata_) {
>  		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);
> +		descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0);

Since we don't sendCaptureResult() and return here we get to
post-processing with a (0, 0) metadata pack. I wonder if the
post-processing code is robust enough to cope with that situation
(even if I suspect I already know the answer :)

It is my understanding the situation is like this already, so it's
fine, but maybe we should add a todo.

>  	}
>
>  	/* Handle post-processing. */
> @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)
>
>  		int ret = cameraStream->process(*src, buffer,
>  						descriptor->settings_,
> -						resultMetadata.get());
> +						descriptor->resultMetadata_.get());
>  		/*
>  		 * Return the FrameBuffer to the CameraStream now that we're
>  		 * done processing it.
> @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)
>  		}
>  	}
>
> -	captureResult.result = resultMetadata->get();
>  	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
>  	sendCaptureResults();
>  }
> @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()
>  		 * impact on performance which should be measured.
>  		 */
>  		lock.unlock();
> -		callbacks_->process_capture_result(callbacks_,
> -						   &descriptor->captureResult_);
> +
> +		camera3_capture_result_t captureResult = {};
> +
> +		captureResult.frame_number = descriptor->frameNumber_;
> +		if (descriptor->resultMetadata_)

Do we need this ? As resultMetadata_ is a unique_ptr<> it is
constructed owning nothing, and calling get() on it simnply return
nullptr, which I think it's what we want.

Thanks
   j
> +			captureResult.result = descriptor->resultMetadata_->get();
> +		captureResult.num_output_buffers = descriptor->buffers_.size();
> +		captureResult.output_buffers = descriptor->buffers_.data();
> +
> +		if (descriptor->status_ == Camera3RequestDescriptor::Status::Success)
> +			captureResult.partial_result = 1;
> +
> +		callbacks_->process_capture_result(callbacks_, &captureResult);
> +
>  		lock.lock();
>  	}
>  }
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index 79dfdb58..db13f624 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -40,8 +40,8 @@ public:
>  	std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>  	CameraMetadata settings_;
>  	std::unique_ptr<CaptureRequest> request_;
> +	std::unique_ptr<CameraMetadata> resultMetadata_;
>
> -	camera3_capture_result_t captureResult_ = {};
>  	Status status_ = Status::Pending;
>
>  private:
> --
> 2.31.0
>