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

[Kieran Bingham]

Quoting Kieran Bingham (2021-11-18 11:59:22)
> 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() ?

Ok, I re-read the commit message. I'm not sure I understand yet what the
difference is but that explains the duplication.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

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