[{"id":16704,"web_url":"https://patchwork.libcamera.org/comment/16704/","msgid":"","date":"2021-04-30T19:04:47","subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo/Laurent,\n\nOn 30/04/2021 17:00, Jacopo Mondi wrote:\n> From: Laurent Pinchart \n> \n> Add a new ControlList::merge() function to merge two control lists by\n> copying in the values in the list passed as parameters.\n> \n> This can be used by pipeline handlers to merge metadata they populate\n> with metadata received from an IPA.\n> \n> Signed-off-by: Laurent Pinchart \n> [reimplement the function by not using std::unordered_map::merge()]\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/controls.h | 2 ++\n> src/libcamera/controls.cpp | 28 ++++++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1a5690a5ccbe..1c9b37e617bc 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -363,7 +363,9 @@ public:\n> \n> \tbool empty() const { return controls_.empty(); }\n> \tstd::size_t size() const { return controls_.size(); }\n> +\n> \tvoid clear() { controls_.clear(); }\n> +\tvoid merge(const ControlList &source);\n> \n> \tbool contains(const ControlId &id) const;\n> \tbool contains(unsigned int id) const;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index c58ed3946f3b..154ca7bfe7c1 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -874,6 +874,34 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida\n> * \\brief Removes all controls from the list\n> */\n> \n> +/**\n> + * \\brief Merge the \\a source into the ControlList\n> + * \\param[in] source The ControlList to merge into this object\n> + *\n> + * Merging two control lists copies elements from the \\a source and insert\n\ns/insert/inserts/\n\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> + * identical to std::unordered_map::merge() in that only internal pointers to\n\nIs there something missing before 'identical'? It's following a period,\nbut doesn't seem to be an opening statement for a sentence...\n\n> + * nodes are updated, without copying or moving the elements.\n\nThis sounds concerning. Can we leak memory or rather point to memory\nwhich we don't own, and could therefore be released ?\n\nThis paragraph seems to conflict with itself.\nIt starts off by saying \"copies elements from source ...\"\nAnd then says \"without copying or moving\"\n\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-cost argument.\n> + */\n> +void ControlList::merge(const ControlList &source)\n> +{\n> +\tASSERT(idmap_ == source.idmap_);\n> +\n> +\tfor (const auto &ctrl : source) {\n> +\t\tif (contains(ctrl.first))\n> +\t\t\tcontinue;\n> +\n> +\t\tset(ctrl.first, ctrl.second);\n> +\t}\n> +}\n> +\n> /**\n> * \\brief Check if the list contains a control with the specified \\a id\n> * \\param[in] id The control ID\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 D64B7BDE4D\n\tfor ;\n\tFri, 30 Apr 2021 19:04:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 485F868901;\n\tFri, 30 Apr 2021 21:04:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 84D4F688A5\n\tfor ;\n\tFri, 30 Apr 2021 21:04:50 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E9BBA45F;\n\tFri, 30 Apr 2021 21:04:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S7oKo0cY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619809490;\n\tbh=WQkKRQB/PSivS7jlPitfLhqrfc1Wb+RgS3Kq6lyTCkg=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=S7oKo0cYChzztYliIHvTp2a3iywdLkiLa7jEBrUQ3QVgl54bdxhCIIU6/h3lp3POL\n\tfwdpVdkDPo98ZcvyvxqUQMRydnmwblF2YGRf4cwCcvKFyQq5YqlOEQDwTREHfPrPO/\n\to8idC+/6kqcyIvyiuAcyqcZ8EaZWIgs0qNMESkXg=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210430160026.190724-1-jacopo@jmondi.org>\n\t<20210430160026.190724-2-jacopo@jmondi.org>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 30 Apr 2021 20:04:47 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210430160026.190724-2-jacopo@jmondi.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16715,"web_url":"https://patchwork.libcamera.org/comment/16715/","msgid":"","date":"2021-05-01T06:41:55","subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-04-30 18:00:11 +0200, Jacopo Mondi wrote:\n> From: Laurent Pinchart \n> \n> Add a new ControlList::merge() function to merge two control lists by\n> copying in the values in the list passed as parameters.\n> \n> This can be used by pipeline handlers to merge metadata they populate\n> with metadata received from an IPA.\n> \n> Signed-off-by: Laurent Pinchart \n> [reimplement the function by not using std::unordered_map::merge()]\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/controls.h | 2 ++\n> src/libcamera/controls.cpp | 28 ++++++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1a5690a5ccbe..1c9b37e617bc 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -363,7 +363,9 @@ public:\n> \n> \tbool empty() const { return controls_.empty(); }\n> \tstd::size_t size() const { return controls_.size(); }\n> +\n> \tvoid clear() { controls_.clear(); }\n> +\tvoid merge(const ControlList &source);\n> \n> \tbool contains(const ControlId &id) const;\n> \tbool contains(unsigned int id) const;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index c58ed3946f3b..154ca7bfe7c1 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -874,6 +874,34 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida\n> * \\brief Removes all controls from the list\n> */\n> \n> +/**\n> + * \\brief Merge the \\a source into the ControlList\n> + * \\param[in] source The ControlList to merge into this object\n> + *\n> + * Merging two control lists copies elements from the \\a source and insert\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> + * identical to std::unordered_map::merge() in that only internal pointers to\n> + * nodes are updated, without copying or moving the elements.\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-cost argument.\n> + */\n> +void ControlList::merge(const ControlList &source)\n> +{\n> +\tASSERT(idmap_ == source.idmap_);\n> +\n> +\tfor (const auto &ctrl : source) {\n> +\t\tif (contains(ctrl.first))\n> +\t\t\tcontinue;\n\nnit1: Should not 'source' have priority over 'this'? If I'm merging \nsomething to 'this' I would expect the new stuff to overwrite my \nexisting state.\n\nnit2: Maybe add a debug log here? It feels like an issue with the \npotential to reduce someone to tears, \"I set the control in the IPA but \nmy application still sees the old value, why?\"\n\n> +\n> +\t\tset(ctrl.first, ctrl.second);\n> +\t}\n> +}\n> +\n> /**\n> * \\brief Check if the list contains a control with the specified \\a id\n> * \\param[in] id The control ID\n> -- \n> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 34025BDE6B\n\tfor ;\n\tSat, 1 May 2021 06:41:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A8B9D60511;\n\tSat, 1 May 2021 08:41:58 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2CD0C602BF\n\tfor ;\n\tSat, 1 May 2021 08:41:57 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id 124so427334lff.5\n\tfor ;\n\tFri, 30 Apr 2021 23:41:57 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ta28sm494328lfb.88.2021.04.30.23.41.55\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 30 Apr 2021 23:41:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"luCQL/Ky\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=Y/gTMm+O5aWsihGRVf835hTDSAu86TjoopQAUvhkG/w=;\n\tb=luCQL/KyustuCgn3+RtpmAml9Z/u+Jh81FstJwJmfSrvClvhuhcVkYcLGFtK47M57C\n\tv6AHkn8d+8nL5MZPnEjzOs/zjkYyosRvsAsItfhXXmWo0TEFg6MKXtOAo6hBxGW/Cy16\n\tAkbbmOzAnXzGhsgfScxBq76pHMGexGO54es7vVGf6g1UsvFRVsIRoRu82Dr4MdSypyA0\n\tVbx5NUAuBTF48jzrV9GthG0aXCbAxVBMXYuCl7B1pVfmZa5vlUruy4T5AKhCz4SJFUh9\n\t2ja599FOgK4LgqYaTNtnPlToKNi0w9nXi0Aw6tWXOFqJhI2vSgzSmVSqKMQnIJfeU0Xy\n\tv6LA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=Y/gTMm+O5aWsihGRVf835hTDSAu86TjoopQAUvhkG/w=;\n\tb=H+EWRyjvOeFE5pWNtjCq4rive28cf6HQX0zWAebqByd4TFKOtqFvJtzb+2qmGnDaez\n\tmDnMz71ZPMi9cq+IxUORCd8WTCYnMaTFPaRIAyj2A3OR1N1HkuSioG+31OT6EJberuvf\n\t40j9qsLdRHQp93q9cZO6B0vJd3nVGDlh3HjG/zy4EOJjBY3/OCPnuXOQ4wtDVQFD4vWv\n\t7/wtNiL7Q4EgaIVfvSptgqsz11isYVB/JT9Ek7JLqABVDNTzz2sUDx/mU759T3VcqK2v\n\tthQq01C/wooHM9GPCWcii7wk1XBq+Z28AxOQeaoeGq/1ehDykashhaxohcYhYZWo6yY+\n\tpELA==","X-Gm-Message-State":"AOAM533CdRIAb8XJiae+EaA/EugH376H7eyiLKX+s5eWDGBX/R3P9QhN\n\tg01f3uYNcJH58hRpP6EjxvZmS7XI/uaRyw==","X-Google-Smtp-Source":"ABdhPJy2vR5IvxLHa5RnRTKFAeKqUQ28PKoisEhLgXHMhDHiTaFfdBss8oxnHUZy9FhAoXBjsjeiOA==","X-Received":"by 2002:ac2:510d:: with SMTP id q13mr6129694lfb.75.1619851316644;\n\tFri, 30 Apr 2021 23:41:56 -0700 (PDT)","Date":"Sat, 1 May 2021 08:41:55 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210430160026.190724-1-jacopo@jmondi.org>\n\t<20210430160026.190724-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210430160026.190724-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16720,"web_url":"https://patchwork.libcamera.org/comment/16720/","msgid":"<20210501075853.tbo4pwgzcqjssdzu@uno.localdomain>","date":"2021-05-01T07:58:53","subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Apr 30, 2021 at 08:04:47PM +0100, Kieran Bingham wrote:\n> Hi Jacopo/Laurent,\n>\n> On 30/04/2021 17:00, Jacopo Mondi wrote:\n> > From: Laurent Pinchart \n> >\n> > Add a new ControlList::merge() function to merge two control lists by\n> > copying in the values in the list passed as parameters.\n> >\n> > This can be used by pipeline handlers to merge metadata they populate\n> > with metadata received from an IPA.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > [reimplement the function by not using std::unordered_map::merge()]\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/controls.h | 2 ++\n> > src/libcamera/controls.cpp | 28 ++++++++++++++++++++++++++++\n> > 2 files changed, 30 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1a5690a5ccbe..1c9b37e617bc 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -363,7 +363,9 @@ public:\n> >\n> > \tbool empty() const { return controls_.empty(); }\n> > \tstd::size_t size() const { return controls_.size(); }\n> > +\n> > \tvoid clear() { controls_.clear(); }\n> > +\tvoid merge(const ControlList &source);\n> >\n> > \tbool contains(const ControlId &id) const;\n> > \tbool contains(unsigned int id) const;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index c58ed3946f3b..154ca7bfe7c1 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -874,6 +874,34 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida\n> > * \\brief Removes all controls from the list\n> > */\n> >\n> > +/**\n> > + * \\brief Merge the \\a source into the ControlList\n> > + * \\param[in] source The ControlList to merge into this object\n> > + *\n> > + * Merging two control lists copies elements from the \\a source and insert\n>\n> s/insert/inserts/\n>\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> > + * identical to std::unordered_map::merge() in that only internal pointers to\n>\n> Is there something missing before 'identical'? It's following a period,\n> but doesn't seem to be an opening statement for a sentence...\n\nSorry, my bad, I kept the older version documentation around and\nforgot to clean it up, and clearly forgot to re-read carefully enough.\n\nThis last lines, and the below one will have to be removed.\n\n>\n> > + * nodes are updated, without copying or moving the elements.\n>\n> This sounds concerning. Can we leak memory or rather point to memory\n> which we don't own, and could therefore be released ?\n\nThis statement describes the std::unordered_map::merge() semantic. It\nwill be removed\n\n>\n> This paragraph seems to conflict with itself.\n> It starts off by saying \"copies elements from source ...\"\n> And then says \"without copying or moving\"\n>\n\nmy bad :(\n\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-cost argument.\n> > + */\n> > +void ControlList::merge(const ControlList &source)\n> > +{\n> > +\tASSERT(idmap_ == source.idmap_);\n> > +\n> > +\tfor (const auto &ctrl : source) {\n> > +\t\tif (contains(ctrl.first))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tset(ctrl.first, ctrl.second);\n> > +\t}\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the list contains a control with the specified \\a id\n> > * \\param[in] id The control ID\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 3C4EEBDE6A\n\tfor ;\n\tSat, 1 May 2021 07:58:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAE45688CA;\n\tSat, 1 May 2021 09:58:12 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E73AE688AE\n\tfor ;\n\tSat, 1 May 2021 09:58:10 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 73DB640005;\n\tSat, 1 May 2021 07:58:10 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Sat, 1 May 2021 09:58:53 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210501075853.tbo4pwgzcqjssdzu@uno.localdomain>","References":"<20210430160026.190724-1-jacopo@jmondi.org>\n\t<20210430160026.190724-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16722,"web_url":"https://patchwork.libcamera.org/comment/16722/","msgid":"<20210501080843.7p4syxoc5qnkyokw@uno.localdomain>","date":"2021-05-01T08:08:43","subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sat, May 01, 2021 at 08:41:55AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-04-30 18:00:11 +0200, Jacopo Mondi wrote:\n> > From: Laurent Pinchart \n> >\n> > Add a new ControlList::merge() function to merge two control lists by\n> > copying in the values in the list passed as parameters.\n> >\n> > This can be used by pipeline handlers to merge metadata they populate\n> > with metadata received from an IPA.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > [reimplement the function by not using std::unordered_map::merge()]\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/controls.h | 2 ++\n> > src/libcamera/controls.cpp | 28 ++++++++++++++++++++++++++++\n> > 2 files changed, 30 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1a5690a5ccbe..1c9b37e617bc 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -363,7 +363,9 @@ public:\n> >\n> > \tbool empty() const { return controls_.empty(); }\n> > \tstd::size_t size() const { return controls_.size(); }\n> > +\n> > \tvoid clear() { controls_.clear(); }\n> > +\tvoid merge(const ControlList &source);\n> >\n> > \tbool contains(const ControlId &id) const;\n> > \tbool contains(unsigned int id) const;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index c58ed3946f3b..154ca7bfe7c1 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -874,6 +874,34 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida\n> > * \\brief Removes all controls from the list\n> > */\n> >\n> > +/**\n> > + * \\brief Merge the \\a source into the ControlList\n> > + * \\param[in] source The ControlList to merge into this object\n> > + *\n> > + * Merging two control lists copies elements from the \\a source and insert\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> > + * identical to std::unordered_map::merge() in that only internal pointers to\n> > + * nodes are updated, without copying or moving the elements.\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-cost argument.\n> > + */\n> > +void ControlList::merge(const ControlList &source)\n> > +{\n> > +\tASSERT(idmap_ == source.idmap_);\n> > +\n> > +\tfor (const auto &ctrl : source) {\n> > +\t\tif (contains(ctrl.first))\n> > +\t\t\tcontinue;\n>\n> nit1: Should not 'source' have priority over 'this'? If I'm merging\n> something to 'this' I would expect the new stuff to overwrite my\n> existing state.\n\nThat's an interesting question... I decided to here maintain the\nsemantic of std::unordere_map::merge() here, as if we'll provide an\noptimized version based on it, the semantic regarding overwriting does\nnot change.\n\n>\n> nit2: Maybe add a debug log here? It feels like an issue with the\n> potential to reduce someone to tears, \"I set the control in the IPA but\n> my application still sees the old value, why?\"\n>\n\nYes, I'm not sure what's best... I considered the idea of returning an\ninteger to report the number of controls that has not been overwritten\n(or the number of the overwritten ones alternatively) but that would\nnot allow you to know which ones are duplicated in the two lists...\n\nA debug message could work equally well, probably better if it doesn't\nget too noisy...\n\nI'll add one\n\nThanks\n j\n\n> > +\n> > +\t\tset(ctrl.first, ctrl.second);\n> > +\t}\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the list contains a control with the specified \\a id\n> > * \\param[in] id The control ID\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 A9112BDE6C\n\tfor ;\n\tSat, 1 May 2021 08:08:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FCE16890E;\n\tSat, 1 May 2021 10:08:02 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC240688AE\n\tfor ;\n\tSat, 1 May 2021 10:08:00 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 3328D200007;\n\tSat, 1 May 2021 08:07:59 +0000 (UTC)"],"Date":"Sat, 1 May 2021 10:08:43 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210501080843.7p4syxoc5qnkyokw@uno.localdomain>","References":"<20210430160026.190724-1-jacopo@jmondi.org>\n\t<20210430160026.190724-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a\n\tfunction to merge two control lists","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]