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

[Kieran Bingham]

Hi Jacopo,

On 04/08/2020 13:37, Jacopo Mondi wrote:
> Hi Kieran,
>    I think we're not getting each other here :)

Maybe ;-)

> On Tue, Aug 04, 2020 at 12:54:04PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
> 
> [snip]
> 
>>>>>> -	/*
>>>>>> -	 * \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.
> 
> You don't need to for JPEG, but the metadata we -currently- return to
> the framework is assembled with fixed values. We'll need to construct
> it inspecting the Request ControlList, and if the Request has not
> succeeded, it might contain invalid values.


But if the request fails, then in CameraDevice::requestComplete(), the
request status is checked:

if (request->status() != Request::RequestComplete) {
	LOG(HAL, Error) << "Request not successfully completed: "
			<< request->status();
	status = CAMERA3_BUFFER_STATUS_ERROR;
}


So status is set to error:


Which means, the metadata will never be assigned anyway...

if (status == CAMERA3_BUFFER_STATUS_OK) {
	notifyShutter(descriptor->frameNumber,
		      buffer->metadata().timestamp);

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



And thus notifyError would get called:

if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
	notifyError(descriptor->frameNumber,
		    descriptor->buffers[0].stream);
}


So I'm not sure what todo to add, as we already ensure that the metadata
isn't added to the captureResult if there is an error in either case,
and if we parse controls which depend on the request being successfully
completed ... well then it's up to that code to decide to only parse the
request if it was successful.

If you'd still like a specific todo adding - let me know the text and
where, and I'll paste it in.

--
Kieran


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

-- 
Regards
--
Kieran