[{"id":28818,"web_url":"https://patchwork.libcamera.org/comment/28818/","msgid":"<5179f508-1597-4145-aa14-e5f893294f31@ideasonboard.com>","date":"2024-03-04T04:48:58","subject":"Re: [PATCH] libcamera: controls: Add overwriteExisting parameter to\n\tControlList::merge","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn 29/02/24 10:27 pm, Stefan Klug wrote:\n> This is useful in many cases although not included in the stl.\n>\n> A typical usecase would be:\n> ContolList controls = getDefaults()\n> controls.merge(someSpecialValues, true)\n> ...\n>\n>\n> Signed-off-by: Stefan Klug \n> ---\n>\n> Hey,\n>\n> this is my first email patch - hurray :-)\n\nGreat !\n> Cheers,\n> Stefan\n>\n> include/libcamera/controls.h | 2 +-\n> src/libcamera/controls.cpp | 9 ++++++---\n> 2 files changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index cf942055..0bf778d8 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -368,7 +368,7 @@ public:\n> \tstd::size_t size() const { return controls_.size(); }\n> \n> \tvoid clear() { controls_.clear(); }\n> -\tvoid merge(const ControlList &source);\n> +\tvoid merge(const ControlList &source, bool overwriteExisting = false);\n> \n> \tbool contains(unsigned int id) const;\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index b808116c..8ef29a96 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -910,10 +910,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\n> /**\n> * \\brief Merge the \\a source into the ControlList\n> * \\param[in] source The ControlList to merge into this object\n> + * \\param[in] overwriteExisting Control if existing elements in *this shall be\n> + * overwritten\n> *\n> * Merging two control lists copies 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 not overwritten.\n> + * present in *this, then those elements are only overwritten if\n> + * overwriteExisting is true.\n\ns/overwriteExisting/\\a 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> @@ -921,7 +924,7 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\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)\n> +void ControlList::merge(const ControlList &source, bool overwriteExisting)\n> {\n> \t/**\n> \t * \\todo ASSERT that the current and source ControlList are derived\n> @@ -936,7 +939,7 @@ void ControlList::merge(const ControlList &source)\n> \t */\n> \n> \tfor (const auto &ctrl : source) {\n> -\t\tif (contains(ctrl.first)) {\n> +\t\tif (!overwriteExisting && contains(ctrl.first)) {\n\nLooks good to me.\n\nStefan, can a small unit test can cover this as well? I see,\n\n                 /*\n                  * Create a new list with a new control and merge it \nwith the\n                  * existing one, verifying that the existing controls\n                  * values don't get overwritten.\n                  */\n\nin test/controls/control_list.cpp, so probably worth adding this new \nbehaviour as well, to that test.\n> \t\t\tconst ControlId *id = idmap_->at(ctrl.first);\n> \t\t\tLOG(Controls, Warning)\n> \t\t\t\t<< \"Control \" << id->name() << \" not overwritten\";","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 22577C326B\n\tfor ;\n\tMon, 4 Mar 2024 04:49:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6854961C85;\n\tMon, 4 Mar 2024 05:49:07 +0100 (CET)","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 1374461C85\n\tfor ;\n\tMon, 4 Mar 2024 05:49:05 +0100 (CET)","from [192.168.1.102] (unknown [103.251.226.116])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E3CE8D0;\n\tMon, 4 Mar 2024 05:48:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rjPPxbjv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709527728;\n\tbh=NhKmCMMk4VU4m45bOcgse/XHwuJGYbK3lhZHsQA+3Pg=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=rjPPxbjvk1Yy4NWUt+wQtGx1igudBSzSVpeT7jstJThnDIzDnRZdfn6cY6JDzoB1U\n\tu7UAaXgusTPPakrpBj0zlIzjGRb6jtNj6L673AezlH8WsWzUi8/Ia5Ts24RV9J6KNv\n\taB7hmw6E6e2wf6HXwYx95RH6MIX6RxKQBIC+vv88=","Message-ID":"<5179f508-1597-4145-aa14-e5f893294f31@ideasonboard.com>","Date":"Mon, 4 Mar 2024 10:18:58 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: controls: Add overwriteExisting parameter to\n\tControlList::merge","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240229165753.154936-1-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240229165753.154936-1-stefan.klug@ideasonboard.com>","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":28824,"web_url":"https://patchwork.libcamera.org/comment/28824/","msgid":"<53dadbdb-a92d-4a83-b534-d8ba457326f7@ideasonboard.com>","date":"2024-03-04T15:43:28","subject":"Re: [PATCH] libcamera: controls: Add overwriteExisting parameter to\n\tControlList::merge","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nthanks for your review. I'll post a updated patch soon.\n\nCheers,\nStefan\n\nAm 04.03.24 um 05:48 schrieb Umang Jain:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On 29/02/24 10:27 pm, Stefan Klug wrote:\n>> This is useful in many cases although not included in the stl.\n>>\n>> A typical usecase would be:\n>> ContolList controls = getDefaults()\n>> controls.merge(someSpecialValues, true)\n>> ...\n>>\n>>\n>> Signed-off-by: Stefan Klug \n>> ---\n>>\n>> Hey,\n>>\n>> this is my first email patch - hurray :-)\n> \n> Great !\n>> Cheers,\n>> Stefan\n>>\n>>   include/libcamera/controls.h | 2 +-\n>>   src/libcamera/controls.cpp   | 9 ++++++---\n>>   2 files changed, 7 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index cf942055..0bf778d8 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -368,7 +368,7 @@ public:\n>>       std::size_t size() const { return controls_.size(); }\n>>         void clear() { controls_.clear(); }\n>> -    void merge(const ControlList &source);\n>> +    void merge(const ControlList &source, bool overwriteExisting =\n>> false);\n>>         bool contains(unsigned int id) const;\n>>   diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index b808116c..8ef29a96 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -910,10 +910,13 @@ ControlList::ControlList(const ControlInfoMap\n>> &infoMap,\n>>   /**\n>>    * \\brief Merge the \\a source into the ControlList\n>>    * \\param[in] source The ControlList to merge into this object\n>> + * \\param[in] overwriteExisting Control if existing elements in *this\n>> shall be\n>> + * overwritten\n>>    *\n>>    * Merging two control lists copies elements from the \\a source and\n>> inserts\n>>    * them in *this. If the \\a source contains elements whose key is\n>> already\n>> - * present in *this, then those elements are not overwritten.\n>> + * present in *this, then those elements are only overwritten if\n>> + * overwriteExisting is true.\n> \n> s/overwriteExisting/\\a overwriteExisting/\n>>    *\n>>    * Only control lists created from the same ControlIdMap or\n>> ControlInfoMap may\n>>    * be merged. Attempting to do otherwise results in undefined\n>> behaviour.\n>> @@ -921,7 +924,7 @@ ControlList::ControlList(const ControlInfoMap\n>> &infoMap,\n>>    * \\todo Reimplement or implement an overloaded version which\n>> internally uses\n>>    * std::unordered_map::merge() and accepts a non-const argument.\n>>    */\n>> -void ControlList::merge(const ControlList &source)\n>> +void ControlList::merge(const ControlList &source, bool\n>> overwriteExisting)\n>>   {\n>>       /**\n>>        * \\todo ASSERT that the current and source ControlList are derived\n>> @@ -936,7 +939,7 @@ void ControlList::merge(const ControlList &source)\n>>        */\n>>         for (const auto &ctrl : source) {\n>> -        if (contains(ctrl.first)) {\n>> +        if (!overwriteExisting && contains(ctrl.first)) {\n> \n> Looks good to me.\n> \n> Stefan, can a small unit test can cover this as well? I see,\n> \n>                 /*\n>                  * Create a new list with a new control and merge it\n> with the\n>                  * existing one, verifying that the existing controls\n>                  * values don't get overwritten.\n>                  */\n> \n> in test/controls/control_list.cpp, so probably worth adding this new\n> behaviour as well, to that test.\n>>               const ControlId *id = idmap_->at(ctrl.first);\n>>               LOG(Controls, Warning)\n>>                   << \"Control \" << id->name() << \" not overwritten\";\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 B2532C326B\n\tfor ;\n\tMon, 4 Mar 2024 15:43:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C828627FC;\n\tMon, 4 Mar 2024 16:43:34 +0100 (CET)","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 335CE627FC\n\tfor ;\n\tMon, 4 Mar 2024 16:43:32 +0100 (CET)","from [IPV6:2a00:6020:448c:6c00:560e:6da1:ef9c:32d0] (unknown\n\t[IPv6:2a00:6020:448c:6c00:560e:6da1:ef9c:32d0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A2EBBBD1;\n\tMon, 4 Mar 2024 16:43:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"soZbifTV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709566995;\n\tbh=5ll99pDRmTotH2SDWeVXT/tcjOonj9v0kx3tJw3xnJ4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=soZbifTVtHA+jDGsQZMrqMS7QR+FU1uKoDmN3qJhLi4UE3qfAWM63Us91M4WKzjq+\n\tv5eUwuPk+JTP+ivquIh6+YJgMikU3MxkRE/Gqd4WSrFkmyaiy7YRFFtucICJN6nNbc\n\t22ApyJaUCty6jW490r9LK5ex2y2RafArD9zaaqLI=","Message-ID":"<53dadbdb-a92d-4a83-b534-d8ba457326f7@ideasonboard.com>","Date":"Mon, 4 Mar 2024 16:43:28 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: controls: Add overwriteExisting parameter to\n\tControlList::merge","Content-Language":"en-US","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240229165753.154936-1-stefan.klug@ideasonboard.com>\n\t<5179f508-1597-4145-aa14-e5f893294f31@ideasonboard.com>","From":"Stefan Klug ","In-Reply-To":"<5179f508-1597-4145-aa14-e5f893294f31@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8","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\" "}}]