[libcamera-devel] android: Check exposure time range for manual sensor capability
diff mbox series

Message ID 20210823103258.1583287-1-paul.elder@ideasonboard.com
State New
Delegated to: Paul Elder
Headers show
Series
  • [libcamera-devel] android: Check exposure time range for manual sensor capability
Related show

Commit Message

Paul Elder Aug. 23, 2021, 10:32 a.m. UTC 
In the manual sensor capability validator, add a check for the exposure
time range.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Laurent Pinchart Aug. 24, 2021, 12:33 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Mon, Aug 23, 2021 at 07:32:58PM +0900, Paul Elder wrote:
> In the manual sensor capability validator, add a check for the exposure
> time range.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index bd661675..5b560b4f 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 (100000 < entry.data.i32[0]) {

You know that C++ has a > operator ? :-) It would match the following
text:

	if (is larger than 100us)
		"must be no larger than 100";

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		LOG(HAL, Info)
> +			<< noMode
> +			<< "exposure time range minimum must be no larger than 100us";
> +		return false;
> +	}
> +
> +	if (entry.data.i32[1] < 100000000) {
> +		LOG(HAL, Info)
> +			<< noMode
> +			<< "exposure time range maximum must be no 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
Jacopo Mondi Aug. 25, 2021, 9:02 a.m. UTC | #2 
Hi Paul,

On Mon, Aug 23, 2021 at 07:32:58PM +0900, Paul Elder wrote:
> In the manual sensor capability validator, add a check for the exposure
> time range.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index bd661675..5b560b4f 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 (100000 < entry.data.i32[0]) {
> +		LOG(HAL, Info)
> +			<< noMode
> +			<< "exposure time range minimum must be no larger than 100us";
> +		return false;
> +	}
> +
> +	if (entry.data.i32[1] < 100000000) {
> +		LOG(HAL, Info)
> +			<< noMode
> +			<< "exposure time range maximum must be no smaller than 100ms";
> +		return false;
> +	}
> +

Why I don't see such requirement for MANUAL_SENSOR capability
description, but rather in the FULL level description only ?

android.sensor.info.exposureTimeRange:
For FULL capability devices (android.info.supportedHardwareLevel ==
FULL),The maximum of the range SHOULD be at least 1 second (1e9), MUST
be at least 100ms.

>  	/*
>  	 * \todo Return true here after we satisfy all the requirements:
>  	 * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR
> --
> 2.27.0
>
Paul Elder Aug. 27, 2021, 3:02 a.m. UTC | #3 
Hi Jacopo,

On Wed, Aug 25, 2021 at 11:02:22AM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Mon, Aug 23, 2021 at 07:32:58PM +0900, Paul Elder wrote:
> > In the manual sensor capability validator, add a check for the exposure
> > time range.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/android/camera_capabilities.cpp | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index bd661675..5b560b4f 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 (100000 < entry.data.i32[0]) {
> > +		LOG(HAL, Info)
> > +			<< noMode
> > +			<< "exposure time range minimum must be no larger than 100us";
> > +		return false;
> > +	}
> > +
> > +	if (entry.data.i32[1] < 100000000) {
> > +		LOG(HAL, Info)
> > +			<< noMode
> > +			<< "exposure time range maximum must be no smaller than 100ms";
> > +		return false;
> > +	}
> > +
> 
> Why I don't see such requirement for MANUAL_SENSOR capability
> description, but rather in the FULL level description only ?
> 
> android.sensor.info.exposureTimeRange:
> For FULL capability devices (android.info.supportedHardwareLevel ==
> FULL),The maximum of the range SHOULD be at least 1 second (1e9), MUST
> be at least 100ms.

From what I saw [1], the first is an overall requirement, while the
second is only a FULL requirement.

The minimum exposure time will be less than 100 us. For FULL capability
devices (android.info.supportedHardwareLevel == FULL), the maximum
exposure time will be greater than 100

So can I assume that the first is satisfied? Or should we check it when
it's set in the static metadata and reject it there?

Also I guess the checks should be "or equal to" :)

[1] https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE


Thanks,

Paul

> 
> >  	/*
> >  	 * \todo Return true here after we satisfy all the requirements:
> >  	 * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR
> > --
> > 2.27.0
> >
Jacopo Mondi Aug. 27, 2021, 6:28 a.m. UTC | #4 
Hi Paul,

On Fri, Aug 27, 2021 at 12:02:47PM +0900, paul.elder@ideasonboard.com wrote:
> Hi Jacopo,
>
> On Wed, Aug 25, 2021 at 11:02:22AM +0200, Jacopo Mondi wrote:
> > Hi Paul,
> >
> > On Mon, Aug 23, 2021 at 07:32:58PM +0900, Paul Elder wrote:
> > > In the manual sensor capability validator, add a check for the exposure
> > > time range.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  src/android/camera_capabilities.cpp | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index bd661675..5b560b4f 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 (100000 < entry.data.i32[0]) {
> > > +		LOG(HAL, Info)
> > > +			<< noMode
> > > +			<< "exposure time range minimum must be no larger than 100us";
> > > +		return false;
> > > +	}
> > > +
> > > +	if (entry.data.i32[1] < 100000000) {
> > > +		LOG(HAL, Info)
> > > +			<< noMode
> > > +			<< "exposure time range maximum must be no smaller than 100ms";
> > > +		return false;
> > > +	}
> > > +
> >
> > Why I don't see such requirement for MANUAL_SENSOR capability
> > description, but rather in the FULL level description only ?
> >
> > android.sensor.info.exposureTimeRange:
> > For FULL capability devices (android.info.supportedHardwareLevel ==
> > FULL),The maximum of the range SHOULD be at least 1 second (1e9), MUST
> > be at least 100ms.
>
> From what I saw [1], the first is an overall requirement, while the
> second is only a FULL requirement.
>
> The minimum exposure time will be less than 100 us. For FULL capability
> devices (android.info.supportedHardwareLevel == FULL), the maximum
> exposure time will be greater than 100

Reading again this quotation, yes it seems the first is a generic
requirement.

>
> So can I assume that the first is satisfied? Or should we check it when
> it's set in the static metadata and reject it there?

I think it would be better verified at static metadata initialization
time ? What do you think ?

	const auto &exposureInfo = controlsInfo.find(&controls::ExposureTime);
	if (exposureInfo != controlsInfo.end()) {
		int64_t exposureTimeRange[2] = {
			exposureInfo->second.min().get<int32_t>() * 1000LL,
			exposureInfo->second.max().get<int32_t>() * 1000LL,

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

while the second requirement has to be verified when testing for FULL
level support ?

Thanks
   j

>
> Also I guess the checks should be "or equal to" :)
>
> [1] https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#SENSOR_INFO_EXPOSURE_TIME_RANGE
>
>
> Thanks,
>
> Paul
>
> >
> > >  	/*
> > >  	 * \todo Return true here after we satisfy all the requirements:
> > >  	 * https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR
> > > --
> > > 2.27.0
> > >

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index bd661675..5b560b4f 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 (100000 < entry.data.i32[0]) {
+		LOG(HAL, Info)
+			<< noMode
+			<< "exposure time range minimum must be no larger than 100us";
+		return false;
+	}
+
+	if (entry.data.i32[1] < 100000000) {
+		LOG(HAL, Info)
+			<< noMode
+			<< "exposure time range maximum must be no 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