[{"id":25033,"web_url":"https://patchwork.libcamera.org/comment/25033/","msgid":"<166371455535.18961.15370617823339965490@Monstersaurus>","date":"2022-09-20T22:55:55","subject":"Re: [libcamera-devel] [PATCH v4 20/32] ipa: rkisp1: awb: Store\n\tper-frame information in frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:48)\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 \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 a0bbe5cb8afa..bb0f6c27fc7d 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> const IPACameraSensorInfo &configInfo)\n> {\n> - context.activeState.awb.gains.red = 1.0;\n> - context.activeState.awb.gains.blue = 1.0;\n> - context.activeState.awb.gains.green = 1.0;\n> + context.activeState.awb.gains.manual.red = 1.0;\n> + context.activeState.awb.gains.manual.blue = 1.0;\n> + context.activeState.awb.gains.manual.green = 1.0;\n> + context.activeState.awb.gains.automatic.red = 1.0;\n> + context.activeState.awb.gains.automatic.blue = 1.0;\n> + context.activeState.awb.gains.automatic.green = 1.0;\n> context.activeState.awb.autoEnabled = true;\n> \n> /*\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> - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> - rkisp1_params_cfg *params)\n> + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> - params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green;\n> - params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue;\n> - params->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red;\n> - params->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green;\n> + /*\n> + * This is the latest time we can read the active state. This is the\n> + * most up-to-date automatic values we can read.\n> + */\n> + if (frameContext.awb.autoEnabled) {\n> + frameContext.awb.gains.red = context.activeState.awb.gains.automatic.red;\n> + frameContext.awb.gains.green = context.activeState.awb.gains.automatic.green;\n> + frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue;\n> + }\n> +\n> + params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green;\n> + params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue;\n> + params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red;\n> + params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green;\n> \n> /* Update the gains. */\n> params->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> [[maybe_unused]] const uint32_t frame,\n> - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> + RkISP1FrameContext &frameContext,\n> const ControlList &controls)\n> {\n> auto &awb = context.activeState.awb;\n> @@ -142,21 +154,29 @@ void Awb::queueRequest(IPAContext &context,\n> \n> const auto &colourGains = controls.get(controls::ColourGains);\n> if (colourGains && !awb.autoEnabled) {\n> - awb.gains.red = (*colourGains)[0];\n> - awb.gains.blue = (*colourGains)[1];\n> + awb.gains.manual.red = (*colourGains)[0];\n> + awb.gains.manual.blue = (*colourGains)[1];\n\nI really do feel like if the application provides a set of manual gains,\nwe should store them in the internal manual gains.\n\nUnless perhaps if we say upon transitioning from automatic, to manual\n(Auto-Disabled)if we do not have a manual value in the same request,\nwe'll use the most recently computed auto values and hold them until\ntold otherwise.\n\nIf we don't - we're going to suddenly jump to 'manually' set values that\nare some historic values... It just feels quite wrong to me.\n\n\n> \n> LOG(RkISP1Awb, Debug)\n> - << \"Set colour gains to red: \" << awb.gains.red\n> - << \", blue: \" << awb.gains.blue;\n> + << \"Set colour gains to red: \" << awb.gains.manual.red\n> + << \", blue: \" << awb.gains.manual.blue;\n> + }\n> +\n> + frameContext.awb.autoEnabled = awb.autoEnabled;\n> +\n> + if (!awb.autoEnabled) {\n> + frameContext.awb.gains.red = awb.gains.manual.red;\n> + frameContext.awb.gains.green = 1.0;\n> + frameContext.awb.gains.blue = awb.gains.manual.blue;\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> [[maybe_unused]] const uint32_t frame,\n> - [[maybe_unused]] RkISP1FrameContext &frameCtx,\n> + RkISP1FrameContext &frameContext,\n> const rkisp1_stat_buffer *stats)\n> {\n> const rkisp1_cif_isp_stat *params = &stats->params;\n> @@ -187,30 +207,27 @@ void Awb::process([[maybe_unused]] IPAContext &context,\n> double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n> double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n> \n> + frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> +\n\nWe likely want to store the temperature of each frame to make sure we\nupdate the metadata, but I wonder if we might also want to track the\ncolour temperature in the Active state so that for instance the LSC can\ncheck what the current temperature is?\n\nIf we are able to get the algorithms to update the metadata structure\ndirectly, then a value like the colour temperature here probably doesn't\nneed to go into the frame context, just to be read later.\n\nBut for now - I think this is accurate.\n\nI suspect perhaps a \"ControlList metadata\" could/should go into the base\nFrameContext, so that all calls given a FrameContext can directly update\nthe metadata which will then be passed to the pipeline handler to put\ninto the request when it completes.\n\n(not this patch, later)\n\n> /* Estimate the red and blue gains to apply in a grey world. */\n> double redGain = greenMean / (redMean + 1);\n> double blueGain = greenMean / (blueMean + 1);\n> \n> /* Filter the values to avoid oscillations. */\n> double speed = 0.2;\n> - redGain = speed * redGain + (1 - speed) * activeState.awb.gains.red;\n> - blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue;\n> + redGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;\n> + blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;\n> \n> /*\n> * Gain values are unsigned integer value, range 0 to 4 with 8 bit\n> - * fractional part.\n> + * fractional part. Hardcode the green gain to 1.0.\n> */\n> - if (activeState.awb.autoEnabled) {\n> - activeState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> - activeState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> - }\n> - /* Hardcode the green gain to 1.0. */\n> - activeState.awb.gains.green = 1.0;\n> + activeState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> + activeState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> + activeState.awb.gains.automatic.green = 1.0;\n> \n> - activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> -\n> - LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << context.activeState.awb.gains.red\n> - << \" and for blue: \" << context.activeState.awb.gains.blue;\n> + LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << activeState.awb.gains.automatic.red\n> + << \" 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 88e0631c8d39..cd1443e1ed46 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 RkISP1FrameContext::awb\n> + * \\brief Automatic White Balance parameters for this frame\n> + *\n> + * \\struct RkISP1FrameContext::awb.gains\n> + * \\brief White balance gains\n> + *\n> + * \\var RkISP1FrameContext::awb.gains.red\n> + * \\brief White balance gain for R channel\n> + *\n> + * \\var RkISP1FrameContext::awb.gains.green\n> + * \\brief White balance gain for G channel\n> + *\n> + * \\var RkISP1FrameContext::awb.gains.blue\n> + * \\brief White balance gain for B channel\n> + *\n> + * \\var RkISP1FrameContext::awb.temperatureK\n> + * \\brief Estimated color temperature\n> + *\n> + * \\var RkISP1FrameContext::awb.autoEnabled\n> + * \\brief Whether the Auto White Balance algorithm is enabled\n> + */\n\nI believe these are all going to get simplified later ... I like that\n... but if the support class came earlier, it could move to that\ndirectly in here.\n\nOh - in fact, it looks like the patch I'm thinking of isn't in this\nseries yet :(\n\n\nOk - so I'm happy enough with this. I look forward to the RGB / Pixel class\nwhich I think helps simplify this code a fair bit, but it's not required\nfor this conversion.\n\n\nI think this patch tells me:\n\n - I need to understand how we globally expect to handle setting manual\n values when an algorithm is configured to use automatic ones.\n\n - I look forward to your class that groups R/G/B values\n\n - We should already add a common metadata object to populate, so we can\n report metadata back in completed requests.\n\n - ColourTemperature should go straight to the metadata\n\n - ActiveState should store the ColourTemperature so that LSC can refer\n to it.\n\n\nBut all of that can be done on top...\n\nso\n\nReviewed-by: Kieran Bingham \n\n> +\n> /**\n> * \\var RkISP1FrameContext::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 ecf993cd22d7..d97aae9a97b4 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -56,12 +56,18 @@ struct IPAActiveState {\n> \n> struct {\n> struct {\n> - double red;\n> - double green;\n> - double blue;\n> + struct {\n> + double red;\n> + double green;\n> + double blue;\n> + } manual;\n> + struct {\n> + double red;\n> + double green;\n> + double blue;\n> + } automatic;\n> } gains;\n> \n> - double temperatureK;\n> bool autoEnabled;\n> } awb;\n> \n> @@ -90,6 +96,17 @@ struct RkISP1FrameContext : public FrameContext {\n> double gain;\n> } agc;\n> \n> + struct {\n> + struct {\n> + double red;\n> + double green;\n> + double blue;\n> + } gains;\n> +\n> + double temperatureK;\n> + bool autoEnabled;\n> + } awb;\n> +\n> struct {\n> uint32_t exposure;\n> double gain;\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","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 9E42EC0DA4\n\tfor ;\n\tTue, 20 Sep 2022 22:56:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9241621CD;\n\tWed, 21 Sep 2022 00:55:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFFAB61F7D\n\tfor ;\n\tWed, 21 Sep 2022 00:55:57 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C9C46BE;\n\tWed, 21 Sep 2022 00:55:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663714559;\n\tbh=yGO7guTk7dr8LmSLlaj5GVlXd+F1Q2SvfRasUw8opSg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=366lJdjgGWwSf/Vto9B+ZuwaVGzr+UQQTPfBcmKLOc1qhRxUMIDsu6H60N9y7NDdK\n\tt7wJFcDdPU/eidcwz2M/TFw5da0w2YtO/PAmf/C3zYwyAkLVHPq+StMVpy1MV4YY9m\n\tFDxhkcZFnTRFfPu5oZUllMI0R35UenJujhnYmpE0scoe+vfDvVgAbPOOj4I/FThuRV\n\tAbPKS8eLq4Nx3803XeRqjlToTNTUjrt4+0zi3nn7nVwdt0W1C2zxTE3anr6Efoi+bP\n\tA+AWVvl6a+RDdyR0h32Bvb1hbZ18/PEYQDooFk9G4BAvlDWRAOXzmh0sbygaBJFcg2\n\tF8NLwTxc49tvg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663714557;\n\tbh=yGO7guTk7dr8LmSLlaj5GVlXd+F1Q2SvfRasUw8opSg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=INhNqz0zrlR9pxEeHBF6L5iWSEc9M5IjLfNO8pckeBfZtldXyOaVK59Cy0G21id60\n\tLtlpGZrWI0TR8OWqsAS2GYTDiEXMZfNl9EUrSp0x/LH4DYB/x47kX6jHw1pHhCgrIV\n\t7/bHXcdBh811ON1Tpp96Ra6rZrcaveUWtM5AmGyM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"INhNqz0z\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-21-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-21-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 20 Sep 2022 23:55:55 +0100","Message-ID":"<166371455535.18961.15370617823339965490@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 20/32] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25066,"web_url":"https://patchwork.libcamera.org/comment/25066/","msgid":"<20220921203404.si375nd5lehrnt2u@uno.localdomain>","date":"2022-09-21T20:34:04","subject":"Re: [libcamera-devel] [PATCH v4 20/32] 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, Kieran\n\nOn Tue, Sep 20, 2022 at 11:55:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:48)\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 \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 a0bbe5cb8afa..bb0f6c27fc7d 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> > const IPACameraSensorInfo &configInfo)\n> > {\n> > - context.activeState.awb.gains.red = 1.0;\n> > - context.activeState.awb.gains.blue = 1.0;\n> > - context.activeState.awb.gains.green = 1.0;\n> > + context.activeState.awb.gains.manual.red = 1.0;\n> > + context.activeState.awb.gains.manual.blue = 1.0;\n> > + context.activeState.awb.gains.manual.green = 1.0;\n> > + context.activeState.awb.gains.automatic.red = 1.0;\n> > + context.activeState.awb.gains.automatic.blue = 1.0;\n> > + context.activeState.awb.gains.automatic.green = 1.0;\n> > context.activeState.awb.autoEnabled = true;\n> >\n> > /*\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> > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > - rkisp1_params_cfg *params)\n> > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > {\n> > - params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green;\n> > - params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue;\n> > - params->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red;\n> > - params->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green;\n> > + /*\n> > + * This is the latest time we can read the active state. This is the\n> > + * most up-to-date automatic values we can read.\n> > + */\n> > + if (frameContext.awb.autoEnabled) {\n> > + frameContext.awb.gains.red = context.activeState.awb.gains.automatic.red;\n> > + frameContext.awb.gains.green = context.activeState.awb.gains.automatic.green;\n> > + frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue;\n> > + }\n> > +\n> > + params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green;\n> > + params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue;\n> > + params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red;\n> > + params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green;\n> >\n> > /* Update the gains. */\n> > params->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> > [[maybe_unused]] const uint32_t frame,\n> > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > + RkISP1FrameContext &frameContext,\n> > const ControlList &controls)\n> > {\n> > auto &awb = context.activeState.awb;\n> > @@ -142,21 +154,29 @@ void Awb::queueRequest(IPAContext &context,\n> >\n> > const auto &colourGains = controls.get(controls::ColourGains);\n> > if (colourGains && !awb.autoEnabled) {\n> > - awb.gains.red = (*colourGains)[0];\n> > - awb.gains.blue = (*colourGains)[1];\n> > + awb.gains.manual.red = (*colourGains)[0];\n> > + awb.gains.manual.blue = (*colourGains)[1];\n>\n> I really do feel like if the application provides a set of manual gains,\n> we should store them in the internal manual gains.\n>\n> Unless perhaps if we say upon transitioning from automatic, to manual\n> (Auto-Disabled)if we do not have a manual value in the same request,\n> we'll use the most recently computed auto values and hold them until\n> told otherwise.\n>\n> If we don't - we're going to suddenly jump to 'manually' set values that\n> are some historic values... It just feels quite wrong to me.\n>\n\nI side with Kieran, even if libcamera::AwbEnable, probably because\nit's old, doesn't tell much about the expected behaviour. After many\ndiscussions on AEGC, the decision was to re-start from the last\ncomputed vaues in the previous mode, when transitioning from Auto to\nManual or viceversa.\n\nWe can easily detect if a transition is happening here and handle it\nhere maybe ?\n>\n> >\n> > LOG(RkISP1Awb, Debug)\n> > - << \"Set colour gains to red: \" << awb.gains.red\n> > - << \", blue: \" << awb.gains.blue;\n> > + << \"Set colour gains to red: \" << awb.gains.manual.red\n> > + << \", blue: \" << awb.gains.manual.blue;\n> > + }\n> > +\n> > + frameContext.awb.autoEnabled = awb.autoEnabled;\n> > +\n> > + if (!awb.autoEnabled) {\n> > + frameContext.awb.gains.red = awb.gains.manual.red;\n> > + frameContext.awb.gains.green = 1.0;\n> > + frameContext.awb.gains.blue = awb.gains.manual.blue;\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> > [[maybe_unused]] const uint32_t frame,\n> > - [[maybe_unused]] RkISP1FrameContext &frameCtx,\n> > + RkISP1FrameContext &frameContext,\n> > const rkisp1_stat_buffer *stats)\n> > {\n> > const rkisp1_cif_isp_stat *params = &stats->params;\n> > @@ -187,30 +207,27 @@ void Awb::process([[maybe_unused]] IPAContext &context,\n> > double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n> > double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n> >\n> > + frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> > +\n>\n> We likely want to store the temperature of each frame to make sure we\n> update the metadata, but I wonder if we might also want to track the\n> colour temperature in the Active state so that for instance the LSC can\n> check what the current temperature is?\n>\n> If we are able to get the algorithms to update the metadata structure\n> directly, then a value like the colour temperature here probably doesn't\n> need to go into the frame context, just to be read later.\n>\n> But for now - I think this is accurate.\n>\n> I suspect perhaps a \"ControlList metadata\" could/should go into the base\n> FrameContext, so that all calls given a FrameContext can directly update\n> the metadata which will then be passed to the pipeline handler to put\n> into the request when it completes.\n>\n> (not this patch, later)\n>\n> > /* Estimate the red and blue gains to apply in a grey world. */\n> > double redGain = greenMean / (redMean + 1);\n> > double blueGain = greenMean / (blueMean + 1);\n> >\n> > /* Filter the values to avoid oscillations. */\n> > double speed = 0.2;\n> > - redGain = speed * redGain + (1 - speed) * activeState.awb.gains.red;\n> > - blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue;\n> > + redGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;\n> > + blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;\n> >\n> > /*\n> > * Gain values are unsigned integer value, range 0 to 4 with 8 bit\n> > - * fractional part.\n> > + * fractional part. Hardcode the green gain to 1.0.\n> > */\n> > - if (activeState.awb.autoEnabled) {\n> > - activeState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> > - activeState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> > - }\n> > - /* Hardcode the green gain to 1.0. */\n> > - activeState.awb.gains.green = 1.0;\n> > + activeState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> > + activeState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> > + activeState.awb.gains.automatic.green = 1.0;\n> >\n> > - activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> > -\n> > - LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << context.activeState.awb.gains.red\n> > - << \" and for blue: \" << context.activeState.awb.gains.blue;\n> > + LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << activeState.awb.gains.automatic.red\n> > + << \" 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 88e0631c8d39..cd1443e1ed46 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 RkISP1FrameContext::awb\n> > + * \\brief Automatic White Balance parameters for this frame\n> > + *\n> > + * \\struct RkISP1FrameContext::awb.gains\n> > + * \\brief White balance gains\n> > + *\n> > + * \\var RkISP1FrameContext::awb.gains.red\n> > + * \\brief White balance gain for R channel\n> > + *\n> > + * \\var RkISP1FrameContext::awb.gains.green\n> > + * \\brief White balance gain for G channel\n> > + *\n> > + * \\var RkISP1FrameContext::awb.gains.blue\n> > + * \\brief White balance gain for B channel\n> > + *\n> > + * \\var RkISP1FrameContext::awb.temperatureK\n> > + * \\brief Estimated color temperature\n> > + *\n> > + * \\var RkISP1FrameContext::awb.autoEnabled\n> > + * \\brief Whether the Auto White Balance algorithm is enabled\n> > + */\n>\n> I believe these are all going to get simplified later ... I like that\n> ... but if the support class came earlier, it could move to that\n> directly in here.\n>\n> Oh - in fact, it looks like the patch I'm thinking of isn't in this\n> series yet :(\n>\n>\n> Ok - so I'm happy enough with this. I look forward to the RGB / Pixel class\n> which I think helps simplify this code a fair bit, but it's not required\n> for this conversion.\n>\n>\n> I think this patch tells me:\n>\n> - I need to understand how we globally expect to handle setting manual\n> values when an algorithm is configured to use automatic ones.\n\nWe surely need to push for a single model for all controls. I haven't\nthought too much about which algorithms would not be able to comply and\nwhy, but if no obvious reasons, I think we should aim to be\nconsistent.\n\n>\n> - I look forward to your class that groups R/G/B values\n>\n> - We should already add a common metadata object to populate, so we can\n> report metadata back in completed requests.\n>\n> - ColourTemperature should go straight to the metadata\n>\n> - ActiveState should store the ColourTemperature so that LSC can refer\n> to it.\n>\n>\n> But all of that can be done on top...\n>\n> so\n>\n> Reviewed-by: Kieran Bingham \n>\n> > +\n> > /**\n> > * \\var RkISP1FrameContext::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 ecf993cd22d7..d97aae9a97b4 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -56,12 +56,18 @@ struct IPAActiveState {\n> >\n> > struct {\n> > struct {\n> > - double red;\n> > - double green;\n> > - double blue;\n> > + struct {\n> > + double red;\n> > + double green;\n> > + double blue;\n> > + } manual;\n> > + struct {\n> > + double red;\n> > + double green;\n> > + double blue;\n> > + } automatic;\n> > } gains;\n> >\n> > - double temperatureK;\n> > bool autoEnabled;\n> > } awb;\n> >\n> > @@ -90,6 +96,17 @@ struct RkISP1FrameContext : public FrameContext {\n> > double gain;\n> > } agc;\n> >\n> > + struct {\n> > + struct {\n> > + double red;\n> > + double green;\n> > + double blue;\n> > + } gains;\n> > +\n> > + double temperatureK;\n> > + bool autoEnabled;\n> > + } awb;\n> > +\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >","headers":{"Return-Path":"","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 B30B3C0DA4\n\tfor ;\n\tWed, 21 Sep 2022 20:34:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C94C621F7;\n\tWed, 21 Sep 2022 22:34:09 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 120C9600AA\n\tfor ;\n\tWed, 21 Sep 2022 22:34:07 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 097991C0003;\n\tWed, 21 Sep 2022 20:34:05 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663792449;\n\tbh=VPGgziSB0deqnLsE3TI8WbRpvwc2sjN70s+FvoKp3l4=;\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=ACWLViJ0gP5YpUE+OhoUsKlGLuISEEoywaUQvyxVKmJ94bPO8G9/8OGUe5ZPS+zjk\n\tgheCmBNICRVIOSpCTV+Y3ZBU8se6s4I78GvHd8GBYV5DQSj/gYvo9UyPdiKHzgHYFw\n\tBP9fb6rwjdd0Om3CNVewMmFylbL8nu2rsdE19Cv+Om8EkhzsrMHo3yvY9Itm/NLCSr\n\tmU2z1M28Lqst3VUO/Wgb0z2z6WaxeQmKp4ekYZIyplwF1Zkir1HKb3om+VLzuAvKBc\n\tF0sbNTlh161dZP2ikroXQA1N6J0WrbM2mTyVpyvbcMAJS6AJB8KsMvvMXzYpclGlzW\n\tEx/2rt9OSvx6A==","Date":"Wed, 21 Sep 2022 22:34:04 +0200","To":"Kieran Bingham ","Message-ID":"<20220921203404.si375nd5lehrnt2u@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-21-laurent.pinchart@ideasonboard.com>\n\t<166371455535.18961.15370617823339965490@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166371455535.18961.15370617823339965490@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 20/32] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25072,"web_url":"https://patchwork.libcamera.org/comment/25072/","msgid":"<166383797151.56880.9141645036894131390@Monstersaurus>","date":"2022-09-22T09:12:51","subject":"Re: [libcamera-devel] [PATCH v4 20/32] ipa: rkisp1: awb: Store\n\tper-frame information in frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-09-21 21:34:04)\n> Hi Laurent, Kieran\n> \n> On Tue, Sep 20, 2022 at 11:55:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:48)\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 \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 a0bbe5cb8afa..bb0f6c27fc7d 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> > > const IPACameraSensorInfo &configInfo)\n> > > {\n> > > - context.activeState.awb.gains.red = 1.0;\n> > > - context.activeState.awb.gains.blue = 1.0;\n> > > - context.activeState.awb.gains.green = 1.0;\n> > > + context.activeState.awb.gains.manual.red = 1.0;\n> > > + context.activeState.awb.gains.manual.blue = 1.0;\n> > > + context.activeState.awb.gains.manual.green = 1.0;\n> > > + context.activeState.awb.gains.automatic.red = 1.0;\n> > > + context.activeState.awb.gains.automatic.blue = 1.0;\n> > > + context.activeState.awb.gains.automatic.green = 1.0;\n> > > context.activeState.awb.autoEnabled = true;\n> > >\n> > > /*\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> > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > - rkisp1_params_cfg *params)\n> > > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > > {\n> > > - params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green;\n> > > - params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue;\n> > > - params->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red;\n> > > - params->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green;\n> > > + /*\n> > > + * This is the latest time we can read the active state. This is the\n> > > + * most up-to-date automatic values we can read.\n> > > + */\n> > > + if (frameContext.awb.autoEnabled) {\n> > > + frameContext.awb.gains.red = context.activeState.awb.gains.automatic.red;\n> > > + frameContext.awb.gains.green = context.activeState.awb.gains.automatic.green;\n> > > + frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue;\n> > > + }\n> > > +\n> > > + params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green;\n> > > + params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue;\n> > > + params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red;\n> > > + params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green;\n> > >\n> > > /* Update the gains. */\n> > > params->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> > > [[maybe_unused]] const uint32_t frame,\n> > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > + RkISP1FrameContext &frameContext,\n> > > const ControlList &controls)\n> > > {\n> > > auto &awb = context.activeState.awb;\n> > > @@ -142,21 +154,29 @@ void Awb::queueRequest(IPAContext &context,\n> > >\n> > > const auto &colourGains = controls.get(controls::ColourGains);\n> > > if (colourGains && !awb.autoEnabled) {\n> > > - awb.gains.red = (*colourGains)[0];\n> > > - awb.gains.blue = (*colourGains)[1];\n> > > + awb.gains.manual.red = (*colourGains)[0];\n> > > + awb.gains.manual.blue = (*colourGains)[1];\n> >\n> > I really do feel like if the application provides a set of manual gains,\n> > we should store them in the internal manual gains.\n> >\n> > Unless perhaps if we say upon transitioning from automatic, to manual\n> > (Auto-Disabled)if we do not have a manual value in the same request,\n> > we'll use the most recently computed auto values and hold them until\n> > told otherwise.\n> >\n> > If we don't - we're going to suddenly jump to 'manually' set values that\n> > are some historic values... It just feels quite wrong to me.\n> >\n> \n> I side with Kieran, even if libcamera::AwbEnable, probably because\n> it's old, doesn't tell much about the expected behaviour. After many\n> discussions on AEGC, the decision was to re-start from the last\n> computed vaues in the previous mode, when transitioning from Auto to\n> Manual or viceversa.\n> \n> We can easily detect if a transition is happening here and handle it\n> here maybe ?\n\nYes, I think it can be detected here. I would have:\n\n\n const auto &awbEnable = controls.get(controls::AwbEnable);\n if (awbEnable && *awbEnable != awb.autoEnabled) {\n awb.autoEnabled = *awbEnable;\n\n LOG(RkISP1Awb, Debug)\n << (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n\n+\t\t/*\n+\t\t * Pre-set manual controls with the current state to\n+\t\t * support switching to auto disabled mode when the user\n+\t\t * does not provide manual values in the same request.\n+\t\t */\n+\t\tif (!awb.autoEnabled) {\n+\t\t\tawb.gains.manual.red = awb.gains.automatic.red;\n+\t\t\tawb.gains.manual.blue = awb.gains.automatic.blue;\n+\t\t}\n }\n\n\n\n\n> >\n> > >\n> > > LOG(RkISP1Awb, Debug)\n> > > - << \"Set colour gains to red: \" << awb.gains.red\n> > > - << \", blue: \" << awb.gains.blue;\n> > > + << \"Set colour gains to red: \" << awb.gains.manual.red\n> > > + << \", blue: \" << awb.gains.manual.blue;\n> > > + }\n> > > +\n> > > + frameContext.awb.autoEnabled = awb.autoEnabled;\n> > > +\n> > > + if (!awb.autoEnabled) {\n> > > + frameContext.awb.gains.red = awb.gains.manual.red;\n> > > + frameContext.awb.gains.green = 1.0;\n> > > + frameContext.awb.gains.blue = awb.gains.manual.blue;\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> > > [[maybe_unused]] const uint32_t frame,\n> > > - [[maybe_unused]] RkISP1FrameContext &frameCtx,\n> > > + RkISP1FrameContext &frameContext,\n> > > const rkisp1_stat_buffer *stats)\n> > > {\n> > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > @@ -187,30 +207,27 @@ void Awb::process([[maybe_unused]] IPAContext &context,\n> > > double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n> > > double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n> > >\n> > > + frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> > > +\n> >\n> > We likely want to store the temperature of each frame to make sure we\n> > update the metadata, but I wonder if we might also want to track the\n> > colour temperature in the Active state so that for instance the LSC can\n> > check what the current temperature is?\n\nI see this gets introduced later anyway - so it's fine.\n\n> > If we are able to get the algorithms to update the metadata structure\n> > directly, then a value like the colour temperature here probably doesn't\n> > need to go into the frame context, just to be read later.\n> >\n> > But for now - I think this is accurate.\n> >\n> > I suspect perhaps a \"ControlList metadata\" could/should go into the base\n> > FrameContext, so that all calls given a FrameContext can directly update\n> > the metadata which will then be passed to the pipeline handler to put\n> > into the request when it completes.\n> >\n> > (not this patch, later)\n> >\n> > > /* Estimate the red and blue gains to apply in a grey world. */\n> > > double redGain = greenMean / (redMean + 1);\n> > > double blueGain = greenMean / (blueMean + 1);\n> > >\n> > > /* Filter the values to avoid oscillations. */\n> > > double speed = 0.2;\n> > > - redGain = speed * redGain + (1 - speed) * activeState.awb.gains.red;\n> > > - blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue;\n> > > + redGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;\n> > > + blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;\n> > >\n> > > /*\n> > > * Gain values are unsigned integer value, range 0 to 4 with 8 bit\n> > > - * fractional part.\n> > > + * fractional part. Hardcode the green gain to 1.0.\n> > > */\n> > > - if (activeState.awb.autoEnabled) {\n> > > - activeState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> > > - activeState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> > > - }\n> > > - /* Hardcode the green gain to 1.0. */\n> > > - activeState.awb.gains.green = 1.0;\n> > > + activeState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> > > + activeState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> > > + activeState.awb.gains.automatic.green = 1.0;\n> > >\n> > > - activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> > > -\n> > > - LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << context.activeState.awb.gains.red\n> > > - << \" and for blue: \" << context.activeState.awb.gains.blue;\n> > > + LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << activeState.awb.gains.automatic.red\n> > > + << \" 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 88e0631c8d39..cd1443e1ed46 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 RkISP1FrameContext::awb\n> > > + * \\brief Automatic White Balance parameters for this frame\n> > > + *\n> > > + * \\struct RkISP1FrameContext::awb.gains\n> > > + * \\brief White balance gains\n> > > + *\n> > > + * \\var RkISP1FrameContext::awb.gains.red\n> > > + * \\brief White balance gain for R channel\n> > > + *\n> > > + * \\var RkISP1FrameContext::awb.gains.green\n> > > + * \\brief White balance gain for G channel\n> > > + *\n> > > + * \\var RkISP1FrameContext::awb.gains.blue\n> > > + * \\brief White balance gain for B channel\n> > > + *\n> > > + * \\var RkISP1FrameContext::awb.temperatureK\n> > > + * \\brief Estimated color temperature\n> > > + *\n> > > + * \\var RkISP1FrameContext::awb.autoEnabled\n> > > + * \\brief Whether the Auto White Balance algorithm is enabled\n> > > + */\n> >\n> > I believe these are all going to get simplified later ... I like that\n> > ... but if the support class came earlier, it could move to that\n> > directly in here.\n> >\n> > Oh - in fact, it looks like the patch I'm thinking of isn't in this\n> > series yet :(\n> >\n> >\n> > Ok - so I'm happy enough with this. I look forward to the RGB / Pixel class\n> > which I think helps simplify this code a fair bit, but it's not required\n> > for this conversion.\n> >\n> >\n> > I think this patch tells me:\n> >\n> > - I need to understand how we globally expect to handle setting manual\n> > values when an algorithm is configured to use automatic ones.\n> \n> We surely need to push for a single model for all controls. I haven't\n> thought too much about which algorithms would not be able to comply and\n> why, but if no obvious reasons, I think we should aim to be\n> consistent.\n\nI like the idea that switching from automatic to manual without\nspecifying a manual value will take the most up to date automatic values\nand lock them in as the manual values at that point. Then applications\ncan of course update them to new manual values (even in the request that\ncauses the transition) - or ... it simply disables the automatic\noperation from that point, and doesn't cause any visual switch to some\nhistoric manual value.\n\n\n> > - I look forward to your class that groups R/G/B values\n> >\n> > - We should already add a common metadata object to populate, so we can\n> > report metadata back in completed requests.\n> >\n> > - ColourTemperature should go straight to the metadata\n> >\n> > - ActiveState should store the ColourTemperature so that LSC can refer\n> > to it.\n> >\n> >\n> > But all of that can be done on top...\n> >\n> > so\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > +\n> > > /**\n> > > * \\var RkISP1FrameContext::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 ecf993cd22d7..d97aae9a97b4 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -56,12 +56,18 @@ struct IPAActiveState {\n> > >\n> > > struct {\n> > > struct {\n> > > - double red;\n> > > - double green;\n> > > - double blue;\n> > > + struct {\n> > > + double red;\n> > > + double green;\n> > > + double blue;\n> > > + } manual;\n> > > + struct {\n> > > + double red;\n> > > + double green;\n> > > + double blue;\n> > > + } automatic;\n> > > } gains;\n> > >\n> > > - double temperatureK;\n> > > bool autoEnabled;\n> > > } awb;\n> > >\n> > > @@ -90,6 +96,17 @@ struct RkISP1FrameContext : public FrameContext {\n> > > double gain;\n> > > } agc;\n> > >\n> > > + struct {\n> > > + struct {\n> > > + double red;\n> > > + double green;\n> > > + double blue;\n> > > + } gains;\n> > > +\n> > > + double temperatureK;\n> > > + bool autoEnabled;\n> > > + } awb;\n> > > +\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > >","headers":{"Return-Path":"","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 F3CB0C3272\n\tfor ;\n\tThu, 22 Sep 2022 09:12:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40CCC62201;\n\tThu, 22 Sep 2022 11:12:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 322646219B\n\tfor ;\n\tThu, 22 Sep 2022 11:12:54 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 947006BB;\n\tThu, 22 Sep 2022 11:12:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663837975;\n\tbh=R29dy7eljHmXFQlUOg/ZU4yfx7wHi91XHy3gcVB3gPY=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=N/mFFXZL63+POr0Vmc1IJ/ZLB0jhkpUAq+Yxnv5YLJYJKR1EKm6RzyI/yPohnNGT7\n\t7/yRFqTETTA96smiGyKC6FGrUHIdE1PtU3d8y7BddpY2GnS0NoZBKl/Ep6Ys5SGZGz\n\tsJNO7Gk0MaQrO2iTcNqEJWcLqk0bi4aldKPRtyaBDjc52KRr2aXufmca6CWdj+K7gS\n\tpiTfAHqjxw2CNO8SoAkRdTyJ3/++TPmn3NuNFrXPQiZXlvik0HGqvoEw/lLRuFh2V6\n\tfrnDvWc+5rWWS4Qqd9DR8yKiyhE8OMKD/JvQNJlu5XT03qEYBVg32bSQgBvQoM54dA\n\tnJsF/iG4sEWLA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663837973;\n\tbh=R29dy7eljHmXFQlUOg/ZU4yfx7wHi91XHy3gcVB3gPY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=a2TEUE8Y3IAfFIBhPJK00UxqzKRf2iMIJwAPJ6gTxoYeJZIsFPNACUShZtWXoFRXu\n\trPGpmq56mOZEdjSRNJyEwfZcZOfcIdUczzzA5L7C9fr3bFBKUFedyhI3zQ8KUiyWwT\n\t3T6AHdWXaQPKN+FjWjNYd3daNKJOkzKPVNYUcRuI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"a2TEUE8Y\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220921203404.si375nd5lehrnt2u@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-21-laurent.pinchart@ideasonboard.com>\n\t<166371455535.18961.15370617823339965490@Monstersaurus>\n\t<20220921203404.si375nd5lehrnt2u@uno.localdomain>","To":"Jacopo Mondi ","Date":"Thu, 22 Sep 2022 10:12:51 +0100","Message-ID":"<166383797151.56880.9141645036894131390@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 20/32] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25097,"web_url":"https://patchwork.libcamera.org/comment/25097/","msgid":"","date":"2022-09-22T20:52:28","subject":"Re: [libcamera-devel] [PATCH v4 20/32] ipa: rkisp1: awb: Store\n\tper-frame information in frame context","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Sep 22, 2022 at 10:12:51AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2022-09-21 21:34:04)\n> > On Tue, Sep 20, 2022 at 11:55:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:48)\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 \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 a0bbe5cb8afa..bb0f6c27fc7d 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> > > > const IPACameraSensorInfo &configInfo)\n> > > > {\n> > > > - context.activeState.awb.gains.red = 1.0;\n> > > > - context.activeState.awb.gains.blue = 1.0;\n> > > > - context.activeState.awb.gains.green = 1.0;\n> > > > + context.activeState.awb.gains.manual.red = 1.0;\n> > > > + context.activeState.awb.gains.manual.blue = 1.0;\n> > > > + context.activeState.awb.gains.manual.green = 1.0;\n> > > > + context.activeState.awb.gains.automatic.red = 1.0;\n> > > > + context.activeState.awb.gains.automatic.blue = 1.0;\n> > > > + context.activeState.awb.gains.automatic.green = 1.0;\n> > > > context.activeState.awb.autoEnabled = true;\n> > > >\n> > > > /*\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> > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > - rkisp1_params_cfg *params)\n> > > > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > > > {\n> > > > - params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green;\n> > > > - params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue;\n> > > > - params->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red;\n> > > > - params->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green;\n> > > > + /*\n> > > > + * This is the latest time we can read the active state. This is the\n> > > > + * most up-to-date automatic values we can read.\n> > > > + */\n> > > > + if (frameContext.awb.autoEnabled) {\n> > > > + frameContext.awb.gains.red = context.activeState.awb.gains.automatic.red;\n> > > > + frameContext.awb.gains.green = context.activeState.awb.gains.automatic.green;\n> > > > + frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue;\n> > > > + }\n> > > > +\n> > > > + params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green;\n> > > > + params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue;\n> > > > + params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red;\n> > > > + params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green;\n> > > >\n> > > > /* Update the gains. */\n> > > > params->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> > > > [[maybe_unused]] const uint32_t frame,\n> > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > + RkISP1FrameContext &frameContext,\n> > > > const ControlList &controls)\n> > > > {\n> > > > auto &awb = context.activeState.awb;\n> > > > @@ -142,21 +154,29 @@ void Awb::queueRequest(IPAContext &context,\n> > > >\n> > > > const auto &colourGains = controls.get(controls::ColourGains);\n> > > > if (colourGains && !awb.autoEnabled) {\n> > > > - awb.gains.red = (*colourGains)[0];\n> > > > - awb.gains.blue = (*colourGains)[1];\n> > > > + awb.gains.manual.red = (*colourGains)[0];\n> > > > + awb.gains.manual.blue = (*colourGains)[1];\n> > >\n> > > I really do feel like if the application provides a set of manual gains,\n> > > we should store them in the internal manual gains.\n> > >\n> > > Unless perhaps if we say upon transitioning from automatic, to manual\n> > > (Auto-Disabled)if we do not have a manual value in the same request,\n> > > we'll use the most recently computed auto values and hold them until\n> > > told otherwise.\n> > >\n> > > If we don't - we're going to suddenly jump to 'manually' set values that\n> > > are some historic values... It just feels quite wrong to me.\n> > \n> > I side with Kieran, even if libcamera::AwbEnable, probably because\n> > it's old, doesn't tell much about the expected behaviour. After many\n> > discussions on AEGC, the decision was to re-start from the last\n> > computed vaues in the previous mode, when transitioning from Auto to\n> > Manual or viceversa.\n\nI'd like to have this discussion in the context of the new AWB and AEGC\ncontrols, on top of this patch series. I think this patch provides a\ngood base for the discussion actually, as we'll be able to see the\neffect on the code and test the runtime behaviour.\n\n> > We can easily detect if a transition is happening here and handle it\n> > here maybe ?\n> \n> Yes, I think it can be detected here. I would have:\n> \n> const auto &awbEnable = controls.get(controls::AwbEnable);\n> if (awbEnable && *awbEnable != awb.autoEnabled) {\n> awb.autoEnabled = *awbEnable;\n> \n> LOG(RkISP1Awb, Debug)\n> << (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> \n> +\t\t/*\n> +\t\t * Pre-set manual controls with the current state to\n> +\t\t * support switching to auto disabled mode when the user\n> +\t\t * does not provide manual values in the same request.\n> +\t\t */\n> +\t\tif (!awb.autoEnabled) {\n> +\t\t\tawb.gains.manual.red = awb.gains.automatic.red;\n> +\t\t\tawb.gains.manual.blue = awb.gains.automatic.blue;\n> +\t\t}\n> }\n> \n> > > >\n> > > > LOG(RkISP1Awb, Debug)\n> > > > - << \"Set colour gains to red: \" << awb.gains.red\n> > > > - << \", blue: \" << awb.gains.blue;\n> > > > + << \"Set colour gains to red: \" << awb.gains.manual.red\n> > > > + << \", blue: \" << awb.gains.manual.blue;\n> > > > + }\n> > > > +\n> > > > + frameContext.awb.autoEnabled = awb.autoEnabled;\n> > > > +\n> > > > + if (!awb.autoEnabled) {\n> > > > + frameContext.awb.gains.red = awb.gains.manual.red;\n> > > > + frameContext.awb.gains.green = 1.0;\n> > > > + frameContext.awb.gains.blue = awb.gains.manual.blue;\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> > > > [[maybe_unused]] const uint32_t frame,\n> > > > - [[maybe_unused]] RkISP1FrameContext &frameCtx,\n> > > > + RkISP1FrameContext &frameContext,\n> > > > const rkisp1_stat_buffer *stats)\n> > > > {\n> > > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > > @@ -187,30 +207,27 @@ void Awb::process([[maybe_unused]] IPAContext &context,\n> > > > double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n> > > > double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n> > > >\n> > > > + frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> > > > +\n> > >\n> > > We likely want to store the temperature of each frame to make sure we\n> > > update the metadata, but I wonder if we might also want to track the\n> > > colour temperature in the Active state so that for instance the LSC can\n> > > check what the current temperature is?\n> \n> I see this gets introduced later anyway - so it's fine.\n> \n> > > If we are able to get the algorithms to update the metadata structure\n> > > directly, then a value like the colour temperature here probably doesn't\n> > > need to go into the frame context, just to be read later.\n\nI agree, it should be stored in metadata. I'll try to implement that,\nbut on top of this series as it's quite large already.\n\n> > > But for now - I think this is accurate.\n> > >\n> > > I suspect perhaps a \"ControlList metadata\" could/should go into the base\n> > > FrameContext, so that all calls given a FrameContext can directly update\n> > > the metadata which will then be passed to the pipeline handler to put\n> > > into the request when it completes.\n> > >\n> > > (not this patch, later)\n> > >\n> > > > /* Estimate the red and blue gains to apply in a grey world. */\n> > > > double redGain = greenMean / (redMean + 1);\n> > > > double blueGain = greenMean / (blueMean + 1);\n> > > >\n> > > > /* Filter the values to avoid oscillations. */\n> > > > double speed = 0.2;\n> > > > - redGain = speed * redGain + (1 - speed) * activeState.awb.gains.red;\n> > > > - blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue;\n> > > > + redGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;\n> > > > + blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;\n> > > >\n> > > > /*\n> > > > * Gain values are unsigned integer value, range 0 to 4 with 8 bit\n> > > > - * fractional part.\n> > > > + * fractional part. Hardcode the green gain to 1.0.\n> > > > */\n> > > > - if (activeState.awb.autoEnabled) {\n> > > > - activeState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> > > > - activeState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> > > > - }\n> > > > - /* Hardcode the green gain to 1.0. */\n> > > > - activeState.awb.gains.green = 1.0;\n> > > > + activeState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> > > > + activeState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> > > > + activeState.awb.gains.automatic.green = 1.0;\n> > > >\n> > > > - activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);\n> > > > -\n> > > > - LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << context.activeState.awb.gains.red\n> > > > - << \" and for blue: \" << context.activeState.awb.gains.blue;\n> > > > + LOG(RkISP1Awb, Debug) << \"Gain found for red: \" << activeState.awb.gains.automatic.red\n> > > > + << \" 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 88e0631c8d39..cd1443e1ed46 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 RkISP1FrameContext::awb\n> > > > + * \\brief Automatic White Balance parameters for this frame\n> > > > + *\n> > > > + * \\struct RkISP1FrameContext::awb.gains\n> > > > + * \\brief White balance gains\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::awb.gains.red\n> > > > + * \\brief White balance gain for R channel\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::awb.gains.green\n> > > > + * \\brief White balance gain for G channel\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::awb.gains.blue\n> > > > + * \\brief White balance gain for B channel\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::awb.temperatureK\n> > > > + * \\brief Estimated color temperature\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::awb.autoEnabled\n> > > > + * \\brief Whether the Auto White Balance algorithm is enabled\n> > > > + */\n> > >\n> > > I believe these are all going to get simplified later ... I like that\n> > > ... but if the support class came earlier, it could move to that\n> > > directly in here.\n> > >\n> > > Oh - in fact, it looks like the patch I'm thinking of isn't in this\n> > > series yet :(\n> > >\n> > >\n> > > Ok - so I'm happy enough with this. I look forward to the RGB / Pixel class\n> > > which I think helps simplify this code a fair bit, but it's not required\n> > > for this conversion.\n> > >\n> > >\n> > > I think this patch tells me:\n> > >\n> > > - I need to understand how we globally expect to handle setting manual\n> > > values when an algorithm is configured to use automatic ones.\n> > \n> > We surely need to push for a single model for all controls. I haven't\n> > thought too much about which algorithms would not be able to comply and\n> > why, but if no obvious reasons, I think we should aim to be\n> > consistent.\n> \n> I like the idea that switching from automatic to manual without\n> specifying a manual value will take the most up to date automatic values\n> and lock them in as the manual values at that point. Then applications\n> can of course update them to new manual values (even in the request that\n> causes the transition) - or ... it simply disables the automatic\n> operation from that point, and doesn't cause any visual switch to some\n> historic manual value.\n> \n> > > - I look forward to your class that groups R/G/B values\n\nThat one is a big rabbit hole :-)\n\n> > > - We should already add a common metadata object to populate, so we can\n> > > report metadata back in completed requests.\n> > >\n> > > - ColourTemperature should go straight to the metadata\n\nWill do so on top.\n\n> > > - ActiveState should store the ColourTemperature so that LSC can refer\n> > > to it.\n> > >\n> > >\n> > > But all of that can be done on top...\n> > >\n> > > so\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > +\n> > > > /**\n> > > > * \\var RkISP1FrameContext::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 ecf993cd22d7..d97aae9a97b4 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -56,12 +56,18 @@ struct IPAActiveState {\n> > > >\n> > > > struct {\n> > > > struct {\n> > > > - double red;\n> > > > - double green;\n> > > > - double blue;\n> > > > + struct {\n> > > > + double red;\n> > > > + double green;\n> > > > + double blue;\n> > > > + } manual;\n> > > > + struct {\n> > > > + double red;\n> > > > + double green;\n> > > > + double blue;\n> > > > + } automatic;\n> > > > } gains;\n> > > >\n> > > > - double temperatureK;\n> > > > bool autoEnabled;\n> > > > } awb;\n> > > >\n> > > > @@ -90,6 +96,17 @@ struct RkISP1FrameContext : public FrameContext {\n> > > > double gain;\n> > > > } agc;\n> > > >\n> > > > + struct {\n> > > > + struct {\n> > > > + double red;\n> > > > + double green;\n> > > > + double blue;\n> > > > + } gains;\n> > > > +\n> > > > + double temperatureK;\n> > > > + bool autoEnabled;\n> > > > + } awb;\n> > > > +\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;","headers":{"Return-Path":"","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 62C5BBD16B\n\tfor ;\n\tThu, 22 Sep 2022 20:52:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB3476221E;\n\tThu, 22 Sep 2022 22:52:45 +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 977186219A\n\tfor ;\n\tThu, 22 Sep 2022 22:52:44 +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 AC4466BE;\n\tThu, 22 Sep 2022 22:52:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663879965;\n\tbh=OksUZzYAcFiHYaILbu8m2ElER6BCxen1m6fgMbHg+Os=;\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=SzSm+MdlwNbg7MGFJysb4eiUqGPhIJ3B+ieHPgv84pwan3lPAA/jkVXI52Q30e20E\n\ti8+zcHQssVEypGPWCQSrZjebD48qJjjui9SyHkRj3DcUDt71MhW5Wm3/6URSavtbcm\n\tA/O5jAW0+M4dp4ytpSGwbPl0mT6PsmoxPjgXC8biB04yVUMCrQWs7nCVUqho+gzyLR\n\t8s2LAjJma2eHBby0ppET9PNEPz7BMBhoKpnq60aNKSmNh8mt9RMhoL7QJUdxOFhdBF\n\t9XstKntaGPegaTtDBxQJxlg2K6/ea+VrRG1PchLEcQQVMov/EH+Hhs/PWwowEkvNOW\n\tHZadlaM33LepA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663879964;\n\tbh=OksUZzYAcFiHYaILbu8m2ElER6BCxen1m6fgMbHg+Os=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DnzGmWuCBkAvMaqBi+9EWuwifKX7jcpZE5aCYxeOHCGB6puJ+byrMfj2EOQHdY6q+\n\tSCCmQn8MrfqWRHfuag9HDd2ahXPmMGvNz0ygkg5XufMY+cLF24pyVGybYGRIVtAgLo\n\tZruJI9bPTaBq78gUsmT4reLgfin+sLzMGITX9Wl0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DnzGmWuC\"; dkim-atps=neutral","Date":"Thu, 22 Sep 2022 23:52:28 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-21-laurent.pinchart@ideasonboard.com>\n\t<166371455535.18961.15370617823339965490@Monstersaurus>\n\t<20220921203404.si375nd5lehrnt2u@uno.localdomain>\n\t<166383797151.56880.9141645036894131390@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166383797151.56880.9141645036894131390@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 20/32] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]