[libcamera-devel,v2,11/13] android: camera_device: Handle NOISE_REDUCTION_MODES
diff mbox series

Message ID 20201020180534.36855-12-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: Introduce draft controls
Related show

Commit Message

Jacopo Mondi Oct. 20, 2020, 6:05 p.m. UTC 
Register the ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES
static metadata property inspecting the values retuned by the pipeline
handler.

Reserve in the static metadata pack enough space to support all the 5
available noise reduction modes Android defines.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Kieran Bingham Oct. 21, 2020, 1:17 p.m. UTC | #1 
Hi Jacopo,

On 20/10/2020 19:05, Jacopo Mondi wrote:
> Register the ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES
> static metadata property inspecting the values retuned by the pipeline
> handler.
> 
> Reserve in the static metadata pack enough space to support all the 5
> available noise reduction modes Android defines.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 8166b09bb69a..a4d9e6ddc519 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -553,7 +553,7 @@ std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
>  	 * Currently: 51 entries, 687 bytes of static metadata
>  	 */
>  	uint32_t numEntries = 51;
> -	uint32_t byteSize = 687;
> +	uint32_t byteSize = 691;
>  
>  	/*
>  	 * Calculate space occupation in bytes for dynamically built metadata
> @@ -830,9 +830,18 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  &minFocusDistance, 1);
>  
>  	/* Noise reduction modes. */
> -	uint8_t noiseReductionModes = ANDROID_NOISE_REDUCTION_MODE_OFF;
> -	staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> -				  &noiseReductionModes, 1);
> +	{
> +		std::vector<uint8_t> data(5);
> +		const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);
> +		if (infoMap != controlsInfo.end()) {
> +			for (const auto &value : infoMap->second.values())
> +				data.push_back(value.get<int32_t>());

I'm a bit concerned by this, as it has the hidden assumption that the
control values are the same value as the ANDROID enum values.

While they're in draft that's true, but it might not be when we remove
::draft:: ... I suspect the best option here is to add a \todo: or
something to highlight that fact so it's not a hidden assumption.

Anyway, I won't block on that, as this is clearly still in ::draft::.

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

> +		} else {
> +			data.push_back(ANDROID_NOISE_REDUCTION_MODE_OFF);
> +		}
> +		staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> +					  data.data(), data.size());
> +	}
>  
>  	/* Scaler static metadata. */
>  	float maxDigitalZoom = 1;
>
Jacopo Mondi Oct. 21, 2020, 1:57 p.m. UTC | #2 
Hi Kieran,

On Wed, Oct 21, 2020 at 02:17:52PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 20/10/2020 19:05, Jacopo Mondi wrote:
> > Register the ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES
> > static metadata property inspecting the values retuned by the pipeline
> > handler.
> >
> > Reserve in the static metadata pack enough space to support all the 5
> > available noise reduction modes Android defines.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 8166b09bb69a..a4d9e6ddc519 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -553,7 +553,7 @@ std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> >  	 * Currently: 51 entries, 687 bytes of static metadata
> >  	 */
> >  	uint32_t numEntries = 51;
> > -	uint32_t byteSize = 687;
> > +	uint32_t byteSize = 691;
> >
> >  	/*
> >  	 * Calculate space occupation in bytes for dynamically built metadata
> > @@ -830,9 +830,18 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  				  &minFocusDistance, 1);
> >
> >  	/* Noise reduction modes. */
> > -	uint8_t noiseReductionModes = ANDROID_NOISE_REDUCTION_MODE_OFF;
> > -	staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > -				  &noiseReductionModes, 1);
> > +	{
> > +		std::vector<uint8_t> data(5);
> > +		const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);
> > +		if (infoMap != controlsInfo.end()) {
> > +			for (const auto &value : infoMap->second.values())
> > +				data.push_back(value.get<int32_t>());
>
> I'm a bit concerned by this, as it has the hidden assumption that the
> control values are the same value as the ANDROID enum values.
>
> While they're in draft that's true, but it might not be when we remove
> ::draft:: ... I suspect the best option here is to add a \todo: or
> something to highlight that fact so it's not a hidden assumption.
>
> Anyway, I won't block on that, as this is clearly still in ::draft::.

Whenerver the draft control will be moved outside of the draft::
namespace this will anyway be reworked

>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > +		} else {
> > +			data.push_back(ANDROID_NOISE_REDUCTION_MODE_OFF);
> > +		}
> > +		staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > +					  data.data(), data.size());
> > +	}
> >
> >  	/* Scaler static metadata. */
> >  	float maxDigitalZoom = 1;
> >
>
> --
> Regards
> --
> Kieran

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 8166b09bb69a..a4d9e6ddc519 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -553,7 +553,7 @@  std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
 	 * Currently: 51 entries, 687 bytes of static metadata
 	 */
 	uint32_t numEntries = 51;
-	uint32_t byteSize = 687;
+	uint32_t byteSize = 691;
 
 	/*
 	 * Calculate space occupation in bytes for dynamically built metadata
@@ -830,9 +830,18 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 				  &minFocusDistance, 1);
 
 	/* Noise reduction modes. */
-	uint8_t noiseReductionModes = ANDROID_NOISE_REDUCTION_MODE_OFF;
-	staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
-				  &noiseReductionModes, 1);
+	{
+		std::vector<uint8_t> data(5);
+		const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);
+		if (infoMap != controlsInfo.end()) {
+			for (const auto &value : infoMap->second.values())
+				data.push_back(value.get<int32_t>());
+		} else {
+			data.push_back(ANDROID_NOISE_REDUCTION_MODE_OFF);
+		}
+		staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
+					  data.data(), data.size());
+	}
 
 	/* Scaler static metadata. */
 	float maxDigitalZoom = 1;