[{"id":25532,"web_url":"https://patchwork.libcamera.org/comment/25532/","msgid":"<20221024075440.GB3874866@pyrite.rasen.tech>","date":"2022-10-24T07:54:40","subject":"Re: [libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support\n\traw capture","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Mon, Oct 24, 2022 at 03:03:48AM +0300, Laurent Pinchart wrote:\n> Support raw capture by allowing manual control of the exposure time and\n> analogue gain.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 51 ++++++++++++++++++++-----------\n> src/ipa/rkisp1/algorithms/agc.h | 2 ++\n> 2 files changed, 36 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 377a3e8a0efd..973965babed2 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -62,6 +62,7 @@ static constexpr double kRelativeLuminanceTarget = 0.4;\n> Agc::Agc()\n> \t: frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s)\n> {\n> +\tsupportsRaw_ = true;\n> }\n> \n> /**\n> @@ -81,7 +82,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t\t10ms / context.configuration.sensor.lineDuration;\n> \tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> -\tcontext.activeState.agc.autoEnabled = true;\n> +\tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n> \n> \t/*\n> \t * According to the RkISP1 documentation:\n> @@ -123,12 +124,14 @@ void Agc::queueRequest(IPAContext &context,\n> {\n> \tauto &agc = context.activeState.agc;\n> \n> -\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> -\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> -\t\tagc.autoEnabled = *agcEnable;\n> +\tif (!context.configuration.raw) {\n> +\t\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> +\t\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> +\t\t\tagc.autoEnabled = *agcEnable;\n> \n> -\t\tLOG(RkISP1Agc, Debug)\n> -\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> +\t\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> +\t\t}\n> \t}\n> \n> \tconst auto &exposure = controls.get(controls::ExposureTime);\n> @@ -160,6 +163,9 @@ void Agc::queueRequest(IPAContext &context,\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> +\tif (!params)\n> +\t\treturn;\n> +\n> \tif (frameContext.agc.autoEnabled) {\n> \t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> @@ -367,6 +373,22 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> \treturn histogram.interQuantileMean(0.98, 1.0);\n> }\n> \n> +void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> +\t\t ControlList &metadata)\n> +{\n> +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> +\t\t\t\t * frameContext.sensor.exposure;\n> +\tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> +\tmetadata.set(controls::ExposureTime, exposureTime.get());\n> +\n> +\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> +\tuint32_t vTotal = context.configuration.sensor.size.height\n> +\t\t\t+ context.configuration.sensor.defVBlank;\n> +\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> +\t\t\t\t * vTotal;\n> +\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> +}\n> +\n> /**\n> * \\brief Process RkISP1 statistics, and run AGC operations\n> * \\param[in] context The shared IPA context\n> @@ -382,6 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats,\n> \t\t ControlList &metadata)\n> {\n> +\tif (!stats) {\n> +\t\tfillMetadata(context, frameContext, metadata);\n> +\t\treturn;\n> +\t}\n> +\n> \t/*\n> \t * \\todo Verify that the exposure and gain applied by the sensor for\n> \t * this frame match what has been requested. This isn't a hard\n> @@ -424,17 +451,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> \n> -\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> -\t\t\t\t * frameContext.sensor.exposure;\n> -\tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> -\tmetadata.set(controls::ExposureTime, exposureTime.get());\n> -\n> -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> -\tuint32_t vTotal = context.configuration.sensor.size.height\n> -\t\t\t+ context.configuration.sensor.defVBlank;\n> -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> -\t\t\t\t * vTotal;\n> -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> +\tfillMetadata(context, frameContext, metadata);\n> }\n> \n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index a228d0c37768..8a22263741b6 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -44,6 +44,8 @@ private:\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> \tdouble estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> \tdouble measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> +\tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> +\t\t\t ControlList &metadata);\n> \n> \tuint64_t frameCount_;\n> \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 00269BDB16\n\tfor ;\n\tMon, 24 Oct 2022 07:54:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51C0962EF7;\n\tMon, 24 Oct 2022 09:54:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D4F162EBC\n\tfor ;\n\tMon, 24 Oct 2022 09:54:48 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90D73471;\n\tMon, 24 Oct 2022 09:54:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666598090;\n\tbh=Jbc6AEjYOQJa0TB1FqwpyGPKeigVQAJT+m8/hnCMlvc=;\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=tJ9DfIjeQ3CuX46jJgqH3XxJ7KbK5gUrZHoKtIXx5DsbJWnxzUEmititsQdR2+xcA\n\t1mFUKHi4IsdEGagQRbYQQu/ExxoE+QY27WlwBT+orNZRFqq8Rp+8VR+mf8VXbq66RI\n\tcwnovc3NIcX41pkHYblN+AYAlpU+2Y2DWZQOOPFf9e1s7cfiqLeQiSpFr0EFsEaRLH\n\tbycYaCRUOMFEaBgYN1e+DmjI3/r6qgsI3ANu9FRCVpGTayatAMI0xH0pkh3tlpGitF\n\t8BX6nTGl++nCNm2JSQCZhKCXphUj/ubRPSVHM3m84LMYNTOnOH8On0hs3R6inqem+E\n\tlvdh5EQLpQf6g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666598088;\n\tbh=Jbc6AEjYOQJa0TB1FqwpyGPKeigVQAJT+m8/hnCMlvc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KZ8u9d0sZhA7SQC5EL0XYTBUhaNkmvq9WlCcMaAxr24hU9fWwqkxwR1AHpRZt4dUa\n\t/wgw/A+X7KIjlRcx8nWzNjU+H+w45AhBSnyvtsJ6zDL57pjFD1PGls1XTbLHFFDd59\n\tB55hBS58bTC/Nc8SI8+/43vf0ALNdinhSDwIVLfs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KZ8u9d0s\"; dkim-atps=neutral","Date":"Mon, 24 Oct 2022 16:54:40 +0900","To":"Laurent Pinchart ","Message-ID":"<20221024075440.GB3874866@pyrite.rasen.tech>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-6-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support\n\traw capture","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25601,"web_url":"https://patchwork.libcamera.org/comment/25601/","msgid":"<20221026145230.ba7ny3fzci3lmzec@uno.localdomain>","date":"2022-10-26T14:52:30","subject":"Re: [libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support\n\traw capture","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Oct 24, 2022 at 03:03:48AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Support raw capture by allowing manual control of the exposure time and\n> analogue gain.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 51 ++++++++++++++++++++-----------\n> src/ipa/rkisp1/algorithms/agc.h | 2 ++\n> 2 files changed, 36 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 377a3e8a0efd..973965babed2 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -62,6 +62,7 @@ static constexpr double kRelativeLuminanceTarget = 0.4;\n> Agc::Agc()\n> \t: frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s)\n> {\n> +\tsupportsRaw_ = true;\n> }\n>\n> /**\n> @@ -81,7 +82,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t\t10ms / context.configuration.sensor.lineDuration;\n> \tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> -\tcontext.activeState.agc.autoEnabled = true;\n> +\tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n>\n> \t/*\n> \t * According to the RkISP1 documentation:\n> @@ -123,12 +124,14 @@ void Agc::queueRequest(IPAContext &context,\n> {\n> \tauto &agc = context.activeState.agc;\n>\n> -\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> -\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> -\t\tagc.autoEnabled = *agcEnable;\n> +\tif (!context.configuration.raw) {\n\nI wonder if we shouldn't complain loud when the application tries to\nenable AutoExposure with a RAW configuration..\n\n> +\t\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> +\t\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> +\t\t\tagc.autoEnabled = *agcEnable;\n>\n> -\t\tLOG(RkISP1Agc, Debug)\n> -\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> +\t\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> +\t\t}\n> \t}\n>\n> \tconst auto &exposure = controls.get(controls::ExposureTime);\n> @@ -160,6 +163,9 @@ void Agc::queueRequest(IPAContext &context,\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> +\tif (!params)\n> +\t\treturn;\n> +\n\nI might have missed where params is set to nullptr in the IPA module.\n\n\nvoid IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n{\n\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n\trkisp1_params_cfg *params =\n\t\treinterpret_cast(\n\t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n\n\t/* Prepare parameters buffer. */\n\tmemset(params, 0, sizeof(*params));\n\n\tfor (auto const &algo : algorithms())\n\t\talgo->prepare(context_, frame, frameContext, params);\n\nDoes 'params' need to be treated as 'stats' in\nIPARkISP1::processStatsBuffer() (ie set to nullptr if\nconfiguration.raw) ?\n\n\n> \tif (frameContext.agc.autoEnabled) {\n> \t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> @@ -367,6 +373,22 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> \treturn histogram.interQuantileMean(0.98, 1.0);\n> }\n>\n> +void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> +\t\t ControlList &metadata)\n> +{\n> +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> +\t\t\t\t * frameContext.sensor.exposure;\n> +\tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> +\tmetadata.set(controls::ExposureTime, exposureTime.get());\n> +\n> +\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> +\tuint32_t vTotal = context.configuration.sensor.size.height\n> +\t\t\t+ context.configuration.sensor.defVBlank;\n> +\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> +\t\t\t\t * vTotal;\n> +\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> +}\n> +\n> /**\n> * \\brief Process RkISP1 statistics, and run AGC operations\n> * \\param[in] context The shared IPA context\n> @@ -382,6 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats,\n> \t\t ControlList &metadata)\n> {\n> +\tif (!stats) {\n> +\t\tfillMetadata(context, frameContext, metadata);\n> +\t\treturn;\n> +\t}\n> +\n> \t/*\n> \t * \\todo Verify that the exposure and gain applied by the sensor for\n> \t * this frame match what has been requested. This isn't a hard\n> @@ -424,17 +451,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n>\n> -\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> -\t\t\t\t * frameContext.sensor.exposure;\n> -\tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> -\tmetadata.set(controls::ExposureTime, exposureTime.get());\n> -\n> -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> -\tuint32_t vTotal = context.configuration.sensor.size.height\n> -\t\t\t+ context.configuration.sensor.defVBlank;\n> -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> -\t\t\t\t * vTotal;\n> -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> +\tfillMetadata(context, frameContext, metadata);\n> }\n>\n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index a228d0c37768..8a22263741b6 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -44,6 +44,8 @@ private:\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> \tdouble estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> \tdouble measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> +\tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> +\t\t\t ControlList &metadata);\n>\n> \tuint64_t frameCount_;\n>\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 B1C1ABD16B\n\tfor ;\n\tWed, 26 Oct 2022 14:52:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F7F761F4B;\n\tWed, 26 Oct 2022 16:52:34 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F03D61F4B\n\tfor ;\n\tWed, 26 Oct 2022 16:52:32 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 15B7D20003;\n\tWed, 26 Oct 2022 14:52:31 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666795954;\n\tbh=oPHsfaNY8a1eCb+X7w4osNQbrqo0WabOR0wUCyGkNzk=;\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=cieRI5He1Rb+svYLWFRMzbPg3+HTbkhaWlCh1PB7XEgSaAn74INqNkS1DY+kFgJHd\n\tdVi6iYUzV8jZkc0c2IzWCUnGQaffz1k3QpsovGfPCGlDzerHxfzPLeuhTHaXXsqGWg\n\tyV3UmMmLKmwW3smeb0pCJ8qJx2lqR3QthdFTSpGh4GmUCeliblelBxhkmOgfnI0k+7\n\t6oZi/LotlqgwfC59T88GYx4ytxn6+OikcF6KhVJ6YCF2bCoGiHjWIuiki5TXJG9rxX\n\tGIrzfie5bpbPHOgYGk6g+vOj9uPBIZo5pilcm1+AVD+PYsAXgwXQe5QjJ4jcmowROw\n\tVfzVv3Od+sH+A==","Date":"Wed, 26 Oct 2022 16:52:30 +0200","To":"Laurent Pinchart ","Message-ID":"<20221026145230.ba7ny3fzci3lmzec@uno.localdomain>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-6-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support\n\traw capture","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":25684,"web_url":"https://patchwork.libcamera.org/comment/25684/","msgid":"","date":"2022-10-30T17:17:27","subject":"Re: [libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support\n\traw capture","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Oct 26, 2022 at 04:52:30PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 24, 2022 at 03:03:48AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > Support raw capture by allowing manual control of the exposure time and\n> > analogue gain.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 51 ++++++++++++++++++++-----------\n> > src/ipa/rkisp1/algorithms/agc.h | 2 ++\n> > 2 files changed, 36 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 377a3e8a0efd..973965babed2 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -62,6 +62,7 @@ static constexpr double kRelativeLuminanceTarget = 0.4;\n> > Agc::Agc()\n> > \t: frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s)\n> > {\n> > +\tsupportsRaw_ = true;\n> > }\n> >\n> > /**\n> > @@ -81,7 +82,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \t\t10ms / context.configuration.sensor.lineDuration;\n> > \tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > -\tcontext.activeState.agc.autoEnabled = true;\n> > +\tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n> >\n> > \t/*\n> > \t * According to the RkISP1 documentation:\n> > @@ -123,12 +124,14 @@ void Agc::queueRequest(IPAContext &context,\n> > {\n> > \tauto &agc = context.activeState.agc;\n> >\n> > -\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> > -\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> > -\t\tagc.autoEnabled = *agcEnable;\n> > +\tif (!context.configuration.raw) {\n> \n> I wonder if we shouldn't complain loud when the application tries to\n> enable AutoExposure with a RAW configuration..\n\nWhy should we complain more loudly than for any other invalid\ncombination of control values ?\n\n> > +\t\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> > +\t\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> > +\t\t\tagc.autoEnabled = *agcEnable;\n> >\n> > -\t\tLOG(RkISP1Agc, Debug)\n> > -\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> > +\t\t\tLOG(RkISP1Agc, Debug)\n> > +\t\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> > +\t\t}\n> > \t}\n> >\n> > \tconst auto &exposure = controls.get(controls::ExposureTime);\n> > @@ -160,6 +163,9 @@ void Agc::queueRequest(IPAContext &context,\n> > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> > {\n> > +\tif (!params)\n> > +\t\treturn;\n> > +\n> \n> I might have missed where params is set to nullptr in the IPA module.\n> \n> \n> void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> {\n> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \trkisp1_params_cfg *params =\n> \t\treinterpret_cast(\n> \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n> \n> \t/* Prepare parameters buffer. */\n> \tmemset(params, 0, sizeof(*params));\n> \n> \tfor (auto const &algo : algorithms())\n> \t\talgo->prepare(context_, frame, frameContext, params);\n> \n> Does 'params' need to be treated as 'stats' in\n> IPARkISP1::processStatsBuffer() (ie set to nullptr if\n> configuration.raw) ?\n\nNo, you're right, it can't be null, so I'll drop this check.\n\n> > \tif (frameContext.agc.autoEnabled) {\n> > \t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> > \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > @@ -367,6 +373,22 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> > \treturn histogram.interQuantileMean(0.98, 1.0);\n> > }\n> >\n> > +void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > +\t\t ControlList &metadata)\n> > +{\n> > +\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> > +\t\t\t\t * frameContext.sensor.exposure;\n> > +\tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > +\tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > +\n> > +\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > +\tuint32_t vTotal = context.configuration.sensor.size.height\n> > +\t\t\t+ context.configuration.sensor.defVBlank;\n> > +\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > +\t\t\t\t * vTotal;\n> > +\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > +}\n> > +\n> > /**\n> > * \\brief Process RkISP1 statistics, and run AGC operations\n> > * \\param[in] context The shared IPA context\n> > @@ -382,6 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats,\n> > \t\t ControlList &metadata)\n> > {\n> > +\tif (!stats) {\n> > +\t\tfillMetadata(context, frameContext, metadata);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > \t/*\n> > \t * \\todo Verify that the exposure and gain applied by the sensor for\n> > \t * this frame match what has been requested. This isn't a hard\n> > @@ -424,17 +451,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> > \tframeCount_++;\n> >\n> > -\tutils::Duration exposureTime = context.configuration.sensor.lineDuration\n> > -\t\t\t\t * frameContext.sensor.exposure;\n> > -\tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > -\tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > -\n> > -\t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > -\tuint32_t vTotal = context.configuration.sensor.size.height\n> > -\t\t\t+ context.configuration.sensor.defVBlank;\n> > -\tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > -\t\t\t\t * vTotal;\n> > -\tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > +\tfillMetadata(context, frameContext, metadata);\n> > }\n> >\n> > REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index a228d0c37768..8a22263741b6 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -44,6 +44,8 @@ private:\n> > \tutils::Duration filterExposure(utils::Duration exposureValue);\n> > \tdouble estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> > \tdouble measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> > +\tvoid fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > +\t\t\t ControlList &metadata);\n> >\n> > \tuint64_t frameCount_;\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 1550DBD16B\n\tfor ;\n\tSun, 30 Oct 2022 17:17:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B5BF63022;\n\tSun, 30 Oct 2022 18:17:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D97661F47\n\tfor ;\n\tSun, 30 Oct 2022 18:17:50 +0100 (CET)","from pendragon.ideasonboard.com (85-76-1-64-nat.elisa-mobile.fi\n\t[85.76.1.64])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B3537C5;\n\tSun, 30 Oct 2022 18:17:49 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667150272;\n\tbh=A+A8piWNHwNNH/b2JYpsvHv9iowEjfuxHMa08Py7U5g=;\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=uTxMRTxE0ydBa1HLAMNwruV5iVig/vE4wuCtlB7OAuqzLonFbRN8NtwYFI9Vkv4xv\n\tgTx6i+EZ7E3a+X03w9+RpPPPxRRCM1H9kGbWrHNDiFVfug4nAhdv2Oa0vcQfcMI1Xb\n\tT1iYw0wwugRFEJTJVC/XlqT3zj25zfpK9OvJqag7dYAi5l+2lM1Lnkojr5cxIbtoWl\n\tepPfK8JoR+Yxc6A1NOvMAj/KvJKXWZ54pSakpvUeHGpEVDSFUnvOHRvwNBST2mRuA+\n\t28JNBX8PareGaZ7XHRED289AEqYwiTpb2phn5+q+MLx1rGzOtowQEoRY25oICgAuUE\n\tB3QIEK85UV6eg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667150269;\n\tbh=A+A8piWNHwNNH/b2JYpsvHv9iowEjfuxHMa08Py7U5g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ip1UBjyqsNVi4SM/4oXIj3dRuvfyXknFUQGpC7Xlgj+OYLjmemcQnfEVgU7r8ONCr\n\t36PgfcsnLdKEdCAnqkUn1oe/zO+/HlWiz2GGtq5Yx+to7Jy20h7xe5JdTBpEv9u5xV\n\tTSaH/4EwE3DiW9tYtJNW3LW3Xxn45FSbgdf40RjQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ip1UBjyq\"; dkim-atps=neutral","Date":"Sun, 30 Oct 2022 19:17:27 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-6-laurent.pinchart@ideasonboard.com>\n\t<20221026145230.ba7ny3fzci3lmzec@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221026145230.ba7ny3fzci3lmzec@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support\n\traw capture","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\" "}}]