[{"id":26818,"web_url":"https://patchwork.libcamera.org/comment/26818/","msgid":"<20230403130732.szbyy3trz7imxcff@uno.localdomain>","date":"2023-04-03T13:07:32","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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 sorry I have neglected the Windows related patches in previous\nreviews as I hoped someone would have shared an opinion on the\nSignal-based mechanism.\n\nI don't have better ideas to propose, so let's go with it\n\nOn Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Add support for setting user defined auto focus window in the\n> AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> Each time, there is a need for changing the window configuration in the\n> ISP, the signal is emitted. Platform layer that wants to use\n> the \"Windows\" metering mode, needs to connect to this signal\n> and configure the ISP on each emission.\n>\n> Currently only one window is supported.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> 2 files changed, 66 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> index 244b8803..0fb17df3 100644\n> --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> * movement range rather than coarse search result.\n> * \\todo Implement setRange.\n> * \\todo Implement setSpeed.\n> - * \\todo Implement setMeteringMode.\n> - * \\todo Implement setWindows.\n> * \\todo Implement the AfPauseDeferred mode.\n> + * \\todo Implement support for multiple AF windows.\n> */\n>\n> /**\n> @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> \treturn 0;\n> }\n>\n> +/**\n> + * \\brief Configure the AfHillClimbing with sensor information\n> + * \\param[in] outputSize Camera sensor output size\n> + *\n> + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> + * method of the platform layer.\n> + */\n> +void AfHillClimbing::configure(const Size &outputSize)\n\nAccording to the AfWindows control definition\n\n Sets the focus windows used by the AF algorithm when AfMetering is set\n to AfMeteringWindows. The units used are pixels within the rectangle\n returned by the ScalerCropMaximum property.\n\nThe final sensor's output size can be obtained by downscaling/cropping\na larger portion of the pixel array. The portion of the pixel array\nprocessed to obtain the final image is named AnalogueCropRectangle and\nall valid crop rectangles lie inside it.\n\nMost of our controls, such as ScalerCrop, are expressed with the\nAnalogueCrop as reference, to allow expressing them regardless of the\ncurrent output size. It's up to the pipeline handler to re-scale any\nvalid control in the current configured output.\n\nI'm not 100% sure why we decided to use ScalerCropMaximum here, most\nprobably to allow pipeline handler express any additional processing\nmargins required by any cropping/scaling that happens on the ISP.\n\nHowever as the RkISP1 pipeline doesn't express any of those margin\nyet, I would simply use here the sensor's analogue crop, which is part\nof the IPACameraSensorInfo you already use in the platform layer.\n\nTo remove ambiguities I would call the parameter here \"maxWindow\" or\nsomething similar.\n\nI would also initialize userWindow_ to the same value, so that if an\napplication switches to AfMeteringWindows mode the rectangle is at\nleast initialized to something.\n\n\n> +{\n> +\t/*\n> +\t * Default AF window of 3/4 size of the camera sensor output,\n> +\t * placed at the center\n> +\t */\n> +\tdefaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> +\t\t\t\t static_cast(outputSize.height / 8),\n> +\t\t\t\t 3 * outputSize.width / 4,\n> +\t\t\t\t 3 * outputSize.height / 4);\n> +\n> +\twindowUpdateRequested.emit(defaultWindow_);\n> +}\n> +\n> /**\n> * \\brief Run the auto focus algorithm loop\n> * \\param[in] currentContrast New value of contrast measured for current frame\n> @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> \t\tframesToSkip_ = n;\n> }\n>\n> +/**\n> + * \\var AfHillClimbing::windowUpdateRequested\n> + * \\brief Signal emitted when change in AF window was requested\n> + *\n> + * Platform layer supporting AF windows should connect to this signal\n> + * and configure the ISP with new window on each emition.\n> + */\n> +\n> void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> {\n> \tif (mode == mode_)\n> @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> \tLOG(Af, Error) << \"setSpeed() not implemented!\";\n> }\n>\n> -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> {\n> -\tLOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> +\tif (metering == meteringMode_)\n> +\t\treturn;\n> +\n> +\tmeteringMode_ = metering;\n> +\n> +\tswitch (metering) {\n> +\tcase controls::AfMeteringAuto:\n> +\t\twindowUpdateRequested.emit(defaultWindow_);\n> +\t\tbreak;\n> +\tcase controls::AfMeteringWindows:\n> +\t\twindowUpdateRequested.emit(userWindow_);\n> +\t\tbreak;\n> +\t}\n> }\n>\n> -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> +void AfHillClimbing::setWindows(Span windows)\n> {\n> -\tLOG(Af, Error) << \"setWindows() not implemented!\";\n> +\tif (windows.size() != 1) {\n> +\t\tLOG(Af, Error) << \"Only one AF window is supported\";\n> +\t\treturn;\n> +\t}\n\n\nShould this be an hard error or should we just want and continue by\nonly considering the first available rectangle ?\n\n> +\n> +\tLOG(Af, Debug) << \"setWindows: \" << windows[0];\n> +\n> +\tuserWindow_ = windows[0];\n> +\n> +\tif (meteringMode_ == controls::AfMeteringWindows)\n> +\t\twindowUpdateRequested.emit(userWindow_);\n> }\n>\n> void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> index 2147939b..0f7c65db 100644\n> --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -10,6 +10,7 @@\n> #pragma once\n>\n> #include \n> +#include \n>\n> #include \"af.h\"\n>\n> @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> public:\n> \tint init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> \t\t const YamlObject &tuningData);\n> +\tvoid configure(const Size &outputSize);\n> \tint32_t process(double currentContrast);\n> \tvoid skipFrames(uint32_t n);\n>\n> \tcontrols::AfStateEnum state() override { return state_; }\n> \tcontrols::AfPauseStateEnum pauseState() override { return pauseState_; }\n>\n> +\tSignal windowUpdateRequested;\n> +\n> private:\n> \tvoid setMode(controls::AfModeEnum mode) override;\n> \tvoid setRange(controls::AfRangeEnum range) override;\n> @@ -54,6 +58,7 @@ private:\n> \tcontrols::AfModeEnum mode_ = controls::AfModeManual;\n> \tcontrols::AfStateEnum state_ = controls::AfStateIdle;\n> \tcontrols::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> +\tcontrols::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n>\n> \t/* Current focus lens position. */\n> \tint32_t lensPosition_ = 0;\n> @@ -84,6 +89,11 @@ private:\n>\n> \t/* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> \tdouble maxChange_;\n> +\n> +\t/* Default AF window. */\n> +\tRectangle defaultWindow_;\n> +\t/* AF window set by user using AF_WINDOWS control. */\n> +\tRectangle userWindow_;\n> };\n>\n> } /* namespace ipa::algorithms */\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 24320C326B\n\tfor ;\n\tMon, 3 Apr 2023 13:07:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69C7A6271C;\n\tMon, 3 Apr 2023 15:07:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6EFE626E3\n\tfor ;\n\tMon, 3 Apr 2023 15:07:35 +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 68D1B4A7;\n\tMon, 3 Apr 2023 15:07:35 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680527257;\n\tbh=XSr2YkAwHLp95tzhc6A2ioBbA2llY2yZgvgihiUaAo0=;\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=Rcufq+CBAa6doH3AvjPjpLfMeTdX0/5UuGaKTUioZHswHgyqT12tUvhWFgPpN++su\n\tAm+YYERDyTosFuN5xj5Ppf7bylNO/K6Bw6u2vFM/CBfUagb692aeJdTj8gMV2uek0A\n\ta95MlCzL2zwbYbGAW+vQGaPiYQnlQtXRS+hxw90unG//86hnVg+kVAkG42zLf8Luyq\n\turawF28ijvAUxKEzwwxLa4XHTiSQrJCqhQb01pg00H96QtfMIIsHRVbaq0vZmiV7+h\n\t3XR0yhRkYaqyYhZSIda/Q79SjbPqA11n4rFEsBhNp/TBXyjA7S8Sbqh8E7h0waj0Pp\n\tyJhtqK4jFy0Cw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680527255;\n\tbh=XSr2YkAwHLp95tzhc6A2ioBbA2llY2yZgvgihiUaAo0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c0/L2Z0lS5ClGr/Ej/O8eMt/zPnBalea4uvCgjzwf2f50ANpMZxu+7aBrPgmbnsWm\n\t5cGVEcpx09R8y9ZkupkK9gc7iKnW3nRhLEyglGTFTcvFL9HNaGjhNBJy/akL6w4FGI\n\tFBEdhG5tPObtvwqSt2ZXtdG/GK4+ekziQjmQxVoE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"c0/L2Z0l\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 15:07:32 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230403130732.szbyy3trz7imxcff@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230331081930.19289-6-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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":26832,"web_url":"https://patchwork.libcamera.org/comment/26832/","msgid":"","date":"2023-04-04T09:06:54","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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:07 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n> sorry I have neglected the Windows related patches in previous\n> reviews as I hoped someone would have shared an opinion on the\n> Signal-based mechanism.\n>\n> I don't have better ideas to propose, so let's go with it\n>\n> On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Add support for setting user defined auto focus window in the\n> > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > Each time, there is a need for changing the window configuration in the\n> > ISP, the signal is emitted. Platform layer that wants to use\n> > the \"Windows\" metering mode, needs to connect to this signal\n> > and configure the ISP on each emission.\n> >\n> > Currently only one window is supported.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > 2 files changed, 66 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > index 244b8803..0fb17df3 100644\n> > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > * movement range rather than coarse search result.\n> > * \\todo Implement setRange.\n> > * \\todo Implement setSpeed.\n> > - * \\todo Implement setMeteringMode.\n> > - * \\todo Implement setWindows.\n> > * \\todo Implement the AfPauseDeferred mode.\n> > + * \\todo Implement support for multiple AF windows.\n> > */\n> >\n> > /**\n> > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > return 0;\n> > }\n> >\n> > +/**\n> > + * \\brief Configure the AfHillClimbing with sensor information\n> > + * \\param[in] outputSize Camera sensor output size\n> > + *\n> > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > + * method of the platform layer.\n> > + */\n> > +void AfHillClimbing::configure(const Size &outputSize)\n>\n> According to the AfWindows control definition\n>\n> Sets the focus windows used by the AF algorithm when AfMetering is set\n> to AfMeteringWindows. The units used are pixels within the rectangle\n> returned by the ScalerCropMaximum property.\n>\n> The final sensor's output size can be obtained by downscaling/cropping\n> a larger portion of the pixel array. The portion of the pixel array\n> processed to obtain the final image is named AnalogueCropRectangle and\n> all valid crop rectangles lie inside it.\n>\n> Most of our controls, such as ScalerCrop, are expressed with the\n> AnalogueCrop as reference, to allow expressing them regardless of the\n> current output size. It's up to the pipeline handler to re-scale any\n> valid control in the current configured output.\n\nFor the 1024x768 stream, I have the following parameters:\n- activeAreaSize: 2592x1944\n- analogCrop: (0, 0)/2592x1944\n- outputSize: 1296x972\n\nWhen using analogCrop, I will also need to scale the AF window\nsize when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\nvalues in reference to the ISP input size\n(IPACameraSensorInfo::outputSize).\nBased on what you wrote earlier, I understand that it is supposed to be\nlike that?\n\n>\n> I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> probably to allow pipeline handler express any additional processing\n> margins required by any cropping/scaling that happens on the ISP.\n>\n> However as the RkISP1 pipeline doesn't express any of those margin\n> yet, I would simply use here the sensor's analogue crop, which is part\n> of the IPACameraSensorInfo you already use in the platform layer.\n>\n\nI have some concerns about the ScalerCropMaximum.\nWhat if the ISP have different size margins for cropping and auto-focus\nwindows? I see there are margins defined for the AF window, but nothing\nabout the cropping margins in the RK3399 documentation. In this case,\nit will not be a problem, but what if some ISP will have margins for\ncropping, but no margins for AF window?\n\n> To remove ambiguities I would call the parameter here \"maxWindow\" or\n> something similar.\n>\n> I would also initialize userWindow_ to the same value, so that if an\n> application switches to AfMeteringWindows mode the rectangle is at\n> least initialized to something.\n\nGood point!\n\n>\n>\n> > +{\n> > + /*\n> > + * Default AF window of 3/4 size of the camera sensor output,\n> > + * placed at the center\n> > + */\n> > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > + static_cast(outputSize.height / 8),\n> > + 3 * outputSize.width / 4,\n> > + 3 * outputSize.height / 4);\n> > +\n> > + windowUpdateRequested.emit(defaultWindow_);\n> > +}\n> > +\n> > /**\n> > * \\brief Run the auto focus algorithm loop\n> > * \\param[in] currentContrast New value of contrast measured for current frame\n> > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > framesToSkip_ = n;\n> > }\n> >\n> > +/**\n> > + * \\var AfHillClimbing::windowUpdateRequested\n> > + * \\brief Signal emitted when change in AF window was requested\n> > + *\n> > + * Platform layer supporting AF windows should connect to this signal\n> > + * and configure the ISP with new window on each emition.\n> > + */\n> > +\n> > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > {\n> > if (mode == mode_)\n> > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > }\n> >\n> > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > {\n> > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > + if (metering == meteringMode_)\n> > + return;\n> > +\n> > + meteringMode_ = metering;\n> > +\n> > + switch (metering) {\n> > + case controls::AfMeteringAuto:\n> > + windowUpdateRequested.emit(defaultWindow_);\n> > + break;\n> > + case controls::AfMeteringWindows:\n> > + windowUpdateRequested.emit(userWindow_);\n> > + break;\n> > + }\n> > }\n> >\n> > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > +void AfHillClimbing::setWindows(Span windows)\n> > {\n> > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > + if (windows.size() != 1) {\n> > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > + return;\n> > + }\n>\n>\n> Should this be an hard error or should we just want and continue by\n> only considering the first available rectangle ?\n\nI will change it to a warning that only the first window was used.\n\n>\n> > +\n> > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > +\n> > + userWindow_ = windows[0];\n> > +\n> > + if (meteringMode_ == controls::AfMeteringWindows)\n> > + windowUpdateRequested.emit(userWindow_);\n> > }\n> >\n> > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > index 2147939b..0f7c65db 100644\n> > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > @@ -10,6 +10,7 @@\n> > #pragma once\n> >\n> > #include \n> > +#include \n> >\n> > #include \"af.h\"\n> >\n> > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > public:\n> > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > const YamlObject &tuningData);\n> > + void configure(const Size &outputSize);\n> > int32_t process(double currentContrast);\n> > void skipFrames(uint32_t n);\n> >\n> > controls::AfStateEnum state() override { return state_; }\n> > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> >\n> > + Signal windowUpdateRequested;\n> > +\n> > private:\n> > void setMode(controls::AfModeEnum mode) override;\n> > void setRange(controls::AfRangeEnum range) override;\n> > @@ -54,6 +58,7 @@ private:\n> > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> >\n> > /* Current focus lens position. */\n> > int32_t lensPosition_ = 0;\n> > @@ -84,6 +89,11 @@ private:\n> >\n> > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > double maxChange_;\n> > +\n> > + /* Default AF window. */\n> > + Rectangle defaultWindow_;\n> > + /* AF window set by user using AF_WINDOWS control. */\n> > + Rectangle userWindow_;\n> > };\n> >\n> > } /* namespace ipa::algorithms */\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 8949CC0F2A\n\tfor ;\n\tTue, 4 Apr 2023 09:07:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D823B62724;\n\tTue, 4 Apr 2023 11:07:07 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59C8A626DA\n\tfor ;\n\tTue, 4 Apr 2023 11:07:06 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id h8so127736695ede.8\n\tfor ;\n\tTue, 04 Apr 2023 02:07:06 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680599227;\n\tbh=tUQy7BSlRON0witfchnJ0qsKBAEjH+pVjMzzIi/5Pfs=;\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=LTKruHs39gwSD+YhybrltPsftrzL60RQ5nKEBJ7fBZ25moqgeAN/BJUh2SvIrWSx8\n\t/0z85l3XX8XMrJvo7FkQI0jNoDpyN+O+yQJy9GJtkH/Mu7w8QoPrk3E8Igr1s0+JPR\n\t14EQGsz+RKO2TvHRprd+nAUJwWtVxeUlQGNkLl+QE8C/OqFomvvmw0lgLvugf2PwFG\n\tuH0VXIrd80C2Uj8O7RCEJflk2kTIFUz+rX6BP0nD3LCUIp4zbutkvJVhvj6SeI/Boq\n\tsUf8/N5mV/2EYt8VPSLzXpQkg+SCdYQiXZDfUu33vF/rfIOFODcUyVXYLKSX8EPLiA\n\t5l5iqGhm1uvdA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1680599226;\n\tx=1683191226; \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=Mll32DQeGDL8aJI1LjBSRjncpLlRlxVjHeYZbtpeO+0=;\n\tb=hmKy+QkkulJVs50j7vtwbcE94z51IvsAQzNwFjlrW0X2Wb1KRQL+PLUX3Lv7KJqCn4\n\t8Cs1Ug3GWIDpwETTUiSYZI1L/WOqS3223X3FMM1DV42+pC9VkWOsrEGksAix34R0lGmV\n\tsBe647aKzk3pXJt49vtJIhaHVflXThyIfkQy1qAk40ZYSzzlg4VoUp/mFG5oFPG6nZPn\n\t2DxVDtJO7OJQ63GkyaP9GqIzD4ZXTKhF/wPxUaN+tMMRH/mzQQdArvdfZzmwZXD7ZLXC\n\tyKqGsOQ/YTufsQSopknVF9an4JUpO71iS6AgRurd3okaB8YvoWiBz4jtYHoJneB6i0o0\n\tvKfA=="],"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=\"hmKy+Qkk\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680599226; x=1683191226;\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=Mll32DQeGDL8aJI1LjBSRjncpLlRlxVjHeYZbtpeO+0=;\n\tb=z6C0aZFn8056trsrEyXSt4rhWTmoOe/y9UpNCF1APkzSa1zRVYY2/RS319NIkrnk3l\n\tYc6l7wKs0h0fpFc4plcJh4fyeSYR/yCM4CWtneXJuKtgcpgpl0EkGhJ+c5NSmmJp1pB1\n\tBvlfDIrvMNelDsgnF1PrcEWw6aqZp8AP53hY5WQqrKD++Fsa+hlA0X58ACZ/SiKUdN3N\n\tu9p2fgmzfm+Y9NyMibnP1QKmQGUfczJp4c8HFBCvBspqzpqGEP+ewIZHDIRbCZCx/msa\n\t5lC6nGIpEl6uBNTwdXqHVWIyUPYnEIeDTDniRg7qI9Ds8Fhcd0B3zF8lzyqtf2wDjSh5\n\t8opw==","X-Gm-Message-State":"AAQBX9cnutLZai5Rz5b7iCLB9cbgySpbwcnEXLaar731t4vy2zqNbBU1\n\tMSo1UGyJejkbhBtXIm51GUNDOVrDd1Y/XSrtKqj5SQ==","X-Google-Smtp-Source":"AKy350Y9/VIFZfmvsC56/3r+GkYDGpIaA38HWBKrGWGN6w5Zq8bFR1CUhz0oqtX+reflvVxu/kem4w6opOdxsSB+zZY=","X-Received":"by 2002:a17:907:a0b:b0:932:da0d:9375 with SMTP id\n\tbb11-20020a1709070a0b00b00932da0d9375mr1127156ejc.4.1680599225744;\n\tTue, 04 Apr 2023 02:07:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>","In-Reply-To":"<20230403130732.szbyy3trz7imxcff@uno.localdomain>","Date":"Tue, 4 Apr 2023 11:06:54 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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":26842,"web_url":"https://patchwork.libcamera.org/comment/26842/","msgid":"<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>","date":"2023-04-04T10:59:11","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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 cc RPi folks which originally introduced the AF controls and which\n have recently been working a new implementation\n\nOn Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Hi Jacopo,\n>\n> On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Daniel\n> > sorry I have neglected the Windows related patches in previous\n> > reviews as I hoped someone would have shared an opinion on the\n> > Signal-based mechanism.\n> >\n> > I don't have better ideas to propose, so let's go with it\n> >\n> > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > Add support for setting user defined auto focus window in the\n> > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > Each time, there is a need for changing the window configuration in the\n> > > ISP, the signal is emitted. Platform layer that wants to use\n> > > the \"Windows\" metering mode, needs to connect to this signal\n> > > and configure the ISP on each emission.\n> > >\n> > > Currently only one window is supported.\n> > >\n> > > Signed-off-by: Daniel Semkowicz \n> > > ---\n> > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > index 244b8803..0fb17df3 100644\n> > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > * movement range rather than coarse search result.\n> > > * \\todo Implement setRange.\n> > > * \\todo Implement setSpeed.\n> > > - * \\todo Implement setMeteringMode.\n> > > - * \\todo Implement setWindows.\n> > > * \\todo Implement the AfPauseDeferred mode.\n> > > + * \\todo Implement support for multiple AF windows.\n> > > */\n> > >\n> > > /**\n> > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > return 0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > + * \\param[in] outputSize Camera sensor output size\n> > > + *\n> > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > + * method of the platform layer.\n> > > + */\n> > > +void AfHillClimbing::configure(const Size &outputSize)\n> >\n> > According to the AfWindows control definition\n> >\n> > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > to AfMeteringWindows. The units used are pixels within the rectangle\n> > returned by the ScalerCropMaximum property.\n> >\n> > The final sensor's output size can be obtained by downscaling/cropping\n> > a larger portion of the pixel array. The portion of the pixel array\n> > processed to obtain the final image is named AnalogueCropRectangle and\n> > all valid crop rectangles lie inside it.\n> >\n> > Most of our controls, such as ScalerCrop, are expressed with the\n> > AnalogueCrop as reference, to allow expressing them regardless of the\n> > current output size. It's up to the pipeline handler to re-scale any\n> > valid control in the current configured output.\n>\n> For the 1024x768 stream, I have the following parameters:\n> - activeAreaSize: 2592x1944\n> - analogCrop: (0, 0)/2592x1944\n> - outputSize: 1296x972\n>\n> When using analogCrop, I will also need to scale the AF window\n> size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> values in reference to the ISP input size\n> (IPACameraSensorInfo::outputSize).\n> Based on what you wrote earlier, I understand that it is supposed to be\n> like that?\n>\n\nI presume so.\n\nI should check how RPi handles windowing\n\n> >\n> > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > probably to allow pipeline handler express any additional processing\n> > margins required by any cropping/scaling that happens on the ISP.\n> >\n> > However as the RkISP1 pipeline doesn't express any of those margin\n> > yet, I would simply use here the sensor's analogue crop, which is part\n> > of the IPACameraSensorInfo you already use in the platform layer.\n> >\n>\n> I have some concerns about the ScalerCropMaximum.\n> What if the ISP have different size margins for cropping and auto-focus\n> windows? I see there are margins defined for the AF window, but nothing\n> about the cropping margins in the RK3399 documentation. In this case,\n> it will not be a problem, but what if some ISP will have margins for\n> cropping, but no margins for AF window?\n>\n\nGood point. Do we need an AfWindowMaximum ?\n\nRPi any opinions here ?\n\n> > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > something similar.\n> >\n> > I would also initialize userWindow_ to the same value, so that if an\n> > application switches to AfMeteringWindows mode the rectangle is at\n> > least initialized to something.\n>\n> Good point!\n>\n> >\n> >\n> > > +{\n> > > + /*\n> > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > + * placed at the center\n> > > + */\n> > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > + static_cast(outputSize.height / 8),\n> > > + 3 * outputSize.width / 4,\n> > > + 3 * outputSize.height / 4);\n> > > +\n> > > + windowUpdateRequested.emit(defaultWindow_);\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Run the auto focus algorithm loop\n> > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > framesToSkip_ = n;\n> > > }\n> > >\n> > > +/**\n> > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > + * \\brief Signal emitted when change in AF window was requested\n> > > + *\n> > > + * Platform layer supporting AF windows should connect to this signal\n> > > + * and configure the ISP with new window on each emition.\n> > > + */\n> > > +\n> > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > {\n> > > if (mode == mode_)\n> > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > }\n> > >\n> > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > {\n> > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > + if (metering == meteringMode_)\n> > > + return;\n> > > +\n> > > + meteringMode_ = metering;\n> > > +\n> > > + switch (metering) {\n> > > + case controls::AfMeteringAuto:\n> > > + windowUpdateRequested.emit(defaultWindow_);\n> > > + break;\n> > > + case controls::AfMeteringWindows:\n> > > + windowUpdateRequested.emit(userWindow_);\n> > > + break;\n> > > + }\n> > > }\n> > >\n> > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > +void AfHillClimbing::setWindows(Span windows)\n> > > {\n> > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > + if (windows.size() != 1) {\n> > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > + return;\n> > > + }\n> >\n> >\n> > Should this be an hard error or should we just want and continue by\n> > only considering the first available rectangle ?\n>\n> I will change it to a warning that only the first window was used.\n>\n> >\n> > > +\n> > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > +\n> > > + userWindow_ = windows[0];\n> > > +\n> > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > + windowUpdateRequested.emit(userWindow_);\n> > > }\n> > >\n> > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > index 2147939b..0f7c65db 100644\n> > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > @@ -10,6 +10,7 @@\n> > > #pragma once\n> > >\n> > > #include \n> > > +#include \n> > >\n> > > #include \"af.h\"\n> > >\n> > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > public:\n> > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > const YamlObject &tuningData);\n> > > + void configure(const Size &outputSize);\n> > > int32_t process(double currentContrast);\n> > > void skipFrames(uint32_t n);\n> > >\n> > > controls::AfStateEnum state() override { return state_; }\n> > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > >\n> > > + Signal windowUpdateRequested;\n> > > +\n> > > private:\n> > > void setMode(controls::AfModeEnum mode) override;\n> > > void setRange(controls::AfRangeEnum range) override;\n> > > @@ -54,6 +58,7 @@ private:\n> > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > >\n> > > /* Current focus lens position. */\n> > > int32_t lensPosition_ = 0;\n> > > @@ -84,6 +89,11 @@ private:\n> > >\n> > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > double maxChange_;\n> > > +\n> > > + /* Default AF window. */\n> > > + Rectangle defaultWindow_;\n> > > + /* AF window set by user using AF_WINDOWS control. */\n> > > + Rectangle userWindow_;\n> > > };\n> > >\n> > > } /* namespace ipa::algorithms */\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 ECF01C0F2A\n\tfor ;\n\tTue, 4 Apr 2023 10:59:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46B2C62724;\n\tTue, 4 Apr 2023 12:59:15 +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 D36C6626DB\n\tfor ;\n\tTue, 4 Apr 2023 12:59:13 +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 59D027F8;\n\tTue, 4 Apr 2023 12:59:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680605955;\n\tbh=AzYmS0vEBtO8SAAqE1ew7mkca4OURHRt+nW++gg0yII=;\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=cGFwO7Sa7HytfUUgY0aMmVCP2Y8lYmFMzOy89LR9HpQxCz8UwGtHRV9H3rYbRTJn/\n\tyB0N9nL9X3gFQy1sDUwZKV1p+iFubxJC5g1MDAaJTHbfDia+GgfPFvAg1m0ttzpIvV\n\txXXn6M4kbISkRWXwDsKnJdGLy2QqilwDvfZLaEAlRsrBAHfkx+9KQCNDey3FgJiiLo\n\tmU4tDUPVUCT92WHYHS3unQeMiGTYCmF/WcxMeEQBCoRosNW1pkf9WZNaR4g8Y6AT20\n\tfh5MmBZS8ZQRg27qwGZVsR12gvjybweFnIdMoPu5FyqlIdpSuLO5+BIYvnbH5eYzQa\n\tCossuiBnigsyQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680605953;\n\tbh=AzYmS0vEBtO8SAAqE1ew7mkca4OURHRt+nW++gg0yII=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=frhc1JNl8PyKqAKSZot4HoXCfi19UPAqKQ2mLehHO1SP8jSVJt/Cx8dhrLMwA9uTj\n\tYQ5+MPnCvNqwb9W8fVXYMgKRNKnrZd4NaLyN0yxDj5i8WjJIyaQSHDt9UuI6asYVRe\n\tZaAsopmC0U3zVC6Go6+NC8W2PzriR12QxDQOtq2k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"frhc1JNl\"; dkim-atps=neutral","Date":"Tue, 4 Apr 2023 12:59:11 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@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 05/10] ipa: af_hill_climbing: 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,\n\tNick Hollinghurst ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26855,"web_url":"https://patchwork.libcamera.org/comment/26855/","msgid":"","date":"2023-04-05T06:02:54","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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":"On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n> cc RPi folks which originally introduced the AF controls and which\n> have recently been working a new implementation\n>\n> On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Daniel\n> > > sorry I have neglected the Windows related patches in previous\n> > > reviews as I hoped someone would have shared an opinion on the\n> > > Signal-based mechanism.\n> > >\n> > > I don't have better ideas to propose, so let's go with it\n> > >\n> > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Add support for setting user defined auto focus window in the\n> > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > > Each time, there is a need for changing the window configuration in the\n> > > > ISP, the signal is emitted. Platform layer that wants to use\n> > > > the \"Windows\" metering mode, needs to connect to this signal\n> > > > and configure the ISP on each emission.\n> > > >\n> > > > Currently only one window is supported.\n> > > >\n> > > > Signed-off-by: Daniel Semkowicz \n> > > > ---\n> > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > index 244b8803..0fb17df3 100644\n> > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > > * movement range rather than coarse search result.\n> > > > * \\todo Implement setRange.\n> > > > * \\todo Implement setSpeed.\n> > > > - * \\todo Implement setMeteringMode.\n> > > > - * \\todo Implement setWindows.\n> > > > * \\todo Implement the AfPauseDeferred mode.\n> > > > + * \\todo Implement support for multiple AF windows.\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > > + * \\param[in] outputSize Camera sensor output size\n> > > > + *\n> > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > > + * method of the platform layer.\n> > > > + */\n> > > > +void AfHillClimbing::configure(const Size &outputSize)\n> > >\n> > > According to the AfWindows control definition\n> > >\n> > > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > > to AfMeteringWindows. The units used are pixels within the rectangle\n> > > returned by the ScalerCropMaximum property.\n> > >\n> > > The final sensor's output size can be obtained by downscaling/cropping\n> > > a larger portion of the pixel array. The portion of the pixel array\n> > > processed to obtain the final image is named AnalogueCropRectangle and\n> > > all valid crop rectangles lie inside it.\n> > >\n> > > Most of our controls, such as ScalerCrop, are expressed with the\n> > > AnalogueCrop as reference, to allow expressing them regardless of the\n> > > current output size. It's up to the pipeline handler to re-scale any\n> > > valid control in the current configured output.\n> >\n> > For the 1024x768 stream, I have the following parameters:\n> > - activeAreaSize: 2592x1944\n> > - analogCrop: (0, 0)/2592x1944\n> > - outputSize: 1296x972\n> >\n> > When using analogCrop, I will also need to scale the AF window\n> > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> > values in reference to the ISP input size\n> > (IPACameraSensorInfo::outputSize).\n> > Based on what you wrote earlier, I understand that it is supposed to be\n> > like that?\n> >\n>\n> I presume so.\n>\n> I should check how RPi handles windowing\n>\n> > >\n> > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > > probably to allow pipeline handler express any additional processing\n> > > margins required by any cropping/scaling that happens on the ISP.\n> > >\n> > > However as the RkISP1 pipeline doesn't express any of those margin\n> > > yet, I would simply use here the sensor's analogue crop, which is part\n> > > of the IPACameraSensorInfo you already use in the platform layer.\n> > >\n> >\n> > I have some concerns about the ScalerCropMaximum.\n> > What if the ISP have different size margins for cropping and auto-focus\n> > windows? I see there are margins defined for the AF window, but nothing\n> > about the cropping margins in the RK3399 documentation. In this case,\n> > it will not be a problem, but what if some ISP will have margins for\n> > cropping, but no margins for AF window?\n> >\n>\n> Good point. Do we need an AfWindowMaximum ?\n\nTo make sense, AfWindowMaximum would need to be expressed in reference\nto how ISP handles that (sensor output size on rkisp) to allow precise\nAF window configuration. AF window margins on rkisp are small: 2px and\n5px. I would like to avoid scaling, so small values compared to\nthe output size.\nI am not sure if AfWindowMaximum as additional property is actually\nneeded. Maximum value for AfWindows would not be enough?\n\nIs there a need to expose the window margins to the user at all?\nWe could allow the window size of the whole ISP input size to be set\nby the user and clamp the size in the IPA before configuring the ISP?\n\nIn summary, I see two options:\n1. Maximum AF window expressed in units that are directly set to ISP\n (output size for rkisp). Maximum AF window will include margins.\n2. Keep the current approach and express the AF window in reference to\n Analogue crop size. Just allow the full size instead of the\n ScalerCropMaximum. Clamp the margins inside the IPA if user\n requested larger window.\n\n>\n> RPi any opinions here ?\n>\n> > > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > > something similar.\n> > >\n> > > I would also initialize userWindow_ to the same value, so that if an\n> > > application switches to AfMeteringWindows mode the rectangle is at\n> > > least initialized to something.\n> >\n> > Good point!\n> >\n> > >\n> > >\n> > > > +{\n> > > > + /*\n> > > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > > + * placed at the center\n> > > > + */\n> > > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > > + static_cast(outputSize.height / 8),\n> > > > + 3 * outputSize.width / 4,\n> > > > + 3 * outputSize.height / 4);\n> > > > +\n> > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Run the auto focus algorithm loop\n> > > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > > framesToSkip_ = n;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > > + * \\brief Signal emitted when change in AF window was requested\n> > > > + *\n> > > > + * Platform layer supporting AF windows should connect to this signal\n> > > > + * and configure the ISP with new window on each emition.\n> > > > + */\n> > > > +\n> > > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > {\n> > > > if (mode == mode_)\n> > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > > }\n> > > >\n> > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > > {\n> > > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > > + if (metering == meteringMode_)\n> > > > + return;\n> > > > +\n> > > > + meteringMode_ = metering;\n> > > > +\n> > > > + switch (metering) {\n> > > > + case controls::AfMeteringAuto:\n> > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > + break;\n> > > > + case controls::AfMeteringWindows:\n> > > > + windowUpdateRequested.emit(userWindow_);\n> > > > + break;\n> > > > + }\n> > > > }\n> > > >\n> > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > +void AfHillClimbing::setWindows(Span windows)\n> > > > {\n> > > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > > + if (windows.size() != 1) {\n> > > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > > + return;\n> > > > + }\n> > >\n> > >\n> > > Should this be an hard error or should we just want and continue by\n> > > only considering the first available rectangle ?\n> >\n> > I will change it to a warning that only the first window was used.\n> >\n> > >\n> > > > +\n> > > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > > +\n> > > > + userWindow_ = windows[0];\n> > > > +\n> > > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > > + windowUpdateRequested.emit(userWindow_);\n> > > > }\n> > > >\n> > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > index 2147939b..0f7c65db 100644\n> > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > @@ -10,6 +10,7 @@\n> > > > #pragma once\n> > > >\n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \"af.h\"\n> > > >\n> > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > > public:\n> > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > const YamlObject &tuningData);\n> > > > + void configure(const Size &outputSize);\n> > > > int32_t process(double currentContrast);\n> > > > void skipFrames(uint32_t n);\n> > > >\n> > > > controls::AfStateEnum state() override { return state_; }\n> > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > > >\n> > > > + Signal windowUpdateRequested;\n> > > > +\n> > > > private:\n> > > > void setMode(controls::AfModeEnum mode) override;\n> > > > void setRange(controls::AfRangeEnum range) override;\n> > > > @@ -54,6 +58,7 @@ private:\n> > > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > > >\n> > > > /* Current focus lens position. */\n> > > > int32_t lensPosition_ = 0;\n> > > > @@ -84,6 +89,11 @@ private:\n> > > >\n> > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > double maxChange_;\n> > > > +\n> > > > + /* Default AF window. */\n> > > > + Rectangle defaultWindow_;\n> > > > + /* AF window set by user using AF_WINDOWS control. */\n> > > > + Rectangle userWindow_;\n> > > > };\n> > > >\n> > > > } /* namespace ipa::algorithms */\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 092FDC0F2A\n\tfor ;\n\tWed, 5 Apr 2023 06:03:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86FBF62761;\n\tWed, 5 Apr 2023 08:03:07 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8691961EC2\n\tfor ;\n\tWed, 5 Apr 2023 08:03:06 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id eh3so138305501edb.11\n\tfor ;\n\tTue, 04 Apr 2023 23:03:06 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680674587;\n\tbh=//1YJgMQtci174xPO/pvHbTgukv27rivUSPXuMauSdc=;\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=Wu9VJHJUqDgXMkqq6WiRpSqcenE7D9f3vY5zt3CcAQc25ui+0H06yrtbehe1FK4uM\n\trE9E7K0wlVSPs4UHgJYwQm6hyRgC7vCtIAvlxV6Kqq31UtKkbVMddcaj/PEKKomZcL\n\tLPSu3yLzw9ObphRlNRdq0RPKfmn8KoxUf6EXIgm6AYSIuxJ7tyd6Pm5xRJlOBqpwv0\n\tBff605Is/pZT3XTJyMRhzwP8Qrw04rM9IpdOLn+uPg4dLFk6fEltJlIO7Ba48IFkRv\n\tUoKUygpHwivYxRc3B1V+gz62SldjWnW5tjynZgPDleQdUGSPz5OJ5qX0zAqIeMnHp+\n\tTrNyjEwWdlfMQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1680674586;\n\tx=1683266586; \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=7ZJ9JU8V5OWeahxLWhu2SU5ro1qpt9Q3EUI+lR3EESI=;\n\tb=kGzUN/mTvT3Q3M4r30+jNbhKAhfFWmmUKDu23KHa+MbnQvQ3YTPW3HNC4oNEljj90R\n\twVdoKMsUvOQRdpld8yTcgWtpcBxCzabx1n6lF94DfV8HJaXXBmBTWavShk00C7+v7NG6\n\tMK79wDsMg4wZQz7jPeYFdlEr+ShCr0UtVZPYDcpTI9RWUlfmqkszpytpSVIJI1j29vZ8\n\ta36oqGobdDVIZ+gYbRUFPd3QvuRwE3re2l22jConIjceg6rjxnhod/esa911MgTr7jsX\n\t7Nz1rh4tjdx6BmH/k+LZRJ3bSQ9xqO3H1CZ9iJ97b93dCcCvKv6AFRgVbkECJoy8N/rv\n\tCyKw=="],"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=\"kGzUN/mT\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680674586; x=1683266586;\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=7ZJ9JU8V5OWeahxLWhu2SU5ro1qpt9Q3EUI+lR3EESI=;\n\tb=msOXZVKaL9QyyQ2XLoVO6TE9hmqf6kLd6IWtCRB+/GE7hfhTvmXgJc/mDM0Nbdh+g3\n\t4H+Jq0nTBzB5OGVUuoIdvnqaKZzOLb7UPq/RY/9RYavxtJ5oBmli85z3Y/fYZb7whkP6\n\t2K9BfMaKgngEH5z9qnLx/kNCP4+DpEdwZdHeCvHQivynrErk1Qlvfu4mKyfbY1sClaEm\n\tfDgBjVtWZZLKiHBSDCVO8LKuwS6rXLzLnZpzE24p1MJpznGeHptU9Tsh3+490b3QnqGz\n\tPNB8ekOuRtLY4Qa3Xi52MJLSnLE/YSQ13StCBIfVkf/KljWCZ3HWe1gnsuriTk/92WeC\n\tNObg==","X-Gm-Message-State":"AAQBX9cPqIuXZjSSUGif5a06nL+ISi6/7oPYnRrXJc9K3qIlx5q0Mz4l\n\taGCCef/BFvgdhrlUKxP/croNe19znQ6n+4qI4Ciyeg==","X-Google-Smtp-Source":"AKy350ZP+caUpdcuMTQti4VIOWXE9Ac+7rp+DomX/IG1quTnuCdV1dbq6L4ClAvVNG76FEMrdif/eVNq9euILoILHKo=","X-Received":"by 2002:a17:907:961a:b0:8af:22b4:99d2 with SMTP id\n\tgb26-20020a170907961a00b008af22b499d2mr1793309ejc.5.1680674585968;\n\tTue, 04 Apr 2023 23:03:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>\n\t\n\t<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>","In-Reply-To":"<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>","Date":"Wed, 5 Apr 2023 08:02:54 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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,\n\tNick Hollinghurst ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26861,"web_url":"https://patchwork.libcamera.org/comment/26861/","msgid":"<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>","date":"2023-04-05T14:39:38","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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 Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote:\n> On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Daniel\n> > cc RPi folks which originally introduced the AF controls and which\n> > have recently been working a new implementation\n> >\n> > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi Daniel\n> > > > sorry I have neglected the Windows related patches in previous\n> > > > reviews as I hoped someone would have shared an opinion on the\n> > > > Signal-based mechanism.\n> > > >\n> > > > I don't have better ideas to propose, so let's go with it\n> > > >\n> > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > Add support for setting user defined auto focus window in the\n> > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > > > Each time, there is a need for changing the window configuration in the\n> > > > > ISP, the signal is emitted. Platform layer that wants to use\n> > > > > the \"Windows\" metering mode, needs to connect to this signal\n> > > > > and configure the ISP on each emission.\n> > > > >\n> > > > > Currently only one window is supported.\n> > > > >\n> > > > > Signed-off-by: Daniel Semkowicz \n> > > > > ---\n> > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > index 244b8803..0fb17df3 100644\n> > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > > > * movement range rather than coarse search result.\n> > > > > * \\todo Implement setRange.\n> > > > > * \\todo Implement setSpeed.\n> > > > > - * \\todo Implement setMeteringMode.\n> > > > > - * \\todo Implement setWindows.\n> > > > > * \\todo Implement the AfPauseDeferred mode.\n> > > > > + * \\todo Implement support for multiple AF windows.\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > > > + * \\param[in] outputSize Camera sensor output size\n> > > > > + *\n> > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > > > + * method of the platform layer.\n> > > > > + */\n> > > > > +void AfHillClimbing::configure(const Size &outputSize)\n> > > >\n> > > > According to the AfWindows control definition\n> > > >\n> > > > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > > > to AfMeteringWindows. The units used are pixels within the rectangle\n> > > > returned by the ScalerCropMaximum property.\n> > > >\n> > > > The final sensor's output size can be obtained by downscaling/cropping\n> > > > a larger portion of the pixel array. The portion of the pixel array\n> > > > processed to obtain the final image is named AnalogueCropRectangle and\n> > > > all valid crop rectangles lie inside it.\n> > > >\n> > > > Most of our controls, such as ScalerCrop, are expressed with the\n> > > > AnalogueCrop as reference, to allow expressing them regardless of the\n> > > > current output size. It's up to the pipeline handler to re-scale any\n> > > > valid control in the current configured output.\n> > >\n> > > For the 1024x768 stream, I have the following parameters:\n> > > - activeAreaSize: 2592x1944\n> > > - analogCrop: (0, 0)/2592x1944\n> > > - outputSize: 1296x972\n> > >\n> > > When using analogCrop, I will also need to scale the AF window\n> > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> > > values in reference to the ISP input size\n> > > (IPACameraSensorInfo::outputSize).\n> > > Based on what you wrote earlier, I understand that it is supposed to be\n> > > like that?\n> > >\n> >\n> > I presume so.\n> >\n> > I should check how RPi handles windowing\n> >\n> > > >\n> > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > > > probably to allow pipeline handler express any additional processing\n> > > > margins required by any cropping/scaling that happens on the ISP.\n> > > >\n> > > > However as the RkISP1 pipeline doesn't express any of those margin\n> > > > yet, I would simply use here the sensor's analogue crop, which is part\n> > > > of the IPACameraSensorInfo you already use in the platform layer.\n> > > >\n> > >\n> > > I have some concerns about the ScalerCropMaximum.\n> > > What if the ISP have different size margins for cropping and auto-focus\n> > > windows? I see there are margins defined for the AF window, but nothing\n> > > about the cropping margins in the RK3399 documentation. In this case,\n> > > it will not be a problem, but what if some ISP will have margins for\n> > > cropping, but no margins for AF window?\n> > >\n> >\n> > Good point. Do we need an AfWindowMaximum ?\n>\n> To make sense, AfWindowMaximum would need to be expressed in reference\n> to how ISP handles that (sensor output size on rkisp) to allow precise\n> AF window configuration. AF window margins on rkisp are small: 2px and\n> 5px. I would like to avoid scaling, so small values compared to\n> the output size.\n\nFor ScalerCrop we decided to have a ScalerCropMaximum property which\nbasically reports the AnalogueCrop rectangle, as this is the maximum\nrectangle an application can set as a cropping area. The reference is\nthe PixelArrayActiveAreas size.\n\nFor AfWindows we don't care about AnalogCrop but only about the\nsensor's output as, if my understanding is correct, this is the\nrectangle on which the ISP performs windowing on.\n\nAs we don't expose the sensor's output size to applications, the only\nreference we can use is full PixelArrayActiveAreas and re-scale before\nsetting the windows.\n\n> I am not sure if AfWindowMaximum as additional property is actually\n> needed. Maximum value for AfWindows would not be enough?\n>\n\nAs a comment on the ScalerCropMaximum property reports\n\n \\todo Turn this property into a \"maximum control value\" for the\n ScalerCrop control once \"dynamic\" controls have been implemented.\n\nI presume this is very old, as right now (at least for RkISP1) I see\nthe \"ControlInfoMap *ipaControls\" parameter passed in to configure() and\nbeing overwritten completely with new control limits.\n\nSo my suggestion for a new property was probably not correct.\n\n> Is there a need to expose the window margins to the user at all?\n\nI think so, it would avoid setting controls which can't be applied as\nthey are.\n\nI presume you can initialize the maximum value of AfWindows as\n\n { 0, 0, PixelArrayActiveAreas.width - 4,\n PixelArrayActiveAreas.height - 10 }\n\nWhen re-scaling, you first need to translate it to take into\nconsideration the processing margins then re-scale it using the\nactiveArea-to-outputSize ratio.\n\n(only compiled, not run-time tested)\n\n Span userWindows =\n *request->controls().get>(controls::AfWindows);\n std::vector afWindows;\n\n for (auto userWindow : userWindows) {\n Rectangle window = userWindow.translatedBy({2, 5});\n window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize);\n afWindows.push_back(window);\n }\n\n (as you only support one window, this could be even\n simplified)\n\nNow, I would need to do the re-scaling in the RkISP1 Af algorithm\nimplementation, and you would need to pass to the algorithm the active\narea size. One way to do so, as algorithms have access to the\ncontext_, is to add the active area size to\nIPASessionConfiguration::sensor which already contains the sensor's\noutput size.\n\nWhat do you think ?\n\n> We could allow the window size of the whole ISP input size to be set\n> by the user and clamp the size in the IPA before configuring the ISP?\n>\n> In summary, I see two options:\n> 1. Maximum AF window expressed in units that are directly set to ISP\n> (output size for rkisp). Maximum AF window will include margins.\n> 2. Keep the current approach and express the AF window in reference to\n> Analogue crop size. Just allow the full size instead of the\n> ScalerCropMaximum. Clamp the margins inside the IPA if user\n> requested larger window.\n>\n> >\n> > RPi any opinions here ?\n> >\n> > > > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > > > something similar.\n> > > >\n> > > > I would also initialize userWindow_ to the same value, so that if an\n> > > > application switches to AfMeteringWindows mode the rectangle is at\n> > > > least initialized to something.\n> > >\n> > > Good point!\n> > >\n> > > >\n> > > >\n> > > > > +{\n> > > > > + /*\n> > > > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > > > + * placed at the center\n> > > > > + */\n> > > > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > > > + static_cast(outputSize.height / 8),\n> > > > > + 3 * outputSize.width / 4,\n> > > > > + 3 * outputSize.height / 4);\n> > > > > +\n> > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Run the auto focus algorithm loop\n> > > > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > > > framesToSkip_ = n;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > > > + * \\brief Signal emitted when change in AF window was requested\n> > > > > + *\n> > > > > + * Platform layer supporting AF windows should connect to this signal\n> > > > > + * and configure the ISP with new window on each emition.\n> > > > > + */\n> > > > > +\n> > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > > {\n> > > > > if (mode == mode_)\n> > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > > > }\n> > > > >\n> > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > > > {\n> > > > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > > > + if (metering == meteringMode_)\n> > > > > + return;\n> > > > > +\n> > > > > + meteringMode_ = metering;\n> > > > > +\n> > > > > + switch (metering) {\n> > > > > + case controls::AfMeteringAuto:\n> > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > + break;\n> > > > > + case controls::AfMeteringWindows:\n> > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > + break;\n> > > > > + }\n> > > > > }\n> > > > >\n> > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > > +void AfHillClimbing::setWindows(Span windows)\n> > > > > {\n> > > > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > > > + if (windows.size() != 1) {\n> > > > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > > > + return;\n> > > > > + }\n> > > >\n> > > >\n> > > > Should this be an hard error or should we just want and continue by\n> > > > only considering the first available rectangle ?\n> > >\n> > > I will change it to a warning that only the first window was used.\n> > >\n> > > >\n> > > > > +\n> > > > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > > > +\n> > > > > + userWindow_ = windows[0];\n> > > > > +\n> > > > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > }\n> > > > >\n> > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > index 2147939b..0f7c65db 100644\n> > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > @@ -10,6 +10,7 @@\n> > > > > #pragma once\n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > >\n> > > > > #include \"af.h\"\n> > > > >\n> > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > > > public:\n> > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > const YamlObject &tuningData);\n> > > > > + void configure(const Size &outputSize);\n> > > > > int32_t process(double currentContrast);\n> > > > > void skipFrames(uint32_t n);\n> > > > >\n> > > > > controls::AfStateEnum state() override { return state_; }\n> > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > > > >\n> > > > > + Signal windowUpdateRequested;\n> > > > > +\n> > > > > private:\n> > > > > void setMode(controls::AfModeEnum mode) override;\n> > > > > void setRange(controls::AfRangeEnum range) override;\n> > > > > @@ -54,6 +58,7 @@ private:\n> > > > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > > > >\n> > > > > /* Current focus lens position. */\n> > > > > int32_t lensPosition_ = 0;\n> > > > > @@ -84,6 +89,11 @@ private:\n> > > > >\n> > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > > double maxChange_;\n> > > > > +\n> > > > > + /* Default AF window. */\n> > > > > + Rectangle defaultWindow_;\n> > > > > + /* AF window set by user using AF_WINDOWS control. */\n> > > > > + Rectangle userWindow_;\n> > > > > };\n> > > > >\n> > > > > } /* namespace ipa::algorithms */\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 7AD1BC0F2A\n\tfor ;\n\tWed, 5 Apr 2023 14:39:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92D0962756;\n\tWed, 5 Apr 2023 16:39:43 +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 C7A3C603A4\n\tfor ;\n\tWed, 5 Apr 2023 16:39:41 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21653183F;\n\tWed, 5 Apr 2023 16:39:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680705583;\n\tbh=pH5FtuO7Z0N/vn98OUKjT1ebhvRavXfMiBxFYy5sTmo=;\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=eRY+YJoudepHmNHTuic2jlAU9R7PgGJ329DMy6TEYIW+U/7UuRSSFQOxltbxCZ2Rd\n\t8SIB+WG6bZZQe0pRRVnQQlITk9b6BiE34n6cAlEOxaXmufXGkMldyEA1RuhQFEPeoo\n\tnzNAGtEubmAqTjC2A07OsEKMnOC4RsGllY3QK+G1xngAnPiKSppNsYD4aj5Wosx5MK\n\tC9ABmvjPVVu38w7H319kxjLaWyw3YVi/TmIgNygXpzwf/rgLQp/C40P1JeWv/nyhdH\n\tIyA8Ubzu1w9aGapOGvdScznYlFwEJ4ULYC8J0RwyTlzB4EY2Ruuc3TOejJABAn1URe\n\tv6l5jcxgVtV2g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680705581;\n\tbh=pH5FtuO7Z0N/vn98OUKjT1ebhvRavXfMiBxFYy5sTmo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sxIFTLJonM8ua9MpPU5F21omokwvn/iiGZZitN1ZJKGe8+UxB5YSBvRCOwNiAFBoa\n\tMdKrJtGKHDoV6/aHcLyO5W8kCXMS8GcQKK474ewgoKhm51OG6cwiWmCMhlIi0rHI2Z\n\tYAQvnlQMO/GPBY3b66DaaD6BJvWIE+OeLqtb95NY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"sxIFTLJo\"; dkim-atps=neutral","Date":"Wed, 5 Apr 2023 16:39:38 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>\n\t\n\t<20230404105911.7e5cp5ymzh2udasb@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 05/10] ipa: af_hill_climbing: 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,\n\tNick Hollinghurst ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26932,"web_url":"https://patchwork.libcamera.org/comment/26932/","msgid":"","date":"2023-04-24T14:15:22","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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 Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n>\n> On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote:\n> > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Daniel\n> > > cc RPi folks which originally introduced the AF controls and which\n> > > have recently been working a new implementation\n> > >\n> > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi\n> > > > wrote:\n> > > > >\n> > > > > Hi Daniel\n> > > > > sorry I have neglected the Windows related patches in previous\n> > > > > reviews as I hoped someone would have shared an opinion on the\n> > > > > Signal-based mechanism.\n> > > > >\n> > > > > I don't have better ideas to propose, so let's go with it\n> > > > >\n> > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > Add support for setting user defined auto focus window in the\n> > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > > > > Each time, there is a need for changing the window configuration in the\n> > > > > > ISP, the signal is emitted. Platform layer that wants to use\n> > > > > > the \"Windows\" metering mode, needs to connect to this signal\n> > > > > > and configure the ISP on each emission.\n> > > > > >\n> > > > > > Currently only one window is supported.\n> > > > > >\n> > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > ---\n> > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > > > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > index 244b8803..0fb17df3 100644\n> > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > > > > * movement range rather than coarse search result.\n> > > > > > * \\todo Implement setRange.\n> > > > > > * \\todo Implement setSpeed.\n> > > > > > - * \\todo Implement setMeteringMode.\n> > > > > > - * \\todo Implement setWindows.\n> > > > > > * \\todo Implement the AfPauseDeferred mode.\n> > > > > > + * \\todo Implement support for multiple AF windows.\n> > > > > > */\n> > > > > >\n> > > > > > /**\n> > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > > > > + * \\param[in] outputSize Camera sensor output size\n> > > > > > + *\n> > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > > > > + * method of the platform layer.\n> > > > > > + */\n> > > > > > +void AfHillClimbing::configure(const Size &outputSize)\n> > > > >\n> > > > > According to the AfWindows control definition\n> > > > >\n> > > > > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > > > > to AfMeteringWindows. The units used are pixels within the rectangle\n> > > > > returned by the ScalerCropMaximum property.\n> > > > >\n> > > > > The final sensor's output size can be obtained by downscaling/cropping\n> > > > > a larger portion of the pixel array. The portion of the pixel array\n> > > > > processed to obtain the final image is named AnalogueCropRectangle and\n> > > > > all valid crop rectangles lie inside it.\n> > > > >\n> > > > > Most of our controls, such as ScalerCrop, are expressed with the\n> > > > > AnalogueCrop as reference, to allow expressing them regardless of the\n> > > > > current output size. It's up to the pipeline handler to re-scale any\n> > > > > valid control in the current configured output.\n> > > >\n> > > > For the 1024x768 stream, I have the following parameters:\n> > > > - activeAreaSize: 2592x1944\n> > > > - analogCrop: (0, 0)/2592x1944\n> > > > - outputSize: 1296x972\n> > > >\n> > > > When using analogCrop, I will also need to scale the AF window\n> > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> > > > values in reference to the ISP input size\n> > > > (IPACameraSensorInfo::outputSize).\n> > > > Based on what you wrote earlier, I understand that it is supposed to be\n> > > > like that?\n> > > >\n> > >\n> > > I presume so.\n> > >\n> > > I should check how RPi handles windowing\n> > >\n> > > > >\n> > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > > > > probably to allow pipeline handler express any additional processing\n> > > > > margins required by any cropping/scaling that happens on the ISP.\n> > > > >\n> > > > > However as the RkISP1 pipeline doesn't express any of those margin\n> > > > > yet, I would simply use here the sensor's analogue crop, which is part\n> > > > > of the IPACameraSensorInfo you already use in the platform layer.\n> > > > >\n> > > >\n> > > > I have some concerns about the ScalerCropMaximum.\n> > > > What if the ISP have different size margins for cropping and auto-focus\n> > > > windows? I see there are margins defined for the AF window, but nothing\n> > > > about the cropping margins in the RK3399 documentation. In this case,\n> > > > it will not be a problem, but what if some ISP will have margins for\n> > > > cropping, but no margins for AF window?\n> > > >\n> > >\n> > > Good point. Do we need an AfWindowMaximum ?\n> >\n> > To make sense, AfWindowMaximum would need to be expressed in reference\n> > to how ISP handles that (sensor output size on rkisp) to allow precise\n> > AF window configuration. AF window margins on rkisp are small: 2px and\n> > 5px. I would like to avoid scaling, so small values compared to\n> > the output size.\n>\n> For ScalerCrop we decided to have a ScalerCropMaximum property which\n> basically reports the AnalogueCrop rectangle, as this is the maximum\n> rectangle an application can set as a cropping area. The reference is\n> the PixelArrayActiveAreas size.\n>\n> For AfWindows we don't care about AnalogCrop but only about the\n> sensor's output as, if my understanding is correct, this is the\n> rectangle on which the ISP performs windowing on.\n>\n> As we don't expose the sensor's output size to applications, the only\n> reference we can use is full PixelArrayActiveAreas and re-scale before\n> setting the windows.\n>\n> > I am not sure if AfWindowMaximum as additional property is actually\n> > needed. Maximum value for AfWindows would not be enough?\n> >\n>\n> As a comment on the ScalerCropMaximum property reports\n>\n> \\todo Turn this property into a \"maximum control value\" for the\n> ScalerCrop control once \"dynamic\" controls have been implemented.\n>\n> I presume this is very old, as right now (at least for RkISP1) I see\n> the \"ControlInfoMap *ipaControls\" parameter passed in to configure() and\n> being overwritten completely with new control limits.\n>\n> So my suggestion for a new property was probably not correct.\n>\n> > Is there a need to expose the window margins to the user at all?\n>\n> I think so, it would avoid setting controls which can't be applied as\n> they are.\n>\n> I presume you can initialize the maximum value of AfWindows as\n>\n> { 0, 0, PixelArrayActiveAreas.width - 4,\n> PixelArrayActiveAreas.height - 10 }\n>\n> When re-scaling, you first need to translate it to take into\n> consideration the processing margins then re-scale it using the\n> activeArea-to-outputSize ratio.\n>\n> (only compiled, not run-time tested)\n>\n> Span userWindows =\n> *request->controls().get>(controls::AfWindows);\n> std::vector afWindows;\n>\n> for (auto userWindow : userWindows) {\n> Rectangle window = userWindow.translatedBy({2, 5});\n> window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize);\n> afWindows.push_back(window);\n> }\n>\n> (as you only support one window, this could be even\n> simplified)\n>\n> Now, I would need to do the re-scaling in the RkISP1 Af algorithm\n> implementation, and you would need to pass to the algorithm the active\n> area size. One way to do so, as algorithms have access to the\n> context_, is to add the active area size to\n> IPASessionConfiguration::sensor which already contains the sensor's\n> output size.\n>\n> What do you think ?\n\nI am not fully happy with this approach as this requires scaling\nthe values two times, but as you said, expressing it directly\nin the output size would require exposing additional controls.\n\nThe other way, I was thinking of, is to express the AF window\nin the final output image size (1024x768 in my already mentioned\nexample). This way, it is easier to think about from the user\nperspective. However, this is an opposite approach than the already\nexisting one for the ScalerCrop.\n\nMaybe I am not 100% happy with this, but it looks that expressing\nthe AF window as you show above (in reference to the current\nPixelArrayActiveAreas) makes the most sense for the existing code,\nbecause it will be the common approach with the ScalerCrop.\nI can change my implementation to this approach.\n\nDo we need to change also the documentation related to the AF Windows?\n\nBest regards\nDaniel\n\n>\n> > We could allow the window size of the whole ISP input size to be set\n> > by the user and clamp the size in the IPA before configuring the ISP?\n> >\n> > In summary, I see two options:\n> > 1. Maximum AF window expressed in units that are directly set to ISP\n> > (output size for rkisp). Maximum AF window will include margins.\n> > 2. Keep the current approach and express the AF window in reference to\n> > Analogue crop size. Just allow the full size instead of the\n> > ScalerCropMaximum. Clamp the margins inside the IPA if user\n> > requested larger window.\n> >\n> > >\n> > > RPi any opinions here ?\n> > >\n> > > > > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > > > > something similar.\n> > > > >\n> > > > > I would also initialize userWindow_ to the same value, so that if an\n> > > > > application switches to AfMeteringWindows mode the rectangle is at\n> > > > > least initialized to something.\n> > > >\n> > > > Good point!\n> > > >\n> > > > >\n> > > > >\n> > > > > > +{\n> > > > > > + /*\n> > > > > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > > > > + * placed at the center\n> > > > > > + */\n> > > > > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > > > > + static_cast(outputSize.height / 8),\n> > > > > > + 3 * outputSize.width / 4,\n> > > > > > + 3 * outputSize.height / 4);\n> > > > > > +\n> > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\brief Run the auto focus algorithm loop\n> > > > > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > > > > framesToSkip_ = n;\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > > > > + * \\brief Signal emitted when change in AF window was requested\n> > > > > > + *\n> > > > > > + * Platform layer supporting AF windows should connect to this signal\n> > > > > > + * and configure the ISP with new window on each emition.\n> > > > > > + */\n> > > > > > +\n> > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > > > {\n> > > > > > if (mode == mode_)\n> > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > > > > }\n> > > > > >\n> > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > > > > {\n> > > > > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > > > > + if (metering == meteringMode_)\n> > > > > > + return;\n> > > > > > +\n> > > > > > + meteringMode_ = metering;\n> > > > > > +\n> > > > > > + switch (metering) {\n> > > > > > + case controls::AfMeteringAuto:\n> > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > + break;\n> > > > > > + case controls::AfMeteringWindows:\n> > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > + break;\n> > > > > > + }\n> > > > > > }\n> > > > > >\n> > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > > > +void AfHillClimbing::setWindows(Span windows)\n> > > > > > {\n> > > > > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > > > > + if (windows.size() != 1) {\n> > > > > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > > > > + return;\n> > > > > > + }\n> > > > >\n> > > > >\n> > > > > Should this be an hard error or should we just want and continue by\n> > > > > only considering the first available rectangle ?\n> > > >\n> > > > I will change it to a warning that only the first window was used.\n> > > >\n> > > > >\n> > > > > > +\n> > > > > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > > > > +\n> > > > > > + userWindow_ = windows[0];\n> > > > > > +\n> > > > > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > }\n> > > > > >\n> > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > index 2147939b..0f7c65db 100644\n> > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > @@ -10,6 +10,7 @@\n> > > > > > #pragma once\n> > > > > >\n> > > > > > #include \n> > > > > > +#include \n> > > > > >\n> > > > > > #include \"af.h\"\n> > > > > >\n> > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > > > > public:\n> > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > const YamlObject &tuningData);\n> > > > > > + void configure(const Size &outputSize);\n> > > > > > int32_t process(double currentContrast);\n> > > > > > void skipFrames(uint32_t n);\n> > > > > >\n> > > > > > controls::AfStateEnum state() override { return state_; }\n> > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > > > > >\n> > > > > > + Signal windowUpdateRequested;\n> > > > > > +\n> > > > > > private:\n> > > > > > void setMode(controls::AfModeEnum mode) override;\n> > > > > > void setRange(controls::AfRangeEnum range) override;\n> > > > > > @@ -54,6 +58,7 @@ private:\n> > > > > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > > > > >\n> > > > > > /* Current focus lens position. */\n> > > > > > int32_t lensPosition_ = 0;\n> > > > > > @@ -84,6 +89,11 @@ private:\n> > > > > >\n> > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > > > double maxChange_;\n> > > > > > +\n> > > > > > + /* Default AF window. */\n> > > > > > + Rectangle defaultWindow_;\n> > > > > > + /* AF window set by user using AF_WINDOWS control. */\n> > > > > > + Rectangle userWindow_;\n> > > > > > };\n> > > > > >\n> > > > > > } /* namespace ipa::algorithms */\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 9494AC3272\n\tfor ;\n\tMon, 24 Apr 2023 14:15:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FAD8627D5;\n\tMon, 24 Apr 2023 16:15:35 +0200 (CEST)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3959061EB1\n\tfor ;\n\tMon, 24 Apr 2023 16:15:34 +0200 (CEST)","by mail-ed1-x529.google.com with SMTP id\n\t4fb4d7f45d1cf-50685f1b6e0so8243641a12.0\n\tfor ;\n\tMon, 24 Apr 2023 07:15:34 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682345735;\n\tbh=mTmptkf82h2J02ta2+U+AuDnKeseHFLazdKLEJ6P8yo=;\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=Lla8PgJ4B4SZ8H4qh7tKQfDV3pNjH0I7FYXMrEXOfOgasajNhS5wbTrILHQsbrQ1O\n\txufE1bCX3qYTSfaCpOqAcp5R2Px7KOsA0biQUAIfrQeXOwQwZx89hN1oBSjNS5D68A\n\t2HFKqr0Q9dcbcvqRPJcFzDwGzvItU+EVtrRL9XgHHOiFH12jVXW2+avEH4lp7YWodz\n\t7PJ1l98pyVvohA2w1kreThyp2u/HvBWuJ+FgUaCD7N9WT37jMrPERNtrXawHsrvS7Z\n\tnbTvMHCm3b/m2qzj42IgdF2fIfV8naaesoYg4nGVr0DTfsZ9axvV+HM+7XmWwlhAOJ\n\tHCFQZARA8fQwA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1682345733;\n\tx=1684937733; \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=qzvbsA2N5uQRDZ1/q2Mb4EAgMIGI0cIGQMdrYMP24WM=;\n\tb=per4qBbCVP3xWGXwJA5N/a3XDCcu7TqOjmAqm1PgUvp94ZnczruDYzR8A1qRnPEStl\n\tSVDPwUoHxDMTVX1mltweO+FI+RoANECgdm5OcYDX1QoUBe/sWCWvP0wBE2aLXvarxb0P\n\tVMG37mc8NGD+0gqQjZ54ZSaVuTAiVdTDKRTELPfC3gb0e6WDNT+w7FXkSsCJ0LtTBPds\n\tFp0WHGd0KucFLqRB67cVk0+fkQzsgVAkCxsijeH2+p3rGPDZcElqfHq3KHsMBOsoDj+S\n\t5Rb9ZYFAvuuK04ARtYCsHO7lObmnlOVZObzEUPq4pNrgzHcB3b7/HGMtSk6D2QC0G3Cx\n\tx59Q=="],"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=\"per4qBbC\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682345733; x=1684937733;\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=qzvbsA2N5uQRDZ1/q2Mb4EAgMIGI0cIGQMdrYMP24WM=;\n\tb=QAKfUh1+ZHdBjY0ptceKv2Xp7QRbfrYHdgpGfaEg8vpUiXz0HThUBXLB3H7ejNjBpD\n\tsgf168xss1JqyYCiQK1r4vl5Y7YDbYNZf0CmatTDj3cqJYUgBt2fFSkckJQugFBh9mpC\n\tVSbyQ7q4aHwzLN3o8Tl9gLcawkpDq/K0f4zek0sFNHByGjsjpNzz9z6CvpdSMmLVAqAL\n\tJkoJRSh3AK3jwpYEpkhQnUwk8l3sPEBCKNu21AkvGDcvQxDzW2aD6iS9Ej4hWl4WdvGY\n\td0BlPU4715Db32Vx2SHS2O4XhhP+DaisCRUX8H3iocQF6RbrMB7eb1wDfxsjp+5mwt3s\n\t3T0w==","X-Gm-Message-State":"AAQBX9dUx7UJuekgPuDQTqFi4LJENgQa7bkjXQF93GbWMzXTTUtCflUc\n\t6dcmGFnKKf609glI4ZSLiBbu+j56CYTAXdSWYOmqXg==","X-Google-Smtp-Source":"AKy350Y64R5Pns2xxb/4kDUllxnV3/jaGLCr6ORd6XHmGRjlKpWcPiL0UaC8/IRAi1XQlGRoq5lkqIR5gxvQkjhsNio=","X-Received":"by 2002:aa7:dc10:0:b0:506:bf63:c4ed with SMTP id\n\tb16-20020aa7dc10000000b00506bf63c4edmr12496763edu.20.1682345733606;\n\tMon, 24 Apr 2023 07:15:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>\n\t\n\t<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>\n\t\n\t<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>","In-Reply-To":"<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>","Date":"Mon, 24 Apr 2023 16:15:22 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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,\n\tNick Hollinghurst ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26944,"web_url":"https://patchwork.libcamera.org/comment/26944/","msgid":"","date":"2023-04-26T16:29:51","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: Add\n\t\"Windows\" metering mode","submitter":{"id":130,"url":"https://patchwork.libcamera.org/api/people/130/","name":"Nick Hollinghurst","email":"nick.hollinghurst@raspberrypi.com"},"content":"Hi Laurent, Daniel,\n\nOn Wed, 26 Apr 2023 at 04:12, Laurent Pinchart\n wrote:\n>\n> Hi Daniel,\n>\n> (Nick, there's a question for you below)\n>\n> On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote:\n> > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote:\n> > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi Daniel\n> > > > > cc RPi folks which originally introduced the AF controls and which\n> > > > > have recently been working a new implementation\n> > > > >\n> > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote:\n> > > > > > >\n> > > > > > > Hi Daniel\n> > > > > > > sorry I have neglected the Windows related patches in previous\n> > > > > > > reviews as I hoped someone would have shared an opinion on the\n> > > > > > > Signal-based mechanism.\n> > > > > > >\n> > > > > > > I don't have better ideas to propose, so let's go with it\n>\n> I don't like the signal much either. We could simply implement a\n> function to retrieve the window.\n>\n> > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > > Add support for setting user defined auto focus window in the\n> > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > > > > > > Each time, there is a need for changing the window configuration in the\n>\n> s/Each time,/Each time/\n>\n> > > > > > > > ISP, the signal is emitted. Platform layer that wants to use\n>\n> s/layer that wants/Layers that want/\n>\n> > > > > > > > the \"Windows\" metering mode, needs to connect to this signal\n>\n> s/mode, needs/mode need/\n>\n> > > > > > > > and configure the ISP on each emission.\n> > > > > > > >\n> > > > > > > > Currently only one window is supported.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > > > ---\n> > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > index 244b8803..0fb17df3 100644\n> > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > > > > > > * movement range rather than coarse search result.\n> > > > > > > > * \\todo Implement setRange.\n> > > > > > > > * \\todo Implement setSpeed.\n> > > > > > > > - * \\todo Implement setMeteringMode.\n> > > > > > > > - * \\todo Implement setWindows.\n> > > > > > > > * \\todo Implement the AfPauseDeferred mode.\n> > > > > > > > + * \\todo Implement support for multiple AF windows.\n> > > > > > > > */\n> > > > > > > >\n> > > > > > > > /**\n> > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > return 0;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > > > > > > + * \\param[in] outputSize Camera sensor output size\n> > > > > > > > + *\n> > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n>\n> s/method/function/\n>\n> > > > > > > > + * method of the platform layer.\n> > > > > > > > + */\n> > > > > > > > +void AfHillClimbing::configure(const Size &outputSize)\n> > > > > > >\n> > > > > > > According to the AfWindows control definition\n> > > > > > >\n> > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle\n> > > > > > > returned by the ScalerCropMaximum property.\n> > > > > > >\n> > > > > > > The final sensor's output size can be obtained by downscaling/cropping\n> > > > > > > a larger portion of the pixel array. The portion of the pixel array\n> > > > > > > processed to obtain the final image is named AnalogueCropRectangle and\n> > > > > > > all valid crop rectangles lie inside it.\n> > > > > > >\n> > > > > > > Most of our controls, such as ScalerCrop, are expressed with the\n> > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the\n> > > > > > > current output size. It's up to the pipeline handler to re-scale any\n> > > > > > > valid control in the current configured output.\n> > > > > >\n> > > > > > For the 1024x768 stream, I have the following parameters:\n> > > > > > - activeAreaSize: 2592x1944\n> > > > > > - analogCrop: (0, 0)/2592x1944\n> > > > > > - outputSize: 1296x972\n> > > > > >\n> > > > > > When using analogCrop, I will also need to scale the AF window\n> > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> > > > > > values in reference to the ISP input size\n> > > > > > (IPACameraSensorInfo::outputSize).\n> > > > > > Based on what you wrote earlier, I understand that it is supposed to be\n> > > > > > like that?\n> > > > >\n> > > > > I presume so.\n> > > > >\n> > > > > I should check how RPi handles windowing\n>\n> Nick, that's the first question for you.\n\nOur implementation is based on what we understand to be the current\nlibcamera specification: That AF windows are in the same coordinate\nsystem as ScalerCropMaximum, i.e. raw sensor pixels.\n\nI found the documentation phrase \"The units used are pixels within the\nrectangle...\" ambiguous -- does it suggest that the origin of the\nAfWIndows coordinate system is the top-left corner of\nScalerCropMaximum? I took the answer to be \"no\", that those rectangles\nwere in a common coordinate system, without offsetting. This in turn\nmeans that Af Windows will stick to the same objects in the scene,\nregardless of mode or digital zoom.\n\n\n> > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > > > > > > probably to allow pipeline handler express any additional processing\n> > > > > > > margins required by any cropping/scaling that happens on the ISP.\n> > > > > > >\n> > > > > > > However as the RkISP1 pipeline doesn't express any of those margin\n> > > > > > > yet, I would simply use here the sensor's analogue crop, which is part\n> > > > > > > of the IPACameraSensorInfo you already use in the platform layer.\n> > > > > > >\n> > > > > >\n> > > > > > I have some concerns about the ScalerCropMaximum.\n> > > > > > What if the ISP have different size margins for cropping and auto-focus\n> > > > > > windows? I see there are margins defined for the AF window, but nothing\n> > > > > > about the cropping margins in the RK3399 documentation. In this case,\n> > > > > > it will not be a problem, but what if some ISP will have margins for\n> > > > > > cropping, but no margins for AF window?\n> > > > >\n> > > > > Good point. Do we need an AfWindowMaximum ?\n> > > >\n> > > > To make sense, AfWindowMaximum would need to be expressed in reference\n> > > > to how ISP handles that (sensor output size on rkisp) to allow precise\n> > > > AF window configuration. AF window margins on rkisp are small: 2px and\n> > > > 5px. I would like to avoid scaling, so small values compared to\n> > > > the output size.\n> > >\n> > > For ScalerCrop we decided to have a ScalerCropMaximum property which\n> > > basically reports the AnalogueCrop rectangle, as this is the maximum\n> > > rectangle an application can set as a cropping area. The reference is\n> > > the PixelArrayActiveAreas size.\n> > >\n> > > For AfWindows we don't care about AnalogCrop but only about the\n> > > sensor's output as, if my understanding is correct, this is the\n> > > rectangle on which the ISP performs windowing on.\n> > >\n> > > As we don't expose the sensor's output size to applications, the only\n> > > reference we can use is full PixelArrayActiveAreas and re-scale before\n> > > setting the windows.\n> > >\n> > > > I am not sure if AfWindowMaximum as additional property is actually\n> > > > needed. Maximum value for AfWindows would not be enough?\n\nEncoding the largest useful window as the control maximum value (where\nthis means minimum X, Y and maximum width,height?) sounds useful. BTW\nis there any way to encode the maximum number of windows?\n\n> > >\n> > > As a comment on the ScalerCropMaximum property reports\n> > >\n> > > \\todo Turn this property into a \"maximum control value\" for the\n> > > ScalerCrop control once \"dynamic\" controls have been implemented.\n> > >\n> > > I presume this is very old, as right now (at least for RkISP1) I see\n> > > the \"ControlInfoMap *ipaControls\" parameter passed in to configure() and\n> > > being overwritten completely with new control limits.\n> > >\n> > > So my suggestion for a new property was probably not correct.\n> > >\n> > > > Is there a need to expose the window margins to the user at all?\n> > >\n> > > I think so, it would avoid setting controls which can't be applied as\n> > > they are.\n> > >\n> > > I presume you can initialize the maximum value of AfWindows as\n> > >\n> > > { 0, 0, PixelArrayActiveAreas.width - 4,\n> > > PixelArrayActiveAreas.height - 10 }\n> > >\n> > > When re-scaling, you first need to translate it to take into\n> > > consideration the processing margins then re-scale it using the\n> > > activeArea-to-outputSize ratio.\n> > >\n> > > (only compiled, not run-time tested)\n> > >\n> > > Span userWindows =\n> > > *request->controls().get>(controls::AfWindows);\n> > > std::vector afWindows;\n> > >\n> > > for (auto userWindow : userWindows) {\n> > > Rectangle window = userWindow.translatedBy({2, 5});\n> > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize);\n> > > afWindows.push_back(window);\n> > > }\n> > >\n> > > (as you only support one window, this could be even\n> > > simplified)\n> > >\n> > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm\n> > > implementation, and you would need to pass to the algorithm the active\n> > > area size. One way to do so, as algorithms have access to the\n> > > context_, is to add the active area size to\n> > > IPASessionConfiguration::sensor which already contains the sensor's\n> > > output size.\n> > >\n> > > What do you think ?\n> >\n> > I am not fully happy with this approach as this requires scaling\n> > the values two times, but as you said, expressing it directly\n> > in the output size would require exposing additional controls.\n> >\n> > The other way, I was thinking of, is to express the AF window\n> > in the final output image size (1024x768 in my already mentioned\n> > example). This way, it is easier to think about from the user\n> > perspective. However, this is an opposite approach than the already\n> > existing one for the ScalerCrop.\n> >\n> > Maybe I am not 100% happy with this, but it looks that expressing\n> > the AF window as you show above (in reference to the current\n> > PixelArrayActiveAreas) makes the most sense for the existing code,\n> > because it will be the common approach with the ScalerCrop.\n> > I can change my implementation to this approach.\n> >\n> > Do we need to change also the documentation related to the AF Windows?\n> >\n> > > > We could allow the window size of the whole ISP input size to be set\n> > > > by the user and clamp the size in the IPA before configuring the ISP?\n> > > >\n> > > > In summary, I see two options:\n> > > > 1. Maximum AF window expressed in units that are directly set to ISP\n> > > > (output size for rkisp). Maximum AF window will include margins.\n> > > > 2. Keep the current approach and express the AF window in reference to\n> > > > Analogue crop size. Just allow the full size instead of the\n> > > > ScalerCropMaximum. Clamp the margins inside the IPA if user\n> > > > requested larger window.\n> > > >\n> > > > > RPi any opinions here ?\n>\n> And this is the second question for Nick.\n\nI don't have very strong opinions. Option 2 seems the most general.\n(Again, I've been assuming there's no implicit offsetting for the\n\"reference\" rectangle -- so the only difference between those\ndefinitions is a question of cropping?)\n\nI suspect there is not a one-size-fits-all approach. Some sensors may\nbe able to generate focus information outside the cropped region;\nothers may not. Some platforms may only be able to measure focus\nwithin the final image. Cropping \"as required\" sounds like a\nreasonable way to resolve that -- but we ought to give the user some\nway to predict when a window might end up being cropped to nothing\n(and thus ignored)!\n\n> > > > > > > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > > > > > > something similar.\n> > > > > > >\n> > > > > > > I would also initialize userWindow_ to the same value, so that if an\n> > > > > > > application switches to AfMeteringWindows mode the rectangle is at\n> > > > > > > least initialized to something.\n> > > > > >\n> > > > > > Good point!\n> > > > > >\n> > > > > > > > +{\n> > > > > > > > + /*\n> > > > > > > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > > > > > > + * placed at the center\n> > > > > > > > + */\n> > > > > > > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > > > > > > + static_cast(outputSize.height / 8),\n> > > > > > > > + 3 * outputSize.width / 4,\n> > > > > > > > + 3 * outputSize.height / 4);\n> > > > > > > > +\n> > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > /**\n> > > > > > > > * \\brief Run the auto focus algorithm loop\n> > > > > > > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > > > > > > framesToSkip_ = n;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > > > > > > + * \\brief Signal emitted when change in AF window was requested\n> > > > > > > > + *\n> > > > > > > > + * Platform layer supporting AF windows should connect to this signal\n>\n> s/layer/layers/\n> s/connect to/connect/\n>\n> > > > > > > > + * and configure the ISP with new window on each emition.\n>\n> s/new window/the new window/\n> s/emition/emission/\n>\n> The new window will likely need a few frames to become effective. I'm\n> worried that the AF algorithm doesn't take that into account at all (it\n> also doesn't take the lens movement delays into account, which is\n> another of my worries). This can affect the performance and stability of\n> the auto-focus algorithm.\n>\n> How have you tested this ?\n>\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > > > > > {\n> > > > > > > > if (mode == mode_)\n> > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > > > > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > > > > > > {\n> > > > > > > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > > > > > > + if (metering == meteringMode_)\n> > > > > > > > + return;\n> > > > > > > > +\n> > > > > > > > + meteringMode_ = metering;\n> > > > > > > > +\n> > > > > > > > + switch (metering) {\n> > > > > > > > + case controls::AfMeteringAuto:\n> > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > + break;\n> > > > > > > > + case controls::AfMeteringWindows:\n> > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > + break;\n>\n> This needs a default case to avoid modifying meteringMode_. We have two\n> modes currently defined, if we add a third one, the implementation will\n> break.\n>\n> When the window is changed the contrast value may drastically change.\n> The optimal focus point will likely change too. I think you need to\n> reset the AF and possibly trigger a rescan for continuous AF mode.\n>\n> > > > > > > > + }\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > > > > > +void AfHillClimbing::setWindows(Span windows)\n> > > > > > > > {\n> > > > > > > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > > > > > > + if (windows.size() != 1) {\n> > > > > > > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > > > > > > + return;\n> > > > > > > > + }\n> > > > > > >\n> > > > > > > Should this be an hard error or should we just want and continue by\n> > > > > > > only considering the first available rectangle ?\n> > > > > >\n> > > > > > I will change it to a warning that only the first window was used.\n> > > > > >\n> > > > > > > > +\n> > > > > > > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > > > > > > +\n> > > > > > > > + userWindow_ = windows[0];\n> > > > > > > > +\n> > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > index 2147939b..0f7c65db 100644\n> > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > @@ -10,6 +10,7 @@\n> > > > > > > > #pragma once\n> > > > > > > >\n> > > > > > > > #include \n> > > > > > > > +#include \n> > > > > > > >\n> > > > > > > > #include \"af.h\"\n> > > > > > > >\n> > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > > > > > > public:\n> > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > const YamlObject &tuningData);\n> > > > > > > > + void configure(const Size &outputSize);\n> > > > > > > > int32_t process(double currentContrast);\n> > > > > > > > void skipFrames(uint32_t n);\n> > > > > > > >\n> > > > > > > > controls::AfStateEnum state() override { return state_; }\n> > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > > > > > > >\n> > > > > > > > + Signal windowUpdateRequested;\n> > > > > > > > +\n> > > > > > > > private:\n> > > > > > > > void setMode(controls::AfModeEnum mode) override;\n> > > > > > > > void setRange(controls::AfRangeEnum range) override;\n> > > > > > > > @@ -54,6 +58,7 @@ private:\n> > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > > > > > > >\n> > > > > > > > /* Current focus lens position. */\n> > > > > > > > int32_t lensPosition_ = 0;\n> > > > > > > > @@ -84,6 +89,11 @@ private:\n> > > > > > > >\n> > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > > > > > double maxChange_;\n> > > > > > > > +\n> > > > > > > > + /* Default AF window. */\n> > > > > > > > + Rectangle defaultWindow_;\n> > > > > > > > + /* AF window set by user using AF_WINDOWS control. */\n> > > > > > > > + Rectangle userWindow_;\n> > > > > > > > };\n> > > > > > > >\n> > > > > > > > } /* namespace ipa::algorithms */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nRegards,\n\n Nick","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 9E3B3BDCBD\n\tfor ;\n\tWed, 26 Apr 2023 16:30:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2B27627DF;\n\tWed, 26 Apr 2023 18:30:04 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A90B627D4\n\tfor ;\n\tWed, 26 Apr 2023 18:30:03 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id\n\t4fb4d7f45d1cf-507bdc5ca2aso12989400a12.3\n\tfor ;\n\tWed, 26 Apr 2023 09:30:03 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682526604;\n\tbh=qaCy4Bmh8QRBLi834gHMlPrKKy+RYPz1cxpLQODBb6g=;\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=bbCaxKKpHgwuOVLJqhxqcipCypFinbk/iU3SNttTdR2VEAUQ3J1QiY5q+TOZonLHs\n\tbhqYVFgW3q1BSUmsqwM5NeksQa+AumvbJeVyFu0uYnqCJZnXrIvIh8zTGs99W7E4xA\n\tQOnxSYyrH+dWmgHn6dCNBl06ZVfPpPROfazA/FZlAcMv3Ob26j5NXKgF3/lBnabtyb\n\tlx5KqXaYh9NKnnzKP/qGjDq8FcBnT6IcTuep8IPyB0uueNDhXMhcnB43YlkxXkqVfG\n\tmt5XTmkpqFrVxw9znvk2LXHfw7kegnStlibd0NR5I6LYX5rgQOynYNnQrNcaabYYR+\n\tcPVIDSgviNEtA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1682526603; x=1685118603;\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=vs5J6BrgkZUbPgeXwY1Wzb/InN376buPmAB7JinAe8U=;\n\tb=T7uWvBOc7EbKAxZlLDBimQnqNrpvbyC/ZfGwxCNKV+Ni9n9gMU1rytGvTJ9D0oew4N\n\t1I44U1HzBmnzY6/VQnlThdRi9AwNK/oNUj5wLM2NYJI2StTSfoK6GTiXQSrJMhd4+hoV\n\tTRYHHfH9tPJv7YSqzMFw0tmZj0C/QQa/41HpQEIpEm1Vd/D59S/JnUYJHmx/dNvRNSJn\n\tPfDk7d/Lcbw3eXAMuuWAvSqHnqhYzfauDkIvbjnRzEVyjXMz+RhLTVTfH85erJwEhMdS\n\tq/BCm3Huhdv0/hUXGcJ+UYHLRQUFDRNE4XcTYRxIza6znoiO0OC4BpgPL34IU6StY76H\n\t2n6A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"T7uWvBOc\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682526603; x=1685118603;\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=vs5J6BrgkZUbPgeXwY1Wzb/InN376buPmAB7JinAe8U=;\n\tb=Mr90sowrFSF0tJt/gVbaVRH0SxeZ4zedmnK5VCJpCClDrrMJcilLFhLvN7jNhBdDqD\n\tWN7KPxYkLPhEVakUyGUuEpI66cvUVj8tFDep7eK6WGjFxV0Z39Xpe+wlbqPvQ5n+Vdfj\n\tsfyTpyqJM+RAHTiWVE8PoYKuYwVCZ0CpCZCGj4fvD6kWBbrI0Ty579FsGjrWGhzvtxYl\n\tNF7UpgvkSaAMdMsuywhY3jsIL8MfjqHB6R4XqffCtgWlTj+uJEj0bU4BjgAOpxw72OTp\n\taO/xzeVs9l6mogsLgL4Kb9b4ZPvK55sxK9CgzfgoElK0H6CDwgvX3o5/6sXQ/a5hTSRR\n\t/pig==","X-Gm-Message-State":"AAQBX9e3QZGiz2RiNca/qL9Zz3w/7cMwqpUhwwoPE1mdx9EMucHwPiIK\n\tS6jxtPrW46LTLPkEna11QyT6zdXSzPDvKcNT8dEa/w==","X-Google-Smtp-Source":"AKy350bXlDTXxFPp/kc49YGAMEZCyV3r26J9PtjxQFyfdA13O1zswooBQ73V+s30Jl5/oX6OgOGMYuLKKGQBhJuHgwc=","X-Received":"by 2002:a05:6402:3d1:b0:506:be3f:ebb1 with SMTP id\n\tt17-20020a05640203d100b00506be3febb1mr17695987edw.26.1682526602680;\n\tWed, 26 Apr 2023 09:30:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>\n\t\n\t<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>\n\t\n\t<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>\n\t\n\t<20230426031305.GF1667@pendragon.ideasonboard.com>","In-Reply-To":"<20230426031305.GF1667@pendragon.ideasonboard.com>","Date":"Wed, 26 Apr 2023 17:29:51 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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":"Nick Hollinghurst via libcamera-devel\n\t","Reply-To":"Nick Hollinghurst ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27112,"web_url":"https://patchwork.libcamera.org/comment/27112/","msgid":"","date":"2023-05-18T13:14:04","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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":"Hello Laurent, Hello Nick,\n\nOn Wed, Apr 26, 2023 at 6:30 PM Nick Hollinghurst\n wrote:\n>\n> Hi Laurent, Daniel,\n>\n> On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart\n> wrote:\n> >\n> > Hi Daniel,\n> >\n> > (Nick, there's a question for you below)\n> >\n> > On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote:\n> > > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote:\n> > > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Hi Daniel\n> > > > > > cc RPi folks which originally introduced the AF controls and which\n> > > > > > have recently been working a new implementation\n> > > > > >\n> > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote:\n> > > > > > > >\n> > > > > > > > Hi Daniel\n> > > > > > > > sorry I have neglected the Windows related patches in previous\n> > > > > > > > reviews as I hoped someone would have shared an opinion on the\n> > > > > > > > Signal-based mechanism.\n> > > > > > > >\n> > > > > > > > I don't have better ideas to propose, so let's go with it\n> >\n> > I don't like the signal much either. We could simply implement a\n> > function to retrieve the window.\n\nSignal has the advantage that it will be invoked only when the window\nhas changed. With regular function, the function will need to be called\non each frame to check if there is a new window.\nWhat are the disadvantages of using the signal?\n\n> >\n> > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > > > Add support for setting user defined auto focus window in the\n> > > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > > > > > > > Each time, there is a need for changing the window configuration in the\n> >\n> > s/Each time,/Each time/\n> >\n> > > > > > > > > ISP, the signal is emitted. Platform layer that wants to use\n> >\n> > s/layer that wants/Layers that want/\n> >\n> > > > > > > > > the \"Windows\" metering mode, needs to connect to this signal\n> >\n> > s/mode, needs/mode need/\n> >\n> > > > > > > > > and configure the ISP on each emission.\n> > > > > > > > >\n> > > > > > > > > Currently only one window is supported.\n> > > > > > > > >\n> > > > > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > > > > ---\n> > > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > > > > > > > >\n> > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > index 244b8803..0fb17df3 100644\n> > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > > > > > > > * movement range rather than coarse search result.\n> > > > > > > > > * \\todo Implement setRange.\n> > > > > > > > > * \\todo Implement setSpeed.\n> > > > > > > > > - * \\todo Implement setMeteringMode.\n> > > > > > > > > - * \\todo Implement setWindows.\n> > > > > > > > > * \\todo Implement the AfPauseDeferred mode.\n> > > > > > > > > + * \\todo Implement support for multiple AF windows.\n> > > > > > > > > */\n> > > > > > > > >\n> > > > > > > > > /**\n> > > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > > return 0;\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > +/**\n> > > > > > > > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > > > > > > > + * \\param[in] outputSize Camera sensor output size\n> > > > > > > > > + *\n> > > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> >\n> > s/method/function/\n> >\n> > > > > > > > > + * method of the platform layer.\n> > > > > > > > > + */\n> > > > > > > > > +void AfHillClimbing::configure(const Size &outputSize)\n> > > > > > > >\n> > > > > > > > According to the AfWindows control definition\n> > > > > > > >\n> > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle\n> > > > > > > > returned by the ScalerCropMaximum property.\n> > > > > > > >\n> > > > > > > > The final sensor's output size can be obtained by downscaling/cropping\n> > > > > > > > a larger portion of the pixel array. The portion of the pixel array\n> > > > > > > > processed to obtain the final image is named AnalogueCropRectangle and\n> > > > > > > > all valid crop rectangles lie inside it.\n> > > > > > > >\n> > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the\n> > > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the\n> > > > > > > > current output size. It's up to the pipeline handler to re-scale any\n> > > > > > > > valid control in the current configured output.\n> > > > > > >\n> > > > > > > For the 1024x768 stream, I have the following parameters:\n> > > > > > > - activeAreaSize: 2592x1944\n> > > > > > > - analogCrop: (0, 0)/2592x1944\n> > > > > > > - outputSize: 1296x972\n> > > > > > >\n> > > > > > > When using analogCrop, I will also need to scale the AF window\n> > > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> > > > > > > values in reference to the ISP input size\n> > > > > > > (IPACameraSensorInfo::outputSize).\n> > > > > > > Based on what you wrote earlier, I understand that it is supposed to be\n> > > > > > > like that?\n> > > > > >\n> > > > > > I presume so.\n> > > > > >\n> > > > > > I should check how RPi handles windowing\n> >\n> > Nick, that's the first question for you.\n>\n> Our implementation is based on what we understand to be the current\n> libcamera specification: That AF windows are in the same coordinate\n> system as ScalerCropMaximum, i.e. raw sensor pixels.\n>\n> I found the documentation phrase \"The units used are pixels within the\n> rectangle...\" ambiguous -- does it suggest that the origin of the\n> AfWIndows coordinate system is the top-left corner of\n> ScalerCropMaximum? I took the answer to be \"no\", that those rectangles\n> were in a common coordinate system, without offsetting. This in turn\n> means that Af Windows will stick to the same objects in the scene,\n> regardless of mode or digital zoom.\n\nI had the same problems with understanding this documentation part, so\nprobably it should be rephrased to clearly state what is the reference\ncoordination system for AfWindows.\n\n>\n>\n> > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > > > > > > > probably to allow pipeline handler express any additional processing\n> > > > > > > > margins required by any cropping/scaling that happens on the ISP.\n> > > > > > > >\n> > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin\n> > > > > > > > yet, I would simply use here the sensor's analogue crop, which is part\n> > > > > > > > of the IPACameraSensorInfo you already use in the platform layer.\n> > > > > > > >\n> > > > > > >\n> > > > > > > I have some concerns about the ScalerCropMaximum.\n> > > > > > > What if the ISP have different size margins for cropping and auto-focus\n> > > > > > > windows? I see there are margins defined for the AF window, but nothing\n> > > > > > > about the cropping margins in the RK3399 documentation. In this case,\n> > > > > > > it will not be a problem, but what if some ISP will have margins for\n> > > > > > > cropping, but no margins for AF window?\n> > > > > >\n> > > > > > Good point. Do we need an AfWindowMaximum ?\n> > > > >\n> > > > > To make sense, AfWindowMaximum would need to be expressed in reference\n> > > > > to how ISP handles that (sensor output size on rkisp) to allow precise\n> > > > > AF window configuration. AF window margins on rkisp are small: 2px and\n> > > > > 5px. I would like to avoid scaling, so small values compared to\n> > > > > the output size.\n> > > >\n> > > > For ScalerCrop we decided to have a ScalerCropMaximum property which\n> > > > basically reports the AnalogueCrop rectangle, as this is the maximum\n> > > > rectangle an application can set as a cropping area. The reference is\n> > > > the PixelArrayActiveAreas size.\n> > > >\n> > > > For AfWindows we don't care about AnalogCrop but only about the\n> > > > sensor's output as, if my understanding is correct, this is the\n> > > > rectangle on which the ISP performs windowing on.\n> > > >\n> > > > As we don't expose the sensor's output size to applications, the only\n> > > > reference we can use is full PixelArrayActiveAreas and re-scale before\n> > > > setting the windows.\n> > > >\n> > > > > I am not sure if AfWindowMaximum as additional property is actually\n> > > > > needed. Maximum value for AfWindows would not be enough?\n>\n> Encoding the largest useful window as the control maximum value (where\n> this means minimum X, Y and maximum width,height?) sounds useful. BTW\n> is there any way to encode the maximum number of windows?\n\nThat is a good point. If we specify the maximum windows size, we should\nalso specify the maximum number of windows.\nThe only solution I can think of is something like this:\n\n Rectangle minWindow(0, 0, 0, 0);\n ControlValue min(Span({minWindow}));\n\n Rectangle maxWindow(0, 0, 640, 480);\n ControlValue max(Span({maxWindow, maxWindow, maxWindow}));\n\n Rectangle defWindow(100, 100, 200, 200);\n ControlValue def(Span({defWindow}));\n\n ctrls[&controls::AfWindows] = ControlInfo(min, max,def);\n\ncam lists such control as:\n\n Control: AfWindows: [[ (0, 0)/0x0 ]..[ (0, 0)/640x480, (0,\n0)/640x480, (0, 0)/640x480 ]]\n\nI am not convinced this is a readable solution for the user...\nEspecially, if there will be bigger number of windows supported.\nI expect that minimum and maximum size of will be the same for all\nwindows, regardless of the number of windows.\nFrom this, it can be deduced that we could expose only the simple pair:\n{ window size, number of windows } for each min, max and def ControlInfo\nvalue.\n\n>\n> > > >\n> > > > As a comment on the ScalerCropMaximum property reports\n> > > >\n> > > > \\todo Turn this property into a \"maximum control value\" for the\n> > > > ScalerCrop control once \"dynamic\" controls have been implemented.\n> > > >\n> > > > I presume this is very old, as right now (at least for RkISP1) I see\n> > > > the \"ControlInfoMap *ipaControls\" parameter passed in to configure() and\n> > > > being overwritten completely with new control limits.\n> > > >\n> > > > So my suggestion for a new property was probably not correct.\n> > > >\n> > > > > Is there a need to expose the window margins to the user at all?\n> > > >\n> > > > I think so, it would avoid setting controls which can't be applied as\n> > > > they are.\n> > > >\n> > > > I presume you can initialize the maximum value of AfWindows as\n> > > >\n> > > > { 0, 0, PixelArrayActiveAreas.width - 4,\n> > > > PixelArrayActiveAreas.height - 10 }\n> > > >\n> > > > When re-scaling, you first need to translate it to take into\n> > > > consideration the processing margins then re-scale it using the\n> > > > activeArea-to-outputSize ratio.\n> > > >\n> > > > (only compiled, not run-time tested)\n> > > >\n> > > > Span userWindows =\n> > > > *request->controls().get>(controls::AfWindows);\n> > > > std::vector afWindows;\n> > > >\n> > > > for (auto userWindow : userWindows) {\n> > > > Rectangle window = userWindow.translatedBy({2, 5});\n> > > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize);\n> > > > afWindows.push_back(window);\n> > > > }\n> > > >\n> > > > (as you only support one window, this could be even\n> > > > simplified)\n> > > >\n> > > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm\n> > > > implementation, and you would need to pass to the algorithm the active\n> > > > area size. One way to do so, as algorithms have access to the\n> > > > context_, is to add the active area size to\n> > > > IPASessionConfiguration::sensor which already contains the sensor's\n> > > > output size.\n> > > >\n> > > > What do you think ?\n> > >\n> > > I am not fully happy with this approach as this requires scaling\n> > > the values two times, but as you said, expressing it directly\n> > > in the output size would require exposing additional controls.\n> > >\n> > > The other way, I was thinking of, is to express the AF window\n> > > in the final output image size (1024x768 in my already mentioned\n> > > example). This way, it is easier to think about from the user\n> > > perspective. However, this is an opposite approach than the already\n> > > existing one for the ScalerCrop.\n> > >\n> > > Maybe I am not 100% happy with this, but it looks that expressing\n> > > the AF window as you show above (in reference to the current\n> > > PixelArrayActiveAreas) makes the most sense for the existing code,\n> > > because it will be the common approach with the ScalerCrop.\n> > > I can change my implementation to this approach.\n> > >\n> > > Do we need to change also the documentation related to the AF Windows?\n> > >\n> > > > > We could allow the window size of the whole ISP input size to be set\n> > > > > by the user and clamp the size in the IPA before configuring the ISP?\n> > > > >\n> > > > > In summary, I see two options:\n> > > > > 1. Maximum AF window expressed in units that are directly set to ISP\n> > > > > (output size for rkisp). Maximum AF window will include margins.\n> > > > > 2. Keep the current approach and express the AF window in reference to\n> > > > > Analogue crop size. Just allow the full size instead of the\n> > > > > ScalerCropMaximum. Clamp the margins inside the IPA if user\n> > > > > requested larger window.\n> > > > >\n> > > > > > RPi any opinions here ?\n> >\n> > And this is the second question for Nick.\n>\n> I don't have very strong opinions. Option 2 seems the most general.\n> (Again, I've been assuming there's no implicit offsetting for the\n> \"reference\" rectangle -- so the only difference between those\n> definitions is a question of cropping?)\n>\n> I suspect there is not a one-size-fits-all approach. Some sensors may\n> be able to generate focus information outside the cropped region;\n> others may not. Some platforms may only be able to measure focus\n> within the final image. Cropping \"as required\" sounds like a\n> reasonable way to resolve that -- but we ought to give the user some\n> way to predict when a window might end up being cropped to nothing\n> (and thus ignored)!\n\nOk, so my idea with cropping the margins without the user knowledge\nis not a good solution... Then, probably We end up with something\nsimilar to the ScalerCrop that is already implemented in the Raspberry\nimplementation, if I understand it correctly:\n\nThe (x,y) location of the AfWindow is relative to the\nPixelArrayActiveAreas that is being used. The units remain native sensor\npixels. Maximum value of the AfWindows ControlInfo (position + size),\nalready includes all the margins that need to be considered\n(e.g. required by the ISP).\n\nWould you agree with the above definition?\n\n>\n> > > > > > > > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > > > > > > > something similar.\n> > > > > > > >\n> > > > > > > > I would also initialize userWindow_ to the same value, so that if an\n> > > > > > > > application switches to AfMeteringWindows mode the rectangle is at\n> > > > > > > > least initialized to something.\n> > > > > > >\n> > > > > > > Good point!\n> > > > > > >\n> > > > > > > > > +{\n> > > > > > > > > + /*\n> > > > > > > > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > > > > > > > + * placed at the center\n> > > > > > > > > + */\n> > > > > > > > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > > > > > > > + static_cast(outputSize.height / 8),\n> > > > > > > > > + 3 * outputSize.width / 4,\n> > > > > > > > > + 3 * outputSize.height / 4);\n> > > > > > > > > +\n> > > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > /**\n> > > > > > > > > * \\brief Run the auto focus algorithm loop\n> > > > > > > > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > > > > > > > framesToSkip_ = n;\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > +/**\n> > > > > > > > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > > > > > > > + * \\brief Signal emitted when change in AF window was requested\n> > > > > > > > > + *\n> > > > > > > > > + * Platform layer supporting AF windows should connect to this signal\n> >\n> > s/layer/layers/\n> > s/connect to/connect/\n> >\n> > > > > > > > > + * and configure the ISP with new window on each emition.\n> >\n> > s/new window/the new window/\n> > s/emition/emission/\n> >\n> > The new window will likely need a few frames to become effective. I'm\n> > worried that the AF algorithm doesn't take that into account at all (it\n> > also doesn't take the lens movement delays into account, which is\n> > another of my worries). This can affect the performance and stability of\n> > the auto-focus algorithm.\n\nThe delay when configuring new window is set in the platform layer in\nthe patch 07/10. The delay can be different for different ISPs, so I\ndecided it will be better to leave the delay control to the platform\nlayer.\n\nLens delay is a more complex problem and there were discussions\npreviously about it. We need a proper solution for that.\nAs a workaround, I introduced the \"wait-frames-lens\" tuning file\nparameter in 06/10.\n\n> >\n> > How have you tested this ?\n\nYes, and most of the delays configured using AfHillClimbing::skipFrames()\nbase on my experiments on RK3399 and camera lens available on my device.\nUnfortunately, the documentation I have access to, says very little\nabout the delays in ISP.\n\n> >\n> > > > > > > > > + */\n> > > > > > > > > +\n> > > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > > > > > > {\n> > > > > > > > > if (mode == mode_)\n> > > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > > > > > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > > > > > > > {\n> > > > > > > > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > > > > > > > + if (metering == meteringMode_)\n> > > > > > > > > + return;\n> > > > > > > > > +\n> > > > > > > > > + meteringMode_ = metering;\n> > > > > > > > > +\n> > > > > > > > > + switch (metering) {\n> > > > > > > > > + case controls::AfMeteringAuto:\n> > > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > > + break;\n> > > > > > > > > + case controls::AfMeteringWindows:\n> > > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > > + break;\n> >\n> > This needs a default case to avoid modifying meteringMode_. We have two\n> > modes currently defined, if we add a third one, the implementation will\n> > break.\n\nWell, in v4 there was a suggestion to remove the default case from\nswitch statements, so the compiler will complain early about the missing\nimplementation.\n\n> >\n> > When the window is changed the contrast value may drastically change.\n> > The optimal focus point will likely change too. I think you need to\n> > reset the AF and possibly trigger a rescan for continuous AF mode.\n\nThis is true and sounds like a general problem. Do you think this\nbehaviour should be documented, so other platforms will also follow this\nrule?\n\n> >\n> > > > > > > > > + }\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > > > > > > +void AfHillClimbing::setWindows(Span windows)\n> > > > > > > > > {\n> > > > > > > > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > > > > > > > + if (windows.size() != 1) {\n> > > > > > > > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > > > > > > > + return;\n> > > > > > > > > + }\n> > > > > > > >\n> > > > > > > > Should this be an hard error or should we just want and continue by\n> > > > > > > > only considering the first available rectangle ?\n> > > > > > >\n> > > > > > > I will change it to a warning that only the first window was used.\n> > > > > > >\n> > > > > > > > > +\n> > > > > > > > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > > > > > > > +\n> > > > > > > > > + userWindow_ = windows[0];\n> > > > > > > > > +\n> > > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > index 2147939b..0f7c65db 100644\n> > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > @@ -10,6 +10,7 @@\n> > > > > > > > > #pragma once\n> > > > > > > > >\n> > > > > > > > > #include \n> > > > > > > > > +#include \n> > > > > > > > >\n> > > > > > > > > #include \"af.h\"\n> > > > > > > > >\n> > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > > > > > > > public:\n> > > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > > const YamlObject &tuningData);\n> > > > > > > > > + void configure(const Size &outputSize);\n> > > > > > > > > int32_t process(double currentContrast);\n> > > > > > > > > void skipFrames(uint32_t n);\n> > > > > > > > >\n> > > > > > > > > controls::AfStateEnum state() override { return state_; }\n> > > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > > > > > > > >\n> > > > > > > > > + Signal windowUpdateRequested;\n> > > > > > > > > +\n> > > > > > > > > private:\n> > > > > > > > > void setMode(controls::AfModeEnum mode) override;\n> > > > > > > > > void setRange(controls::AfRangeEnum range) override;\n> > > > > > > > > @@ -54,6 +58,7 @@ private:\n> > > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > > > > > > > >\n> > > > > > > > > /* Current focus lens position. */\n> > > > > > > > > int32_t lensPosition_ = 0;\n> > > > > > > > > @@ -84,6 +89,11 @@ private:\n> > > > > > > > >\n> > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > > > > > > double maxChange_;\n> > > > > > > > > +\n> > > > > > > > > + /* Default AF window. */\n> > > > > > > > > + Rectangle defaultWindow_;\n> > > > > > > > > + /* AF window set by user using AF_WINDOWS control. */\n> > > > > > > > > + Rectangle userWindow_;\n> > > > > > > > > };\n> > > > > > > > >\n> > > > > > > > > } /* namespace ipa::algorithms */\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>\n> Regards,\n>\n> Nick\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 7CBAEBDE6B\n\tfor ;\n\tThu, 18 May 2023 13:14:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5C996285F;\n\tThu, 18 May 2023 15:14:17 +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 1C4FC627D4\n\tfor ;\n\tThu, 18 May 2023 15:14:16 +0200 (CEST)","by mail-ed1-x52c.google.com with SMTP id\n\t4fb4d7f45d1cf-510e419d701so1548428a12.1\n\tfor ;\n\tThu, 18 May 2023 06:14:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684415657;\n\tbh=R/VpaJ75gl4qxbGf5vfoqBBLa0n1xADgNlcKDFyouLA=;\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=UblXZRNz4Q3jjGONNC6CsamSONEznbxgimiJvJOBa6vNEzSF7GDO8RrwCwWKcil3N\n\tdLNshZV5zz8TF+gsTz5djLwa/F5cWCFzss+9wZe4Wdx3eZ5Vn7/YwR/lWyBkRyScJV\n\t4JZDou17mIvC/5SZfNRx4s8pdrPdOM3bwAP9m1lvivVoYYNSlY1JnP7XvvghoDWNLL\n\tEvgjwCdxzr43t0EbKUIrx5aU1g+Zh6OuIe8qj2bC8Z/aVBx8rtOHGQl+2WmKm1tdzu\n\tfnqKDJusF62rEl1Lk/JuGCEHp4IWatqsAPY4yu4+wOe+1AdrAxuDLCueSNuNd4/Dw+\n\tJInQqs9Wd0hOg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1684415655;\n\tx=1687007655; \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=zajEcH5kh2uV/OfOztJFP2UbNVulmVmQixVPQ1uwY2g=;\n\tb=yE2NfdD9xGEMB0sKW/cOUor3ra2zcsUXcCX3SxF8D1la29n6kzFsoq4e/GKetKpeWU\n\tRQDfP9tU8lU6F+vRT2fEXk+aR8JeF2IloClIqv8kLGdynn83AKnHCB6pqQDKrEVat90i\n\tCUItHrlemdzK0rP9vqZ0+ZN3YZpFdXvKCZtPRmhWwpO9jSUTIKck6oy3tsX4CM3M8oGh\n\turgyOMMVbCAwXfAY7sr0WBIKwnMp7RQwYgLCHqSjHz8rEPdFdgZMTJc+6xMm6BNT1rVD\n\tBBWj5bjYkzivqUKLoOSKqWPWtIb3b8fn4KWOzmyFFTXmQg2OtJE7dS3pA/uIItvlTbuN\n\tWtAw=="],"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=\"yE2NfdD9\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1684415655; x=1687007655;\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=zajEcH5kh2uV/OfOztJFP2UbNVulmVmQixVPQ1uwY2g=;\n\tb=OmYH3zvKu3Nnz2cgy+U/OwQaGWYibQiiC/CW8w3EqvzSZwH5GMXuBQ7/mCzndG4gig\n\tKU0v/ZD6+QFSYZpZfqmTepb4KPgfMDvwCD48wgvicaIqoiy2+W/+Em7Aat7D48XPxt0o\n\tVXNQE7xJUEqeGf+dkJDMkdZk4STa0n2HFE4ypo30zdfQmeI45G7pYhegrS2NvF47qKH6\n\tuoYQ3E7ii5NTnEt1hgPvmrDR2AtYm2QW45lKVsrfr9EEBfHJCG5MQMldRLSocUFiC72U\n\t1F6dC4WhJeIALN/FZkx2Cz1myPicuJzFxihvnXWYoVDciYJ9jkCefKG0UEdvrh/jVTqL\n\tXbBw==","X-Gm-Message-State":"AC+VfDx0z4NNqszogSjCaP9mSbBSB7VrkGVvgRgMqvEhRUt0vGv8tMpe\n\tRtNVAX89cjrblZzwzVwqmsnkcLFyjLPton+fQQgvWA==","X-Google-Smtp-Source":"ACHHUZ7WQc0c5XEZZI0mVkNIv50vfKvHfZQs2dWL6zE9swjbHoGODivjh2koAUWAUmAgqe+GW//QBwBeQ6FiVBT6FUw=","X-Received":"by 2002:a05:6402:c5:b0:50e:412:5a50 with SMTP id\n\ti5-20020a05640200c500b0050e04125a50mr4923097edu.29.1684415655240;\n\tThu, 18 May 2023 06:14:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>\n\t\n\t<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>\n\t\n\t<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>\n\t\n\t<20230426031305.GF1667@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","Date":"Thu, 18 May 2023 15:14:04 +0200","Message-ID":"","To":"Nick Hollinghurst ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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":27199,"web_url":"https://patchwork.libcamera.org/comment/27199/","msgid":"<20230531173150.GG24749@pendragon.ideasonboard.com>","date":"2023-05-31T17:31:50","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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":"Hello,\n\n(CC'ing David)\n\nOn Thu, May 18, 2023 at 03:14:04PM +0200, Daniel Semkowicz wrote:\n> On Wed, Apr 26, 2023 at 6:30 PM Nick Hollinghurst wrote:\n> > On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart wrote:\n> > > On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote:\n> > > > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote:\n> > > > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote:\n> > > > > > >\n> > > > > > > Hi Daniel\n> > > > > > > cc RPi folks which originally introduced the AF controls and which\n> > > > > > > have recently been working a new implementation\n> > > > > > >\n> > > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote:\n> > > > > > > > >\n> > > > > > > > > Hi Daniel\n> > > > > > > > > sorry I have neglected the Windows related patches in previous\n> > > > > > > > > reviews as I hoped someone would have shared an opinion on the\n> > > > > > > > > Signal-based mechanism.\n> > > > > > > > >\n> > > > > > > > > I don't have better ideas to propose, so let's go with it\n> > >\n> > > I don't like the signal much either. We could simply implement a\n> > > function to retrieve the window.\n> \n> Signal has the advantage that it will be invoked only when the window\n> has changed. With regular function, the function will need to be called\n> on each frame to check if there is a new window.\n> What are the disadvantages of using the signal?\n\nI was initially thinking we could always set the AF parameters in the\nISP configuration buffer, but that would burn extra CPU cycles in the\nkernel in interrupt context (for rkisp1 at least) to reprogram\nregisters, which should be avoided.\n\nThe issue with a signal is that it creates a mid-layer, in the sense\nthat the AF implementation mandates a particular implementation of the\nplatform-specific code. You would need to clearly specify where the\nsignal could be emitted from, as the platform-specific code may not\noperate properly when getting the window changed signalled at random\ntimes.\n\nFor instance, you're currently emitting the signal from\n\n- AfHillClimbing::configure()\n- AfHillClimbing::setMeteringMode()\n- AfHillClimbing::setWindows()\n\nThe last two functions are called from algorithms::Af::queueRequest().\nThe signal is connect to the\nrkisp1::algorithms::Af::updateCurrentWindow() function, which just\nstores the rectangle in the updateWindow_ member variable, and that\nvariable is used in rkisp1::algorithms::Af::prepare() to set the ISP\nparameters buffer. This means that rkisp1::algorithms::Af::prepare()\noperates with the parameters set by the very latest request, which isn't\nright. It should operate with the parameters set for the request that\ncorresponds to the frame being prepared. Fixing that is possible, using\nthe frame context (we do so in the AWB algorithm for instance), but that\nwill require the signal handler to know exactly where the signal can be\nemitted from, and get the right context. The context isn't passed to the\nsignal handler (it doesn't get told for which request the window is\nset), so we'll have a problem. The rkisp1::algorithms::Af class could\nstill implement this correctly by knowing the signal is emitted from the\nalgorithms::Af::queueRequest() function, and using updateWindow_ in\nrkisp1::algorithms::Af::queueRequest() after calling\nalgorithms::Af::queueRequest(), but that's implicit knowledge of the\ninternal algorithms::Af helper implementation, which could change in the\nfuture.\n\nI think it will be simpler if the platform queries the AF window\nmanually, as it will then be in full control of when it retrieves the\nvalue and how it uses it. This means the new value will need to be\ncompared with a cached value to decide whether or not to configure the\nISP, but that's simpler than going through a signal in the end.\n\n> > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > > > > Add support for setting user defined auto focus window in the\n> > > > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > > > > > > > > Each time, there is a need for changing the window configuration in the\n> > >\n> > > s/Each time,/Each time/\n> > >\n> > > > > > > > > > ISP, the signal is emitted. Platform layer that wants to use\n> > >\n> > > s/layer that wants/Layers that want/\n> > >\n> > > > > > > > > > the \"Windows\" metering mode, needs to connect to this signal\n> > >\n> > > s/mode, needs/mode need/\n> > >\n> > > > > > > > > > and configure the ISP on each emission.\n> > > > > > > > > >\n> > > > > > > > > > Currently only one window is supported.\n> > > > > > > > > >\n> > > > > > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > > > > > ---\n> > > > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > > > > > > > > >\n> > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > > index 244b8803..0fb17df3 100644\n> > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > > > > > > > > * movement range rather than coarse search result.\n> > > > > > > > > > * \\todo Implement setRange.\n> > > > > > > > > > * \\todo Implement setSpeed.\n> > > > > > > > > > - * \\todo Implement setMeteringMode.\n> > > > > > > > > > - * \\todo Implement setWindows.\n> > > > > > > > > > * \\todo Implement the AfPauseDeferred mode.\n> > > > > > > > > > + * \\todo Implement support for multiple AF windows.\n> > > > > > > > > > */\n> > > > > > > > > >\n> > > > > > > > > > /**\n> > > > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > > > return 0;\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > +/**\n> > > > > > > > > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > > > > > > > > + * \\param[in] outputSize Camera sensor output size\n> > > > > > > > > > + *\n> > > > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > >\n> > > s/method/function/\n> > >\n> > > > > > > > > > + * method of the platform layer.\n> > > > > > > > > > + */\n> > > > > > > > > > +void AfHillClimbing::configure(const Size &outputSize)\n> > > > > > > > >\n> > > > > > > > > According to the AfWindows control definition\n> > > > > > > > >\n> > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > > > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle\n> > > > > > > > > returned by the ScalerCropMaximum property.\n> > > > > > > > >\n> > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping\n> > > > > > > > > a larger portion of the pixel array. The portion of the pixel array\n> > > > > > > > > processed to obtain the final image is named AnalogueCropRectangle and\n> > > > > > > > > all valid crop rectangles lie inside it.\n> > > > > > > > >\n> > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the\n> > > > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the\n> > > > > > > > > current output size. It's up to the pipeline handler to re-scale any\n> > > > > > > > > valid control in the current configured output.\n> > > > > > > >\n> > > > > > > > For the 1024x768 stream, I have the following parameters:\n> > > > > > > > - activeAreaSize: 2592x1944\n> > > > > > > > - analogCrop: (0, 0)/2592x1944\n> > > > > > > > - outputSize: 1296x972\n> > > > > > > >\n> > > > > > > > When using analogCrop, I will also need to scale the AF window\n> > > > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> > > > > > > > values in reference to the ISP input size\n> > > > > > > > (IPACameraSensorInfo::outputSize).\n> > > > > > > > Based on what you wrote earlier, I understand that it is supposed to be\n> > > > > > > > like that?\n> > > > > > >\n> > > > > > > I presume so.\n> > > > > > >\n> > > > > > > I should check how RPi handles windowing\n> > >\n> > > Nick, that's the first question for you.\n> >\n> > Our implementation is based on what we understand to be the current\n> > libcamera specification: That AF windows are in the same coordinate\n> > system as ScalerCropMaximum, i.e. raw sensor pixels.\n> >\n> > I found the documentation phrase \"The units used are pixels within the\n> > rectangle...\" ambiguous -- does it suggest that the origin of the\n> > AfWIndows coordinate system is the top-left corner of\n> > ScalerCropMaximum? I took the answer to be \"no\", that those rectangles\n> > were in a common coordinate system, without offsetting. This in turn\n> > means that Af Windows will stick to the same objects in the scene,\n> > regardless of mode or digital zoom.\n> \n> I had the same problems with understanding this documentation part, so\n> probably it should be rephrased to clearly state what is the reference\n> coordination system for AfWindows.\n\nDavid, git pointed to you as the author of the text :-) I share Nick's\nunderstanding here, what is yours ?\n\nThe documentation should be clarified, and I would like to propose\nchanging the wording to make AfWindows relative to the\nPixelArrayActiveAreas property, not the ScalerCropMaximum. The\ncoordinate system for ScalerCropMaximum isn't defined, but the\ncoordinate system for ScalerCrop is documented as relative to\nPixelArrayActiveAreas. Having both ScalerCrop and AfWindows expressed\nin the same coordinate system would make sense to me.\n\nThere's still a question of what to do with windows outside (or\npartially outside) of the ScalerCrop. Does it make sense to allow the\nuser to focus on something that isn't visible in the output image ? Use\ncases seem dubious to me, and well-written applications will likely\nensure that the AF windows full within the visible image, but should we\nenforce this in libcamera ?\n\n> > > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > > > > > > > > probably to allow pipeline handler express any additional processing\n> > > > > > > > > margins required by any cropping/scaling that happens on the ISP.\n> > > > > > > > >\n> > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin\n> > > > > > > > > yet, I would simply use here the sensor's analogue crop, which is part\n> > > > > > > > > of the IPACameraSensorInfo you already use in the platform layer.\n> > > > > > > > >\n> > > > > > > >\n> > > > > > > > I have some concerns about the ScalerCropMaximum.\n> > > > > > > > What if the ISP have different size margins for cropping and auto-focus\n> > > > > > > > windows? I see there are margins defined for the AF window, but nothing\n> > > > > > > > about the cropping margins in the RK3399 documentation. In this case,\n> > > > > > > > it will not be a problem, but what if some ISP will have margins for\n> > > > > > > > cropping, but no margins for AF window?\n> > > > > > >\n> > > > > > > Good point. Do we need an AfWindowMaximum ?\n> > > > > >\n> > > > > > To make sense, AfWindowMaximum would need to be expressed in reference\n> > > > > > to how ISP handles that (sensor output size on rkisp) to allow precise\n> > > > > > AF window configuration. AF window margins on rkisp are small: 2px and\n> > > > > > 5px. I would like to avoid scaling, so small values compared to\n> > > > > > the output size.\n> > > > >\n> > > > > For ScalerCrop we decided to have a ScalerCropMaximum property which\n> > > > > basically reports the AnalogueCrop rectangle, as this is the maximum\n> > > > > rectangle an application can set as a cropping area. The reference is\n> > > > > the PixelArrayActiveAreas size.\n> > > > >\n> > > > > For AfWindows we don't care about AnalogCrop but only about the\n> > > > > sensor's output as, if my understanding is correct, this is the\n> > > > > rectangle on which the ISP performs windowing on.\n> > > > >\n> > > > > As we don't expose the sensor's output size to applications, the only\n> > > > > reference we can use is full PixelArrayActiveAreas and re-scale before\n> > > > > setting the windows.\n> > > > >\n> > > > > > I am not sure if AfWindowMaximum as additional property is actually\n> > > > > > needed. Maximum value for AfWindows would not be enough?\n> >\n> > Encoding the largest useful window as the control maximum value (where\n> > this means minimum X, Y and maximum width,height?) sounds useful. BTW\n> > is there any way to encode the maximum number of windows?\n> \n> That is a good point. If we specify the maximum windows size, we should\n> also specify the maximum number of windows.\n> The only solution I can think of is something like this:\n> \n> Rectangle minWindow(0, 0, 0, 0);\n> ControlValue min(Span({minWindow}));\n> \n> Rectangle maxWindow(0, 0, 640, 480);\n> ControlValue max(Span({maxWindow, maxWindow, maxWindow}));\n> \n> Rectangle defWindow(100, 100, 200, 200);\n> ControlValue def(Span({defWindow}));\n> \n> ctrls[&controls::AfWindows] = ControlInfo(min, max,def);\n> \n> cam lists such control as:\n> \n> Control: AfWindows: [[ (0, 0)/0x0 ]..[ (0, 0)/640x480, (0,\n> 0)/640x480, (0, 0)/640x480 ]]\n> \n> I am not convinced this is a readable solution for the user...\n> Especially, if there will be bigger number of windows supported.\n> I expect that minimum and maximum size of will be the same for all\n> windows, regardless of the number of windows.\n> From this, it can be deduced that we could expose only the simple pair:\n> { window size, number of windows } for each min, max and def ControlInfo\n> value.\n\nHow about simply adding a AfMaxWindows property ?\n\n> > > > >\n> > > > > As a comment on the ScalerCropMaximum property reports\n> > > > >\n> > > > > \\todo Turn this property into a \"maximum control value\" for the\n> > > > > ScalerCrop control once \"dynamic\" controls have been implemented.\n> > > > >\n> > > > > I presume this is very old, as right now (at least for RkISP1) I see\n> > > > > the \"ControlInfoMap *ipaControls\" parameter passed in to configure() and\n> > > > > being overwritten completely with new control limits.\n> > > > >\n> > > > > So my suggestion for a new property was probably not correct.\n> > > > >\n> > > > > > Is there a need to expose the window margins to the user at all?\n> > > > >\n> > > > > I think so, it would avoid setting controls which can't be applied as\n> > > > > they are.\n> > > > >\n> > > > > I presume you can initialize the maximum value of AfWindows as\n> > > > >\n> > > > > { 0, 0, PixelArrayActiveAreas.width - 4,\n> > > > > PixelArrayActiveAreas.height - 10 }\n> > > > >\n> > > > > When re-scaling, you first need to translate it to take into\n> > > > > consideration the processing margins then re-scale it using the\n> > > > > activeArea-to-outputSize ratio.\n> > > > >\n> > > > > (only compiled, not run-time tested)\n> > > > >\n> > > > > Span userWindows =\n> > > > > *request->controls().get>(controls::AfWindows);\n> > > > > std::vector afWindows;\n> > > > >\n> > > > > for (auto userWindow : userWindows) {\n> > > > > Rectangle window = userWindow.translatedBy({2, 5});\n> > > > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize);\n> > > > > afWindows.push_back(window);\n> > > > > }\n> > > > >\n> > > > > (as you only support one window, this could be even\n> > > > > simplified)\n> > > > >\n> > > > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm\n> > > > > implementation, and you would need to pass to the algorithm the active\n> > > > > area size. One way to do so, as algorithms have access to the\n> > > > > context_, is to add the active area size to\n> > > > > IPASessionConfiguration::sensor which already contains the sensor's\n> > > > > output size.\n> > > > >\n> > > > > What do you think ?\n> > > >\n> > > > I am not fully happy with this approach as this requires scaling\n> > > > the values two times, but as you said, expressing it directly\n> > > > in the output size would require exposing additional controls.\n> > > >\n> > > > The other way, I was thinking of, is to express the AF window\n> > > > in the final output image size (1024x768 in my already mentioned\n> > > > example). This way, it is easier to think about from the user\n> > > > perspective. However, this is an opposite approach than the already\n> > > > existing one for the ScalerCrop.\n> > > >\n> > > > Maybe I am not 100% happy with this, but it looks that expressing\n> > > > the AF window as you show above (in reference to the current\n> > > > PixelArrayActiveAreas) makes the most sense for the existing code,\n> > > > because it will be the common approach with the ScalerCrop.\n> > > > I can change my implementation to this approach.\n> > > >\n> > > > Do we need to change also the documentation related to the AF Windows?\n> > > >\n> > > > > > We could allow the window size of the whole ISP input size to be set\n> > > > > > by the user and clamp the size in the IPA before configuring the ISP?\n> > > > > >\n> > > > > > In summary, I see two options:\n> > > > > > 1. Maximum AF window expressed in units that are directly set to ISP\n> > > > > > (output size for rkisp). Maximum AF window will include margins.\n> > > > > > 2. Keep the current approach and express the AF window in reference to\n> > > > > > Analogue crop size. Just allow the full size instead of the\n> > > > > > ScalerCropMaximum. Clamp the margins inside the IPA if user\n> > > > > > requested larger window.\n> > > > > >\n> > > > > > > RPi any opinions here ?\n> > >\n> > > And this is the second question for Nick.\n> >\n> > I don't have very strong opinions. Option 2 seems the most general.\n> > (Again, I've been assuming there's no implicit offsetting for the\n> > \"reference\" rectangle -- so the only difference between those\n> > definitions is a question of cropping?)\n> >\n> > I suspect there is not a one-size-fits-all approach. Some sensors may\n> > be able to generate focus information outside the cropped region;\n> > others may not. Some platforms may only be able to measure focus\n> > within the final image. Cropping \"as required\" sounds like a\n> > reasonable way to resolve that -- but we ought to give the user some\n> > way to predict when a window might end up being cropped to nothing\n> > (and thus ignored)!\n> \n> Ok, so my idea with cropping the margins without the user knowledge\n> is not a good solution... Then, probably We end up with something\n> similar to the ScalerCrop that is already implemented in the Raspberry\n> implementation, if I understand it correctly:\n> \n> The (x,y) location of the AfWindow is relative to the\n> PixelArrayActiveAreas that is being used. The units remain native sensor\n> pixels. Maximum value of the AfWindows ControlInfo (position + size),\n> already includes all the margins that need to be considered\n> (e.g. required by the ISP).\n> \n> Would you agree with the above definition?\n\nI think so. Depending on the platform, the AF statistics can be computed\nright away on the image received from the sensor, or after some ISP\nprocessing steps that will crop a few lines and columns on the edges. I\nwould like to avoid a need for applications to be aware of that\ncropping, as it should be small. If the window(s) need to be expressed,\nat the hardware level, in a cropped coordinate system, the IPA module\nshould perform the translation between PixelArrayActiveAreas and the\nhardware coordinates.\n\nIf analog crop and/or digital crop gets applied in the sensor, there may\nbe significant cropping, and the application may need to be aware of\nthis. I think you can ignore this for now, we'll need to revisit the\nprocessing pipeline model exposed to applications at some point.\n\nFor platforms that calculate the AF statistics directly on the sensor\n(or more precisely, where the sensor generates AF data, consumed by the\nalgorithms directly or after processing, as in the PDAF case), the\nwindows could even exceed the sensor digital crop (I doubt they could\nexceed the analog crop, as pixels outside the analog crop rectangle are\nnot read out by definition). We will have to take that into account too\nI suppose, but I think you can ignore it for now as you're not dealing\nwith sensor-side AF data.\n\n> > > > > > > > > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > > > > > > > > something similar.\n> > > > > > > > >\n> > > > > > > > > I would also initialize userWindow_ to the same value, so that if an\n> > > > > > > > > application switches to AfMeteringWindows mode the rectangle is at\n> > > > > > > > > least initialized to something.\n> > > > > > > >\n> > > > > > > > Good point!\n> > > > > > > >\n> > > > > > > > > > +{\n> > > > > > > > > > + /*\n> > > > > > > > > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > > > > > > > > + * placed at the center\n> > > > > > > > > > + */\n> > > > > > > > > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > > > > > > > > + static_cast(outputSize.height / 8),\n> > > > > > > > > > + 3 * outputSize.width / 4,\n> > > > > > > > > > + 3 * outputSize.height / 4);\n> > > > > > > > > > +\n> > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > > > > > > > /**\n> > > > > > > > > > * \\brief Run the auto focus algorithm loop\n> > > > > > > > > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > > > > > > > > framesToSkip_ = n;\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > +/**\n> > > > > > > > > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > > > > > > > > + * \\brief Signal emitted when change in AF window was requested\n> > > > > > > > > > + *\n> > > > > > > > > > + * Platform layer supporting AF windows should connect to this signal\n> > >\n> > > s/layer/layers/\n> > > s/connect to/connect/\n> > >\n> > > > > > > > > > + * and configure the ISP with new window on each emition.\n> > >\n> > > s/new window/the new window/\n> > > s/emition/emission/\n> > >\n> > > The new window will likely need a few frames to become effective. I'm\n> > > worried that the AF algorithm doesn't take that into account at all (it\n> > > also doesn't take the lens movement delays into account, which is\n> > > another of my worries). This can affect the performance and stability of\n> > > the auto-focus algorithm.\n> \n> The delay when configuring new window is set in the platform layer in\n> the patch 07/10. The delay can be different for different ISPs, so I\n> decided it will be better to leave the delay control to the platform\n> layer.\n\nThe ISP doesn't add a delay as such. What happens is that a set of ISP\nparameters buffers are queued to the ISP, and they are applied one by\none to consecutive frames. There's no internal ISP delay, the delay\ncomes from the fact that we queue parameters buffers, so the parameters\nin the last queued buffer will only be applied after all other buffers\nget processed. This should be handled by programming the AF window in an\nISP parameters buffer not right away when it gets queued in a request,\nbut at the right time so that it will be applied to the correct frame.\n\n> Lens delay is a more complex problem and there were discussions\n> previously about it. We need a proper solution for that.\n> As a workaround, I introduced the \"wait-frames-lens\" tuning file\n> parameter in 06/10.\n\nThe physical properties of the lens movement is indeed a separate and\nmore complex question, which I'm fine handling separately (and later).\n\n> > > How have you tested this ?\n> \n> Yes, and most of the delays configured using AfHillClimbing::skipFrames()\n> base on my experiments on RK3399 and camera lens available on my device.\n> Unfortunately, the documentation I have access to, says very little\n> about the delays in ISP.\n\nGood news, there's no delay in the ISP itself :-)\n\n> > > > > > > > > > + */\n> > > > > > > > > > +\n> > > > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > > > > > > > {\n> > > > > > > > > > if (mode == mode_)\n> > > > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > > > > > > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > > > > > > > > {\n> > > > > > > > > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > > > > > > > > + if (metering == meteringMode_)\n> > > > > > > > > > + return;\n> > > > > > > > > > +\n> > > > > > > > > > + meteringMode_ = metering;\n> > > > > > > > > > +\n> > > > > > > > > > + switch (metering) {\n> > > > > > > > > > + case controls::AfMeteringAuto:\n> > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > > > + break;\n> > > > > > > > > > + case controls::AfMeteringWindows:\n> > > > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > > > + break;\n> > >\n> > > This needs a default case to avoid modifying meteringMode_. We have two\n> > > modes currently defined, if we add a third one, the implementation will\n> > > break.\n> \n> Well, in v4 there was a suggestion to remove the default case from\n> switch statements, so the compiler will complain early about the missing\n> implementation.\n\nGood point, please ignore my comment :-)\n\n> > > When the window is changed the contrast value may drastically change.\n> > > The optimal focus point will likely change too. I think you need to\n> > > reset the AF and possibly trigger a rescan for continuous AF mode.\n> \n> This is true and sounds like a general problem. Do you think this\n> behaviour should be documented, so other platforms will also follow this\n> rule?\n\nThat's a good idea I think. I'm not sure where to best document it\nthough, the documentation in control_ids.yaml shouldn't assume a\nparticular type of AF implementation, and PDAF-based platforms don't\nneed a rescan as such as they don't really scan (well, they may as a\nfallback...). We could write the requirement in a generic way that\ndoesn't imply a particular AF method, or document it elsewhere. David,\nany opinion ?\n\n> > > > > > > > > > + }\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > > > > > > > +void AfHillClimbing::setWindows(Span windows)\n> > > > > > > > > > {\n> > > > > > > > > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > > > > > > > > + if (windows.size() != 1) {\n> > > > > > > > > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > > > > > > > > + return;\n> > > > > > > > > > + }\n> > > > > > > > >\n> > > > > > > > > Should this be an hard error or should we just want and continue by\n> > > > > > > > > only considering the first available rectangle ?\n> > > > > > > >\n> > > > > > > > I will change it to a warning that only the first window was used.\n> > > > > > > >\n> > > > > > > > > > +\n> > > > > > > > > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > > > > > > > > +\n> > > > > > > > > > + userWindow_ = windows[0];\n> > > > > > > > > > +\n> > > > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > > index 2147939b..0f7c65db 100644\n> > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > > @@ -10,6 +10,7 @@\n> > > > > > > > > > #pragma once\n> > > > > > > > > >\n> > > > > > > > > > #include \n> > > > > > > > > > +#include \n> > > > > > > > > >\n> > > > > > > > > > #include \"af.h\"\n> > > > > > > > > >\n> > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > > > > > > > > public:\n> > > > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > > > const YamlObject &tuningData);\n> > > > > > > > > > + void configure(const Size &outputSize);\n> > > > > > > > > > int32_t process(double currentContrast);\n> > > > > > > > > > void skipFrames(uint32_t n);\n> > > > > > > > > >\n> > > > > > > > > > controls::AfStateEnum state() override { return state_; }\n> > > > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > > > > > > > > >\n> > > > > > > > > > + Signal windowUpdateRequested;\n> > > > > > > > > > +\n> > > > > > > > > > private:\n> > > > > > > > > > void setMode(controls::AfModeEnum mode) override;\n> > > > > > > > > > void setRange(controls::AfRangeEnum range) override;\n> > > > > > > > > > @@ -54,6 +58,7 @@ private:\n> > > > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > > > > > > > > >\n> > > > > > > > > > /* Current focus lens position. */\n> > > > > > > > > > int32_t lensPosition_ = 0;\n> > > > > > > > > > @@ -84,6 +89,11 @@ private:\n> > > > > > > > > >\n> > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > > > > > > > double maxChange_;\n> > > > > > > > > > +\n> > > > > > > > > > + /* Default AF window. */\n> > > > > > > > > > + Rectangle defaultWindow_;\n> > > > > > > > > > + /* AF window set by user using AF_WINDOWS control. */\n> > > > > > > > > > + Rectangle userWindow_;\n> > > > > > > > > > };\n> > > > > > > > > >\n> > > > > > > > > > } /* namespace ipa::algorithms */","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 09DCCC3213\n\tfor ;\n\tWed, 31 May 2023 17:31:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAE9162722;\n\tWed, 31 May 2023 19:31:53 +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 DE8E1626F8\n\tfor ;\n\tWed, 31 May 2023 19:31:51 +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 4A03C844;\n\tWed, 31 May 2023 19:31:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685554313;\n\tbh=KwAM91HN5A7zB7X2caPczXzphROy/Nv2Mt3AaUxHBfU=;\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=wtXBFqdCBLJmyKDBoE/OBA4r8zGKyF7ySrvI4HPQEmabyqAh3RXaiznANIlbgmu/k\n\tCqV0HOG2bQkFSdJqz154tA9+u3rDYE4i+I9wbQHvLyDwEo6Y3maW1h4yRBhnK98mXo\n\txXhJh94qu/ps8asRII25mwFk03U8Qp8fbKSJUMFRqYZAk8ZZuf1ruBqbb5aDdoequS\n\tw2Dd0R/oxHUAFYiYVp7+sMNIxBMjOk9REgrubQx2JXbO9L5VDWyWXJTJ5YI5qknVZp\n\t3Wi4sw3vj5nmtuQqw7l38FaJEFkcESyh4v+wB/NBQWBb4334H5gSBKxVKyMg8nbMao\n\tVOstDAkUGwbqA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685554290;\n\tbh=KwAM91HN5A7zB7X2caPczXzphROy/Nv2Mt3AaUxHBfU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MKEOBlA09XBERpaAVnQ+XQT6+I23ELL8G6IdT8tkC/azkYH/Sl2Xg5HQs93hB3Ia4\n\taXnd+uBw2ZqETzzqPXo3IB2SMp88inTU/8hvbTYZ8WG/ezpExtgbyOMfy+M7Qbk6pG\n\tzV5/95jmMzAh4iHdgywBY6LRS75zjY0zVdee+6Vw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MKEOBlA0\"; dkim-atps=neutral","Date":"Wed, 31 May 2023 20:31:50 +0300","To":"Daniel Semkowicz ","Message-ID":"<20230531173150.GG24749@pendragon.ideasonboard.com>","References":"<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>\n\t\n\t<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>\n\t\n\t<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>\n\t\n\t<20230426031305.GF1667@pendragon.ideasonboard.com>\n\t\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 05/10] ipa: af_hill_climbing: 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\tNick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27331,"web_url":"https://patchwork.libcamera.org/comment/27331/","msgid":"","date":"2023-06-13T13:17:35","subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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 Laurent,\n\nOn Wed, May 31, 2023 at 7:31 PM Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> (CC'ing David)\n>\n> On Thu, May 18, 2023 at 03:14:04PM +0200, Daniel Semkowicz wrote:\n> > On Wed, Apr 26, 2023 at 6:30 PM Nick Hollinghurst wrote:\n> > > On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart wrote:\n> > > > On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote:\n> > > > > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote:\n> > > > > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote:\n> > > > > > > >\n> > > > > > > > Hi Daniel\n> > > > > > > > cc RPi folks which originally introduced the AF controls and which\n> > > > > > > > have recently been working a new implementation\n> > > > > > > >\n> > > > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote:\n> > > > > > > > > >\n> > > > > > > > > > Hi Daniel\n> > > > > > > > > > sorry I have neglected the Windows related patches in previous\n> > > > > > > > > > reviews as I hoped someone would have shared an opinion on the\n> > > > > > > > > > Signal-based mechanism.\n> > > > > > > > > >\n> > > > > > > > > > I don't have better ideas to propose, so let's go with it\n> > > >\n> > > > I don't like the signal much either. We could simply implement a\n> > > > function to retrieve the window.\n> >\n> > Signal has the advantage that it will be invoked only when the window\n> > has changed. With regular function, the function will need to be called\n> > on each frame to check if there is a new window.\n> > What are the disadvantages of using the signal?\n>\n> I was initially thinking we could always set the AF parameters in the\n> ISP configuration buffer, but that would burn extra CPU cycles in the\n> kernel in interrupt context (for rkisp1 at least) to reprogram\n> registers, which should be avoided.\n>\n> The issue with a signal is that it creates a mid-layer, in the sense\n> that the AF implementation mandates a particular implementation of the\n> platform-specific code. You would need to clearly specify where the\n> signal could be emitted from, as the platform-specific code may not\n> operate properly when getting the window changed signalled at random\n> times.\n>\n> For instance, you're currently emitting the signal from\n>\n> - AfHillClimbing::configure()\n> - AfHillClimbing::setMeteringMode()\n> - AfHillClimbing::setWindows()\n>\n> The last two functions are called from algorithms::Af::queueRequest().\n> The signal is connect to the\n> rkisp1::algorithms::Af::updateCurrentWindow() function, which just\n> stores the rectangle in the updateWindow_ member variable, and that\n> variable is used in rkisp1::algorithms::Af::prepare() to set the ISP\n> parameters buffer. This means that rkisp1::algorithms::Af::prepare()\n> operates with the parameters set by the very latest request, which isn't\n> right. It should operate with the parameters set for the request that\n> corresponds to the frame being prepared. Fixing that is possible, using\n> the frame context (we do so in the AWB algorithm for instance), but that\n> will require the signal handler to know exactly where the signal can be\n> emitted from, and get the right context. The context isn't passed to the\n> signal handler (it doesn't get told for which request the window is\n> set), so we'll have a problem. The rkisp1::algorithms::Af class could\n> still implement this correctly by knowing the signal is emitted from the\n> algorithms::Af::queueRequest() function, and using updateWindow_ in\n> rkisp1::algorithms::Af::queueRequest() after calling\n> algorithms::Af::queueRequest(), but that's implicit knowledge of the\n> internal algorithms::Af helper implementation, which could change in the\n> future.\n>\n> I think it will be simpler if the platform queries the AF window\n> manually, as it will then be in full control of when it retrieves the\n> value and how it uses it. This means the new value will need to be\n> compared with a cached value to decide whether or not to configure the\n> ISP, but that's simpler than going through a signal in the end.\n\nAlright, the situation looks a bit more complex...\nI initially thought that queueRequest(), prepare() and process() are\njust executed sequentially for each frame, but now I see there is an\noptimisation implemented, where queueRequest() and prepare() are called\nearly for each request and buffered. And later, process() is called\nfor each frame with the previously buffered data. Now I understand\nyour worries about signal and delays. Thank you for the explanation!\n\nIt looks that I should make use of the IPAFrameContext and move the\nAf state there, configured for each frame, instead of keeping just the\ncurrent state internally in the Af algorithm classes. Keeping the\nclass state outside the class is not very pretty to me, but I see\nthis is the only reasonable solution there.\nIs my understanding correct?\n\n>\n> > > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > > > > > Add support for setting user defined auto focus window in the\n> > > > > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.\n> > > > > > > > > > > Each time, there is a need for changing the window configuration in the\n> > > >\n> > > > s/Each time,/Each time/\n> > > >\n> > > > > > > > > > > ISP, the signal is emitted. Platform layer that wants to use\n> > > >\n> > > > s/layer that wants/Layers that want/\n> > > >\n> > > > > > > > > > > the \"Windows\" metering mode, needs to connect to this signal\n> > > >\n> > > > s/mode, needs/mode need/\n> > > >\n> > > > > > > > > > > and configure the ISP on each emission.\n> > > > > > > > > > >\n> > > > > > > > > > > Currently only one window is supported.\n> > > > > > > > > > >\n> > > > > > > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > > > > > > ---\n> > > > > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++--\n> > > > > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++\n> > > > > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-)\n> > > > > > > > > > >\n> > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > > > index 244b8803..0fb17df3 100644\n> > > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)\n> > > > > > > > > > > * movement range rather than coarse search result.\n> > > > > > > > > > > * \\todo Implement setRange.\n> > > > > > > > > > > * \\todo Implement setSpeed.\n> > > > > > > > > > > - * \\todo Implement setMeteringMode.\n> > > > > > > > > > > - * \\todo Implement setWindows.\n> > > > > > > > > > > * \\todo Implement the AfPauseDeferred mode.\n> > > > > > > > > > > + * \\todo Implement support for multiple AF windows.\n> > > > > > > > > > > */\n> > > > > > > > > > >\n> > > > > > > > > > > /**\n> > > > > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > > > > return 0;\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > +/**\n> > > > > > > > > > > + * \\brief Configure the AfHillClimbing with sensor information\n> > > > > > > > > > > + * \\param[in] outputSize Camera sensor output size\n> > > > > > > > > > > + *\n> > > > > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > >\n> > > > s/method/function/\n> > > >\n> > > > > > > > > > > + * method of the platform layer.\n> > > > > > > > > > > + */\n> > > > > > > > > > > +void AfHillClimbing::configure(const Size &outputSize)\n> > > > > > > > > >\n> > > > > > > > > > According to the AfWindows control definition\n> > > > > > > > > >\n> > > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set\n> > > > > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle\n> > > > > > > > > > returned by the ScalerCropMaximum property.\n> > > > > > > > > >\n> > > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping\n> > > > > > > > > > a larger portion of the pixel array. The portion of the pixel array\n> > > > > > > > > > processed to obtain the final image is named AnalogueCropRectangle and\n> > > > > > > > > > all valid crop rectangles lie inside it.\n> > > > > > > > > >\n> > > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the\n> > > > > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the\n> > > > > > > > > > current output size. It's up to the pipeline handler to re-scale any\n> > > > > > > > > > valid control in the current configured output.\n> > > > > > > > >\n> > > > > > > > > For the 1024x768 stream, I have the following parameters:\n> > > > > > > > > - activeAreaSize: 2592x1944\n> > > > > > > > > - analogCrop: (0, 0)/2592x1944\n> > > > > > > > > - outputSize: 1296x972\n> > > > > > > > >\n> > > > > > > > > When using analogCrop, I will also need to scale the AF window\n> > > > > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes\n> > > > > > > > > values in reference to the ISP input size\n> > > > > > > > > (IPACameraSensorInfo::outputSize).\n> > > > > > > > > Based on what you wrote earlier, I understand that it is supposed to be\n> > > > > > > > > like that?\n> > > > > > > >\n> > > > > > > > I presume so.\n> > > > > > > >\n> > > > > > > > I should check how RPi handles windowing\n> > > >\n> > > > Nick, that's the first question for you.\n> > >\n> > > Our implementation is based on what we understand to be the current\n> > > libcamera specification: That AF windows are in the same coordinate\n> > > system as ScalerCropMaximum, i.e. raw sensor pixels.\n> > >\n> > > I found the documentation phrase \"The units used are pixels within the\n> > > rectangle...\" ambiguous -- does it suggest that the origin of the\n> > > AfWIndows coordinate system is the top-left corner of\n> > > ScalerCropMaximum? I took the answer to be \"no\", that those rectangles\n> > > were in a common coordinate system, without offsetting. This in turn\n> > > means that Af Windows will stick to the same objects in the scene,\n> > > regardless of mode or digital zoom.\n> >\n> > I had the same problems with understanding this documentation part, so\n> > probably it should be rephrased to clearly state what is the reference\n> > coordination system for AfWindows.\n>\n> David, git pointed to you as the author of the text :-) I share Nick's\n> understanding here, what is yours ?\n>\n> The documentation should be clarified, and I would like to propose\n> changing the wording to make AfWindows relative to the\n> PixelArrayActiveAreas property, not the ScalerCropMaximum. The\n> coordinate system for ScalerCropMaximum isn't defined, but the\n> coordinate system for ScalerCrop is documented as relative to\n> PixelArrayActiveAreas. Having both ScalerCrop and AfWindows expressed\n> in the same coordinate system would make sense to me.\n>\n> There's still a question of what to do with windows outside (or\n> partially outside) of the ScalerCrop. Does it make sense to allow the\n> user to focus on something that isn't visible in the output image ? Use\n> cases seem dubious to me, and well-written applications will likely\n> ensure that the AF windows full within the visible image, but should we\n> enforce this in libcamera ?\n\nIf such a situation is possible, then why not allow this? I would not\nadd more constraints than hardware already requires. It does not make\nimplementation easier :)\n\n>\n> > > > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most\n> > > > > > > > > > probably to allow pipeline handler express any additional processing\n> > > > > > > > > > margins required by any cropping/scaling that happens on the ISP.\n> > > > > > > > > >\n> > > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin\n> > > > > > > > > > yet, I would simply use here the sensor's analogue crop, which is part\n> > > > > > > > > > of the IPACameraSensorInfo you already use in the platform layer.\n> > > > > > > > > >\n> > > > > > > > >\n> > > > > > > > > I have some concerns about the ScalerCropMaximum.\n> > > > > > > > > What if the ISP have different size margins for cropping and auto-focus\n> > > > > > > > > windows? I see there are margins defined for the AF window, but nothing\n> > > > > > > > > about the cropping margins in the RK3399 documentation. In this case,\n> > > > > > > > > it will not be a problem, but what if some ISP will have margins for\n> > > > > > > > > cropping, but no margins for AF window?\n> > > > > > > >\n> > > > > > > > Good point. Do we need an AfWindowMaximum ?\n> > > > > > >\n> > > > > > > To make sense, AfWindowMaximum would need to be expressed in reference\n> > > > > > > to how ISP handles that (sensor output size on rkisp) to allow precise\n> > > > > > > AF window configuration. AF window margins on rkisp are small: 2px and\n> > > > > > > 5px. I would like to avoid scaling, so small values compared to\n> > > > > > > the output size.\n> > > > > >\n> > > > > > For ScalerCrop we decided to have a ScalerCropMaximum property which\n> > > > > > basically reports the AnalogueCrop rectangle, as this is the maximum\n> > > > > > rectangle an application can set as a cropping area. The reference is\n> > > > > > the PixelArrayActiveAreas size.\n> > > > > >\n> > > > > > For AfWindows we don't care about AnalogCrop but only about the\n> > > > > > sensor's output as, if my understanding is correct, this is the\n> > > > > > rectangle on which the ISP performs windowing on.\n> > > > > >\n> > > > > > As we don't expose the sensor's output size to applications, the only\n> > > > > > reference we can use is full PixelArrayActiveAreas and re-scale before\n> > > > > > setting the windows.\n> > > > > >\n> > > > > > > I am not sure if AfWindowMaximum as additional property is actually\n> > > > > > > needed. Maximum value for AfWindows would not be enough?\n> > >\n> > > Encoding the largest useful window as the control maximum value (where\n> > > this means minimum X, Y and maximum width,height?) sounds useful. BTW\n> > > is there any way to encode the maximum number of windows?\n> >\n> > That is a good point. If we specify the maximum windows size, we should\n> > also specify the maximum number of windows.\n> > The only solution I can think of is something like this:\n> >\n> > Rectangle minWindow(0, 0, 0, 0);\n> > ControlValue min(Span({minWindow}));\n> >\n> > Rectangle maxWindow(0, 0, 640, 480);\n> > ControlValue max(Span({maxWindow, maxWindow, maxWindow}));\n> >\n> > Rectangle defWindow(100, 100, 200, 200);\n> > ControlValue def(Span({defWindow}));\n> >\n> > ctrls[&controls::AfWindows] = ControlInfo(min, max,def);\n> >\n> > cam lists such control as:\n> >\n> > Control: AfWindows: [[ (0, 0)/0x0 ]..[ (0, 0)/640x480, (0,\n> > 0)/640x480, (0, 0)/640x480 ]]\n> >\n> > I am not convinced this is a readable solution for the user...\n> > Especially, if there will be bigger number of windows supported.\n> > I expect that minimum and maximum size of will be the same for all\n> > windows, regardless of the number of windows.\n> > From this, it can be deduced that we could expose only the simple pair:\n> > { window size, number of windows } for each min, max and def ControlInfo\n> > value.\n>\n> How about simply adding a AfMaxWindows property ?\n\nI still do not understand the reason for existence of both \"maximum\nvalue\" properties and maximum values in the controls. Isn't it\na duplication? Am I missing something?\n\n>\n> > > > > >\n> > > > > > As a comment on the ScalerCropMaximum property reports\n> > > > > >\n> > > > > > \\todo Turn this property into a \"maximum control value\" for the\n> > > > > > ScalerCrop control once \"dynamic\" controls have been implemented.\n> > > > > >\n> > > > > > I presume this is very old, as right now (at least for RkISP1) I see\n> > > > > > the \"ControlInfoMap *ipaControls\" parameter passed in to configure() and\n> > > > > > being overwritten completely with new control limits.\n> > > > > >\n> > > > > > So my suggestion for a new property was probably not correct.\n> > > > > >\n> > > > > > > Is there a need to expose the window margins to the user at all?\n> > > > > >\n> > > > > > I think so, it would avoid setting controls which can't be applied as\n> > > > > > they are.\n> > > > > >\n> > > > > > I presume you can initialize the maximum value of AfWindows as\n> > > > > >\n> > > > > > { 0, 0, PixelArrayActiveAreas.width - 4,\n> > > > > > PixelArrayActiveAreas.height - 10 }\n> > > > > >\n> > > > > > When re-scaling, you first need to translate it to take into\n> > > > > > consideration the processing margins then re-scale it using the\n> > > > > > activeArea-to-outputSize ratio.\n> > > > > >\n> > > > > > (only compiled, not run-time tested)\n> > > > > >\n> > > > > > Span userWindows =\n> > > > > > *request->controls().get>(controls::AfWindows);\n> > > > > > std::vector afWindows;\n> > > > > >\n> > > > > > for (auto userWindow : userWindows) {\n> > > > > > Rectangle window = userWindow.translatedBy({2, 5});\n> > > > > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize);\n> > > > > > afWindows.push_back(window);\n> > > > > > }\n> > > > > >\n> > > > > > (as you only support one window, this could be even\n> > > > > > simplified)\n> > > > > >\n> > > > > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm\n> > > > > > implementation, and you would need to pass to the algorithm the active\n> > > > > > area size. One way to do so, as algorithms have access to the\n> > > > > > context_, is to add the active area size to\n> > > > > > IPASessionConfiguration::sensor which already contains the sensor's\n> > > > > > output size.\n> > > > > >\n> > > > > > What do you think ?\n> > > > >\n> > > > > I am not fully happy with this approach as this requires scaling\n> > > > > the values two times, but as you said, expressing it directly\n> > > > > in the output size would require exposing additional controls.\n> > > > >\n> > > > > The other way, I was thinking of, is to express the AF window\n> > > > > in the final output image size (1024x768 in my already mentioned\n> > > > > example). This way, it is easier to think about from the user\n> > > > > perspective. However, this is an opposite approach than the already\n> > > > > existing one for the ScalerCrop.\n> > > > >\n> > > > > Maybe I am not 100% happy with this, but it looks that expressing\n> > > > > the AF window as you show above (in reference to the current\n> > > > > PixelArrayActiveAreas) makes the most sense for the existing code,\n> > > > > because it will be the common approach with the ScalerCrop.\n> > > > > I can change my implementation to this approach.\n> > > > >\n> > > > > Do we need to change also the documentation related to the AF Windows?\n> > > > >\n> > > > > > > We could allow the window size of the whole ISP input size to be set\n> > > > > > > by the user and clamp the size in the IPA before configuring the ISP?\n> > > > > > >\n> > > > > > > In summary, I see two options:\n> > > > > > > 1. Maximum AF window expressed in units that are directly set to ISP\n> > > > > > > (output size for rkisp). Maximum AF window will include margins.\n> > > > > > > 2. Keep the current approach and express the AF window in reference to\n> > > > > > > Analogue crop size. Just allow the full size instead of the\n> > > > > > > ScalerCropMaximum. Clamp the margins inside the IPA if user\n> > > > > > > requested larger window.\n> > > > > > >\n> > > > > > > > RPi any opinions here ?\n> > > >\n> > > > And this is the second question for Nick.\n> > >\n> > > I don't have very strong opinions. Option 2 seems the most general.\n> > > (Again, I've been assuming there's no implicit offsetting for the\n> > > \"reference\" rectangle -- so the only difference between those\n> > > definitions is a question of cropping?)\n> > >\n> > > I suspect there is not a one-size-fits-all approach. Some sensors may\n> > > be able to generate focus information outside the cropped region;\n> > > others may not. Some platforms may only be able to measure focus\n> > > within the final image. Cropping \"as required\" sounds like a\n> > > reasonable way to resolve that -- but we ought to give the user some\n> > > way to predict when a window might end up being cropped to nothing\n> > > (and thus ignored)!\n> >\n> > Ok, so my idea with cropping the margins without the user knowledge\n> > is not a good solution... Then, probably We end up with something\n> > similar to the ScalerCrop that is already implemented in the Raspberry\n> > implementation, if I understand it correctly:\n> >\n> > The (x,y) location of the AfWindow is relative to the\n> > PixelArrayActiveAreas that is being used. The units remain native sensor\n> > pixels. Maximum value of the AfWindows ControlInfo (position + size),\n> > already includes all the margins that need to be considered\n> > (e.g. required by the ISP).\n> >\n> > Would you agree with the above definition?\n>\n> I think so. Depending on the platform, the AF statistics can be computed\n> right away on the image received from the sensor, or after some ISP\n> processing steps that will crop a few lines and columns on the edges. I\n> would like to avoid a need for applications to be aware of that\n> cropping, as it should be small. If the window(s) need to be expressed,\n> at the hardware level, in a cropped coordinate system, the IPA module\n> should perform the translation between PixelArrayActiveAreas and the\n> hardware coordinates.\n>\n> If analog crop and/or digital crop gets applied in the sensor, there may\n> be significant cropping, and the application may need to be aware of\n> this. I think you can ignore this for now, we'll need to revisit the\n> processing pipeline model exposed to applications at some point.\n>\n> For platforms that calculate the AF statistics directly on the sensor\n> (or more precisely, where the sensor generates AF data, consumed by the\n> algorithms directly or after processing, as in the PDAF case), the\n> windows could even exceed the sensor digital crop (I doubt they could\n> exceed the analog crop, as pixels outside the analog crop rectangle are\n> not read out by definition). We will have to take that into account too\n> I suppose, but I think you can ignore it for now as you're not dealing\n> with sensor-side AF data.\n>\n> > > > > > > > > > To remove ambiguities I would call the parameter here \"maxWindow\" or\n> > > > > > > > > > something similar.\n> > > > > > > > > >\n> > > > > > > > > > I would also initialize userWindow_ to the same value, so that if an\n> > > > > > > > > > application switches to AfMeteringWindows mode the rectangle is at\n> > > > > > > > > > least initialized to something.\n> > > > > > > > >\n> > > > > > > > > Good point!\n> > > > > > > > >\n> > > > > > > > > > > +{\n> > > > > > > > > > > + /*\n> > > > > > > > > > > + * Default AF window of 3/4 size of the camera sensor output,\n> > > > > > > > > > > + * placed at the center\n> > > > > > > > > > > + */\n> > > > > > > > > > > + defaultWindow_ = Rectangle(static_cast(outputSize.width / 8),\n> > > > > > > > > > > + static_cast(outputSize.height / 8),\n> > > > > > > > > > > + 3 * outputSize.width / 4,\n> > > > > > > > > > > + 3 * outputSize.height / 4);\n> > > > > > > > > > > +\n> > > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > > > > +}\n> > > > > > > > > > > +\n> > > > > > > > > > > /**\n> > > > > > > > > > > * \\brief Run the auto focus algorithm loop\n> > > > > > > > > > > * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n)\n> > > > > > > > > > > framesToSkip_ = n;\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > +/**\n> > > > > > > > > > > + * \\var AfHillClimbing::windowUpdateRequested\n> > > > > > > > > > > + * \\brief Signal emitted when change in AF window was requested\n> > > > > > > > > > > + *\n> > > > > > > > > > > + * Platform layer supporting AF windows should connect to this signal\n> > > >\n> > > > s/layer/layers/\n> > > > s/connect to/connect/\n> > > >\n> > > > > > > > > > > + * and configure the ISP with new window on each emition.\n> > > >\n> > > > s/new window/the new window/\n> > > > s/emition/emission/\n> > > >\n> > > > The new window will likely need a few frames to become effective. I'm\n> > > > worried that the AF algorithm doesn't take that into account at all (it\n> > > > also doesn't take the lens movement delays into account, which is\n> > > > another of my worries). This can affect the performance and stability of\n> > > > the auto-focus algorithm.\n> >\n> > The delay when configuring new window is set in the platform layer in\n> > the patch 07/10. The delay can be different for different ISPs, so I\n> > decided it will be better to leave the delay control to the platform\n> > layer.\n>\n> The ISP doesn't add a delay as such. What happens is that a set of ISP\n> parameters buffers are queued to the ISP, and they are applied one by\n> one to consecutive frames. There's no internal ISP delay, the delay\n> comes from the fact that we queue parameters buffers, so the parameters\n> in the last queued buffer will only be applied after all other buffers\n> get processed. This should be handled by programming the AF window in an\n> ISP parameters buffer not right away when it gets queued in a request,\n> but at the right time so that it will be applied to the correct frame.\n>\n> > Lens delay is a more complex problem and there were discussions\n> > previously about it. We need a proper solution for that.\n> > As a workaround, I introduced the \"wait-frames-lens\" tuning file\n> > parameter in 06/10.\n>\n> The physical properties of the lens movement is indeed a separate and\n> more complex question, which I'm fine handling separately (and later).\n>\n> > > > How have you tested this ?\n> >\n> > Yes, and most of the delays configured using AfHillClimbing::skipFrames()\n> > base on my experiments on RK3399 and camera lens available on my device.\n> > Unfortunately, the documentation I have access to, says very little\n> > about the delays in ISP.\n>\n> Good news, there's no delay in the ISP itself :-)\n>\n> > > > > > > > > > > + */\n> > > > > > > > > > > +\n> > > > > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > > > > > > > > {\n> > > > > > > > > > > if (mode == mode_)\n> > > > > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > > > > > > > > LOG(Af, Error) << \"setSpeed() not implemented!\";\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)\n> > > > > > > > > > > {\n> > > > > > > > > > > - LOG(Af, Error) << \"setMeteringMode() not implemented!\";\n> > > > > > > > > > > + if (metering == meteringMode_)\n> > > > > > > > > > > + return;\n> > > > > > > > > > > +\n> > > > > > > > > > > + meteringMode_ = metering;\n> > > > > > > > > > > +\n> > > > > > > > > > > + switch (metering) {\n> > > > > > > > > > > + case controls::AfMeteringAuto:\n> > > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_);\n> > > > > > > > > > > + break;\n> > > > > > > > > > > + case controls::AfMeteringWindows:\n> > > > > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > > > > + break;\n> > > >\n> > > > This needs a default case to avoid modifying meteringMode_. We have two\n> > > > modes currently defined, if we add a third one, the implementation will\n> > > > break.\n> >\n> > Well, in v4 there was a suggestion to remove the default case from\n> > switch statements, so the compiler will complain early about the missing\n> > implementation.\n>\n> Good point, please ignore my comment :-)\n>\n> > > > When the window is changed the contrast value may drastically change.\n> > > > The optimal focus point will likely change too. I think you need to\n> > > > reset the AF and possibly trigger a rescan for continuous AF mode.\n> >\n> > This is true and sounds like a general problem. Do you think this\n> > behaviour should be documented, so other platforms will also follow this\n> > rule?\n>\n> That's a good idea I think. I'm not sure where to best document it\n> though, the documentation in control_ids.yaml shouldn't assume a\n> particular type of AF implementation, and PDAF-based platforms don't\n> need a rescan as such as they don't really scan (well, they may as a\n> fallback...). We could write the requirement in a generic way that\n> doesn't imply a particular AF method, or document it elsewhere. David,\n> any opinion ?\n>\n> > > > > > > > > > > + }\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > > > > > > > > +void AfHillClimbing::setWindows(Span windows)\n> > > > > > > > > > > {\n> > > > > > > > > > > - LOG(Af, Error) << \"setWindows() not implemented!\";\n> > > > > > > > > > > + if (windows.size() != 1) {\n> > > > > > > > > > > + LOG(Af, Error) << \"Only one AF window is supported\";\n> > > > > > > > > > > + return;\n> > > > > > > > > > > + }\n> > > > > > > > > >\n> > > > > > > > > > Should this be an hard error or should we just want and continue by\n> > > > > > > > > > only considering the first available rectangle ?\n> > > > > > > > >\n> > > > > > > > > I will change it to a warning that only the first window was used.\n> > > > > > > > >\n> > > > > > > > > > > +\n> > > > > > > > > > > + LOG(Af, Debug) << \"setWindows: \" << windows[0];\n> > > > > > > > > > > +\n> > > > > > > > > > > + userWindow_ = windows[0];\n> > > > > > > > > > > +\n> > > > > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows)\n> > > > > > > > > > > + windowUpdateRequested.emit(userWindow_);\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > > > index 2147939b..0f7c65db 100644\n> > > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > > > > > @@ -10,6 +10,7 @@\n> > > > > > > > > > > #pragma once\n> > > > > > > > > > >\n> > > > > > > > > > > #include \n> > > > > > > > > > > +#include \n> > > > > > > > > > >\n> > > > > > > > > > > #include \"af.h\"\n> > > > > > > > > > >\n> > > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af\n> > > > > > > > > > > public:\n> > > > > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition,\n> > > > > > > > > > > const YamlObject &tuningData);\n> > > > > > > > > > > + void configure(const Size &outputSize);\n> > > > > > > > > > > int32_t process(double currentContrast);\n> > > > > > > > > > > void skipFrames(uint32_t n);\n> > > > > > > > > > >\n> > > > > > > > > > > controls::AfStateEnum state() override { return state_; }\n> > > > > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; }\n> > > > > > > > > > >\n> > > > > > > > > > > + Signal windowUpdateRequested;\n> > > > > > > > > > > +\n> > > > > > > > > > > private:\n> > > > > > > > > > > void setMode(controls::AfModeEnum mode) override;\n> > > > > > > > > > > void setRange(controls::AfRangeEnum range) override;\n> > > > > > > > > > > @@ -54,6 +58,7 @@ private:\n> > > > > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> > > > > > > > > > >\n> > > > > > > > > > > /* Current focus lens position. */\n> > > > > > > > > > > int32_t lensPosition_ = 0;\n> > > > > > > > > > > @@ -84,6 +89,11 @@ private:\n> > > > > > > > > > >\n> > > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > > > > > > > > double maxChange_;\n> > > > > > > > > > > +\n> > > > > > > > > > > + /* Default AF window. */\n> > > > > > > > > > > + Rectangle defaultWindow_;\n> > > > > > > > > > > + /* AF window set by user using AF_WINDOWS control. */\n> > > > > > > > > > > + Rectangle userWindow_;\n> > > > > > > > > > > };\n> > > > > > > > > > >\n> > > > > > > > > > > } /* namespace ipa::algorithms */\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 55AA0C31E9\n\tfor ;\n\tTue, 13 Jun 2023 13:17:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A82E861E4B;\n\tTue, 13 Jun 2023 15:17:51 +0200 (CEST)","from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com\n\t[IPv6:2607:f8b0:4864:20::102d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ABFB661E49\n\tfor ;\n\tTue, 13 Jun 2023 15:17:49 +0200 (CEST)","by mail-pj1-x102d.google.com with SMTP id\n\t98e67ed59e1d1-25be7b579e2so1743625a91.3\n\tfor ;\n\tTue, 13 Jun 2023 06:17:49 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1686662271;\n\tbh=bCgmHy/XndUhcM+ryz1cn0WOkSgxGV6DjMqv0UxWpYE=;\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=YjRMTPUBNlgW1cW4H6jLLzk13VaQiYnvqM+0+I30dwFt7gyvdM0ZmEwSb5YMNAGF5\n\tJRHqhtDzP+zfjc228EMwfFtbtEgjo8oIrjNy7e751K0TpM5DLTAUTYEhjlXXTvR2kf\n\tKPyxItv6nhFKGsODrEEy5dKVxTfC1N+AR51mKFoA2vTUd68T4S7g2osbBR+OZC6960\n\t7J0M4LjJDL0IGjrzBqM8ypYm+7tV65poB/8LHdyo31/Q1dk8SpdE2kl6n+U70W3wgZ\n\tbtBQJ4RdQB4MxK88p0uPxHu7YVAfWLbMa1kPNhoxlWCkBDEJ3eMDVDAaL+mJT3RAub\n\t7IdYd8zzXAY0A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1686662268;\n\tx=1689254268; \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=xBQnNsATyfKIuAMVQsmWTirRl5ui+BfFnPMv0K2fAk0=;\n\tb=GR/Y6+2JkViUmhqQiQITNdllk5IFNKHSnKua4kcCMUzv8glz+OhedUoqKtmrbBl+X1\n\tBwlSSKHbgBCwRE1DVppf98IkKFtjVpywnTx42zpNvaqBjl7ateEFE0/tEzaZVSYq7F3I\n\t+MtsJ8GhFRbXCYbHMTLSocrPM1O8EX5Gs2Sv33knaUj4GSDnZpvuTbTijvkWpArdIVih\n\tAUT7Gxjt4o/rMCx1nEL99/NSowvxcuXBnUc650pZT55gYPCNV4/TzSQj+Fo0jkw6Zbh1\n\tIgd9B2zj8Pgi1wwRt/pzHAHB2BG8/Cq1ZZqDQ0cItaWagle4RdLbhsR/4N3D5Li1DHH/\n\tPAmw=="],"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=\"GR/Y6+2J\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1686662268; x=1689254268;\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=xBQnNsATyfKIuAMVQsmWTirRl5ui+BfFnPMv0K2fAk0=;\n\tb=YdLC2jIH5/byHeIHTmt0V7bLikHNhXIse2G4Xvr/Lg8/3E28eGs9W5dwH66sFDvALn\n\t09UAK0GvQEIa8ILV8SvDSiHsiAEnV/TuZ7lpm1tRLJKzAzvSnACFX3Au09xYPly1z80g\n\tb13epAY5nvX6XumRYiSAnnM5golFvxGHEeo5lOvul0llbth05QImtjY4ArHQb4V9Da/u\n\tQn2LDRdzDNvnwf5ASrlSh+U+K1TyAjXmfwkr2EoItygxqG0LTiGF2bVDi9f56PSswJVC\n\tVx9JTnGOTVmQN3/U1zJUUcPGclhtvSdkJFk4kDl6smebK7g5Y9kWI/1X837w7AC+0p4d\n\tLzyw==","X-Gm-Message-State":"AC+VfDxt5o1SHqsP3XvWfYcKTywlDsOfn2zLXxppyRxzCHM7GBQJunzN\n\tR8+t/4LIeUGTvNFsarEaCgy/Fz0y7hh0ESzjsEdjfFXS0axSOT1sQeE=","X-Google-Smtp-Source":"ACHHUZ4B1h5vrh811UeJkVwibdDISrja2uNiQ+cz1kwifJ1R0L3eSxzdNU43BqgoxixT/IUUfGDLcRQ086B5AAr98zk=","X-Received":"by 2002:a17:90a:181:b0:255:d86c:baec with SMTP id\n\t1-20020a17090a018100b00255d86cbaecmr12149824pjc.46.1686662267746;\n\tTue, 13 Jun 2023 06:17:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-6-dse@thaumatec.com>\n\t<20230403130732.szbyy3trz7imxcff@uno.localdomain>\n\t\n\t<20230404105911.7e5cp5ymzh2udasb@uno.localdomain>\n\t\n\t<20230405143938.sf5irwnqzxlaoi7j@uno.localdomain>\n\t\n\t<20230426031305.GF1667@pendragon.ideasonboard.com>\n\t\n\t\n\t<20230531173150.GG24749@pendragon.ideasonboard.com>","In-Reply-To":"<20230531173150.GG24749@pendragon.ideasonboard.com>","Date":"Tue, 13 Jun 2023 15:17:35 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: 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\tNick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]