[{"id":19688,"web_url":"https://patchwork.libcamera.org/comment/19688/","msgid":"","date":"2021-09-14T22:21:08","subject":"Re: [libcamera-devel] [PATCH v8 4/5] ipa: ipu3: Make the naming\n\tconsistent","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Tue, Sep 14, 2021 at 06:37:08PM +0200, Jean-Michel Hautbois wrote:\n> The variables mix the terms cell, region and zone. It can confuse the\n> reader, and make the algorithm more difficult to follow. Rename the\n> local variables consistent with their definitions:\n> - Cells are defined in Pixels\n> - Zones are defined in Cells\n> There is no \"region\" as such, so replace it with the correct term.\n\nThis looks good for the AWB side, which should be in its own patch. The\nAGC bug fix should be separate, and have a Fixes: line.\n\n> While reading again the code for this renaming, the\n> Agc::processBrightness() function was really too difficult to follow,\n> and mixed pixels, cells, region of interest. The history is that AGC was\n> meant to be done in one particular region of the input frame (the\n> center, or anything else) but there is no need for that, and there is no\n> easy way to define this region of interest right now. It will be better\n> to introduce metering instead.\n> \n> While we are renaming and making things more consistent, let's improve\n> Agc at the same time:\n> - Remove the aeRegion reference, as start_x and y_start are always 0\n> - Use only the grid passed to the function, and not the one in the stats\n> - rename i and j accordingly, to follow the Awb way\n> - remove the unused variables (count, kCellSize, etc.)\n\nThe most important part of this patch is that it fixes an actual bug in\nthe AGC code. What the bug is is supposed to be explained in the commit\nmessage.\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 44 +++++++++------------------------\n> src/ipa/ipu3/algorithms/awb.cpp | 26 +++++++++----------\n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 5ff50f4a..52e8015e 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 \n> #include \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> @@ -70,37 +68,19 @@ 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\nWhy was there a copy ? :-(\n\n> -\tRectangle aeRegion = { statsAeGrid.x_start,\n> -\t\t\t statsAeGrid.y_start,\n> -\t\t\t static_cast(statsAeGrid.x_end - statsAeGrid.x_start) + 1,\n> -\t\t\t static_cast(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> +\n> +\tfor (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n> +\t\tfor (unsigned int cellX = 0; cellX < grid.width; cellX++) {\n> +\t\t\tuint32_t cellPosition = (cellY * grid.width + 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\tIpu3AwbCell *currentCell = reinterpret_cast(const_cast(&stats->awb_raw_buffer.meta_data[cellPosition]));\n\nYuck. Why do you need a const_cast ? Is there anything wrong with\n\n\t\t\tconst Ipu3AwbCell *cell =\n\t\t\t\treinterpret_cast(\n\t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n\t\t\t\t);\n\n?\n\nI know the awb code does the same below, which looks equally bad to me.\n\nI'd go one step further.\n\n\tuint32_t hist[knumHistogramBins] = { 0 };\n\n\tfor (unsigned int i = 0; i < grid.width * grid.height; ++i) {\n\t\tconst Ipu3AwbCell *cell =\n\t\t\treinterpret_cast(\n\t\t\t\t&stats->awb_raw_buffer.meta_data[i * sizeof(Ipu3AwbCell)]\n\t\t\t);\n\n\t\tif (cell->satRatio == 0) {\n\t\t\tuint8_t gr = cell->greenRedAvg;\n\t\t\tuint8_t gb = cell->greenBlueAvg;\n\t\t\thist[(gr + gb) / 2]++;\n\t\t}\n\t}\n\nHere's a possible commit message:\n\nipa: ipu3: agc: Fix invalid access to statistics cells\n\nThe code in Agc::processBrightness() that iterates over the statistics\ncells is difficult to follow as it mixes pixels, cells, region of\ninterest. This has resulted in the index in the stats cells data being\ncalculated incorrectly based on pixel coordinates instead of cell\ncoordinates. The statistics are thus not read correctly.\n\nWe could clean up the code by using more descriptive variable names and\nfix the issue, but this was designed with the assumption that the\nalgorithm would only operate on a particular region of the input frame,\nand that assumption doesn't hold anymore. Let's simplify the code\ninstead, looping over all cells, and fixing the bug.\n\n> +\t\t\tif (currentCell->satRatio == 0) {\n> +\t\t\t\tuint8_t Gr = currentCell->greenRedAvg;\n> +\t\t\t\tuint8_t Gb = currentCell->greenBlueAvg;\n> \t\t\t\thist[(Gr + Gb) / 2]++;\n> -\t\t\t\tcount++;\n> \t\t\t}\n> \t\t}\n> \t}\n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index d78ae4f4..b96a6ebf 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -220,31 +220,31 @@ void Awb::generateZones(std::vector &zones)\n> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid)\n> {\n> -\tuint32_t regionWidth = round(grid.width / static_cast(kAwbStatsSizeX));\n> -\tuint32_t regionHeight = round(grid.height / static_cast(kAwbStatsSizeY));\n> +\tuint32_t cellsPerZoneX = round(grid.width / static_cast(kAwbStatsSizeX));\n> +\tuint32_t cellsPerZoneY = round(grid.height / static_cast(kAwbStatsSizeY));\n> \n> \t/*\n> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n> \t * (grid.width x grid.height).\n> \t */\n> -\tfor (unsigned int j = 0; j < kAwbStatsSizeY * regionHeight; j++) {\n> -\t\tfor (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {\n> -\t\t\tuint32_t cellPosition = j * grid.width + i;\n> -\t\t\tuint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;\n> -\t\t\tuint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;\n> +\tfor (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) {\n> +\t\tfor (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) {\n> +\t\t\tuint32_t cellPosition = (cellY * grid.width + cellX)\n> +\t\t\t\t\t * sizeof(Ipu3AwbCell);\n> +\t\t\tuint32_t zoneX = cellX / cellsPerZoneX;\n> +\t\t\tuint32_t zoneY = cellY / cellsPerZoneY;\n> \n> -\t\t\tuint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;\n> -\t\t\tcellPosition *= 8;\n> +\t\t\tuint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;\n> \n> \t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n> \t\t\tIpu3AwbCell *currentCell = reinterpret_cast(const_cast(&stats->awb_raw_buffer.meta_data[cellPosition]));\n> \t\t\tif (currentCell->satRatio == 0) {\n> \t\t\t\t/* The cell is not saturated, use the current cell */\n> -\t\t\t\tawbStats_[awbRegionPosition].counted++;\n> +\t\t\t\tawbStats_[awbZonePosition].counted++;\n> \t\t\t\tuint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;\n> -\t\t\t\tawbStats_[awbRegionPosition].sum.green += greenValue / 2;\n> -\t\t\t\tawbStats_[awbRegionPosition].sum.red += currentCell->redAvg;\n> -\t\t\t\tawbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;\n> +\t\t\t\tawbStats_[awbZonePosition].sum.green += greenValue / 2;\n> +\t\t\t\tawbStats_[awbZonePosition].sum.red += currentCell->redAvg;\n> +\t\t\t\tawbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;\n> \t\t\t}\n> \t\t}\n> \t}","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 69AD8BDB1D\n\tfor ;\n\tTue, 14 Sep 2021 22:21:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 959D76024C;\n\tWed, 15 Sep 2021 00:21:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70CFD60248\n\tfor ;\n\tWed, 15 Sep 2021 00:21:33 +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 DCC572A5;\n\tWed, 15 Sep 2021 00:21:32 +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=\"H62Ao801\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631658093;\n\tbh=VW55dQiwt9O8J4DUYO2wLC5dumQAKYSfGwSdMBdQypU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H62Ao801v4VAICNtSwOyAe5bfonST0ZQAjll700Lztsgiun49Lq2onoCcby3x7feW\n\t1B41qZh+JOx9mXJb1yNisEY942ARU+tAry7TVTOn6+S/lp7+1EaH9HYC+o0Z+4+Q6p\n\tuzKVESgE7Efki5W6aPjrp4es2gWifxh0X61ULKHY=","Date":"Wed, 15 Sep 2021 01:21:08 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210914163709.85751-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210914163709.85751-5-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210914163709.85751-5-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v8 4/5] ipa: ipu3: Make the naming\n\tconsistent","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\" "}}]