[{"id":25036,"web_url":"https://patchwork.libcamera.org/comment/25036/","msgid":"<166371539680.18961.4195478152813315579@Monstersaurus>","date":"2022-09-20T23:09:56","subject":"Re: [libcamera-devel] [PATCH v4 23/32] ipa: rkisp1: filter: Store\n\tper-frame information in frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:51)\n> Rework the algorithm's usage of the active state, to store the value of\n> controls for the last queued request in the queueRequest() function, and\n> store a copy of the values in the corresponding frame context. The\n> latter is used in the prepare() function to populate the ISP parameters\n> with values corresponding to the right frame.\n> \n\nLooks good again. I like that the default case with the unsupported\ndenoise value clearly leaves us such that we assign the most recent /\nactiveState to the frame context, and mark that no update will occur.\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/filter.cpp | 30 +++++++++++++++-------------\n> src/ipa/rkisp1/ipa_context.cpp | 18 ++++++++++++++---\n> src/ipa/rkisp1/ipa_context.h | 7 ++++++-\n> 3 files changed, 37 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp\n> index e64bd6a6d68f..ac743729e247 100644\n> --- a/src/ipa/rkisp1/algorithms/filter.cpp\n> +++ b/src/ipa/rkisp1/algorithms/filter.cpp\n> @@ -44,15 +44,16 @@ static constexpr uint32_t kFiltModeDefault = 0x000004f2;\n> */\n> void Filter::queueRequest(IPAContext &context,\n> [[maybe_unused]] const uint32_t frame,\n> - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> + RkISP1FrameContext &frameContext,\n> const ControlList &controls)\n> {\n> auto &filter = context.activeState.filter;\n> + bool update = false;\n> \n> const auto &sharpness = controls.get(controls::Sharpness);\n> if (sharpness) {\n> filter.sharpness = std::round(std::clamp(*sharpness, 0.0f, 10.0f));\n> - filter.updateParams = true;\n> + update = true;\n> \n> LOG(RkISP1Filter, Debug) << \"Set sharpness to \" << *sharpness;\n> }\n> @@ -64,41 +65,42 @@ void Filter::queueRequest(IPAContext &context,\n> switch (*denoise) {\n> case controls::draft::NoiseReductionModeOff:\n> filter.denoise = 0;\n> - filter.updateParams = true;\n> + update = true;\n> break;\n> case controls::draft::NoiseReductionModeMinimal:\n> filter.denoise = 1;\n> - filter.updateParams = true;\n> + update = true;\n> break;\n> case controls::draft::NoiseReductionModeHighQuality:\n> case controls::draft::NoiseReductionModeFast:\n> filter.denoise = 3;\n> - filter.updateParams = true;\n> + update = true;\n> break;\n> default:\n> LOG(RkISP1Filter, Error)\n> << \"Unsupported denoise value \"\n> << *denoise;\n> + break;\n> }\n> }\n> +\n> + frameContext.filter.denoise = filter.denoise;\n> + frameContext.filter.sharpness = filter.sharpness;\n> + frameContext.filter.update = update;\n> }\n> \n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> -void Filter::prepare(IPAContext &context,\n> +void Filter::prepare([[maybe_unused]] IPAContext &context,\n> [[maybe_unused]] const uint32_t frame,\n> - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> + RkISP1FrameContext &frameContext,\n> rkisp1_params_cfg *params)\n> {\n> - auto &filter = context.activeState.filter;\n> -\n> /* Check if the algorithm configuration has been updated. */\n> - if (!filter.updateParams)\n> + if (!frameContext.filter.update)\n> return;\n> \n> - filter.updateParams = false;\n> -\n> static constexpr uint16_t filt_fac_sh0[] = {\n> 0x04, 0x07, 0x0a, 0x0c, 0x10, 0x14, 0x1a, 0x1e, 0x24, 0x2a, 0x30\n> };\n> @@ -147,8 +149,8 @@ void Filter::prepare(IPAContext &context,\n> 0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3\n> };\n> \n> - uint8_t denoise = filter.denoise;\n> - uint8_t sharpness = filter.sharpness;\n> + uint8_t denoise = frameContext.filter.denoise;\n> + uint8_t sharpness = frameContext.filter.sharpness;\n> auto &flt_config = params->others.flt_config;\n> \n> flt_config.fac_sh0 = filt_fac_sh0[sharpness];\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index b0210a978559..b2628ef73d49 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -179,9 +179,6 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * \\var IPAActiveState::filter.sharpness\n> * \\brief Sharpness level\n> - *\n> - * \\var IPAActiveState::filter.updateParams\n> - * \\brief Indicates if ISP parameters need to be updated\n> */\n> \n> /**\n> @@ -260,6 +257,21 @@ namespace libcamera::ipa::rkisp1 {\n> * compared to the previous frame\n> */\n> \n> +/**\n> + * \\var RkISP1FrameContext::filter\n> + * \\brief Filter parameters for this frame\n> + *\n> + * \\struct RkISP1FrameContext::filter.denoise\n> + * \\brief Denoising level\n> + *\n> + * \\var RkISP1FrameContext::filter.sharpness\n> + * \\brief Sharpness level\n> + *\n> + * \\var RkISP1FrameContext::filter.updateParams\n> + * \\brief Indicates if the filter parameters have been updated compared to the\n> + * previous frame\n> + */\n> +\n> /**\n> * \\var RkISP1FrameContext::sensor\n> * \\brief Sensor configuration that used been used for this frame\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index c22e1f099c23..c15b908afcc8 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -84,7 +84,6 @@ struct IPAActiveState {\n> struct {\n> uint8_t denoise;\n> uint8_t sharpness;\n> - bool updateParams;\n> } filter;\n> };\n> \n> @@ -117,6 +116,12 @@ struct RkISP1FrameContext : public FrameContext {\n> bool update;\n> } dpf;\n> \n> + struct {\n> + uint8_t denoise;\n> + uint8_t sharpness;\n> + bool update;\n> + } filter;\n> +\n> struct {\n> uint32_t exposure;\n> double gain;\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 5660EC3272\n\tfor ;\n\tTue, 20 Sep 2022 23:10:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A275461F7D;\n\tWed, 21 Sep 2022 01:10:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10BC761F7D\n\tfor ;\n\tWed, 21 Sep 2022 01:10:00 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 87209415;\n\tWed, 21 Sep 2022 01:09:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663715401;\n\tbh=9bVGRrt8vgJZwHLF2048aiOPOg1oYQT/6f7H8lg56uw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=udVfRCuePzcRUorV0avS0LaBJx6tQuCjpm6BTsB3uws6bpN5VujqIjS8HNKaZWAtl\n\t2MM47L+75WZ7nU0sxtyIGe5Vz7QV0emiMklswp3tXJrSOdrS6pbi/unENmZ+qGJmqM\n\tjSzHoSaj+kTA0ZvbmJyg4NwNB+ZGiRG3uieLyAnrLl05m00Fvmd1ICZXan1OEPHE64\n\tVd2dFN+/97ZJPdQlDLjHxChyp+CbMlISVcxWHlCLes4ZMQLkYNN8K9sMBqUKuU3Bas\n\tt5Et8HaEjt5Fcd5MbaiI0X3nTB0GDg38MmdgpIewEix9ix3G2OfQymIg7ta9L0lD2J\n\tcDr+kukJtu9Tw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663715399;\n\tbh=9bVGRrt8vgJZwHLF2048aiOPOg1oYQT/6f7H8lg56uw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=JRAVz7cacCmLf/b5swwlI/Asp1QUjBSItvsS3/mu7zugo5zQ7UWdshhyhz5YpIyE9\n\ti6m0O2b+Eliogvqnexhjo6nQgsIVWWVvIb6647O7vknLiPpaPwSZKafVmZEk6SpXU7\n\tkcgO/SBjtnBSqbgJJdKXfxGiXR5YWeqYCPVPV6d4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JRAVz7ca\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-24-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-24-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 21 Sep 2022 00:09:56 +0100","Message-ID":"<166371539680.18961.4195478152813315579@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 23/32] ipa: rkisp1: filter: Store\n\tper-frame information in frame context","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25069,"web_url":"https://patchwork.libcamera.org/comment/25069/","msgid":"<20220921204559.hrqnxfjgqa4hagby@uno.localdomain>","date":"2022-09-21T20:45:59","subject":"Re: [libcamera-devel] [PATCH v4 23/32] ipa: rkisp1: filter: Store\n\tper-frame information in frame context","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Same question as the previous one and same assumption this is\nintentional\n\nReviewed-by: Jacopo Mondi \n\n\nOn Thu, Sep 08, 2022 at 04:41:51AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Rework the algorithm's usage of the active state, to store the value of\n> controls for the last queued request in the queueRequest() function, and\n> store a copy of the values in the corresponding frame context. The\n> latter is used in the prepare() function to populate the ISP parameters\n> with values corresponding to the right frame.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/filter.cpp | 30 +++++++++++++++-------------\n> src/ipa/rkisp1/ipa_context.cpp | 18 ++++++++++++++---\n> src/ipa/rkisp1/ipa_context.h | 7 ++++++-\n> 3 files changed, 37 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp\n> index e64bd6a6d68f..ac743729e247 100644\n> --- a/src/ipa/rkisp1/algorithms/filter.cpp\n> +++ b/src/ipa/rkisp1/algorithms/filter.cpp\n> @@ -44,15 +44,16 @@ static constexpr uint32_t kFiltModeDefault = 0x000004f2;\n> */\n> void Filter::queueRequest(IPAContext &context,\n> \t\t\t [[maybe_unused]] const uint32_t frame,\n> -\t\t\t [[maybe_unused]] RkISP1FrameContext &frameContext,\n> +\t\t\t RkISP1FrameContext &frameContext,\n> \t\t\t const ControlList &controls)\n> {\n> \tauto &filter = context.activeState.filter;\n> +\tbool update = false;\n>\n> \tconst auto &sharpness = controls.get(controls::Sharpness);\n> \tif (sharpness) {\n> \t\tfilter.sharpness = std::round(std::clamp(*sharpness, 0.0f, 10.0f));\n> -\t\tfilter.updateParams = true;\n> +\t\tupdate = true;\n>\n> \t\tLOG(RkISP1Filter, Debug) << \"Set sharpness to \" << *sharpness;\n> \t}\n> @@ -64,41 +65,42 @@ void Filter::queueRequest(IPAContext &context,\n> \t\tswitch (*denoise) {\n> \t\tcase controls::draft::NoiseReductionModeOff:\n> \t\t\tfilter.denoise = 0;\n> -\t\t\tfilter.updateParams = true;\n> +\t\t\tupdate = true;\n> \t\t\tbreak;\n> \t\tcase controls::draft::NoiseReductionModeMinimal:\n> \t\t\tfilter.denoise = 1;\n> -\t\t\tfilter.updateParams = true;\n> +\t\t\tupdate = true;\n> \t\t\tbreak;\n> \t\tcase controls::draft::NoiseReductionModeHighQuality:\n> \t\tcase controls::draft::NoiseReductionModeFast:\n> \t\t\tfilter.denoise = 3;\n> -\t\t\tfilter.updateParams = true;\n> +\t\t\tupdate = true;\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tLOG(RkISP1Filter, Error)\n> \t\t\t\t<< \"Unsupported denoise value \"\n> \t\t\t\t<< *denoise;\n> +\t\t\tbreak;\n> \t\t}\n> \t}\n> +\n> +\tframeContext.filter.denoise = filter.denoise;\n> +\tframeContext.filter.sharpness = filter.sharpness;\n> +\tframeContext.filter.update = update;\n> }\n>\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> -void Filter::prepare(IPAContext &context,\n> +void Filter::prepare([[maybe_unused]] IPAContext &context,\n> \t\t [[maybe_unused]] const uint32_t frame,\n> -\t\t [[maybe_unused]] RkISP1FrameContext &frameContext,\n> +\t\t RkISP1FrameContext &frameContext,\n> \t\t rkisp1_params_cfg *params)\n> {\n> -\tauto &filter = context.activeState.filter;\n> -\n> \t/* Check if the algorithm configuration has been updated. */\n> -\tif (!filter.updateParams)\n> +\tif (!frameContext.filter.update)\n> \t\treturn;\n>\n> -\tfilter.updateParams = false;\n> -\n> \tstatic constexpr uint16_t filt_fac_sh0[] = {\n> \t\t0x04, 0x07, 0x0a, 0x0c, 0x10, 0x14, 0x1a, 0x1e, 0x24, 0x2a, 0x30\n> \t};\n> @@ -147,8 +149,8 @@ void Filter::prepare(IPAContext &context,\n> \t\t0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3\n> \t};\n>\n> -\tuint8_t denoise = filter.denoise;\n> -\tuint8_t sharpness = filter.sharpness;\n> +\tuint8_t denoise = frameContext.filter.denoise;\n> +\tuint8_t sharpness = frameContext.filter.sharpness;\n> \tauto &flt_config = params->others.flt_config;\n>\n> \tflt_config.fac_sh0 = filt_fac_sh0[sharpness];\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index b0210a978559..b2628ef73d49 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -179,9 +179,6 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * \\var IPAActiveState::filter.sharpness\n> * \\brief Sharpness level\n> - *\n> - * \\var IPAActiveState::filter.updateParams\n> - * \\brief Indicates if ISP parameters need to be updated\n> */\n>\n> /**\n> @@ -260,6 +257,21 @@ namespace libcamera::ipa::rkisp1 {\n> * compared to the previous frame\n> */\n>\n> +/**\n> + * \\var RkISP1FrameContext::filter\n> + * \\brief Filter parameters for this frame\n> + *\n> + * \\struct RkISP1FrameContext::filter.denoise\n> + * \\brief Denoising level\n> + *\n> + * \\var RkISP1FrameContext::filter.sharpness\n> + * \\brief Sharpness level\n> + *\n> + * \\var RkISP1FrameContext::filter.updateParams\n> + * \\brief Indicates if the filter parameters have been updated compared to the\n> + * previous frame\n> + */\n> +\n> /**\n> * \\var RkISP1FrameContext::sensor\n> * \\brief Sensor configuration that used been used for this frame\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index c22e1f099c23..c15b908afcc8 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -84,7 +84,6 @@ struct IPAActiveState {\n> \tstruct {\n> \t\tuint8_t denoise;\n> \t\tuint8_t sharpness;\n> -\t\tbool updateParams;\n> \t} filter;\n> };\n>\n> @@ -117,6 +116,12 @@ struct RkISP1FrameContext : public FrameContext {\n> \t\tbool update;\n> \t} dpf;\n>\n> +\tstruct {\n> +\t\tuint8_t denoise;\n> +\t\tuint8_t sharpness;\n> +\t\tbool update;\n> +\t} filter;\n> +\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> --\n> Regards,\n>\n> Laurent Pinchart\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 D50FFC0DA4\n\tfor ;\n\tWed, 21 Sep 2022 20:46:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A4AB621FC;\n\tWed, 21 Sep 2022 22:46:03 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEEB4600AA\n\tfor ;\n\tWed, 21 Sep 2022 22:46:01 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2C71B200005;\n\tWed, 21 Sep 2022 20:46:00 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663793163;\n\tbh=NHtcHUwzEBgpyhlsTzRLzWfQkDcMVYQL505WbMVjH1E=;\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=DdE8jTuIYRLaF3lHSm/4pcvOm/XmjIErqz8SmiaPYK2BgxJ0FCSBUQurbZ5Ns/q+0\n\tAMDnyAOan7A1kQg5dIhLUzS/M8sceOlbvpPBkupmS02/ve4e2p06j8PHubJynL6BaJ\n\tP7tVHdsNKKXC41is0H94T7hnJSuxHFkb0ENrgVzwkn2jA8qFRgyKYVw0UBUzeNwK23\n\tCFjwrs07PIucvC14gGd4ZwJ0IP9LqlBSYJtnKzOtTZGdt3vG82GxTnM+ifhoy7BdMC\n\tPGRXtm5X31QMdKSTzVggsqJ9cXyzaRKfp3tDCRpfK6irb2JbST6iYIYGgL3Sau3O5r\n\tjPI4LBI5mBB6A==","Date":"Wed, 21 Sep 2022 22:45:59 +0200","To":"Laurent Pinchart ","Message-ID":"<20220921204559.hrqnxfjgqa4hagby@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-24-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220908014200.28728-24-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 23/32] ipa: rkisp1: filter: Store\n\tper-frame information in frame context","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\" "}}]