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

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-18 11:10:58)
> Hi Paul,
> 
> Thanks for the patch !
> 
> On 18/11/2021 10:48, Paul Elder wrote:
> > In the manual sensor capability validator, add a check for the exposure
> > time range. While at it, since the minimum exposure time is not limited
> > to the manual sensor capability, add a check for it when initializing
> > static metadata.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > ---
> > 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
> >    - this only prints error, should we return -EINVAL instead?

Is the issue caught by
CameraCapabilities::validateManualSensorCapability() sufficiently?


> > ---
> >   src/android/camera_capabilities.cpp | 30 +++++++++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> > 
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index f357902e..6e46f163 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,26 @@ 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.i32[0] >= 100000) {
> > +             LOG(HAL, Info)
> > +                     << noMode
> > +                     << "exposure time range minimum must not be equal nor larger than 100us";
> > +             return false;
> > +     }
> > +
> > +     if (entry.data.i32[1] <= 100000000) {
> > +             LOG(HAL, Info)
> > +                     << noMode
> > +                     << "exposure time range maximum must not be equal nor smaller 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
> > @@ -1074,6 +1096,14 @@ int CameraCapabilities::initializeStaticMetadata()
> >                       exposureInfo->second.min().get<int32_t>() * 1000LL,
> >                       exposureInfo->second.max().get<int32_t>() * 1000LL,
> >               };
> > +
> > +             if (exposureTimeRange[0] >= 100000) {
> > +                     LOG(HAL, Error)
> > +                             << "Minimum exposure time "
> > +                             << exposureTimeRange[0]
> > +                             << "ns is too big (should be smaller than 100us)";
> > +             }
> 
> I read the doc (better now than never :-)) in 
> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE
> 
> Interestingly, the ExposureTimeRange is in nanoseconds. Our 
> Controls::ExposureTime metadata is in microseconds. Should it be changed ?

I can see some *1000LL above which I presume are converting the
microseconds to nanoseconds ...

Makes me wonder if we could use std::chrono more in the android layer
too...

Why do we only report an error in initializeStaticMetadata() on the
minimum, and not check the maximum?

Do we need to check the minimum there at all if it's already validated
in validateManualSensorCapability() ?


> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> 
> > +
> >               staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> >                                         exposureTimeRange, 2);
> >       }
> >