[{"id":31644,"web_url":"https://patchwork.libcamera.org/comment/31644/","msgid":"","date":"2024-10-09T14:05:29","subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. október 9., szerda 15:10 keltezéssel, Stefan Klug írta:\n\n> When merging two control lists, copies of the entries are made. Add a\n> overload of the merge function that accepts a non-const source and\n> provides a merge without copy. This was deemed important in the light of\n> debug metadata were the data should not be copied more often than\n> necessary.\n> \n> This change does not break the API, but it changes the behavior of\n> applications because the source list now gets modified if it is\n> non-const (which might be a common case).\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Todo: After we agreed on a approach, the tests need to be adapted\n> accordingly.\n> ---\n> include/libcamera/controls.h | 1 +\n> src/libcamera/controls.cpp | 32 +++++++++++++++++++++++++++++---\n> 2 files changed, 30 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index ca60bbacad17..2c28ab9447a8 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -401,6 +401,7 @@ public:\n> \n> \tvoid clear() { controls_.clear(); }\n> \tvoid merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> +\tvoid merge(ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n\nWhy not use an rvalue ref, i.e. `ControlList&&`? I think that would be a\nmore natural way to express this.\n\n\n> \n> \tbool contains(unsigned int id) const;\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 62185d643ecd..cb977e1212d8 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -963,11 +963,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\n> * present in *this, then those elements are only overwritten if\n> * \\a policy is MergePolicy::OverwriteExisting.\n> *\n> + * If \\a source is non-const, it will contain the dropped entries after the\n> + * merge. These are either the overwritten ones if policy was\n> + * MergePolicy::OverwriteExisting or the skipped ones if policy was\n> + * MergePolicy::KeepExisting\n> + *\n> * Only control lists created from the same ControlIdMap or ControlInfoMap may\n> * be merged. Attempting to do otherwise results in undefined behaviour.\n> - *\n> - * \\todo Reimplement or implement an overloaded version which internally uses\n> - * std::unordered_map::merge() and accepts a non-const argument.\n> */\n> void ControlList::merge(const ControlList &source, MergePolicy policy)\n> {\n> @@ -995,6 +997,30 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n> \t}\n> }\n> \n> +/**\n> + * \\copydoc ControlList::merge(const ControlList &source, MergePolicy policy)\n> + */\n> +void ControlList::merge(ControlList &source, MergePolicy policy)\n> +{\n> +\t/**\n> +\t * \\todo ASSERT that the current and source ControlList are derived\n> +\t * from a compatible ControlIdMap, to prevent undefined behaviour due to\n> +\t * id collisions.\n> +\t *\n> +\t * This can not currently be a direct pointer comparison due to the\n> +\t * duplication of the ControlIdMaps in the isolated IPA use cases.\n> +\t * Furthermore, manually checking each entry of the id map is identical\n> +\t * is expensive.\n> +\t * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n> +\t */\n> +\n> +\tif (policy == MergePolicy::OverwriteExisting) {\n> +\t\tsource.controls_.merge(controls_);\n> +\t\tsource.controls_.swap(controls_);\n> +\t} else\n> +\t\tcontrols_.merge(source.controls_);\n> +}\n> +\n> /**\n> * \\brief Check if the list contains a control with the specified \\a id\n> * \\param[in] id The control numerical ID\n> --\n> 2.43.0\n> \n\n\nRegards,\nBarnabás Pőcze","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 C62BAC32DE\n\tfor ;\n\tWed, 9 Oct 2024 14:05:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7342E618C9;\n\tWed, 9 Oct 2024 16:05:35 +0200 (CEST)","from mail-40133.protonmail.ch (mail-40133.protonmail.ch\n\t[185.70.40.133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE518618C5\n\tfor ;\n\tWed, 9 Oct 2024 16:05:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"YMT/xN3I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1728482733; x=1728741933;\n\tbh=O4wXqupKlhxMXoejajC3z85BRdlDr1hNW9wABBA8V/E=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=YMT/xN3IDZXIADXlikm80CoF8hmUApRDE2qB5J6s35glV+4KdiXeGZ1GRtOTRm7rN\n\tQ7FsF8pDk157z1jUid9dbhIO+7LK0/nw+e7+cWWU9a0IFh+J2hwU5X4fVyn74Klt07\n\tTzzJ1Y9TRNzlf1Odc+XYULgq8yTiv4l/1SjCoJSWQBee6EWWb9cg55mjZfX/Lw/nj6\n\tgp3pFgJ1Qe6TjbvTSFQxpnydimquA9kPAc4vnRB5QzFEam9v0KgbTbdwUmPDI2S58p\n\tEK6v41mMJSTpr+0DKncB7Srj9z9v6YYiv1kThaA2J7xFmisSvQOJ/u4jSsxptWZL7e\n\thzaCvwQKz0gTQ==","Date":"Wed, 09 Oct 2024 14:05:29 +0000","To":"Stefan Klug ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","Message-ID":"","In-Reply-To":"<20241009131109.547742-1-stefan.klug@ideasonboard.com>","References":"<20241009131109.547742-1-stefan.klug@ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"9398efe40b24d03660ce825ec0a50a751e484364","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":31647,"web_url":"https://patchwork.libcamera.org/comment/31647/","msgid":"<20241009142142.GF2733@pendragon.ideasonboard.com>","date":"2024-10-09T14:21:42","subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 09, 2024 at 02:05:29PM +0000, Barnabás Pőcze wrote:\n> 2024. október 9., szerda 15:10 keltezéssel, Stefan Klug írta:\n> \n> > When merging two control lists, copies of the entries are made. Add a\n> > overload of the merge function that accepts a non-const source and\n> > provides a merge without copy. This was deemed important in the light of\n> > debug metadata were the data should not be copied more often than\n> > necessary.\n> > \n> > This change does not break the API, but it changes the behavior of\n> > applications because the source list now gets modified if it is\n> > non-const (which might be a common case).\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > Todo: After we agreed on a approach, the tests need to be adapted\n> > accordingly.\n> > ---\n> > include/libcamera/controls.h | 1 +\n> > src/libcamera/controls.cpp | 32 +++++++++++++++++++++++++++++---\n> > 2 files changed, 30 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index ca60bbacad17..2c28ab9447a8 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -401,6 +401,7 @@ public:\n> > \n> > \tvoid clear() { controls_.clear(); }\n> > \tvoid merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> > +\tvoid merge(ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> \n> Why not use an rvalue ref, i.e. `ControlList&&`? I think that would be a\n> more natural way to express this.\n\nI was going to mention the same :-)\n\n> > \tbool contains(unsigned int id) const;\n> > \n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 62185d643ecd..cb977e1212d8 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -963,11 +963,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\n> > * present in *this, then those elements are only overwritten if\n> > * \\a policy is MergePolicy::OverwriteExisting.\n> > *\n> > + * If \\a source is non-const, it will contain the dropped entries after the\n> > + * merge. These are either the overwritten ones if policy was\n> > + * MergePolicy::OverwriteExisting or the skipped ones if policy was\n> > + * MergePolicy::KeepExisting\n> > + *\n> > * Only control lists created from the same ControlIdMap or ControlInfoMap may\n> > * be merged. Attempting to do otherwise results in undefined behaviour.\n> > - *\n> > - * \\todo Reimplement or implement an overloaded version which internally uses\n> > - * std::unordered_map::merge() and accepts a non-const argument.\n> > */\n> > void ControlList::merge(const ControlList &source, MergePolicy policy)\n> > {\n> > @@ -995,6 +997,30 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n> > \t}\n> > }\n> > \n> > +/**\n> > + * \\copydoc ControlList::merge(const ControlList &source, MergePolicy policy)\n> > + */\n> > +void ControlList::merge(ControlList &source, MergePolicy policy)\n> > +{\n> > +\t/**\n> > +\t * \\todo ASSERT that the current and source ControlList are derived\n> > +\t * from a compatible ControlIdMap, to prevent undefined behaviour due to\n> > +\t * id collisions.\n> > +\t *\n> > +\t * This can not currently be a direct pointer comparison due to the\n> > +\t * duplication of the ControlIdMaps in the isolated IPA use cases.\n> > +\t * Furthermore, manually checking each entry of the id map is identical\n> > +\t * is expensive.\n> > +\t * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n> > +\t */\n> > +\n> > +\tif (policy == MergePolicy::OverwriteExisting) {\n> > +\t\tsource.controls_.merge(controls_);\n> > +\t\tsource.controls_.swap(controls_);\n> > +\t} else\n> > +\t\tcontrols_.merge(source.controls_);\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the list contains a control with the specified \\a id\n> > * \\param[in] id The control numerical ID","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 30780C32DE\n\tfor ;\n\tWed, 9 Oct 2024 14:21:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFABB6536B;\n\tWed, 9 Oct 2024 16:21:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 73491618C9\n\tfor ;\n\tWed, 9 Oct 2024 16:21:46 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.3])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC1CC2EC;\n\tWed, 9 Oct 2024 16:20:08 +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=\"G0sN1Dwd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728483609;\n\tbh=YEqfbjimV++ifYJnPxOzmAs2fH6t2Gjw4xGjlgDTSwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G0sN1DwdYX93+gTDpg9j3fzznyisAgDqYUf/Xl2+/yQyc4+Q7rXTi4IqJmI5xvvkg\n\ts1dAybqr2eT0pNbrnhUSb3V3Pz5ExT6OwR8uuEphqt0fsajMY3Zb1d/cLlxbJIkENs\n\txX49B/2L4YKDM9SwwlvgXhzXZV5OnnP9R0cHLk+g=","Date":"Wed, 9 Oct 2024 17:21:42 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","Message-ID":"<20241009142142.GF2733@pendragon.ideasonboard.com>","References":"<20241009131109.547742-1-stefan.klug@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":31656,"web_url":"https://patchwork.libcamera.org/comment/31656/","msgid":"","date":"2024-10-09T15:20:02","subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Wed, Oct 09, 2024 at 05:21:42PM +0300, Laurent Pinchart wrote:\n> On Wed, Oct 09, 2024 at 02:05:29PM +0000, Barnabás Pőcze wrote:\n> > 2024. október 9., szerda 15:10 keltezéssel, Stefan Klug írta:\n> > \n> > > When merging two control lists, copies of the entries are made. Add a\n> > > overload of the merge function that accepts a non-const source and\n> > > provides a merge without copy. This was deemed important in the light of\n> > > debug metadata were the data should not be copied more often than\n> > > necessary.\n> > > \n> > > This change does not break the API, but it changes the behavior of\n> > > applications because the source list now gets modified if it is\n> > > non-const (which might be a common case).\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > \n> > > ---\n> > > \n> > > Todo: After we agreed on a approach, the tests need to be adapted\n> > > accordingly.\n> > > ---\n> > > include/libcamera/controls.h | 1 +\n> > > src/libcamera/controls.cpp | 32 +++++++++++++++++++++++++++++---\n> > > 2 files changed, 30 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index ca60bbacad17..2c28ab9447a8 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -401,6 +401,7 @@ public:\n> > > \n> > > \tvoid clear() { controls_.clear(); }\n> > > \tvoid merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> > > +\tvoid merge(ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> > \n> > Why not use an rvalue ref, i.e. `ControlList&&`? I think that would be a\n> > more natural way to express this.\n> \n> I was going to mention the same :-)\n\nYes I thought about that too, but the todo was so explicit that I\ndid that version first. And who knows, maybe having access to\nthe remaining control values is of some use... \n\nAnyways, if everyone is fine with an rvalue, I'll do that.\n\nCheers,\nStefan\n\n> \n> > > \tbool contains(unsigned int id) const;\n> > > \n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 62185d643ecd..cb977e1212d8 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -963,11 +963,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\n> > > * present in *this, then those elements are only overwritten if\n> > > * \\a policy is MergePolicy::OverwriteExisting.\n> > > *\n> > > + * If \\a source is non-const, it will contain the dropped entries after the\n> > > + * merge. These are either the overwritten ones if policy was\n> > > + * MergePolicy::OverwriteExisting or the skipped ones if policy was\n> > > + * MergePolicy::KeepExisting\n> > > + *\n> > > * Only control lists created from the same ControlIdMap or ControlInfoMap may\n> > > * be merged. Attempting to do otherwise results in undefined behaviour.\n> > > - *\n> > > - * \\todo Reimplement or implement an overloaded version which internally uses\n> > > - * std::unordered_map::merge() and accepts a non-const argument.\n> > > */\n> > > void ControlList::merge(const ControlList &source, MergePolicy policy)\n> > > {\n> > > @@ -995,6 +997,30 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n> > > \t}\n> > > }\n> > > \n> > > +/**\n> > > + * \\copydoc ControlList::merge(const ControlList &source, MergePolicy policy)\n> > > + */\n> > > +void ControlList::merge(ControlList &source, MergePolicy policy)\n> > > +{\n> > > +\t/**\n> > > +\t * \\todo ASSERT that the current and source ControlList are derived\n> > > +\t * from a compatible ControlIdMap, to prevent undefined behaviour due to\n> > > +\t * id collisions.\n> > > +\t *\n> > > +\t * This can not currently be a direct pointer comparison due to the\n> > > +\t * duplication of the ControlIdMaps in the isolated IPA use cases.\n> > > +\t * Furthermore, manually checking each entry of the id map is identical\n> > > +\t * is expensive.\n> > > +\t * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n> > > +\t */\n> > > +\n> > > +\tif (policy == MergePolicy::OverwriteExisting) {\n> > > +\t\tsource.controls_.merge(controls_);\n> > > +\t\tsource.controls_.swap(controls_);\n> > > +\t} else\n> > > +\t\tcontrols_.merge(source.controls_);\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Check if the list contains a control with the specified \\a id\n> > > * \\param[in] id The control numerical ID\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 B3841C32DE\n\tfor ;\n\tWed, 9 Oct 2024 15:20:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D8E365369;\n\tWed, 9 Oct 2024 17:20:06 +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 02FC9618C5\n\tfor ;\n\tWed, 9 Oct 2024 17:20:05 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:749:b3f1:dbb0:6c33])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9E6A2594;\n\tWed, 9 Oct 2024 17:18:27 +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=\"JbJ0gXi7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728487107;\n\tbh=uKC+QkYzvB/HalMglf3vH0+fOyrMejs2xVMsrrWX8uQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JbJ0gXi7MBY4fiudtfdjITZpKFq3jtu7RNKcMQANE+H/A+s2t1tqImPi+LgpEh8p2\n\taMFW7srJAtgz2xrzHkPxMmu5geCGm8FClyb6BZgJhyxaTCcIEJ/p4VIyOE8bZcgKAf\n\tTSpeEwHLuk0BH7F+4r1gg+3IE0EySaPAkuBVjV0c=","Date":"Wed, 9 Oct 2024 17:20:02 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","Message-ID":"","References":"<20241009131109.547742-1-stefan.klug@ideasonboard.com>\n\t\n\t<20241009142142.GF2733@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241009142142.GF2733@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\" "}},{"id":31661,"web_url":"https://patchwork.libcamera.org/comment/31661/","msgid":"","date":"2024-10-09T16:09:46","subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2024. október 9., szerda 17:20 keltezéssel, Stefan Klug írta:\n\n> On Wed, Oct 09, 2024 at 05:21:42PM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 09, 2024 at 02:05:29PM +0000, Barnabás Pőcze wrote:\n> > > 2024. október 9., szerda 15:10 keltezéssel, Stefan Klug írta:\n> > >\n> > > > When merging two control lists, copies of the entries are made. Add a\n> > > > overload of the merge function that accepts a non-const source and\n> > > > provides a merge without copy. This was deemed important in the light of\n> > > > debug metadata were the data should not be copied more often than\n> > > > necessary.\n> > > >\n> > > > This change does not break the API, but it changes the behavior of\n> > > > applications because the source list now gets modified if it is\n> > > > non-const (which might be a common case).\n> > > >\n> > > > Signed-off-by: Stefan Klug \n> > > >\n> > > > ---\n> > > >\n> > > > Todo: After we agreed on a approach, the tests need to be adapted\n> > > > accordingly.\n> > > > ---\n> > > > include/libcamera/controls.h | 1 +\n> > > > src/libcamera/controls.cpp | 32 +++++++++++++++++++++++++++++---\n> > > > 2 files changed, 30 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > index ca60bbacad17..2c28ab9447a8 100644\n> > > > --- a/include/libcamera/controls.h\n> > > > +++ b/include/libcamera/controls.h\n> > > > @@ -401,6 +401,7 @@ public:\n> > > >\n> > > > \tvoid clear() { controls_.clear(); }\n> > > > \tvoid merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> > > > +\tvoid merge(ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> > >\n> > > Why not use an rvalue ref, i.e. `ControlList&&`? I think that would be a\n> > > more natural way to express this.\n> >\n> > I was going to mention the same :-)\n> \n> Yes I thought about that too, but the todo was so explicit that I\n> did that version first. And who knows, maybe having access to\n> the remaining control values is of some use...\n\nYou would still be able to access the remaining control values.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Anyways, if everyone is fine with an rvalue, I'll do that.\n> \n> Cheers,\n> Stefan\n> \n> >\n> > > > \tbool contains(unsigned int id) const;\n> > > >\n> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > index 62185d643ecd..cb977e1212d8 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -963,11 +963,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\n> > > > * present in *this, then those elements are only overwritten if\n> > > > * \\a policy is MergePolicy::OverwriteExisting.\n> > > > *\n> > > > + * If \\a source is non-const, it will contain the dropped entries after the\n> > > > + * merge. These are either the overwritten ones if policy was\n> > > > + * MergePolicy::OverwriteExisting or the skipped ones if policy was\n> > > > + * MergePolicy::KeepExisting\n> > > > + *\n> > > > * Only control lists created from the same ControlIdMap or ControlInfoMap may\n> > > > * be merged. Attempting to do otherwise results in undefined behaviour.\n> > > > - *\n> > > > - * \\todo Reimplement or implement an overloaded version which internally uses\n> > > > - * std::unordered_map::merge() and accepts a non-const argument.\n> > > > */\n> > > > void ControlList::merge(const ControlList &source, MergePolicy policy)\n> > > > {\n> > > > @@ -995,6 +997,30 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n> > > > \t}\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\copydoc ControlList::merge(const ControlList &source, MergePolicy policy)\n> > > > + */\n> > > > +void ControlList::merge(ControlList &source, MergePolicy policy)\n> > > > +{\n> > > > +\t/**\n> > > > +\t * \\todo ASSERT that the current and source ControlList are derived\n> > > > +\t * from a compatible ControlIdMap, to prevent undefined behaviour due to\n> > > > +\t * id collisions.\n> > > > +\t *\n> > > > +\t * This can not currently be a direct pointer comparison due to the\n> > > > +\t * duplication of the ControlIdMaps in the isolated IPA use cases.\n> > > > +\t * Furthermore, manually checking each entry of the id map is identical\n> > > > +\t * is expensive.\n> > > > +\t * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n> > > > +\t */\n> > > > +\n> > > > +\tif (policy == MergePolicy::OverwriteExisting) {\n> > > > +\t\tsource.controls_.merge(controls_);\n> > > > +\t\tsource.controls_.swap(controls_);\n> > > > +\t} else\n> > > > +\t\tcontrols_.merge(source.controls_);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Check if the list contains a control with the specified \\a id\n> > > > * \\param[in] id The control numerical ID\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 B06DBC32E0\n\tfor ;\n\tWed, 9 Oct 2024 16:09:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C916063538;\n\tWed, 9 Oct 2024 18:09:52 +0200 (CEST)","from mail-40134.protonmail.ch (mail-40134.protonmail.ch\n\t[185.70.40.134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67E97618C5\n\tfor ;\n\tWed, 9 Oct 2024 18:09:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"aYiSFMoI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1728490190; x=1728749390;\n\tbh=SeN+JmL+5J5USsnl+UvYr8+IYdOfgqxnVmJ+2L6hgw8=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=aYiSFMoIPhe7z6f8IMjo0dcEzYzJiq/VciUotMybf4GWVh1l/al5qWlJlF4xItdjN\n\tUKv6M46v0RYOE/j/gWyet221dKalTC34Wo8TDFhj7Nv3+y9bXbpzg7m6h0BIVHGsxF\n\tVTkqsjg4qTJwu69lmPplfuJnce3WjelzXIT+S3SODukA2D/Qh6gBXmHDj7W9xJt0mL\n\tXj0RTjDUD8Upj21+W8skml7hKl9w3AOfCtB+wdur0oQd2Y2+jyDj1WL32vhGu3iDEE\n\tuY8n81QMdtm284VIdcPEA8rhV6bkIFinIlWKLSzVcSngaXRnK594F/rBow9+p9p8+Y\n\t5xFou4iOhMqtg==","Date":"Wed, 09 Oct 2024 16:09:46 +0000","To":"Stefan Klug ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: controls: Add ControlList::merge overload\n\tthat moves entries","Message-ID":"","In-Reply-To":"","References":"<20241009131109.547742-1-stefan.klug@ideasonboard.com>\n\t\n\t<20241009142142.GF2733@pendragon.ideasonboard.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"cef50c144e9452545df449089ce49413424dfd70","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}}]