[{"id":18162,"web_url":"https://patchwork.libcamera.org/comment/18162/","msgid":"<961dac7f-2b59-860a-5b5e-04077445d02c@ideasonboard.com>","date":"2021-07-13T16:06:38","subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: ipu3: Use metadata and\n\timprove the doc","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n> Using the metadata to exchange the status of the awb algorithm helps to\n> simplify the interface. The structures used by the AWB statistics are in\n> ipu3_awb.h and documented.\n> A bit of the doc has been improved in this same patch.\n\nI would split out the documentation additions as a separate patch.\n\nIt feels like there are a few other potentially unrelated changes in\nhere too ...\n\n\n> There is one metadata variable which will be used and set in the AGC\n> algorithm in a near future, which is the agcGamma. Use it as if it exists, the\n> Metadata class won't find it and awb will default to a 1.0 value.\n\nEeep, ok - I wonder if we should add that part when it is there instead\n- but maybe that highlights one of the design benefits of the Metadata\nclass so I'll see below...\n\n\n> Doing it now is convenient to have nice process() and updateParamaters() calls.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 8 ++-\n> src/ipa/ipu3/ipu3_awb.cpp | 116 ++++++++++++++++++++++++++++----------\n> src/ipa/ipu3/ipu3_awb.h | 34 ++++++-----\n> 3 files changed, 111 insertions(+), 47 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 091856f5..1a3d98e9 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -80,6 +80,8 @@ private:\n> \tstd::unique_ptr agcAlgo_;\n> \t/* Interface to the Camera Helper */\n> \tstd::unique_ptr camHelper_;\n> +\t/* Metadata storage */\n\n\"Metadata metadata_\" might already express that. What can we add on top\nto explain what it stores?\n\n/* Key/value pairs for algorithms to share results and context */ ?\n\n\n\n> +\tMetadata metadata_;\n> \n> \t/* Local parameter storage */\n> \tstruct ipu3_uapi_params params_;\n> @@ -277,7 +279,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> \tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> +\t\tawbAlgo_->updateWbParameters(params_, &metadata_);\n\nGiven that metadata_ is a big global shared data, I wonder if we should\npass it to the algorithms at init time, so they have a reference to it -\nand can reference it internally.\n\nIs that worthwhile? or is it anticipated that there might be a new\nmetadata instance for each sequential run of the algorithms?\n\n\n\n> \t*params = params_;\n> \n> @@ -297,8 +299,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> \tagcAlgo_->process(stats, exposure_, gain);\n> \tgain_ = camHelper_->gainCode(gain);\n> \n> -\tawbAlgo_->calculateWBGains(stats);\n> +\t/* Calculate the AWB gains */\n> +\tawbAlgo_->process(stats, &metadata_);\n> \n> +\t/* Update the exposure and gains on sensor side */\n> \tif (agcAlgo_->updateControls())\n> \t\tsetControls(frame);\n> \n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> index 9b409c8f..d441e835 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -18,29 +18,54 @@ namespace ipa::ipu3 {\n> \n> LOG_DEFINE_CATEGORY(IPU3Awb)\n> \n> -static constexpr uint32_t kMinZonesCounted = 16;\n> -static constexpr uint32_t kMinGreenLevelInZone = 32;\n> +/**\n\nShould this be the documentation for this AWB class? It doesn't seem to\nhave an identifier here...\n\n\n> + * The Grey World algorithm assumes that the scene, in average, is neutral grey.\n> + * Reference: Lam, Edmund & Fung, George. (2008). Automatic White Balancing in\n> + * Digital Photography. 10.1201/9781420054538.ch10.\n\nIs there a public link to that? or is it a paper that would be likely\npaywalled? (Still valid to reference it even if it's not public I think).\n\nI was going to ask what the 10.1201... code is, but it looks like\ngoogling that brought me to researchgate.net and gave that text as the\ncorrect 'citation', so I guess this is a defined reference style.\n\n\n> + *\n> + * The IPU3 is generating statistics from the Bayer Demosaic Scaler output\n\ns/is generating/generates/\n\nIs it 'Demosaic' or 'Domain' ?\n\nI thought BDS would be Bayer domain scaler ... but I could be wrong.\n\n\n> + * into a grid defined in the ipu3_uapi_awb_config_s structure.\n> + *\n> + * For example, when the BDS output is 2592x1944 then the grid is calculated to be:\n> + * 81*30 with a cell beeing of size 32*64.\n\ns/beeing/\n\n\n\"\"\"\nFor example, when the BDS outputs a frame of 2592x1944, the grid may be\nconfigured to 81x30 cells each with a size of 32x64 pixels.\n\"\"\"\n\n> + * We then have an average of 2048 R, G and B pixels per cell.\n> + *\n> + * The AWB algorithm could use those variable grid sizes as an input, but it would\n> + * make it a bit more complex. In order to have something consistent with what is\n> + * done on RPi, fix a default grid size to kAwbStatsSizeX x kAwbStatsSizeY.\n\nThe IPU3 doesn't need to reference being consistent with other IPAs -\nthis should document what it is doing.\n\n\n> + *\n> + * Before calculating the gains, we will convert the statistics to go from the BDS\n> + * grid configuration to this intern grid size in generateAwbStats.\n\n\"we will convert the statistics from the BDS grid to an internal grid\nconfiguration in generateAwbStats.\"\n\n\n> + * When the stats are converted, the saturation flag in the initial grid is used to\n\n\"As part of converting the statistics to an internal grid, the\nsaturation flag from the originating grid cell is used to\"\n\n(I've only tried to improve the wording there, I don't know if that's\naccurate).\n\n\n\n> + * decide if the zone is saturated or not, making the zone relevant or not.\n\nWhat happens to saturated cells?\n\nWhat is a zone, is that a grid cell?\n\n> + *\n> + * The Grey World algorithm will then estimate the red and blue gains to apply, and\n> + * send those back through metadata.\n\n/send those back through/store the results in the/\n\n> + */\n> \n> /**\n> - * \\struct IspStatsRegion\n> + * \\struct StatsRegion\n\nRenaming a structure should be an independent patch.\n\n\n> * \\brief RGB statistics for a given region\n\nIs a region a cell? or a zone? or just a defined area that comprises of\n... some pixels?\n\nIt's not clear which terms define which concepts.\n\n\n> *\n> - * The IspStatsRegion structure is intended to abstract the ISP specific\n> - * statistics and use an agnostic algorithm to compute AWB.\n> + * The StatsRegion structure is intended to abstract the ISP specific\n\nIs this just a good intention? I suspect it either does it, or it doesn't.\n\n\"The StatsRegions structure abstracts the ISP specific...\"\n\n\n> + * statistics to compute AWB. The Grey World algorithm uses an average\n\nan average of what?\n\n\n\n> + * for a specific counted pixels. \n\noh, perhaps that was \"an average of the pixels in a given region.\"\n\n\n> When a specific zone in the scene is\n> + * saturated, we want to exclude it from the calculation, and consider\n> + * it as an outlier.\n\naha, ok - that answers an earlier question.\n\n\n> *\n> - * \\var IspStatsRegion::counted\n> - * \\brief Number of pixels used to calculate the sums\n> + * \\var StatsRegion::counted\n> + * \\brief Number of unsatured pixels used to calculate the sums\n\ns/unsatured/unsaturated/\n\nOh - So it distinguishes between a pixel being saturated, not a zone\ncontaining saturat\n\n\n> *\n> - * \\var IspStatsRegion::uncounted\n> - * \\brief Remaining number of pixels in the region\n> + * \\var StatsRegion::uncounted\n> + * \\brief Remaining number of pixels in the region (ie saturated)\n> *\n> - * \\var IspStatsRegion::rSum\n> + * \\var StatsRegion::rSum\n> * \\brief Sum of the red values in the region\n> *\n> - * \\var IspStatsRegion::gSum\n> + * \\var StatsRegion::gSum\n> * \\brief Sum of the green values in the region\n> *\n> - * \\var IspStatsRegion::bSum\n> + * \\var StatsRegion::bSum\n> * \\brief Sum of the blue values in the region\n> */\n> \n> @@ -48,26 +73,35 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n> * \\struct AwbStatus\n> * \\brief AWB parameters calculated\n> *\n> - * The AwbStatus structure is intended to store the AWB\n> - * parameters calculated by the algorithm\n> + * The AwbStatus structure is intended to store the AWB parameters\n\nI think we should declare what it does, not an intention.\n\n> + * calculated by the algorithm, and shared through the metadata\n> + * object, for other algorithms\n\ns/algorithms/algorithms./\n\n\n> *\n> * \\var AwbStatus::temperatureK\n> - * \\brief Color temperature calculated\n> + * \\brief Color temperature calculated, in Kelvin\n> *\n> * \\var AwbStatus::redGain\n> - * \\brief Gain calculated for the red channel\n> + * \\brief Gain calculated for the red channel. This is a floating-point\n> + * value used as a multiplier on the ISP side\n> *\n> * \\var AwbStatus::greenGain\n> - * \\brief Gain calculated for the green channel\n> + * \\brief Gain calculated for the green channel. This is a floating-point\n> + * value used as a multiplier on the ISP side\n> *\n> * \\var AwbStatus::blueGain\n> - * \\brief Gain calculated for the blue channel\n> + * \\brief Gain calculated for the blue channel. This is a floating-point\n> + * value used as a multiplier on the ISP side\n> */\n> \n> /**\n> * \\struct Ipu3AwbCell\n> * \\brief Memory layout for each cell in AWB metadata\n> *\n> + * This is the internal layout on IPU3 for one cell of AWB statistics.\n> + * There is ipu3_uapi_awb_config_s->grid.width * 2^block_width_log2 per\n\n/There is/There are/\n\nBut this isn't very readable or clear :S\n\n> + * ipu3_uapi_awb_config_s->grid.height * 2^block_height_log2 cells for\n> + * one frame statistics.\n\nI'm not sure I can correctly interpret it to rewrite it at the moment...\n\nIn particular 'per' is throwing me off.\n\nI presume this is describing that the grid has a width and a height? and\nthat the number of cells is ... width * height, but width and height are\nstored as the log2 ?\n\n\n> + *\n> * The Ipu3AwbCell structure is used to get individual values\n> * such as red average or saturation ratio in a particular cell.\n> *\n> @@ -84,7 +118,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n> * \\brief Green average for blue lines\n> *\n> * \\var Ipu3AwbCell::satRatio\n> - * \\brief Saturation ratio in the cell\n> + * \\brief Saturation ratio in the cell. It depends on the rgbs_thr_* values, and\n> + * will be set if rgbs_thr_b has IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit set.\n\n\"Saturation ratio of the cell\" sounds like the brief, and the expanded\nsentences sound like something that would be a new line to make it\nrender as more context beyond the brief.\n\nBut I'd express this then as:\n\n\"\"\"\nThe saturation ratio is determined based upon the threshold values set\nin rgbs_thr_*, and will be set if the threshold value has the\ncorresponding enable bit set in the (Where is it set?)..\n\nFor example, to obtain the Blue saturation value, the\nIPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit must be set in rgbs_thr_b.\n\"\"\"\n\n(But it might be good to reference what rgbs_thr_b is and where it can\nbe set perhaps ...)\n\n\n\n\n> *\n> * \\var Ipu3AwbCell::padding\n> * \\brief array of unused bytes for padding\n> @@ -159,6 +194,9 @@ const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {\n> \t7807, 7871, 7935, 7999, 8063, 8127, 8191\n> } };\n> \n> +/* Minimum level of green (on a 8 bits base) in a given zone */\n\nWhat is an 8 bits base?\n\nDo you mean it must be a multiple of 8?\nWhat happens if it isn't? Is it rounded up? down?\n\n\n> +static constexpr uint32_t kMinGreenLevelInZone = 16;\n> +\n> IPU3Awb::IPU3Awb()\n> \t: Algorithm()\n> {\n> @@ -166,6 +204,7 @@ IPU3Awb::IPU3Awb()\n> \tasyncResults_.greenGain = 1.0;\n> \tasyncResults_.redGain = 1.0;\n> \tasyncResults_.temperatureK = 4500;\n> +\tminZonesCounted_ = 0;\n> }\n> \n> IPU3Awb::~IPU3Awb()\n> @@ -202,7 +241,7 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> \tparams.acc_param.gamma.gc_lut = imguCssGammaLut;\n> \tparams.acc_param.gamma.gc_ctrl.enable = 1;\n> \n> -\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> +\tzones_.reserve(kAwbStatsSize);\n> }\n> \n> /**\n> @@ -238,10 +277,10 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)\n> /* Generate an RGB vector with the average values for each region */\n> void IPU3Awb::generateZones(std::vector &zones)\n> {\n> -\tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> +\tfor (unsigned int i = 0; i < kAwbStatsSize; i++) {\n> \t\tRGB zone;\n> \t\tdouble counted = awbStats_[i].counted;\n> -\t\tif (counted >= kMinZonesCounted) {\n> +\t\tif (counted >= minZonesCounted_) {\n> \t\t\tzone.G = awbStats_[i].gSum / counted;\n> \t\t\tif (zone.G >= kMinGreenLevelInZone) {\n> \t\t\t\tzone.R = awbStats_[i].rSum / counted;\n> @@ -258,6 +297,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \tuint32_t regionWidth = round(awbGrid_.width / static_cast(kAwbStatsSizeX));\n> \tuint32_t regionHeight = round(awbGrid_.height / static_cast(kAwbStatsSizeY));\n> \n> +\tminZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;\n\nWhere does 4/5 come from here?\n\n\n> \t/*\n> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n> \t * (awbGrid_.width x awbGrid_.height).\n> @@ -269,7 +309,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \t\t\tuint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;\n> \n> \t\t\tuint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;\n> -\t\t\tcellPosition *= 8;\n> +\t\t\tcellPosition *= sizeof(Ipu3AwbCell);\n\nIs this a bug fix? or because of the move to metadata? Looks like it\nalmost deserves it's own patch ? I can't quite tell.\n\n\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> @@ -287,7 +327,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \n> void IPU3Awb::clearAwbStats()\n> {\n> -\tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> +\tfor (unsigned int i = 0; i < kAwbStatsSize; i++) {\n> \t\tawbStats_[i].bSum = 0;\n> \t\tawbStats_[i].rSum = 0;\n> \t\tawbStats_[i].gSum = 0;\n> @@ -344,24 +384,38 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> \tgenerateAwbStats(stats);\n> \tgenerateZones(zones_);\n> \tLOG(IPU3Awb, Debug) << \"Valid zones: \" << zones_.size();\n> -\tif (zones_.size() > 10) {\n> +\n> +\t/* We need at least 5% of valid zones to estimate the gain correction */\n\n5 sounds quite low? Is that determined from empirical results? or just a\nrough estimate?\n\nWhat effect does increasing this ratio have?\n\n> +\tif (zones_.size() > kAwbStatsSize / 20) {\n> \t\tawbGreyWorld();\n> \t\tLOG(IPU3Awb, Debug) << \"Gain found for red: \" << asyncResults_.redGain\n> \t\t\t\t << \" and for blue: \" << asyncResults_.blueGain;\n> \t}\n> }\n> \n> -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma)\n> +void IPU3Awb::process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata)\n> +{\n> +\tcalculateWBGains(stats);\n> +\t/* We need to update the AWB status, to give back the gains */\n> +\timageMetadata->set(\"awb.status\", asyncResults_);\n\nI still think these look like results not status ;)\n\nDo we need to define a naming scheme for the metadata keys?\n\n\n> +}\n> +\n> +void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, Metadata *imageMetadata)\n> {\n> \t/*\n> \t * Green gains should not be touched and considered 1.\n> \t * Default is 16, so do not change it at all.\n> -\t * 4096 is the value for a gain of 1.0\n> +\t * 8192 is the value for a gain of 1.0\n> \t */\n> -\tparams.acc_param.bnr.wb_gains.gr = 16;\n> -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n> -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n> -\tparams.acc_param.bnr.wb_gains.gb = 16;\n> +\tparams.acc_param.bnr.wb_gains.gr = 8192;\n> +\tparams.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;\n> +\tparams.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;\n> +\tparams.acc_param.bnr.wb_gains.gb = 8192;\n\nIs this change part of using the metadata?\n\nWe're going from 4096 to 8192. That sounds like a specific change worthy\nof it's own commit message to explain why this change occurs ?\n\n\n\n> +\n> +\t/* When the AGC algorithm has run, it may have set a new gamma */\n> +\tdouble agcGamma = 1.0;\n> +\tif (imageMetadata->get(\"agc.gamma\", agcGamma) != 0)\n> +\t\tLOG(IPU3Awb, Debug) << \"Awb: no gamma found, defaulted to 1.0\";\n> \n> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK\n> \t\t\t << \" and gamma calculated: \" << agcGamma;\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index 122cf68c..cf12032d 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -14,14 +14,18 @@\n> #include \n> \n> #include \"libipa/algorithm.h\"\n> +#include \"libipa/metadata.h\"\n> \n> namespace libcamera {\n> \n> namespace ipa::ipu3 {\n> \n> -/* Region size for the statistics generation algorithm */\n> +/* Width of the AWB regions used for Grey World calculation */\n> static constexpr uint32_t kAwbStatsSizeX = 16;\n> +/* Height of the AWB regions used for Grey World calculation */\n> static constexpr uint32_t kAwbStatsSizeY = 12;\n> +/* Total size of the AWB regions used for Grey World calculation */\n> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY;\n> \n> class IPU3Awb : public Algorithm\n\nShould this be named IPU3AwbGreyworld?\n\n\nPresumably we might potentially have another Awb implementation which\nwould run a different algorithm?\n\n(I wonder then what the implications are for switching between the\nalgorithms in that case).\n\nBut perhaps we should leave that name as is for now until we have a more\nmodular construction for the independent algorithms.\n\n\n\n> {\n> @@ -30,17 +34,8 @@ public:\n> \t~IPU3Awb();\n> \n> \tvoid initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n> -\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> -\tvoid updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma);\n> -\n> -\tstruct Ipu3AwbCell {\n> -\t\tunsigned char greenRedAvg;\n> -\t\tunsigned char redAvg;\n> -\t\tunsigned char blueAvg;\n> -\t\tunsigned char greenBlueAvg;\n> -\t\tunsigned char satRatio;\n> -\t\tunsigned char padding[3];\n> -\t} __attribute__((packed));\n> +\tvoid process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata);\n> +\tvoid updateWbParameters(ipu3_uapi_params ¶ms, Metadata *imageMetadata);\n> \n> \t/* \\todo Make these three structs available to all the ISPs ? */\n> \tstruct RGB {\n> @@ -56,7 +51,7 @@ public:\n> \t\t}\n> \t};\n> \n> -\tstruct IspStatsRegion {\n> +\tstruct StatsRegion {\n> \t\tunsigned int counted;\n> \t\tunsigned int uncounted;\n> \t\tunsigned long long rSum;\n> @@ -71,18 +66,29 @@ public:\n> \t\tdouble blueGain;\n> \t};\n> \n> +\tstruct Ipu3AwbCell {\n> +\t\tunsigned char greenRedAvg;\n> +\t\tunsigned char redAvg;\n> +\t\tunsigned char blueAvg;\n> +\t\tunsigned char greenBlueAvg;\n> +\t\tunsigned char satRatio;\n> +\t\tunsigned char padding[3];\n> +\t};\n> +\n> private:\n> \tvoid generateZones(std::vector &zones);\n> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> \tvoid clearAwbStats();\n> \tvoid awbGreyWorld();\n> \tuint32_t estimateCCT(double red, double green, double blue);\n> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> \n> \tstruct ipu3_uapi_grid_config awbGrid_;\n> \n> \tstd::vector zones_;\n> -\tIspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n> +\tStatsRegion awbStats_[kAwbStatsSize];\n> \tAwbStatus asyncResults_;\n> +\tuint32_t minZonesCounted_;\n> };\n> \n> } /* namespace ipa::ipu3 */\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 8FA52C3226\n\tfor ;\n\tTue, 13 Jul 2021 16:06:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 805B368525;\n\tTue, 13 Jul 2021 18:06:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7E2E684F3\n\tfor ;\n\tTue, 13 Jul 2021 18:06:41 +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 69035CC;\n\tTue, 13 Jul 2021 18:06:41 +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=\"Y50Lr7nk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626192401;\n\tbh=jLYwjH5qysxYBdalTGdaYssSdV+UC/kuUt+9m7fyF60=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=Y50Lr7nkQ2GbIPfmPsDz2UGW8Nid2D1pe33Iz2NbGJ1Sq7vpP872gnl7LxakUuzqV\n\tUZtHZ+HJIUmQNhKECBWVGVG5fbhVFK30mwezYDehEVLp04ORut+7SBLVoNhTtjH7w3\n\tIxA0XIznO6u2XPaGzohEQ1izx3l76uIJXS/JvDME=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-3-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<961dac7f-2b59-860a-5b5e-04077445d02c@ideasonboard.com>","Date":"Tue, 13 Jul 2021 17:06:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210712131630.73914-3-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: ipu3: Use metadata and\n\timprove the doc","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\" "}},{"id":18164,"web_url":"https://patchwork.libcamera.org/comment/18164/","msgid":"<66cba20c-ae76-45a4-3b4d-87acb5842609@ideasonboard.com>","date":"2021-07-14T08:00:55","subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: ipu3: Use metadata and\n\timprove the doc","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 13/07/2021 18:06, Kieran Bingham wrote:\n> Hi JM,\n> \n> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n>> Using the metadata to exchange the status of the awb algorithm helps to\n>> simplify the interface. The structures used by the AWB statistics are in\n>> ipu3_awb.h and documented.\n>> A bit of the doc has been improved in this same patch.\n> \n> I would split out the documentation additions as a separate patch.\n> \n> It feels like there are a few other potentially unrelated changes in\n> here too ...\n\nYes, I will split it :-)\n\n> \n>> There is one metadata variable which will be used and set in the AGC\n>> algorithm in a near future, which is the agcGamma. Use it as if it exists, the\n>> Metadata class won't find it and awb will default to a 1.0 value.\n> \n> Eeep, ok - I wonder if we should add that part when it is there instead\n> - but maybe that highlights one of the design benefits of the Metadata\n> class so I'll see below...\n> \n> \n>> Doing it now is convenient to have nice process() and updateParamaters() calls.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 8 ++-\n>> src/ipa/ipu3/ipu3_awb.cpp | 116 ++++++++++++++++++++++++++++----------\n>> src/ipa/ipu3/ipu3_awb.h | 34 ++++++-----\n>> 3 files changed, 111 insertions(+), 47 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 091856f5..1a3d98e9 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -80,6 +80,8 @@ private:\n>> \tstd::unique_ptr agcAlgo_;\n>> \t/* Interface to the Camera Helper */\n>> \tstd::unique_ptr camHelper_;\n>> +\t/* Metadata storage */\n> \n> \"Metadata metadata_\" might already express that. What can we add on top\n> to explain what it stores?\n> \n> /* Key/value pairs for algorithms to share results and context */ ?\n> \n> \n> \n>> +\tMetadata metadata_;\n>> \n>> \t/* Local parameter storage */\n>> \tstruct ipu3_uapi_params params_;\n>> @@ -277,7 +279,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>> {\n>> \tif (agcAlgo_->updateControls())\n>> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n>> +\t\tawbAlgo_->updateWbParameters(params_, &metadata_);\n> \n> Given that metadata_ is a big global shared data, I wonder if we should\n> pass it to the algorithms at init time, so they have a reference to it -\n> and can reference it internally.\n> \n> Is that worthwhile? or is it anticipated that there might be a new\n> metadata instance for each sequential run of the algorithms?\n> \n\nWe might, in the future, want to pass a different reference, even for\nnow, it could make sense to remove the metadata reference from the\nprocess() calls...\nIt makes its design easier to understand from a new comer, I think, as\nyou can see the algorithm takes some data as input... ?\nBut it is very subjective.\n\n> \n>> \t*params = params_;\n>> \n>> @@ -297,8 +299,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>> \tagcAlgo_->process(stats, exposure_, gain);\n>> \tgain_ = camHelper_->gainCode(gain);\n>> \n>> -\tawbAlgo_->calculateWBGains(stats);\n>> +\t/* Calculate the AWB gains */\n>> +\tawbAlgo_->process(stats, &metadata_);\n>> \n>> +\t/* Update the exposure and gains on sensor side */\n>> \tif (agcAlgo_->updateControls())\n>> \t\tsetControls(frame);\n>> \n>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>> index 9b409c8f..d441e835 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>> @@ -18,29 +18,54 @@ namespace ipa::ipu3 {\n>> \n>> LOG_DEFINE_CATEGORY(IPU3Awb)\n>> \n>> -static constexpr uint32_t kMinZonesCounted = 16;\n>> -static constexpr uint32_t kMinGreenLevelInZone = 32;\n>> +/**\n> \n> Should this be the documentation for this AWB class? It doesn't seem to\n> have an identifier here...\n> \n> \n>> + * The Grey World algorithm assumes that the scene, in average, is neutral grey.\n>> + * Reference: Lam, Edmund & Fung, George. (2008). Automatic White Balancing in\n>> + * Digital Photography. 10.1201/9781420054538.ch10.\n> \n> Is there a public link to that? or is it a paper that would be likely\n> paywalled? (Still valid to reference it even if it's not public I think).\n> \n> I was going to ask what the 10.1201... code is, but it looks like\n> googling that brought me to researchgate.net and gave that text as the\n> correct 'citation', so I guess this is a defined reference style.\n> \n\nThis is the DOI: Digital Object Identifier\n\n>> + *\n>> + * The IPU3 is generating statistics from the Bayer Demosaic Scaler output\n> \n> s/is generating/generates/\n> \n> Is it 'Demosaic' or 'Domain' ?\n> \n> I thought BDS would be Bayer domain scaler ... but I could be wrong.\n> \n\nThanks for noticing the type, it is Bayer Down Scaler btw ;-)\n\n>> + * into a grid defined in the ipu3_uapi_awb_config_s structure.\n>> + *\n>> + * For example, when the BDS output is 2592x1944 then the grid is calculated to be:\n>> + * 81*30 with a cell beeing of size 32*64.\n> \n> s/beeing/\n> \n> \n> \"\"\"\n> For example, when the BDS outputs a frame of 2592x1944, the grid may be\n> configured to 81x30 cells each with a size of 32x64 pixels.\n> \"\"\"\n> \n>> + * We then have an average of 2048 R, G and B pixels per cell.\n>> + *\n>> + * The AWB algorithm could use those variable grid sizes as an input, but it would\n>> + * make it a bit more complex. In order to have something consistent with what is\n>> + * done on RPi, fix a default grid size to kAwbStatsSizeX x kAwbStatsSizeY.\n> \n> The IPU3 doesn't need to reference being consistent with other IPAs -\n> this should document what it is doing.\n> \n> \n>> + *\n>> + * Before calculating the gains, we will convert the statistics to go from the BDS\n>> + * grid configuration to this intern grid size in generateAwbStats.\n> \n> \"we will convert the statistics from the BDS grid to an internal grid\n> configuration in generateAwbStats.\"\n> \n> \n>> + * When the stats are converted, the saturation flag in the initial grid is used to\n> \n> \"As part of converting the statistics to an internal grid, the\n> saturation flag from the originating grid cell is used to\"\n> \n> (I've only tried to improve the wording there, I don't know if that's\n> accurate).\n> \n> \n> \n>> + * decide if the zone is saturated or not, making the zone relevant or not.\n> \n> What happens to saturated cells?\n> \n> What is a zone, is that a grid cell?\n> \n>> + *\n>> + * The Grey World algorithm will then estimate the red and blue gains to apply, and\n>> + * send those back through metadata.\n> \n> /send those back through/store the results in the/\n> \n>> + */\n>> \n>> /**\n>> - * \\struct IspStatsRegion\n>> + * \\struct StatsRegion\n> \n> Renaming a structure should be an independent patch.\n> \n> \n>> * \\brief RGB statistics for a given region\n> \n> Is a region a cell? or a zone? or just a defined area that comprises of\n> ... some pixels?\n> \n> It's not clear which terms define which concepts.\n> \n> \n>> *\n>> - * The IspStatsRegion structure is intended to abstract the ISP specific\n>> - * statistics and use an agnostic algorithm to compute AWB.\n>> + * The StatsRegion structure is intended to abstract the ISP specific\n> \n> Is this just a good intention? I suspect it either does it, or it doesn't.\n> \n> \"The StatsRegions structure abstracts the ISP specific...\"\n> \n> \n>> + * statistics to compute AWB. The Grey World algorithm uses an average\n> \n> an average of what?\n> \n> \n> \n>> + * for a specific counted pixels. \n> \n> oh, perhaps that was \"an average of the pixels in a given region.\"\n> \n> \n>> When a specific zone in the scene is\n>> + * saturated, we want to exclude it from the calculation, and consider\n>> + * it as an outlier.\n> \n> aha, ok - that answers an earlier question.\n> \n> \n>> *\n>> - * \\var IspStatsRegion::counted\n>> - * \\brief Number of pixels used to calculate the sums\n>> + * \\var StatsRegion::counted\n>> + * \\brief Number of unsatured pixels used to calculate the sums\n> \n> s/unsatured/unsaturated/\n> \n> Oh - So it distinguishes between a pixel being saturated, not a zone\n> containing saturat\n> \n> \n>> *\n>> - * \\var IspStatsRegion::uncounted\n>> - * \\brief Remaining number of pixels in the region\n>> + * \\var StatsRegion::uncounted\n>> + * \\brief Remaining number of pixels in the region (ie saturated)\n>> *\n>> - * \\var IspStatsRegion::rSum\n>> + * \\var StatsRegion::rSum\n>> * \\brief Sum of the red values in the region\n>> *\n>> - * \\var IspStatsRegion::gSum\n>> + * \\var StatsRegion::gSum\n>> * \\brief Sum of the green values in the region\n>> *\n>> - * \\var IspStatsRegion::bSum\n>> + * \\var StatsRegion::bSum\n>> * \\brief Sum of the blue values in the region\n>> */\n>> \n>> @@ -48,26 +73,35 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n>> * \\struct AwbStatus\n>> * \\brief AWB parameters calculated\n>> *\n>> - * The AwbStatus structure is intended to store the AWB\n>> - * parameters calculated by the algorithm\n>> + * The AwbStatus structure is intended to store the AWB parameters\n> \n> I think we should declare what it does, not an intention.\n> \n>> + * calculated by the algorithm, and shared through the metadata\n>> + * object, for other algorithms\n> \n> s/algorithms/algorithms./\n> \n> \n>> *\n>> * \\var AwbStatus::temperatureK\n>> - * \\brief Color temperature calculated\n>> + * \\brief Color temperature calculated, in Kelvin\n>> *\n>> * \\var AwbStatus::redGain\n>> - * \\brief Gain calculated for the red channel\n>> + * \\brief Gain calculated for the red channel. This is a floating-point\n>> + * value used as a multiplier on the ISP side\n>> *\n>> * \\var AwbStatus::greenGain\n>> - * \\brief Gain calculated for the green channel\n>> + * \\brief Gain calculated for the green channel. This is a floating-point\n>> + * value used as a multiplier on the ISP side\n>> *\n>> * \\var AwbStatus::blueGain\n>> - * \\brief Gain calculated for the blue channel\n>> + * \\brief Gain calculated for the blue channel. This is a floating-point\n>> + * value used as a multiplier on the ISP side\n>> */\n>> \n>> /**\n>> * \\struct Ipu3AwbCell\n>> * \\brief Memory layout for each cell in AWB metadata\n>> *\n>> + * This is the internal layout on IPU3 for one cell of AWB statistics.\n>> + * There is ipu3_uapi_awb_config_s->grid.width * 2^block_width_log2 per\n> \n> /There is/There are/\n> \n> But this isn't very readable or clear :S\n> \n>> + * ipu3_uapi_awb_config_s->grid.height * 2^block_height_log2 cells for\n>> + * one frame statistics.\n> \n> I'm not sure I can correctly interpret it to rewrite it at the moment...\n> \n> In particular 'per' is throwing me off.\n> \n> I presume this is describing that the grid has a width and a height? and\n> that the number of cells is ... width * height, but width and height are\n> stored as the log2 ?\n> \n\nYes. How should it be reworded :-) ?\n\n>> + *\n>> * The Ipu3AwbCell structure is used to get individual values\n>> * such as red average or saturation ratio in a particular cell.\n>> *\n>> @@ -84,7 +118,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n>> * \\brief Green average for blue lines\n>> *\n>> * \\var Ipu3AwbCell::satRatio\n>> - * \\brief Saturation ratio in the cell\n>> + * \\brief Saturation ratio in the cell. It depends on the rgbs_thr_* values, and\n>> + * will be set if rgbs_thr_b has IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit set.\n> \n> \"Saturation ratio of the cell\" sounds like the brief, and the expanded\n> sentences sound like something that would be a new line to make it\n> render as more context beyond the brief.\n> \n> But I'd express this then as:\n> \n> \"\"\"\n> The saturation ratio is determined based upon the threshold values set\n> in rgbs_thr_*, and will be set if the threshold value has the\n> corresponding enable bit set in the (Where is it set?)..\n> \n> For example, to obtain the Blue saturation value, the\n> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit must be set in rgbs_thr_b.\n> \"\"\"\n> \n> (But it might be good to reference what rgbs_thr_b is and where it can\n> be set perhaps ...)\n> \n> \n> \n> \n>> *\n>> * \\var Ipu3AwbCell::padding\n>> * \\brief array of unused bytes for padding\n>> @@ -159,6 +194,9 @@ const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {\n>> \t7807, 7871, 7935, 7999, 8063, 8127, 8191\n>> } };\n>> \n>> +/* Minimum level of green (on a 8 bits base) in a given zone */\n> \n> What is an 8 bits base?\n> \n> Do you mean it must be a multiple of 8?\n> What happens if it isn't? Is it rounded up? down?\n\nIt means that the value has a maximum of 256, so it is not really useful\nto say anything about it.\n\n+/* Minimum level of green in a given zone */\n\n> \n>> +static constexpr uint32_t kMinGreenLevelInZone = 16;\n>> +\n>> IPU3Awb::IPU3Awb()\n>> \t: Algorithm()\n>> {\n>> @@ -166,6 +204,7 @@ IPU3Awb::IPU3Awb()\n>> \tasyncResults_.greenGain = 1.0;\n>> \tasyncResults_.redGain = 1.0;\n>> \tasyncResults_.temperatureK = 4500;\n>> +\tminZonesCounted_ = 0;\n>> }\n>> \n>> IPU3Awb::~IPU3Awb()\n>> @@ -202,7 +241,7 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n>> \tparams.acc_param.gamma.gc_lut = imguCssGammaLut;\n>> \tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>> \n>> -\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>> +\tzones_.reserve(kAwbStatsSize);\n>> }\n>> \n>> /**\n>> @@ -238,10 +277,10 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)\n>> /* Generate an RGB vector with the average values for each region */\n>> void IPU3Awb::generateZones(std::vector &zones)\n>> {\n>> -\tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n>> +\tfor (unsigned int i = 0; i < kAwbStatsSize; i++) {\n>> \t\tRGB zone;\n>> \t\tdouble counted = awbStats_[i].counted;\n>> -\t\tif (counted >= kMinZonesCounted) {\n>> +\t\tif (counted >= minZonesCounted_) {\n>> \t\t\tzone.G = awbStats_[i].gSum / counted;\n>> \t\t\tif (zone.G >= kMinGreenLevelInZone) {\n>> \t\t\t\tzone.R = awbStats_[i].rSum / counted;\n>> @@ -258,6 +297,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n>> \tuint32_t regionWidth = round(awbGrid_.width / static_cast(kAwbStatsSizeX));\n>> \tuint32_t regionHeight = round(awbGrid_.height / static_cast(kAwbStatsSizeY));\n>> \n>> +\tminZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;\n> \n> Where does 4/5 come from here?\n\nThis is a way to say 80% :-p.\nIt is the minimum proportion of pixels counted within AWB region for it\nto be relevant. But this value is empirical.\n\n> \n>> \t/*\n>> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n>> \t * (awbGrid_.width x awbGrid_.height).\n>> @@ -269,7 +309,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n>> \t\t\tuint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;\n>> \n>> \t\t\tuint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;\n>> -\t\t\tcellPosition *= 8;\n>> +\t\t\tcellPosition *= sizeof(Ipu3AwbCell);\n> \n> Is this a bug fix? or because of the move to metadata? Looks like it\n> almost deserves it's own patch ? I can't quite tell.\n> \n> \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>> @@ -287,7 +327,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n>> \n>> void IPU3Awb::clearAwbStats()\n>> {\n>> -\tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n>> +\tfor (unsigned int i = 0; i < kAwbStatsSize; i++) {\n>> \t\tawbStats_[i].bSum = 0;\n>> \t\tawbStats_[i].rSum = 0;\n>> \t\tawbStats_[i].gSum = 0;\n>> @@ -344,24 +384,38 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>> \tgenerateAwbStats(stats);\n>> \tgenerateZones(zones_);\n>> \tLOG(IPU3Awb, Debug) << \"Valid zones: \" << zones_.size();\n>> -\tif (zones_.size() > 10) {\n>> +\n>> +\t/* We need at least 5% of valid zones to estimate the gain correction */\n> \n> 5 sounds quite low? Is that determined from empirical results? or just a\n> rough estimate?\n> \n> What effect does increasing this ratio have?\n\nIf you increase it too much you will have to wait longer for the AWB to\nstart correcting the colors. And if the AGC is \"slow\" then you may\nexperience weird colors before it is locked.\n\n> \n>> +\tif (zones_.size() > kAwbStatsSize / 20) {\n>> \t\tawbGreyWorld();\n>> \t\tLOG(IPU3Awb, Debug) << \"Gain found for red: \" << asyncResults_.redGain\n>> \t\t\t\t << \" and for blue: \" << asyncResults_.blueGain;\n>> \t}\n>> }\n>> \n>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma)\n>> +void IPU3Awb::process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata)\n>> +{\n>> +\tcalculateWBGains(stats);\n>> +\t/* We need to update the AWB status, to give back the gains */\n>> +\timageMetadata->set(\"awb.status\", asyncResults_);\n> \n> I still think these look like results not status ;)\n> \n> Do we need to define a naming scheme for the metadata keys?\n\nI will add the name used in the s/AwbStatus/AwbResults structure\ndocumentation. I don't really care using awb.status or awb.results.\n\n> \n> \n>> +}\n>> +\n>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, Metadata *imageMetadata)\n>> {\n>> \t/*\n>> \t * Green gains should not be touched and considered 1.\n>> \t * Default is 16, so do not change it at all.\n>> -\t * 4096 is the value for a gain of 1.0\n>> +\t * 8192 is the value for a gain of 1.0\n>> \t */\n>> -\tparams.acc_param.bnr.wb_gains.gr = 16;\n>> -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n>> -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n>> -\tparams.acc_param.bnr.wb_gains.gb = 16;\n>> +\tparams.acc_param.bnr.wb_gains.gr = 8192;\n>> +\tparams.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;\n>> +\tparams.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;\n>> +\tparams.acc_param.bnr.wb_gains.gb = 8192;\n> \n> Is this change part of using the metadata?\n> \n> We're going from 4096 to 8192. That sounds like a specific change worthy\n> of it's own commit message to explain why this change occurs ?\n\nYes.\n\n> \n> \n>> +\n>> +\t/* When the AGC algorithm has run, it may have set a new gamma */\n>> +\tdouble agcGamma = 1.0;\n>> +\tif (imageMetadata->get(\"agc.gamma\", agcGamma) != 0)\n>> +\t\tLOG(IPU3Awb, Debug) << \"Awb: no gamma found, defaulted to 1.0\";\n>> \n>> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK\n>> \t\t\t << \" and gamma calculated: \" << agcGamma;\n>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>> index 122cf68c..cf12032d 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.h\n>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>> @@ -14,14 +14,18 @@\n>> #include \n>> \n>> #include \"libipa/algorithm.h\"\n>> +#include \"libipa/metadata.h\"\n>> \n>> namespace libcamera {\n>> \n>> namespace ipa::ipu3 {\n>> \n>> -/* Region size for the statistics generation algorithm */\n>> +/* Width of the AWB regions used for Grey World calculation */\n>> static constexpr uint32_t kAwbStatsSizeX = 16;\n>> +/* Height of the AWB regions used for Grey World calculation */\n>> static constexpr uint32_t kAwbStatsSizeY = 12;\n>> +/* Total size of the AWB regions used for Grey World calculation */\n>> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY;\n>> \n>> class IPU3Awb : public Algorithm\n> \n> Should this be named IPU3AwbGreyworld?\n> \n> \n> Presumably we might potentially have another Awb implementation which\n> would run a different algorithm?\n> \n> (I wonder then what the implications are for switching between the\n> algorithms in that case).\n> \n> But perhaps we should leave that name as is for now until we have a more\n> modular construction for the independent algorithms.\n> \n\nI think we should not name it differently for now, as we don't have\nanother algorithm yet :-).\n\n> \n>> {\n>> @@ -30,17 +34,8 @@ public:\n>> \t~IPU3Awb();\n>> \n>> \tvoid initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n>> -\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>> -\tvoid updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma);\n>> -\n>> -\tstruct Ipu3AwbCell {\n>> -\t\tunsigned char greenRedAvg;\n>> -\t\tunsigned char redAvg;\n>> -\t\tunsigned char blueAvg;\n>> -\t\tunsigned char greenBlueAvg;\n>> -\t\tunsigned char satRatio;\n>> -\t\tunsigned char padding[3];\n>> -\t} __attribute__((packed));\n>> +\tvoid process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata);\n>> +\tvoid updateWbParameters(ipu3_uapi_params ¶ms, Metadata *imageMetadata);\n>> \n>> \t/* \\todo Make these three structs available to all the ISPs ? */\n>> \tstruct RGB {\n>> @@ -56,7 +51,7 @@ public:\n>> \t\t}\n>> \t};\n>> \n>> -\tstruct IspStatsRegion {\n>> +\tstruct StatsRegion {\n>> \t\tunsigned int counted;\n>> \t\tunsigned int uncounted;\n>> \t\tunsigned long long rSum;\n>> @@ -71,18 +66,29 @@ public:\n>> \t\tdouble blueGain;\n>> \t};\n>> \n>> +\tstruct Ipu3AwbCell {\n>> +\t\tunsigned char greenRedAvg;\n>> +\t\tunsigned char redAvg;\n>> +\t\tunsigned char blueAvg;\n>> +\t\tunsigned char greenBlueAvg;\n>> +\t\tunsigned char satRatio;\n>> +\t\tunsigned char padding[3];\n>> +\t};\n>> +\n>> private:\n>> \tvoid generateZones(std::vector &zones);\n>> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n>> \tvoid clearAwbStats();\n>> \tvoid awbGreyWorld();\n>> \tuint32_t estimateCCT(double red, double green, double blue);\n>> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>> \n>> \tstruct ipu3_uapi_grid_config awbGrid_;\n>> \n>> \tstd::vector zones_;\n>> -\tIspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n>> +\tStatsRegion awbStats_[kAwbStatsSize];\n>> \tAwbStatus asyncResults_;\n>> +\tuint32_t minZonesCounted_;\n>> };\n>> \n>> } /* namespace ipa::ipu3 */\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 5E47BC3226\n\tfor ;\n\tWed, 14 Jul 2021 08:01:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B57BF68525;\n\tWed, 14 Jul 2021 10:01: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 6B05360282\n\tfor ;\n\tWed, 14 Jul 2021 10:00:58 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:e67c:9465:36df:f139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EA5DDCC;\n\tWed, 14 Jul 2021 10:00:57 +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=\"n3eJAMa/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626249658;\n\tbh=Iw39VWozwXwGWBDxUu72qmM02UKbg0hCMB5nZNqCAMY=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=n3eJAMa/tb2iVC8OxaZurJBZsQro/OXsvZ8mwdNc0FXb+IkCk2C+OF6TWZsBvrl7p\n\tJYwk1H7Q/jTUQOUE92hnNq9WupMOAmYnMaRAgZlcsUZtPMqqVwkMtBQf0fzvPHcxHL\n\tQwppQphLMFZ6HOaAxsxyB/SxJ0UgJEuoR58K9jgk=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-3-jeanmichel.hautbois@ideasonboard.com>\n\t<961dac7f-2b59-860a-5b5e-04077445d02c@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<66cba20c-ae76-45a4-3b4d-87acb5842609@ideasonboard.com>","Date":"Wed, 14 Jul 2021 10:00:55 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<961dac7f-2b59-860a-5b5e-04077445d02c@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: ipu3: Use metadata and\n\timprove the doc","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\" "}},{"id":18171,"web_url":"https://patchwork.libcamera.org/comment/18171/","msgid":"","date":"2021-07-14T11:20:58","subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: ipu3: Use metadata and\n\timprove the doc","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 14/07/2021 09:00, Jean-Michel Hautbois wrote:\n> Hi Kieran,\n> \n> On 13/07/2021 18:06, Kieran Bingham wrote:\n>> Hi JM,\n>>\n>> On 12/07/2021 14:16, Jean-Michel Hautbois wrote:\n>>> Using the metadata to exchange the status of the awb algorithm helps to\n>>> simplify the interface. The structures used by the AWB statistics are in\n>>> ipu3_awb.h and documented.\n>>> A bit of the doc has been improved in this same patch.\n>>\n>> I would split out the documentation additions as a separate patch.\n>>\n>> It feels like there are a few other potentially unrelated changes in\n>> here too ...\n> \n> Yes, I will split it :-)\n> \n>>\n>>> There is one metadata variable which will be used and set in the AGC\n>>> algorithm in a near future, which is the agcGamma. Use it as if it exists, the\n>>> Metadata class won't find it and awb will default to a 1.0 value.\n>>\n>> Eeep, ok - I wonder if we should add that part when it is there instead\n>> - but maybe that highlights one of the design benefits of the Metadata\n>> class so I'll see below...\n>>\n>>\n>>> Doing it now is convenient to have nice process() and updateParamaters() calls.\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/ipu3.cpp | 8 ++-\n>>> src/ipa/ipu3/ipu3_awb.cpp | 116 ++++++++++++++++++++++++++++----------\n>>> src/ipa/ipu3/ipu3_awb.h | 34 ++++++-----\n>>> 3 files changed, 111 insertions(+), 47 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index 091856f5..1a3d98e9 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -80,6 +80,8 @@ private:\n>>> \tstd::unique_ptr agcAlgo_;\n>>> \t/* Interface to the Camera Helper */\n>>> \tstd::unique_ptr camHelper_;\n>>> +\t/* Metadata storage */\n>>\n>> \"Metadata metadata_\" might already express that. What can we add on top\n>> to explain what it stores?\n>>\n>> /* Key/value pairs for algorithms to share results and context */ ?\n>>\n>>\n>>\n>>> +\tMetadata metadata_;\n>>> \n>>> \t/* Local parameter storage */\n>>> \tstruct ipu3_uapi_params params_;\n>>> @@ -277,7 +279,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>> {\n>>> \tif (agcAlgo_->updateControls())\n>>> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n>>> +\t\tawbAlgo_->updateWbParameters(params_, &metadata_);\n>>\n>> Given that metadata_ is a big global shared data, I wonder if we should\n>> pass it to the algorithms at init time, so they have a reference to it -\n>> and can reference it internally.\n>>\n>> Is that worthwhile? or is it anticipated that there might be a new\n>> metadata instance for each sequential run of the algorithms?\n>>\n> \n> We might, in the future, want to pass a different reference, even for\n> now, it could make sense to remove the metadata reference from the\n> process() calls...\n> It makes its design easier to understand from a new comer, I think, as\n> you can see the algorithm takes some data as input... ?\n> But it is very subjective.\n> \n>>\n>>> \t*params = params_;\n>>> \n>>> @@ -297,8 +299,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>> \tagcAlgo_->process(stats, exposure_, gain);\n>>> \tgain_ = camHelper_->gainCode(gain);\n>>> \n>>> -\tawbAlgo_->calculateWBGains(stats);\n>>> +\t/* Calculate the AWB gains */\n>>> +\tawbAlgo_->process(stats, &metadata_);\n>>> \n>>> +\t/* Update the exposure and gains on sensor side */\n>>> \tif (agcAlgo_->updateControls())\n>>> \t\tsetControls(frame);\n>>> \n>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>>> index 9b409c8f..d441e835 100644\n>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>>> @@ -18,29 +18,54 @@ namespace ipa::ipu3 {\n>>> \n>>> LOG_DEFINE_CATEGORY(IPU3Awb)\n>>> \n>>> -static constexpr uint32_t kMinZonesCounted = 16;\n>>> -static constexpr uint32_t kMinGreenLevelInZone = 32;\n>>> +/**\n>>\n>> Should this be the documentation for this AWB class? It doesn't seem to\n>> have an identifier here...\n>>\n>>\n>>> + * The Grey World algorithm assumes that the scene, in average, is neutral grey.\n>>> + * Reference: Lam, Edmund & Fung, George. (2008). Automatic White Balancing in\n>>> + * Digital Photography. 10.1201/9781420054538.ch10.\n>>\n>> Is there a public link to that? or is it a paper that would be likely\n>> paywalled? (Still valid to reference it even if it's not public I think).\n>>\n>> I was going to ask what the 10.1201... code is, but it looks like\n>> googling that brought me to researchgate.net and gave that text as the\n>> correct 'citation', so I guess this is a defined reference style.\n>>\n> \n> This is the DOI: Digital Object Identifier\n> \n>>> + *\n>>> + * The IPU3 is generating statistics from the Bayer Demosaic Scaler output\n>>\n>> s/is generating/generates/\n>>\n>> Is it 'Demosaic' or 'Domain' ?\n>>\n>> I thought BDS would be Bayer domain scaler ... but I could be wrong.\n>>\n> \n> Thanks for noticing the type, it is Bayer Down Scaler btw ;-)\n> \n>>> + * into a grid defined in the ipu3_uapi_awb_config_s structure.\n>>> + *\n>>> + * For example, when the BDS output is 2592x1944 then the grid is calculated to be:\n>>> + * 81*30 with a cell beeing of size 32*64.\n>>\n>> s/beeing/\n>>\n>>\n>> \"\"\"\n>> For example, when the BDS outputs a frame of 2592x1944, the grid may be\n>> configured to 81x30 cells each with a size of 32x64 pixels.\n>> \"\"\"\n>>\n>>> + * We then have an average of 2048 R, G and B pixels per cell.\n>>> + *\n>>> + * The AWB algorithm could use those variable grid sizes as an input, but it would\n>>> + * make it a bit more complex. In order to have something consistent with what is\n>>> + * done on RPi, fix a default grid size to kAwbStatsSizeX x kAwbStatsSizeY.\n>>\n>> The IPU3 doesn't need to reference being consistent with other IPAs -\n>> this should document what it is doing.\n>>\n>>\n>>> + *\n>>> + * Before calculating the gains, we will convert the statistics to go from the BDS\n>>> + * grid configuration to this intern grid size in generateAwbStats.\n>>\n>> \"we will convert the statistics from the BDS grid to an internal grid\n>> configuration in generateAwbStats.\"\n>>\n>>\n>>> + * When the stats are converted, the saturation flag in the initial grid is used to\n>>\n>> \"As part of converting the statistics to an internal grid, the\n>> saturation flag from the originating grid cell is used to\"\n>>\n>> (I've only tried to improve the wording there, I don't know if that's\n>> accurate).\n>>\n>>\n>>\n>>> + * decide if the zone is saturated or not, making the zone relevant or not.\n>>\n>> What happens to saturated cells?\n>>\n>> What is a zone, is that a grid cell?\n>>\n>>> + *\n>>> + * The Grey World algorithm will then estimate the red and blue gains to apply, and\n>>> + * send those back through metadata.\n>>\n>> /send those back through/store the results in the/\n>>\n>>> + */\n>>> \n>>> /**\n>>> - * \\struct IspStatsRegion\n>>> + * \\struct StatsRegion\n>>\n>> Renaming a structure should be an independent patch.\n>>\n>>\n>>> * \\brief RGB statistics for a given region\n>>\n>> Is a region a cell? or a zone? or just a defined area that comprises of\n>> ... some pixels?\n>>\n>> It's not clear which terms define which concepts.\n>>\n>>\n>>> *\n>>> - * The IspStatsRegion structure is intended to abstract the ISP specific\n>>> - * statistics and use an agnostic algorithm to compute AWB.\n>>> + * The StatsRegion structure is intended to abstract the ISP specific\n>>\n>> Is this just a good intention? I suspect it either does it, or it doesn't.\n>>\n>> \"The StatsRegions structure abstracts the ISP specific...\"\n>>\n>>\n>>> + * statistics to compute AWB. The Grey World algorithm uses an average\n>>\n>> an average of what?\n>>\n>>\n>>\n>>> + * for a specific counted pixels. \n>>\n>> oh, perhaps that was \"an average of the pixels in a given region.\"\n>>\n>>\n>>> When a specific zone in the scene is\n>>> + * saturated, we want to exclude it from the calculation, and consider\n>>> + * it as an outlier.\n>>\n>> aha, ok - that answers an earlier question.\n>>\n>>\n>>> *\n>>> - * \\var IspStatsRegion::counted\n>>> - * \\brief Number of pixels used to calculate the sums\n>>> + * \\var StatsRegion::counted\n>>> + * \\brief Number of unsatured pixels used to calculate the sums\n>>\n>> s/unsatured/unsaturated/\n>>\n>> Oh - So it distinguishes between a pixel being saturated, not a zone\n>> containing saturat\n>>\n>>\n>>> *\n>>> - * \\var IspStatsRegion::uncounted\n>>> - * \\brief Remaining number of pixels in the region\n>>> + * \\var StatsRegion::uncounted\n>>> + * \\brief Remaining number of pixels in the region (ie saturated)\n>>> *\n>>> - * \\var IspStatsRegion::rSum\n>>> + * \\var StatsRegion::rSum\n>>> * \\brief Sum of the red values in the region\n>>> *\n>>> - * \\var IspStatsRegion::gSum\n>>> + * \\var StatsRegion::gSum\n>>> * \\brief Sum of the green values in the region\n>>> *\n>>> - * \\var IspStatsRegion::bSum\n>>> + * \\var StatsRegion::bSum\n>>> * \\brief Sum of the blue values in the region\n>>> */\n>>> \n>>> @@ -48,26 +73,35 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n>>> * \\struct AwbStatus\n>>> * \\brief AWB parameters calculated\n>>> *\n>>> - * The AwbStatus structure is intended to store the AWB\n>>> - * parameters calculated by the algorithm\n>>> + * The AwbStatus structure is intended to store the AWB parameters\n>>\n>> I think we should declare what it does, not an intention.\n>>\n>>> + * calculated by the algorithm, and shared through the metadata\n>>> + * object, for other algorithms\n>>\n>> s/algorithms/algorithms./\n>>\n>>\n>>> *\n>>> * \\var AwbStatus::temperatureK\n>>> - * \\brief Color temperature calculated\n>>> + * \\brief Color temperature calculated, in Kelvin\n>>> *\n>>> * \\var AwbStatus::redGain\n>>> - * \\brief Gain calculated for the red channel\n>>> + * \\brief Gain calculated for the red channel. This is a floating-point\n>>> + * value used as a multiplier on the ISP side\n>>> *\n>>> * \\var AwbStatus::greenGain\n>>> - * \\brief Gain calculated for the green channel\n>>> + * \\brief Gain calculated for the green channel. This is a floating-point\n>>> + * value used as a multiplier on the ISP side\n>>> *\n>>> * \\var AwbStatus::blueGain\n>>> - * \\brief Gain calculated for the blue channel\n>>> + * \\brief Gain calculated for the blue channel. This is a floating-point\n>>> + * value used as a multiplier on the ISP side\n>>> */\n>>> \n>>> /**\n>>> * \\struct Ipu3AwbCell\n>>> * \\brief Memory layout for each cell in AWB metadata\n>>> *\n>>> + * This is the internal layout on IPU3 for one cell of AWB statistics.\n>>> + * There is ipu3_uapi_awb_config_s->grid.width * 2^block_width_log2 per\n>>\n>> /There is/There are/\n>>\n>> But this isn't very readable or clear :S\n>>\n>>> + * ipu3_uapi_awb_config_s->grid.height * 2^block_height_log2 cells for\n>>> + * one frame statistics.\n>>\n>> I'm not sure I can correctly interpret it to rewrite it at the moment...\n>>\n>> In particular 'per' is throwing me off.\n>>\n>> I presume this is describing that the grid has a width and a height? and\n>> that the number of cells is ... width * height, but width and height are\n>> stored as the log2 ?\n>>\n> \n> Yes. How should it be reworded :-) ?\n\nThat's not easy for me to do, as I haven't understood the existing\ndescription, and I don't know what a cell is to document it myself.\n\nI think the documentation summary for the struct needs to define or\ndescribe some or all of the following:\n\n - [1] What the structure is\n - [2] What the structure is used for\n - [3] What the structure needs\n - [4] How the structure is used by other parts.\n\nI do not know if any of the following is true or accurate, so please do\nnot directly copy and paste this. It's just an effort to show how this\nstructure could be documented.\n\n\n\"\"\"\nAn Ipu3AwbCell represents one sector of the statistics data of the image\nwhen divided into a grid by the ISP.\n\nThe cell reports the statistics from a rectangular region of the image\ngiving an average for each colour component within that region, along\nwith a saturation ratio, which details how many of the pixels were\nsaturated beyond the saturation threshold (if enabled).\n\nThe cell size is defined as part of the grid configuration, where the\nthe grid size is defined through the ipu3_uapi_awb_config_s structure.\n\nOn the IPU3, the cell width and height must be a multiple of 2, and\ngreater than 16.\n\"\"\"\n\n\nIt sort of feels like that's documentation and a structure that should\nhave been provided as part of the IPU3 kernel driver though ;-) - but if\nwe declare the structure here, then we should document what we know\nabout it.\n\n\n>>> + *\n>>> * The Ipu3AwbCell structure is used to get individual values\n>>> * such as red average or saturation ratio in a particular cell.\n>>> *\n>>> @@ -84,7 +118,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n>>> * \\brief Green average for blue lines\n>>> *\n>>> * \\var Ipu3AwbCell::satRatio\n>>> - * \\brief Saturation ratio in the cell\n>>> + * \\brief Saturation ratio in the cell. It depends on the rgbs_thr_* values, and\n>>> + * will be set if rgbs_thr_b has IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit set.\n>>\n>> \"Saturation ratio of the cell\" sounds like the brief, and the expanded\n>> sentences sound like something that would be a new line to make it\n>> render as more context beyond the brief.\n>>\n>> But I'd express this then as:\n>>\n>> \"\"\"\n>> The saturation ratio is determined based upon the threshold values set\n>> in rgbs_thr_*, and will be set if the threshold value has the\n>> corresponding enable bit set in the (Where is it set?)..\n>>\n>> For example, to obtain the Blue saturation value, the\n>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT bit must be set in rgbs_thr_b.\n>> \"\"\"\n>>\n>> (But it might be good to reference what rgbs_thr_b is and where it can\n>> be set perhaps ...)\n>>\n>>\n>>\n>>\n>>> *\n>>> * \\var Ipu3AwbCell::padding\n>>> * \\brief array of unused bytes for padding\n>>> @@ -159,6 +194,9 @@ const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {\n>>> \t7807, 7871, 7935, 7999, 8063, 8127, 8191\n>>> } };\n>>> \n>>> +/* Minimum level of green (on a 8 bits base) in a given zone */\n>>\n>> What is an 8 bits base?\n>>\n>> Do you mean it must be a multiple of 8?\n>> What happens if it isn't? Is it rounded up? down?\n> \n> It means that the value has a maximum of 256, so it is not really useful\n> to say anything about it.\n> \n> +/* Minimum level of green in a given zone */\n> \n>>\n>>> +static constexpr uint32_t kMinGreenLevelInZone = 16;\n>>> +\n>>> IPU3Awb::IPU3Awb()\n>>> \t: Algorithm()\n>>> {\n>>> @@ -166,6 +204,7 @@ IPU3Awb::IPU3Awb()\n>>> \tasyncResults_.greenGain = 1.0;\n>>> \tasyncResults_.redGain = 1.0;\n>>> \tasyncResults_.temperatureK = 4500;\n>>> +\tminZonesCounted_ = 0;\n>>> }\n>>> \n>>> IPU3Awb::~IPU3Awb()\n>>> @@ -202,7 +241,7 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n>>> \tparams.acc_param.gamma.gc_lut = imguCssGammaLut;\n>>> \tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>>> \n>>> -\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>>> +\tzones_.reserve(kAwbStatsSize);\n>>> }\n>>> \n>>> /**\n>>> @@ -238,10 +277,10 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)\n>>> /* Generate an RGB vector with the average values for each region */\n>>> void IPU3Awb::generateZones(std::vector &zones)\n>>> {\n>>> -\tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n>>> +\tfor (unsigned int i = 0; i < kAwbStatsSize; i++) {\n>>> \t\tRGB zone;\n>>> \t\tdouble counted = awbStats_[i].counted;\n>>> -\t\tif (counted >= kMinZonesCounted) {\n>>> +\t\tif (counted >= minZonesCounted_) {\n>>> \t\t\tzone.G = awbStats_[i].gSum / counted;\n>>> \t\t\tif (zone.G >= kMinGreenLevelInZone) {\n>>> \t\t\t\tzone.R = awbStats_[i].rSum / counted;\n>>> @@ -258,6 +297,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n>>> \tuint32_t regionWidth = round(awbGrid_.width / static_cast(kAwbStatsSizeX));\n>>> \tuint32_t regionHeight = round(awbGrid_.height / static_cast(kAwbStatsSizeY));\n>>> \n>>> +\tminZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;\n>>\n>> Where does 4/5 come from here?\n> \n> This is a way to say 80% :-p.\n> It is the minimum proportion of pixels counted within AWB region for it\n> to be relevant. But this value is empirical.\n\n\nIt's fine to be an empirical value, but it should be documented as such,\nand perhaps even guidance on how to tweak it if desired (i.e., why was\nthis particular value selected, what happens if you increase it,\ndecrease it).\n\n\n> \n>>\n>>> \t/*\n>>> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n>>> \t * (awbGrid_.width x awbGrid_.height).\n>>> @@ -269,7 +309,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n>>> \t\t\tuint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;\n>>> \n>>> \t\t\tuint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;\n>>> -\t\t\tcellPosition *= 8;\n>>> +\t\t\tcellPosition *= sizeof(Ipu3AwbCell);\n>>\n>> Is this a bug fix? or because of the move to metadata? Looks like it\n>> almost deserves it's own patch ? I can't quite tell.\n>>\n>>\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>>> @@ -287,7 +327,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n>>> \n>>> void IPU3Awb::clearAwbStats()\n>>> {\n>>> -\tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n>>> +\tfor (unsigned int i = 0; i < kAwbStatsSize; i++) {\n>>> \t\tawbStats_[i].bSum = 0;\n>>> \t\tawbStats_[i].rSum = 0;\n>>> \t\tawbStats_[i].gSum = 0;\n>>> @@ -344,24 +384,38 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>>> \tgenerateAwbStats(stats);\n>>> \tgenerateZones(zones_);\n>>> \tLOG(IPU3Awb, Debug) << \"Valid zones: \" << zones_.size();\n>>> -\tif (zones_.size() > 10) {\n>>> +\n>>> +\t/* We need at least 5% of valid zones to estimate the gain correction */\n>>\n>> 5 sounds quite low? Is that determined from empirical results? or just a\n>> rough estimate?\n>>\n>> What effect does increasing this ratio have?\n> \n> If you increase it too much you will have to wait longer for the AWB to\n> start correcting the colors. And if the AGC is \"slow\" then you may\n> experience weird colors before it is locked.\n\nOk - more descriptions that deserve a local comment then, like the previous.\n\n\n\n>>> +\tif (zones_.size() > kAwbStatsSize / 20) {\n>>> \t\tawbGreyWorld();\n>>> \t\tLOG(IPU3Awb, Debug) << \"Gain found for red: \" << asyncResults_.redGain\n>>> \t\t\t\t << \" and for blue: \" << asyncResults_.blueGain;\n>>> \t}\n>>> }\n>>> \n>>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma)\n>>> +void IPU3Awb::process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata)\n>>> +{\n>>> +\tcalculateWBGains(stats);\n>>> +\t/* We need to update the AWB status, to give back the gains */\n>>> +\timageMetadata->set(\"awb.status\", asyncResults_);\n>>\n>> I still think these look like results not status ;)\n>>\n>> Do we need to define a naming scheme for the metadata keys?\n> \n> I will add the name used in the s/AwbStatus/AwbResults structure\n> documentation. I don't really care using awb.status or awb.results.\n\nI hear from Laurent he may also prefer an integer key. I like the sound\nof that as it means we can define the storage elements as an enum at\nleast by name.\n\nI would expect that in turn would save on either the string compares or\nthe hash function of the map (though, even integers I presume would get\nhashed for a map).\n\nBut at least it defines what entries 'can' go into the metadata somewhere.\n\nThat would then need a centralised header that all users of the metadata\ninclude to define those keys though. I'm not sure if I see that as a\nbenefit or a compromise yet ;-)\n\n\n>>\n>>\n>>> +}\n>>> +\n>>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, Metadata *imageMetadata)\n>>> {\n>>> \t/*\n>>> \t * Green gains should not be touched and considered 1.\n>>> \t * Default is 16, so do not change it at all.\n>>> -\t * 4096 is the value for a gain of 1.0\n>>> +\t * 8192 is the value for a gain of 1.0\n>>> \t */\n>>> -\tparams.acc_param.bnr.wb_gains.gr = 16;\n>>> -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n>>> -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n>>> -\tparams.acc_param.bnr.wb_gains.gb = 16;\n>>> +\tparams.acc_param.bnr.wb_gains.gr = 8192;\n>>> +\tparams.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;\n>>> +\tparams.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;\n>>> +\tparams.acc_param.bnr.wb_gains.gb = 8192;\n>>\n>> Is this change part of using the metadata?\n>>\n>> We're going from 4096 to 8192. That sounds like a specific change worthy\n>> of it's own commit message to explain why this change occurs ?\n> \n> Yes.\n> \n>>\n>>\n>>> +\n>>> +\t/* When the AGC algorithm has run, it may have set a new gamma */\n>>> +\tdouble agcGamma = 1.0;\n>>> +\tif (imageMetadata->get(\"agc.gamma\", agcGamma) != 0)\n>>> +\t\tLOG(IPU3Awb, Debug) << \"Awb: no gamma found, defaulted to 1.0\";\n>>> \n>>> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK\n>>> \t\t\t << \" and gamma calculated: \" << agcGamma;\n>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>>> index 122cf68c..cf12032d 100644\n>>> --- a/src/ipa/ipu3/ipu3_awb.h\n>>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>>> @@ -14,14 +14,18 @@\n>>> #include \n>>> \n>>> #include \"libipa/algorithm.h\"\n>>> +#include \"libipa/metadata.h\"\n>>> \n>>> namespace libcamera {\n>>> \n>>> namespace ipa::ipu3 {\n>>> \n>>> -/* Region size for the statistics generation algorithm */\n>>> +/* Width of the AWB regions used for Grey World calculation */\n>>> static constexpr uint32_t kAwbStatsSizeX = 16;\n>>> +/* Height of the AWB regions used for Grey World calculation */\n>>> static constexpr uint32_t kAwbStatsSizeY = 12;\n>>> +/* Total size of the AWB regions used for Grey World calculation */\n>>> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY;\n>>> \n>>> class IPU3Awb : public Algorithm\n>>\n>> Should this be named IPU3AwbGreyworld?\n>>\n>>\n>> Presumably we might potentially have another Awb implementation which\n>> would run a different algorithm?\n>>\n>> (I wonder then what the implications are for switching between the\n>> algorithms in that case).\n>>\n>> But perhaps we should leave that name as is for now until we have a more\n>> modular construction for the independent algorithms.\n>>\n> \n> I think we should not name it differently for now, as we don't have\n> another algorithm yet :-).\n> \n>>\n>>> {\n>>> @@ -30,17 +34,8 @@ public:\n>>> \t~IPU3Awb();\n>>> \n>>> \tvoid initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n>>> -\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>>> -\tvoid updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma);\n>>> -\n>>> -\tstruct Ipu3AwbCell {\n>>> -\t\tunsigned char greenRedAvg;\n>>> -\t\tunsigned char redAvg;\n>>> -\t\tunsigned char blueAvg;\n>>> -\t\tunsigned char greenBlueAvg;\n>>> -\t\tunsigned char satRatio;\n>>> -\t\tunsigned char padding[3];\n>>> -\t} __attribute__((packed));\n>>> +\tvoid process(const ipu3_uapi_stats_3a *stats, Metadata *imageMetadata);\n>>> +\tvoid updateWbParameters(ipu3_uapi_params ¶ms, Metadata *imageMetadata);\n>>> \n>>> \t/* \\todo Make these three structs available to all the ISPs ? */\n>>> \tstruct RGB {\n>>> @@ -56,7 +51,7 @@ public:\n>>> \t\t}\n>>> \t};\n>>> \n>>> -\tstruct IspStatsRegion {\n>>> +\tstruct StatsRegion {\n>>> \t\tunsigned int counted;\n>>> \t\tunsigned int uncounted;\n>>> \t\tunsigned long long rSum;\n>>> @@ -71,18 +66,29 @@ public:\n>>> \t\tdouble blueGain;\n>>> \t};\n>>> \n>>> +\tstruct Ipu3AwbCell {\n>>> +\t\tunsigned char greenRedAvg;\n>>> +\t\tunsigned char redAvg;\n>>> +\t\tunsigned char blueAvg;\n>>> +\t\tunsigned char greenBlueAvg;\n>>> +\t\tunsigned char satRatio;\n>>> +\t\tunsigned char padding[3];\n>>> +\t};\n>>> +\n>>> private:\n>>> \tvoid generateZones(std::vector &zones);\n>>> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n>>> \tvoid clearAwbStats();\n>>> \tvoid awbGreyWorld();\n>>> \tuint32_t estimateCCT(double red, double green, double blue);\n>>> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>>> \n>>> \tstruct ipu3_uapi_grid_config awbGrid_;\n>>> \n>>> \tstd::vector zones_;\n>>> -\tIspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n>>> +\tStatsRegion awbStats_[kAwbStatsSize];\n>>> \tAwbStatus asyncResults_;\n>>> +\tuint32_t minZonesCounted_;\n>>> };\n>>> \n>>> } /* namespace ipa::ipu3 */\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 B56C3C3226\n\tfor ;\n\tWed, 14 Jul 2021 11:21:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0396268506;\n\tWed, 14 Jul 2021 13:21: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 5F4BC68503\n\tfor ;\n\tWed, 14 Jul 2021 13:21:03 +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 C8927CC;\n\tWed, 14 Jul 2021 13:21:02 +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=\"wOPyIHu2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626261663;\n\tbh=lhuRMXtAD9uiWoDDcnEFkDGXl0eCEBQby7U5a00XRL4=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=wOPyIHu2OS0NQTAgM35AhdC/zAWiftyWKGMKRC1iyIUwSDWTzuhnVbwCm6PpogT31\n\tZsquiNI6yYMKsW5OSaP64Q9dmpMvibbxs5trWZ2wPioQHt7GSvaPrFLr7E2+eyTPDb\n\t5j4UIGSdDHLAZp8d7xFTrjkAW6lFprcEugUZL8tU=","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210712131630.73914-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210712131630.73914-3-jeanmichel.hautbois@ideasonboard.com>\n\t<961dac7f-2b59-860a-5b5e-04077445d02c@ideasonboard.com>\n\t<66cba20c-ae76-45a4-3b4d-87acb5842609@ideasonboard.com>","Message-ID":"","Date":"Wed, 14 Jul 2021 12:20:58 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<66cba20c-ae76-45a4-3b4d-87acb5842609@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] ipa: ipu3: Use metadata and\n\timprove the doc","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\" "}}]