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

14622 diff mbox series

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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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?
---
 src/android/camera_capabilities.cpp | 30 +++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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?
> ---
>   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 ?

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> +
>   		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
>   					  exposureTimeRange, 2);
>   	}
>

On Thu, Nov 18, 2021 at 12:10:58PM +0100, Jean-Michel Hautbois wrote:
> 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@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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?
> > ---
> >   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";

Maybe "must be smaller 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";

Same here, "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
> > @@ -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)";

Do we need to handle the error somehow ?

> > +		}
> 
> 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 think we should standardize all times in nanoseconds in libcamera at
some point, unless there's a good technical reason not to do so.

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

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@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> 
> > +
> >               staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> >                                         exposureTimeRange, 2);
> >       }
> >

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@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>

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

Hi Laurent,

On Thu, Nov 18, 2021 at 01:56:47PM +0200, Laurent Pinchart wrote:
> On Thu, Nov 18, 2021 at 12:10:58PM +0100, Jean-Michel Hautbois wrote:
> > 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@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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?
> > > ---
> > >   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";
> 
> Maybe "must be smaller 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";
> 
> Same here, "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
> > > @@ -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)";
> 
> Do we need to handle the error somehow ?

Okay, reading the docs again carefully, perhaps we have an answer.

- This key (SENSOR_INFO_EXPOSURE_TIME_RANGE) is only required on FULL
  level devices
- The minimum value for this key must always be less than 100us
- The maximum value for this key, if hardware level is FULL, must be
  greater than 100ms

Since this key is optional on non-FULL, if the minimum value is not
satisfied, then we'll just not report the key, and the manual sensor
mode (and therefore FULL mode) will not be reported as available.

...here comes a v3.


Paul

> 
> > > +		}
> > 
> > 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 think we should standardize all times in nanoseconds in libcamera at
> some point, unless there's a good technical reason not to do so.
> 
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > 
> > > +
> > >   		staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> > >   					  exposureTimeRange, 2);
> > >   	}
> > >