[{"id":25034,"web_url":"https://patchwork.libcamera.org/comment/25034/","msgid":"<166371495206.18961.1392520108897231032@Monstersaurus>","date":"2022-09-20T23:02:32","subject":"Re: [libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: 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:49)\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/cproc.cpp | 51 ++++++++++++++++++-----------\n> src/ipa/rkisp1/ipa_context.cpp | 21 ++++++++++--\n> src/ipa/rkisp1/ipa_context.h | 8 ++++-\n> 3 files changed, 56 insertions(+), 24 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> index 22a70e0b70c7..9c442cd56b3f 100644\n> --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> @@ -38,55 +38,66 @@ LOG_DEFINE_CATEGORY(RkISP1CProc)\n> */\n> void ColorProcessing::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 &cproc = context.activeState.cproc;\n> + bool update = false;\n> \n> const auto &brightness = controls.get(controls::Brightness);\n> if (brightness) {\n> - cproc.brightness = std::clamp(std::lround(*brightness * 128), -128, 127);\n> - cproc.updateParams = true;\n> + int value = std::clamp(std::lround(*brightness * 128), -128, 127);\n> + if (cproc.brightness != value) {\n> + cproc.brightness = value;\n> + update = true;\n> + }\n> \n> - LOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> + LOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> }\n> \n> const auto &contrast = controls.get(controls::Contrast);\n> if (contrast) {\n> - cproc.contrast = std::clamp(std::lround(*contrast * 128), 0, 255);\n> - cproc.updateParams = true;\n> + int value = std::clamp(std::lround(*contrast * 128), 0, 255);\n> + if (cproc.contrast != value) {\n> + cproc.contrast = value;\n> + update = true;\n> + }\n> \n> - LOG(RkISP1CProc, Debug) << \"Set contrast to \" << *contrast;\n> + LOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> }\n> \n> const auto saturation = controls.get(controls::Saturation);\n> if (saturation) {\n> - cproc.saturation = std::clamp(std::lround(*saturation * 128), 0, 255);\n> - cproc.updateParams = true;\n> + int value = std::clamp(std::lround(*saturation * 128), 0, 255);\n> + if (cproc.saturation != value) {\n> + cproc.saturation = value;\n> + update = true;\n> + }\n> \n> - LOG(RkISP1CProc, Debug) << \"Set saturation to \" << *saturation;\n> + LOG(RkISP1CProc, Debug) << \"Set saturation to \" << value;\n> }\n> +\n> + frameContext.cproc.brightness = cproc.brightness;\n> + frameContext.cproc.contrast = cproc.contrast;\n> + frameContext.cproc.saturation = cproc.saturation;\n> + frameContext.cproc.update = update;\n\nI think this all sounds right.\n\nI wonder what common patterns will emerge from these conversions ...\n\n> }\n> \n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> -void ColorProcessing::prepare(IPAContext &context,\n> +void ColorProcessing::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 &cproc = context.activeState.cproc;\n> -\n> /* Check if the algorithm configuration has been updated. */\n> - if (!cproc.updateParams)\n> + if (!frameContext.cproc.update)\n> return;\n\nYes, this now responds to the frame context for the correct frame.\n\n> \n> - cproc.updateParams = false;\n> -\n> - params->others.cproc_config.brightness = cproc.brightness;\n> - params->others.cproc_config.contrast = cproc.contrast;\n> - params->others.cproc_config.sat = cproc.saturation;\n> + params->others.cproc_config.brightness = frameContext.cproc.brightness;\n> + params->others.cproc_config.contrast = frameContext.cproc.contrast;\n> + params->others.cproc_config.sat = frameContext.cproc.saturation;\n> \n> params->module_en_update |= RKISP1_CIF_ISP_MODULE_CPROC;\n> params->module_ens |= RKISP1_CIF_ISP_MODULE_CPROC;\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index cd1443e1ed46..936b77709417 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -160,9 +160,6 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * \\var IPAActiveState::cproc.saturation\n> * \\brief Saturation level\n> - *\n> - * \\var IPAActiveState::cproc.updateParams\n> - * \\brief Indicates if ISP parameters need to be updated\n> */\n> \n> /**\n> @@ -236,6 +233,24 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Whether the Auto White Balance algorithm is enabled\n> */\n> \n> +/**\n> + * \\var RkISP1FrameContext::cproc\n> + * \\brief Color Processing parameters for this frame\n> + *\n> + * \\struct RkISP1FrameContext::cproc.brightness\n> + * \\brief Brightness level\n> + *\n> + * \\var RkISP1FrameContext::cproc.contrast\n> + * \\brief Contrast level\n> + *\n> + * \\var RkISP1FrameContext::cproc.saturation\n> + * \\brief Saturation level\n> + *\n> + * \\var RkISP1FrameContext::cproc.update\n> + * \\brief Indicates if the color processing parameters have been updated\n> + * compared to the 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 d97aae9a97b4..78edb607d039 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -75,7 +75,6 @@ struct IPAActiveState {\n> int8_t brightness;\n> uint8_t contrast;\n> uint8_t saturation;\n> - bool updateParams;\n> } cproc;\n> \n> struct {\n> @@ -107,6 +106,13 @@ struct RkISP1FrameContext : public FrameContext {\n> bool autoEnabled;\n> } awb;\n> \n> + struct {\n> + int8_t brightness;\n> + uint8_t contrast;\n> + uint8_t saturation;\n> + bool update;\n> + } cproc;\n\nAnd I wonder if the common pattern will be lots of anonymous structures\nthat are in both the active state, and the frame context, while the\nframe context has an 'updated' flag...\n\nMaybe we can optimise this later - but I think this gets us moving\nanyway to explore what it will become.\n\n\nReviewed-by: Kieran Bingham \n\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 94932C3272\n\tfor ;\n\tTue, 20 Sep 2022 23:02:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7B5B621CD;\n\tWed, 21 Sep 2022 01:02:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66D7761F7D\n\tfor ;\n\tWed, 21 Sep 2022 01:02:35 +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 DAC0E415;\n\tWed, 21 Sep 2022 01:02:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663714956;\n\tbh=gpQxKF+n7SH1s9MRbEIauOskd0AG5lSMhg7+h3uttjs=;\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=tS2O0NqdGCEdLpqpTaCNbsRFYsuFQCFqiUvAJTq6uCl0P/fc5PIErc5dlnA+E38cj\n\tgpwwKRh44L/rE5HvHJJOjNZ1KnMYUsHQBVgtNcQRAZr2lWfIAzDEiQXS0ObVnG//6Y\n\tN210imuzARzoO/wL/gN3Qf9mwzQt+m/X2KNJXwTGMkzPJ7v6g2St/NDmf12Q4mqbSF\n\taMoyQgsz2s5X5ml8yYIHD8Y05s4qu7SXZLbRhn5Pe9CWCRlO2sYNGOf+MczYSpoJ99\n\tP+8wacUPEgkkjGNesMgptW6dmo3+c9RO2KnDQ1CnzFQ5IN8zSRIWgf9SAYiMm+H3ha\n\tbwXJ4EvM/JJ2Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663714955;\n\tbh=gpQxKF+n7SH1s9MRbEIauOskd0AG5lSMhg7+h3uttjs=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=R4MexuQlsu9XqcwtFiPUWFbNhdFH0F19rnqSfCUUX655UFROw9EY0HXSCb/YL9Mq0\n\tDkOzp/VsYeOhU4oo0mpoxjJeLzyZoDKyGWili1LLFTEvpeex7pc9p4yJDbZa5//oSO\n\toYNyjWPxSs1bqa8qU3H8sJFPMBF0lXLzYw4g8c6k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"R4MexuQl\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-22-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-22-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 21 Sep 2022 00:02:32 +0100","Message-ID":"<166371495206.18961.1392520108897231032@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: 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":25067,"web_url":"https://patchwork.libcamera.org/comment/25067/","msgid":"<20220921203640.wi4ub2laxln5aixy@uno.localdomain>","date":"2022-09-21T20:36:40","subject":"Re: [libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: 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":"Hi Laurent,\n\nOn Thu, Sep 08, 2022 at 04:41:49AM +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\nThis one is easier indeed\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/ipa/rkisp1/algorithms/cproc.cpp | 51 ++++++++++++++++++-----------\n> src/ipa/rkisp1/ipa_context.cpp | 21 ++++++++++--\n> src/ipa/rkisp1/ipa_context.h | 8 ++++-\n> 3 files changed, 56 insertions(+), 24 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> index 22a70e0b70c7..9c442cd56b3f 100644\n> --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> @@ -38,55 +38,66 @@ LOG_DEFINE_CATEGORY(RkISP1CProc)\n> */\n> void ColorProcessing::queueRequest(IPAContext &context,\n> \t\t\t\t [[maybe_unused]] const uint32_t frame,\n> -\t\t\t\t [[maybe_unused]] RkISP1FrameContext &frameContext,\n> +\t\t\t\t RkISP1FrameContext &frameContext,\n> \t\t\t\t const ControlList &controls)\n> {\n> \tauto &cproc = context.activeState.cproc;\n> +\tbool update = false;\n>\n> \tconst auto &brightness = controls.get(controls::Brightness);\n> \tif (brightness) {\n> -\t\tcproc.brightness = std::clamp(std::lround(*brightness * 128), -128, 127);\n> -\t\tcproc.updateParams = true;\n> +\t\tint value = std::clamp(std::lround(*brightness * 128), -128, 127);\n> +\t\tif (cproc.brightness != value) {\n> +\t\t\tcproc.brightness = value;\n> +\t\t\tupdate = true;\n> +\t\t}\n>\n> -\t\tLOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> +\t\tLOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> \t}\n>\n> \tconst auto &contrast = controls.get(controls::Contrast);\n> \tif (contrast) {\n> -\t\tcproc.contrast = std::clamp(std::lround(*contrast * 128), 0, 255);\n> -\t\tcproc.updateParams = true;\n> +\t\tint value = std::clamp(std::lround(*contrast * 128), 0, 255);\n> +\t\tif (cproc.contrast != value) {\n> +\t\t\tcproc.contrast = value;\n> +\t\t\tupdate = true;\n> +\t\t}\n>\n> -\t\tLOG(RkISP1CProc, Debug) << \"Set contrast to \" << *contrast;\n> +\t\tLOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> \t}\n>\n> \tconst auto saturation = controls.get(controls::Saturation);\n> \tif (saturation) {\n> -\t\tcproc.saturation = std::clamp(std::lround(*saturation * 128), 0, 255);\n> -\t\tcproc.updateParams = true;\n> +\t\tint value = std::clamp(std::lround(*saturation * 128), 0, 255);\n> +\t\tif (cproc.saturation != value) {\n> +\t\t\tcproc.saturation = value;\n> +\t\t\tupdate = true;\n> +\t\t}\n>\n> -\t\tLOG(RkISP1CProc, Debug) << \"Set saturation to \" << *saturation;\n> +\t\tLOG(RkISP1CProc, Debug) << \"Set saturation to \" << value;\n> \t}\n> +\n> +\tframeContext.cproc.brightness = cproc.brightness;\n> +\tframeContext.cproc.contrast = cproc.contrast;\n> +\tframeContext.cproc.saturation = cproc.saturation;\n> +\tframeContext.cproc.update = update;\n> }\n>\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> -void ColorProcessing::prepare(IPAContext &context,\n> +void ColorProcessing::prepare([[maybe_unused]] 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 rkisp1_params_cfg *params)\n> {\n> -\tauto &cproc = context.activeState.cproc;\n> -\n> \t/* Check if the algorithm configuration has been updated. */\n> -\tif (!cproc.updateParams)\n> +\tif (!frameContext.cproc.update)\n> \t\treturn;\n>\n> -\tcproc.updateParams = false;\n> -\n> -\tparams->others.cproc_config.brightness = cproc.brightness;\n> -\tparams->others.cproc_config.contrast = cproc.contrast;\n> -\tparams->others.cproc_config.sat = cproc.saturation;\n> +\tparams->others.cproc_config.brightness = frameContext.cproc.brightness;\n> +\tparams->others.cproc_config.contrast = frameContext.cproc.contrast;\n> +\tparams->others.cproc_config.sat = frameContext.cproc.saturation;\n>\n> \tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_CPROC;\n> \tparams->module_ens |= RKISP1_CIF_ISP_MODULE_CPROC;\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index cd1443e1ed46..936b77709417 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -160,9 +160,6 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * \\var IPAActiveState::cproc.saturation\n> * \\brief Saturation level\n> - *\n> - * \\var IPAActiveState::cproc.updateParams\n> - * \\brief Indicates if ISP parameters need to be updated\n> */\n>\n> /**\n> @@ -236,6 +233,24 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Whether the Auto White Balance algorithm is enabled\n> */\n>\n> +/**\n> + * \\var RkISP1FrameContext::cproc\n> + * \\brief Color Processing parameters for this frame\n> + *\n> + * \\struct RkISP1FrameContext::cproc.brightness\n> + * \\brief Brightness level\n> + *\n> + * \\var RkISP1FrameContext::cproc.contrast\n> + * \\brief Contrast level\n> + *\n> + * \\var RkISP1FrameContext::cproc.saturation\n> + * \\brief Saturation level\n> + *\n> + * \\var RkISP1FrameContext::cproc.update\n> + * \\brief Indicates if the color processing parameters have been updated\n> + * compared to the 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 d97aae9a97b4..78edb607d039 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -75,7 +75,6 @@ struct IPAActiveState {\n> \t\tint8_t brightness;\n> \t\tuint8_t contrast;\n> \t\tuint8_t saturation;\n> -\t\tbool updateParams;\n> \t} cproc;\n>\n> \tstruct {\n> @@ -107,6 +106,13 @@ struct RkISP1FrameContext : public FrameContext {\n> \t\tbool autoEnabled;\n> \t} awb;\n>\n> +\tstruct {\n> +\t\tint8_t brightness;\n> +\t\tuint8_t contrast;\n> +\t\tuint8_t saturation;\n> +\t\tbool update;\n> +\t} cproc;\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 6A07FC0DA4\n\tfor ;\n\tWed, 21 Sep 2022 20:36:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B729B621F8;\n\tWed, 21 Sep 2022 22:36:43 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74A18600AA\n\tfor ;\n\tWed, 21 Sep 2022 22:36:42 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id AE08E1C0003;\n\tWed, 21 Sep 2022 20:36:41 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663792603;\n\tbh=d83oAfuhVcIrgBPqdvInWQkvyTBPZ9KEXohtBkNljtc=;\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=mTT4WX2wnSHsgX16KHI3hWIastwO6yV2yFCKDOMk6+ubeuFPRHLdwV/CcP9WylU0U\n\tnFdUYECfJ/0+qQzfZsWkM25rGuVQjU58JQtj7k/+I15NkNMGzkYvE7sLRsxbp41+vz\n\tA+uis97FBNJYV7Xuzjx8txC9VJRISqj2AWgR1geWAGpNlOERCz5FS2lhlsQURH9JIa\n\tJP5C/O4uUBXrJMhxpxB25vFwu6XC6aXFVkNq4guQh7PyjcBCAj0rKBYeuapsH0OD8G\n\trE5dCAnF1OqIG4ozXR5mmqwV/jAWZKJ1343UE95Cbb0pOKEK+huO+d8yDW5xJ3U/R6\n\t+lzSWPMzAcz1g==","Date":"Wed, 21 Sep 2022 22:36:40 +0200","To":"Laurent Pinchart ","Message-ID":"<20220921203640.wi4ub2laxln5aixy@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-22-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220908014200.28728-22-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: 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\" "}},{"id":25094,"web_url":"https://patchwork.libcamera.org/comment/25094/","msgid":"","date":"2022-09-22T20:32:31","subject":"Re: [libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: Store\n\tper-frame information in frame context","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Sep 21, 2022 at 12:02:32AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:49)\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/cproc.cpp | 51 ++++++++++++++++++-----------\n> > src/ipa/rkisp1/ipa_context.cpp | 21 ++++++++++--\n> > src/ipa/rkisp1/ipa_context.h | 8 ++++-\n> > 3 files changed, 56 insertions(+), 24 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > index 22a70e0b70c7..9c442cd56b3f 100644\n> > --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > @@ -38,55 +38,66 @@ LOG_DEFINE_CATEGORY(RkISP1CProc)\n> > */\n> > void ColorProcessing::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 &cproc = context.activeState.cproc;\n> > + bool update = false;\n> > \n> > const auto &brightness = controls.get(controls::Brightness);\n> > if (brightness) {\n> > - cproc.brightness = std::clamp(std::lround(*brightness * 128), -128, 127);\n> > - cproc.updateParams = true;\n> > + int value = std::clamp(std::lround(*brightness * 128), -128, 127);\n> > + if (cproc.brightness != value) {\n> > + cproc.brightness = value;\n> > + update = true;\n> > + }\n> > \n> > - LOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> > + LOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> > }\n> > \n> > const auto &contrast = controls.get(controls::Contrast);\n> > if (contrast) {\n> > - cproc.contrast = std::clamp(std::lround(*contrast * 128), 0, 255);\n> > - cproc.updateParams = true;\n> > + int value = std::clamp(std::lround(*contrast * 128), 0, 255);\n> > + if (cproc.contrast != value) {\n> > + cproc.contrast = value;\n> > + update = true;\n> > + }\n> > \n> > - LOG(RkISP1CProc, Debug) << \"Set contrast to \" << *contrast;\n> > + LOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> > }\n> > \n> > const auto saturation = controls.get(controls::Saturation);\n> > if (saturation) {\n> > - cproc.saturation = std::clamp(std::lround(*saturation * 128), 0, 255);\n> > - cproc.updateParams = true;\n> > + int value = std::clamp(std::lround(*saturation * 128), 0, 255);\n> > + if (cproc.saturation != value) {\n> > + cproc.saturation = value;\n> > + update = true;\n> > + }\n> > \n> > - LOG(RkISP1CProc, Debug) << \"Set saturation to \" << *saturation;\n> > + LOG(RkISP1CProc, Debug) << \"Set saturation to \" << value;\n> > }\n> > +\n> > + frameContext.cproc.brightness = cproc.brightness;\n> > + frameContext.cproc.contrast = cproc.contrast;\n> > + frameContext.cproc.saturation = cproc.saturation;\n> > + frameContext.cproc.update = update;\n> \n> I think this all sounds right.\n> \n> I wonder what common patterns will emerge from these conversions ...\n> \n> > }\n> > \n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > */\n> > -void ColorProcessing::prepare(IPAContext &context,\n> > +void ColorProcessing::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 &cproc = context.activeState.cproc;\n> > -\n> > /* Check if the algorithm configuration has been updated. */\n> > - if (!cproc.updateParams)\n> > + if (!frameContext.cproc.update)\n> > return;\n> \n> Yes, this now responds to the frame context for the correct frame.\n> \n> > \n> > - cproc.updateParams = false;\n> > -\n> > - params->others.cproc_config.brightness = cproc.brightness;\n> > - params->others.cproc_config.contrast = cproc.contrast;\n> > - params->others.cproc_config.sat = cproc.saturation;\n> > + params->others.cproc_config.brightness = frameContext.cproc.brightness;\n> > + params->others.cproc_config.contrast = frameContext.cproc.contrast;\n> > + params->others.cproc_config.sat = frameContext.cproc.saturation;\n> > \n> > params->module_en_update |= RKISP1_CIF_ISP_MODULE_CPROC;\n> > params->module_ens |= RKISP1_CIF_ISP_MODULE_CPROC;\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index cd1443e1ed46..936b77709417 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -160,9 +160,6 @@ namespace libcamera::ipa::rkisp1 {\n> > *\n> > * \\var IPAActiveState::cproc.saturation\n> > * \\brief Saturation level\n> > - *\n> > - * \\var IPAActiveState::cproc.updateParams\n> > - * \\brief Indicates if ISP parameters need to be updated\n> > */\n> > \n> > /**\n> > @@ -236,6 +233,24 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\brief Whether the Auto White Balance algorithm is enabled\n> > */\n> > \n> > +/**\n> > + * \\var RkISP1FrameContext::cproc\n> > + * \\brief Color Processing parameters for this frame\n> > + *\n> > + * \\struct RkISP1FrameContext::cproc.brightness\n> > + * \\brief Brightness level\n> > + *\n> > + * \\var RkISP1FrameContext::cproc.contrast\n> > + * \\brief Contrast level\n> > + *\n> > + * \\var RkISP1FrameContext::cproc.saturation\n> > + * \\brief Saturation level\n> > + *\n> > + * \\var RkISP1FrameContext::cproc.update\n> > + * \\brief Indicates if the color processing parameters have been updated\n> > + * compared to the 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 d97aae9a97b4..78edb607d039 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -75,7 +75,6 @@ struct IPAActiveState {\n> > int8_t brightness;\n> > uint8_t contrast;\n> > uint8_t saturation;\n> > - bool updateParams;\n> > } cproc;\n> > \n> > struct {\n> > @@ -107,6 +106,13 @@ struct RkISP1FrameContext : public FrameContext {\n> > bool autoEnabled;\n> > } awb;\n> > \n> > + struct {\n> > + int8_t brightness;\n> > + uint8_t contrast;\n> > + uint8_t saturation;\n> > + bool update;\n> > + } cproc;\n> \n> And I wonder if the common pattern will be lots of anonymous structures\n> that are in both the active state, and the frame context, while the\n> frame context has an 'updated' flag...\n\nIt seems so. In v5 there will be a few code blocks along the lines of\n\n\tunsigned int value = std::round(std::clamp(*sharpness, 0.0f, 10.0f));\n\n\tif (filter.sharpness != value) {\n\t\tfilter.sharpness = value;\n\t\tupdate = true;\n\t}\n\nor\n\n\tif (filter.denoise != 1) {\n\t\tfilter.denoise = 1;\n\t\tupdate = true;\n\t}\n\nIt would be nice to simplify that, but I'm not sure how yet. It think we\nneed more algorithms converted, in different IPA modules, before seeing\nhow to best refactor the code.\n\n> Maybe we can optimise this later - but I think this gets us moving\n> anyway to explore what it will become.\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\n> > struct {\n> > uint32_t exposure;\n> > double gain;","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 2698CC0DA4\n\tfor ;\n\tThu, 22 Sep 2022 20:32:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FE896221A;\n\tThu, 22 Sep 2022 22:32:48 +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 15F6B6219A\n\tfor ;\n\tThu, 22 Sep 2022 22:32:47 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 56F766BE;\n\tThu, 22 Sep 2022 22:32:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663878768;\n\tbh=a01tQYkns6jUJhd0GmPT2FEHO1MNUvzZS7hKYlPzbcw=;\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=fuKObl6DhqZSKA57UNcBJkxXM4Sm5Sq58bRg9FgTkgksu4aX0iHAG9+bIfw4gQPpU\n\tp3rni23/AxfwM4n2ydnTvQCdjtCxz6rU91aAKSseuy/kV2oqjRcSd2LFY3BqLXkGDr\n\tqsMWHLNekguSBuFTx/xKV3jZCaRBwm+dHGhk1l1vWX5sbU+YkANkroX8yvGcVz3b2d\n\t1GjAsBUbmN4+zjlUz/ca83b7/Indoq6qAcd0QET0qvXV6vgRZlvCYEJnKUDEwori/2\n\tYNu9llQyBPyZJIRDWgeXt99Kqr0YcT9RDrgAh1zBpaI+9jucQvGkgB3D7xscptuQ29\n\tE5/iZtgHk7DGw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663878766;\n\tbh=a01tQYkns6jUJhd0GmPT2FEHO1MNUvzZS7hKYlPzbcw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q4ho3UkdtJVgUKMn7wm0r0J/jZrXz6mywHJQR39st9COqRQR6SkXqzsNDO7c7CZK7\n\t52fMn0HwtfB6t6HlSDHrptFnDZsuS/grflnB4umQ8QXYib3FQCv4xeFTd3IK6QMuYY\n\t8dyRRKvrdlzJQH8mJTVMR1BthsCK3Kck3katC2U8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Q4ho3Ukd\"; dkim-atps=neutral","Date":"Thu, 22 Sep 2022 23:32:31 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-22-laurent.pinchart@ideasonboard.com>\n\t<166371495206.18961.1392520108897231032@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166371495206.18961.1392520108897231032@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]