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

[Kieran Bingham]

Hi Jacopo,

On 04/08/2020 12:55, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Tue, Aug 04, 2020 at 12:10:45PM +0100, Kieran Bingham wrote:
>> 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?
> 
> What I meant to say is that before this change the metadata buffer was
> generated in the
> 	if (status == CAMERA3_BUFFER_STATUS_OK)
> case, while now, if I got this right, it's generated unconditionally.
> When we'll assemble it inspecting the ControlList associated with the
> just completed Request, if it has not completed succesfully, we might
> try to access an invalid control list or a control list with unset
> values. If that's correct, I think it has to be recorded.

No, I don't touch any control list. The added information comes only
from the generated JPEG content, which is local to the android layer,
not the underlying libcamera stream.

--
K

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

-- 
Regards
--
Kieran