[{"id":23890,"web_url":"https://patchwork.libcamera.org/comment/23890/","msgid":"<20220714195632.mdprg5ycwwm6l6fe@uno.localdomain>","date":"2022-07-14T19:56:32","subject":"Re: [libcamera-devel] [PATCH v2 08/11] ipa: rkisp1: Add \"Windows\"\n\tMetering mode to auto focus algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel\n\nOn Wed, Jul 13, 2022 at 10:43:14AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Allow manually setting auto focus window. Currently only one window is\n> enabled, but ISP allows up to three of them.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> src/ipa/rkisp1/algorithms/af.cpp | 74 ++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/af.h | 9 +++\n> src/ipa/rkisp1/rkisp1.cpp | 20 +++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> 4 files changed, 99 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\n> index 6e047804..cd4f0e08 100644\n> --- a/src/ipa/rkisp1/algorithms/af.cpp\n> +++ b/src/ipa/rkisp1/algorithms/af.cpp\n> @@ -20,20 +20,54 @@ namespace libcamera::ipa::rkisp1::algorithms {\n>\n> LOG_DEFINE_CATEGORY(RkISP1Af)\n>\n> +\n> +static rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)\n> +{\n> +\trkisp1_cif_isp_window ispwindow;\n> +\n> +\tispwindow.h_offs = rectangle.x;\n> +\tispwindow.v_offs = rectangle.y;\n> +\tispwindow.h_size = rectangle.width;\n> +\tispwindow.v_size = rectangle.height;\n> +\n> +\treturn ispwindow;\n> +}\n> +\n\nnit: libcamera uses anonymous namspaces for static functions\n\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::configure\n> */\n> int Af::configure([[maybe_unused]] IPAContext &context,\n> -\t\t [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> +\t\t const IPACameraSensorInfo &configInfo)\n> {\n> +\t/* Default AF window of 3/4 size of the screen placed at the center */\n> +\tdefaultWindow_ = Rectangle(configInfo.outputSize.width / 8,\n> +\t\t\t\t configInfo.outputSize.height / 8,\n> +\t\t\t\t 3 * configInfo.outputSize.width / 4,\n> +\t\t\t\t 3 * configInfo.outputSize.height / 4);\n\nThis is never changed, could be made a class constexpr\n\n> +\tupdateCurrentWindow(defaultWindow_);\n> +\n> \treturn 0;\n> }\n>\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> -void Af::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] rkisp1_params_cfg *params)\n> +void Af::prepare([[maybe_unused]] IPAContext &context, rkisp1_params_cfg *params)\n> {\n> +\tif (updateAfwindow_) {\n\n if (!updateAfwindow_)\n return;\n\n And save one indentation level\n\nActually this function could likely grow and handle other controls, so\nmaybe it's fine like this\n\n> +\t\tparams->meas.afc_config.num_afm_win = 1;\n> +\t\tparams->meas.afc_config.thres = 128;\n> +\t\tparams->meas.afc_config.var_shift = 4;\n> +\t\t/* \\todo Allow setting thres and var_shift in tuning file */\n> +\n> +\t\tparams->meas.afc_config.afm_win[0] = rectangleToIspWindow(currentWindow_);\n\nThis function is only used here and basically initialize afm_win[0].\nShould it be inlined here ?\n\n> +\n> +\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> +\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;\n> +\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;\n> +\n> +\t\tupdateAfwindow_ = false;\n\nI wonder how this would look like with an std::optional in\nwhich could both store the value and the state to replacef\nupdateAfwindow_ and currentWindow_ with a single flag.\n\nOnly if you like the idea though :)\n\n> +\t}\n> }\n>\n> /**\n> @@ -55,14 +89,42 @@ void Af::process(IPAContext &context,\n> \tcontext.frameContext.af.focus = lensPosition;\n> }\n>\n> -void Af::setMetering([[maybe_unused]] controls::AfMeteringEnum metering)\n> +void Af::setMetering(controls::AfMeteringEnum metering)\n\nsetMeteringMode() ?\n\n> {\n> -\tLOG(RkISP1Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +\tif (metering == meteringMode_)\n> +\t\treturn;\n> +\n> +\tif (metering == controls::AfMeteringWindows) {\n> +\t\tupdateCurrentWindow(userWindow_);\n> +\t} else {\n> +\t\tupdateCurrentWindow(defaultWindow_);\n> +\t}\n\nNo braces needed\n\n> +\n> +\tmeteringMode_ = metering;\n> +}\n> +\n> +void Af::setWindows(Span windows)\n> +{\n> +\tif (windows.size() != 1) {\n\n >= ?\n\nActually I wonder if there's any risk the Span<> of windows created by\nthe application can be of dynamic extent and give back INT_MAX here..\n\n> +\t\tLOG(RkISP1Af, Error) << \"Only one AF window is supported\";\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* \\todo Check if window size is valid for ISP */\n> +\n> +\tLOG(RkISP1Af, Debug) << \"setWindows: \" << userWindow_;\n> +\n> +\tuserWindow_ = windows[0];\n> +\n> +\tif (meteringMode_ == controls::AfMeteringWindows) {\n> +\t\tupdateCurrentWindow(userWindow_);\n> +\t}\n\nNo braces and let's check this before assigning userWindow_.\n\nActually this might be subtle as it defines how the algorithm behaves\nwhen it receives conflicting controls (as here it receives a windows\nwhile it cannot apply it). This is something we should better define\nin the controls description actually (for all controls, not just AF,\nideally they will all work the same).\n\nAnyway, what should the algorithm do here ? Forget it and when later the\nmode is switched back to manual wait for a new one, or cache it and\napply it as soon as the mode is switched ? I feel the first one is\nmore logic, but I guess we haven't reached full consensus.\n\nAny opinion ?\n\n> }\n>\n> -void Af::setWindows([[maybe_unused]] Span windows)\n> +void Af::updateCurrentWindow(const Rectangle &window)\n> {\n> -\tLOG(RkISP1Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +\tcurrentWindow_ = window;\n> +\tupdateAfwindow_ = true;\n> }\n>\n> REGISTER_IPA_ALGORITHM(Af, \"Af\")\n> diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\n> index e9afbb98..eeb806c0 100644\n> --- a/src/ipa/rkisp1/algorithms/af.h\n> +++ b/src/ipa/rkisp1/algorithms/af.h\n> @@ -27,6 +27,15 @@ public:\n>\n> \tvoid setMetering(controls::AfMeteringEnum metering) override;\n> \tvoid setWindows(Span windows) override;\n> +\n> +private:\n> +\tvoid updateCurrentWindow(const Rectangle &window);\n> +\n> +\tcontrols::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;\n> +\tRectangle currentWindow_;\n> +\tRectangle defaultWindow_;\n> +\tRectangle userWindow_;\n> +\tbool updateAfwindow_ = false;\n> };\n>\n> } /* namespace libcamera::ipa::rkisp1::algorithms */\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 53b53f12..99ac1fb7 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -319,6 +319,26 @@ void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> \t\t\taf->setMode(static_cast(ctrlValue.get()));\n> \t\t\tbreak;\n> \t\t}\n> +\t\tcase controls::AF_METERING: {\n> +\t\t\tAf *af = getAlgorithm();\n> +\t\t\tif (!af) {\n> +\t\t\t\tLOG(IPARkISP1, Warning) << \"Could not set AF_WINDOWS - no AF algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\taf->setMetering(static_cast(ctrlValue.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase controls::AF_WINDOWS: {\n> +\t\t\tAf *af = getAlgorithm();\n> +\t\t\tif (!af) {\n> +\t\t\t\tLOG(IPARkISP1, Warning) << \"Could not set AF_WINDOWS - no AF algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\taf->setWindows(ctrlValue.get>());\n> +\t\t\tbreak;\n> +\t\t}\n> \t\tcase controls::AF_TRIGGER: {\n> \t\t\tAf *af = getAlgorithm();\n> \t\t\tif (!af) {\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 99d66b1d..7ab03057 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -961,6 +961,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tControlInfoMap::Map ctrls({\n> \t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> \t\t{ &controls::AfMode, ControlInfo(controls::AfModeValues) },\n> +\t\t{ &controls::AfMetering, ControlInfo(controls::AfMeteringValues) },\n> +\t\t{ &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n\nThis should be initialized to match the sensor's active pixel area if\nI'm not mistaken\n\n - AfWindows:\n type: Rectangle\n description: |\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 - ScalerCropMaximum:\n type: Rectangle\n description: |\n The maximum valid rectangle for the controls::ScalerCrop control. This\n reflects the minimum mandatory cropping applied in the camera sensor and\n the rest of the pipeline. Just as the ScalerCrop control, it defines a\n rectangle taken from the sensor's active pixel array.\n\n> \t\t{ &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) },\n> \t\t{ &controls::AfPause, ControlInfo(controls::AfPauseValues) }\n> \t});\n> --\n> 2.34.1\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 5B92EBE173\n\tfor ;\n\tThu, 14 Jul 2022 19:56:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B910163312;\n\tThu, 14 Jul 2022 21:56:35 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::223])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF88B6330D\n\tfor ;\n\tThu, 14 Jul 2022 21:56:34 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 48ACB60002;\n\tThu, 14 Jul 2022 19:56:34 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657828595;\n\tbh=a/Qk0A1uLOn0kv27co8ZhGSh+BcTK0bp5bTzpfDgqR4=;\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=LJ/N8dYNjfVm1Za56GNzl51nSKjYgEaTsR2bgTunA/297tV947NB/gU0sNpGY8WLn\n\tCI37jzJMCxvcFcT83G23+G9/cHLfqOBwTICcHx5m1JIN6gNaJuOw8oNo/t79FWofcM\n\tq5f71QCNMIhV87izj6OEvf6ybQMbtRGYXQz2h+MLdFOeD+9YaaKKX4xwda2i4ZSdKB\n\tqU3oUA0eqrx1375CM1Ir13PNeYSk9voHkJNzD2KRgbkvEPBKjVciFqss6xKpm3BeEo\n\tMD/d9bvyH4p3rtdWkNqrj88R2MYuzASFhYb0uvCWC8VwOspuDI9jcsYhjblt8P0j/l\n\thtiw0K+mqUzGw==","Date":"Thu, 14 Jul 2022 21:56:32 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220714195632.mdprg5ycwwm6l6fe@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-9-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220713084317.24268-9-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v2 08/11] ipa: rkisp1: Add \"Windows\"\n\tMetering mode to auto focus algorithm","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\" "}}]