[{"id":32270,"web_url":"https://patchwork.libcamera.org/comment/32270/","msgid":"<87r077fqea.fsf@redhat.com>","date":"2024-11-19T10:51:09","subject":"Re: [PATCH v3 15/17] ipa: rkisp1: awb: Use RGB class to store\n\tcolour gains","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n\n> Replace the manual storage of gains in the IPA active state and frame\n> context with usage of the RGB class. This simplifies the code thanks to\n> usage of the arithmetic functions provided by the RGB class.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Milan Zamazal <mzamazal@redhat.com>\n\n> ---\n>  src/ipa/rkisp1/algorithms/awb.cpp | 71 +++++++++++--------------------\n>  src/ipa/rkisp1/ipa_context.cpp    | 31 +-------------\n>  src/ipa/rkisp1/ipa_context.h      | 20 ++-------\n>  3 files changed, 32 insertions(+), 90 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index dbeaf81565ff..c330feff9f5d 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -47,12 +47,8 @@ Awb::Awb()\n>  int Awb::configure(IPAContext &context,\n>  \t\t   const IPACameraSensorInfo &configInfo)\n>  {\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.gains.manual = RGB<double>{ 1.0 };\n> +\tcontext.activeState.awb.gains.automatic = RGB<double>{ 1.0 };\n>  \tcontext.activeState.awb.autoEnabled = true;\n>  \n>  \t/*\n> @@ -89,21 +85,17 @@ void Awb::queueRequest(IPAContext &context,\n>  \n>  \tconst auto &colourGains = controls.get(controls::ColourGains);\n>  \tif (colourGains && !awb.autoEnabled) {\n> -\t\tawb.gains.manual.red = (*colourGains)[0];\n> -\t\tawb.gains.manual.blue = (*colourGains)[1];\n> +\t\tawb.gains.manual.r() = (*colourGains)[0];\n> +\t\tawb.gains.manual.b() = (*colourGains)[1];\n>  \n>  \t\tLOG(RkISP1Awb, Debug)\n> -\t\t\t<< \"Set colour gains to red: \" << awb.gains.manual.red\n> -\t\t\t<< \", blue: \" << awb.gains.manual.blue;\n> +\t\t\t<< \"Set colour gains to \" << awb.gains.manual;\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> -\t}\n> +\tif (!awb.autoEnabled)\n> +\t\tframeContext.awb.gains = awb.gains.manual;\n>  }\n>  \n>  /**\n> @@ -116,19 +108,16 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\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> +\tif (frameContext.awb.autoEnabled)\n> +\t\tframeContext.awb.gains = context.activeState.awb.gains.automatic;\n>  \n>  \tauto gainConfig = params->block<BlockType::AwbGain>();\n>  \tgainConfig.setEnabled(true);\n>  \n> -\tgainConfig->gain_green_b = std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff);\n> -\tgainConfig->gain_blue = std::clamp<int>(256 * frameContext.awb.gains.blue, 0, 0x3ff);\n> -\tgainConfig->gain_red = std::clamp<int>(256 * frameContext.awb.gains.red, 0, 0x3ff);\n> -\tgainConfig->gain_green_r = std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff);\n> +\tgainConfig->gain_green_b = std::clamp<int>(256 * frameContext.awb.gains.g(), 0, 0x3ff);\n> +\tgainConfig->gain_blue = std::clamp<int>(256 * frameContext.awb.gains.b(), 0, 0x3ff);\n> +\tgainConfig->gain_red = std::clamp<int>(256 * frameContext.awb.gains.r(), 0, 0x3ff);\n> +\tgainConfig->gain_green_r = std::clamp<int>(256 * frameContext.awb.gains.g(), 0, 0x3ff);\n>  \n>  \t/* If we have already set the AWB measurement parameters, return. */\n>  \tif (frame > 0)\n> @@ -196,8 +185,8 @@ void Awb::process(IPAContext &context,\n>  \n>  \tmetadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);\n>  \tmetadata.set(controls::ColourGains, {\n> -\t\t\tstatic_cast<float>(frameContext.awb.gains.red),\n> -\t\t\tstatic_cast<float>(frameContext.awb.gains.blue)\n> +\t\t\tstatic_cast<float>(frameContext.awb.gains.r()),\n> +\t\t\tstatic_cast<float>(frameContext.awb.gains.b())\n>  \t\t});\n>  \tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n>  \n> @@ -253,12 +242,7 @@ void Awb::process(IPAContext &context,\n>  \t * divide by the gains that were used to get the raw means from the\n>  \t * sensor.\n>  \t */\n> -\tRGB<double> gains{{\n> -\t\tframeContext.awb.gains.red,\n> -\t\tframeContext.awb.gains.green,\n> -\t\tframeContext.awb.gains.blue\n> -\t}};\n> -\trgbMeans /= gains;\n> +\trgbMeans /= frameContext.awb.gains;\n>  \n>  \t/*\n>  \t * If the means are too small we don't have enough information to\n> @@ -278,8 +262,11 @@ void Awb::process(IPAContext &context,\n>  \t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n>  \t * divisor to a minimum value of 1.0.\n>  \t */\n> -\tdouble redGain = rgbMeans.g() / std::max(rgbMeans.r(), 1.0);\n> -\tdouble blueGain = rgbMeans.g() / std::max(rgbMeans.b(), 1.0);\n> +\tRGB<double> gains({\n> +\t\trgbMeans.g() / std::max(rgbMeans.r(), 1.0),\n> +\t\t1.0,\n> +\t\trgbMeans.g() / std::max(rgbMeans.b(), 1.0)\n> +\t});\n>  \n>  \t/*\n>  \t * Clamp the gain values to the hardware, which expresses gains as Q2.8\n> @@ -287,24 +274,18 @@ void Awb::process(IPAContext &context,\n>  \t * divisions by zero when computing the raw means in subsequent\n>  \t * iterations.\n>  \t */\n> -\tredGain = std::clamp(redGain, 1.0 / 256, 1023.0 / 256);\n> -\tblueGain = std::clamp(blueGain, 1.0 / 256, 1023.0 / 256);\n> +\tgains = gains.max(1.0 / 256).min(1023.0 / 256);\n>  \n>  \t/* Filter the values to avoid oscillations. */\n>  \tdouble speed = 0.2;\n> -\tredGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;\n> -\tblueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;\n> +\tgains = gains * speed + activeState.awb.gains.automatic * (1 - speed);\n>  \n> -\tactiveState.awb.gains.automatic.red = redGain;\n> -\tactiveState.awb.gains.automatic.blue = blueGain;\n> -\tactiveState.awb.gains.automatic.green = 1.0;\n> +\tactiveState.awb.gains.automatic = gains;\n>  \n>  \tLOG(RkISP1Awb, Debug)\n>  \t\t<< std::showpoint\n> -\t\t<< \"Means \" << rgbMeans\n> -\t\t<< \", gains [\" << activeState.awb.gains.automatic.red << \", \"\n> -\t\t<< activeState.awb.gains.automatic.green << \", \"\n> -\t\t<< activeState.awb.gains.automatic.blue << \"], temp \"\n> +\t\t<< \"Means \" << rgbMeans << \", gains \"\n> +\t\t<< activeState.awb.gains.automatic << \", temp \"\n>  \t\t<< activeState.awb.temperatureK << \"K\";\n>  }\n>  \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 14d0c02a2b32..8f545cd76d52 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -188,30 +188,12 @@ namespace libcamera::ipa::rkisp1 {\n>   * \\struct IPAActiveState::awb.gains\n>   * \\brief White balance gains\n>   *\n> - * \\struct IPAActiveState::awb.gains.manual\n> + * \\var IPAActiveState::awb.gains.manual\n>   * \\brief Manual white balance gains (set through requests)\n>   *\n> - * \\var IPAActiveState::awb.gains.manual.red\n> - * \\brief Manual white balance gain for R channel\n> - *\n> - * \\var IPAActiveState::awb.gains.manual.green\n> - * \\brief Manual white balance gain for G channel\n> - *\n> - * \\var IPAActiveState::awb.gains.manual.blue\n> - * \\brief Manual white balance gain for B channel\n> - *\n> - * \\struct IPAActiveState::awb.gains.automatic\n> + * \\var 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.temperatureK\n>   * \\brief Estimated color temperature\n>   *\n> @@ -333,15 +315,6 @@ namespace libcamera::ipa::rkisp1 {\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> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 7b93a9e9461d..b4dec0c3288d 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -25,6 +25,7 @@\n>  #include <libipa/camera_sensor_helper.h>\n>  #include <libipa/fc_queue.h>\n>  #include <libipa/matrix.h>\n> +#include <libipa/vector.h>\n>  \n>  namespace libcamera {\n>  \n> @@ -87,16 +88,8 @@ struct IPAActiveState {\n>  \n>  \tstruct {\n>  \t\tstruct {\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\tRGB<double> manual;\n> +\t\t\tRGB<double> automatic;\n>  \t\t} gains;\n>  \n>  \t\tunsigned int temperatureK;\n> @@ -140,12 +133,7 @@ struct IPAFrameContext : public FrameContext {\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\tRGB<double> gains;\n>  \t\tbool autoEnabled;\n>  \t} awb;","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 3A86DC32F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 19 Nov 2024 10:51:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DD0265EF7;\n\tTue, 19 Nov 2024 11:51:18 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01B1C6589D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Nov 2024 11:51:15 +0100 (CET)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-623-iVCtYEZQPmW4dzWPYjSx1w-1; Tue, 19 Nov 2024 05:51:13 -0500","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-4316e2dde9eso29345555e9.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Nov 2024 02:51:13 -0800 (PST)","from nuthatch ([2a00:102a:400a:489a:34bf:5bf1:e776:7185])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-432dac216a4sm187849615e9.44.2024.11.19.02.51.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 19 Nov 2024 02:51:10 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"TxFdZ/TU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1732013474;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=7El+5B1gk8FkZWJEvQW6mloreHW9PRdBhGB31eYiUN8=;\n\tb=TxFdZ/TUw1OtPV8/G5Uc4sf4Yna9PLUGCquz9JX5sLu7b98bmymxt/jIGitZdta81IW1mG\n\tRNDoni7LScwUPYVhwNz1B9Ge501lVfAqZn0b6NJT7Mirk0HaOSZUxsyYaECGgkMcUjZcJ+\n\tsnnSfGm0i6E8tMBpjOCnol/5SxoibgE=","X-MC-Unique":"iVCtYEZQPmW4dzWPYjSx1w-1","X-Mimecast-MFC-AGG-ID":"iVCtYEZQPmW4dzWPYjSx1w","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732013472; x=1732618272;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=7El+5B1gk8FkZWJEvQW6mloreHW9PRdBhGB31eYiUN8=;\n\tb=GZ/JXdcEusW7Fp4wVPgf4cG+3HxE21LsNXcsaN/A+Vj/6dnJgdodQelNIN2nz5BDdb\n\tftsHcm2TwSZ6ILzEpHxKGOh3tj5v61TH1sKHWx8Hj6s7VMtZNBwLZGRTIeHScmgyMHxZ\n\tM9vAD7tpEAQPd9uiNbZ75LsSGM1GyPvEyvDqFp6v3YJmz02rDRLyhZXgdfB2L2kX6dkJ\n\tM3r/Wk9J2ZmNT+MLSdM6RdT8yWCxNz3bdkl8CAI3zn2bJ38eRnzI2ToERZRgATaQQK0f\n\t1ogZibq8Tn5rPvRnEnhvWizQK6MZgH9/KEp7R2WYg6Fh00By/kQf0UH4mnVSZ2dKXcMo\n\trLxg==","X-Gm-Message-State":"AOJu0YxDI7dA++Aij6BtTy5qBslbcgwtSbnhjyZkhb0TRDUlv8l1iB/l\n\ttY9zJ4gmGJd4pDZCgHp2HgrrTNElWANTPyXWMWtqFVRMjRMIn3pCep9k60VFVPmBc3dq9G3FdqF\n\t0BHFFbQiSURPzCbPHiVxSpUi6Gxu05s/TjL9B9XntUPX6uEo3JQtWmHCdk6pzFyDM7y1lzUcb0o\n\t7mbjYoP+YFtOABlqvDcWSQxsMmfBR/AAYm6PG1SZvJjFKWGNENfE0N8Q4=","X-Received":["by 2002:a05:600c:510f:b0:431:5c3d:1700 with SMTP id\n\t5b1f17b1804b1-432df78f39fmr146406875e9.21.1732013471931; \n\tTue, 19 Nov 2024 02:51:11 -0800 (PST)","by 2002:a05:600c:510f:b0:431:5c3d:1700 with SMTP id\n\t5b1f17b1804b1-432df78f39fmr146406605e9.21.1732013471439; \n\tTue, 19 Nov 2024 02:51:11 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFoQo0Y57oIVv9doNUIURCxY9E1pc6ue4jJkd29zYHi+334xAgYmgs28Hw4io0LIT1SVNTTIA==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 15/17] ipa: rkisp1: awb: Use RGB class to store\n\tcolour gains","In-Reply-To":"<20241118221618.13953-16-laurent.pinchart@ideasonboard.com>\n\t(Laurent Pinchart's message of \"Tue, 19 Nov 2024 00:16:16 +0200\")","References":"<20241118221618.13953-1-laurent.pinchart@ideasonboard.com>\n\t<20241118221618.13953-16-laurent.pinchart@ideasonboard.com>","Date":"Tue, 19 Nov 2024 11:51:09 +0100","Message-ID":"<87r077fqea.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"GsKpMG0nmKAnTPovyladXzZNzqSY1tKBqC5sSBpDnFE_1732013472","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]