[libcamera-devel] [PATCH] android: CameraDevice: factorize generating a capture result

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Apr 26, 2021 at 12:45:02PM +0900, Hirokazu Honda wrote:
> This factorizes a code of generating camera3_capture_result. It
> will be useful to report capture result in other places than
> CameraDevice::reqeustComplete().
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> 
> ---
> I was going to implement
> reportShutter() - createCaptureResult() + notifyShutter(), and
> reportError() - createCaptureResult() + notifyError().
> 
> But reportError() can be called after reportShutter(). It is a
> bit less efficient to create capture result twice in the case.
> So I only factorize a code of generating a capture result.

Can't we have a single function to do all that ? Right now we have

	/* Prepare to call back the Android camera stack. */
	camera3_capture_result_t captureResult = {};
	captureResult.frame_number = descriptor.frameNumber_;
	captureResult.num_output_buffers = descriptor.buffers_.size();
	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
		buffer.acquire_fence = -1;
		buffer.release_fence = -1;
		buffer.status = status;
	}
	captureResult.output_buffers = descriptor.buffers_.data();

	if (status == CAMERA3_BUFFER_STATUS_OK) {
		notifyShutter(descriptor.frameNumber_, timestamp);

		captureResult.partial_result = 1;
		captureResult.result = resultMetadata->get();
	}

	if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
		/* \todo Improve error handling. In case we notify an error
		 * because the metadata generation fails, a shutter event has
		 * already been notified for this frame number before the error
		 * is here signalled. Make sure the error path plays well with
		 * the camera stack state machine.
		 */
		notifyError(descriptor.frameNumber_,
			    descriptor.buffers_[0].stream);
	}

	callbacks_->process_capture_result(callbacks_, &captureResult);

Could we move all that to a function that takes descriptor, status,
timestamp and resultMedadata as arguments ? In stop(), to implement
flush(), it would be called with status == CAMERA3_BUFFER_STATUS_ERROR,
so notifyShutter() would be skipped.

If you think that would cause issues, this patch is already an
improvement, so we could proceed with it.

> ---
>  src/android/camera_device.cpp | 29 ++++++++++++++++++++---------
>  src/android/camera_device.h   |  3 +++
>  2 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a71aee2f..ced8efcc 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -2094,15 +2094,8 @@ void CameraDevice::requestComplete(Request *request)
>  	}
> 
>  	/* Prepare to call back the Android camera stack. */
> -	camera3_capture_result_t captureResult = {};
> -	captureResult.frame_number = descriptor.frameNumber_;
> -	captureResult.num_output_buffers = descriptor.buffers_.size();
> -	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> -		buffer.acquire_fence = -1;
> -		buffer.release_fence = -1;
> -		buffer.status = status;
> -	}
> -	captureResult.output_buffers = descriptor.buffers_.data();
> +	camera3_capture_result_t captureResult =
> +		createCaptureResult(descriptor, status);
> 
>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
>  		notifyShutter(descriptor.frameNumber_, timestamp);
> @@ -2130,6 +2123,24 @@ std::string CameraDevice::logPrefix() const
>  	return "'" + camera_->id() + "'";
>  }
> 
> +
> +camera3_capture_result_t CameraDevice::createCaptureResult(
> +	Camera3RequestDescriptor &descriptor,
> +	camera3_buffer_status status) const
> +{
> +	camera3_capture_result_t captureResult = {};
> +	captureResult.frame_number = descriptor.frameNumber_;
> +	captureResult.num_output_buffers = descriptor.buffers_.size();
> +	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> +		buffer.acquire_fence = -1;
> +		buffer.release_fence = -1;
> +		buffer.status = status;
> +	}
> +	captureResult.output_buffers = descriptor.buffers_.data();
> +
> +	return captureResult;
> +}
> +
>  void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
>  {
>  	camera3_notify_msg_t notify = {};
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index c63e8e21..a1abcead 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -100,6 +100,9 @@ private:
> 
>  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> +	camera3_capture_result_t createCaptureResult(
> +		Camera3RequestDescriptor &descriptor,
> +		camera3_buffer_status status) const;
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>  	std::unique_ptr<CameraMetadata> requestTemplatePreview();

-- 
Regards,

Laurent Pinchart