[libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable with AwbMode

[Naushir Patuck]

Hi Paul,

Thank you for your patch.

On Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder at ideasonboard.com>
wrote:

> Previously it was possible to have AwbEnable set to false, yet have
> AwbMode on anything. This caused a confusion situation, so merge the two
> into AwbMode. While at it, pull in the android AWB modes.
>
> Adjust the previous users of AwbEnable accordingly.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.h |  1 -
>  src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------
>  src/libcamera/control_ids.yaml      | 32 +++++++++++++++--------------
>  test/controls/control_list.cpp      |  6 +++---
>  4 files changed, 29 insertions(+), 37 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> index a8790000..63392a26 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {
>         { &controls::AeConstraintMode,
> ControlInfo(controls::AeConstraintModeValues) },
>         { &controls::AeExposureMode,
> ControlInfo(controls::AeExposureModeValues) },
>         { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> -       { &controls::AwbEnable, ControlInfo(false, true) },
>         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index ad0132c0..ed5f1250 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         break;
>                 }
>
> -               case controls::AWB_ENABLE: {
> -                       RPiController::Algorithm *awb =
> controller_.GetAlgorithm("awb");
> -                       if (!awb) {
> -                               LOG(IPARPI, Warning)
> -                                       << "Could not set AWB_ENABLE - no
> AWB algorithm";
> -                               break;
> -                       }
> -
> -                       if (ctrl.second.get<bool>() == false)
> -                               awb->Pause();
> -                       else
> -                               awb->Resume();
> -
> -                       libcameraMetadata_.set(controls::AwbEnable,
> -                                              ctrl.second.get<bool>());
> -                       break;
> -               }
> -
>                 case controls::AWB_MODE: {
>                         RPiController::AwbAlgorithm *awb =
> dynamic_cast<RPiController::AwbAlgorithm *>(
>                                 controller_.GetAlgorithm("awb"));
> @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         }
>
>                         int32_t idx = ctrl.second.get<int32_t>();
> +
> +                       if (idx == controls::AwbOff) {
> +                               awb->Pause();
> +                               break;
> +                       }
> +
> +                       if (idx == controls::AwbAuto)
> +                               awb->Resume();
>

I think the logic here may need adjusting such that any state that is not
controls::AwbOff will need to call awb->Resume(), or conditionally call
resume if adb->IsPaused() returns true.

Thanks,
Naush



>                         if (AwbModeTable.count(idx)) {
>                                 awb->SetMode(AwbModeTable.at(idx));
>                                 libcameraMetadata_.set(controls::AwbMode,
> idx);
> diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> index 5717bc1f..2e62f61b 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -229,13 +229,6 @@ controls:
>          Report an estimate of the current illuminance level in lux. The
> Lux
>          control can only be returned in metadata.
>
> -  - AwbEnable:
> -      type: bool
> -      description: |
> -        Enable or disable the AWB.
> -
> -        \sa ColourGains
> -
>    # AwbMode needs further attention:
>    # - Auto-generate max enum value.
>    # - Better handling of custom types.
> @@ -245,29 +238,38 @@ controls:
>          Specify the range of illuminants to use for the AWB algorithm.
> The modes
>          supported are platform specific, and not all modes may be
> supported.
>        enum:
> -        - name: AwbAuto
> +        - name: AwbOff
>            value: 0
> +          description: The AWB routune is disabled.
> +        - name: AwbAuto
> +          value: 1
>            description: Search over the whole colour temperature range.
>          - name: AwbIncandescent
> -          value: 1
> -          description: Incandescent AWB lamp mode.
> -        - name: AwbTungsten
>            value: 2
> -          description: Tungsten AWB lamp mode.
> +          description: Incandescent AWB lamp mode.
>          - name: AwbFluorescent
>            value: 3
>            description: Fluorescent AWB lamp mode.
> -        - name: AwbIndoor
> +        - name: AwbWarmFluorescent
>            value: 4
> -          description: Indoor AWB lighting mode.
> +          description: Warm fluorescent AWB lamp mode.
>          - name: AwbDaylight
>            value: 5
>            description: Daylight AWB lighting mode.
>          - name: AwbCloudy
>            value: 6
>            description: Cloudy AWB lighting mode.
> -        - name: AwbCustom
> +        - name: AwbTwilight
>            value: 7
> +          description: Twilight AWB lamp mode.
> +        - name: AwbTungsten
> +          value: 8
> +          description: Tungsten AWB lamp mode.
> +        - name: AwbIndoor
> +          value: 9
> +          description: Indoor AWB lighting mode.
> +        - name: AwbCustom
> +          value: 10
>            description: Custom AWB mode.
>
>    - AwbLocked:
> diff --git a/test/controls/control_list.cpp
> b/test/controls/control_list.cpp
> index 70cf61b8..ce55d09b 100644
> --- a/test/controls/control_list.cpp
> +++ b/test/controls/control_list.cpp
> @@ -143,10 +143,10 @@ protected:
>                  * Attempt to set an invalid control and verify that the
>                  * operation failed.
>                  */
> -               list.set(controls::AwbEnable, true);
> +               list.set(controls::AwbMode, true);
>
> -               if (list.contains(controls::AwbEnable)) {
> -                       cout << "List shouldn't contain AwbEnable control"
> << endl;
> +               if (list.contains(controls::AwbMode)) {
> +                       cout << "List shouldn't contain AwbMode control"
> << endl;
>                         return TestFail;
>                 }
>
> --
> 2.27.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210618/9feca279/attachment.htm>