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

[Kieran Bingham]

Quoting paul.elder at ideasonboard.com (2021-11-23 10:50:32)
> Hello,
> 
> On Tue, Nov 23, 2021 at 07:40:40PM +0900, Paul Elder wrote:
> > 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 875d38da..f7397d27 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
> > @@ -798,7 +813,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,
> > @@ -876,7 +890,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,
> > @@ -1082,8 +1095,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);
> 
> Welp, it turns out this adds CTS failures:
> 
> junit.framework.AssertionFailedError: Missing field: SENSOR_EXPOSURE_TIME
> 
> java.lang.Throwable(Test failed for camera 0: Key android.sensor.exposureTime shouldn't be null)
> 
> Is it fine to have these failures, since they'll be fixed soon with the
> AE series? Or should I replace the result key one with a todo?

I'd be tempted to move this patch to the series that adds the
requirements.

Then we don't lose the patch, and don't add a regression when testing.



> 
> 
> Paul
> 
> > +             } 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
> >