<div dir="ltr"><div dir="ltr">Hi Jacopo, thank you for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 27, 2021 at 6:24 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Wed, May 19, 2021 at 04:59:41PM +0900, Hirokazu Honda wrote:<br>
> Report to the Android camera stack the list of supported test<br>
> pattern modes constructed by inspecting the values reported<br>
> by libcamera through the controls::draft::TestPatternMode control.<br>
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
>  src/android/camera_device.cpp | 47 ++++++++++++++++++++++++++++++++---<br>
>  1 file changed, 44 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> index b32e8be5..a07679a3 100644<br>
> --- a/src/android/camera_device.cpp<br>
> +++ b/src/android/camera_device.cpp<br>
> @@ -1035,11 +1035,52 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()<br>
><br>
>       staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, orientation_);<br>
><br>
> -     std::vector<int32_t> testPatterModes = {<br>
> -             ANDROID_SENSOR_TEST_PATTERN_MODE_OFF,<br>
> +     std::vector<int32_t> testPatternModes = {<br>
> +             ANDROID_SENSOR_TEST_PATTERN_MODE_OFF<br>
>       };<br>
> +     if (const auto &testPatternsInfo =<br>
> +                 controlsInfo.find(&controls::draft::TestPatternMode);<br>
> +         testPatternsInfo != controlsInfo.end()) {<br>
<br>
Is this intentional ? It compiles as it is legal, I'm surprised<br>
testPatternsInfo is visibile in the if() { } block scope.<br>
<br>
Honestly, it's kind of unusual, but maybe it's just me. If it compiles<br>
and it works...<br>
<br></blockquote><div><br></div><div><br></div><div>Yes, it is. It is C++ grammer introduced since C++17. <a href="https://skebanga.github.io/if-with-initializer/">https://skebanga.github.io/if-with-initializer/</a></div><div>I think it is useful if a variable is used only in if-condition and its clause.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +             const auto &values = testPatternsInfo->second.values();<br>
> +             ASSERT(!values.empty());<br>
> +             for (const auto &value : values) {<br>
> +                     switch (value.get<int32_t>()) {<br>
> +                     case controls::draft::TestPatternModeOff:<br>
> +                             /*<br>
> +                              * ANDROID_SENSOR_TEST_PATTERN_MODE_OFF is<br>
> +                              * already in testPatternModes.<br>
> +                              */<br>
> +                             break;<br>
> +                     case controls::draft::TestPatternModeSolidColor:<br>
> +                             testPatternModes.push_back(<br>
> +                                     ANDROID_SENSOR_TEST_PATTERN_MODE_SOLID_COLOR);<br>
> +                             break;<br>
> +                     case controls::draft::TestPatternModeColorBars:<br>
> +                             testPatternModes.push_back(<br>
> +                                     ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS);<br>
> +                             break;<br>
> +                     case controls::draft::TestPatternModeColorBarsFadeToGray:<br>
> +                             testPatternModes.push_back(<br>
> +                                     ANDROID_SENSOR_TEST_PATTERN_MODE_COLOR_BARS_FADE_TO_GRAY);<br>
> +                             break;<br>
> +                     case controls::draft::TestPatternModePn9:<br>
> +                             testPatternModes.push_back(<br>
> +                                     ANDROID_SENSOR_TEST_PATTERN_MODE_PN9);<br>
> +                             break;<br>
> +                     case controls::draft::TestPatternModeCustom1:<br>
> +                             testPatternModes.push_back(<br>
> +                                     ANDROID_SENSOR_TEST_PATTERN_MODE_CUSTOM1);<br>
> +                             break;<br>
> +                     default:<br>
> +                             LOG(HAL, Error)<br>
> +                                     << "Unknown test pattern mode: "<br>
<br>
Doesn't it fit on the previous line ?<br>
<br>
The patch looks good<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
  j<br>
<br>
<br>
> +                                     << value.get<int32_t>();<br>
> +                             continue;<br>
> +                     }<br>
> +             }<br>
> +     }<br>
>       staticMetadata_->addEntry(ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,<br>
> -                               testPatterModes);<br>
> +                               testPatternModes);<br>
><br>
>       uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;<br>
>       staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,<br>
> --<br>
> 2.31.1.751.gd2f1c929bd-goog<br>
><br>
</blockquote></div></div>