[{"id":25139,"web_url":"https://patchwork.libcamera.org/comment/25139/","msgid":"<20220928054016.46somewh5newfnsg@uno.localdomain>","date":"2022-09-28T05:40:16","subject":"Re: [libcamera-devel] [PATCH v5 21/33] ipa: rkisp1: awb: Store\n\tper-frame information in frame context","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Sep 27, 2022 at 05:36:30AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Rework the algorithm's usage of the active state and frame context to\n> store data in the right place.\n>\n> The active state stores two distinct categories of information:\n>\n> - The consolidated value of all algorithm controls. Requests passed to\n>   the queueRequest() function store values for controls that the\n>   application wants to modify for that particular frame, and the\n>   queueRequest() function updates the active state with those values.\n>   The active state thus contains a consolidated view of the value of all\n>   controls handled by the algorithm.\n>\n> - The value of parameters computed by the algorithm when running in auto\n>   mode. Algorithms running in auto mode compute new parameters every\n>   time statistics buffers are received (either synchronously, or\n>   possibly in a background thread). The latest computed value of those\n>   parameters is stored in the active state in the process() function.\n>\n> The frame context also stores two categories of information:\n>\n> - The value of the controls to be applied to the frame. These values are\n>   typically set in the queueRequest() function, from the consolidated\n>   control values stored in the active state. The frame context thus\n>   stores values for all controls related to the algorithm, not limited\n>   to the controls specified in the corresponding request, but\n>   consolidated from all requests that have been queued so far.\n>\n>   For controls that can be specified manually or computed by the\n>   algorithm depending on the operation mode (such as the colour gains),\n>   the control value will be stored in the frame context in\n>   queueRequest() only when operating in manual mode. When operating in\n>   auto mode, the values are computed by the algorithm and stored in the\n>   frame context in prepare(), just before being stored in the ISP\n>   parameters buffer.\n>\n>   The queueRequest() function can also store ancillary data in the frame\n>   context, such as flags to indicate if (and what) control values have\n>   changed compared to the previous request.\n>\n> - Status information computed by the algorithm for a frame. For\n>   instance, the colour temperature estimated by the algorithm from ISP\n>   statistics calculated on a frame is stored in the frame context for\n>   that frame in the process() function.\n>\n> The active state and frame context thus both contain identical members\n> for most control values, but store values that have a different meaning.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/ipa/rkisp1/algorithms/awb.cpp | 75 +++++++++++++++++++------------\n>  src/ipa/rkisp1/ipa_context.cpp    | 51 +++++++++++++++++----\n>  src/ipa/rkisp1/ipa_context.h      | 25 +++++++++--\n>  3 files changed, 110 insertions(+), 41 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 78398927e462..e491cf7507e0 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -36,9 +36,12 @@ LOG_DEFINE_CATEGORY(RkISP1Awb)\n>  int Awb::configure(IPAContext &context,\n>  \t\t   const IPACameraSensorInfo &configInfo)\n>  {\n> -\tcontext.activeState.awb.gains.red = 1.0;\n> -\tcontext.activeState.awb.gains.blue = 1.0;\n> -\tcontext.activeState.awb.gains.green = 1.0;\n> +\tcontext.activeState.awb.gains.manual.red = 1.0;\n> +\tcontext.activeState.awb.gains.manual.blue = 1.0;\n> +\tcontext.activeState.awb.gains.manual.green = 1.0;\n> +\tcontext.activeState.awb.gains.automatic.red = 1.0;\n> +\tcontext.activeState.awb.gains.automatic.blue = 1.0;\n> +\tcontext.activeState.awb.gains.automatic.green = 1.0;\n>  \tcontext.activeState.awb.autoEnabled = true;\n>\n>  \t/*\n> @@ -75,13 +78,22 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)\n>   * \\copydoc libcamera::ipa::Algorithm::prepare\n>   */\n>  void Awb::prepare(IPAContext &context, const uint32_t frame,\n> -\t\t  [[maybe_unused]] IPAFrameContext &frameContext,\n> -\t\t  rkisp1_params_cfg *params)\n> +\t\t  IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n>  {\n> -\tparams->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green;\n> -\tparams->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue;\n> -\tparams->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red;\n> -\tparams->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green;\n> +\t/*\n> +\t * This is the latest time we can read the active state. This is the\n> +\t * most up-to-date automatic values we can read.\n> +\t */\n> +\tif (frameContext.awb.autoEnabled) {\n> +\t\tframeContext.awb.gains.red = context.activeState.awb.gains.automatic.red;\n> +\t\tframeContext.awb.gains.green = context.activeState.awb.gains.automatic.green;\n> +\t\tframeContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue;\n> +\t}\n> +\n> +\tparams->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green;\n> +\tparams->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue;\n> +\tparams->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red;\n> +\tparams->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green;\n>\n>  \t/* Update the gains. */\n>  \tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN;\n> @@ -127,7 +139,7 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n>   */\n>  void Awb::queueRequest(IPAContext &context,\n>  \t\t       [[maybe_unused]] const uint32_t frame,\n> -\t\t       [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t       IPAFrameContext &frameContext,\n>  \t\t       const ControlList &controls)\n>  {\n>  \tauto &awb = context.activeState.awb;\n> @@ -142,21 +154,29 @@ void Awb::queueRequest(IPAContext &context,\n>\n>  \tconst auto &colourGains = controls.get(controls::ColourGains);\n>  \tif (colourGains && !awb.autoEnabled) {\n> -\t\tawb.gains.red = (*colourGains)[0];\n> -\t\tawb.gains.blue = (*colourGains)[1];\n> +\t\tawb.gains.manual.red = (*colourGains)[0];\n> +\t\tawb.gains.manual.blue = (*colourGains)[1];\n>\n>  \t\tLOG(RkISP1Awb, Debug)\n> -\t\t\t<< \"Set colour gains to red: \" << awb.gains.red\n> -\t\t\t<< \", blue: \" << awb.gains.blue;\n> +\t\t\t<< \"Set colour gains to red: \" << awb.gains.manual.red\n> +\t\t\t<< \", blue: \" << awb.gains.manual.blue;\n> +\t}\n> +\n> +\tframeContext.awb.autoEnabled = awb.autoEnabled;\n> +\n> +\tif (!awb.autoEnabled) {\n> +\t\tframeContext.awb.gains.red = awb.gains.manual.red;\n> +\t\tframeContext.awb.gains.green = 1.0;\n> +\t\tframeContext.awb.gains.blue = awb.gains.manual.blue;\n\nDo I read it wrong or if (!awb.autoEnabled && !colourGains) we here\ninitialize frameContext.awb.gains with an un-initialized\nawb.gains.manual ?\n\n\n>  }\n>\n>  /**\n>   * \\copydoc libcamera::ipa::Algorithm::process\n>   */\n> -void Awb::process([[maybe_unused]] IPAContext &context,\n> +void Awb::process(IPAContext &context,\n>  \t\t  [[maybe_unused]] const uint32_t frame,\n> -\t\t  [[maybe_unused]] IPAFrameContext &frameCtx,\n> +\t\t  IPAFrameContext &frameContext,\n>  \t\t  const rkisp1_stat_buffer *stats)\n>  {\n>  \tconst rkisp1_cif_isp_stat *params = &stats->params;\n> @@ -187,30 +207,27 @@ void Awb::process([[maybe_unused]] IPAContext &context,\n>  \tdouble greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n>  \tdouble blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n>\n> +\tframeContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> +\n>  \t/* Estimate the red and blue gains to apply in a grey world. */\n>  \tdouble redGain = greenMean / (redMean + 1);\n>  \tdouble blueGain = greenMean / (blueMean + 1);\n>\n>  \t/* Filter the values to avoid oscillations. */\n>  \tdouble speed = 0.2;\n> -\tredGain = speed * redGain + (1 - speed) * activeState.awb.gains.red;\n> -\tblueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue;\n> +\tredGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;\n> +\tblueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;\n>\n>  \t/*\n>  \t * Gain values are unsigned integer value, range 0 to 4 with 8 bit\n> -\t * fractional part.\n> +\t * fractional part. Hardcode the green gain to 1.0.\n>  \t */\n> -\tif (activeState.awb.autoEnabled) {\n> -\t\tactiveState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> -\t\tactiveState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> -\t}\n> -\t/* Hardcode the green gain to 1.0. */\n> -\tactiveState.awb.gains.green = 1.0;\n> +\tactiveState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> +\tactiveState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> +\tactiveState.awb.gains.automatic.green = 1.0;\n>\n> -\tactiveState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> -\n> -\tLOG(RkISP1Awb, Debug) << \"Gain found for red: \" << context.activeState.awb.gains.red\n> -\t\t\t      << \" and for blue: \" << context.activeState.awb.gains.blue;\n> +\tLOG(RkISP1Awb, Debug) << \"Gain found for red: \" << activeState.awb.gains.automatic.red\n> +\t\t\t      << \" and for blue: \" << activeState.awb.gains.automatic.blue;\n>  }\n>\n>  REGISTER_IPA_ALGORITHM(Awb, \"Awb\")\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index c7d5b1b6ec5a..ba80a0707ef7 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -120,17 +120,29 @@ namespace libcamera::ipa::rkisp1 {\n>   * \\struct IPAActiveState::awb.gains\n>   * \\brief White balance gains\n>   *\n> - * \\var IPAActiveState::awb.gains.red\n> - * \\brief White balance gain for R channel\n> + * \\struct IPAActiveState::awb.gains.manual\n> + * \\brief Manual white balance gains (set through requests)\n>   *\n> - * \\var IPAActiveState::awb.gains.green\n> - * \\brief White balance gain for G channel\n> + * \\var IPAActiveState::awb.gains.manual.red\n> + * \\brief Manual white balance gain for R channel\n>   *\n> - * \\var IPAActiveState::awb.gains.blue\n> - * \\brief White balance gain for B channel\n> + * \\var IPAActiveState::awb.gains.manual.green\n> + * \\brief Manual white balance gain for G channel\n>   *\n> - * \\var IPAActiveState::awb.temperatureK\n> - * \\brief Estimated color temperature\n> + * \\var IPAActiveState::awb.gains.manual.blue\n> + * \\brief Manual white balance gain for B channel\n> + *\n> + * \\struct IPAActiveState::awb.gains.automatic\n> + * \\brief Automatic white balance gains (computed by the algorithm)\n> + *\n> + * \\var IPAActiveState::awb.gains.automatic.red\n> + * \\brief Automatic white balance gain for R channel\n> + *\n> + * \\var IPAActiveState::awb.gains.automatic.green\n> + * \\brief Automatic white balance gain for G channel\n> + *\n> + * \\var IPAActiveState::awb.gains.automatic.blue\n> + * \\brief Automatic white balance gain for B channel\n>   *\n>   * \\var IPAActiveState::awb.autoEnabled\n>   * \\brief Whether the Auto White Balance algorithm is enabled\n> @@ -201,6 +213,29 @@ namespace libcamera::ipa::rkisp1 {\n>   * The gain should be adapted to the sensor specific gain code before applying.\n>   */\n>\n> +/**\n> + * \\var IPAFrameContext::awb\n> + * \\brief Automatic White Balance parameters for this frame\n> + *\n> + * \\struct IPAFrameContext::awb.gains\n> + * \\brief White balance gains\n> + *\n> + * \\var IPAFrameContext::awb.gains.red\n> + * \\brief White balance gain for R channel\n> + *\n> + * \\var IPAFrameContext::awb.gains.green\n> + * \\brief White balance gain for G channel\n> + *\n> + * \\var IPAFrameContext::awb.gains.blue\n> + * \\brief White balance gain for B channel\n> + *\n> + * \\var IPAFrameContext::awb.temperatureK\n> + * \\brief Estimated color temperature\n> + *\n> + * \\var IPAFrameContext::awb.autoEnabled\n> + * \\brief Whether the Auto White Balance algorithm is enabled\n> + */\n> +\n>  /**\n>   * \\var IPAFrameContext::sensor\n>   * \\brief Sensor configuration that used been used for this frame\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index a4d134e700b5..f23ebb2c9004 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -57,12 +57,18 @@ struct IPAActiveState {\n>\n>  \tstruct {\n>  \t\tstruct {\n> -\t\t\tdouble red;\n> -\t\t\tdouble green;\n> -\t\t\tdouble blue;\n> +\t\t\tstruct {\n> +\t\t\t\tdouble red;\n> +\t\t\t\tdouble green;\n> +\t\t\t\tdouble blue;\n> +\t\t\t} manual;\n> +\t\t\tstruct {\n> +\t\t\t\tdouble red;\n> +\t\t\t\tdouble green;\n> +\t\t\t\tdouble blue;\n> +\t\t\t} automatic;\n>  \t\t} gains;\n>\n> -\t\tdouble temperatureK;\n>  \t\tbool autoEnabled;\n>  \t} awb;\n>\n> @@ -91,6 +97,17 @@ struct IPAFrameContext : public FrameContext {\n>  \t\tdouble gain;\n>  \t} agc;\n>\n> +\tstruct {\n> +\t\tstruct {\n> +\t\t\tdouble red;\n> +\t\t\tdouble green;\n> +\t\t\tdouble blue;\n> +\t\t} gains;\n> +\n> +\t\tdouble temperatureK;\n> +\t\tbool autoEnabled;\n> +\t} awb;\n> +\n>  \tstruct {\n>  \t\tuint32_t exposure;\n>  \t\tdouble gain;\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 35995BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Sep 2022 05:40:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40B886227B;\n\tWed, 28 Sep 2022 07:40:20 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B75A861F78\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Sep 2022 07:40:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2D1C4FF806;\n\tWed, 28 Sep 2022 05:40:17 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664343620;\n\tbh=FqyWSc3wKikJExdKqj00uS5v5j3ZEn5bopwZULoyaCA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YW52tYN/DgANINKQvpFLJqRjRNFzuyH1ECTpaQbmvQkfWP+Nz1vR06u3RmyRVENbO\n\tSfdEi9itZx81yIjy7XRFksTLNtGxP4W2eNSYZYi/i1NeRjGpE0TV4XT2n2y3hN2VYP\n\ttn0EcoEzTdvyzNwSsplO82xg7W7vTLoaRxdO+uu4fJ5KmR+S13JtZ+2KiNFILsFqK3\n\tLHEeD9LYR9YQjdO2B+2YKDaYvZxU3LCc2t3ya6nSK+AiXBOmSBhPnKEd8dj7S/SUzJ\n\th+ESfakV3wPM+IAvSYKlvnSugqNlUBC6E+HkjEZXoBX0NsUhSKjJwtEoA0kolGILVi\n\tAWIy0wD2xlk1g==","Date":"Wed, 28 Sep 2022 07:40:16 +0200","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20220928054016.46somewh5newfnsg@uno.localdomain>","References":"<20220927023642.12341-1-laurent.pinchart@ideasonboard.com>\n\t<20220927023642.12341-22-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220927023642.12341-22-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 21/33] ipa: rkisp1: awb: Store\n\tper-frame information in frame context","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>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]