[{"id":26819,"web_url":"https://patchwork.libcamera.org/comment/26819/","msgid":"<20230403135947.idhctz72b5b44xs7@uno.localdomain>","date":"2023-04-03T13:59:47","subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Fri, Mar 31, 2023 at 10:19:27AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Add platform related code for configuring auto focus window on the\n> rkisp1. Connect to the windowUpdateRequested() signal exposed by the\n> AfHillClimbing and configure the window on each signal emission.\n> This enables support of AfMetering and AfWindows controls on the rkisp1\n> platform.\n>\n> Currently, only one window is enabled, but ISP allows up to three\n> of them.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> src/ipa/rkisp1/algorithms/af.cpp | 64 +++++++++++++++++++++++++++++++-\n> src/ipa/rkisp1/algorithms/af.h | 13 ++++++-\n> 2 files changed, 75 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\n> index fde924d4..b6f6eee4 100644\n> --- a/src/ipa/rkisp1/algorithms/af.cpp\n> +++ b/src/ipa/rkisp1/algorithms/af.cpp\n> @@ -32,16 +32,43 @@ namespace ipa::rkisp1::algorithms {\n> * amount of time on each movement. This parameter should be set according\n> * to the worst case - the number of frames it takes to move lens between\n> * limit positions.\n> + * - **isp-threshold:** Threshold used for minimizing the influence of noise.\n> + * This affects the ISP sharpness calculation.\n> + * - **isp-var-shift:** The number of bits for the shift operation at the end\n> + * of the calculation chain. This affects the ISP sharpness calculation.\n\nI wonder if the introduction of these values belong to this patch or\nnot. I guess they affect the AF algorithm globally, as a default\nwindow has to be programmed to have it working (something that happens in this\npatch, you might say :)\n\nRegardless of that, have you been able to identify with a little more\naccuracy how these values should be computed ? For the threshold I\nguess the explanation is somewhat clear, but the var-shift parameter I\nwonder how one should compute it ? As I read it, the var-shift value\nrepresents the number number of bits to right-shift the pixel values\nbefore summing them to avoid overflows of the afm_sum register (32\nbits). How did you come up with a value of 4 in your configuration\nfile ? Does this value depend on the window size or does it depend on\nthe sensor in use ?\n\n> * .\n> * \\sa libcamera::ipa::algorithms::AfHillClimbing for additional tuning\n> * parameters.\n> *\n> * \\todo Model the lens delay as number of frames required for the lens position\n> * to stabilize in the CameraLens class.\n> + * \\todo Check if requested window size is valid. RkISP supports AF window size\n> + * few pixels smaller than sensor output size.\n> + * \\todo Implement support for all available AF windows. RkISP supports up to 3\n> + * AF windows.\n> */\n>\n> LOG_DEFINE_CATEGORY(RkISP1Af)\n>\n> +namespace {\n> +\n> +constexpr rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)\n> +{\n> +\treturn rkisp1_cif_isp_window{\n> +\t\t.h_offs = static_cast(rectangle.x),\n> +\t\t.v_offs = static_cast(rectangle.y),\n> +\t\t.h_size = static_cast(rectangle.width),\n> +\t\t.v_size = static_cast(rectangle.height)\n> +\t};\n> +}\n> +\n> +} /* namespace */\n> +\n> +Af::Af()\n> +{\n> +\taf.windowUpdateRequested.connect(this, &Af::updateCurrentWindow);\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::init\n> */\n> @@ -54,8 +81,12 @@ int Af::init(IPAContext &context, const YamlObject &tuningData)\n> \t}\n>\n> \twaitFramesLens_ = tuningData[\"wait-frames-lens\"].get(1);\n> +\tispThreshold_ = tuningData[\"isp-threshold\"].get(128);\n> +\tispVarShift_ = tuningData[\"isp-var-shift\"].get(4);\n>\n> -\tLOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_;\n> +\tLOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_\n> +\t\t\t << \", ispThreshold_: \" << ispThreshold_\n> +\t\t\t << \", ispVarShift_: \" << ispVarShift_;\n>\n> \treturn af.init(lensConfig->minFocusPosition,\n> \t\t lensConfig->maxFocusPosition, tuningData);\n> @@ -89,6 +120,32 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,\n> \taf.queueRequest(controls);\n> }\n>\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Af::prepare([[maybe_unused]] IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t rkisp1_params_cfg *params)\n> +{\n> +\tif (updateWindow_) {\n\nor\n if (!updateWindow_)\n return;\n\n> +\t\tparams->meas.afc_config.num_afm_win = 1;\n> +\t\tparams->meas.afc_config.thres = ispThreshold_;\n> +\t\tparams->meas.afc_config.var_shift = ispVarShift_;\n> +\t\tparams->meas.afc_config.afm_win[0] =\n> +\t\t\trectangleToIspWindow(*updateWindow_);\n> +\n> +\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> +\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;\n> +\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> +\n> +\t\tupdateWindow_.reset();\n> +\n> +\t\t/* Wait one frame for the ISP to apply changes. */\n> +\t\taf.skipFrames(1);\n> +\t}\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::process\n> */\n> @@ -114,6 +171,11 @@ void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t}\n> }\n>\n> +void Af::updateCurrentWindow(const Rectangle &window)\n> +{\n> +\tupdateWindow_ = window;\n> +}\n> +\n> REGISTER_IPA_ALGORITHM(Af, \"Af\")\n>\n> } /* namespace ipa::rkisp1::algorithms */\n> diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\n> index 3ba66d38..6f5adb19 100644\n> --- a/src/ipa/rkisp1/algorithms/af.h\n> +++ b/src/ipa/rkisp1/algorithms/af.h\n> @@ -20,21 +20,32 @@ namespace ipa::rkisp1::algorithms {\n> class Af : public Algorithm\n> {\n> public:\n> +\tAf();\n> +\n> \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> \tint configure(IPAContext &context,\n> \t\t const IPACameraSensorInfo &configInfo) override;\n> \tvoid queueRequest(IPAContext &context, uint32_t frame,\n> \t\t\t IPAFrameContext &frameContext,\n> \t\t\t const ControlList &controls) override;\n> +\tvoid prepare(IPAContext &context, uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t rkisp1_params_cfg *params) override;\n> \tvoid process(IPAContext &context, uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const rkisp1_stat_buffer *stats,\n> \t\t ControlList &metadata) override;\n>\n> private:\n> +\tvoid updateCurrentWindow(const Rectangle &window);\n> +\n> \tipa::algorithms::AfHillClimbing af;\n>\n> -\t/* Wait number of frames after changing lens position */\n> +\tstd::optional updateWindow_;\n> +\tuint32_t ispThreshold_ = 0;\n> +\tuint32_t ispVarShift_ = 0;\n> +\n> +\t/* Wait number of frames after changing lens position. */\n> \tuint32_t waitFramesLens_ = 0;\n> };\n>\n> --\n> 2.39.2\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 92C4DC0F2A\n\tfor ;\n\tMon, 3 Apr 2023 13:59:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE2D462724;\n\tMon, 3 Apr 2023 15:59:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78DE0626E3\n\tfor ;\n\tMon, 3 Apr 2023 15:59:50 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F07034A7;\n\tMon, 3 Apr 2023 15:59:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680530391;\n\tbh=rQkIDb2dENZ37g3gKJPt0H43MRFkwNlRTolFt+sX7To=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ySlRsGoYF/ZXDPLdILjAVTfH0O5Ml4F/Mhwf5Qt58p2MI/rvbTzw0pX26ZuHnJXla\n\tHSsPMTRVvT9TVC8IN+uXW8hn/16ZyAOEbO6idPgy1ZFLsKfigCCRlHFWrCmn59udeo\n\t594MliZX6dWQ1Pbl7YGm1qs6x2W1wSeNvAj0jnZ8Ewl0OyHpVcmTh4q7Od0aWU9CBP\n\tfv+4mvV0IHZV8zCQaXcJlJwky1HIME7ZhOMlSyHHaGYk2CBpXsygDhQDi/c0UWEfWo\n\tQD1rlPOlIdbB42Nrq0c4O79nh0WX/iSwomcsulqNnbW8o6BfPTcexHVQEZB0o6LSdf\n\trWfOkOVkA+rZg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680530390;\n\tbh=rQkIDb2dENZ37g3gKJPt0H43MRFkwNlRTolFt+sX7To=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gP6ZNpBeNOfxnjXJ6RTJrnaJgxmS5YTWNDOkjRKtSPykTq0VxuuHZXujcI1D4f5oH\n\tZr/VMNaxKNDkuf1oIwBvkMhLcTtuGFY5FfrReC3GrKxnTxjrijqswTznjFEHYQLr54\n\tHWhbWu0inwTQUwXX+nM20lw1ULskB6QUpN85b5MI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gP6ZNpBe\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 15:59:47 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230403135947.idhctz72b5b44xs7@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-8-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230331081930.19289-8-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26835,"web_url":"https://patchwork.libcamera.org/comment/26835/","msgid":"","date":"2023-04-04T09:39:46","subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 3, 2023 at 3:59 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n>\n> On Fri, Mar 31, 2023 at 10:19:27AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Add platform related code for configuring auto focus window on the\n> > rkisp1. Connect to the windowUpdateRequested() signal exposed by the\n> > AfHillClimbing and configure the window on each signal emission.\n> > This enables support of AfMetering and AfWindows controls on the rkisp1\n> > platform.\n> >\n> > Currently, only one window is enabled, but ISP allows up to three\n> > of them.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > src/ipa/rkisp1/algorithms/af.cpp | 64 +++++++++++++++++++++++++++++++-\n> > src/ipa/rkisp1/algorithms/af.h | 13 ++++++-\n> > 2 files changed, 75 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\n> > index fde924d4..b6f6eee4 100644\n> > --- a/src/ipa/rkisp1/algorithms/af.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/af.cpp\n> > @@ -32,16 +32,43 @@ namespace ipa::rkisp1::algorithms {\n> > * amount of time on each movement. This parameter should be set according\n> > * to the worst case - the number of frames it takes to move lens between\n> > * limit positions.\n> > + * - **isp-threshold:** Threshold used for minimizing the influence of noise.\n> > + * This affects the ISP sharpness calculation.\n> > + * - **isp-var-shift:** The number of bits for the shift operation at the end\n> > + * of the calculation chain. This affects the ISP sharpness calculation.\n>\n> I wonder if the introduction of these values belong to this patch or\n> not. I guess they affect the AF algorithm globally, as a default\n> window has to be programmed to have it working (something that happens in this\n> patch, you might say :)\n\nYou are right, these parameters should be moved to the previous commit.\nI found that if no AF window was configured, ISP has a default one\n(probably maximum AF window size), but this is not documented. This\nway rkisp1 AF algo should work even without this commit.\n\n>\n> Regardless of that, have you been able to identify with a little more\n> accuracy how these values should be computed ? For the threshold I\n> guess the explanation is somewhat clear, but the var-shift parameter I\n> wonder how one should compute it ? As I read it, the var-shift value\n> represents the number number of bits to right-shift the pixel values\n> before summing them to avoid overflows of the afm_sum register (32\n> bits). How did you come up with a value of 4 in your configuration\n> file ? Does this value depend on the window size or does it depend on\n> the sensor in use ?\n\nI came up with 4 just by experimenting with my setup. It worked even\nwith 0, but I shifted it a little bit for safety.\nThere is very little documentation on this, so unfortunately I know\nonly as much as you.\n\n>\n> > * .\n> > * \\sa libcamera::ipa::algorithms::AfHillClimbing for additional tuning\n> > * parameters.\n> > *\n> > * \\todo Model the lens delay as number of frames required for the lens position\n> > * to stabilize in the CameraLens class.\n> > + * \\todo Check if requested window size is valid. RkISP supports AF window size\n> > + * few pixels smaller than sensor output size.\n> > + * \\todo Implement support for all available AF windows. RkISP supports up to 3\n> > + * AF windows.\n> > */\n> >\n> > LOG_DEFINE_CATEGORY(RkISP1Af)\n> >\n> > +namespace {\n> > +\n> > +constexpr rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)\n> > +{\n> > + return rkisp1_cif_isp_window{\n> > + .h_offs = static_cast(rectangle.x),\n> > + .v_offs = static_cast(rectangle.y),\n> > + .h_size = static_cast(rectangle.width),\n> > + .v_size = static_cast(rectangle.height)\n> > + };\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > +Af::Af()\n> > +{\n> > + af.windowUpdateRequested.connect(this, &Af::updateCurrentWindow);\n> > +}\n> > +\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::init\n> > */\n> > @@ -54,8 +81,12 @@ int Af::init(IPAContext &context, const YamlObject &tuningData)\n> > }\n> >\n> > waitFramesLens_ = tuningData[\"wait-frames-lens\"].get(1);\n> > + ispThreshold_ = tuningData[\"isp-threshold\"].get(128);\n> > + ispVarShift_ = tuningData[\"isp-var-shift\"].get(4);\n> >\n> > - LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_;\n> > + LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_\n> > + << \", ispThreshold_: \" << ispThreshold_\n> > + << \", ispVarShift_: \" << ispVarShift_;\n> >\n> > return af.init(lensConfig->minFocusPosition,\n> > lensConfig->maxFocusPosition, tuningData);\n> > @@ -89,6 +120,32 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,\n> > af.queueRequest(controls);\n> > }\n> >\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > +void Af::prepare([[maybe_unused]] IPAContext &context,\n> > + [[maybe_unused]] const uint32_t frame,\n> > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > + rkisp1_params_cfg *params)\n> > +{\n> > + if (updateWindow_) {\n>\n> or\n> if (!updateWindow_)\n> return;\n>\n> > + params->meas.afc_config.num_afm_win = 1;\n> > + params->meas.afc_config.thres = ispThreshold_;\n> > + params->meas.afc_config.var_shift = ispVarShift_;\n> > + params->meas.afc_config.afm_win[0] =\n> > + rectangleToIspWindow(*updateWindow_);\n> > +\n> > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> > + params->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;\n> > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> > +\n> > + updateWindow_.reset();\n> > +\n> > + /* Wait one frame for the ISP to apply changes. */\n> > + af.skipFrames(1);\n> > + }\n> > +}\n> > +\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::process\n> > */\n> > @@ -114,6 +171,11 @@ void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > }\n> > }\n> >\n> > +void Af::updateCurrentWindow(const Rectangle &window)\n> > +{\n> > + updateWindow_ = window;\n> > +}\n> > +\n> > REGISTER_IPA_ALGORITHM(Af, \"Af\")\n> >\n> > } /* namespace ipa::rkisp1::algorithms */\n> > diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\n> > index 3ba66d38..6f5adb19 100644\n> > --- a/src/ipa/rkisp1/algorithms/af.h\n> > +++ b/src/ipa/rkisp1/algorithms/af.h\n> > @@ -20,21 +20,32 @@ namespace ipa::rkisp1::algorithms {\n> > class Af : public Algorithm\n> > {\n> > public:\n> > + Af();\n> > +\n> > int init(IPAContext &context, const YamlObject &tuningData) override;\n> > int configure(IPAContext &context,\n> > const IPACameraSensorInfo &configInfo) override;\n> > void queueRequest(IPAContext &context, uint32_t frame,\n> > IPAFrameContext &frameContext,\n> > const ControlList &controls) override;\n> > + void prepare(IPAContext &context, uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + rkisp1_params_cfg *params) override;\n> > void process(IPAContext &context, uint32_t frame,\n> > IPAFrameContext &frameContext,\n> > const rkisp1_stat_buffer *stats,\n> > ControlList &metadata) override;\n> >\n> > private:\n> > + void updateCurrentWindow(const Rectangle &window);\n> > +\n> > ipa::algorithms::AfHillClimbing af;\n> >\n> > - /* Wait number of frames after changing lens position */\n> > + std::optional updateWindow_;\n> > + uint32_t ispThreshold_ = 0;\n> > + uint32_t ispVarShift_ = 0;\n> > +\n> > + /* Wait number of frames after changing lens position. */\n> > uint32_t waitFramesLens_ = 0;\n> > };\n> >\n> > --\n> > 2.39.2\n> >\n\nBest regards\nDaniel","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 BFC9FC0F2A\n\tfor ;\n\tTue, 4 Apr 2023 09:40:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFA036271C;\n\tTue, 4 Apr 2023 11:39:59 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 230E7626DA\n\tfor ;\n\tTue, 4 Apr 2023 11:39:58 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id i5so128253873eda.0\n\tfor ;\n\tTue, 04 Apr 2023 02:39:58 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680601199;\n\tbh=29Z3LflWQUjhrPiPOm3Lwlgn+gFr44Vdhr+K6zladSs=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=EAJefYzkW+luEC5aBqTAMr4HIbLLC8KsHYyY1z6Hfsy5bmoxexjon7mnhcmJgnkPB\n\t+9O+wk+HxW9810BfCsWmm7w6VhBmSIJ2qutTFJCyEY4v2aKIFrKbwpL/6vvBboaemM\n\t6pPn28cRKHxtUU+1e4/H1ZEIbHyYFaWxrxan11EmxgG2UMl/n9hIrIqRracZEGVXhv\n\twpJMBpjmNHKqfyp1TJR+SKGyHm4+pqmxImuqwfbpZSlrh4z3/x6TDps6hFKwTRRRha\n\t56Fcd/bkBlmTaqtsTOC2MDPZbzTXGNjKPb3GTIpJAY8O7kJiYfm4RDEoh+KQmcKTES\n\ts3HzNejSnJSBA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1680601197;\n\tx=1683193197; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=XgClHsNsqMoWLlobwFI0oQpm7ywMwb68qeRyehLAwNc=;\n\tb=gk2bKBot2wIBaWKlYrOO8c7kpwe4If7hPGGmRHvtM8GttIR5uACS0iYwSssopcvvCO\n\tPiv61ET351hWkZzw8+boSLF4/ipWNGrt16Hq1hEualIlYMokhcdtQKxk99JUw+MOi5Wd\n\tJW/le1PbpgoKv0bxhzz/o+f3jBcgPMgWdloJkll/zWc5fsj7RcXSxtQe+n/rpokdkwWA\n\tYLugpEaj/i0+OI0j2U/n2gO6vFZqBVfxn0RIkRhYZNR8XGQK0kiyEgb241RpSiVpr0R3\n\tpyb85xVRccWk0ermp7DnJCgOA+7fmQFMgi4j9KGOV+V0S5Jrx3/3jRhmMsOLqFLUlrOV\n\tF+LA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20210112.gappssmtp.com\n\theader.i=@thaumatec-com.20210112.gappssmtp.com header.b=\"gk2bKBot\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680601197; x=1683193197;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=XgClHsNsqMoWLlobwFI0oQpm7ywMwb68qeRyehLAwNc=;\n\tb=lfL/2fnAudeM15fYmldFBBFjtP4+KhXrdRwzqkBON6fEz4OfJfUskz8+I1fl2t5lTw\n\td1cNFgrcKJHLtgL/pKa6rACgQM5ce9sVMqpqDD3Sasg6eiDViGWLersAXbIyrCJTLjm5\n\tbvyEQi33kCTOxlBhaQcNSxMoPoMa3X3WkICHWetR9uVBlc3GAZmqMcgMoaAJ0c20nlqF\n\tos6J68TJQG0MUoSKDvCEkQkJoIGrYZodTKrWQCPpCO2qrS0aqXBx8k+7lioY2OcEZPcH\n\tBhYL1l3ayla1RVyjl3nRxOs3T9lfXXBOBZe3hhVWW3Y2K5n3vLN5KjanuFu1sagytP3l\n\tpuWQ==","X-Gm-Message-State":"AAQBX9fPqaxri8W9W3u9R0eNwu53KBuSRs362vaLV+00GmMuL00N758a\n\tIUG/3dmQ1UjKsqh3iQmHikrwHT6janjebEXenLPSKA==","X-Google-Smtp-Source":"AKy350Y5rrL0dM+Xg80eFYa3Mfo2vCjOB++SQZIy4Y3bbIRaVks36RB9HkE9MWPF0DbvzDPK8hgd/fIWDrFRU3ybqGg=","X-Received":"by 2002:a17:906:948:b0:878:4a24:1a5c with SMTP id\n\tj8-20020a170906094800b008784a241a5cmr876851ejd.6.1680601197657;\n\tTue, 04 Apr 2023 02:39:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-8-dse@thaumatec.com>\n\t<20230403135947.idhctz72b5b44xs7@uno.localdomain>","In-Reply-To":"<20230403135947.idhctz72b5b44xs7@uno.localdomain>","Date":"Tue, 4 Apr 2023 11:39:46 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26841,"web_url":"https://patchwork.libcamera.org/comment/26841/","msgid":"<20230404105326.odezbhbdco34tsaz@uno.localdomain>","date":"2023-04-04T10:53:26","subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Tue, Apr 04, 2023 at 11:39:46AM +0200, Daniel Semkowicz wrote:\n> Hi Jacopo,\n>\n> On Mon, Apr 3, 2023 at 3:59 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Daniel\n> >\n> > On Fri, Mar 31, 2023 at 10:19:27AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > Add platform related code for configuring auto focus window on the\n> > > rkisp1. Connect to the windowUpdateRequested() signal exposed by the\n> > > AfHillClimbing and configure the window on each signal emission.\n> > > This enables support of AfMetering and AfWindows controls on the rkisp1\n> > > platform.\n> > >\n> > > Currently, only one window is enabled, but ISP allows up to three\n> > > of them.\n> > >\n> > > Signed-off-by: Daniel Semkowicz \n> > > ---\n> > > src/ipa/rkisp1/algorithms/af.cpp | 64 +++++++++++++++++++++++++++++++-\n> > > src/ipa/rkisp1/algorithms/af.h | 13 ++++++-\n> > > 2 files changed, 75 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\n> > > index fde924d4..b6f6eee4 100644\n> > > --- a/src/ipa/rkisp1/algorithms/af.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/af.cpp\n> > > @@ -32,16 +32,43 @@ namespace ipa::rkisp1::algorithms {\n> > > * amount of time on each movement. This parameter should be set according\n> > > * to the worst case - the number of frames it takes to move lens between\n> > > * limit positions.\n> > > + * - **isp-threshold:** Threshold used for minimizing the influence of noise.\n> > > + * This affects the ISP sharpness calculation.\n> > > + * - **isp-var-shift:** The number of bits for the shift operation at the end\n> > > + * of the calculation chain. This affects the ISP sharpness calculation.\n> >\n> > I wonder if the introduction of these values belong to this patch or\n> > not. I guess they affect the AF algorithm globally, as a default\n> > window has to be programmed to have it working (something that happens in this\n> > patch, you might say :)\n>\n> You are right, these parameters should be moved to the previous commit.\n> I found that if no AF window was configured, ISP has a default one\n> (probably maximum AF window size), but this is not documented. This\n> way rkisp1 AF algo should work even without this commit.\n>\n> >\n> > Regardless of that, have you been able to identify with a little more\n> > accuracy how these values should be computed ? For the threshold I\n> > guess the explanation is somewhat clear, but the var-shift parameter I\n> > wonder how one should compute it ? As I read it, the var-shift value\n> > represents the number number of bits to right-shift the pixel values\n> > before summing them to avoid overflows of the afm_sum register (32\n> > bits). How did you come up with a value of 4 in your configuration\n> > file ? Does this value depend on the window size or does it depend on\n> > the sensor in use ?\n>\n> I came up with 4 just by experimenting with my setup. It worked even\n> with 0, but I shifted it a little bit for safety.\n> There is very little documentation on this, so unfortunately I know\n> only as much as you.\n>\n\na 4 position bit-shift does halve the information for an 8-bit Bayer\nsensor... I presume this is safe when it comes to overflow but reduces\nthe accuracy.\n\nIdeally this value would be tuned according to the number of pixels\nthat will be summed (per window ??) and the sensor's bit depth. By\ndividing (2^32-1 / num_pixels) you would get what the maximum allowed\nvalue per pixel would be and then you would have to adust right shift\nenough to make sure all your pixel values staty in that limit.\n\nI'm not asking to do this right now, but I wonder if these values\nshould really come from configuration file or should be computed here\n(hard-coded for the moment).\n\nDid you ever experienced overflow ?\n\n> >\n> > > * .\n> > > * \\sa libcamera::ipa::algorithms::AfHillClimbing for additional tuning\n> > > * parameters.\n> > > *\n> > > * \\todo Model the lens delay as number of frames required for the lens position\n> > > * to stabilize in the CameraLens class.\n> > > + * \\todo Check if requested window size is valid. RkISP supports AF window size\n> > > + * few pixels smaller than sensor output size.\n> > > + * \\todo Implement support for all available AF windows. RkISP supports up to 3\n> > > + * AF windows.\n> > > */\n> > >\n> > > LOG_DEFINE_CATEGORY(RkISP1Af)\n> > >\n> > > +namespace {\n> > > +\n> > > +constexpr rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)\n> > > +{\n> > > + return rkisp1_cif_isp_window{\n> > > + .h_offs = static_cast(rectangle.x),\n> > > + .v_offs = static_cast(rectangle.y),\n> > > + .h_size = static_cast(rectangle.width),\n> > > + .v_size = static_cast(rectangle.height)\n> > > + };\n> > > +}\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > +Af::Af()\n> > > +{\n> > > + af.windowUpdateRequested.connect(this, &Af::updateCurrentWindow);\n> > > +}\n> > > +\n> > > /**\n> > > * \\copydoc libcamera::ipa::Algorithm::init\n> > > */\n> > > @@ -54,8 +81,12 @@ int Af::init(IPAContext &context, const YamlObject &tuningData)\n> > > }\n> > >\n> > > waitFramesLens_ = tuningData[\"wait-frames-lens\"].get(1);\n> > > + ispThreshold_ = tuningData[\"isp-threshold\"].get(128);\n> > > + ispVarShift_ = tuningData[\"isp-var-shift\"].get(4);\n> > >\n> > > - LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_;\n> > > + LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_\n> > > + << \", ispThreshold_: \" << ispThreshold_\n> > > + << \", ispVarShift_: \" << ispVarShift_;\n> > >\n> > > return af.init(lensConfig->minFocusPosition,\n> > > lensConfig->maxFocusPosition, tuningData);\n> > > @@ -89,6 +120,32 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,\n> > > af.queueRequest(controls);\n> > > }\n> > >\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > + */\n> > > +void Af::prepare([[maybe_unused]] IPAContext &context,\n> > > + [[maybe_unused]] const uint32_t frame,\n> > > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > > + rkisp1_params_cfg *params)\n> > > +{\n> > > + if (updateWindow_) {\n> >\n> > or\n> > if (!updateWindow_)\n> > return;\n> >\n> > > + params->meas.afc_config.num_afm_win = 1;\n> > > + params->meas.afc_config.thres = ispThreshold_;\n> > > + params->meas.afc_config.var_shift = ispVarShift_;\n> > > + params->meas.afc_config.afm_win[0] =\n> > > + rectangleToIspWindow(*updateWindow_);\n> > > +\n> > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;\n> > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> > > +\n> > > + updateWindow_.reset();\n> > > +\n> > > + /* Wait one frame for the ISP to apply changes. */\n> > > + af.skipFrames(1);\n> > > + }\n> > > +}\n> > > +\n> > > /**\n> > > * \\copydoc libcamera::ipa::Algorithm::process\n> > > */\n> > > @@ -114,6 +171,11 @@ void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > }\n> > > }\n> > >\n> > > +void Af::updateCurrentWindow(const Rectangle &window)\n> > > +{\n> > > + updateWindow_ = window;\n> > > +}\n> > > +\n> > > REGISTER_IPA_ALGORITHM(Af, \"Af\")\n> > >\n> > > } /* namespace ipa::rkisp1::algorithms */\n> > > diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\n> > > index 3ba66d38..6f5adb19 100644\n> > > --- a/src/ipa/rkisp1/algorithms/af.h\n> > > +++ b/src/ipa/rkisp1/algorithms/af.h\n> > > @@ -20,21 +20,32 @@ namespace ipa::rkisp1::algorithms {\n> > > class Af : public Algorithm\n> > > {\n> > > public:\n> > > + Af();\n> > > +\n> > > int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > int configure(IPAContext &context,\n> > > const IPACameraSensorInfo &configInfo) override;\n> > > void queueRequest(IPAContext &context, uint32_t frame,\n> > > IPAFrameContext &frameContext,\n> > > const ControlList &controls) override;\n> > > + void prepare(IPAContext &context, uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + rkisp1_params_cfg *params) override;\n> > > void process(IPAContext &context, uint32_t frame,\n> > > IPAFrameContext &frameContext,\n> > > const rkisp1_stat_buffer *stats,\n> > > ControlList &metadata) override;\n> > >\n> > > private:\n> > > + void updateCurrentWindow(const Rectangle &window);\n> > > +\n> > > ipa::algorithms::AfHillClimbing af;\n> > >\n> > > - /* Wait number of frames after changing lens position */\n> > > + std::optional updateWindow_;\n> > > + uint32_t ispThreshold_ = 0;\n> > > + uint32_t ispVarShift_ = 0;\n> > > +\n> > > + /* Wait number of frames after changing lens position. */\n> > > uint32_t waitFramesLens_ = 0;\n> > > };\n> > >\n> > > --\n> > > 2.39.2\n> > >\n>\n> Best regards\n> Daniel","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 EEFC4C326B\n\tfor ;\n\tTue, 4 Apr 2023 10:53:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A07C6271F;\n\tTue, 4 Apr 2023 12:53: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 D50AC6271C\n\tfor ;\n\tTue, 4 Apr 2023 12:53:29 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 611D97F8;\n\tTue, 4 Apr 2023 12:53:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680605611;\n\tbh=F7zSZ4pc+xqcDaaXx1ehof7m186wUwq+vFOlaYfa6Vc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gl2/F78VLfEk1Mz2K0l/Jw//VoDBM1DsNVIOZTqrI3YkqAqlWr+xavYKXgfkpxSzx\n\twqCiVVqQJh4AgkqAJWXxUCAUEX4vg7iKl02NnX/qUg65w5pxwGoFtohoTBGLCNIxIV\n\tL9ikiKFh8zJjVTvCN3FobEldQuB6Uc7hJTWcL8L2wjre7A9MAksz4KWUPe0O7ldAH7\n\t82qPvnjXvc+5SPZE5aZOFgA8MhdGDebsz3Z0D71IFWYeTJoURnSMGTeEbK0KDzqKnH\n\t2fylQVpPYJNTz/YF6FrRiVcEV4g+jLbzJAFPXqnnoHngU978Q7JJgvYR6W0xYZtp70\n\teL+No6zUd5f9g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680605609;\n\tbh=F7zSZ4pc+xqcDaaXx1ehof7m186wUwq+vFOlaYfa6Vc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=q+0ACXyUFTWqg5k9tfRCaeIw+q6NDAWo76thRSaqGt7SB5ZBUIuYr8GH+mmxtfufr\n\tfMDG3EXjD5rRRPjOzfVMC5aO+lnnEYIkuiVt/mHVxxmM24/GHI7oCbbX1ZUINAjUAt\n\tXnwJgYFnJKkGBLnTTWHW7ho2gMR+K+U4e+3/5/uQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"q+0ACXyU\"; dkim-atps=neutral","Date":"Tue, 4 Apr 2023 12:53:26 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230404105326.odezbhbdco34tsaz@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-8-dse@thaumatec.com>\n\t<20230403135947.idhctz72b5b44xs7@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27145,"web_url":"https://patchwork.libcamera.org/comment/27145/","msgid":"","date":"2023-05-26T10:54:38","subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo, Hi Laurent,\n\nOn Wed, Apr 26, 2023 at 9:04 AM Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> On Tue, Apr 04, 2023 at 12:53:26PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > On Tue, Apr 04, 2023 at 11:39:46AM +0200, Daniel Semkowicz wrote:\n> > > On Mon, Apr 3, 2023 at 3:59 PM Jacopo Mondi wrote:\n> > > > On Fri, Mar 31, 2023 at 10:19:27AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > Add platform related code for configuring auto focus window on the\n> > > > > rkisp1. Connect to the windowUpdateRequested() signal exposed by the\n> > > > > AfHillClimbing and configure the window on each signal emission.\n> > > > > This enables support of AfMetering and AfWindows controls on the rkisp1\n> > > > > platform.\n> > > > >\n> > > > > Currently, only one window is enabled, but ISP allows up to three\n> > > > > of them.\n> > > > >\n> > > > > Signed-off-by: Daniel Semkowicz \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/af.cpp | 64 +++++++++++++++++++++++++++++++-\n> > > > > src/ipa/rkisp1/algorithms/af.h | 13 ++++++-\n> > > > > 2 files changed, 75 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\n> > > > > index fde924d4..b6f6eee4 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/af.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/af.cpp\n> > > > > @@ -32,16 +32,43 @@ namespace ipa::rkisp1::algorithms {\n> > > > > * amount of time on each movement. This parameter should be set according\n> > > > > * to the worst case - the number of frames it takes to move lens between\n> > > > > * limit positions.\n> > > > > + * - **isp-threshold:** Threshold used for minimizing the influence of noise.\n> > > > > + * This affects the ISP sharpness calculation.\n> > > > > + * - **isp-var-shift:** The number of bits for the shift operation at the end\n> > > > > + * of the calculation chain. This affects the ISP sharpness calculation.\n> > > >\n> > > > I wonder if the introduction of these values belong to this patch or\n> > > > not. I guess they affect the AF algorithm globally, as a default\n> > > > window has to be programmed to have it working (something that happens in this\n> > > > patch, you might say :)\n> > >\n> > > You are right, these parameters should be moved to the previous commit.\n> > > I found that if no AF window was configured, ISP has a default one\n> > > (probably maximum AF window size), but this is not documented. This\n> > > way rkisp1 AF algo should work even without this commit.\n> > >\n> > > > Regardless of that, have you been able to identify with a little more\n> > > > accuracy how these values should be computed ? For the threshold I\n> > > > guess the explanation is somewhat clear, but the var-shift parameter I\n> > > > wonder how one should compute it ? As I read it, the var-shift value\n> > > > represents the number number of bits to right-shift the pixel values\n> > > > before summing them to avoid overflows of the afm_sum register (32\n> > > > bits). How did you come up with a value of 4 in your configuration\n> > > > file ? Does this value depend on the window size or does it depend on\n> > > > the sensor in use ?\n> > >\n> > > I came up with 4 just by experimenting with my setup. It worked even\n> > > with 0, but I shifted it a little bit for safety.\n> > > There is very little documentation on this, so unfortunately I know\n> > > only as much as you.\n> >\n> > a 4 position bit-shift does halve the information for an 8-bit Bayer\n> > sensor... I presume this is safe when it comes to overflow but reduces\n> > the accuracy.\n> >\n> > Ideally this value would be tuned according to the number of pixels\n> > that will be summed (per window ??) and the sensor's bit depth. By\n> > dividing (2^32-1 / num_pixels) you would get what the maximum allowed\n> > value per pixel would be and then you would have to adust right shift\n> > enough to make sure all your pixel values staty in that limit.\n> >\n> > I'm not asking to do this right now, but I wonder if these values\n> > should really come from configuration file or should be computed here\n> > (hard-coded for the moment).\n>\n> The shift value should be computed, it shouldn't come from the tuning\n> file.\n>\n> Both the sharpness and luminance are computed on the 8 MSBs of the pixel\n> value as far as I can tell, so you don't need to take the bit depth of\n> the sensor into account, only the window size.\n>\n> The var_shift field actually combines two shift values, one for the\n> sharpness (in bits [2:0], you can call it afm_var_shift) and one for the\n> luminance (in bits [18:16], you can call it lum_var_shift). The reason\n> why two different shifts are needed is that the luminance value is\n> linear, while the sharpness value is quadratic.\n\nI was not aware this field maps directly to the register... The comment\nin rkisp1-config.h about this field is a bit misleading, as it does not\nmention there are actually two values. I should look into the driver\ncode more often instead of making assumptions :P\nIt would be good to have two different fields in the\nrkisp1_cif_isp_afc_config structure for lum and afm var shift, but I\nguess it is not easy to change now, as it breaks the interface.\n\n>\n> The luminance is stored in a 24-bit register value. It's simple to\n> compute the shift needed to avoid overflows:\n>\n> uint32_t pixelCount = window.width * window.height;\n> unsigned int div = (pixelCount + 255 * 255 - 1) / (255 * 255);\n> unsigned int shift = div > 1 ? 32 - __builtin_clz(div - 1) : 0;\n>\n> The hardware computes the sharpness value by summing the square of the\n> gradients in the window, using the Tenengrad function.\n>\n> \\[ \\sum_{i=1}^{M} \\sum_{j=1}^{N} G(i,j) \\]\n>\n> G is the gradient squared:\n>\n> G(i,j) = G_{x}(i,j}^{2} + G_{y}(i,j}^{2}\n>\n> G_{x} and G_{y} are the gradients in the horizontal and vertical\n> directions respectively, computed using Sobel filters (see\n> https://en.wikipedia.org/wiki/Sobel_operator for those).\n>\n> If I'm not mistaken, the worst case input is a black and white\n> chessboard pattern with squares of 2x2 pixels. A sample 3x3 block of\n> pixels would have the following values:\n>\n> [ 255 0 0\n> 0 255 255\n> 0 255 255 ]\n>\n> This gives\n>\n> G_{x}(i,j) = 1*255 + 0*0 + (-1)*0\n> + 2*0 + 0*255 + (-2)*255\n> + 1*0 + 0*255 + (-1)*255\n> = 510\n>\n> Similarly, G_{y}(i,j) is also equal to 510. I'll leave to the reader the\n> exercise of checking that the value is the same for all (i,j). The\n> square of the gradient for a pixel is thus equal to\n>\n> 510^{2} + 510^{2} = 520200\n>\n> The gradient is calculated on the green pixels only, so the image is\n> effectively subsampled by a factor of 2 horizontally. For a window of\n> 128x128 pixels, we would thus get an accumulated sharpness value of\n>\n> 520200 * (128/2) * 128 = 4261478400\n>\n> Now, I'm slightly annoyed, because documentation I have access to\n> mentions a maximum value of 4227989504 for a window size of 128x128. The\n> difference is small, but it shows I'm missing something somewhere\n> (there's also a chance the documentation is wrong, but that wouldn't be\n> my first bet). We could start by using the above calculation to be on\n> the safe side, as it gives a very slightly worst case than the value\n> from the documentation. The calculations should be captured in a comment\n> in the code.\n\nThank you for the details! I made my own calculations basing on your\ndescription and I achieved the same result of value 4261478400 for the\n2x2 chessboard.\n\nI made some additional calculations, and it looks that 2x2 chessboard is\nnot the worst case for the Tenengrad function.\nWhat I found to be the worst case is the stripes pattern with 2 black\npixels and two white pixels. The following 4x4 block of pixels\nrepresents the stripes pattern:\n\n [ 255 255 0 0\n 255 255 0 0\n 255 255 0 0\n 255 255 0 0 ]\n\nThis gives:\n\n G_{x}(2,2) = (-1*255) + (0*255) + (1*0)\n + (-2*255) + (0*255) + (2*0)\n + (-1*255) + (0*255) + (1*0)\n = 1020\n\nG_{x}(i,j) will be 1020 for all positions.\n\nG_{y}(i,j) will be always 0 for this pattern as there are changes only\nalong the X axis.\n\nThe sum of the squares in this case will be equal to:\n\n 1020^{2} + 0^{2} = 1040400\n\nBasing on your description, for 128x128 this pattern should give the\nsharpness value:\n\n 1040400 * (128/2) * 128 = 8522956800\n\nThis value is two times bigger than what you mentioned to be found\nin the documentation, however, it is in the same order of magnitude.\n\nThere are a lot of uncertainties of how exactly the ISP calculates the\nsharpness value. If documentation states 4227989504 for the 128x128\nwindow, then maybe it would be safer to base the maximum value on\nthe documentation? Something like this:\n\n 4227989504 / (128 * 128) = 258056\n\n maxSharpness = window.width * window.height * 258056\n\n>\n> The threshold value should likely be dynamic too, as it should depend on\n> the noise level, but that can be done later.\n\nDo I understand correctly that threshold here means that if calculated\nsharpness value give the value lower than threshold, the sharpness will\nbe set to 0? If this is true, the threshold can be hardcoded to 0,\nbecause the Af algorithm already includes safety margin when looking\nfor max sharpness value.\n\n>\n> > Did you ever experienced overflow ?\n\nNo, I did not. I set the bit shift just for safety.\n\n> >\n> > > > > * .\n> > > > > * \\sa libcamera::ipa::algorithms::AfHillClimbing for additional tuning\n> > > > > * parameters.\n> > > > > *\n> > > > > * \\todo Model the lens delay as number of frames required for the lens position\n> > > > > * to stabilize in the CameraLens class.\n> > > > > + * \\todo Check if requested window size is valid. RkISP supports AF window size\n> > > > > + * few pixels smaller than sensor output size.\n> > > > > + * \\todo Implement support for all available AF windows. RkISP supports up to 3\n> > > > > + * AF windows.\n> > > > > */\n> > > > >\n> > > > > LOG_DEFINE_CATEGORY(RkISP1Af)\n> > > > >\n> > > > > +namespace {\n> > > > > +\n> > > > > +constexpr rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)\n> > > > > +{\n> > > > > + return rkisp1_cif_isp_window{\n> > > > > + .h_offs = static_cast(rectangle.x),\n> > > > > + .v_offs = static_cast(rectangle.y),\n> > > > > + .h_size = static_cast(rectangle.width),\n> > > > > + .v_size = static_cast(rectangle.height)\n> > > > > + };\n> > > > > +}\n>\n> This function could be shared with other algorithm, as\n> rkisp1_cif_isp_window isn't specific to AF. We can address that later,\n> or move the function to algorithm.h already.\n>\n> > > > > +\n> > > > > +} /* namespace */\n> > > > > +\n> > > > > +Af::Af()\n> > > > > +{\n> > > > > + af.windowUpdateRequested.connect(this, &Af::updateCurrentWindow);\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\copydoc libcamera::ipa::Algorithm::init\n> > > > > */\n> > > > > @@ -54,8 +81,12 @@ int Af::init(IPAContext &context, const YamlObject &tuningData)\n> > > > > }\n> > > > >\n> > > > > waitFramesLens_ = tuningData[\"wait-frames-lens\"].get(1);\n> > > > > + ispThreshold_ = tuningData[\"isp-threshold\"].get(128);\n> > > > > + ispVarShift_ = tuningData[\"isp-var-shift\"].get(4);\n> > > > >\n> > > > > - LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_;\n> > > > > + LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_\n> > > > > + << \", ispThreshold_: \" << ispThreshold_\n> > > > > + << \", ispVarShift_: \" << ispVarShift_;\n> > > > >\n> > > > > return af.init(lensConfig->minFocusPosition,\n> > > > > lensConfig->maxFocusPosition, tuningData);\n> > > > > @@ -89,6 +120,32 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,\n> > > > > af.queueRequest(controls);\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > + */\n> > > > > +void Af::prepare([[maybe_unused]] IPAContext &context,\n> > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > > > > + rkisp1_params_cfg *params)\n> > > > > +{\n> > > > > + if (updateWindow_) {\n> > > >\n> > > > or\n> > > > if (!updateWindow_)\n> > > > return;\n> > > >\n> > > > > + params->meas.afc_config.num_afm_win = 1;\n> > > > > + params->meas.afc_config.thres = ispThreshold_;\n> > > > > + params->meas.afc_config.var_shift = ispVarShift_;\n> > > > > + params->meas.afc_config.afm_win[0] =\n> > > > > + rectangleToIspWindow(*updateWindow_);\n> > > > > +\n> > > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> > > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;\n> > > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;\n>\n> Should we disable the AF stats calculation when in manual focus mode to\n> save power, or is that a useless microoptimization ?\n\nI would suggest doing this in a separate change as it requires\nadditional testing to make sure changing the state does not affect\nsharpness calculations.\n\n>\n> > > > > +\n> > > > > + updateWindow_.reset();\n> > > > > +\n> > > > > + /* Wait one frame for the ISP to apply changes. */\n> > > > > + af.skipFrames(1);\n>\n> This delay sounds dodgy, have you verified that it takes exactly one\n> frame from here ?\n\nYes, in my experiments it took one frame. However, I am not 100% this\nwill be the same for all cases.\n\n>\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\copydoc libcamera::ipa::Algorithm::process\n> > > > > */\n> > > > > @@ -114,6 +171,11 @@ void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > +void Af::updateCurrentWindow(const Rectangle &window)\n> > > > > +{\n> > > > > + updateWindow_ = window;\n> > > > > +}\n> > > > > +\n> > > > > REGISTER_IPA_ALGORITHM(Af, \"Af\")\n> > > > >\n> > > > > } /* namespace ipa::rkisp1::algorithms */\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\n> > > > > index 3ba66d38..6f5adb19 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/af.h\n> > > > > +++ b/src/ipa/rkisp1/algorithms/af.h\n> > > > > @@ -20,21 +20,32 @@ namespace ipa::rkisp1::algorithms {\n> > > > > class Af : public Algorithm\n> > > > > {\n> > > > > public:\n> > > > > + Af();\n> > > > > +\n> > > > > int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > > int configure(IPAContext &context,\n> > > > > const IPACameraSensorInfo &configInfo) override;\n> > > > > void queueRequest(IPAContext &context, uint32_t frame,\n> > > > > IPAFrameContext &frameContext,\n> > > > > const ControlList &controls) override;\n> > > > > + void prepare(IPAContext &context, uint32_t frame,\n> > > > > + IPAFrameContext &frameContext,\n> > > > > + rkisp1_params_cfg *params) override;\n> > > > > void process(IPAContext &context, uint32_t frame,\n> > > > > IPAFrameContext &frameContext,\n> > > > > const rkisp1_stat_buffer *stats,\n> > > > > ControlList &metadata) override;\n> > > > >\n> > > > > private:\n> > > > > + void updateCurrentWindow(const Rectangle &window);\n> > > > > +\n> > > > > ipa::algorithms::AfHillClimbing af;\n> > > > >\n> > > > > - /* Wait number of frames after changing lens position */\n> > > > > + std::optional updateWindow_;\n> > > > > + uint32_t ispThreshold_ = 0;\n> > > > > + uint32_t ispVarShift_ = 0;\n> > > > > +\n> > > > > + /* Wait number of frames after changing lens position. */\n> > > > > uint32_t waitFramesLens_ = 0;\n> > > > > };\n> > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nBest regards\nDaniel","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 382CFC3213\n\tfor ;\n\tFri, 26 May 2023 10:54:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8CA43626FA;\n\tFri, 26 May 2023 12:54:52 +0200 (CEST)","from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com\n\t[IPv6:2a00:1450:4864:20::52c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DB3E626F4\n\tfor ;\n\tFri, 26 May 2023 12:54:50 +0200 (CEST)","by mail-ed1-x52c.google.com with SMTP id\n\t4fb4d7f45d1cf-51456392cbbso1080774a12.0\n\tfor ;\n\tFri, 26 May 2023 03:54:50 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685098492;\n\tbh=IceuVpav0IfZaLL9wzcx0JTTFY3sgmB4WbfPOhM4rLU=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ACajWLtvMOATIQ8YN+J/hVB4bE7+iG8sXs9AbFWBZ+wkm5WdwAsZRIie/0RO37b+m\n\tlpddyMuBf9lQV7MRNMS2QWUiS3o3aPHipLwaEaGdrUD+G39FYM0ZiD/COyZpe2qT81\n\t689jexyrAPefrEepeUliXumi6NU9kluv+IZldCAtXlDW11vIzucf0rJPfxylgUdl/P\n\tV5M00nKEMy5s81cuX88THPFrNHmaq/+8n0uo6iVazYEtrV7vQmAdThIiRu3j9UrCtq\n\tqcA4uSHN2X8MHiVdsMmSzUySjH9HDmGcfGJoNiKCxwhxXFOrz+yKBFKyLwcxdZ3LZU\n\t4vDwx9jRrDRJw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1685098489;\n\tx=1687690489; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=m2quUJ6mnlBrGdBpQr+SoPjXhEC7YEysfzp60/Yc5EQ=;\n\tb=pO65b/aypf/XUQdVscIS+HdH4HSZ3xHgSJaM95E3LxHr+4pExrpdLum4FFF4z1i3Az\n\tGeoWtlnf3NEQrOtF3oqjIwk71iyeKwJLBChG6cLGo5frsOq0bCXanG8a+XXlvYAJLacy\n\t996v3p0FfbpymUP90gj0+z7ks3Wo4Gq0EWw2tl3YVttA4Oe0Tuvu8wP+y1YfnCZ5sYTI\n\tfsyAM0+bdJxJmoBXwnj4cLVgO6RNnJXfjeeA8O+bx1R4RHZbbtDTby+mWDU6FYutUMpd\n\tK6IrbYzDgghtmN24ltlOYRnXWEiVX3+NB1wGByyjWsseSz6hWmHIIkibS5dDRkwmX3ev\n\t0zvg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20221208.gappssmtp.com\n\theader.i=@thaumatec-com.20221208.gappssmtp.com header.b=\"pO65b/ay\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685098490; x=1687690490;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=m2quUJ6mnlBrGdBpQr+SoPjXhEC7YEysfzp60/Yc5EQ=;\n\tb=Q3MLZuqiIrRHjoqAlkkW+4nRnN11NQS5S41VaJRK94XVlBiYffUcoW5CTvsPVtOEG1\n\tLVKVuL4i9AMk+sizc6DDqBDmRdU9kBL3i8tzeYHiJY9YB442BwyTn1Q7aP+3LM+bh9Lt\n\t+PcBM8kQZJKxK3KUpUR9tzgiX+IvPZCIdwfxUs75DyQT989gPHQ0rTe40mAA9xWGbP9V\n\t799grojx1Jdcc/LRjTlejMrk1ud/jo3jzTleK1YzgYSl9euZK3y+ihpu/KbT/KG1Ogfx\n\tOmYHCX3JEKFxIEg8YxfMiHlvmFzRSqVU6aRs25bbJhcsVboZUe9zxLS4SSVVJquQSgM/\n\t6m0Q==","X-Gm-Message-State":"AC+VfDzm9Br3pCcLYC9gTsYMAzyjwwipsa7MgVHx+zDbyrxIIJtdEldX\n\ty34GeNzAjahbGgc+qohuU0kBDcMo/OqELUWT6iMdXw==","X-Google-Smtp-Source":"ACHHUZ5cMkt/q/saOYHe0fYMqrUyEuoi8GCQO9HZqkqIqltD0LFZTGCcszJ0SP4CcWk+s+fiosfbPrCjVx+AY3YhObM=","X-Received":"by 2002:a05:6402:124f:b0:504:a3ec:eacc with SMTP id\n\tl15-20020a056402124f00b00504a3eceaccmr1329545edw.4.1685098489521;\n\tFri, 26 May 2023 03:54:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-8-dse@thaumatec.com>\n\t<20230403135947.idhctz72b5b44xs7@uno.localdomain>\n\t\n\t<20230404105326.odezbhbdco34tsaz@uno.localdomain>\n\t<20230426070416.GK1667@pendragon.ideasonboard.com>","In-Reply-To":"<20230426070416.GK1667@pendragon.ideasonboard.com>","Date":"Fri, 26 May 2023 12:54:38 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27201,"web_url":"https://patchwork.libcamera.org/comment/27201/","msgid":"<20230531180711.GI24749@pendragon.ideasonboard.com>","date":"2023-05-31T18:07:11","subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Daniel,\n\nOn Fri, May 26, 2023 at 12:54:38PM +0200, Daniel Semkowicz wrote:\n> On Wed, Apr 26, 2023 at 9:04 AM Laurent Pinchart wrote:\n> > On Tue, Apr 04, 2023 at 12:53:26PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > On Tue, Apr 04, 2023 at 11:39:46AM +0200, Daniel Semkowicz wrote:\n> > > > On Mon, Apr 3, 2023 at 3:59 PM Jacopo Mondi wrote:\n> > > > > On Fri, Mar 31, 2023 at 10:19:27AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > Add platform related code for configuring auto focus window on the\n> > > > > > rkisp1. Connect to the windowUpdateRequested() signal exposed by the\n> > > > > > AfHillClimbing and configure the window on each signal emission.\n> > > > > > This enables support of AfMetering and AfWindows controls on the rkisp1\n> > > > > > platform.\n> > > > > >\n> > > > > > Currently, only one window is enabled, but ISP allows up to three\n> > > > > > of them.\n> > > > > >\n> > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > ---\n> > > > > > src/ipa/rkisp1/algorithms/af.cpp | 64 +++++++++++++++++++++++++++++++-\n> > > > > > src/ipa/rkisp1/algorithms/af.h | 13 ++++++-\n> > > > > > 2 files changed, 75 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\n> > > > > > index fde924d4..b6f6eee4 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/af.cpp\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/af.cpp\n> > > > > > @@ -32,16 +32,43 @@ namespace ipa::rkisp1::algorithms {\n> > > > > > * amount of time on each movement. This parameter should be set according\n> > > > > > * to the worst case - the number of frames it takes to move lens between\n> > > > > > * limit positions.\n> > > > > > + * - **isp-threshold:** Threshold used for minimizing the influence of noise.\n> > > > > > + * This affects the ISP sharpness calculation.\n> > > > > > + * - **isp-var-shift:** The number of bits for the shift operation at the end\n> > > > > > + * of the calculation chain. This affects the ISP sharpness calculation.\n> > > > >\n> > > > > I wonder if the introduction of these values belong to this patch or\n> > > > > not. I guess they affect the AF algorithm globally, as a default\n> > > > > window has to be programmed to have it working (something that happens in this\n> > > > > patch, you might say :)\n> > > >\n> > > > You are right, these parameters should be moved to the previous commit.\n> > > > I found that if no AF window was configured, ISP has a default one\n> > > > (probably maximum AF window size), but this is not documented. This\n> > > > way rkisp1 AF algo should work even without this commit.\n> > > >\n> > > > > Regardless of that, have you been able to identify with a little more\n> > > > > accuracy how these values should be computed ? For the threshold I\n> > > > > guess the explanation is somewhat clear, but the var-shift parameter I\n> > > > > wonder how one should compute it ? As I read it, the var-shift value\n> > > > > represents the number number of bits to right-shift the pixel values\n> > > > > before summing them to avoid overflows of the afm_sum register (32\n> > > > > bits). How did you come up with a value of 4 in your configuration\n> > > > > file ? Does this value depend on the window size or does it depend on\n> > > > > the sensor in use ?\n> > > >\n> > > > I came up with 4 just by experimenting with my setup. It worked even\n> > > > with 0, but I shifted it a little bit for safety.\n> > > > There is very little documentation on this, so unfortunately I know\n> > > > only as much as you.\n> > >\n> > > a 4 position bit-shift does halve the information for an 8-bit Bayer\n> > > sensor... I presume this is safe when it comes to overflow but reduces\n> > > the accuracy.\n> > >\n> > > Ideally this value would be tuned according to the number of pixels\n> > > that will be summed (per window ??) and the sensor's bit depth. By\n> > > dividing (2^32-1 / num_pixels) you would get what the maximum allowed\n> > > value per pixel would be and then you would have to adust right shift\n> > > enough to make sure all your pixel values staty in that limit.\n> > >\n> > > I'm not asking to do this right now, but I wonder if these values\n> > > should really come from configuration file or should be computed here\n> > > (hard-coded for the moment).\n> >\n> > The shift value should be computed, it shouldn't come from the tuning\n> > file.\n> >\n> > Both the sharpness and luminance are computed on the 8 MSBs of the pixel\n> > value as far as I can tell, so you don't need to take the bit depth of\n> > the sensor into account, only the window size.\n> >\n> > The var_shift field actually combines two shift values, one for the\n> > sharpness (in bits [2:0], you can call it afm_var_shift) and one for the\n> > luminance (in bits [18:16], you can call it lum_var_shift). The reason\n> > why two different shifts are needed is that the luminance value is\n> > linear, while the sharpness value is quadratic.\n> \n> I was not aware this field maps directly to the register... The comment\n> in rkisp1-config.h about this field is a bit misleading, as it does not\n> mention there are actually two values. I should look into the driver\n> code more often instead of making assumptions :P\n> It would be good to have two different fields in the\n> rkisp1_cif_isp_afc_config structure for lum and afm var shift, but I\n> guess it is not easy to change now, as it breaks the interface.\n\nYes, it would be difficult to change in a backward-compatible way. If we\ncould assume libcamera is the only user of the rkisp1 AF we could\npossibly modify the interface, but that wouldn't be a very humble\nassumption :-)\n\n> > The luminance is stored in a 24-bit register value. It's simple to\n> > compute the shift needed to avoid overflows:\n> >\n> > uint32_t pixelCount = window.width * window.height;\n> > unsigned int div = (pixelCount + 255 * 255 - 1) / (255 * 255);\n> > unsigned int shift = div > 1 ? 32 - __builtin_clz(div - 1) : 0;\n> >\n> > The hardware computes the sharpness value by summing the square of the\n> > gradients in the window, using the Tenengrad function.\n> >\n> > \\[ \\sum_{i=1}^{M} \\sum_{j=1}^{N} G(i,j) \\]\n> >\n> > G is the gradient squared:\n> >\n> > G(i,j) = G_{x}(i,j}^{2} + G_{y}(i,j}^{2}\n> >\n> > G_{x} and G_{y} are the gradients in the horizontal and vertical\n> > directions respectively, computed using Sobel filters (see\n> > https://en.wikipedia.org/wiki/Sobel_operator for those).\n> >\n> > If I'm not mistaken, the worst case input is a black and white\n> > chessboard pattern with squares of 2x2 pixels. A sample 3x3 block of\n> > pixels would have the following values:\n> >\n> > [ 255 0 0\n> > 0 255 255\n> > 0 255 255 ]\n> >\n> > This gives\n> >\n> > G_{x}(i,j) = 1*255 + 0*0 + (-1)*0\n> > + 2*0 + 0*255 + (-2)*255\n> > + 1*0 + 0*255 + (-1)*255\n> > = 510\n> >\n> > Similarly, G_{y}(i,j) is also equal to 510. I'll leave to the reader the\n> > exercise of checking that the value is the same for all (i,j). The\n> > square of the gradient for a pixel is thus equal to\n> >\n> > 510^{2} + 510^{2} = 520200\n> >\n> > The gradient is calculated on the green pixels only, so the image is\n> > effectively subsampled by a factor of 2 horizontally. For a window of\n> > 128x128 pixels, we would thus get an accumulated sharpness value of\n> >\n> > 520200 * (128/2) * 128 = 4261478400\n> >\n> > Now, I'm slightly annoyed, because documentation I have access to\n> > mentions a maximum value of 4227989504 for a window size of 128x128. The\n> > difference is small, but it shows I'm missing something somewhere\n> > (there's also a chance the documentation is wrong, but that wouldn't be\n> > my first bet). We could start by using the above calculation to be on\n> > the safe side, as it gives a very slightly worst case than the value\n> > from the documentation. The calculations should be captured in a comment\n> > in the code.\n> \n> Thank you for the details! I made my own calculations basing on your\n> description and I achieved the same result of value 4261478400 for the\n> 2x2 chessboard.\n> \n> I made some additional calculations, and it looks that 2x2 chessboard is\n> not the worst case for the Tenengrad function.\n> What I found to be the worst case is the stripes pattern with 2 black\n> pixels and two white pixels. The following 4x4 block of pixels\n> represents the stripes pattern:\n> \n> [ 255 255 0 0\n> 255 255 0 0\n> 255 255 0 0\n> 255 255 0 0 ]\n> \n> This gives:\n> \n> G_{x}(2,2) = (-1*255) + (0*255) + (1*0)\n> + (-2*255) + (0*255) + (2*0)\n> + (-1*255) + (0*255) + (1*0)\n> = 1020\n\nThat's -1020 actually, but not relevant as the value is then squared.\n\n> \n> G_{x}(i,j) will be 1020 for all positions.\n> \n> G_{y}(i,j) will be always 0 for this pattern as there are changes only\n> along the X axis.\n> \n> The sum of the squares in this case will be equal to:\n> \n> 1020^{2} + 0^{2} = 1040400\n\nGood catch, I had missed that !\n\n> Basing on your description, for 128x128 this pattern should give the\n> sharpness value:\n> \n> 1040400 * (128/2) * 128 = 8522956800\n> \n> This value is two times bigger than what you mentioned to be found\n> in the documentation, however, it is in the same order of magnitude.\n> \n> There are a lot of uncertainties of how exactly the ISP calculates the\n> sharpness value. If documentation states 4227989504 for the 128x128\n> window, then maybe it would be safer to base the maximum value on\n> the documentation? Something like this:\n\nWe could actually test it, by using a test pattern from the sensor, and\ncarefully selecting the AF window, but that's a bit of work.\n\n> 4227989504 / (128 * 128) = 258056\n> \n> maxSharpness = window.width * window.height * 258056\n\nWe can start with this, that's fine with me. Please capture the above\nexplanation in a comment, and add a \\todo to measure the sharpness value\nwith a vertical bar test pattern from the sensor, to double-check the\ncalculation.\n\n> > The threshold value should likely be dynamic too, as it should depend on\n> > the noise level, but that can be done later.\n> \n> Do I understand correctly that threshold here means that if calculated\n> sharpness value give the value lower than threshold, the sharpness will\n> be set to 0?\n\nThat's a very good question. I'm afraid I don't have an answer, I\nhaven't been able to find documentation about this.\n\n> If this is true, the threshold can be hardcoded to 0,\n> because the Af algorithm already includes safety margin when looking\n> for max sharpness value.\n\nI'm fine setting it to 0 to start with (please add a \\todo comment to\ntell that it should likely be computed based on the noise level).\n\n> > > Did you ever experienced overflow ?\n> \n> No, I did not. I set the bit shift just for safety.\n> \n> > > > > > * .\n> > > > > > * \\sa libcamera::ipa::algorithms::AfHillClimbing for additional tuning\n> > > > > > * parameters.\n> > > > > > *\n> > > > > > * \\todo Model the lens delay as number of frames required for the lens position\n> > > > > > * to stabilize in the CameraLens class.\n> > > > > > + * \\todo Check if requested window size is valid. RkISP supports AF window size\n> > > > > > + * few pixels smaller than sensor output size.\n> > > > > > + * \\todo Implement support for all available AF windows. RkISP supports up to 3\n> > > > > > + * AF windows.\n> > > > > > */\n> > > > > >\n> > > > > > LOG_DEFINE_CATEGORY(RkISP1Af)\n> > > > > >\n> > > > > > +namespace {\n> > > > > > +\n> > > > > > +constexpr rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)\n> > > > > > +{\n> > > > > > + return rkisp1_cif_isp_window{\n> > > > > > + .h_offs = static_cast(rectangle.x),\n> > > > > > + .v_offs = static_cast(rectangle.y),\n> > > > > > + .h_size = static_cast(rectangle.width),\n> > > > > > + .v_size = static_cast(rectangle.height)\n> > > > > > + };\n> > > > > > +}\n> >\n> > This function could be shared with other algorithm, as\n> > rkisp1_cif_isp_window isn't specific to AF. We can address that later,\n> > or move the function to algorithm.h already.\n> >\n> > > > > > +\n> > > > > > +} /* namespace */\n> > > > > > +\n> > > > > > +Af::Af()\n> > > > > > +{\n> > > > > > + af.windowUpdateRequested.connect(this, &Af::updateCurrentWindow);\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\copydoc libcamera::ipa::Algorithm::init\n> > > > > > */\n> > > > > > @@ -54,8 +81,12 @@ int Af::init(IPAContext &context, const YamlObject &tuningData)\n> > > > > > }\n> > > > > >\n> > > > > > waitFramesLens_ = tuningData[\"wait-frames-lens\"].get(1);\n> > > > > > + ispThreshold_ = tuningData[\"isp-threshold\"].get(128);\n> > > > > > + ispVarShift_ = tuningData[\"isp-var-shift\"].get(4);\n> > > > > >\n> > > > > > - LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_;\n> > > > > > + LOG(RkISP1Af, Debug) << \"waitFramesLens_: \" << waitFramesLens_\n> > > > > > + << \", ispThreshold_: \" << ispThreshold_\n> > > > > > + << \", ispVarShift_: \" << ispVarShift_;\n> > > > > >\n> > > > > > return af.init(lensConfig->minFocusPosition,\n> > > > > > lensConfig->maxFocusPosition, tuningData);\n> > > > > > @@ -89,6 +120,32 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,\n> > > > > > af.queueRequest(controls);\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > > + */\n> > > > > > +void Af::prepare([[maybe_unused]] IPAContext &context,\n> > > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > > > > > + rkisp1_params_cfg *params)\n> > > > > > +{\n> > > > > > + if (updateWindow_) {\n> > > > >\n> > > > > or\n> > > > > if (!updateWindow_)\n> > > > > return;\n> > > > >\n> > > > > > + params->meas.afc_config.num_afm_win = 1;\n> > > > > > + params->meas.afc_config.thres = ispThreshold_;\n> > > > > > + params->meas.afc_config.var_shift = ispVarShift_;\n> > > > > > + params->meas.afc_config.afm_win[0] =\n> > > > > > + rectangleToIspWindow(*updateWindow_);\n> > > > > > +\n> > > > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> > > > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;\n> > > > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> >\n> > Should we disable the AF stats calculation when in manual focus mode to\n> > save power, or is that a useless microoptimization ?\n> \n> I would suggest doing this in a separate change as it requires\n> additional testing to make sure changing the state does not affect\n> sharpness calculations.\n> \n> > > > > > +\n> > > > > > + updateWindow_.reset();\n> > > > > > +\n> > > > > > + /* Wait one frame for the ISP to apply changes. */\n> > > > > > + af.skipFrames(1);\n> >\n> > This delay sounds dodgy, have you verified that it takes exactly one\n> > frame from here ?\n> \n> Yes, in my experiments it took one frame. However, I am not 100% this\n> will be the same for all cases.\n> \n> > > > > > + }\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\copydoc libcamera::ipa::Algorithm::process\n> > > > > > */\n> > > > > > @@ -114,6 +171,11 @@ void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > }\n> > > > > > }\n> > > > > >\n> > > > > > +void Af::updateCurrentWindow(const Rectangle &window)\n> > > > > > +{\n> > > > > > + updateWindow_ = window;\n> > > > > > +}\n> > > > > > +\n> > > > > > REGISTER_IPA_ALGORITHM(Af, \"Af\")\n> > > > > >\n> > > > > > } /* namespace ipa::rkisp1::algorithms */\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\n> > > > > > index 3ba66d38..6f5adb19 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/af.h\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/af.h\n> > > > > > @@ -20,21 +20,32 @@ namespace ipa::rkisp1::algorithms {\n> > > > > > class Af : public Algorithm\n> > > > > > {\n> > > > > > public:\n> > > > > > + Af();\n> > > > > > +\n> > > > > > int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > > > int configure(IPAContext &context,\n> > > > > > const IPACameraSensorInfo &configInfo) override;\n> > > > > > void queueRequest(IPAContext &context, uint32_t frame,\n> > > > > > IPAFrameContext &frameContext,\n> > > > > > const ControlList &controls) override;\n> > > > > > + void prepare(IPAContext &context, uint32_t frame,\n> > > > > > + IPAFrameContext &frameContext,\n> > > > > > + rkisp1_params_cfg *params) override;\n> > > > > > void process(IPAContext &context, uint32_t frame,\n> > > > > > IPAFrameContext &frameContext,\n> > > > > > const rkisp1_stat_buffer *stats,\n> > > > > > ControlList &metadata) override;\n> > > > > >\n> > > > > > private:\n> > > > > > + void updateCurrentWindow(const Rectangle &window);\n> > > > > > +\n> > > > > > ipa::algorithms::AfHillClimbing af;\n> > > > > >\n> > > > > > - /* Wait number of frames after changing lens position */\n> > > > > > + std::optional updateWindow_;\n> > > > > > + uint32_t ispThreshold_ = 0;\n> > > > > > + uint32_t ispVarShift_ = 0;\n> > > > > > +\n> > > > > > + /* Wait number of frames after changing lens position. */\n> > > > > > uint32_t waitFramesLens_ = 0;\n> > > > > > };\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 2E0ECC3213\n\tfor ;\n\tWed, 31 May 2023 18:07:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C2A862722;\n\tWed, 31 May 2023 20:07:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B19DF626F8\n\tfor ;\n\tWed, 31 May 2023 20:07:13 +0200 (CEST)","from pendragon.ideasonboard.com (om126205251136.34.openmobile.ne.jp\n\t[126.205.251.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DEC4B904;\n\tWed, 31 May 2023 20:06:50 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685556435;\n\tbh=kpXlKiiUses2gc7mJ6ZpzFdNp+dt85Fw1uaZUUdQWrY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3s+yiWzwY/PQFcaItZQPF25mGVd/PlxHzCC688AG65Z8QsPXnYz1zUsYKvXVO4AqX\n\tADeqyR4Aiqj9sVqplygtjCdHrOnMvtYcAhPTuDhKQEbDM4xJlhozuz0rSUkUc2L3pd\n\t9RNbslsLonv5uEaIsRHY0sGHQ0j0iPnplzX5uNJMDioqe8GS5beGP5eHjgZqKep/c2\n\tGsh6htC3H0j3Aj9wmbJJqnvKkWblk67/63AFefZCz2k7Cv/Ip3SDLtC+4F2BGHaTpi\n\tM2lpduvnU1jiDF/38ePUnVNDUKC+zunkzk6xK/PwnAM9wUqWm6igHLId5wFmD7fBlQ\n\tLADQVIgp6raFg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685556412;\n\tbh=kpXlKiiUses2gc7mJ6ZpzFdNp+dt85Fw1uaZUUdQWrY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uZnd/Bq/Qe3u5On0MaycG5gleLnyw7GLe9vHfWQQk51qXJo9bS3WrEisdvdnPcaFT\n\t5TLnmKFR0L6IeBTaiuK27mADxdpnOENVeLPmY9wo1vJbURo7aZsmCoxm0xIATopMJ4\n\tijD0GNuxHy3RyO6h1WlwNI6nxt2fuMWFtPh3XeFA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uZnd/Bq/\"; dkim-atps=neutral","Date":"Wed, 31 May 2023 21:07:11 +0300","To":"Daniel Semkowicz ","Message-ID":"<20230531180711.GI24749@pendragon.ideasonboard.com>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-8-dse@thaumatec.com>\n\t<20230403135947.idhctz72b5b44xs7@uno.localdomain>\n\t\n\t<20230404105326.odezbhbdco34tsaz@uno.localdomain>\n\t<20230426070416.GK1667@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add\n\t\"Windows\" Metering mode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]