| Message ID | 20210128102140.160134-1-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi me, 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@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; > + /* \todo Handle IPA appropriately */ > + bool ret = descriptor->settings_.getEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, &entry); > + resultMetadata->updateEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, This should be addEntry. Paul > + ret ? entry.data.u8 : &value, 1); > > 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); > > -- > 2.27.0 >
Hi Paul, 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@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 ? 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); > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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. > > 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); > >
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@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@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
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; + /* \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); 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);
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@ideasonboard.com> --- Changes in v2: - move setting the tag to getResultMetadata() --- src/android/camera_device.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)