[{"id":19938,"web_url":"https://patchwork.libcamera.org/comment/19938/","msgid":"<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","date":"2021-09-28T16:26:01","subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:\n> Now that we know how exactly the AWB statistics are formatted, use a\n> simplified loop in processBrightness() to parse the green values and get the\n> histogram.\n> \n> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> ---\n>  src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------\n>  src/ipa/ipu3/algorithms/agc.h   |  2 ++\n>  2 files changed, 23 insertions(+), 33 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 5ff50f4a..ebbc5676 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -6,6 +6,7 @@\n>   */\n>  \n>  #include \"agc.h\"\n> +#include \"awb.h\"\n>  \n>  #include <algorithm>\n>  #include <chrono>\n> @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;\n>  static constexpr uint32_t knumHistogramBins = 256;\n>  static constexpr double kEvGainTarget = 0.5;\n>  \n> -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */\n> -static constexpr uint8_t kCellSize = 8;\n> -\n>  Agc::Agc()\n>  \t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>  \t  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),\n> @@ -60,6 +58,10 @@ Agc::Agc()\n>  int Agc::configure([[maybe_unused]] IPAContext &context,\n\nlooks like context is no longer [[maybe_unused]].\n\n\n>  \t\t   const IPAConfigInfo &configInfo)\n>  {\n> +\tconst ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;\n> +\t/* The grid is aligned to the next multiple of 4 */\n> +\tstride_ = (grid.width + 3) / 4 * 4;\n\nThis is familiar...\n\nHow about putting a stride into context.configuration.grid.stride and\ncalculating it once during calculateBdsGrid()?\n\n> +\n>  \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n>  \t\t      / configInfo.sensorInfo.pixelRate;\n>  \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> @@ -70,37 +72,23 @@ int Agc::configure([[maybe_unused]] IPAContext &context,\n>  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>  \t\t\t    const ipu3_uapi_grid_config &grid)\n>  {\n> -\tconst struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;\n> -\tRectangle aeRegion = { statsAeGrid.x_start,\n> -\t\t\t       statsAeGrid.y_start,\n> -\t\t\t       static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,\n> -\t\t\t       static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };\n> -\tPoint topleft = aeRegion.topLeft();\n> -\tint topleftX = topleft.x >> grid.block_width_log2;\n> -\tint topleftY = topleft.y >> grid.block_height_log2;\n> -\n> -\t/* Align to the grid cell width and height */\n> -\tuint32_t startX = topleftX << grid.block_width_log2;\n> -\tuint32_t startY = topleftY * grid.width << grid.block_width_log2;\n> -\tuint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;\n> -\tuint32_t i, j;\n> -\tuint32_t count = 0;\n> -\n>  \tuint32_t hist[knumHistogramBins] = { 0 };\n> -\tfor (j = topleftY;\n> -\t     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);\n> -\t     j++) {\n> -\t\tfor (i = startX + startY; i < endX + startY; i += kCellSize) {\n> -\t\t\t/*\n> -\t\t\t * The grid width (and maybe height) is not reliable.\n> -\t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n> -\t\t\t * Use the one passed at init time.\n> -\t\t\t */\n> -\t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n> -\t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n> -\t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\n> -\t\t\t\thist[(Gr + Gb) / 2]++;\n> -\t\t\t\tcount++;\n> +\n> +\tfor (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n> +\t\tfor (unsigned int cellX = 0; cellX < stride_; cellX++) {\n> +\t\t\tuint32_t cellPosition = cellY * stride_ + cellX\n> +\t\t\t\t\t      * sizeof(Ipu3AwbCell);\n> +\n> +\t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n> +\t\t\tconst Ipu3AwbCell *cell =\n> +\t\t\t\treinterpret_cast<const Ipu3AwbCell *>(\n> +\t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n> +\t\t\t\t);\n> +\n> +\t\t\tif (cell->satRatio == 0) {\n\nIs the satRatio usage clear yet? Is it always 0? or do you see other\nvalues here?\n\nShould it be less than a saturation threshold? (perhaps not unless we\nhave a way to configure that anyway)\n\n\n> +\t\t\t\tuint8_t gr = cell->greenRedAvg;\n> +\t\t\t\tuint8_t gb = cell->greenBlueAvg;\n> +\t\t\t\thist[(gr + gb) / 2]++;\n>  \t\t\t}\n>  \t\t}\n>  \t}\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index e36797d3..64b71c65 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -50,6 +50,8 @@ private:\n>  \tDuration prevExposureNoDg_;\n>  \tDuration currentExposure_;\n>  \tDuration currentExposureNoDg_;\n> +\n> +\tuint32_t stride_;\n>  };\n>  \n>  } /* namespace ipa::ipu3::algorithms */\n> -- \n> 2.30.2\n> \n\n--\nKieran","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 9AFBAC3243\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 28 Sep 2021 16:26:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D49869195;\n\tTue, 28 Sep 2021 18:26:05 +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 AF26269188\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 28 Sep 2021 18:26:03 +0200 (CEST)","from 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 56B563F6;\n\tTue, 28 Sep 2021 18:26:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"whxgb6NP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632846363;\n\tbh=fy8C2NDq8T09QfgdULTsz+YNRyjm+zwUvyTBhCO7o1U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=whxgb6NPz/GQZ5yPRdU8euPfve6+lcPUutnCqTJVAM3sU4/BRLKNMzEdYfKJOlOY8\n\tsmmbZB59hwLiVh8MZHg1f/teNxmSktAoYqDyVvibMNGk728HNBvW1GV+FntAsqav7Z\n\tf7DQ+JsplvH/BG874XCo9gKXqnlh47V/m00aTxdE=","Date":"Tue, 28 Sep 2021 17:26:01 +0100","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-12-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210923081625.60276-12-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":19976,"web_url":"https://patchwork.libcamera.org/comment/19976/","msgid":"<c0f3b79b-f416-6539-aea6-a2243e785ff6@ideasonboard.com>","date":"2021-09-29T09:02:11","subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 28/09/2021 18:26, Kieran Bingham wrote:\n> On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:\n>> Now that we know how exactly the AWB statistics are formatted, use a\n>> simplified loop in processBrightness() to parse the green values and get the\n>> histogram.\n>>\n>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>> ---\n>>  src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------\n>>  src/ipa/ipu3/algorithms/agc.h   |  2 ++\n>>  2 files changed, 23 insertions(+), 33 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 5ff50f4a..ebbc5676 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -6,6 +6,7 @@\n>>   */\n>>  \n>>  #include \"agc.h\"\n>> +#include \"awb.h\"\n>>  \n>>  #include <algorithm>\n>>  #include <chrono>\n>> @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;\n>>  static constexpr uint32_t knumHistogramBins = 256;\n>>  static constexpr double kEvGainTarget = 0.5;\n>>  \n>> -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */\n>> -static constexpr uint8_t kCellSize = 8;\n>> -\n>>  Agc::Agc()\n>>  \t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>>  \t  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),\n>> @@ -60,6 +58,10 @@ Agc::Agc()\n>>  int Agc::configure([[maybe_unused]] IPAContext &context,\n> \n> looks like context is no longer [[maybe_unused]].\n> \n> \n>>  \t\t   const IPAConfigInfo &configInfo)\n>>  {\n>> +\tconst ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;\n>> +\t/* The grid is aligned to the next multiple of 4 */\n>> +\tstride_ = (grid.width + 3) / 4 * 4;\n> \n> This is familiar...\n> \n> How about putting a stride into context.configuration.grid.stride and\n> calculating it once during calculateBdsGrid()?\n> \n\nThen I need to pass the grid to the processBrightness(), again (and in\nAWB too) while we could get rid of it. Or cache the value in configure() ?\n\n>> +\n>>  \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n>>  \t\t      / configInfo.sensorInfo.pixelRate;\n>>  \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n>> @@ -70,37 +72,23 @@ int Agc::configure([[maybe_unused]] IPAContext &context,\n>>  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>>  \t\t\t    const ipu3_uapi_grid_config &grid)\n>>  {\n>> -\tconst struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;\n>> -\tRectangle aeRegion = { statsAeGrid.x_start,\n>> -\t\t\t       statsAeGrid.y_start,\n>> -\t\t\t       static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,\n>> -\t\t\t       static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };\n>> -\tPoint topleft = aeRegion.topLeft();\n>> -\tint topleftX = topleft.x >> grid.block_width_log2;\n>> -\tint topleftY = topleft.y >> grid.block_height_log2;\n>> -\n>> -\t/* Align to the grid cell width and height */\n>> -\tuint32_t startX = topleftX << grid.block_width_log2;\n>> -\tuint32_t startY = topleftY * grid.width << grid.block_width_log2;\n>> -\tuint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;\n>> -\tuint32_t i, j;\n>> -\tuint32_t count = 0;\n>> -\n>>  \tuint32_t hist[knumHistogramBins] = { 0 };\n>> -\tfor (j = topleftY;\n>> -\t     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);\n>> -\t     j++) {\n>> -\t\tfor (i = startX + startY; i < endX + startY; i += kCellSize) {\n>> -\t\t\t/*\n>> -\t\t\t * The grid width (and maybe height) is not reliable.\n>> -\t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n>> -\t\t\t * Use the one passed at init time.\n>> -\t\t\t */\n>> -\t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n>> -\t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n>> -\t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\n>> -\t\t\t\thist[(Gr + Gb) / 2]++;\n>> -\t\t\t\tcount++;\n>> +\n>> +\tfor (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n>> +\t\tfor (unsigned int cellX = 0; cellX < stride_; cellX++) {\n>> +\t\t\tuint32_t cellPosition = cellY * stride_ + cellX\n>> +\t\t\t\t\t      * sizeof(Ipu3AwbCell);\n>> +\n>> +\t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n>> +\t\t\tconst Ipu3AwbCell *cell =\n>> +\t\t\t\treinterpret_cast<const Ipu3AwbCell *>(\n>> +\t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n>> +\t\t\t\t);\n>> +\n>> +\t\t\tif (cell->satRatio == 0) {\n> \n> Is the satRatio usage clear yet? Is it always 0? or do you see other\n> values here?\n> \n> Should it be less than a saturation threshold? (perhaps not unless we\n> have a way to configure that anyway)\n> \n> \n>> +\t\t\t\tuint8_t gr = cell->greenRedAvg;\n>> +\t\t\t\tuint8_t gb = cell->greenBlueAvg;\n>> +\t\t\t\thist[(gr + gb) / 2]++;\n>>  \t\t\t}\n>>  \t\t}\n>>  \t}\n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index e36797d3..64b71c65 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -50,6 +50,8 @@ private:\n>>  \tDuration prevExposureNoDg_;\n>>  \tDuration currentExposure_;\n>>  \tDuration currentExposureNoDg_;\n>> +\n>> +\tuint32_t stride_;\n>>  };\n>>  \n>>  } /* namespace ipa::ipu3::algorithms */\n>> -- \n>> 2.30.2\n>>\n> \n> --\n> Kieran\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 92898BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 29 Sep 2021 09:02:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5BFC1691AE;\n\tWed, 29 Sep 2021 11:02:16 +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 A78E26919D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 Sep 2021 11:02:14 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:b4ce:851b:eca:f3fe])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 063263F0;\n\tWed, 29 Sep 2021 11:02:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UGpiXjbZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632906134;\n\tbh=bKy9GqqIwxhSmx/kJaZiHheGjg4lACgoZCz5L0czOm4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=UGpiXjbZYMJ1IX5yW8MqFrKEhvARl+nIPwMNPUeqMo4LSY+W24DEnkG79sE2dWk66\n\tqIatltNIsQxAqBoFT41yuB9PP54C4kXV6Y0LrJM/cue3RPhcBVlDJY4T3tJ29Vvi/O\n\tuUdrWzI+Rsx90CnaRmZPwxrczxHUOxpjWx4HhQ9k=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-12-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<c0f3b79b-f416-6539-aea6-a2243e785ff6@ideasonboard.com>","Date":"Wed, 29 Sep 2021 11:02:11 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":19985,"web_url":"https://patchwork.libcamera.org/comment/19985/","msgid":"<5ff757cc-9d0a-7525-4386-d70b7c61eda8@ideasonboard.com>","date":"2021-09-29T11:15:04","subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 29/09/2021 10:02, Jean-Michel Hautbois wrote:\n> On 28/09/2021 18:26, Kieran Bingham wrote:\n>> On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:\n>>> Now that we know how exactly the AWB statistics are formatted, use a\n>>> simplified loop in processBrightness() to parse the green values and get the\n>>> histogram.\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>>> ---\n>>>  src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------\n>>>  src/ipa/ipu3/algorithms/agc.h   |  2 ++\n>>>  2 files changed, 23 insertions(+), 33 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>>> index 5ff50f4a..ebbc5676 100644\n>>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>>> @@ -6,6 +6,7 @@\n>>>   */\n>>>  \n>>>  #include \"agc.h\"\n>>> +#include \"awb.h\"\n>>>  \n>>>  #include <algorithm>\n>>>  #include <chrono>\n>>> @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;\n>>>  static constexpr uint32_t knumHistogramBins = 256;\n>>>  static constexpr double kEvGainTarget = 0.5;\n>>>  \n>>> -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */\n>>> -static constexpr uint8_t kCellSize = 8;\n>>> -\n>>>  Agc::Agc()\n>>>  \t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>>>  \t  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),\n>>> @@ -60,6 +58,10 @@ Agc::Agc()\n>>>  int Agc::configure([[maybe_unused]] IPAContext &context,\n>>\n>> looks like context is no longer [[maybe_unused]].\n>>\n>>\n>>>  \t\t   const IPAConfigInfo &configInfo)\n>>>  {\n>>> +\tconst ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;\n>>> +\t/* The grid is aligned to the next multiple of 4 */\n>>> +\tstride_ = (grid.width + 3) / 4 * 4;\n>>\n>> This is familiar...\n>>\n>> How about putting a stride into context.configuration.grid.stride and\n>> calculating it once during calculateBdsGrid()?\n>>\n> \n> Then I need to pass the grid to the processBrightness(), again (and in\n> AWB too) while we could get rid of it. Or cache the value in configure() ?\n\nI think it's fine, and better to cache it in configure.\n\n--\nKieran","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 3F313BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 29 Sep 2021 11:15:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6C1B691AA;\n\tWed, 29 Sep 2021 13:15:09 +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 BFFA669185\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 Sep 2021 13:15:07 +0200 (CEST)","from [192.168.0.20]\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 3FA643F0;\n\tWed, 29 Sep 2021 13:15:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Coibti98\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632914107;\n\tbh=Ouwo1CuHcX7S74lZHEZHryr9xhWz8pgu7LiA4r3+hBs=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=Coibti981hlZiGlwQ1YAjYhS9roSQ1Be5ah5naid1EOpwiQKPM6azfphTh1jbeaTs\n\tupnE1MnIvWE72k4YlfJI+Vtb2Qc3A2QZig67FPUTtY7nfOh9ypcJwxBmvH7p3MXHN9\n\tzWfJzEUPSy1TGqioBhyjRboNSy061mA8KjAOyG9Y=","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-12-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>\n\t<c0f3b79b-f416-6539-aea6-a2243e785ff6@ideasonboard.com>","Message-ID":"<5ff757cc-9d0a-7525-4386-d70b7c61eda8@ideasonboard.com>","Date":"Wed, 29 Sep 2021 12:15:04 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<c0f3b79b-f416-6539-aea6-a2243e785ff6@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20000,"web_url":"https://patchwork.libcamera.org/comment/20000/","msgid":"<YVRiXCbq9wnAqNnE@pendragon.ideasonboard.com>","date":"2021-09-29T12:55:56","subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Sep 28, 2021 at 05:26:01PM +0100, Kieran Bingham wrote:\n> On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:\n> > Now that we know how exactly the AWB statistics are formatted, use a\n> > simplified loop in processBrightness() to parse the green values and get the\n> > histogram.\n> > \n> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> > ---\n> >  src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------\n> >  src/ipa/ipu3/algorithms/agc.h   |  2 ++\n> >  2 files changed, 23 insertions(+), 33 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index 5ff50f4a..ebbc5676 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -6,6 +6,7 @@\n> >   */\n> >  \n> >  #include \"agc.h\"\n> > +#include \"awb.h\"\n> >  \n> >  #include <algorithm>\n> >  #include <chrono>\n> > @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;\n> >  static constexpr uint32_t knumHistogramBins = 256;\n> >  static constexpr double kEvGainTarget = 0.5;\n> >  \n> > -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */\n> > -static constexpr uint8_t kCellSize = 8;\n> > -\n> >  Agc::Agc()\n> >  \t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n> >  \t  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),\n> > @@ -60,6 +58,10 @@ Agc::Agc()\n> >  int Agc::configure([[maybe_unused]] IPAContext &context,\n> \n> looks like context is no longer [[maybe_unused]].\n> \n> >  \t\t   const IPAConfigInfo &configInfo)\n> >  {\n> > +\tconst ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;\n> > +\t/* The grid is aligned to the next multiple of 4 */\n> > +\tstride_ = (grid.width + 3) / 4 * 4;\n> \n> This is familiar...\n\nutils::alignUp() to the rescue again :-)\n\n> How about putting a stride into context.configuration.grid.stride and\n> calculating it once during calculateBdsGrid()?\n> \n> > +\n> >  \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> >  \t\t      / configInfo.sensorInfo.pixelRate;\n> >  \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> > @@ -70,37 +72,23 @@ int Agc::configure([[maybe_unused]] IPAContext &context,\n> >  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> >  \t\t\t    const ipu3_uapi_grid_config &grid)\n> >  {\n> > -\tconst struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;\n> > -\tRectangle aeRegion = { statsAeGrid.x_start,\n> > -\t\t\t       statsAeGrid.y_start,\n> > -\t\t\t       static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,\n> > -\t\t\t       static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };\n> > -\tPoint topleft = aeRegion.topLeft();\n> > -\tint topleftX = topleft.x >> grid.block_width_log2;\n> > -\tint topleftY = topleft.y >> grid.block_height_log2;\n> > -\n> > -\t/* Align to the grid cell width and height */\n> > -\tuint32_t startX = topleftX << grid.block_width_log2;\n> > -\tuint32_t startY = topleftY * grid.width << grid.block_width_log2;\n> > -\tuint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;\n> > -\tuint32_t i, j;\n> > -\tuint32_t count = 0;\n> > -\n> >  \tuint32_t hist[knumHistogramBins] = { 0 };\n> > -\tfor (j = topleftY;\n> > -\t     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);\n> > -\t     j++) {\n> > -\t\tfor (i = startX + startY; i < endX + startY; i += kCellSize) {\n> > -\t\t\t/*\n> > -\t\t\t * The grid width (and maybe height) is not reliable.\n> > -\t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n> > -\t\t\t * Use the one passed at init time.\n> > -\t\t\t */\n> > -\t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n> > -\t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n> > -\t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\n> > -\t\t\t\thist[(Gr + Gb) / 2]++;\n> > -\t\t\t\tcount++;\n> > +\n> > +\tfor (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n> > +\t\tfor (unsigned int cellX = 0; cellX < stride_; cellX++) {\n\nSame issue as in AWB, you'll use the padding cells in your calculations,\nwhile they should be ignored.\n\n> > +\t\t\tuint32_t cellPosition = cellY * stride_ + cellX\n> > +\t\t\t\t\t      * sizeof(Ipu3AwbCell);\n> > +\n> > +\t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n> > +\t\t\tconst Ipu3AwbCell *cell =\n> > +\t\t\t\treinterpret_cast<const Ipu3AwbCell *>(\n> > +\t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n> > +\t\t\t\t);\n> > +\n> > +\t\t\tif (cell->satRatio == 0) {\n> \n> Is the satRatio usage clear yet? Is it always 0? or do you see other\n> values here?\n> \n> Should it be less than a saturation threshold? (perhaps not unless we\n> have a way to configure that anyway)\n> \n> > +\t\t\t\tuint8_t gr = cell->greenRedAvg;\n> > +\t\t\t\tuint8_t gb = cell->greenBlueAvg;\n> > +\t\t\t\thist[(gr + gb) / 2]++;\n> >  \t\t\t}\n> >  \t\t}\n> >  \t}\n> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> > index e36797d3..64b71c65 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.h\n> > +++ b/src/ipa/ipu3/algorithms/agc.h\n> > @@ -50,6 +50,8 @@ private:\n> >  \tDuration prevExposureNoDg_;\n> >  \tDuration currentExposure_;\n> >  \tDuration currentExposureNoDg_;\n> > +\n> > +\tuint32_t stride_;\n> >  };\n> >  \n> >  } /* namespace ipa::ipu3::algorithms */","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 1CAC2C3243\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 29 Sep 2021 12:56:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72057691AA;\n\tWed, 29 Sep 2021 14:56:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F52069185\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 Sep 2021 14:55:58 +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 0F1CC3F0;\n\tWed, 29 Sep 2021 14:55:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"O3fbTWlP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632920158;\n\tbh=wSKGUPYrkpVVO+cK5FjIvvUHQXzY9CjvfOxVQNjG23E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O3fbTWlPISUZ9yxAFXqrDqbCvBHi0gbHbalLXDYIyt2dXFguMTp4LTMCBT93cK2xI\n\thSv1kXOFKcP1bm6iFkWQHzKvHLn+WCNfHX4vhQZZ0yIsuU9hu6SMZGSSk4/lXpUvOs\n\tCyu+iwPKRMgEqr3a6IybmuV+9cW5w3PCi4maLA+s=","Date":"Wed, 29 Sep 2021 15:55:56 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YVRiXCbq9wnAqNnE@pendragon.ideasonboard.com>","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-12-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20012,"web_url":"https://patchwork.libcamera.org/comment/20012/","msgid":"<77f73579-b7c8-7144-4707-073951656bad@ideasonboard.com>","date":"2021-09-30T09:04:28","subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 28/09/2021 18:26, Kieran Bingham wrote:\n> On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:\n>> Now that we know how exactly the AWB statistics are formatted, use a\n>> simplified loop in processBrightness() to parse the green values and get the\n>> histogram.\n>>\n>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>> ---\n>>  src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------\n>>  src/ipa/ipu3/algorithms/agc.h   |  2 ++\n>>  2 files changed, 23 insertions(+), 33 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 5ff50f4a..ebbc5676 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -6,6 +6,7 @@\n>>   */\n>>  \n>>  #include \"agc.h\"\n>> +#include \"awb.h\"\n>>  \n>>  #include <algorithm>\n>>  #include <chrono>\n>> @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;\n>>  static constexpr uint32_t knumHistogramBins = 256;\n>>  static constexpr double kEvGainTarget = 0.5;\n>>  \n>> -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */\n>> -static constexpr uint8_t kCellSize = 8;\n>> -\n>>  Agc::Agc()\n>>  \t: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),\n>>  \t  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),\n>> @@ -60,6 +58,10 @@ Agc::Agc()\n>>  int Agc::configure([[maybe_unused]] IPAContext &context,\n> \n> looks like context is no longer [[maybe_unused]].\n> \n> \n>>  \t\t   const IPAConfigInfo &configInfo)\n>>  {\n>> +\tconst ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;\n>> +\t/* The grid is aligned to the next multiple of 4 */\n>> +\tstride_ = (grid.width + 3) / 4 * 4;\n> \n> This is familiar...\n> \n> How about putting a stride into context.configuration.grid.stride and\n> calculating it once during calculateBdsGrid()?\n> \n>> +\n>>  \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n>>  \t\t      / configInfo.sensorInfo.pixelRate;\n>>  \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n>> @@ -70,37 +72,23 @@ int Agc::configure([[maybe_unused]] IPAContext &context,\n>>  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>>  \t\t\t    const ipu3_uapi_grid_config &grid)\n>>  {\n>> -\tconst struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;\n>> -\tRectangle aeRegion = { statsAeGrid.x_start,\n>> -\t\t\t       statsAeGrid.y_start,\n>> -\t\t\t       static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,\n>> -\t\t\t       static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };\n>> -\tPoint topleft = aeRegion.topLeft();\n>> -\tint topleftX = topleft.x >> grid.block_width_log2;\n>> -\tint topleftY = topleft.y >> grid.block_height_log2;\n>> -\n>> -\t/* Align to the grid cell width and height */\n>> -\tuint32_t startX = topleftX << grid.block_width_log2;\n>> -\tuint32_t startY = topleftY * grid.width << grid.block_width_log2;\n>> -\tuint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;\n>> -\tuint32_t i, j;\n>> -\tuint32_t count = 0;\n>> -\n>>  \tuint32_t hist[knumHistogramBins] = { 0 };\n>> -\tfor (j = topleftY;\n>> -\t     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);\n>> -\t     j++) {\n>> -\t\tfor (i = startX + startY; i < endX + startY; i += kCellSize) {\n>> -\t\t\t/*\n>> -\t\t\t * The grid width (and maybe height) is not reliable.\n>> -\t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n>> -\t\t\t * Use the one passed at init time.\n>> -\t\t\t */\n>> -\t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n>> -\t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n>> -\t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\n>> -\t\t\t\thist[(Gr + Gb) / 2]++;\n>> -\t\t\t\tcount++;\n>> +\n>> +\tfor (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n>> +\t\tfor (unsigned int cellX = 0; cellX < stride_; cellX++) {\n>> +\t\t\tuint32_t cellPosition = cellY * stride_ + cellX\n>> +\t\t\t\t\t      * sizeof(Ipu3AwbCell);\n>> +\n>> +\t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n>> +\t\t\tconst Ipu3AwbCell *cell =\n>> +\t\t\t\treinterpret_cast<const Ipu3AwbCell *>(\n>> +\t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n>> +\t\t\t\t);\n>> +\n>> +\t\t\tif (cell->satRatio == 0) {\n> \n> Is the satRatio usage clear yet? Is it always 0? or do you see other\n> values here?\n> \n> Should it be less than a saturation threshold? (perhaps not unless we\n> have a way to configure that anyway)\n\nI now have a clearer idea on how it works, but it needs to be\nconfigurable indeed.\n\n> \n> \n>> +\t\t\t\tuint8_t gr = cell->greenRedAvg;\n>> +\t\t\t\tuint8_t gb = cell->greenBlueAvg;\n>> +\t\t\t\thist[(gr + gb) / 2]++;\n>>  \t\t\t}\n>>  \t\t}\n>>  \t}\n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index e36797d3..64b71c65 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -50,6 +50,8 @@ private:\n>>  \tDuration prevExposureNoDg_;\n>>  \tDuration currentExposure_;\n>>  \tDuration currentExposureNoDg_;\n>> +\n>> +\tuint32_t stride_;\n>>  };\n>>  \n>>  } /* namespace ipa::ipu3::algorithms */\n>> -- \n>> 2.30.2\n>>\n> \n> --\n> Kieran\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 61754BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 30 Sep 2021 09:04:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2F92691AB;\n\tThu, 30 Sep 2021 11:04:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C3A069189\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 30 Sep 2021 11:04:30 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:bab4:22c5:662d:e478])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A9B6B2B;\n\tThu, 30 Sep 2021 11:04:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j3IvaGO8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632992670;\n\tbh=WQlpw1x6Ky0Y1gKb7nh5nlMyrDcOiXT3OEdwEr6qXJU=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=j3IvaGO8KCzWR9MyQ5iA1ypQ5YXhtrUxsOAxBTqTrShRc7tHp8GWU7Rq05Ekcx/YI\n\tPffjzdVXq2OUXZvs/N4K2XLmsmlTOzn9kP3WXV2Fsvmg+BqRy/O6bGffDvWXBnKrku\n\tlNHd9s0JrSjOacMOMoYwL2axW5dCO+YWipgZ2a/M=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-12-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<77f73579-b7c8-7144-4707-073951656bad@ideasonboard.com>","Date":"Thu, 30 Sep 2021 11:04:28 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<20210928162601.j4hmdubmlcefbaxe@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and\n\tsimplify the brightness loop","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]