[{"id":28846,"web_url":"https://patchwork.libcamera.org/comment/28846/","msgid":"<36f9218e-492e-4ca2-b514-63afc00214dd@ideasonboard.com>","date":"2024-03-05T08:12:16","subject":"Re: [PATCH v2] libcamera: controls: Add overwriteExisting parameter\n\tto ControlList::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 04/03/24 9:28 pm, Stefan Klug wrote:\n> This is useful in many cases although not included in the stl.\n>\n> Signed-off-by: Stefan Klug \n\nnit: Subject line, missing ():\n\"libcamera: controls: Add overwriteExisting parameter to \nControlList::merge()\"\n\nCan be applied while merging, no need for v3 in my opinion,\n\nLGTM,\n\nReviewed-by: Umang Jain \n\n> ---\n>\n> This version adds a unittest and fixes a doxygen doc.\n>\n> include/libcamera/controls.h | 2 +-\n> src/libcamera/controls.cpp | 9 ++++--\n> test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++\n> 3 files changed, 57 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..93ce4efe 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> + * \\a overwriteExisting is true.\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> \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\";\n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index c03f230e..304cabfc 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -196,6 +196,56 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * Create two lists with overlapping controls. Merge them with\n> +\t\t * overwriteExisting = true, verifying that the existing control\n> +\t\t * values *get* overwritten.\n> +\t\t */\n> +\t\tmergeList.clear();\n> +\t\tmergeList.set(controls::Brightness, 0.7f);\n> +\t\tmergeList.set(controls::Saturation, 0.4f);\n> +\n> +\t\tlist.clear();\n> +\t\tlist.set(controls::Brightness, 0.5f);\n> +\t\tlist.set(controls::Contrast, 1.1f);\n> +\n> +\t\tmergeList.merge(list, true);\n> +\t\tif (mergeList.size() != 3) {\n> +\t\t\tcout << \"Merged list should contain three elements\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (list.size() != 2) {\n> +\t\t\tcout << \"The list to merge should contain two elements\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!mergeList.get(controls::Brightness) ||\n> +\t\t !mergeList.get(controls::Contrast) ||\n> +\t\t !mergeList.get(controls::Saturation)) {\n> +\t\t\tcout << \"Merged list does not contain all controls\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (mergeList.get(controls::Brightness) != 0.5f) {\n> +\t\t\tcout << \"Brightness control value did not change after merging lists\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (mergeList.get(controls::Contrast) != 1.1f) {\n> +\t\t\tcout << \"Contrast control value did not change after merging lists\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (mergeList.get(controls::Saturation) != 0.4f) {\n> +\t\t\tcout << \"Saturation control value changed after merging lists\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> \t\treturn TestPass;\n> \t}\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 A8553BD160\n\tfor ;\n\tTue, 5 Mar 2024 08:12:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8AF162868;\n\tTue, 5 Mar 2024 09:12:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B54962865\n\tfor ;\n\tTue, 5 Mar 2024 09:12:24 +0100 (CET)","from [192.168.1.102] (unknown [103.251.226.116])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23EFBCC8;\n\tTue, 5 Mar 2024 09:12:05 +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=\"hraBYM5P\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709626327;\n\tbh=XiuXJQdAlqn1c44tQ/CDVtE1/xwOcvrXvuYsKnzZEPE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=hraBYM5PmHJWi69fAmHHSfoWsQlzxVOdjyuOXcqW6/5i7280N/VzJIr8M3I1AyMjJ\n\t9wZ5aXgaVJVfU2r84XrUNAE7l6EQt2HPiaAdXyDfzu8pRU1kQswhYWWjxd+XAuLhEO\n\t3pShtwjbkuZ1XKpUNesN0erMrS9PTq0ONzWnc1cA=","Message-ID":"<36f9218e-492e-4ca2-b514-63afc00214dd@ideasonboard.com>","Date":"Tue, 5 Mar 2024 13:42:16 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] libcamera: controls: Add overwriteExisting parameter\n\tto ControlList::merge","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240304155852.105079-1-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240304155852.105079-1-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":28852,"web_url":"https://patchwork.libcamera.org/comment/28852/","msgid":"<20240305083813.GG12503@pendragon.ideasonboard.com>","date":"2024-03-05T08:38:13","subject":"Re: [PATCH v2] libcamera: controls: Add overwriteExisting parameter\n\tto ControlList::merge","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Mon, Mar 04, 2024 at 04:58:53PM +0100, Stefan Klug wrote:\n> This is useful in many cases although not included in the stl.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> \n> This version adds a unittest and fixes a doxygen doc.\n> \n> include/libcamera/controls.h | 2 +-\n> src/libcamera/controls.cpp | 9 ++++--\n> test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++\n> 3 files changed, 57 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\nThe way to implement a.merge(b, true) with C++ STL would be\n\n\tb.merge(a);\n\tb.swap(a);\n\nI understand this is not ideal though, and that a nicer API would be,\nwell, nicer. I am however concerned with the bool flag, as it isn't very\nreadable. Consider the following code:\n\n\ta.merge(b);\n\ta.merge(b, false);\n\ta.merge(b, true);\n\nReaders can't easily tell what happens. An alternative would be to use\nan enum.\n\nclass ControList\n{\n\t...\n\n\tenum class MergePolicy {\n\t\tIgnore,\n\t\tOverwrite,\n\t};\n\n\tvoid merge(const ControlList &source,\n\t\t MergePolicy policy = MergePolicy::Ignore);\n\n\t...\n};\n\n(bikeshedding on the names is allowed)\n\nWith that, reading\n\n\ta.merge(b);\n\ta.merge(b, ControlList::MergePolicy::Ignore);\n\ta.merge(b, ControlList::MergePolicy::Overwrite);\n\nbecomes clearer. You still can't tell what the default is (that's the\ntrouble with default parameters), but the second and third line are much\nmore explicit.\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 b808116c..93ce4efe 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> + * \\a overwriteExisting is true.\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> \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\";\n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index c03f230e..304cabfc 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -196,6 +196,56 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * Create two lists with overlapping controls. Merge them with\n> +\t\t * overwriteExisting = true, verifying that the existing control\n> +\t\t * values *get* overwritten.\n> +\t\t */\n> +\t\tmergeList.clear();\n> +\t\tmergeList.set(controls::Brightness, 0.7f);\n> +\t\tmergeList.set(controls::Saturation, 0.4f);\n> +\n> +\t\tlist.clear();\n> +\t\tlist.set(controls::Brightness, 0.5f);\n> +\t\tlist.set(controls::Contrast, 1.1f);\n> +\n> +\t\tmergeList.merge(list, true);\n> +\t\tif (mergeList.size() != 3) {\n> +\t\t\tcout << \"Merged list should contain three elements\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (list.size() != 2) {\n> +\t\t\tcout << \"The list to merge should contain two elements\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!mergeList.get(controls::Brightness) ||\n> +\t\t !mergeList.get(controls::Contrast) ||\n> +\t\t !mergeList.get(controls::Saturation)) {\n> +\t\t\tcout << \"Merged list does not contain all controls\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (mergeList.get(controls::Brightness) != 0.5f) {\n> +\t\t\tcout << \"Brightness control value did not change after merging lists\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (mergeList.get(controls::Contrast) != 1.1f) {\n> +\t\t\tcout << \"Contrast control value did not change after merging lists\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (mergeList.get(controls::Saturation) != 0.4f) {\n> +\t\t\tcout << \"Saturation control value changed after merging lists\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> \t\treturn TestPass;\n> \t}\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 F2580BD160\n\tfor ;\n\tTue, 5 Mar 2024 08:38:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4857962871;\n\tTue, 5 Mar 2024 09:38:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CFE2F62868\n\tfor ;\n\tTue, 5 Mar 2024 09:38:11 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C56ADCC8;\n\tTue, 5 Mar 2024 09:37:54 +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=\"tvwlnIyj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709627875;\n\tbh=MZcJLuwKhuHkzBeZ2NALSjVZ4t4u+5bcLofZO1EJrhE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tvwlnIyjC4VvLscANFmIk12h5tdB9++7nlDQ0dlHxoZiykocg5hPUTU+ijEQ7HLnK\n\twMcdBNcYtgkJrShRk37o4imHWNH9a7Q2gC1TsIttNUrExW8CawE/HAzswde+pnpd1e\n\tis0KT9RSOq0uekS7gjEiLe396WVJsiAkUESR/vy8=","Date":"Tue, 5 Mar 2024 10:38:13 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Subject":"Re: [PATCH v2] libcamera: controls: Add overwriteExisting parameter\n\tto ControlList::merge","Message-ID":"<20240305083813.GG12503@pendragon.ideasonboard.com>","References":"<20240304155852.105079-1-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240304155852.105079-1-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28860,"web_url":"https://patchwork.libcamera.org/comment/28860/","msgid":"<787ca474-7684-48eb-9e9e-686340180b0c@ideasonboard.com>","date":"2024-03-05T09:28:41","subject":"Re: [PATCH v2] libcamera: controls: Add overwriteExisting parameter\n\tto ControlList::merge","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nthanks for your comment.\nAm 05.03.24 um 09:38 schrieb Laurent Pinchart:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 04, 2024 at 04:58:53PM +0100, Stefan Klug wrote:\n>> This is useful in many cases although not included in the stl.\n>>\n>> Signed-off-by: Stefan Klug \n>> ---\n>>\n>> This version adds a unittest and fixes a doxygen doc.\n>>\n>> include/libcamera/controls.h | 2 +-\n>> src/libcamera/controls.cpp | 9 ++++--\n>> test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++\n>> 3 files changed, 57 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> The way to implement a.merge(b, true) with C++ STL would be\n> \n> \tb.merge(a);\n> \tb.swap(a);\n> \n> I understand this is not ideal though, and that a nicer API would be,\n> well, nicer. I am however concerned with the bool flag, as it isn't very\n> readable. Consider the following code:\n> \n> \ta.merge(b);\n> \ta.merge(b, false);\n> \ta.merge(b, true);\n> \n> Readers can't easily tell what happens. An alternative would be to use\n> an enum.>\n> class ControList\n> {\n> \t...\n> \n> \tenum class MergePolicy {\n> \t\tIgnore,\n> \t\tOverwrite,\n> \t};\n> \n> \tvoid merge(const ControlList &source,\n> \t\t MergePolicy policy = MergePolicy::Ignore);\n> \n> \t...\n> };\n> \n> (bikeshedding on the names is allowed)\n> \n> With that, reading\n> \n> \ta.merge(b);\n> \ta.merge(b, ControlList::MergePolicy::Ignore);\n> \ta.merge(b, ControlList::MergePolicy::Overwrite);\n\nYes, I agree that the boolean isn't readable. Another option would be to\njust create an additional function. I fail to come up with a nice name\n(maybe mergeWithOverwrite() ). That would have the added benefit to not\nbreak the ABI but would add code duplication. The more I think about it,\nthe enum feels better.\n\nIf we go for the enum, what about calling the first entry \"Keep\" or even\nKeepDuplicates and OverwriteDuplicates.\n\nI'm fine with either solution. Please give me q quick heads-up which way\nyou'd prefer.\n\nCheers Stefan\n\n> \n> becomes clearer. You still can't tell what the default is (that's the\n> trouble with default parameters), but the second and third line are much\n> more explicit.\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 b808116c..93ce4efe 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>> + * \\a overwriteExisting is true.\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>> \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\";\n>> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n>> index c03f230e..304cabfc 100644\n>> --- a/test/controls/control_list.cpp\n>> +++ b/test/controls/control_list.cpp\n>> @@ -196,6 +196,56 @@ protected:\n>> \t\t\treturn TestFail;\n>> \t\t}\n>> \n>> +\t\t/*\n>> +\t\t * Create two lists with overlapping controls. Merge them with\n>> +\t\t * overwriteExisting = true, verifying that the existing control\n>> +\t\t * values *get* overwritten.\n>> +\t\t */\n>> +\t\tmergeList.clear();\n>> +\t\tmergeList.set(controls::Brightness, 0.7f);\n>> +\t\tmergeList.set(controls::Saturation, 0.4f);\n>> +\n>> +\t\tlist.clear();\n>> +\t\tlist.set(controls::Brightness, 0.5f);\n>> +\t\tlist.set(controls::Contrast, 1.1f);\n>> +\n>> +\t\tmergeList.merge(list, true);\n>> +\t\tif (mergeList.size() != 3) {\n>> +\t\t\tcout << \"Merged list should contain three elements\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tif (list.size() != 2) {\n>> +\t\t\tcout << \"The list to merge should contain two elements\"\n>> +\t\t\t << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tif (!mergeList.get(controls::Brightness) ||\n>> +\t\t !mergeList.get(controls::Contrast) ||\n>> +\t\t !mergeList.get(controls::Saturation)) {\n>> +\t\t\tcout << \"Merged list does not contain all controls\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tif (mergeList.get(controls::Brightness) != 0.5f) {\n>> +\t\t\tcout << \"Brightness control value did not change after merging lists\"\n>> +\t\t\t << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tif (mergeList.get(controls::Contrast) != 1.1f) {\n>> +\t\t\tcout << \"Contrast control value did not change after merging lists\"\n>> +\t\t\t << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tif (mergeList.get(controls::Saturation) != 0.4f) {\n>> +\t\t\tcout << \"Saturation control value changed after merging lists\"\n>> +\t\t\t << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> \t\treturn TestPass;\n>> \t}\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 5A593C326B\n\tfor ;\n\tTue, 5 Mar 2024 09:28:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 12DEB62874;\n\tTue, 5 Mar 2024 10:28:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0417A62865\n\tfor ;\n\tTue, 5 Mar 2024 10:28:45 +0100 (CET)","from [IPV6:2a00:6020:448c:6c00:aace:771d:49e7:e0b0] (unknown\n\t[IPv6:2a00:6020:448c:6c00:aace:771d:49e7:e0b0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35743A27;\n\tTue, 5 Mar 2024 10:28:28 +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=\"aDjwp8KA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709630908;\n\tbh=ypr9pjuacYkXO8llr8qhPtlRRo81qU2oU0czfVLVZ30=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=aDjwp8KA+fJosHB65L5ewHCysYlvzWVs9sa7Hje0jGLCaNCpN4/vs98giOkN3yi0u\n\t8Qx9NAquPXV6S+3RGfZMOstHmvS2QQca/M5uR+0B2sCirZU9puNMD4OXzOT3FlfecD\n\t74aazmt0m03psg2u/oFupi7b45kZrSbG57ZuVYO4=","Message-ID":"<787ca474-7684-48eb-9e9e-686340180b0c@ideasonboard.com>","Date":"Tue, 5 Mar 2024 10:28:41 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] libcamera: controls: Add overwriteExisting parameter\n\tto ControlList::merge","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20240304155852.105079-1-stefan.klug@ideasonboard.com>\n\t<20240305083813.GG12503@pendragon.ideasonboard.com>","From":"Stefan Klug ","In-Reply-To":"<20240305083813.GG12503@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]