[libcamera-devel,v5,6/6] android: CameraDevice: Report queried test pattern modes
diff mbox series

Message ID 20210519075941.1337388-6-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,v5,1/6] libcamera: controls: Add sensor test pattern mode
Related show

Commit Message

Hirokazu Honda May 19, 2021, 7:59 a.m. UTC 
Report to the Android camera stack the list of supported test
pattern modes constructed by inspecting the values reported
by libcamera through the controls::draft::TestPatternMode control.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 47 ++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi May 26, 2021, 9:25 p.m. UTC | #1 
Hi Hiro,

On Wed, May 19, 2021 at 04:59:41PM +0900, Hirokazu Honda wrote:
> Report to the Android camera stack the list of supported test
> pattern modes constructed by inspecting the values reported
> by libcamera through the controls::draft::TestPatternMode control.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 47 ++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b32e8be5..a07679a3 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1035,11 +1035,52 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>
>  	staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, orientation_);
>
> -	std::vector<int32_t> testPatterModes = {
> -		ANDROID_SENSOR_TEST_PATTERN_MODE_OFF,
> +	std::vector<int32_t> testPatternModes = {
> +		ANDROID_SENSOR_TEST_PATTERN_MODE_OFF
>  	};
> +	if (const auto &testPatternsInfo =
> +		    controlsInfo.find(&controls::draft::TestPatternMode);
> +	    testPatternsInfo != controlsInfo.end()) {

Is this intentional ? It compiles as it is legal, I'm surprised
testPatternsInfo is visibile in the if() { } block scope.

Honestly, it's kind of unusual, but maybe it's just me. If it compiles
and it works...

> +		const auto &values = testPatternsInfo->second.values();
> +		ASSERT(!values.empty());
> +		for (const auto &value : values) {
> +			switch (value.get<int32_t>()) {
> +			case controls::draft::TestPatternModeOff:
> +				/*
> +				 * ANDROID_SENSOR_TEST_PATTERN_MODE_OFF is
> +				 * already in testPatternModes.
> +				 */
> +				break;
> +			case controls::draft::TestPatternModeSolidColor:
> +				testPatternModes.push_back(
> +					ANDROID_SENSOR_TEST_PATTERN_MODE_SOLID_COLOR);
> +				break;
> +			case controls::draft::TestPatternModeColorBars:
> +				testPatternModes.push_back(
> +					ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS);
> +				break;
> +			case controls::draft::TestPatternModeColorBarsFadeToGray:
> +				testPatternModes.push_back(
> +					ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS_FADE_TO_GRAY);
> +				break;
> +			case controls::draft::TestPatternModePn9:
> +				testPatternModes.push_back(
> +					ANDROID_SENSOR_TEST_PATTERN_MODE_PN9);
> +				break;
> +			case controls::draft::TestPatternModeCustom1:
> +				testPatternModes.push_back(
> +					ANDROID_SENSOR_TEST_PATTERN_MODE_CUSTOM1);
> +				break;
> +			default:
> +				LOG(HAL, Error)
> +					<< "Unknown test pattern mode: "

Doesn't it fit on the previous line ?

The patch looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> +					<< value.get<int32_t>();
> +				continue;
> +			}
> +		}
> +	}
>  	staticMetadata_->addEntry(ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
> -				  testPatterModes);
> +				  testPatternModes);
>
>  	uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
>  	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
> --
> 2.31.1.751.gd2f1c929bd-goog
>
Hirokazu Honda May 27, 2021, 6:42 a.m. UTC | #2 
Hi Jacopo, thank you for reviewing.

On Thu, May 27, 2021 at 6:24 AM Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Hiro,
>
> On Wed, May 19, 2021 at 04:59:41PM +0900, Hirokazu Honda wrote:
> > Report to the Android camera stack the list of supported test
> > pattern modes constructed by inspecting the values reported
> > by libcamera through the controls::draft::TestPatternMode control.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 47 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 44 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp
> b/src/android/camera_device.cpp
> > index b32e8be5..a07679a3 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1035,11 +1035,52 @@ const camera_metadata_t
> *CameraDevice::getStaticMetadata()
> >
> >       staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,
> orientation_);
> >
> > -     std::vector<int32_t> testPatterModes = {
> > -             ANDROID_SENSOR_TEST_PATTERN_MODE_OFF,
> > +     std::vector<int32_t> testPatternModes = {
> > +             ANDROID_SENSOR_TEST_PATTERN_MODE_OFF
> >       };
> > +     if (const auto &testPatternsInfo =
> > +                 controlsInfo.find(&controls::draft::TestPatternMode);
> > +         testPatternsInfo != controlsInfo.end()) {
>
> Is this intentional ? It compiles as it is legal, I'm surprised
> testPatternsInfo is visibile in the if() { } block scope.
>
> Honestly, it's kind of unusual, but maybe it's just me. If it compiles
> and it works...
>
>

Yes, it is. It is C++ grammer introduced since C++17.
https://skebanga.github.io/if-with-initializer/
I think it is useful if a variable is used only in if-condition and its
clause.


> > +             const auto &values = testPatternsInfo->second.values();
> > +             ASSERT(!values.empty());
> > +             for (const auto &value : values) {
> > +                     switch (value.get<int32_t>()) {
> > +                     case controls::draft::TestPatternModeOff:
> > +                             /*
> > +                              * ANDROID_SENSOR_TEST_PATTERN_MODE_OFF is
> > +                              * already in testPatternModes.
> > +                              */
> > +                             break;
> > +                     case controls::draft::TestPatternModeSolidColor:
> > +                             testPatternModes.push_back(
> > +
>  ANDROID_SENSOR_TEST_PATTERN_MODE_SOLID_COLOR);
> > +                             break;
> > +                     case controls::draft::TestPatternModeColorBars:
> > +                             testPatternModes.push_back(
> > +
>  ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS);
> > +                             break;
> > +                     case
> controls::draft::TestPatternModeColorBarsFadeToGray:
> > +                             testPatternModes.push_back(
> > +
>  ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS_FADE_TO_GRAY);
> > +                             break;
> > +                     case controls::draft::TestPatternModePn9:
> > +                             testPatternModes.push_back(
> > +
>  ANDROID_SENSOR_TEST_PATTERN_MODE_PN9);
> > +                             break;
> > +                     case controls::draft::TestPatternModeCustom1:
> > +                             testPatternModes.push_back(
> > +
>  ANDROID_SENSOR_TEST_PATTERN_MODE_CUSTOM1);
> > +                             break;
> > +                     default:
> > +                             LOG(HAL, Error)
> > +                                     << "Unknown test pattern mode: "
>
> Doesn't it fit on the previous line ?
>
> The patch looks good
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
>
> > +                                     << value.get<int32_t>();
> > +                             continue;
> > +                     }
> > +             }
> > +     }
> >
>  staticMetadata_->addEntry(ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
> > -                               testPatterModes);
> > +                               testPatternModes);
> >
> >       uint8_t timestampSource =
> ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
> >       staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
> > --
> > 2.31.1.751.gd2f1c929bd-goog
> >
>

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index b32e8be5..a07679a3 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1035,11 +1035,52 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 
 	staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, orientation_);
 
-	std::vector<int32_t> testPatterModes = {
-		ANDROID_SENSOR_TEST_PATTERN_MODE_OFF,
+	std::vector<int32_t> testPatternModes = {
+		ANDROID_SENSOR_TEST_PATTERN_MODE_OFF
 	};
+	if (const auto &testPatternsInfo =
+		    controlsInfo.find(&controls::draft::TestPatternMode);
+	    testPatternsInfo != controlsInfo.end()) {
+		const auto &values = testPatternsInfo->second.values();
+		ASSERT(!values.empty());
+		for (const auto &value : values) {
+			switch (value.get<int32_t>()) {
+			case controls::draft::TestPatternModeOff:
+				/*
+				 * ANDROID_SENSOR_TEST_PATTERN_MODE_OFF is
+				 * already in testPatternModes.
+				 */
+				break;
+			case controls::draft::TestPatternModeSolidColor:
+				testPatternModes.push_back(
+					ANDROID_SENSOR_TEST_PATTERN_MODE_SOLID_COLOR);
+				break;
+			case controls::draft::TestPatternModeColorBars:
+				testPatternModes.push_back(
+					ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS);
+				break;
+			case controls::draft::TestPatternModeColorBarsFadeToGray:
+				testPatternModes.push_back(
+					ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS_FADE_TO_GRAY);
+				break;
+			case controls::draft::TestPatternModePn9:
+				testPatternModes.push_back(
+					ANDROID_SENSOR_TEST_PATTERN_MODE_PN9);
+				break;
+			case controls::draft::TestPatternModeCustom1:
+				testPatternModes.push_back(
+					ANDROID_SENSOR_TEST_PATTERN_MODE_CUSTOM1);
+				break;
+			default:
+				LOG(HAL, Error)
+					<< "Unknown test pattern mode: "
+					<< value.get<int32_t>();
+				continue;
+			}
+		}
+	}
 	staticMetadata_->addEntry(ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
-				  testPatterModes);
+				  testPatternModes);
 
 	uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
 	staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,