[libcamera-devel] [PATCH v2] android: camera_device: Set AE precapture trigger according to request

[Jacopo Mondi]

Hi

On Thu, Jan 28, 2021 at 01:16:16PM +0200, Laurent Pinchart wrote:
> Hi Jacopo and Paul,
>
> On Thu, Jan 28, 2021 at 12:03:03PM +0100, Jacopo Mondi wrote:
> > On Thu, Jan 28, 2021 at 07:21:40PM +0900, Paul Elder wrote:
> > > Set the AE precapture triggler tag in the android result metadata
> > > according to what was passed in the request metadata.
> > >
> > > This allows the following CTS test to pass:
> > > - android.hardware.camera2.cts.StillCaptureTest#testAePrecaptureTriggerCancelJpegCapture
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - move setting the tag to getResultMetadata()
> > > ---
> > >  src/android/camera_device.cpp | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 82bf0d3a..f5066709 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1965,8 +1965,11 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> > >  				 aeFpsTarget.data(), aeFpsTarget.size());
> > >
> > >  	value = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;
> > > -	resultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > -				 &value, 1);
> > > +	camera_metadata_ro_entry_t entry;
> >
> > You can move this one at the beginning of the function. It is used in
> > many places, and we'll continue moving it otherwise.
> >
> > > +	/* \todo Handle IPA appropriately */
> > > +	bool ret = descriptor->settings_.getEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, &entry);
> > > +	resultMetadata->updateEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > +				    ret ? entry.data.u8 : &value, 1);
> >
> > idk... we're blindly taking this from from the settings, while I think
> > it would be worth plumbing it into the pipeline handler already,
> > even if it will be functionally equivalent to what you have here.
> >
> > There's also a draft control already defined for this.
> >
> > To close the loop this would require to:
> > - register the control in the pipeline handler (for IPU3 at initControls() time)
> > - translate the android metadata into a libcamera control in
> >   CameraDevice::processControls()
> > - Return in the request complete handler of the pipeline handler the
> >   value passed in the request settings (see how ScalerCrop is handled
> >   in the IPU3 imguOutputBufferReady() slot)
> > - Translate back the libcamera control from the request to the android
> >   metadata (look again at how ScalerCrop is handled in
> >   getResultMetadata())
> >
> > This might seems like a useless exercize, but once the IPA needs to
> > handle this control for real, this will have to be done and I would
> > not delay it.
> >
> > Laurent, do you think this is worth going to the pipeline handler loop
> > already ?
>
> There's a state machine to we need to implement to handle the AE. It
> involves multiple controls. As we don't support still image capture yet
> in the libcamera core I think it could be too early to plumb this
> through, I'm be tempted to just go with this shortcut for the very short
> term.
>

I see, makes sense.

For this patch then
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> > >  	value = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > >  	resultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, &value, 1);
> > > @@ -2010,8 +2013,7 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> > >  	value = ANDROID_FLASH_STATE_UNAVAILABLE;
> > >  	resultMetadata->addEntry(ANDROID_FLASH_STATE, &value, 1);
> > >
> > > -	camera_metadata_ro_entry_t entry;
> > > -	int ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);
> > > +	ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);
> > >  	if (ret)
> > >  		resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
> > >
>
> --
> Regards,
>
> Laurent Pinchart