[libcamera-devel] [PATCH v2 10/12] android: camera_device: Generate ResultMetadata earlier

[Jacopo Mondi]

Hi Kieran

On Mon, Aug 03, 2020 at 05:18:14PM +0100, Kieran Bingham wrote:
> Generate the ResultMetadata before performing JPEG compression so that
> JPEG specific metadata can be added to the metadata when it has been
> processed.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae52a5ca8b86..e23ab055d012 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1138,6 +1138,8 @@ void CameraDevice::requestComplete(Request *request)
>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>  	std::unique_ptr<CameraMetadata> resultMetadata;
> +	Camera3RequestDescriptor *descriptor =
> +		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
>
>  	if (request->status() != Request::RequestComplete) {
>  		LOG(HAL, Error) << "Request not successfully completed: "
> @@ -1145,9 +1147,18 @@ void CameraDevice::requestComplete(Request *request)
>  		status = CAMERA3_BUFFER_STATUS_ERROR;
>  	}
>
> +	/*
> +	 * \todo The timestamp used for the metadata is currently always taken
> +	 * from the first buffer (which may be the first stream) in the Request.
> +	 * It might be appropriate to return a 'correct' (as determined by
> +	 * pipeline handlers) timestamp in the Request itself.
> +	 */
> +	FrameBuffer *buffer = buffers.begin()->second;
> +	resultMetadata = getResultMetadata(descriptor->frameNumber,
> +					   buffer->metadata().timestamp);
> +
>  	/* Prepare to call back the Android camera stack. */
> -	Camera3RequestDescriptor *descriptor =
> -		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> +

One empty line is enough

>
>  	camera3_capture_result_t captureResult = {};
>  	captureResult.frame_number = descriptor->frameNumber;
> @@ -1160,21 +1171,13 @@ void CameraDevice::requestComplete(Request *request)
>  	captureResult.output_buffers =
>  		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
>
> -	/*
> -	 * \todo The timestamp used for the metadata is currently always taken
> -	 * from the first buffer (which may be the first stream) in the Request.
> -	 * It might be appropriate to return a 'correct' (as determined by
> -	 * pipeline handlers) timestamp in the Request itself.
> -	 */
> -	FrameBuffer *buffer = buffers.begin()->second;
>
>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
>  		notifyShutter(descriptor->frameNumber,
>  			      buffer->metadata().timestamp);
>
>  		captureResult.partial_result = 1;
> -		resultMetadata = getResultMetadata(descriptor->frameNumber,
> -						   buffer->metadata().timestamp);
> +

I would drop this one too

Overall I think this is fine so far, as we generate metadata
statically, but going forward if we have to access the Controls
returned by a completed Request, we shall make sure it completed
successfully. Can we record that with a \todo entry ?

That apart
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  		captureResult.result = resultMetadata->get();
>  	}
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel