[{"id":28697,"web_url":"https://patchwork.libcamera.org/comment/28697/","msgid":"","date":"2024-02-20T09:50:22","subject":"Re: [PATCH v3 1/5] ipa: rkisp1: agc: Wrap variable length C arrays\n\tin spans","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Sun, Feb 18, 2024 at 06:49:04PM +0200, Laurent Pinchart wrote:\n> The RkISP1 statistics structure contains multiple arrays whose length\n> varies depending on the hardware revision. Accessing those arrays is\n> error-prone, wrap them in spans at the top level to reduce risks of\n> out-of-bound accesses.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++++++--------------\n> src/ipa/rkisp1/algorithms/agc.h | 5 +++--\n> 2 files changed, 16 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index e5aeb3426eff..f83a9ba1686a 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -186,8 +186,8 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t/* Produce the luminance histogram. */\n> \tparams->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> \t/* Set an average weighted histogram. */\n> -\tfor (unsigned int histBin = 0; histBin < numHistBins_; histBin++)\n> -\t\tparams->meas.hst_config.hist_weight[histBin] = 1;\n> +\tSpan weights{ params->meas.hst_config.hist_weight, numHistBins_ };\n> +\tstd::fill(weights.begin(), weights.end(), 1);\n> \t/* Step size can't be less than 3. */\n> \tparams->meas.hst_config.histogram_predivider = 4;\n> \n> @@ -318,7 +318,7 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \n> /**\n> * \\brief Estimate the relative luminance of the frame with a given gain\n> - * \\param[in] ae The RkISP1 statistics and ISP results\n> + * \\param[in] expMeans The mean luminance values, from the RkISP1 statistics\n> * \\param[in] gain The gain to apply to the frame\n> *\n> * This function estimates the average relative luminance of the frame that\n> @@ -342,28 +342,27 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> *\n> * \\return The relative luminance\n> */\n> -double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,\n> -\t\t\t double gain)\n> +double Agc::estimateLuminance(Span expMeans, double gain)\n> {\n> \tdouble ySum = 0.0;\n> \n> \t/* Sum the averages, saturated to 255. */\n> -\tfor (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)\n> -\t\tySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);\n> +\tfor (uint8_t expMean : expMeans)\n> +\t\tySum += std::min(expMean * gain, 255.0);\n> \n> \t/* \\todo Weight with the AWB gains */\n> \n> -\treturn ySum / numCells_ / 255;\n> +\treturn ySum / expMeans.size() / 255;\n> }\n> \n> /**\n> * \\brief Estimate the mean value of the top 2% of the histogram\n> - * \\param[in] hist The histogram statistics computed by the ImgU\n> + * \\param[in] hist The histogram statistics computed by the RkISP1\n> * \\return The mean value of the top 2% of the histogram\n> */\n> -double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> +double Agc::measureBrightness(Span hist) const\n> {\n> -\tHistogram histogram{ Span(hist->hist_bins, numHistBins_) };\n> +\tHistogram histogram{ hist };\n> \t/* Estimate the quantile mean of the top 2% of the histogram. */\n> \treturn histogram.interQuantileMean(0.98, 1.0);\n> }\n> @@ -415,11 +414,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tconst rkisp1_cif_isp_stat *params = &stats->params;\n> \tASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);\n> \n> -\tconst rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> -\tconst rkisp1_cif_isp_hist_stat *hist = ¶ms->hist;\n> +\tSpan ae{ params->ae.exp_mean, numCells_ };\n> +\tSpan hist{ params->hist.hist_bins, numHistBins_ };\n> \n> \tdouble iqMean = measureBrightness(hist);\n> -\tdouble iqMeanGain = kEvGainTarget * numHistBins_ / iqMean;\n> +\tdouble iqMeanGain = kEvGainTarget * hist.size() / iqMean;\n> \n> \t/*\n> \t * Estimate the gain needed to achieve a relative luminance target. To\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index 8a22263741b6..ce8594f393ab 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -42,8 +43,8 @@ private:\n> \tvoid computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> \t\t\t double yGain, double iqMeanGain);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> -\tdouble estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> -\tdouble measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> +\tdouble estimateLuminance(Span expMeans, double gain);\n> +\tdouble measureBrightness(Span hist) const;\n> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t ControlList &metadata);\n> \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 03B62BF415\n\tfor ;\n\tTue, 20 Feb 2024 09:50:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5835762813;\n\tTue, 20 Feb 2024 10:50:31 +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 C605762801\n\tfor ;\n\tTue, 20 Feb 2024 10:50:29 +0100 (CET)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AA66D13AC;\n\tTue, 20 Feb 2024 10:50:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Wz/zLNyK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708422622;\n\tbh=8E1AOUQGweixi8r/pcA7TOJuh6tJUMNJGqV82luG/lE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wz/zLNyKTBw7355XmwP5/iP3F7BGlCaERTKwa/znW72QyCGwJTZUi/3pSi1Jy4hTj\n\tyecz+kCn5FWzftLWGucYxL3sFopL9o1lI7cd5XdRfrU4HkZyT5MXeOLRVoc6whoxBz\n\tgLUjMa2Sm50aO6hOVVSU3hNa7Okgf9i/up3B12M4=","Date":"Tue, 20 Feb 2024 18:50:22 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Subject":"Re: [PATCH v3 1/5] ipa: rkisp1: agc: Wrap variable length C arrays\n\tin spans","Message-ID":"","References":"<20240218164908.15921-1-laurent.pinchart@ideasonboard.com>\n\t<20240218164908.15921-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240218164908.15921-2-laurent.pinchart@ideasonboard.com>","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28717,"web_url":"https://patchwork.libcamera.org/comment/28717/","msgid":"<9999a28f-4c3f-4db1-8c37-2b0204b3e227@ideasonboard.com>","date":"2024-02-23T12:53:20","subject":"Re: [PATCH v3 1/5] ipa: rkisp1: agc: Wrap variable length C arrays\n\tin spans","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Am 18.02.24 um 17:49 schrieb Laurent Pinchart:\n> The RkISP1 statistics structure contains multiple arrays whose length\n> varies depending on the hardware revision. Accessing those arrays is\n> error-prone, wrap them in spans at the top level to reduce risks of\n> out-of-bound accesses.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Stefan Klug \n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++++++--------------\n> src/ipa/rkisp1/algorithms/agc.h | 5 +++--\n> 2 files changed, 16 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index e5aeb3426eff..f83a9ba1686a 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -186,8 +186,8 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t/* Produce the luminance histogram. */\n> \tparams->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> \t/* Set an average weighted histogram. */\n> -\tfor (unsigned int histBin = 0; histBin < numHistBins_; histBin++)\n> -\t\tparams->meas.hst_config.hist_weight[histBin] = 1;\n> +\tSpan weights{ params->meas.hst_config.hist_weight, numHistBins_ };\n> +\tstd::fill(weights.begin(), weights.end(), 1);\n> \t/* Step size can't be less than 3. */\n> \tparams->meas.hst_config.histogram_predivider = 4;\n> \n> @@ -318,7 +318,7 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \n> /**\n> * \\brief Estimate the relative luminance of the frame with a given gain\n> - * \\param[in] ae The RkISP1 statistics and ISP results\n> + * \\param[in] expMeans The mean luminance values, from the RkISP1 statistics\n> * \\param[in] gain The gain to apply to the frame\n> *\n> * This function estimates the average relative luminance of the frame that\n> @@ -342,28 +342,27 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> *\n> * \\return The relative luminance\n> */\n> -double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,\n> -\t\t\t double gain)\n> +double Agc::estimateLuminance(Span expMeans, double gain)\n> {\n> \tdouble ySum = 0.0;\n> \n> \t/* Sum the averages, saturated to 255. */\n> -\tfor (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)\n> -\t\tySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);\n> +\tfor (uint8_t expMean : expMeans)\n> +\t\tySum += std::min(expMean * gain, 255.0);\n> \n> \t/* \\todo Weight with the AWB gains */\n> \n> -\treturn ySum / numCells_ / 255;\n> +\treturn ySum / expMeans.size() / 255;\n> }\n> \n> /**\n> * \\brief Estimate the mean value of the top 2% of the histogram\n> - * \\param[in] hist The histogram statistics computed by the ImgU\n> + * \\param[in] hist The histogram statistics computed by the RkISP1\n> * \\return The mean value of the top 2% of the histogram\n> */\n> -double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> +double Agc::measureBrightness(Span hist) const\n> {\n> -\tHistogram histogram{ Span(hist->hist_bins, numHistBins_) };\n> +\tHistogram histogram{ hist };\n> \t/* Estimate the quantile mean of the top 2% of the histogram. */\n> \treturn histogram.interQuantileMean(0.98, 1.0);\n> }\n> @@ -415,11 +414,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tconst rkisp1_cif_isp_stat *params = &stats->params;\n> \tASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);\n> \n> -\tconst rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> -\tconst rkisp1_cif_isp_hist_stat *hist = ¶ms->hist;\n> +\tSpan ae{ params->ae.exp_mean, numCells_ };\n> +\tSpan hist{ params->hist.hist_bins, numHistBins_ };\n> \n> \tdouble iqMean = measureBrightness(hist);\n> -\tdouble iqMeanGain = kEvGainTarget * numHistBins_ / iqMean;\n> +\tdouble iqMeanGain = kEvGainTarget * hist.size() / iqMean;\n> \n> \t/*\n> \t * Estimate the gain needed to achieve a relative luminance target. To\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index 8a22263741b6..ce8594f393ab 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -42,8 +43,8 @@ private:\n> \tvoid computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> \t\t\t double yGain, double iqMeanGain);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> -\tdouble estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> -\tdouble measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> +\tdouble estimateLuminance(Span expMeans, double gain);\n> +\tdouble measureBrightness(Span hist) const;\n> \tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t ControlList &metadata);\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 A1700C0F1B\n\tfor ;\n\tFri, 23 Feb 2024 12:53:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03B1762809;\n\tFri, 23 Feb 2024 13:53:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3552D61CA1\n\tfor ;\n\tFri, 23 Feb 2024 13:53:23 +0100 (CET)","from [IPV6:2a00:6020:448c:6c00:163d:481a:d9f4:bc3d] (unknown\n\t[IPv6:2a00:6020:448c:6c00:163d:481a:d9f4:bc3d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C628B9B6;\n\tFri, 23 Feb 2024 13:53:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pq8o26DK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708692793;\n\tbh=s9xrCug1Fs1rImFtZ5qpX07sI//80T3ZbEhwiIXsETo=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=pq8o26DKWBl3JOmitlHeMV9sQo54nQl+N8zKBCnE+jyws2VN3yn8KRLuKZkjBa+Ty\n\tuZCLMOwtqNdMDVkOzpU3GQ3w/OBtbBrB0l3wCC9plEI5qRiwWKZY6A4mEGTE27OEmo\n\tMJrT2Dy7N2cI6n05DUr9tGvwaxlJeDAze/nbFdio=","Message-ID":"<9999a28f-4c3f-4db1-8c37-2b0204b3e227@ideasonboard.com>","Date":"Fri, 23 Feb 2024 13:53:20 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/5] ipa: rkisp1: agc: Wrap variable length C arrays\n\tin spans","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240218164908.15921-1-laurent.pinchart@ideasonboard.com>\n\t<20240218164908.15921-2-laurent.pinchart@ideasonboard.com>","From":"Stefan Klug ","In-Reply-To":"<20240218164908.15921-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]