[{"id":28925,"web_url":"https://patchwork.libcamera.org/comment/28925/","msgid":"","date":"2024-03-12T09:45:18","subject":"Re: [PATCH v3] libcamera: controls: Add policy parameter to\n\tControlList::merge()","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Thu, Mar 07, 2024 at 11:27:38AM +0100, Stefan Klug wrote:\n> This is useful in many cases although not included in the stl.\n>\n> Note: This is an ABI incompatible change.\n>\n> Signed-off-by: Stefan Klug \n> Reviewed-by: Umang Jain \n> ---\n> Changes in v2:\n> - Added unittest and fixes doxygen docs.\n> Changes in v3:\n> - Replace boolean param by enum for better readability\n> - Added note on ABI breakage\n>\n> include/libcamera/controls.h | 7 ++++-\n> src/libcamera/controls.cpp | 20 ++++++++++++--\n> test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++\n> 3 files changed, 73 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index cf942055..82b95599 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -352,6 +352,11 @@ private:\n> \tusing ControlListMap = std::unordered_map;\n>\n> public:\n> +\tenum class MergePolicy {\n> +\t\tKeepExisting = 0,\n> +\t\tOverwriteExisting,\n> +\t};\n> +\n> \tControlList();\n> \tControlList(const ControlIdMap &idmap, const ControlValidator *validator = nullptr);\n> \tControlList(const ControlInfoMap &infoMap, const ControlValidator *validator = nullptr);\n> @@ -368,7 +373,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, MergePolicy policy = MergePolicy::KeepExisting);\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..16d3547c 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -907,13 +907,27 @@ ControlList::ControlList(const ControlInfoMap &infoMap,\n> * \\brief Removes all controls from the list\n> */\n>\n> +/**\n> + * \\enum ControlList::MergePolicy\n> + * \\brief The policy used by the merge function\n> + *\n> + * \\var ControlList::MergePolicy::KeepExisting\n> + * \\brief Existing controls in the target list are kept\n> + *\n> + * \\var ControlList::MergePolicy::OverwriteExisting\n> + * \\brief Existing controls in the target list are updated\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 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 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> @@ -921,7 +935,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, MergePolicy policy)\n> {\n> \t/**\n> \t * \\todo ASSERT that the current and source ControlList are derived\n> @@ -936,7 +950,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 (policy == MergePolicy::KeepExisting && 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..e86df49f 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, ControlList::MergePolicy::OverwriteExisting);\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\nI guess the error is that the \"control value -changed-\" as you're\nexpecting it to be 1.1f which is the value you have initialized it to\n\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\nAh see, this is right\n\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> \t\treturn TestPass;\n\nNit aparts\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t}\n> };\n> --\n> 2.40.1\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 6EF75BD1F1\n\tfor ;\n\tTue, 12 Mar 2024 09:45:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8D1362C80;\n\tTue, 12 Mar 2024 10:45:23 +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 A75146286C\n\tfor ;\n\tTue, 12 Mar 2024 10:45:21 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A4A816BE;\n\tTue, 12 Mar 2024 10:44:59 +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=\"HWX6tY4r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710236699;\n\tbh=txeK+fyKngiDrs/NVJGLruAkeYRsChS9dYek55TsCTY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HWX6tY4rdn9cAxfpP44nXdY2ZYq7jRe5rfwoS/wNTDRiRaDwawBIZYjF3IJJQoH0q\n\tlyh/SU3Yls4ltE3wWGECcV/Yq+XhvZ2ZZFS3ZkZ5OYp4i4DzDv23j3l04cjl7YTD0z\n\tA09zJ6H3tRuklzcoV4HjbKkwM9/cL20djt3lfyDs=","Date":"Tue, 12 Mar 2024 10:45:18 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Subject":"Re: [PATCH v3] libcamera: controls: Add policy parameter to\n\tControlList::merge()","Message-ID":"","References":"<20240307102738.55324-1-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240307102738.55324-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\" "}}]