[libcamera-devel,v4] android: Check exposure time range for manual sensor capability

14641 diff mbox series

In the manual sensor capability validator, add a check for the presence
of the exposure time range key, and for the maximum exposure time. The
minimum exposure time is a requirement for adding the key in the first
place; add a check for this as well.

If either requirement is not met, the manual sensor capability
validation will fail, therefore disabling the FULL hardware level. The
exposure time range key is optional in non-FULL hardware levels.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

---
Changes in v4:
- s/i32/i64

Changes in v3:
- squash "android: capabilities: Add exposure time keys only if
  available"
- fix the minumum exposure time check
  - only make the exposure time range key available if this check
    passes. additionally, if the max exposure time passes its check,
    tick the box for manual sensor and FULL
- update commit message accordingly

Changes in v2:
- fix comparator order (cosmetic)
- change comparators and comments to "equal or", as that is what is
  specificied in the hal docs
- add check for minimum exposure time when initializing static metadata
---
 src/android/camera_capabilities.cpp | 33 +++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

Quoting Paul Elder (2021-11-19 09:48:22)
> In the manual sensor capability validator, add a check for the presence
> of the exposure time range key, and for the maximum exposure time. The
> minimum exposure time is a requirement for adding the key in the first
> place; add a check for this as well.
> 
> If either requirement is not met, the manual sensor capability
> validation will fail, therefore disabling the FULL hardware level. The
> exposure time range key is optional in non-FULL hardware levels.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> ---
> Changes in v4:
> - s/i32/i64
> 
> Changes in v3:
> - squash "android: capabilities: Add exposure time keys only if
>   available"
> - fix the minumum exposure time check
>   - only make the exposure time range key available if this check
>     passes. additionally, if the max exposure time passes its check,
>     tick the box for manual sensor and FULL
> - update commit message accordingly
> 
> Changes in v2:
> - fix comparator order (cosmetic)
> - change comparators and comments to "equal or", as that is what is
>   specificied in the hal docs
> - add check for minimum exposure time when initializing static metadata
> ---
>  src/android/camera_capabilities.cpp | 33 +++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index f357902e..2a1a428c 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -217,6 +217,8 @@ std::vector<U> setMetadata(CameraMetadata *metadata, uint32_t tag,
>  
>  bool CameraCapabilities::validateManualSensorCapability()
>  {
> +       camera_metadata_ro_entry_t entry;
> +
>         const char *noMode = "Manual sensor capability unavailable: ";
>  
>         if (!staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> @@ -231,6 +233,19 @@ bool CameraCapabilities::validateManualSensorCapability()
>                 return false;
>         }
>  
> +       if (!staticMetadata_->hasEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE)) {
> +               LOG(HAL, Info) << noMode << "missing exposure time range";
> +               return false;
> +       }
> +
> +       staticMetadata_->getEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, &entry);
> +       if (entry.data.i64[1] <= 100000000) {
> +               LOG(HAL, Info)
> +                       << noMode
> +                       << "exposure time range maximum must be larger than 100ms";
> +               return false;
> +       }
> +
>         /*
>          * \todo Return true here after we satisfy all the requirements:
>          * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR
> @@ -790,7 +805,6 @@ int CameraCapabilities::initializeStaticMetadata()
>                 ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
>                 ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
>                 ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> -               ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
>                 ANDROID_SENSOR_INFO_MAX_FRAME_DURATION,
>                 ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
>                 ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> @@ -868,7 +882,6 @@ int CameraCapabilities::initializeStaticMetadata()
>                 ANDROID_NOISE_REDUCTION_MODE,
>                 ANDROID_REQUEST_PIPELINE_DEPTH,
>                 ANDROID_SCALER_CROP_REGION,
> -               ANDROID_SENSOR_EXPOSURE_TIME,
>                 ANDROID_SENSOR_FRAME_DURATION,
>                 ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,
>                 ANDROID_SENSOR_TEST_PATTERN_MODE,
> @@ -1074,8 +1087,20 @@ int CameraCapabilities::initializeStaticMetadata()
>                         exposureInfo->second.min().get<int32_t>() * 1000LL,
>                         exposureInfo->second.max().get<int32_t>() * 1000LL,
>                 };
> -               staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> -                                         exposureTimeRange, 2);
> +
> +               if (exposureTimeRange[0] < 100000) {
> +                       staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> +                                       exposureTimeRange, 2);
> +
> +                       availableCharacteristicsKeys_.insert(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE);
> +                       availableRequestKeys_.insert(ANDROID_SENSOR_EXPOSURE_TIME);
> +                       availableResultKeys_.insert(ANDROID_SENSOR_EXPOSURE_TIME);

Looks fine since the last update I looked at.


I've seen this pattern come in a couple of times in your patches.

(Not for this patch) should we have a helper...
	addOptionalMetadata(characteristic, key, ....)

so that those four lines can be wrapped to:
	addOptionalMetadata(ANDROID_SENSOR_EXPOSURE_TIME,
			    ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
			    exposureTimeRange, 2);


Not essential, just wondering if it would help if that's a repeatable
pattern.

And if it ends up obfuscating the intentions then certainly not worth it
;-)

--
Kieran


> +               } else {
> +                       LOG(HAL, Info)
> +                               << "Minimum exposure time "
> +                               << exposureTimeRange[0]
> +                               << "ns is too big (should be smaller than 100us)";
> +               }
>         }
>  
>         staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, orientation_);
> -- 
> 2.27.0
>