[{"id":26713,"web_url":"https://patchwork.libcamera.org/comment/26713/","msgid":"","date":"2023-03-22T14:41:58","subject":"Re: [libcamera-devel] [PATCH v1 09/10] ipa: raspberrypi: Generalise\n\tthe agc algorithm","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch!\n\nOn Wed, 22 Mar 2023 at 13:06, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Remove any hard-coded assumptions about the target hardware platform\n> from the AGC algorithm. Instead, use the \"target\" string provided by\n> the camera tuning config and generalised statistics structures to\n> determing parameters such as grid and region sizes.\n\nI couldn't see that we did use the \"target\" string, so maybe perhaps\njust remove the reference to it in the commit message?\n\n>\n> This change replaces all hard-coded arrays with equivalent std::vector\n> types.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 19 ++++++++++++-------\n> src/ipa/raspberrypi/controller/rpi/agc.h | 9 +--------\n> 2 files changed, 13 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 4ea0dd41e66c..3e4a8149b9ae 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -31,17 +31,12 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> int AgcMeteringMode::read(const libcamera::YamlObject ¶ms)\n> {\n> const YamlObject &yamlWeights = params[\"weights\"];\n> - if (yamlWeights.size() != AgcStatsSize) {\n> - LOG(RPiAgc, Error) << \"AgcMeteringMode: Incorrect number of weights\";\n> - return -EINVAL;\n> - }\n>\n> - unsigned int num = 0;\n> for (const auto &p : yamlWeights.asList()) {\n> auto value = p.get();\n> if (!value)\n> return -EINVAL;\n> - weights[num++] = *value;\n> + weights.push_back(*value);\n> }\n>\n> return 0;\n> @@ -249,6 +244,14 @@ int Agc::read(const libcamera::YamlObject ¶ms)\n> if (ret)\n> return ret;\n>\n> + const Size &size = getHardwareConfig().agcZoneWeights;\n> + for (auto const &modes : config_.meteringModes) {\n> + if (modes.second.weights.size() != size.width * size.height) {\n> + LOG(RPiAgc, Error) << \"AgcMeteringMode: Incorrect number of weights\";\n> + return -EINVAL;\n> + }\n> + }\n> +\n> /*\n> * Set the config's defaults (which are the first ones it read) as our\n> * current modes, until someone changes them. (they're all known to\n> @@ -582,9 +585,11 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)\n> }\n>\n> static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> - double weights[], double gain)\n> + std::vector &weights, double gain)\n\nI'm guessing the weights could be const? (of course that would have\nbeen true in the original code too, which would be my fault) Don't\nknow if it's worth the trouble though.\n\nOther than that:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n\n> {\n> constexpr uint64_t maxVal = 1 << Statistics::NormalisationFactorPow2;\n> +\n> + ASSERT(weights.size() == stats->agcRegions.numRegions());\n> /*\n> * Note how the calculation below means that equal weights give you\n> * \"average\" metering (i.e. all pixels equally important).\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h\n> index f04896ca25ad..d11a49c9cd85 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> @@ -17,17 +17,10 @@\n>\n> /* This is our implementation of AGC. */\n>\n> -/*\n> - * This is the number actually set up by the firmware, not the maximum possible\n> - * number (which is 16).\n> - */\n> -\n> -constexpr unsigned int AgcStatsSize = 15;\n> -\n> namespace RPiController {\n>\n> struct AgcMeteringMode {\n> - double weights[AgcStatsSize];\n> + std::vector weights;\n> int read(const libcamera::YamlObject ¶ms);\n> };\n>\n> --\n> 2.34.1\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 9E6FBC0F2A\n\tfor ;\n\tWed, 22 Mar 2023 14:42:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD0CF626E5;\n\tWed, 22 Mar 2023 15:42:11 +0100 (CET)","from mail-ot1-x32d.google.com (mail-ot1-x32d.google.com\n\t[IPv6:2607:f8b0:4864:20::32d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 962D761ECE\n\tfor ;\n\tWed, 22 Mar 2023 15:42:10 +0100 (CET)","by mail-ot1-x32d.google.com with SMTP id\n\t103-20020a9d0870000000b0069f000acf40so8055106oty.1\n\tfor ;\n\tWed, 22 Mar 2023 07:42:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679496131;\n\tbh=lvDZP9qZQxuktKn2jErcp0Q6Hw5OPgGye/3fTOtx3JE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=M+XX61ehUgF+Nz0VgQTAnt3wFs5CdV+vFDIMSAHAFIB8Yrstfw/YKyCn+0IjxAWs+\n\t9QzUvomITlcsWXZnwwkQXU8yghrlKPRuQJzvsQDpoXlK00oDz5ibRwGhbqdhBvD786\n\tQsWF8IClGcGOxKfS2h/Ln7FA1xEUavJ2kudRcLYSUsCEvY5Y6uOsauP/b1+kkhSgnC\n\t4y/3KUlz3ebf073NAtHbQ+vekKz1vTPKn3UevImKjwDkKkcCbYBsJRA3Q3jtqirM6O\n\t5bL/WKJ5YTgUECn/bf1DrFZaJJ22vScPSmTK3t7+9AYfWRJiPpcz1I7IR3BZskBzmx\n\tXiZj2cRFIJSrQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679496129;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=nsTj8KXhVQFuZNXkAUwWvlq84pXv2MWRVmRliGaBcp8=;\n\tb=ja625epLENrFImf7IOhaH3LZ+S6u/HYu4wYD3IIH+yhV7yogtBqvkBlUnssH3zQJ3v\n\tStcbP6UA8u6NaScnGpWQ98g6kqCc6w0TtppMmcfxjMJffQdEH5yG91xZCOLYX2feKCTw\n\tWs9m9efnBFCeQSuCiLWqBdclJijPXnLdQJBQRxSMNj8zp674L5oIxoHBYnKCVG+72hMy\n\t7aeCj3fSb8tG44bOnL9ufxZeqwfyR/bdhAk9M7n9SxL9BBJ964LnFWqmopTUPtUVIlX2\n\tvJO/yikq4AkHRrt/oaVgu2U/NkzGy54jV/7r4u9CFcAv2QXmp+zwRnZ9Vwog0MaFjD1a\n\t9RcA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ja625epL\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679496129;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=nsTj8KXhVQFuZNXkAUwWvlq84pXv2MWRVmRliGaBcp8=;\n\tb=M9gKrXTm+UuZUfDG30m9WaGu/qKTNGuirE1lgc8tv3XsiNN3KpKI9SaIO7bjwbX6Cp\n\tqqDUNh0HhSZ+V4DOuwhhRrH1VC0PlrY3ewovWv1yUdo4z/bJbBMA93zuzpVFs+ZMhPN5\n\tB6FkjmkwFz+YzZrsYJ5qdviWddJE2q+09xJhD56taS0VxJZ3pth0hYkE2NApby+AU4T0\n\th7k+AI7sZCl+Aa9H6Cet1TlOjkPgM+TVpB8WOnKUTHJEueoEGac+V9+w2Nz4BAjJ3EHy\n\tDFW3/XSOK59zGhhxp0bDuX4qxVcDYkal+7RNmP8jUHj2FLhBuav8UFlMYjSfhfGM5kWb\n\tMxyQ==","X-Gm-Message-State":"AO0yUKXdfur97FVBiWChTsk1S2oqX5iuXWngqjpxkP4Lm2SteyToEvTn\n\txxiEAj4yDWAWWK+pAe0ieExYlNrFYJ1QXFuckFg8sg==","X-Google-Smtp-Source":"AK7set+u0GoOqtXsP1iY/gj1TIS7UPTXrzmSqWl/6Y4QGzxITaCoemLPDqeAg/ydxR5cxDwHAs9nAOK3JdcYECHNQxk=","X-Received":"by 2002:a9d:7cc4:0:b0:699:bf77:e691 with SMTP id\n\tr4-20020a9d7cc4000000b00699bf77e691mr1107508otn.4.1679496129297;\n\tWed, 22 Mar 2023 07:42:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20230322130612.5208-1-naush@raspberrypi.com>\n\t<20230322130612.5208-10-naush@raspberrypi.com>","In-Reply-To":"<20230322130612.5208-10-naush@raspberrypi.com>","Date":"Wed, 22 Mar 2023 14:41:58 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 09/10] ipa: raspberrypi: Generalise\n\tthe agc algorithm","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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26727,"web_url":"https://patchwork.libcamera.org/comment/26727/","msgid":"<20230324093740.cnahlf6dhgcgbgsv@uno.localdomain>","date":"2023-03-24T09:37:40","subject":"Re: [libcamera-devel] [PATCH v1 09/10] ipa: raspberrypi: Generalise\n\tthe agc algorithm","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Wed, Mar 22, 2023 at 01:06:11PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Remove any hard-coded assumptions about the target hardware platform\n> from the AGC algorithm. Instead, use the \"target\" string provided by\n> the camera tuning config and generalised statistics structures to\n> determing parameters such as grid and region sizes.\n>\n> This change replaces all hard-coded arrays with equivalent std::vector\n> types.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 19 ++++++++++++-------\n> src/ipa/raspberrypi/controller/rpi/agc.h | 9 +--------\n> 2 files changed, 13 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 4ea0dd41e66c..3e4a8149b9ae 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -31,17 +31,12 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> int AgcMeteringMode::read(const libcamera::YamlObject ¶ms)\n> {\n> \tconst YamlObject &yamlWeights = params[\"weights\"];\n> -\tif (yamlWeights.size() != AgcStatsSize) {\n> -\t\tLOG(RPiAgc, Error) << \"AgcMeteringMode: Incorrect number of weights\";\n> -\t\treturn -EINVAL;\n> -\t}\n>\n> -\tunsigned int num = 0;\n> \tfor (const auto &p : yamlWeights.asList()) {\n> \t\tauto value = p.get();\n> \t\tif (!value)\n> \t\t\treturn -EINVAL;\n> -\t\tweights[num++] = *value;\n> +\t\tweights.push_back(*value);\n\n\n> \t}\n>\n> \treturn 0;\n> @@ -249,6 +244,14 @@ int Agc::read(const libcamera::YamlObject ¶ms)\n> \tif (ret)\n> \t\treturn ret;\n>\n> +\tconst Size &size = getHardwareConfig().agcZoneWeights;\n> +\tfor (auto const &modes : config_.meteringModes) {\n> +\t\tif (modes.second.weights.size() != size.width * size.height) {\n> +\t\t\tLOG(RPiAgc, Error) << \"AgcMeteringMode: Incorrect number of weights\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n> +\n> \t/*\n> \t * Set the config's defaults (which are the first ones it read) as our\n> \t * current modes, until someone changes them. (they're all known to\n> @@ -582,9 +585,11 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)\n> }\n>\n> static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> -\t\t\t double weights[], double gain)\n> +\t\t\t std::vector &weights, double gain)\n> {\n> \tconstexpr uint64_t maxVal = 1 << Statistics::NormalisationFactorPow2;\n> +\n> +\tASSERT(weights.size() == stats->agcRegions.numRegions());\n\nMaybe an empty line, but just if you have to resend\n\nReviewed-by: Jacopo Mondi \n\n> \t/*\n> \t * Note how the calculation below means that equal weights give you\n> \t * \"average\" metering (i.e. all pixels equally important).\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h\n> index f04896ca25ad..d11a49c9cd85 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> @@ -17,17 +17,10 @@\n>\n> /* This is our implementation of AGC. */\n>\n> -/*\n> - * This is the number actually set up by the firmware, not the maximum possible\n> - * number (which is 16).\n> - */\n> -\n> -constexpr unsigned int AgcStatsSize = 15;\n> -\n> namespace RPiController {\n>\n> struct AgcMeteringMode {\n> -\tdouble weights[AgcStatsSize];\n> +\tstd::vector weights;\n> \tint read(const libcamera::YamlObject ¶ms);\n> };\n>\n> --\n> 2.34.1\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 6B399C0F2A\n\tfor ;\n\tFri, 24 Mar 2023 09:37:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1B1F626D7;\n\tFri, 24 Mar 2023 10:37:45 +0100 (CET)","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 C981E61ED0\n\tfor ;\n\tFri, 24 Mar 2023 10:37:43 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 52370A49;\n\tFri, 24 Mar 2023 10:37:43 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679650665;\n\tbh=fr96Qpz1SF4vSzSDgGZidtEmYV2BAanZ1Lw8T3DwghA=;\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=t4tQyTILWzxnOYUYggOGT8feq4tfeZU1QsPUbpdPywT/dtgVvWdMXEOL1m7od1EbQ\n\tXXTSDopf0izdEn5Dia8flLWAe3IHeob3xSv+LD0cSIfuNQLBGdDkWZlmTGxPvvtmqB\n\t7Cgs7y1XPpiNdKB0HEHm37UNn0Pv/sZBA9fnUbCiJUsnx4p+ahulla+cuR5dcUixZF\n\taWIJbsw0aFmkzQnxef0jW5YhpRE9GpSFpmn3feuKvhr0m4tVWazZ60v8/1NI7VITLB\n\tjZTj3Ylczz0AYnb/mJplhePFwB2qKgbSwfnSOdyTrpWboBxd90auV7kdYjFg28wN+f\n\tdPz96kC4qkiHA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679650663;\n\tbh=fr96Qpz1SF4vSzSDgGZidtEmYV2BAanZ1Lw8T3DwghA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a4W8tumHxNH9i63uiyYJf2h8FB0K+hlh9WqPgZMxGFD8as6ehBRg5AnqNurtCjUex\n\t+89+CZPTux2HlErdgTmxgswkF6B5xYQPxpkFh7hD+tjlNHT8NOaYMLA+ibJhGD2ypj\n\tw4lcmwHgKJRQJib7T1+uzZ6KAczkf5YiWor2Eolk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"a4W8tumH\"; dkim-atps=neutral","Date":"Fri, 24 Mar 2023 10:37:40 +0100","To":"Naushir Patuck ","Message-ID":"<20230324093740.cnahlf6dhgcgbgsv@uno.localdomain>","References":"<20230322130612.5208-1-naush@raspberrypi.com>\n\t<20230322130612.5208-10-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230322130612.5208-10-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 09/10] ipa: raspberrypi: Generalise\n\tthe agc algorithm","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\" "}}]