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

[Kieran Bingham]

Hi Jacopo,

On 04/08/2020 12:08, Jacopo Mondi wrote:
> 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 ?
> 

I'm not sure I fully understand your comment here, do you mean the
libcamera request should be completed successfully before updating the
resultMetaData?

It will still only be assigned to the captureResult.result if the
 status == CAMERA3_BUFFER_STATUS_OK

check passes still...

You'll see how this gets used in 12/12 of course.


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

Thanks,


> 
> 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

-- 
Regards
--
Kieran