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

[Jacopo Mondi]

Hi Kieran,
   I think we're not getting each other here :)

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.

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