[{"id":36144,"web_url":"https://patchwork.libcamera.org/comment/36144/","msgid":"<175975574758.3214037.5212775310089290402@localhost>","date":"2025-10-06T13:02:27","subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nQuoting Barnabás Pőcze (2025-09-24 14:47:08)\n> Add an overload of `ControlList::merge()` that takes the source `ControlList`\n> object via an rvalue. In contrast to the other overload, this one does not\n> make copies, it uses `std::unordered_map::merge()` to move key-value pairs\n> from one map to the other.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/controls.h | 1 +\n> src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++---\n> 2 files changed, 40 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 32fb31f78..5d4a53c46 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -432,6 +432,7 @@ public:\n> \n> void clear() { controls_.clear(); }\n> void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting);\n> \n> bool contains(unsigned int id) const;\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 1e1b49e6b..54cd3b703 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\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\nThis is interesting. My understanding of that comment was that it might\nbe useful to have a implementation of merge() that receives a non-const\nlist that get's filled with the overwritten or skipped controls the same\nway std::unordered_map::merge does. Now thinking of that, implementing\nsuch a version would modify the behavior and potentially introduce\nmisbehavior in code that worked fine before. So maybe we should decide\nif we want such a version better sooner than later.\n\n> */\n> void ControlList::merge(const ControlList &source, MergePolicy policy)\n> {\n> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n> }\n> }\n> \n> +/**\n> + * \\brief Merge the \\a source into the ControlList\n> + * \\param[in] source The ControlList to merge into this object\n> + * \\param[in] policy Controls if existing elements in *this shall be\n> + * overwritten\n> + *\n> + * Merging two control lists moves elements from the \\a source and inserts\n> + * them in *this. If the \\a source contains elements whose key is already\n> + * present in *this, then those elements are only overwritten if\n> + * \\a policy is MergePolicy::OverwriteExisting.\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> +void ControlList::merge(ControlList &&source, MergePolicy policy)\n> +{\n> + /**\n> + * \\todo ASSERT that the current and source ControlList are derived\n> + * from a compatible ControlIdMap, to prevent undefined behaviour due to\n> + * id collisions.\n> + *\n> + * This can not currently be a direct pointer comparison due to the\n> + * duplication of the ControlIdMaps in the isolated IPA use cases.\n> + * Furthermore, manually checking each entry of the id map is identical\n> + * is expensive.\n> + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n> + */\n> +\n> + switch (policy) {\n> + case MergePolicy::KeepExisting:\n> + controls_.merge(source.controls_);\n> + break;\n> + case MergePolicy::OverwriteExisting:\n> + source.controls_.merge(controls_);\n> + controls_.swap(source.controls_);\n> + break;\n> + }\n> +}\n> +\n\nThat looks fine.\n\nBest regards,\nStefan\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.51.0\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 5AECEC324C\n\tfor ;\n\tMon, 6 Oct 2025 13:02:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBDF36B5AA;\n\tMon, 6 Oct 2025 15:02:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CE096936E\n\tfor ;\n\tMon, 6 Oct 2025 15:02:31 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:2e2:f331:f320:caae])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 636F31BD0; \n\tMon, 6 Oct 2025 15:00:58 +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=\"G6SxCES2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759755658;\n\tbh=53ixvax0ysZFLeJwEvQYXcvGm/F5O0WIav3xFc1OpqU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=G6SxCES2czCnsvTwn0I+PmFYoJl5LSAm2K6E01Y7C7Zg/V6D6VVBjeTqSAVjlAX+N\n\tjM553JSpjwqJDs3moDtQ62dNMMDvFK6d4WJlhPAVmdqp++VlzmYAWMC/DeBz8jkUpI\n\t0N65G7oz3E93/tpaiCqf+YLwOUoTXKPyNIIOo5Og=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250924124713.3361707-4-barnabas.pocze@ideasonboard.com>","References":"<20250924124713.3361707-1-barnabas.pocze@ideasonboard.com>\n\t<20250924124713.3361707-4-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tKieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 06 Oct 2025 15:02:27 +0200","Message-ID":"<175975574758.3214037.5212775310089290402@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":36146,"web_url":"https://patchwork.libcamera.org/comment/36146/","msgid":"","date":"2025-10-06T13:30:30","subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> Quoting Barnabás Pőcze (2025-09-24 14:47:08)\n>> Add an overload of `ControlList::merge()` that takes the source `ControlList`\n>> object via an rvalue. In contrast to the other overload, this one does not\n>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs\n>> from one map to the other.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/controls.h | 1 +\n>> src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++---\n>> 2 files changed, 40 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 32fb31f78..5d4a53c46 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -432,6 +432,7 @@ public:\n>> \n>> void clear() { controls_.clear(); }\n>> void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n>> + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting);\n>> \n>> bool contains(unsigned int id) const;\n>> \n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index 1e1b49e6b..54cd3b703 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\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> This is interesting. My understanding of that comment was that it might\n> be useful to have a implementation of merge() that receives a non-const\n> list that get's filled with the overwritten or skipped controls the same\n> way std::unordered_map::merge does. Now thinking of that, implementing\n> such a version would modify the behavior and potentially introduce\n> misbehavior in code that worked fine before. So maybe we should decide\n> if we want such a version better sooner than later.\n\nSorry, can you clarify? I thought this new overload satisfies the TODO comment.\nIt takes an rvalue reference because adding a `ControlList&` overload would wreak havoc.\nBut otherwise `source` is usable after a call to this new overload of `merge()`\nand it will contain the not overwritten / overwritten (depending on `policy`) items.\nAlthough the above behaviour is intentionally not described in the documentation.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> */\n>> void ControlList::merge(const ControlList &source, MergePolicy policy)\n>> {\n>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n>> }\n>> }\n>> \n>> +/**\n>> + * \\brief Merge the \\a source into the ControlList\n>> + * \\param[in] source The ControlList to merge into this object\n>> + * \\param[in] policy Controls if existing elements in *this shall be\n>> + * overwritten\n>> + *\n>> + * Merging two control lists moves elements from the \\a source and inserts\n>> + * them in *this. If the \\a source contains elements whose key is already\n>> + * present in *this, then those elements are only overwritten if\n>> + * \\a policy is MergePolicy::OverwriteExisting.\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>> +void ControlList::merge(ControlList &&source, MergePolicy policy)\n>> +{\n>> + /**\n>> + * \\todo ASSERT that the current and source ControlList are derived\n>> + * from a compatible ControlIdMap, to prevent undefined behaviour due to\n>> + * id collisions.\n>> + *\n>> + * This can not currently be a direct pointer comparison due to the\n>> + * duplication of the ControlIdMaps in the isolated IPA use cases.\n>> + * Furthermore, manually checking each entry of the id map is identical\n>> + * is expensive.\n>> + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n>> + */\n>> +\n>> + switch (policy) {\n>> + case MergePolicy::KeepExisting:\n>> + controls_.merge(source.controls_);\n>> + break;\n>> + case MergePolicy::OverwriteExisting:\n>> + source.controls_.merge(controls_);\n>> + controls_.swap(source.controls_);\n>> + break;\n>> + }\n>> +}\n>> +\n> \n> That looks fine.\n> \n> Best regards,\n> Stefan\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.51.0\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 B7579C324C\n\tfor ;\n\tMon, 6 Oct 2025 13:30:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F352D6B5F3;\n\tMon, 6 Oct 2025 15:30: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 1B9BC6936E\n\tfor ;\n\tMon, 6 Oct 2025 15:30:34 +0200 (CEST)","from [192.168.33.23] (185.182.214.142.nat.pool.zt.hu\n\t[185.182.214.142])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09A8BB7D;\n\tMon, 6 Oct 2025 15:29:00 +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=\"Gi97gh26\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759757341;\n\tbh=JxW/qfA/sEgrdN2oa+fuTvxziAYaPsNo1xST8xVz/Y4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Gi97gh26oLdOAUUnDdxeKX2ZH31csuSTtUK6ILYHIQwm+oSKo88VGWgvaGljNGbH/\n\tVlgM2SSesNsXCspYeRzaWVVdyPiIjQgg+Pr5PVcXfGyqPuioxvIA6iVC5Yw3IAd/DO\n\toO9sqnh5HYYRkhTmLykOGZ9zINwx6OhFNutaLJpg=","Message-ID":"","Date":"Mon, 6 Oct 2025 15:30:30 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","To":"Stefan Klug ,\n\tKieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250924124713.3361707-1-barnabas.pocze@ideasonboard.com>\n\t<20250924124713.3361707-4-barnabas.pocze@ideasonboard.com>\n\t<175975574758.3214037.5212775310089290402@localhost>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175975574758.3214037.5212775310089290402@localhost>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36147,"web_url":"https://patchwork.libcamera.org/comment/36147/","msgid":"","date":"2025-10-06T13:37:02","subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta:\n> Hi\n> \n> 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta:\n>> Hi Barnabás,\n>>\n>> Thank you for the patch.\n>>\n>> Quoting Barnabás Pőcze (2025-09-24 14:47:08)\n>>> Add an overload of `ControlList::merge()` that takes the source `ControlList`\n>>> object via an rvalue. In contrast to the other overload, this one does not\n>>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs\n>>> from one map to the other.\n>>>\n>>> Signed-off-by: Barnabás Pőcze \n>>> ---\n>>>   include/libcamera/controls.h |  1 +\n>>>   src/libcamera/controls.cpp   | 42 +++++++++++++++++++++++++++++++++---\n>>>   2 files changed, 40 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>> index 32fb31f78..5d4a53c46 100644\n>>> --- a/include/libcamera/controls.h\n>>> +++ b/include/libcamera/controls.h\n>>> @@ -432,6 +432,7 @@ public:\n>>>          void clear() { controls_.clear(); }\n>>>          void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n>>> +       void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting);\n>>>          bool contains(unsigned int id) const;\n>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>> index 1e1b49e6b..54cd3b703 100644\n>>> --- a/src/libcamera/controls.cpp\n>>> +++ b/src/libcamera/controls.cpp\n>>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\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>> This is interesting. My understanding of that comment was that it might\n>> be useful to have a implementation of merge() that receives a non-const\n>> list that get's filled with the overwritten or skipped controls the same\n>> way std::unordered_map::merge does. Now thinking of that, implementing\n>> such a version would modify the behavior and potentially introduce\n>> misbehavior in code that worked fine before. So maybe we should decide\n>> if we want such a version better sooner than later.\n> \n> Sorry, can you clarify? I thought this new overload satisfies the TODO comment.\n> It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc.\n> But otherwise `source` is usable after a call to this new overload of `merge()`\n> and it will contain the not overwritten / overwritten (depending on `policy`) items.\n\nMore specifically:\n\nLet\n *this = A u B\n source = A' u C\nwhere A and A' are the sets of items whose keys are present in both sets.\n\nIf KeepExisting, then source = A',\nif OverwriteExisting, then source = A,\nafter the call to merge.\n\n\n> Although the above behaviour is intentionally not described in the documentation.\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n>>\n>>>    */\n>>>   void ControlList::merge(const ControlList &source, MergePolicy policy)\n>>>   {\n>>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n>>>          }\n>>>   }\n>>> +/**\n>>> + * \\brief Merge the \\a source into the ControlList\n>>> + * \\param[in] source The ControlList to merge into this object\n>>> + * \\param[in] policy Controls if existing elements in *this shall be\n>>> + * overwritten\n>>> + *\n>>> + * Merging two control lists moves elements from the \\a source and inserts\n>>> + * them in *this. If the \\a source contains elements whose key is already\n>>> + * present in *this, then those elements are only overwritten if\n>>> + * \\a policy is MergePolicy::OverwriteExisting.\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>>> +void ControlList::merge(ControlList &&source, MergePolicy policy)\n>>> +{\n>>> +       /**\n>>> +        * \\todo ASSERT that the current and source ControlList are derived\n>>> +        * from a compatible ControlIdMap, to prevent undefined behaviour due to\n>>> +        * id collisions.\n>>> +        *\n>>> +        * This can not currently be a direct pointer comparison due to the\n>>> +        * duplication of the ControlIdMaps in the isolated IPA use cases.\n>>> +        * Furthermore, manually checking each entry of the id map is identical\n>>> +        * is expensive.\n>>> +        * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n>>> +        */\n>>> +\n>>> +       switch (policy) {\n>>> +       case MergePolicy::KeepExisting:\n>>> +               controls_.merge(source.controls_);\n>>> +               break;\n>>> +       case MergePolicy::OverwriteExisting:\n>>> +               source.controls_.merge(controls_);\n>>> +               controls_.swap(source.controls_);\n>>> +               break;\n>>> +       }\n>>> +}\n>>> +\n>>\n>> That looks fine.\n>>\n>> Best regards,\n>> Stefan\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.51.0\n>>>\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 A580FBF415\n\tfor ;\n\tMon, 6 Oct 2025 13:37:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D07626B5F3;\n\tMon, 6 Oct 2025 15:37:08 +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 A8DBA6936E\n\tfor ;\n\tMon, 6 Oct 2025 15:37:06 +0200 (CEST)","from [192.168.33.23] (185.182.214.142.nat.pool.zt.hu\n\t[185.182.214.142])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE7EAB7D;\n\tMon, 6 Oct 2025 15:35:33 +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=\"myPgiUXU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759757734;\n\tbh=lbRkbtsXz4Oh32L2ihArQpvGteZFXlh3bFF3EXDdQ+w=;\n\th=Date:Subject:From:To:References:In-Reply-To:From;\n\tb=myPgiUXUEdtKGq26WPgrIsvxGuf2+wYYM24TL0orKHRk9b09C0glP4YNWdZ9Upt1b\n\tpvD3w2fMsmmiErfzzh5ObgD13n++n/XkrVCgg5hzxxk59HWaQUBsnxY0TVNhenrozr\n\tUs38N684gJjabkihoi3eG4Fjepq7k/ekSzgNrjEE=","Message-ID":"","Date":"Mon, 6 Oct 2025 15:37:02 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"Stefan Klug ,\n\tKieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250924124713.3361707-1-barnabas.pocze@ideasonboard.com>\n\t<20250924124713.3361707-4-barnabas.pocze@ideasonboard.com>\n\t<175975574758.3214037.5212775310089290402@localhost>\n\t","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36151,"web_url":"https://patchwork.libcamera.org/comment/36151/","msgid":"<175977478303.1512356.7403946621974516236@localhost>","date":"2025-10-06T18:19:43","subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-10-06 15:37:02)\n> 2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta:\n> > Hi\n> > \n> > 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta:\n> >> Hi Barnabás,\n> >>\n> >> Thank you for the patch.\n> >>\n> >> Quoting Barnabás Pőcze (2025-09-24 14:47:08)\n> >>> Add an overload of `ControlList::merge()` that takes the source `ControlList`\n> >>> object via an rvalue. In contrast to the other overload, this one does not\n> >>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs\n> >>> from one map to the other.\n> >>>\n> >>> Signed-off-by: Barnabás Pőcze \n> >>> ---\n> >>>   include/libcamera/controls.h |  1 +\n> >>>   src/libcamera/controls.cpp   | 42 +++++++++++++++++++++++++++++++++---\n> >>>   2 files changed, 40 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>> index 32fb31f78..5d4a53c46 100644\n> >>> --- a/include/libcamera/controls.h\n> >>> +++ b/include/libcamera/controls.h\n> >>> @@ -432,6 +432,7 @@ public:\n> >>>          void clear() { controls_.clear(); }\n> >>>          void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> >>> +       void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting);\n> >>>          bool contains(unsigned int id) const;\n> >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>> index 1e1b49e6b..54cd3b703 100644\n> >>> --- a/src/libcamera/controls.cpp\n> >>> +++ b/src/libcamera/controls.cpp\n> >>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\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> >> This is interesting. My understanding of that comment was that it might\n> >> be useful to have a implementation of merge() that receives a non-const\n> >> list that get's filled with the overwritten or skipped controls the same\n> >> way std::unordered_map::merge does. Now thinking of that, implementing\n> >> such a version would modify the behavior and potentially introduce\n> >> misbehavior in code that worked fine before. So maybe we should decide\n> >> if we want such a version better sooner than later.\n> > \n> > Sorry, can you clarify? I thought this new overload satisfies the TODO comment.\n> > It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc.\n> > But otherwise `source` is usable after a call to this new overload of `merge()`\n> > and it will contain the not overwritten / overwritten (depending on `policy`) items.\n> \n> More specifically:\n> \n> Let\n> *this = A u B\n> source = A' u C\n> where A and A' are the sets of items whose keys are present in both sets.\n> \n> If KeepExisting, then source = A',\n> if OverwriteExisting, then source = A,\n> after the call to merge.\n\nSorry for my ignorance (or a lack of C++ knowledge :-)). How would you\nuse that?\n\nLets say:\n\nControlList l1, l2;\nl1.set(A, ...);\nl1.set(B, ...);\nl2.set(A', ...)\n\n// Is this right?\nl1.merge(std::move(l2),...);\n\nAm I allowed to access the data in l2 now?\nI think I never used move semantics in that way.\n\nBest regards,\nStefan\n\n> \n> \n> > Although the above behaviour is intentionally not described in the documentation.\n> > \n> > \n> > Regards,\n> > Barnabás Pőcze\n> > \n> > \n> >>\n> >>>    */\n> >>>   void ControlList::merge(const ControlList &source, MergePolicy policy)\n> >>>   {\n> >>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n> >>>          }\n> >>>   }\n> >>> +/**\n> >>> + * \\brief Merge the \\a source into the ControlList\n> >>> + * \\param[in] source The ControlList to merge into this object\n> >>> + * \\param[in] policy Controls if existing elements in *this shall be\n> >>> + * overwritten\n> >>> + *\n> >>> + * Merging two control lists moves elements from the \\a source and inserts\n> >>> + * them in *this. If the \\a source contains elements whose key is already\n> >>> + * present in *this, then those elements are only overwritten if\n> >>> + * \\a policy is MergePolicy::OverwriteExisting.\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> >>> +void ControlList::merge(ControlList &&source, MergePolicy policy)\n> >>> +{\n> >>> +       /**\n> >>> +        * \\todo ASSERT that the current and source ControlList are derived\n> >>> +        * from a compatible ControlIdMap, to prevent undefined behaviour due to\n> >>> +        * id collisions.\n> >>> +        *\n> >>> +        * This can not currently be a direct pointer comparison due to the\n> >>> +        * duplication of the ControlIdMaps in the isolated IPA use cases.\n> >>> +        * Furthermore, manually checking each entry of the id map is identical\n> >>> +        * is expensive.\n> >>> +        * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n> >>> +        */\n> >>> +\n> >>> +       switch (policy) {\n> >>> +       case MergePolicy::KeepExisting:\n> >>> +               controls_.merge(source.controls_);\n> >>> +               break;\n> >>> +       case MergePolicy::OverwriteExisting:\n> >>> +               source.controls_.merge(controls_);\n> >>> +               controls_.swap(source.controls_);\n> >>> +               break;\n> >>> +       }\n> >>> +}\n> >>> +\n> >>\n> >> That looks fine.\n> >>\n> >> Best regards,\n> >> Stefan\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.51.0\n> >>>\n> > \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 D05C3BF415\n\tfor ;\n\tMon, 6 Oct 2025 18:19:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85A726B5F3;\n\tMon, 6 Oct 2025 20:19:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27E0D6936E\n\tfor ;\n\tMon, 6 Oct 2025 20:19:46 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:2e2:f331:f320:caae])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 50DBDBCA;\n\tMon, 6 Oct 2025 20:18:13 +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=\"LbFxnOYR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759774693;\n\tbh=9Aupmw4dSwiRrsMEr1o4uyY9Mg5qXnJPMtzxgzPZrHs=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=LbFxnOYROd5B8k2PavzDMS8RopugIk/uhP+a76AK8enlDLwyntJ50Z8u1WMB6aPF+\n\tH34fI5et9KtxsWXBT8aZsu5Rup0Sy91pv6wC+21euVxRiezf+wiKPNKlG8zymHiwPW\n\tHDhCVEA0vVcve1IrHJS1sW5K3HBRfT/IslnjyvGc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250924124713.3361707-1-barnabas.pocze@ideasonboard.com>\n\t<20250924124713.3361707-4-barnabas.pocze@ideasonboard.com>\n\t<175975574758.3214037.5212775310089290402@localhost>\n\t\n\t","Subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tKieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 06 Oct 2025 20:19:43 +0200","Message-ID":"<175977478303.1512356.7403946621974516236@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":36160,"web_url":"https://patchwork.libcamera.org/comment/36160/","msgid":"","date":"2025-10-07T08:46:29","subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 10. 06. 20:19 keltezéssel, Stefan Klug írta:\n> Quoting Barnabás Pőcze (2025-10-06 15:37:02)\n>> 2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta:\n>>> Hi\n>>>\n>>> 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta:\n>>>> Hi Barnabás,\n>>>>\n>>>> Thank you for the patch.\n>>>>\n>>>> Quoting Barnabás Pőcze (2025-09-24 14:47:08)\n>>>>> Add an overload of `ControlList::merge()` that takes the source `ControlList`\n>>>>> object via an rvalue. In contrast to the other overload, this one does not\n>>>>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs\n>>>>> from one map to the other.\n>>>>>\n>>>>> Signed-off-by: Barnabás Pőcze \n>>>>> ---\n>>>>>   include/libcamera/controls.h |  1 +\n>>>>>   src/libcamera/controls.cpp   | 42 +++++++++++++++++++++++++++++++++---\n>>>>>   2 files changed, 40 insertions(+), 3 deletions(-)\n>>>>>\n>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>> index 32fb31f78..5d4a53c46 100644\n>>>>> --- a/include/libcamera/controls.h\n>>>>> +++ b/include/libcamera/controls.h\n>>>>> @@ -432,6 +432,7 @@ public:\n>>>>>          void clear() { controls_.clear(); }\n>>>>>          void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n>>>>> +       void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting);\n>>>>>          bool contains(unsigned int id) const;\n>>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>>> index 1e1b49e6b..54cd3b703 100644\n>>>>> --- a/src/libcamera/controls.cpp\n>>>>> +++ b/src/libcamera/controls.cpp\n>>>>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\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>>>> This is interesting. My understanding of that comment was that it might\n>>>> be useful to have a implementation of merge() that receives a non-const\n>>>> list that get's filled with the overwritten or skipped controls the same\n>>>> way std::unordered_map::merge does. Now thinking of that, implementing\n>>>> such a version would modify the behavior and potentially introduce\n>>>> misbehavior in code that worked fine before. So maybe we should decide\n>>>> if we want such a version better sooner than later.\n>>>\n>>> Sorry, can you clarify? I thought this new overload satisfies the TODO comment.\n>>> It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc.\n>>> But otherwise `source` is usable after a call to this new overload of `merge()`\n>>> and it will contain the not overwritten / overwritten (depending on `policy`) items.\n>>\n>> More specifically:\n>>\n>> Let\n>> *this = A u B\n>> source = A' u C\n>> where A and A' are the sets of items whose keys are present in both sets.\n>>\n>> If KeepExisting, then source = A',\n>> if OverwriteExisting, then source = A,\n>> after the call to merge.\n> \n> Sorry for my ignorance (or a lack of C++ knowledge :-)). How would you\n> use that?\n> \n> Lets say:\n> \n> ControlList l1, l2;\n> l1.set(A, ...);\n> l1.set(B, ...);\n> l2.set(A', ...)\n> \n> // Is this right?\n> l1.merge(std::move(l2),...);\n> \n> Am I allowed to access the data in l2 now?\n> I think I never used move semantics in that way.\n\nA moved-from instance of a type is assumed to be in a \"valid but unspecified\" state.\nAt minimum the destructor must be able to run successfully. Certain types may provide\nmore guarantees about what operations are valid in the moved-from state.\n\nBut in any case, there isn't any \"moving\" happening here. The rvalue reference here\nacts as an lvalue reference for all intents and purposes. It is only an rvalue reference\nbecause using an lvalue reference would make it a lot harder to deduce which overload\nis actually getting called.\n\nSo yes, in this case accessing `l2` is perfectly fine, it is not even in a moved-from state.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> Best regards,\n> Stefan\n> \n>>\n>>\n>>> Although the above behaviour is intentionally not described in the documentation.\n>>>\n>>>\n>>> Regards,\n>>> Barnabás Pőcze\n>>>\n>>>\n>>>>\n>>>>>    */\n>>>>>   void ControlList::merge(const ControlList &source, MergePolicy policy)\n>>>>>   {\n>>>>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n>>>>>          }\n>>>>>   }\n>>>>> +/**\n>>>>> + * \\brief Merge the \\a source into the ControlList\n>>>>> + * \\param[in] source The ControlList to merge into this object\n>>>>> + * \\param[in] policy Controls if existing elements in *this shall be\n>>>>> + * overwritten\n>>>>> + *\n>>>>> + * Merging two control lists moves elements from the \\a source and inserts\n>>>>> + * them in *this. If the \\a source contains elements whose key is already\n>>>>> + * present in *this, then those elements are only overwritten if\n>>>>> + * \\a policy is MergePolicy::OverwriteExisting.\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>>>>> +void ControlList::merge(ControlList &&source, MergePolicy policy)\n>>>>> +{\n>>>>> +       /**\n>>>>> +        * \\todo ASSERT that the current and source ControlList are derived\n>>>>> +        * from a compatible ControlIdMap, to prevent undefined behaviour due to\n>>>>> +        * id collisions.\n>>>>> +        *\n>>>>> +        * This can not currently be a direct pointer comparison due to the\n>>>>> +        * duplication of the ControlIdMaps in the isolated IPA use cases.\n>>>>> +        * Furthermore, manually checking each entry of the id map is identical\n>>>>> +        * is expensive.\n>>>>> +        * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n>>>>> +        */\n>>>>> +\n>>>>> +       switch (policy) {\n>>>>> +       case MergePolicy::KeepExisting:\n>>>>> +               controls_.merge(source.controls_);\n>>>>> +               break;\n>>>>> +       case MergePolicy::OverwriteExisting:\n>>>>> +               source.controls_.merge(controls_);\n>>>>> +               controls_.swap(source.controls_);\n>>>>> +               break;\n>>>>> +       }\n>>>>> +}\n>>>>> +\n>>>>\n>>>> That looks fine.\n>>>>\n>>>> Best regards,\n>>>> Stefan\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.51.0\n>>>>>\n>>>\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 AE9E0C324C\n\tfor ;\n\tTue, 7 Oct 2025 08:46:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF7FB6B5AA;\n\tTue, 7 Oct 2025 10:46:35 +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 2630F6B58E\n\tfor ;\n\tTue, 7 Oct 2025 10:46:34 +0200 (CEST)","from [192.168.33.24] (185.182.214.142.nat.pool.zt.hu\n\t[185.182.214.142])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A42031122;\n\tTue, 7 Oct 2025 10:45:00 +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=\"WhEo7A6v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759826700;\n\tbh=tdNzd4ObubcASb1wot2LwV2QmNth+j1mhPZ2UWS1V0Q=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=WhEo7A6vr0xcB9/ecMvrqlmimiHADaMBE6u9FMUF1zgk6CdXY+iIQFTOLh9qQb0YQ\n\tMatwTm5sibm3M7y0S6bxVz0o2zx4PCmiDMP7Ozj5fIdKwWnxSmmWboGImDzXv1eyKr\n\tfIEMBDlC/d6L2hbY9F2wUwB/K6ifsAE0WlWxFbVc=","Message-ID":"","Date":"Tue, 7 Oct 2025 10:46:29 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","To":"Stefan Klug ,\n\tKieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250924124713.3361707-1-barnabas.pocze@ideasonboard.com>\n\t<20250924124713.3361707-4-barnabas.pocze@ideasonboard.com>\n\t<175975574758.3214037.5212775310089290402@localhost>\n\t\n\t\n\t<175977478303.1512356.7403946621974516236@localhost>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175977478303.1512356.7403946621974516236@localhost>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36180,"web_url":"https://patchwork.libcamera.org/comment/36180/","msgid":"<175994996160.1512356.5530280195630893333@localhost>","date":"2025-10-08T18:59:21","subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi,\n\nQuoting Barnabás Pőcze (2025-10-07 10:46:29)\n> 2025. 10. 06. 20:19 keltezéssel, Stefan Klug írta:\n> > Quoting Barnabás Pőcze (2025-10-06 15:37:02)\n> >> 2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta:\n> >>> Hi\n> >>>\n> >>> 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta:\n> >>>> Hi Barnabás,\n> >>>>\n> >>>> Thank you for the patch.\n> >>>>\n> >>>> Quoting Barnabás Pőcze (2025-09-24 14:47:08)\n> >>>>> Add an overload of `ControlList::merge()` that takes the source `ControlList`\n> >>>>> object via an rvalue. In contrast to the other overload, this one does not\n> >>>>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs\n> >>>>> from one map to the other.\n> >>>>>\n> >>>>> Signed-off-by: Barnabás Pőcze \n> >>>>> ---\n> >>>>>   include/libcamera/controls.h |  1 +\n> >>>>>   src/libcamera/controls.cpp   | 42 +++++++++++++++++++++++++++++++++---\n> >>>>>   2 files changed, 40 insertions(+), 3 deletions(-)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>>>> index 32fb31f78..5d4a53c46 100644\n> >>>>> --- a/include/libcamera/controls.h\n> >>>>> +++ b/include/libcamera/controls.h\n> >>>>> @@ -432,6 +432,7 @@ public:\n> >>>>>          void clear() { controls_.clear(); }\n> >>>>>          void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);\n> >>>>> +       void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting);\n> >>>>>          bool contains(unsigned int id) const;\n> >>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>>>> index 1e1b49e6b..54cd3b703 100644\n> >>>>> --- a/src/libcamera/controls.cpp\n> >>>>> +++ b/src/libcamera/controls.cpp\n> >>>>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\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> >>>> This is interesting. My understanding of that comment was that it might\n> >>>> be useful to have a implementation of merge() that receives a non-const\n> >>>> list that get's filled with the overwritten or skipped controls the same\n> >>>> way std::unordered_map::merge does. Now thinking of that, implementing\n> >>>> such a version would modify the behavior and potentially introduce\n> >>>> misbehavior in code that worked fine before. So maybe we should decide\n> >>>> if we want such a version better sooner than later.\n> >>>\n> >>> Sorry, can you clarify? I thought this new overload satisfies the TODO comment.\n> >>> It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc.\n> >>> But otherwise `source` is usable after a call to this new overload of `merge()`\n> >>> and it will contain the not overwritten / overwritten (depending on `policy`) items.\n> >>\n> >> More specifically:\n> >>\n> >> Let\n> >> *this = A u B\n> >> source = A' u C\n> >> where A and A' are the sets of items whose keys are present in both sets.\n> >>\n> >> If KeepExisting, then source = A',\n> >> if OverwriteExisting, then source = A,\n> >> after the call to merge.\n> > \n> > Sorry for my ignorance (or a lack of C++ knowledge :-)). How would you\n> > use that?\n> > \n> > Lets say:\n> > \n> > ControlList l1, l2;\n> > l1.set(A, ...);\n> > l1.set(B, ...);\n> > l2.set(A', ...)\n> > \n> > // Is this right?\n> > l1.merge(std::move(l2),...);\n> > \n> > Am I allowed to access the data in l2 now?\n> > I think I never used move semantics in that way.\n> \n> A moved-from instance of a type is assumed to be in a \"valid but unspecified\" state.\n> At minimum the destructor must be able to run successfully. Certain types may provide\n> more guarantees about what operations are valid in the moved-from state.\n> \n> But in any case, there isn't any \"moving\" happening here. The rvalue reference here\n> acts as an lvalue reference for all intents and purposes. It is only an rvalue reference\n> because using an lvalue reference would make it a lot harder to deduce which overload\n> is actually getting called.\n> \n> So yes, in this case accessing `l2` is perfectly fine, it is not even in a moved-from state.\n\nThat makes all sense now.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> > \n> > Best regards,\n> > Stefan\n> > \n> >>\n> >>\n> >>> Although the above behaviour is intentionally not described in the documentation.\n> >>>\n> >>>\n> >>> Regards,\n> >>> Barnabás Pőcze\n> >>>\n> >>>\n> >>>>\n> >>>>>    */\n> >>>>>   void ControlList::merge(const ControlList &source, MergePolicy policy)\n> >>>>>   {\n> >>>>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)\n> >>>>>          }\n> >>>>>   }\n> >>>>> +/**\n> >>>>> + * \\brief Merge the \\a source into the ControlList\n> >>>>> + * \\param[in] source The ControlList to merge into this object\n> >>>>> + * \\param[in] policy Controls if existing elements in *this shall be\n> >>>>> + * overwritten\n> >>>>> + *\n> >>>>> + * Merging two control lists moves elements from the \\a source and inserts\n> >>>>> + * them in *this. If the \\a source contains elements whose key is already\n> >>>>> + * present in *this, then those elements are only overwritten if\n> >>>>> + * \\a policy is MergePolicy::OverwriteExisting.\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> >>>>> +void ControlList::merge(ControlList &&source, MergePolicy policy)\n> >>>>> +{\n> >>>>> +       /**\n> >>>>> +        * \\todo ASSERT that the current and source ControlList are derived\n> >>>>> +        * from a compatible ControlIdMap, to prevent undefined behaviour due to\n> >>>>> +        * id collisions.\n> >>>>> +        *\n> >>>>> +        * This can not currently be a direct pointer comparison due to the\n> >>>>> +        * duplication of the ControlIdMaps in the isolated IPA use cases.\n> >>>>> +        * Furthermore, manually checking each entry of the id map is identical\n> >>>>> +        * is expensive.\n> >>>>> +        * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details\n> >>>>> +        */\n> >>>>> +\n> >>>>> +       switch (policy) {\n> >>>>> +       case MergePolicy::KeepExisting:\n> >>>>> +               controls_.merge(source.controls_);\n> >>>>> +               break;\n> >>>>> +       case MergePolicy::OverwriteExisting:\n> >>>>> +               source.controls_.merge(controls_);\n> >>>>> +               controls_.swap(source.controls_);\n> >>>>> +               break;\n> >>>>> +       }\n> >>>>> +}\n> >>>>> +\n> >>>>\n> >>>> That looks fine.\n> >>>>\n> >>>> Best regards,\n> >>>> Stefan\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.51.0\n> >>>>>\n> >>>\n> >>\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 00E82BF415\n\tfor ;\n\tWed, 8 Oct 2025 18:59:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAD866B608;\n\tWed, 8 Oct 2025 20:59:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1DA56B58E\n\tfor ;\n\tWed, 8 Oct 2025 20:59:24 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:268d:242f:e321:9ef0])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 74D861831; \n\tWed, 8 Oct 2025 20:57:50 +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=\"VLsMdB8X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759949870;\n\tbh=pMlK1lXGOm+352F5QRJV8iQ0ihmzbW0p3M5GyiQkD/0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=VLsMdB8XBeKc32xMpI8oivDmYAIL6XoFDWuy2H71RQ3X2iOv+jcYY4LglI1zrL9+b\n\tabuvEAFcqRdjt1KGirn/GyWvaly0XKPl6sq/4yEDzgCO/9o9LNDbvY4yAVi1WxAR+z\n\tWcwx8U+zOwUZOF3IRkJO5GZ5SFTjlEJ/VABgPOCY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250924124713.3361707-1-barnabas.pocze@ideasonboard.com>\n\t<20250924124713.3361707-4-barnabas.pocze@ideasonboard.com>\n\t<175975574758.3214037.5212775310089290402@localhost>\n\t\n\t\n\t<175977478303.1512356.7403946621974516236@localhost>\n\t","Subject":"Re: [RFC PATCH v1 3/7] libcamera: controls: Add rvalue\n\t`ControlList::merge()`","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tKieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 08 Oct 2025 20:59:21 +0200","Message-ID":"<175994996160.1512356.5530280195630893333@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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\" "}}]