[libcamera-devel,3/3] android: Plumb noise reduction
diff mbox series

Message ID 20210913102007.2303225-3-paul.elder@ideasonboard.com
State Superseded
Delegated to: Paul Elder
Headers show
Series
  • [libcamera-devel,1/3] controls: Promote NoiseReductionMode to non-draft
Related show

Commit Message

Paul Elder Sept. 13, 2021, 10:20 a.m. UTC 
Add:
- hardware level detection based on available noise reduction values
  (no sepcific capabilities seem to require it; only FULL in general)
- Conversion from android to libcamera noise reduction modes for request
  (use switch-case instead of direct copy just in case)
- Conversion from libcamera to android noise reduction modes for result

We already have the mechanism to report the available noise reduction
modes, so that is not added.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 19 +++++++++++++
 src/android/camera_device.cpp       | 41 +++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

Comments

Laurent Pinchart Sept. 15, 2021, 2:13 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Mon, Sep 13, 2021 at 07:20:07PM +0900, Paul Elder wrote:
> Add:
> - hardware level detection based on available noise reduction values
>   (no sepcific capabilities seem to require it; only FULL in general)

s/sepcific/specific/

> - Conversion from android to libcamera noise reduction modes for request
>   (use switch-case instead of direct copy just in case)
> - Conversion from libcamera to android noise reduction modes for result
> 
> We already have the mechanism to report the available noise reduction
> modes, so that is not added.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 19 +++++++++++++
>  src/android/camera_device.cpp       | 41 +++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 08e44a1a..be6687d9 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -375,6 +375,25 @@ void CameraCapabilities::computeHwLevel(
>  	if (!found || *entry.data.i32 != 0)
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
>  
> +	found = staticMetadata_->entryContains<uint8_t>(
> +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> +		ANDROID_NOISE_REDUCTION_MODE_OFF);
> +	if (!found)
> +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +
> +	found = staticMetadata_->entryContains<uint8_t>(
> +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> +		ANDROID_NOISE_REDUCTION_MODE_FAST);
> +	if (!found)
> +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +
> +	/* Android docs don't say this is required, but CTS does. */
> +	found = staticMetadata_->entryContains<uint8_t>(
> +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> +		ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY);
> +	if (!found)
> +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +
>  	hwLevel_ = hwLevel;
>  }
>  
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ab31bdda..502dd10a 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -826,6 +826,40 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  		controls.set(controls::draft::TestPatternMode, testPatternMode);
>  	}
>  
> +	if (settings.getEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES, &entry)) {
> +		const uint8_t data = *entry.data.u8;
> +		int32_t noiseReductionMode = controls::NoiseReductionModeOff;

As there's a default case you could skip initialization.

> +		switch (data) {
> +		case ANDROID_NOISE_REDUCTION_MODE_OFF:
> +			noiseReductionMode = controls::NoiseReductionModeOff;
> +			break;
> +
> +		case ANDROID_NOISE_REDUCTION_MODE_FAST:
> +			noiseReductionMode = controls::NoiseReductionModeFast;
> +			break;
> +
> +		case ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY:
> +			noiseReductionMode = controls::NoiseReductionModeHighQuality;
> +			break;
> +
> +		case ANDROID_NOISE_REDUCTION_MODE_MINIMAL:
> +			noiseReductionMode = controls::NoiseReductionModeRaw;
> +			break;
> +
> +		case ANDROID_NOISE_REDUCTION_MODE_ZERO_SHUTTER_LAG:
> +			noiseReductionMode = controls::NoiseReductionModeZSL;
> +			break;
> +
> +		default:
> +			LOG(HAL, Error)
> +				<< "Unknown noise reduction mode: " << data;
> +
> +			return -EINVAL;
> +		}
> +
> +		controls.set(controls::NoiseReductionMode, noiseReductionMode);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1379,6 +1413,13 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
>  		resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect);
>  	}
>  
> +	if (metadata.contains(controls::NoiseReductionMode)) {
> +		const int32_t noiseReductionMode =
> +			metadata.get(controls::NoiseReductionMode);
> +		resultMetadata->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> +					 noiseReductionMode);
> +	}

Should you use a switch/case here too, if you use one in
CameraDevice::processControls() ?

> +
>  	if (metadata.contains(controls::draft::TestPatternMode)) {
>  		const int32_t testPatternMode =
>  			metadata.get(controls::draft::TestPatternMode);
Paul Elder Sept. 15, 2021, 3:09 a.m. UTC | #2 
Hi Laurent,

On Wed, Sep 15, 2021 at 05:13:49AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Sep 13, 2021 at 07:20:07PM +0900, Paul Elder wrote:
> > Add:
> > - hardware level detection based on available noise reduction values
> >   (no sepcific capabilities seem to require it; only FULL in general)
> 
> s/sepcific/specific/
> 
> > - Conversion from android to libcamera noise reduction modes for request
> >   (use switch-case instead of direct copy just in case)
> > - Conversion from libcamera to android noise reduction modes for result
> > 
> > We already have the mechanism to report the available noise reduction
> > modes, so that is not added.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/android/camera_capabilities.cpp | 19 +++++++++++++
> >  src/android/camera_device.cpp       | 41 +++++++++++++++++++++++++++++
> >  2 files changed, 60 insertions(+)
> > 
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index 08e44a1a..be6687d9 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -375,6 +375,25 @@ void CameraCapabilities::computeHwLevel(
> >  	if (!found || *entry.data.i32 != 0)
> >  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> >  
> > +	found = staticMetadata_->entryContains<uint8_t>(
> > +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > +		ANDROID_NOISE_REDUCTION_MODE_OFF);
> > +	if (!found)
> > +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > +
> > +	found = staticMetadata_->entryContains<uint8_t>(
> > +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > +		ANDROID_NOISE_REDUCTION_MODE_FAST);
> > +	if (!found)
> > +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > +
> > +	/* Android docs don't say this is required, but CTS does. */
> > +	found = staticMetadata_->entryContains<uint8_t>(
> > +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > +		ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY);
> > +	if (!found)
> > +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > +
> >  	hwLevel_ = hwLevel;
> >  }
> >  
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ab31bdda..502dd10a 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -826,6 +826,40 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >  		controls.set(controls::draft::TestPatternMode, testPatternMode);
> >  	}
> >  
> > +	if (settings.getEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES, &entry)) {
> > +		const uint8_t data = *entry.data.u8;
> > +		int32_t noiseReductionMode = controls::NoiseReductionModeOff;
> 
> As there's a default case you could skip initialization.
> 
> > +		switch (data) {
> > +		case ANDROID_NOISE_REDUCTION_MODE_OFF:
> > +			noiseReductionMode = controls::NoiseReductionModeOff;
> > +			break;
> > +
> > +		case ANDROID_NOISE_REDUCTION_MODE_FAST:
> > +			noiseReductionMode = controls::NoiseReductionModeFast;
> > +			break;
> > +
> > +		case ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY:
> > +			noiseReductionMode = controls::NoiseReductionModeHighQuality;
> > +			break;
> > +
> > +		case ANDROID_NOISE_REDUCTION_MODE_MINIMAL:
> > +			noiseReductionMode = controls::NoiseReductionModeRaw;
> > +			break;
> > +
> > +		case ANDROID_NOISE_REDUCTION_MODE_ZERO_SHUTTER_LAG:
> > +			noiseReductionMode = controls::NoiseReductionModeZSL;
> > +			break;
> > +
> > +		default:
> > +			LOG(HAL, Error)
> > +				<< "Unknown noise reduction mode: " << data;
> > +
> > +			return -EINVAL;
> > +		}
> > +
> > +		controls.set(controls::NoiseReductionMode, noiseReductionMode);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1379,6 +1413,13 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >  		resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect);
> >  	}
> >  
> > +	if (metadata.contains(controls::NoiseReductionMode)) {
> > +		const int32_t noiseReductionMode =
> > +			metadata.get(controls::NoiseReductionMode);
> > +		resultMetadata->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> > +					 noiseReductionMode);
> > +	}
> 
> Should you use a switch/case here too, if you use one in
> CameraDevice::processControls() ?

tbh that's what I thought doo. But look at what TestPatternMode does
right below ↓ :p


imo switch-case is better though.


Thanks,

Paul

> 
> > +
> >  	if (metadata.contains(controls::draft::TestPatternMode)) {
> >  		const int32_t testPatternMode =
> >  			metadata.get(controls::draft::TestPatternMode);
Laurent Pinchart Sept. 15, 2021, 3:45 a.m. UTC | #3 
Hi Paul,

On Wed, Sep 15, 2021 at 12:09:55PM +0900, paul.elder@ideasonboard.com wrote:
> On Wed, Sep 15, 2021 at 05:13:49AM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 13, 2021 at 07:20:07PM +0900, Paul Elder wrote:
> > > Add:
> > > - hardware level detection based on available noise reduction values
> > >   (no sepcific capabilities seem to require it; only FULL in general)
> > 
> > s/sepcific/specific/
> > 
> > > - Conversion from android to libcamera noise reduction modes for request
> > >   (use switch-case instead of direct copy just in case)
> > > - Conversion from libcamera to android noise reduction modes for result
> > > 
> > > We already have the mechanism to report the available noise reduction
> > > modes, so that is not added.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  src/android/camera_capabilities.cpp | 19 +++++++++++++
> > >  src/android/camera_device.cpp       | 41 +++++++++++++++++++++++++++++
> > >  2 files changed, 60 insertions(+)
> > > 
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index 08e44a1a..be6687d9 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -375,6 +375,25 @@ void CameraCapabilities::computeHwLevel(
> > >  	if (!found || *entry.data.i32 != 0)
> > >  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > >  
> > > +	found = staticMetadata_->entryContains<uint8_t>(
> > > +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > > +		ANDROID_NOISE_REDUCTION_MODE_OFF);
> > > +	if (!found)
> > > +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > +
> > > +	found = staticMetadata_->entryContains<uint8_t>(
> > > +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > > +		ANDROID_NOISE_REDUCTION_MODE_FAST);
> > > +	if (!found)
> > > +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > +
> > > +	/* Android docs don't say this is required, but CTS does. */
> > > +	found = staticMetadata_->entryContains<uint8_t>(
> > > +		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > > +		ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY);
> > > +	if (!found)
> > > +		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > +
> > >  	hwLevel_ = hwLevel;
> > >  }
> > >  
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index ab31bdda..502dd10a 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -826,6 +826,40 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > >  		controls.set(controls::draft::TestPatternMode, testPatternMode);
> > >  	}
> > >  
> > > +	if (settings.getEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES, &entry)) {
> > > +		const uint8_t data = *entry.data.u8;
> > > +		int32_t noiseReductionMode = controls::NoiseReductionModeOff;
> > 
> > As there's a default case you could skip initialization.
> > 
> > > +		switch (data) {
> > > +		case ANDROID_NOISE_REDUCTION_MODE_OFF:
> > > +			noiseReductionMode = controls::NoiseReductionModeOff;
> > > +			break;
> > > +
> > > +		case ANDROID_NOISE_REDUCTION_MODE_FAST:
> > > +			noiseReductionMode = controls::NoiseReductionModeFast;
> > > +			break;
> > > +
> > > +		case ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY:
> > > +			noiseReductionMode = controls::NoiseReductionModeHighQuality;
> > > +			break;
> > > +
> > > +		case ANDROID_NOISE_REDUCTION_MODE_MINIMAL:
> > > +			noiseReductionMode = controls::NoiseReductionModeRaw;
> > > +			break;
> > > +
> > > +		case ANDROID_NOISE_REDUCTION_MODE_ZERO_SHUTTER_LAG:
> > > +			noiseReductionMode = controls::NoiseReductionModeZSL;
> > > +			break;
> > > +
> > > +		default:
> > > +			LOG(HAL, Error)
> > > +				<< "Unknown noise reduction mode: " << data;
> > > +
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		controls.set(controls::NoiseReductionMode, noiseReductionMode);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > @@ -1379,6 +1413,13 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> > >  		resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect);
> > >  	}
> > >  
> > > +	if (metadata.contains(controls::NoiseReductionMode)) {
> > > +		const int32_t noiseReductionMode =
> > > +			metadata.get(controls::NoiseReductionMode);
> > > +		resultMetadata->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> > > +					 noiseReductionMode);
> > > +	}
> > 
> > Should you use a switch/case here too, if you use one in
> > CameraDevice::processControls() ?
> 
> tbh that's what I thought doo. But look at what TestPatternMode does
> right below ↓ :p
> 
> imo switch-case is better though.

Either there's a guarantee that the values match, and you can then drop
the switch above, or there isn't, and you need one here.

> > > +
> > >  	if (metadata.contains(controls::draft::TestPatternMode)) {
> > >  		const int32_t testPatternMode =
> > >  			metadata.get(controls::draft::TestPatternMode);

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index 08e44a1a..be6687d9 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -375,6 +375,25 @@  void CameraCapabilities::computeHwLevel(
 	if (!found || *entry.data.i32 != 0)
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
 
+	found = staticMetadata_->entryContains<uint8_t>(
+		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
+		ANDROID_NOISE_REDUCTION_MODE_OFF);
+	if (!found)
+		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
+
+	found = staticMetadata_->entryContains<uint8_t>(
+		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
+		ANDROID_NOISE_REDUCTION_MODE_FAST);
+	if (!found)
+		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
+
+	/* Android docs don't say this is required, but CTS does. */
+	found = staticMetadata_->entryContains<uint8_t>(
+		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
+		ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY);
+	if (!found)
+		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
+
 	hwLevel_ = hwLevel;
 }
 
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index ab31bdda..502dd10a 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -826,6 +826,40 @@  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
 		controls.set(controls::draft::TestPatternMode, testPatternMode);
 	}
 
+	if (settings.getEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES, &entry)) {
+		const uint8_t data = *entry.data.u8;
+		int32_t noiseReductionMode = controls::NoiseReductionModeOff;
+		switch (data) {
+		case ANDROID_NOISE_REDUCTION_MODE_OFF:
+			noiseReductionMode = controls::NoiseReductionModeOff;
+			break;
+
+		case ANDROID_NOISE_REDUCTION_MODE_FAST:
+			noiseReductionMode = controls::NoiseReductionModeFast;
+			break;
+
+		case ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY:
+			noiseReductionMode = controls::NoiseReductionModeHighQuality;
+			break;
+
+		case ANDROID_NOISE_REDUCTION_MODE_MINIMAL:
+			noiseReductionMode = controls::NoiseReductionModeRaw;
+			break;
+
+		case ANDROID_NOISE_REDUCTION_MODE_ZERO_SHUTTER_LAG:
+			noiseReductionMode = controls::NoiseReductionModeZSL;
+			break;
+
+		default:
+			LOG(HAL, Error)
+				<< "Unknown noise reduction mode: " << data;
+
+			return -EINVAL;
+		}
+
+		controls.set(controls::NoiseReductionMode, noiseReductionMode);
+	}
+
 	return 0;
 }
 
@@ -1379,6 +1413,13 @@  CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
 		resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect);
 	}
 
+	if (metadata.contains(controls::NoiseReductionMode)) {
+		const int32_t noiseReductionMode =
+			metadata.get(controls::NoiseReductionMode);
+		resultMetadata->addEntry(ANDROID_NOISE_REDUCTION_MODE,
+					 noiseReductionMode);
+	}
+
 	if (metadata.contains(controls::draft::TestPatternMode)) {
 		const int32_t testPatternMode =
 			metadata.get(controls::draft::TestPatternMode);