[{"id":17629,"web_url":"https://patchwork.libcamera.org/comment/17629/","msgid":"<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>","date":"2021-06-18T12:23:03","subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nThank you for your patch.\n\nOn Fri, 18 Jun 2021 at 11:34, Paul Elder <paul.elder@ideasonboard.com>\nwrote:\n\n> Previously it was possible to have AwbEnable set to false, yet have\n> AwbMode on anything. This caused a confusion situation, so merge the two\n> into AwbMode. While at it, pull in the android AWB modes.\n>\n> Adjust the previous users of AwbEnable accordingly.\n>\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  include/libcamera/ipa/raspberrypi.h |  1 -\n>  src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------\n>  src/libcamera/control_ids.yaml      | 32 +++++++++++++++--------------\n>  test/controls/control_list.cpp      |  6 +++---\n>  4 files changed, 29 insertions(+), 37 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> index a8790000..63392a26 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {\n>         { &controls::AeConstraintMode,\n> ControlInfo(controls::AeConstraintModeValues) },\n>         { &controls::AeExposureMode,\n> ControlInfo(controls::AeExposureModeValues) },\n>         { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> -       { &controls::AwbEnable, ControlInfo(false, true) },\n>         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index ad0132c0..ed5f1250 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList &controls)\n>                         break;\n>                 }\n>\n> -               case controls::AWB_ENABLE: {\n> -                       RPiController::Algorithm *awb =\n> controller_.GetAlgorithm(\"awb\");\n> -                       if (!awb) {\n> -                               LOG(IPARPI, Warning)\n> -                                       << \"Could not set AWB_ENABLE - no\n> AWB algorithm\";\n> -                               break;\n> -                       }\n> -\n> -                       if (ctrl.second.get<bool>() == false)\n> -                               awb->Pause();\n> -                       else\n> -                               awb->Resume();\n> -\n> -                       libcameraMetadata_.set(controls::AwbEnable,\n> -                                              ctrl.second.get<bool>());\n> -                       break;\n> -               }\n> -\n>                 case controls::AWB_MODE: {\n>                         RPiController::AwbAlgorithm *awb =\n> dynamic_cast<RPiController::AwbAlgorithm *>(\n>                                 controller_.GetAlgorithm(\"awb\"));\n> @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList &controls)\n>                         }\n>\n>                         int32_t idx = ctrl.second.get<int32_t>();\n> +\n> +                       if (idx == controls::AwbOff) {\n> +                               awb->Pause();\n> +                               break;\n> +                       }\n> +\n> +                       if (idx == controls::AwbAuto)\n> +                               awb->Resume();\n>\n\nI think the logic here may need adjusting such that any state that is not\ncontrols::AwbOff will need to call awb->Resume(), or conditionally call\nresume if adb->IsPaused() returns true.\n\nThanks,\nNaush\n\n\n\n>                         if (AwbModeTable.count(idx)) {\n>                                 awb->SetMode(AwbModeTable.at(idx));\n>                                 libcameraMetadata_.set(controls::AwbMode,\n> idx);\n> diff --git a/src/libcamera/control_ids.yaml\n> b/src/libcamera/control_ids.yaml\n> index 5717bc1f..2e62f61b 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -229,13 +229,6 @@ controls:\n>          Report an estimate of the current illuminance level in lux. The\n> Lux\n>          control can only be returned in metadata.\n>\n> -  - AwbEnable:\n> -      type: bool\n> -      description: |\n> -        Enable or disable the AWB.\n> -\n> -        \\sa ColourGains\n> -\n>    # AwbMode needs further attention:\n>    # - Auto-generate max enum value.\n>    # - Better handling of custom types.\n> @@ -245,29 +238,38 @@ controls:\n>          Specify the range of illuminants to use for the AWB algorithm.\n> The modes\n>          supported are platform specific, and not all modes may be\n> supported.\n>        enum:\n> -        - name: AwbAuto\n> +        - name: AwbOff\n>            value: 0\n> +          description: The AWB routune is disabled.\n> +        - name: AwbAuto\n> +          value: 1\n>            description: Search over the whole colour temperature range.\n>          - name: AwbIncandescent\n> -          value: 1\n> -          description: Incandescent AWB lamp mode.\n> -        - name: AwbTungsten\n>            value: 2\n> -          description: Tungsten AWB lamp mode.\n> +          description: Incandescent AWB lamp mode.\n>          - name: AwbFluorescent\n>            value: 3\n>            description: Fluorescent AWB lamp mode.\n> -        - name: AwbIndoor\n> +        - name: AwbWarmFluorescent\n>            value: 4\n> -          description: Indoor AWB lighting mode.\n> +          description: Warm fluorescent AWB lamp mode.\n>          - name: AwbDaylight\n>            value: 5\n>            description: Daylight AWB lighting mode.\n>          - name: AwbCloudy\n>            value: 6\n>            description: Cloudy AWB lighting mode.\n> -        - name: AwbCustom\n> +        - name: AwbTwilight\n>            value: 7\n> +          description: Twilight AWB lamp mode.\n> +        - name: AwbTungsten\n> +          value: 8\n> +          description: Tungsten AWB lamp mode.\n> +        - name: AwbIndoor\n> +          value: 9\n> +          description: Indoor AWB lighting mode.\n> +        - name: AwbCustom\n> +          value: 10\n>            description: Custom AWB mode.\n>\n>    - AwbLocked:\n> diff --git a/test/controls/control_list.cpp\n> b/test/controls/control_list.cpp\n> index 70cf61b8..ce55d09b 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -143,10 +143,10 @@ protected:\n>                  * Attempt to set an invalid control and verify that the\n>                  * operation failed.\n>                  */\n> -               list.set(controls::AwbEnable, true);\n> +               list.set(controls::AwbMode, true);\n>\n> -               if (list.contains(controls::AwbEnable)) {\n> -                       cout << \"List shouldn't contain AwbEnable control\"\n> << endl;\n> +               if (list.contains(controls::AwbMode)) {\n> +                       cout << \"List shouldn't contain AwbMode control\"\n> << endl;\n>                         return TestFail;\n>                 }\n>\n> --\n> 2.27.0\n>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6ECCEBD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 18 Jun 2021 12:23:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D696D6050D;\n\tFri, 18 Jun 2021 14:23:21 +0200 (CEST)","from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E7D460298\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 18 Jun 2021 14:23:20 +0200 (CEST)","by mail-lj1-x22a.google.com with SMTP id g13so3416405ljj.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 18 Jun 2021 05:23:20 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"fuEri+IG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=s6tmbNqyIqtco6ZG1YSyqueCmq+t7MqiAjYBq/AREOQ=;\n\tb=fuEri+IG9mpyRKFuxbNgq+DEwtZXFJ7DEN3o8As8I3aNyFOy7qaWpE0AcfcR5qjDa+\n\t++QCHABZF0NpXTws4C5nTidUv43PA25engNB6U3j4KtdW93DSqZkQbt7bFiqtXNzlD4t\n\tHz1HGiHWgg42J9ygdB2L6FIW1GVVFLvKHJcligexHJeTUkJsHk2qc8eadADL4BkONS1B\n\tPJhZLcRRZEMz+/QW+bh5YQM+ndns3VQwhf8vRvdDMtMg1Qv7vCNO37Yfck0NdCw66YOT\n\tvqF2z6SjXND2Mb4+GSL+K+nB1hBCERqJyQue9CpoPHu3qgPSV/LgdaFY5subkEnCPkM8\n\t8FBw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=s6tmbNqyIqtco6ZG1YSyqueCmq+t7MqiAjYBq/AREOQ=;\n\tb=ZqZJxFvtkA9m2b0sEFo7LliwDtO0Eajd79zI7LIUddSsx19YlBcHpH7kAVHkq2O/OF\n\tFJ/rCTXiKIPBv1d9WTR6t7JV6XC2XT3kVqCW0XChgLSItHWCwclY5sUkHqWeI6QWQEvW\n\t5M8IClBFVzRjuw/J8LAeAQipdLt9+HkufcMnFhO/h6hniq6el9qzz/UZWxU1EEtKYGUI\n\txhojrwheXo8M9IGDXc654SBF3xpQZPW3Xp3eCyonc3q98Wa3tSC61oRIGVZkMQYtImhS\n\tshWCgzvmq8MA7ZNgQXbdYEdQJDPKOUO8jg3nBqPiKHBYU8lH8LUmcuN7O0nJbs4MRIM1\n\tXoCA==","X-Gm-Message-State":"AOAM533B3YID1yFIe4L5m7RULujkUmdPrg6qYiNovZxGMhsDNhLW/F/J\n\tOmzPIVIfvdqNp+e0qboBQJtVJzbVPO9vfWF3nq44vQ==","X-Google-Smtp-Source":"ABdhPJwbck5dVw7hgyJOpN/GM4zgGR90ZmOH2AY3Y43lB+0YpAlNHa+o0Nl3co59a83e5ILO7rMAW4ZV5XIAAuQ0+bQ=","X-Received":"by 2002:a2e:a230:: with SMTP id\n\ti16mr9442633ljm.169.1624018999538; \n\tFri, 18 Jun 2021 05:23:19 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-5-paul.elder@ideasonboard.com>","In-Reply-To":"<20210618103351.1642060-5-paul.elder@ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Fri, 18 Jun 2021 13:23:03 +0100","Message-ID":"<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"00000000000093882205c50963f6\"","Subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17828,"web_url":"https://patchwork.libcamera.org/comment/17828/","msgid":"<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>","date":"2021-06-28T01:06:29","subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for your patch.\n\nOn Fri, Jun 18, 2021 at 01:23:03PM +0100, Naushir Patuck wrote:\n> On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:\n> > Previously it was possible to have AwbEnable set to false, yet have\n> > AwbMode on anything. This caused a confusion situation, so merge the two\n> > into AwbMode. While at it, pull in the android AWB modes.\n\nI'd say \"pull in additional AWB modes defined by Android\" (or s/pull\nin/add).\n\n> >\n> > Adjust the previous users of AwbEnable accordingly.\n> >\n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > ---\n> >  include/libcamera/ipa/raspberrypi.h |  1 -\n> >  src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------\n> >  src/libcamera/control_ids.yaml      | 32 +++++++++++++++--------------\n> >  test/controls/control_list.cpp      |  6 +++---\n> >  4 files changed, 29 insertions(+), 37 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index a8790000..63392a26 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {\n> >         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> >         { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> >         { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > -       { &controls::AwbEnable, ControlInfo(false, true) },\n> >         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> >         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> >         { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index ad0132c0..ed5f1250 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList &controls)\n> >                         break;\n> >                 }\n> >\n> > -               case controls::AWB_ENABLE: {\n> > -                       RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > -                       if (!awb) {\n> > -                               LOG(IPARPI, Warning)\n> > -                                       << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > -                               break;\n> > -                       }\n> > -\n> > -                       if (ctrl.second.get<bool>() == false)\n> > -                               awb->Pause();\n> > -                       else\n> > -                               awb->Resume();\n> > -\n> > -                       libcameraMetadata_.set(controls::AwbEnable,\n> > -                                              ctrl.second.get<bool>());\n> > -                       break;\n> > -               }\n> > -\n> >                 case controls::AWB_MODE: {\n> >                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> >                                 controller_.GetAlgorithm(\"awb\"));\n> > @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList &controls)\n> >                         }\n> >\n> >                         int32_t idx = ctrl.second.get<int32_t>();\n> > +\n> > +                       if (idx == controls::AwbOff) {\n> > +                               awb->Pause();\n> > +                               break;\n> > +                       }\n> > +\n> > +                       if (idx == controls::AwbAuto)\n> > +                               awb->Resume();\n> \n> I think the logic here may need adjusting such that any state that is not\n> controls::AwbOff will need to call awb->Resume(), or conditionally call\n> resume if adb->IsPaused() returns true.\n\nAgreed. The RPi AWB implementation differs from the behaviour specified\nby Android in that the RPi AWB algorithm isn't disabled when the mode is\nset to one of the presets. The presets instead serve to limit the search\nrange of the AWB algorithm, instead of setting hardcoded manual values\nas in Android.\n\nNaush, what would happen if the tuning file specified a fixed colour\ntemperature (by setting the min and max to the same value) for AWB\npresets ? Would the AWB algorithm then always produce fixed values for\nthe colour gains ?\n\n> >                         if (AwbModeTable.count(idx)) {\n> >                                 awb->SetMode(AwbModeTable.at(idx));\n> >                                 libcameraMetadata_.set(controls::AwbMode, idx);\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 5717bc1f..2e62f61b 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -229,13 +229,6 @@ controls:\n> >          Report an estimate of the current illuminance level in lux. The Lux\n> >          control can only be returned in metadata.\n> >\n> > -  - AwbEnable:\n> > -      type: bool\n> > -      description: |\n> > -        Enable or disable the AWB.\n> > -\n> > -        \\sa ColourGains\n> > -\n> >    # AwbMode needs further attention:\n> >    # - Auto-generate max enum value.\n> >    # - Better handling of custom types.\n> > @@ -245,29 +238,38 @@ controls:\n> >          Specify the range of illuminants to use for the AWB algorithm. The modes\n> >          supported are platform specific, and not all modes may be supported.\n> >        enum:\n> > -        - name: AwbAuto\n> > +        - name: AwbOff\n> >            value: 0\n> > +          description: The AWB routune is disabled.\n\ns/routine/\n\n> > +        - name: AwbAuto\n> > +          value: 1\n> >            description: Search over the whole colour temperature range.\n> >          - name: AwbIncandescent\n> > -          value: 1\n> > -          description: Incandescent AWB lamp mode.\n> > -        - name: AwbTungsten\n> >            value: 2\n> > -          description: Tungsten AWB lamp mode.\n> > +          description: Incandescent AWB lamp mode.\n> >          - name: AwbFluorescent\n> >            value: 3\n> >            description: Fluorescent AWB lamp mode.\n> > -        - name: AwbIndoor\n> > +        - name: AwbWarmFluorescent\n> >            value: 4\n> > -          description: Indoor AWB lighting mode.\n> > +          description: Warm fluorescent AWB lamp mode.\n> >          - name: AwbDaylight\n> >            value: 5\n> >            description: Daylight AWB lighting mode.\n> >          - name: AwbCloudy\n> >            value: 6\n> >            description: Cloudy AWB lighting mode.\n> > -        - name: AwbCustom\n> > +        - name: AwbTwilight\n> >            value: 7\n> > +          description: Twilight AWB lamp mode.\n> > +        - name: AwbTungsten\n> > +          value: 8\n> > +          description: Tungsten AWB lamp mode.\n> > +        - name: AwbIndoor\n> > +          value: 9\n> > +          description: Indoor AWB lighting mode.\n> > +        - name: AwbCustom\n> > +          value: 10\n> >            description: Custom AWB mode.\n> >\n> >    - AwbLocked:\n> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > index 70cf61b8..ce55d09b 100644\n> > --- a/test/controls/control_list.cpp\n> > +++ b/test/controls/control_list.cpp\n> > @@ -143,10 +143,10 @@ protected:\n> >                  * Attempt to set an invalid control and verify that the\n> >                  * operation failed.\n> >                  */\n> > -               list.set(controls::AwbEnable, true);\n> > +               list.set(controls::AwbMode, true);\n> >\n> > -               if (list.contains(controls::AwbEnable)) {\n> > -                       cout << \"List shouldn't contain AwbEnable control\" << endl;\n> > +               if (list.contains(controls::AwbMode)) {\n> > +                       cout << \"List shouldn't contain AwbMode control\" << endl;\n> >                         return TestFail;\n> >                 }\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 91DA4C321D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Jun 2021 01:06:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F9A8684D7;\n\tMon, 28 Jun 2021 03:06:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00DE26028C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Jun 2021 03:06:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D8E557E;\n\tMon, 28 Jun 2021 03:06:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nKRkv3ta\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624842390;\n\tbh=GEfwmlrGP7Nn60vGPRBlvUS6jwdGoSbj3wf/H5EQHPQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nKRkv3tagOS/aV1p+SBd9MItLk7HTIaDc5rXxXdEUYU43NcAiXsPkHK5BmdXe+kN/\n\tJLfYQGdOFVfJSV4f4SDAk/ZsNEVNxETMihvvI8YUVttwW62Bdr1P/mEvKXhJGBn97m\n\tT450uPoGbX7LmyGxgvpp/GtY4ON6L/EtwuoxfgWo=","Date":"Mon, 28 Jun 2021 04:06:29 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-5-paul.elder@ideasonboard.com>\n\t<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17830,"web_url":"https://patchwork.libcamera.org/comment/17830/","msgid":"<YNkkZr3Fj20cujFT@pendragon.ideasonboard.com>","date":"2021-06-28T01:22:46","subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nA related question, will there be a similar patch for AE ?\n\nOn Mon, Jun 28, 2021 at 04:06:30AM +0300, Laurent Pinchart wrote:\n> On Fri, Jun 18, 2021 at 01:23:03PM +0100, Naushir Patuck wrote:\n> > On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:\n> > > Previously it was possible to have AwbEnable set to false, yet have\n> > > AwbMode on anything. This caused a confusion situation, so merge the two\n> > > into AwbMode. While at it, pull in the android AWB modes.\n> \n> I'd say \"pull in additional AWB modes defined by Android\" (or s/pull\n> in/add).\n> \n> > >\n> > > Adjust the previous users of AwbEnable accordingly.\n> > >\n> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/ipa/raspberrypi.h |  1 -\n> > >  src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------\n> > >  src/libcamera/control_ids.yaml      | 32 +++++++++++++++--------------\n> > >  test/controls/control_list.cpp      |  6 +++---\n> > >  4 files changed, 29 insertions(+), 37 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index a8790000..63392a26 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {\n> > >         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > >         { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > >         { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > > -       { &controls::AwbEnable, ControlInfo(false, true) },\n> > >         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > >         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > >         { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index ad0132c0..ed5f1250 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > >                         break;\n> > >                 }\n> > >\n> > > -               case controls::AWB_ENABLE: {\n> > > -                       RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > -                       if (!awb) {\n> > > -                               LOG(IPARPI, Warning)\n> > > -                                       << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > -                               break;\n> > > -                       }\n> > > -\n> > > -                       if (ctrl.second.get<bool>() == false)\n> > > -                               awb->Pause();\n> > > -                       else\n> > > -                               awb->Resume();\n> > > -\n> > > -                       libcameraMetadata_.set(controls::AwbEnable,\n> > > -                                              ctrl.second.get<bool>());\n> > > -                       break;\n> > > -               }\n> > > -\n> > >                 case controls::AWB_MODE: {\n> > >                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > >                                 controller_.GetAlgorithm(\"awb\"));\n> > > @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > >                         }\n> > >\n> > >                         int32_t idx = ctrl.second.get<int32_t>();\n> > > +\n> > > +                       if (idx == controls::AwbOff) {\n> > > +                               awb->Pause();\n> > > +                               break;\n> > > +                       }\n> > > +\n> > > +                       if (idx == controls::AwbAuto)\n> > > +                               awb->Resume();\n> > \n> > I think the logic here may need adjusting such that any state that is not\n> > controls::AwbOff will need to call awb->Resume(), or conditionally call\n> > resume if adb->IsPaused() returns true.\n> \n> Agreed. The RPi AWB implementation differs from the behaviour specified\n> by Android in that the RPi AWB algorithm isn't disabled when the mode is\n> set to one of the presets. The presets instead serve to limit the search\n> range of the AWB algorithm, instead of setting hardcoded manual values\n> as in Android.\n> \n> Naush, what would happen if the tuning file specified a fixed colour\n> temperature (by setting the min and max to the same value) for AWB\n> presets ? Would the AWB algorithm then always produce fixed values for\n> the colour gains ?\n> \n> > >                         if (AwbModeTable.count(idx)) {\n> > >                                 awb->SetMode(AwbModeTable.at(idx));\n> > >                                 libcameraMetadata_.set(controls::AwbMode, idx);\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 5717bc1f..2e62f61b 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -229,13 +229,6 @@ controls:\n> > >          Report an estimate of the current illuminance level in lux. The Lux\n> > >          control can only be returned in metadata.\n> > >\n> > > -  - AwbEnable:\n> > > -      type: bool\n> > > -      description: |\n> > > -        Enable or disable the AWB.\n> > > -\n> > > -        \\sa ColourGains\n> > > -\n> > >    # AwbMode needs further attention:\n> > >    # - Auto-generate max enum value.\n> > >    # - Better handling of custom types.\n> > > @@ -245,29 +238,38 @@ controls:\n> > >          Specify the range of illuminants to use for the AWB algorithm. The modes\n> > >          supported are platform specific, and not all modes may be supported.\n> > >        enum:\n> > > -        - name: AwbAuto\n> > > +        - name: AwbOff\n> > >            value: 0\n> > > +          description: The AWB routune is disabled.\n> \n> s/routine/\n> \n> > > +        - name: AwbAuto\n> > > +          value: 1\n> > >            description: Search over the whole colour temperature range.\n> > >          - name: AwbIncandescent\n> > > -          value: 1\n> > > -          description: Incandescent AWB lamp mode.\n> > > -        - name: AwbTungsten\n> > >            value: 2\n> > > -          description: Tungsten AWB lamp mode.\n> > > +          description: Incandescent AWB lamp mode.\n> > >          - name: AwbFluorescent\n> > >            value: 3\n> > >            description: Fluorescent AWB lamp mode.\n> > > -        - name: AwbIndoor\n> > > +        - name: AwbWarmFluorescent\n> > >            value: 4\n> > > -          description: Indoor AWB lighting mode.\n> > > +          description: Warm fluorescent AWB lamp mode.\n> > >          - name: AwbDaylight\n> > >            value: 5\n> > >            description: Daylight AWB lighting mode.\n> > >          - name: AwbCloudy\n> > >            value: 6\n> > >            description: Cloudy AWB lighting mode.\n> > > -        - name: AwbCustom\n> > > +        - name: AwbTwilight\n> > >            value: 7\n> > > +          description: Twilight AWB lamp mode.\n> > > +        - name: AwbTungsten\n> > > +          value: 8\n> > > +          description: Tungsten AWB lamp mode.\n> > > +        - name: AwbIndoor\n> > > +          value: 9\n> > > +          description: Indoor AWB lighting mode.\n> > > +        - name: AwbCustom\n> > > +          value: 10\n> > >            description: Custom AWB mode.\n> > >\n> > >    - AwbLocked:\n> > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > > index 70cf61b8..ce55d09b 100644\n> > > --- a/test/controls/control_list.cpp\n> > > +++ b/test/controls/control_list.cpp\n> > > @@ -143,10 +143,10 @@ protected:\n> > >                  * Attempt to set an invalid control and verify that the\n> > >                  * operation failed.\n> > >                  */\n> > > -               list.set(controls::AwbEnable, true);\n> > > +               list.set(controls::AwbMode, true);\n> > >\n> > > -               if (list.contains(controls::AwbEnable)) {\n> > > -                       cout << \"List shouldn't contain AwbEnable control\" << endl;\n> > > +               if (list.contains(controls::AwbMode)) {\n> > > +                       cout << \"List shouldn't contain AwbMode control\" << endl;\n> > >                         return TestFail;\n> > >                 }\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3A0C4C321A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Jun 2021 01:22:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82242684D5;\n\tMon, 28 Jun 2021 03:22:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70B216028C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Jun 2021 03:22:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3BB457E;\n\tMon, 28 Jun 2021 03:22:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tSOxofn2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624843368;\n\tbh=TGc7p9PT22bY1vOc9y8Qq6Sebhv/MynRufSvhyOBkxM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tSOxofn2fCQlSXQJSjZSOSkt98miKcIGdQ7WD9SzVKiDka4DneED46G0p3aUSHaUK\n\taSfmOnXDTtXWAzMUMocOupzVTX+hlIPF7FsHotY0af+78/7r6Yfr7P9dY8hsGX2tCQ\n\tPeFe0GOLsCFPjyGCImtAg4ai0o6r+c1W12MvC4jE=","Date":"Mon, 28 Jun 2021 04:22:46 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<YNkkZr3Fj20cujFT@pendragon.ideasonboard.com>","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-5-paul.elder@ideasonboard.com>\n\t<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>\n\t<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17879,"web_url":"https://patchwork.libcamera.org/comment/17879/","msgid":"<CAEmqJPpY5imKd-02zmPotv9T8TD6+21Rf_rL-nQTGL2VN53Vkw@mail.gmail.com>","date":"2021-06-28T09:13:35","subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Mon, 28 Jun 2021 at 02:06, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Paul,\n>\n> Thank you for your patch.\n>\n> On Fri, Jun 18, 2021 at 01:23:03PM +0100, Naushir Patuck wrote:\n> > On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:\n> > > Previously it was possible to have AwbEnable set to false, yet have\n> > > AwbMode on anything. This caused a confusion situation, so merge the\n> two\n> > > into AwbMode. While at it, pull in the android AWB modes.\n>\n> I'd say \"pull in additional AWB modes defined by Android\" (or s/pull\n> in/add).\n>\n> > >\n> > > Adjust the previous users of AwbEnable accordingly.\n> > >\n> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/ipa/raspberrypi.h |  1 -\n> > >  src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------\n> > >  src/libcamera/control_ids.yaml      | 32 +++++++++++++++--------------\n> > >  test/controls/control_list.cpp      |  6 +++---\n> > >  4 files changed, 29 insertions(+), 37 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > > index a8790000..63392a26 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {\n> > >         { &controls::AeConstraintMode,\n> ControlInfo(controls::AeConstraintModeValues) },\n> > >         { &controls::AeExposureMode,\n> ControlInfo(controls::AeExposureModeValues) },\n> > >         { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > > -       { &controls::AwbEnable, ControlInfo(false, true) },\n> > >         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > >         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > >         { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index ad0132c0..ed5f1250 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > >                         break;\n> > >                 }\n> > >\n> > > -               case controls::AWB_ENABLE: {\n> > > -                       RPiController::Algorithm *awb =\n> controller_.GetAlgorithm(\"awb\");\n> > > -                       if (!awb) {\n> > > -                               LOG(IPARPI, Warning)\n> > > -                                       << \"Could not set AWB_ENABLE -\n> no AWB algorithm\";\n> > > -                               break;\n> > > -                       }\n> > > -\n> > > -                       if (ctrl.second.get<bool>() == false)\n> > > -                               awb->Pause();\n> > > -                       else\n> > > -                               awb->Resume();\n> > > -\n> > > -                       libcameraMetadata_.set(controls::AwbEnable,\n> > > -\n> ctrl.second.get<bool>());\n> > > -                       break;\n> > > -               }\n> > > -\n> > >                 case controls::AWB_MODE: {\n> > >                         RPiController::AwbAlgorithm *awb =\n> dynamic_cast<RPiController::AwbAlgorithm *>(\n> > >                                 controller_.GetAlgorithm(\"awb\"));\n> > > @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > >                         }\n> > >\n> > >                         int32_t idx = ctrl.second.get<int32_t>();\n> > > +\n> > > +                       if (idx == controls::AwbOff) {\n> > > +                               awb->Pause();\n> > > +                               break;\n> > > +                       }\n> > > +\n> > > +                       if (idx == controls::AwbAuto)\n> > > +                               awb->Resume();\n> >\n> > I think the logic here may need adjusting such that any state that is not\n> > controls::AwbOff will need to call awb->Resume(), or conditionally call\n> > resume if adb->IsPaused() returns true.\n>\n> Agreed. The RPi AWB implementation differs from the behaviour specified\n> by Android in that the RPi AWB algorithm isn't disabled when the mode is\n> set to one of the presets. The presets instead serve to limit the search\n> range of the AWB algorithm, instead of setting hardcoded manual values\n> as in Android.\n>\n> Naush, what would happen if the tuning file specified a fixed colour\n> temperature (by setting the min and max to the same value) for AWB\n> presets ? Would the AWB algorithm then always produce fixed values for\n> the colour gains ?\n>\n\nIn this case the RPi AWB will (or at least should, I've never tried :))\nwill still\nsearch a small patch around the region.  So you may not necessarily get the\nexact same gain values for every frame, but they will be close.\n\nRegards,\nNaush\n\n\n>\n> > >                         if (AwbModeTable.count(idx)) {\n> > >                                 awb->SetMode(AwbModeTable.at(idx));\n> > >\n>  libcameraMetadata_.set(controls::AwbMode, idx);\n> > > diff --git a/src/libcamera/control_ids.yaml\n> b/src/libcamera/control_ids.yaml\n> > > index 5717bc1f..2e62f61b 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -229,13 +229,6 @@ controls:\n> > >          Report an estimate of the current illuminance level in lux.\n> The Lux\n> > >          control can only be returned in metadata.\n> > >\n> > > -  - AwbEnable:\n> > > -      type: bool\n> > > -      description: |\n> > > -        Enable or disable the AWB.\n> > > -\n> > > -        \\sa ColourGains\n> > > -\n> > >    # AwbMode needs further attention:\n> > >    # - Auto-generate max enum value.\n> > >    # - Better handling of custom types.\n> > > @@ -245,29 +238,38 @@ controls:\n> > >          Specify the range of illuminants to use for the AWB\n> algorithm. The modes\n> > >          supported are platform specific, and not all modes may be\n> supported.\n> > >        enum:\n> > > -        - name: AwbAuto\n> > > +        - name: AwbOff\n> > >            value: 0\n> > > +          description: The AWB routune is disabled.\n>\n> s/routine/\n>\n> > > +        - name: AwbAuto\n> > > +          value: 1\n> > >            description: Search over the whole colour temperature range.\n> > >          - name: AwbIncandescent\n> > > -          value: 1\n> > > -          description: Incandescent AWB lamp mode.\n> > > -        - name: AwbTungsten\n> > >            value: 2\n> > > -          description: Tungsten AWB lamp mode.\n> > > +          description: Incandescent AWB lamp mode.\n> > >          - name: AwbFluorescent\n> > >            value: 3\n> > >            description: Fluorescent AWB lamp mode.\n> > > -        - name: AwbIndoor\n> > > +        - name: AwbWarmFluorescent\n> > >            value: 4\n> > > -          description: Indoor AWB lighting mode.\n> > > +          description: Warm fluorescent AWB lamp mode.\n> > >          - name: AwbDaylight\n> > >            value: 5\n> > >            description: Daylight AWB lighting mode.\n> > >          - name: AwbCloudy\n> > >            value: 6\n> > >            description: Cloudy AWB lighting mode.\n> > > -        - name: AwbCustom\n> > > +        - name: AwbTwilight\n> > >            value: 7\n> > > +          description: Twilight AWB lamp mode.\n> > > +        - name: AwbTungsten\n> > > +          value: 8\n> > > +          description: Tungsten AWB lamp mode.\n> > > +        - name: AwbIndoor\n> > > +          value: 9\n> > > +          description: Indoor AWB lighting mode.\n> > > +        - name: AwbCustom\n> > > +          value: 10\n> > >            description: Custom AWB mode.\n> > >\n> > >    - AwbLocked:\n> > > diff --git a/test/controls/control_list.cpp\n> b/test/controls/control_list.cpp\n> > > index 70cf61b8..ce55d09b 100644\n> > > --- a/test/controls/control_list.cpp\n> > > +++ b/test/controls/control_list.cpp\n> > > @@ -143,10 +143,10 @@ protected:\n> > >                  * Attempt to set an invalid control and verify that\n> the\n> > >                  * operation failed.\n> > >                  */\n> > > -               list.set(controls::AwbEnable, true);\n> > > +               list.set(controls::AwbMode, true);\n> > >\n> > > -               if (list.contains(controls::AwbEnable)) {\n> > > -                       cout << \"List shouldn't contain AwbEnable\n> control\" << endl;\n> > > +               if (list.contains(controls::AwbMode)) {\n> > > +                       cout << \"List shouldn't contain AwbMode\n> control\" << endl;\n> > >                         return TestFail;\n> > >                 }\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5894AC3220\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Jun 2021 09:13:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B21A0684D8;\n\tMon, 28 Jun 2021 11:13:54 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C9FE684D2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Jun 2021 11:13:52 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id q16so17163031lfr.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Jun 2021 02:13:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"bHkxCY7H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=EqvIsFLPhqUY/t4+1BuY//ae1jb48f1W6ycBKinut6o=;\n\tb=bHkxCY7H5zT57yT9COAMiPMjDTME2yb4zIQ6oe78j90iMzoObCIrPk2xmHe8lMSeCI\n\tSgCJVVp8wZzXedJCAbXLxzO6Z87KLJhExbaabcXPAKC3Z4hYq5bpBDiLScS07tENr8bh\n\tc3wHSUACLDt+770silkGM9YLTWNpGsoyCEh6CNSAiyTwskS1tEpM0FS3vKtoUMcztjnl\n\tDJBwiMBCDHwrSdskKKuonGVNCg3cITTusz7jJnZ+4B/gusho5Ek08A1TfFABcqWOQ664\n\t3zY4K6pIH9j4bR+goSbcW03NWREfXtCwJz/s6XuBXTqAu8kC2kV2E4a2aKvezKprVfj0\n\tH+xg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=EqvIsFLPhqUY/t4+1BuY//ae1jb48f1W6ycBKinut6o=;\n\tb=hmGVuL5F72inaoKVQ/Q1Q2tGzrxZFK/HHead7TSj2tnkD3Mh6zS6CCl0sLFHP0He8G\n\tXvHWkB3EuK4iYhBhuVoNtwmf7eR4rhwjXwTztnyFeazk3KMmxFdQE8Ze2SbOrJn3a9OB\n\tLz+XmAO1/Dyp0wcG8LzNzH4x9qCRvw2ZKjUrXWCDWcdM2oYQVE56WR2wdaCB6S4rWW1u\n\tgbXOQIVEbYXHVzDTCc+fkoyRmvnDLOofvJ7GLNLskxJbfjNXX8Nym8UElSlmC7qvO0Qt\n\tlKYmlJGACC0JeglFK+Cc4VMqDl+qECf5PQF2RVCHTQAUcaTKlNKpfs33u8lgME4F5oxY\n\tVAVQ==","X-Gm-Message-State":"AOAM533TwG1QY15a5KqKcRkR2jj4KlTz7sXb5SmbBiq239IX2k3OwSvD\n\th6kcCJqutnWxwtuzibZaoeFr6v1KH6M/XuUh2/7qrHy2mIE=","X-Google-Smtp-Source":"ABdhPJxX0l+slQ0SSGiqH2IwB9O/yf9U+nDgidY5uXDmFY+fpe8+PHwBnoh8MHEibOzrDf7B/WsvqYK8L0A7yKrADAI=","X-Received":"by 2002:a05:6512:3ba9:: with SMTP id\n\tg41mr11802528lfv.567.1624871631077; \n\tMon, 28 Jun 2021 02:13:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-5-paul.elder@ideasonboard.com>\n\t<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>\n\t<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>","In-Reply-To":"<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Mon, 28 Jun 2021 10:13:35 +0100","Message-ID":"<CAEmqJPpY5imKd-02zmPotv9T8TD6+21Rf_rL-nQTGL2VN53Vkw@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"00000000000060560605c5cfe8ea\"","Subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17892,"web_url":"https://patchwork.libcamera.org/comment/17892/","msgid":"<CAHW6GYKOYd3tz2y=hCpTWcmJmJoorafNQEKmKU28NXsSym5rbA@mail.gmail.com>","date":"2021-06-28T10:54:35","subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Just to add that I think it's wrong to fix the colour gains when a preset\nAWB mode is chosen. Illuminants can differ in other ways besides colour\ntemperature - for example, some lamps are greener than others, especially\ntrue of some modern fluorescent lights. The RPi approach will still take\ncare of this, but algorithms that simply use fixed colour gains can leave\nyou with greenish or purplish images. (Of course, we still let you set\nexplicit values using the control mechanism.)\n\nDavid\n\nOn Mon, 28 Jun 2021 at 10:13, Naushir Patuck <naush@raspberrypi.com> wrote:\n\n> Hi Laurent,\n>\n> On Mon, 28 Jun 2021 at 02:06, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi Paul,\n>>\n>> Thank you for your patch.\n>>\n>> On Fri, Jun 18, 2021 at 01:23:03PM +0100, Naushir Patuck wrote:\n>> > On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:\n>> > > Previously it was possible to have AwbEnable set to false, yet have\n>> > > AwbMode on anything. This caused a confusion situation, so merge the\n>> two\n>> > > into AwbMode. While at it, pull in the android AWB modes.\n>>\n>> I'd say \"pull in additional AWB modes defined by Android\" (or s/pull\n>> in/add).\n>>\n>> > >\n>> > > Adjust the previous users of AwbEnable accordingly.\n>> > >\n>> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n>> > > ---\n>> > >  include/libcamera/ipa/raspberrypi.h |  1 -\n>> > >  src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------\n>> > >  src/libcamera/control_ids.yaml      | 32\n>> +++++++++++++++--------------\n>> > >  test/controls/control_list.cpp      |  6 +++---\n>> > >  4 files changed, 29 insertions(+), 37 deletions(-)\n>> > >\n>> > > diff --git a/include/libcamera/ipa/raspberrypi.h\n>> b/include/libcamera/ipa/raspberrypi.h\n>> > > index a8790000..63392a26 100644\n>> > > --- a/include/libcamera/ipa/raspberrypi.h\n>> > > +++ b/include/libcamera/ipa/raspberrypi.h\n>> > > @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {\n>> > >         { &controls::AeConstraintMode,\n>> ControlInfo(controls::AeConstraintModeValues) },\n>> > >         { &controls::AeExposureMode,\n>> ControlInfo(controls::AeExposureModeValues) },\n>> > >         { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n>> > > -       { &controls::AwbEnable, ControlInfo(false, true) },\n>> > >         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>> > >         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>> > >         { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > index ad0132c0..ed5f1250 100644\n>> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList\n>> &controls)\n>> > >                         break;\n>> > >                 }\n>> > >\n>> > > -               case controls::AWB_ENABLE: {\n>> > > -                       RPiController::Algorithm *awb =\n>> controller_.GetAlgorithm(\"awb\");\n>> > > -                       if (!awb) {\n>> > > -                               LOG(IPARPI, Warning)\n>> > > -                                       << \"Could not set AWB_ENABLE\n>> - no AWB algorithm\";\n>> > > -                               break;\n>> > > -                       }\n>> > > -\n>> > > -                       if (ctrl.second.get<bool>() == false)\n>> > > -                               awb->Pause();\n>> > > -                       else\n>> > > -                               awb->Resume();\n>> > > -\n>> > > -                       libcameraMetadata_.set(controls::AwbEnable,\n>> > > -\n>> ctrl.second.get<bool>());\n>> > > -                       break;\n>> > > -               }\n>> > > -\n>> > >                 case controls::AWB_MODE: {\n>> > >                         RPiController::AwbAlgorithm *awb =\n>> dynamic_cast<RPiController::AwbAlgorithm *>(\n>> > >                                 controller_.GetAlgorithm(\"awb\"));\n>> > > @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList\n>> &controls)\n>> > >                         }\n>> > >\n>> > >                         int32_t idx = ctrl.second.get<int32_t>();\n>> > > +\n>> > > +                       if (idx == controls::AwbOff) {\n>> > > +                               awb->Pause();\n>> > > +                               break;\n>> > > +                       }\n>> > > +\n>> > > +                       if (idx == controls::AwbAuto)\n>> > > +                               awb->Resume();\n>> >\n>> > I think the logic here may need adjusting such that any state that is\n>> not\n>> > controls::AwbOff will need to call awb->Resume(), or conditionally call\n>> > resume if adb->IsPaused() returns true.\n>>\n>> Agreed. The RPi AWB implementation differs from the behaviour specified\n>> by Android in that the RPi AWB algorithm isn't disabled when the mode is\n>> set to one of the presets. The presets instead serve to limit the search\n>> range of the AWB algorithm, instead of setting hardcoded manual values\n>> as in Android.\n>>\n>> Naush, what would happen if the tuning file specified a fixed colour\n>> temperature (by setting the min and max to the same value) for AWB\n>> presets ? Would the AWB algorithm then always produce fixed values for\n>> the colour gains ?\n>>\n>\n> In this case the RPi AWB will (or at least should, I've never tried :))\n> will still\n> search a small patch around the region.  So you may not necessarily get the\n> exact same gain values for every frame, but they will be close.\n>\n> Regards,\n> Naush\n>\n>\n>>\n>> > >                         if (AwbModeTable.count(idx)) {\n>> > >                                 awb->SetMode(AwbModeTable.at(idx));\n>> > >\n>>  libcameraMetadata_.set(controls::AwbMode, idx);\n>> > > diff --git a/src/libcamera/control_ids.yaml\n>> b/src/libcamera/control_ids.yaml\n>> > > index 5717bc1f..2e62f61b 100644\n>> > > --- a/src/libcamera/control_ids.yaml\n>> > > +++ b/src/libcamera/control_ids.yaml\n>> > > @@ -229,13 +229,6 @@ controls:\n>> > >          Report an estimate of the current illuminance level in lux.\n>> The Lux\n>> > >          control can only be returned in metadata.\n>> > >\n>> > > -  - AwbEnable:\n>> > > -      type: bool\n>> > > -      description: |\n>> > > -        Enable or disable the AWB.\n>> > > -\n>> > > -        \\sa ColourGains\n>> > > -\n>> > >    # AwbMode needs further attention:\n>> > >    # - Auto-generate max enum value.\n>> > >    # - Better handling of custom types.\n>> > > @@ -245,29 +238,38 @@ controls:\n>> > >          Specify the range of illuminants to use for the AWB\n>> algorithm. The modes\n>> > >          supported are platform specific, and not all modes may be\n>> supported.\n>> > >        enum:\n>> > > -        - name: AwbAuto\n>> > > +        - name: AwbOff\n>> > >            value: 0\n>> > > +          description: The AWB routune is disabled.\n>>\n>> s/routine/\n>>\n>> > > +        - name: AwbAuto\n>> > > +          value: 1\n>> > >            description: Search over the whole colour temperature\n>> range.\n>> > >          - name: AwbIncandescent\n>> > > -          value: 1\n>> > > -          description: Incandescent AWB lamp mode.\n>> > > -        - name: AwbTungsten\n>> > >            value: 2\n>> > > -          description: Tungsten AWB lamp mode.\n>> > > +          description: Incandescent AWB lamp mode.\n>> > >          - name: AwbFluorescent\n>> > >            value: 3\n>> > >            description: Fluorescent AWB lamp mode.\n>> > > -        - name: AwbIndoor\n>> > > +        - name: AwbWarmFluorescent\n>> > >            value: 4\n>> > > -          description: Indoor AWB lighting mode.\n>> > > +          description: Warm fluorescent AWB lamp mode.\n>> > >          - name: AwbDaylight\n>> > >            value: 5\n>> > >            description: Daylight AWB lighting mode.\n>> > >          - name: AwbCloudy\n>> > >            value: 6\n>> > >            description: Cloudy AWB lighting mode.\n>> > > -        - name: AwbCustom\n>> > > +        - name: AwbTwilight\n>> > >            value: 7\n>> > > +          description: Twilight AWB lamp mode.\n>> > > +        - name: AwbTungsten\n>> > > +          value: 8\n>> > > +          description: Tungsten AWB lamp mode.\n>> > > +        - name: AwbIndoor\n>> > > +          value: 9\n>> > > +          description: Indoor AWB lighting mode.\n>> > > +        - name: AwbCustom\n>> > > +          value: 10\n>> > >            description: Custom AWB mode.\n>> > >\n>> > >    - AwbLocked:\n>> > > diff --git a/test/controls/control_list.cpp\n>> b/test/controls/control_list.cpp\n>> > > index 70cf61b8..ce55d09b 100644\n>> > > --- a/test/controls/control_list.cpp\n>> > > +++ b/test/controls/control_list.cpp\n>> > > @@ -143,10 +143,10 @@ protected:\n>> > >                  * Attempt to set an invalid control and verify that\n>> the\n>> > >                  * operation failed.\n>> > >                  */\n>> > > -               list.set(controls::AwbEnable, true);\n>> > > +               list.set(controls::AwbMode, true);\n>> > >\n>> > > -               if (list.contains(controls::AwbEnable)) {\n>> > > -                       cout << \"List shouldn't contain AwbEnable\n>> control\" << endl;\n>> > > +               if (list.contains(controls::AwbMode)) {\n>> > > +                       cout << \"List shouldn't contain AwbMode\n>> control\" << endl;\n>> > >                         return TestFail;\n>> > >                 }\n>> > >\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D33B5C321F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Jun 2021 10:54:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E0E5684D8;\n\tMon, 28 Jun 2021 12:54:48 +0200 (CEST)","from mail-wr1-x432.google.com (mail-wr1-x432.google.com\n\t[IPv6:2a00:1450:4864:20::432])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6B42684D4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Jun 2021 12:54:46 +0200 (CEST)","by mail-wr1-x432.google.com with SMTP id j2so20717839wrs.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Jun 2021 03:54:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"EiiC9xyW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=T2ZDeZtMxYi50HuMgfj+B1kAwBNefPGB2U7B0LFWsdg=;\n\tb=EiiC9xyWWxmPOk6DUBPtivlu5YUxGElZkldsiuhTiejO8YaD+xZ7aQRxtbiO20Oh7O\n\tOUgjiTClpXMDwyUE7mSJRv/A3QvlDg1M7NWFq6EKxL2GnRYFvXC57SkrHrKUM9GZ9I2m\n\trdlanWW7Pesv+DCGEfq+++Mcn8pWlHKY0ifxJrJNYZsZHibiUAVoYkuTEirnu6t1O0Rc\n\tKDKFgSCdD+hUPI+KutPFvv/B75NaKob4hUQ0UoZNkVbdhopVuh8rzXk2HQFAUcgrU4nJ\n\t3lrGdqHI/uzZYBUZ84NsA5nmAADs5DULxIk8TQ14APyaNiBPagaOXttSLsusDH1dULEq\n\tZV7w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=T2ZDeZtMxYi50HuMgfj+B1kAwBNefPGB2U7B0LFWsdg=;\n\tb=RyJDR6q3TgFBjeCOMF7/6S73nCoYjpk6bMTqjxF6PNNqbOAjIfW/39MwJL5r6epcX8\n\t9gxLgoeYYlQjCEJbA/05PsYusCZkaj4if3Sd9tUcPNMK7VGr3j9qiOVZWnI+XfI1GuTT\n\toq1oiVjQ5H6jIwH+wLz6tJGmDT0WpbN4X4R8kGq9Iss+Q03MaMDC9YQ/0HIYqnc5lckw\n\tTTggZ1ew2GiCMgQ1kYZRVDlfK27Q1mzsKgrJXpMPjv3rgsihQSbiFoeZI495QIKqXKie\n\t8+W18e6NvPBxQHY3U9Pe4AJmaiKljUi4AF6Eo977bRMUMycTKfkAK2yNSq2mf5zfQiyr\n\tgnCA==","X-Gm-Message-State":"AOAM533t8vKyy3N6Uli+DAJI7VT/Iv4DMxs/p/dIO8TeGuebwK3N/Xe/\n\tJ9/QgJ4mCd/CwttHQHw6MvNomnXnhghy/PTnRMlfKQ==","X-Google-Smtp-Source":"ABdhPJxFUuNMVvQJ3ADfOmou8h0WapZPINiuOq+WKw7lkLTsvZQu3p3iyxFA5oTTvGBSTv1SY16hbgOp+76N/i0FKQw=","X-Received":"by 2002:a5d:4dd2:: with SMTP id\n\tf18mr26030258wru.86.1624877686503; \n\tMon, 28 Jun 2021 03:54:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-5-paul.elder@ideasonboard.com>\n\t<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>\n\t<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>\n\t<CAEmqJPpY5imKd-02zmPotv9T8TD6+21Rf_rL-nQTGL2VN53Vkw@mail.gmail.com>","In-Reply-To":"<CAEmqJPpY5imKd-02zmPotv9T8TD6+21Rf_rL-nQTGL2VN53Vkw@mail.gmail.com>","From":"David Plowman <david.plowman@raspberrypi.com>","Date":"Mon, 28 Jun 2021 11:54:35 +0100","Message-ID":"<CAHW6GYKOYd3tz2y=hCpTWcmJmJoorafNQEKmKU28NXsSym5rbA@mail.gmail.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Content-Type":"multipart/alternative; boundary=\"0000000000004eca3a05c5d1513a\"","Subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17949,"web_url":"https://patchwork.libcamera.org/comment/17949/","msgid":"<20210702095244.GA2510@pyrite.rasen.tech>","date":"2021-07-02T09:52:44","subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Mon, Jun 28, 2021 at 04:22:46AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> A related question, will there be a similar patch for AE ?\n\nThe android's AE modes besides off/auto are all just variations of\n\"needs flash\", and when we discussed this last iirc the conclusion was\nthat we don't need them, so off/on is sufficient.\n\n\nPaul\n\n> \n> On Mon, Jun 28, 2021 at 04:06:30AM +0300, Laurent Pinchart wrote:\n> > On Fri, Jun 18, 2021 at 01:23:03PM +0100, Naushir Patuck wrote:\n> > > On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:\n> > > > Previously it was possible to have AwbEnable set to false, yet have\n> > > > AwbMode on anything. This caused a confusion situation, so merge the two\n> > > > into AwbMode. While at it, pull in the android AWB modes.\n> > \n> > I'd say \"pull in additional AWB modes defined by Android\" (or s/pull\n> > in/add).\n> > \n> > > >\n> > > > Adjust the previous users of AwbEnable accordingly.\n> > > >\n> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > > ---\n> > > >  include/libcamera/ipa/raspberrypi.h |  1 -\n> > > >  src/ipa/raspberrypi/raspberrypi.cpp | 27 ++++++++----------------\n> > > >  src/libcamera/control_ids.yaml      | 32 +++++++++++++++--------------\n> > > >  test/controls/control_list.cpp      |  6 +++---\n> > > >  4 files changed, 29 insertions(+), 37 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > > index a8790000..63392a26 100644\n> > > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > > @@ -35,7 +35,6 @@ static const ControlInfoMap Controls = {\n> > > >         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > > >         { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > > >         { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > > > -       { &controls::AwbEnable, ControlInfo(false, true) },\n> > > >         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > >         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > >         { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index ad0132c0..ed5f1250 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -745,24 +745,6 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > >                         break;\n> > > >                 }\n> > > >\n> > > > -               case controls::AWB_ENABLE: {\n> > > > -                       RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > > -                       if (!awb) {\n> > > > -                               LOG(IPARPI, Warning)\n> > > > -                                       << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > > -                               break;\n> > > > -                       }\n> > > > -\n> > > > -                       if (ctrl.second.get<bool>() == false)\n> > > > -                               awb->Pause();\n> > > > -                       else\n> > > > -                               awb->Resume();\n> > > > -\n> > > > -                       libcameraMetadata_.set(controls::AwbEnable,\n> > > > -                                              ctrl.second.get<bool>());\n> > > > -                       break;\n> > > > -               }\n> > > > -\n> > > >                 case controls::AWB_MODE: {\n> > > >                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > > >                                 controller_.GetAlgorithm(\"awb\"));\n> > > > @@ -773,6 +755,15 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > >                         }\n> > > >\n> > > >                         int32_t idx = ctrl.second.get<int32_t>();\n> > > > +\n> > > > +                       if (idx == controls::AwbOff) {\n> > > > +                               awb->Pause();\n> > > > +                               break;\n> > > > +                       }\n> > > > +\n> > > > +                       if (idx == controls::AwbAuto)\n> > > > +                               awb->Resume();\n> > > \n> > > I think the logic here may need adjusting such that any state that is not\n> > > controls::AwbOff will need to call awb->Resume(), or conditionally call\n> > > resume if adb->IsPaused() returns true.\n> > \n> > Agreed. The RPi AWB implementation differs from the behaviour specified\n> > by Android in that the RPi AWB algorithm isn't disabled when the mode is\n> > set to one of the presets. The presets instead serve to limit the search\n> > range of the AWB algorithm, instead of setting hardcoded manual values\n> > as in Android.\n> > \n> > Naush, what would happen if the tuning file specified a fixed colour\n> > temperature (by setting the min and max to the same value) for AWB\n> > presets ? Would the AWB algorithm then always produce fixed values for\n> > the colour gains ?\n> > \n> > > >                         if (AwbModeTable.count(idx)) {\n> > > >                                 awb->SetMode(AwbModeTable.at(idx));\n> > > >                                 libcameraMetadata_.set(controls::AwbMode, idx);\n> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index 5717bc1f..2e62f61b 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -229,13 +229,6 @@ controls:\n> > > >          Report an estimate of the current illuminance level in lux. The Lux\n> > > >          control can only be returned in metadata.\n> > > >\n> > > > -  - AwbEnable:\n> > > > -      type: bool\n> > > > -      description: |\n> > > > -        Enable or disable the AWB.\n> > > > -\n> > > > -        \\sa ColourGains\n> > > > -\n> > > >    # AwbMode needs further attention:\n> > > >    # - Auto-generate max enum value.\n> > > >    # - Better handling of custom types.\n> > > > @@ -245,29 +238,38 @@ controls:\n> > > >          Specify the range of illuminants to use for the AWB algorithm. The modes\n> > > >          supported are platform specific, and not all modes may be supported.\n> > > >        enum:\n> > > > -        - name: AwbAuto\n> > > > +        - name: AwbOff\n> > > >            value: 0\n> > > > +          description: The AWB routune is disabled.\n> > \n> > s/routine/\n> > \n> > > > +        - name: AwbAuto\n> > > > +          value: 1\n> > > >            description: Search over the whole colour temperature range.\n> > > >          - name: AwbIncandescent\n> > > > -          value: 1\n> > > > -          description: Incandescent AWB lamp mode.\n> > > > -        - name: AwbTungsten\n> > > >            value: 2\n> > > > -          description: Tungsten AWB lamp mode.\n> > > > +          description: Incandescent AWB lamp mode.\n> > > >          - name: AwbFluorescent\n> > > >            value: 3\n> > > >            description: Fluorescent AWB lamp mode.\n> > > > -        - name: AwbIndoor\n> > > > +        - name: AwbWarmFluorescent\n> > > >            value: 4\n> > > > -          description: Indoor AWB lighting mode.\n> > > > +          description: Warm fluorescent AWB lamp mode.\n> > > >          - name: AwbDaylight\n> > > >            value: 5\n> > > >            description: Daylight AWB lighting mode.\n> > > >          - name: AwbCloudy\n> > > >            value: 6\n> > > >            description: Cloudy AWB lighting mode.\n> > > > -        - name: AwbCustom\n> > > > +        - name: AwbTwilight\n> > > >            value: 7\n> > > > +          description: Twilight AWB lamp mode.\n> > > > +        - name: AwbTungsten\n> > > > +          value: 8\n> > > > +          description: Tungsten AWB lamp mode.\n> > > > +        - name: AwbIndoor\n> > > > +          value: 9\n> > > > +          description: Indoor AWB lighting mode.\n> > > > +        - name: AwbCustom\n> > > > +          value: 10\n> > > >            description: Custom AWB mode.\n> > > >\n> > > >    - AwbLocked:\n> > > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > > > index 70cf61b8..ce55d09b 100644\n> > > > --- a/test/controls/control_list.cpp\n> > > > +++ b/test/controls/control_list.cpp\n> > > > @@ -143,10 +143,10 @@ protected:\n> > > >                  * Attempt to set an invalid control and verify that the\n> > > >                  * operation failed.\n> > > >                  */\n> > > > -               list.set(controls::AwbEnable, true);\n> > > > +               list.set(controls::AwbMode, true);\n> > > >\n> > > > -               if (list.contains(controls::AwbEnable)) {\n> > > > -                       cout << \"List shouldn't contain AwbEnable control\" << endl;\n> > > > +               if (list.contains(controls::AwbMode)) {\n> > > > +                       cout << \"List shouldn't contain AwbMode control\" << endl;\n> > > >                         return TestFail;\n> > > >                 }\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EF38CC3220\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  2 Jul 2021 09:52:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 54A92684F5;\n\tFri,  2 Jul 2021 11:52:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC6086050A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  2 Jul 2021 11:52:52 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1CA0B4AB;\n\tFri,  2 Jul 2021 11:52:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dqbvCb3r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625219572;\n\tbh=CkKVgSNdkoGkdV94x/5ENlBH/xp7LUVNlqq6qEyfsyU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dqbvCb3rYZALp0AEb4KqkwC19rYbHYAu8EEr8CAPkq+FDHc0KqtO3w1ySvriArvez\n\tdNH5Rh0dZRfdGJJKsmWfku/BTfVEvp1eJLPM2peux32LVhM7AqIxXlHjziSP56BPad\n\t9E4aUOtHHfXjdZpwt9DT7+E8rVUx7fW8+pZXT4Ew=","Date":"Fri, 2 Jul 2021 18:52:44 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210702095244.GA2510@pyrite.rasen.tech>","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-5-paul.elder@ideasonboard.com>\n\t<CAEmqJPqkGNwmPuzgWBzg36+eb_BUxHP7QcR=nvQyFswkxWVpSw@mail.gmail.com>\n\t<YNkglbY13HNGrLxj@pendragon.ideasonboard.com>\n\t<YNkkZr3Fj20cujFT@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<YNkkZr3Fj20cujFT@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 04/14] controls: Replace AwbEnable\n\twith AwbMode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]