[{"id":31573,"web_url":"https://patchwork.libcamera.org/comment/31573/","msgid":"<20241003170558.GC32537@pendragon.ideasonboard.com>","date":"2024-10-03T17:05:58","subject":"Re: [PATCH v1 5/8] ipa: rkisp1: Add debug metadata support to the\n\trkisp1","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Wed, Oct 02, 2024 at 06:19:23PM +0200, Stefan Klug wrote:\n> Add a DebugMetadata helper to the context and add the corresponding\n> plumbing. This is all that is needed to support debug metadata in an\n> IPA.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/ipa_context.h | 5 +++++\n> src/ipa/rkisp1/rkisp1.cpp | 4 ++++\n> 2 files changed, 9 insertions(+)\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index d52e73ad2503..7b93a9e9461d 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -17,8 +17,11 @@\n> #include \n> #include \n> #include \n> +\n> #include \n> \n> +#include \"libcamera/internal/debug_controls.h\"\n> +\n> #include \n> #include \n> #include \n> @@ -194,6 +197,8 @@ struct IPAContext {\n> \n> \tControlInfoMap::Map ctrlMap;\n> \n> +\tDebugMetadata debugMetadata;\n> +\n> \t/* Interface to the Camera Helper */\n> \tstd::unique_ptr camHelper;\n> };\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index a579f21de56f..5b1ef0c372c6 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -117,6 +117,7 @@ const IPAHwSettings ipaHwSettingsV12{\n> const ControlInfoMap::Map rkisp1Controls{\n> \t{ &controls::AwbEnable, ControlInfo(false, true) },\n> \t{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n> +\t{ &controls::DebugMetadataEnable, ControlInfo(false, true, false) },\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> \t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> };\n> @@ -326,6 +327,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \tIPAFrameContext &frameContext = context_.frameContexts.alloc(frame);\n> +\tcontext_.debugMetadata.checkForEnable(controls);\n> \n> \tfor (auto const &a : algorithms()) {\n> \t\tAlgorithm *algo = static_cast(a.get());\n> @@ -368,6 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> \t\tcontext_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> \n> \tControlList metadata(controls::controls);\n> +\tcontext_.debugMetadata.assignControlList(&metadata);\n\nWhat's the advantage of doing this here, instead of...\n\n> \n> \tfor (auto const &a : algorithms()) {\n> \t\tAlgorithm *algo = static_cast(a.get());\n> @@ -377,6 +380,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> \t}\n> \n> \tsetControls(frame);\n> +\tcontext_.debugMetadata.assignControlList(nullptr);\n\n... extracting the control list from debugMetadata here and merging it\ninto metadata ?\n\n> \tmetadataReady.emit(frame, metadata);\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 294B2BD80A\n\tfor ;\n\tThu, 3 Oct 2024 17:06:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0007C6351F;\n\tThu, 3 Oct 2024 19:06:03 +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 DB65862C92\n\tfor ;\n\tThu, 3 Oct 2024 19:06:01 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 67CCF66F;\n\tThu, 3 Oct 2024 19:04:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ibil9Gbj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727975068;\n\tbh=/sqhgn1cHwoNvuav2DbFdO8qeVWN7upYkAgfgCh7zsM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ibil9GbjT+PhImx/dn/qPqdRJipqVXuq/FGQ3ze1XucrkNIUgx/o2gR1zQblobWxu\n\tmPVFqlxBaqDK45VO2kXtntDcBS82QBdNRLccENBfXMNiRxzokf1Ma0/I+XDfR6DdtK\n\tlD+VbM9x4d5C6nCcJbQelV7b6sXbLxdWqIQ9Fowk=","Date":"Thu, 3 Oct 2024 20:05:58 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 5/8] ipa: rkisp1: Add debug metadata support to the\n\trkisp1","Message-ID":"<20241003170558.GC32537@pendragon.ideasonboard.com>","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-6-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241002161933.247091-6-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31574,"web_url":"https://patchwork.libcamera.org/comment/31574/","msgid":"<4szqq7h54n6o3mh6weoxqzx526ug2abmouwzi66y4dkeyifhxt@q7nrokipdtzz>","date":"2024-10-03T20:13:55","subject":"Re: [PATCH v1 5/8] ipa: rkisp1: Add debug metadata support to the\n\trkisp1","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Thu, Oct 03, 2024 at 08:05:58PM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 02, 2024 at 06:19:23PM +0200, Stefan Klug wrote:\n> > Add a DebugMetadata helper to the context and add the corresponding\n> > plumbing. This is all that is needed to support debug metadata in an\n> > IPA.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/ipa_context.h | 5 +++++\n> > src/ipa/rkisp1/rkisp1.cpp | 4 ++++\n> > 2 files changed, 9 insertions(+)\n> > \n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index d52e73ad2503..7b93a9e9461d 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -17,8 +17,11 @@\n> > #include \n> > #include \n> > #include \n> > +\n> > #include \n> > \n> > +#include \"libcamera/internal/debug_controls.h\"\n> > +\n> > #include \n> > #include \n> > #include \n> > @@ -194,6 +197,8 @@ struct IPAContext {\n> > \n> > \tControlInfoMap::Map ctrlMap;\n> > \n> > +\tDebugMetadata debugMetadata;\n> > +\n> > \t/* Interface to the Camera Helper */\n> > \tstd::unique_ptr camHelper;\n> > };\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index a579f21de56f..5b1ef0c372c6 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -117,6 +117,7 @@ const IPAHwSettings ipaHwSettingsV12{\n> > const ControlInfoMap::Map rkisp1Controls{\n> > \t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > \t{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n> > +\t{ &controls::DebugMetadataEnable, ControlInfo(false, true, false) },\n> > \t{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> > \t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> > };\n> > @@ -326,6 +327,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> > {\n> > \tIPAFrameContext &frameContext = context_.frameContexts.alloc(frame);\n> > +\tcontext_.debugMetadata.checkForEnable(controls);\n> > \n> > \tfor (auto const &a : algorithms()) {\n> > \t\tAlgorithm *algo = static_cast(a.get());\n> > @@ -368,6 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > \t\tcontext_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > \n> > \tControlList metadata(controls::controls);\n> > +\tcontext_.debugMetadata.assignControlList(&metadata);\n> \n> What's the advantage of doing this here, instead of...\n> \n> > \n> > \tfor (auto const &a : algorithms()) {\n> > \t\tAlgorithm *algo = static_cast(a.get());\n> > @@ -377,6 +380,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > \t}\n> > \n> > \tsetControls(frame);\n> > +\tcontext_.debugMetadata.assignControlList(nullptr);\n> \n> ... extracting the control list from debugMetadata here and merging it\n> into metadata ?\n\nOh you are right. I had that initial thought of attaching a ControlList\nto a DebugMetadata object for longer than the current function call. But\nin practice that case never happens as the Metadata list is short lived.\nI'll drop the whole assignControlList part in the next version.\n\nRegards,\nStefan\n\n> \n> > \tmetadataReady.emit(frame, metadata);\n> > }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 5D6D2C3257\n\tfor ;\n\tThu, 3 Oct 2024 20:14:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4916463526;\n\tThu, 3 Oct 2024 22:14: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 2517760534\n\tfor ;\n\tThu, 3 Oct 2024 22:13:59 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c4d0:8e70:cd38:b51f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C513A593;\n\tThu, 3 Oct 2024 22:12:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"R0zgNN7V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727986345;\n\tbh=r8UBObJhJUfXFrbJXxVcQX46xlNBHKIDYxzRXjHB28c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R0zgNN7Vcpvr8vrBarIZDtuHcTYG4kCSQZYg6IMOXyEfv4AmWjwZhQWvzQDmHX2QD\n\tGYexPMnBQpYNOkgCLY0XRuDIuM4Zhf2sfeNtm2529W2K+TP3CyvOzxX3OK6Ct7xRIu\n\tjhWeItRua/WtC7+d+LfO1IF0MeNhN9VcsqM6yXVI=","Date":"Thu, 3 Oct 2024 22:13:55 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 5/8] ipa: rkisp1: Add debug metadata support to the\n\trkisp1","Message-ID":"<4szqq7h54n6o3mh6weoxqzx526ug2abmouwzi66y4dkeyifhxt@q7nrokipdtzz>","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-6-stefan.klug@ideasonboard.com>\n\t<20241003170558.GC32537@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241003170558.GC32537@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]