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

[Kieran Bingham]

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 at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain at 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
>