[{"id":16743,"web_url":"https://patchwork.libcamera.org/comment/16743/","msgid":"","date":"2021-05-03T12:25:27","subject":"Re: [libcamera-devel] [PATCH v5 01/17] libcamera: controls: Add a\n\tfunction to merge two control lists","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, May 03, 2021 at 12:41:36PM +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 | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 32 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..d8f104e3734b 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -874,6 +874,36 @@ 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 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> + *\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\ns/cost/const/\n\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\tconst ControlId *id = idmap_->at(ctrl.first);\n> +\t\t\tLOG(Controls, Warning)\n> +\t\t\t\t<< \"Control \" << id->name() << \" not overwritten\";\n> +\t\t\tcontinue;\n> +\t\t}\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","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 30F99BDE78\n\tfor ;\n\tMon, 3 May 2021 12:25:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A2B86890C;\n\tMon, 3 May 2021 14:25:31 +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 7906260511\n\tfor ;\n\tMon, 3 May 2021 14:25:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D475F87F;\n\tMon, 3 May 2021 14:25:28 +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=\"v8nvUj4s\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620044729;\n\tbh=2U2Mdg+uNd2JwUVCE3KAE7AbNdww0INia7GFgsvERzo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v8nvUj4saHXGblTIzpGg8ZeiYExEP4Onbq1gRT8Si2xwr3kvoJtrv9s0Bgn9r8iAK\n\t1BPsd+Sj4XNXA4gnlNC0lqv0LpmnFppq/mQMQDl/FdJYMOad/wjrOJMH5Pg6gPwphl\n\tAB5rkQkAJuGZkyvrBFDOQn5ci/aCZXyiJd8PQrR8=","Date":"Mon, 3 May 2021 15:25:27 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210503104152.34048-1-jacopo@jmondi.org>\n\t<20210503104152.34048-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210503104152.34048-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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":16750,"web_url":"https://patchwork.libcamera.org/comment/16750/","msgid":"","date":"2021-05-04T08:11:03","subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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,\n\nOn 03/05/2021 11:41, 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\nWith the cost/const fixed\n\nReviewed-by: Kieran Bingham \n\n> ---\n> include/libcamera/controls.h | 2 ++\n> src/libcamera/controls.cpp | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 32 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..d8f104e3734b 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -874,6 +874,36 @@ 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 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> + *\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\tconst ControlId *id = idmap_->at(ctrl.first);\n> +\t\t\tLOG(Controls, Warning)\n> +\t\t\t\t<< \"Control \" << id->name() << \" not overwritten\";\n> +\t\t\tcontinue;\n> +\t\t}\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 E1F75BDE79\n\tfor ;\n\tTue, 4 May 2021 08:11:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D53C68918;\n\tTue, 4 May 2021 10:11:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7444F6890C\n\tfor ;\n\tTue, 4 May 2021 10:11:08 +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 001EB580;\n\tTue, 4 May 2021 10:11:07 +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=\"jGyqcoii\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620115868;\n\tbh=c68vnuvWVQpAEBRPGM3zL0f9KIGZgabJUpENU/RsIRQ=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=jGyqcoii5YQmIXrj5nZVQupZ6zMH6tiKAV9HPc8eeeVH6HOKH+jwPgtFD3xLVDplJ\n\t9naJQ5rsQwPUe4+vesLqYnjdvV/LhVtkx2uMd209PWUqONZIlXcPkqu6DHJDZhQO1I\n\t1nIh7hpbXOVzgSCmZ+QsSQ44qeznJSyp/GeK+eUc=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210503104152.34048-1-jacopo@jmondi.org>\n\t<20210503104152.34048-2-jacopo@jmondi.org>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 4 May 2021 09:11:03 +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":"<20210503104152.34048-2-jacopo@jmondi.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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":16779,"web_url":"https://patchwork.libcamera.org/comment/16779/","msgid":"","date":"2021-05-06T04:58:34","subject":"Re: [libcamera-devel] [PATCH v5 01/17] libcamera: controls: Add a\n\tfunction to merge two control lists","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, May 4, 2021 at 5:11 PM Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Jacopo,\n>\n> On 03/05/2021 11:41, 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> With the cost/const fixed\n>\n> Reviewed-by: Kieran Bingham \n>\n> > ---\n> > include/libcamera/controls.h | 2 ++\n> > src/libcamera/controls.cpp | 30 ++++++++++++++++++++++++++++++\n> > 2 files changed, 32 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> > bool empty() const { return controls_.empty(); }\n> > std::size_t size() const { return controls_.size(); }\n> > +\n> > void clear() { controls_.clear(); }\n> > + void merge(const ControlList &source);\n> >\n> > bool contains(const ControlId &id) const;\n> > bool contains(unsigned int id) const;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index c58ed3946f3b..d8f104e3734b 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -874,6 +874,36 @@ ControlList::ControlList(const ControlInfoMap\n> &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\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> > + *\n> > + * Only control lists created from the same ControlIdMap or\n> ControlInfoMap may\n> > + * be merged. Attempting to do otherwise results in undefined behaviour.\n> > + *\n> > + * \\todo Reimplement or implement an overloaded version which\n> internally uses\n> > + * std::unordered_map::merge() and accepts a non-cost argument.\n> > + */\n> > +void ControlList::merge(const ControlList &source)\n> > +{\n> > + ASSERT(idmap_ == source.idmap_);\n>\n\nComparing std::unordered_map is O(NlogN) although the following for-loop is\nO(M), where N is the size of idmap_ and M is the size of source.\nWould you consider if this is worth doing?\n\n\n> > +\n> > + for (const auto &ctrl : source) {\n> > + if (contains(ctrl.first)) {\n> > + const ControlId *id = idmap_->at(ctrl.first);\n> > + LOG(Controls, Warning)\n>\n\nI wonder if this is worth Warning. Debug seems to be natural for me.\nCould you explain the reason?\n\nThanks,\n-Hiro\n\n> > + << \"Control \" << id->name() << \" not\n> overwritten\";\n> > + continue;\n> > + }\n> > +\n> > + set(ctrl.first, ctrl.second);\n> > + }\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\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 85EC5BDE7D\n\tfor ;\n\tThu, 6 May 2021 04:58:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D925F68911;\n\tThu, 6 May 2021 06:58:45 +0200 (CEST)","from mail-ej1-x629.google.com (mail-ej1-x629.google.com\n\t[IPv6:2a00:1450:4864:20::629])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA901688E5\n\tfor ;\n\tThu, 6 May 2021 06:58:44 +0200 (CEST)","by mail-ej1-x629.google.com with SMTP id n2so6294771ejy.7\n\tfor ;\n\tWed, 05 May 2021 21:58:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"nXJVzmUn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=sw0+TKlmqWGZ/OU4YoKhVWnTdTwSMyXk8IpO7ix5vwk=;\n\tb=nXJVzmUnFetms/b2iEHLQiYTtVzoJ2Gx6OChl/lY/xgC1y2quYjT8lEyEBbKoOGjB8\n\toN9sgGR3VxGMVBlumdujXH19/xOOGwDl4bScW7s4lECCbpswjFUImAzPaNHqTrzn2sBF\n\tllmcRLKBoJB+T4nLboDwHqkZQwixBlnh9vkyo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=sw0+TKlmqWGZ/OU4YoKhVWnTdTwSMyXk8IpO7ix5vwk=;\n\tb=tLr8atevvYb4qWW63a5VGJZL7eKVcALVs3CcmHVgcOrWOjJlPfZwQE85Mtdk3C47WU\n\tGDbcmSsu/SlT9H9tQrf4rZrrKvmQNvqfrn5w0q0sjsPrOtOZwX2/GoLPsE6u/XUp4YNj\n\t03cCdYlwV7T3H/KmSfEvrYKqNbHhcwvAqRp9sMtPkKsjHBTU1HbI50l0je3d2RYceljM\n\tlYAuxFTenINcfD6p1GZ/JCvFNJ55xEgcY/eYoEk3d2ljMIyDWB/uPayRpMAvBeRt6pa/\n\tVyZzyJ4BNFQIVGAxI7Kz6OsYavDjMih8qfkp0bzSMr4Q4GpGQ/0JUFFZGKSsw5GIVFa8\n\t9SXA==","X-Gm-Message-State":"AOAM531leTHCimJPtXaYIDcn4gYE3xj48WJSBwWBgJnevg3LG9kpLcg9\n\tJWt5eTKVm8RhBjcdWnVE8wU7D4pbwaY5iFTeAVjYsg==","X-Google-Smtp-Source":"ABdhPJy4yuiE4fFqijJSYqIQzYT39RZ+APF04UyMGz+HaQW/43nB8xhB67LSUtG0MuuZSP/t9Mxny173vYXKXupN1ms=","X-Received":"by 2002:a17:906:5acd:: with SMTP id\n\tx13mr2221831ejs.243.1620277124524; \n\tWed, 05 May 2021 21:58:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503104152.34048-1-jacopo@jmondi.org>\n\t<20210503104152.34048-2-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 6 May 2021 13:58:34 +0900","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============0627687646985584323==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16793,"web_url":"https://patchwork.libcamera.org/comment/16793/","msgid":"<20210506074512.iu4rcon4235plgy5@uno.localdomain>","date":"2021-05-06T07:45:12","subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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 Hiro,\n\nOn Thu, May 06, 2021 at 01:58:34PM +0900, Hirokazu Honda wrote:\n> On Tue, May 4, 2021 at 5:11 PM Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n>\n> > Hi Jacopo,\n> >\n> > On 03/05/2021 11:41, 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> > With the cost/const fixed\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > ---\n> > > include/libcamera/controls.h | 2 ++\n> > > src/libcamera/controls.cpp | 30 ++++++++++++++++++++++++++++++\n> > > 2 files changed, 32 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> > > bool empty() const { return controls_.empty(); }\n> > > std::size_t size() const { return controls_.size(); }\n> > > +\n> > > void clear() { controls_.clear(); }\n> > > + void merge(const ControlList &source);\n> > >\n> > > bool contains(const ControlId &id) const;\n> > > bool contains(unsigned int id) const;\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index c58ed3946f3b..d8f104e3734b 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -874,6 +874,36 @@ ControlList::ControlList(const ControlInfoMap\n> > &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\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> > > + *\n> > > + * Only control lists created from the same ControlIdMap or\n> > ControlInfoMap may\n> > > + * be merged. Attempting to do otherwise results in undefined behaviour.\n> > > + *\n> > > + * \\todo Reimplement or implement an overloaded version which\n> > internally uses\n> > > + * std::unordered_map::merge() and accepts a non-cost argument.\n> > > + */\n> > > +void ControlList::merge(const ControlList &source)\n> > > +{\n> > > + ASSERT(idmap_ == source.idmap_);\n> >\n>\n> Comparing std::unordered_map is O(NlogN) although the following for-loop is\n> O(M), where N is the size of idmap_ and M is the size of source.\n> Would you consider if this is worth doing?\n\nNot sure I got this. Is your concern due to the idmap comparison ? Do\nyou wonder if it is necessary ?\n\nIt seems to me if we skip the comparison, things might seem to work\nwell, as the control values mapped to ids are stored in controls_ not\nin idmap_.\n\nThing is, if you construct two control lists from two different\ncontrols maps you have a risk of id collision, in two different\nmaps the same ControlId can be mapped to a different unsigned int, or\nthe other way around, two different ControlId can be mapped to the\nsame numeric identifier.\n\nHow likely is this to happen at the current development state in\nmainline libcamera ? Close to 0 I would say.\n\nHowever we should consider downstream implementation, which might use\ncustom controls and control maps, especially in IPA. They could be\ntempted to mix them with lists constructed with the canonical\ncontrols::controls and if they're not particularly careful bad things\ncan happen and can be quite difficult to debug.\n\nConsidering the usual length of the control lists we deal with, I\nwould say this check is worth it, but maybe I'm being paranoid and we\ncan safely skip it.\n\nOr there might be better way to handle this, like\n\n for (const auto &ctrl : source) {\n if (idmap_[ctrl.first] != source.idmap_[ctrl.first])\n /* Error out ludly */\n\n\n }\n\nAlthough operator[] on an unordered map is linear in complexity in the\nworst case, so we would get a O(NM) complexity, which is slightly\nbetter than O(NlogNM). We're really talking details here, as with\nlists of up to a few tens of items, I don't think the difference is\nanyway relevant, but I appreciate the concern on staying as efficient\nas we can...\n\n>\n>\n> > > +\n> > > + for (const auto &ctrl : source) {\n> > > + if (contains(ctrl.first)) {\n> > > + const ControlId *id = idmap_->at(ctrl.first);\n> > > + LOG(Controls, Warning)\n> >\n>\n> I wonder if this is worth Warning. Debug seems to be natural for me.\n> Could you explain the reason?\n\nGood question, I went for Warning because I think this is a condition\nthat should not happen, as an attempt to overwrite an existing control\nthrough merging is suspicious and most probably not desired ? I\nrefrained from making this an error because it might be intentional,\nbut to me it's a condition that developers (mostly pipeline\ndevelopers) should be aware without going through the extensive Debug\nlog. As Warnings are less frequent, but ordinarily masked, I considered\nthis the right level. I can happily change this is if other debug\nlevels are considered better.\n\nThanks\n j\n\n>\n> Thanks,\n> -Hiro\n>\n> > > + << \"Control \" << id->name() << \" not\n> > overwritten\";\n> > > + continue;\n> > > + }\n> > > +\n> > > + set(ctrl.first, ctrl.second);\n> > > + }\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\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 52E37BF81D\n\tfor ;\n\tThu, 6 May 2021 07:44:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D55846890E;\n\tThu, 6 May 2021 09:44:30 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8BCF168909\n\tfor ;\n\tThu, 6 May 2021 09:44:29 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id CC865E000D;\n\tThu, 6 May 2021 07:44:28 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 6 May 2021 09:45:12 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210506074512.iu4rcon4235plgy5@uno.localdomain>","References":"<20210503104152.34048-1-jacopo@jmondi.org>\n\t<20210503104152.34048-2-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16794,"web_url":"https://patchwork.libcamera.org/comment/16794/","msgid":"","date":"2021-05-06T08:08:11","subject":"Re: [libcamera-devel] [PATCH v5 01/17] libcamera: controls: Add a\n\tfunction to merge two control lists","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, May 6, 2021 at 4:44 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Thu, May 06, 2021 at 01:58:34PM +0900, Hirokazu Honda wrote:\n> > On Tue, May 4, 2021 at 5:11 PM Kieran Bingham <\n> > kieran.bingham@ideasonboard.com> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > On 03/05/2021 11:41, 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> > > With the cost/const fixed\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > ---\n> > > > include/libcamera/controls.h | 2 ++\n> > > > src/libcamera/controls.cpp | 30 ++++++++++++++++++++++++++++++\n> > > > 2 files changed, 32 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h\n> 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> > > > bool empty() const { return controls_.empty(); }\n> > > > std::size_t size() const { return controls_.size(); }\n> > > > +\n> > > > void clear() { controls_.clear(); }\n> > > > + void merge(const ControlList &source);\n> > > >\n> > > > bool contains(const ControlId &id) const;\n> > > > bool contains(unsigned int id) const;\n> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > index c58ed3946f3b..d8f104e3734b 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -874,6 +874,36 @@ ControlList::ControlList(const ControlInfoMap\n> > > &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\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> > > > + *\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> > > > + *\n> > > > + * \\todo Reimplement or implement an overloaded version which\n> > > internally uses\n> > > > + * std::unordered_map::merge() and accepts a non-cost argument.\n> > > > + */\n> > > > +void ControlList::merge(const ControlList &source)\n> > > > +{\n> > > > + ASSERT(idmap_ == source.idmap_);\n> > >\n> >\n> > Comparing std::unordered_map is O(NlogN) although the following for-loop\n> is\n> > O(M), where N is the size of idmap_ and M is the size of source.\n> > Would you consider if this is worth doing?\n>\n> Not sure I got this. Is your concern due to the idmap comparison ? Do\n> you wonder if it is necessary ?\n>\n> It seems to me if we skip the comparison, things might seem to work\n> well, as the control values mapped to ids are stored in controls_ not\n> in idmap_.\n>\n> Thing is, if you construct two control lists from two different\n> controls maps you have a risk of id collision, in two different\n> maps the same ControlId can be mapped to a different unsigned int, or\n> the other way around, two different ControlId can be mapped to the\n> same numeric identifier.\n>\n> How likely is this to happen at the current development state in\n> mainline libcamera ? Close to 0 I would say.\n>\n> However we should consider downstream implementation, which might use\n> custom controls and control maps, especially in IPA. They could be\n> tempted to mix them with lists constructed with the canonical\n> controls::controls and if they're not particularly careful bad things\n> can happen and can be quite difficult to debug.\n>\n> Considering the usual length of the control lists we deal with, I\n> would say this check is worth it, but maybe I'm being paranoid and we\n> can safely skip it.\n>\n> Or there might be better way to handle this, like\n>\n> for (const auto &ctrl : source) {\n> if (idmap_[ctrl.first] != source.idmap_[ctrl.first])\n> /* Error out ludly */\n>\n>\n> }\n>\n> Although operator[] on an unordered map is linear in complexity in the\n> worst case, so we would get a O(NM) complexity, which is slightly\n> better than O(NlogNM). We're really talking details here, as with\n> lists of up to a few tens of items, I don't think the difference is\n> anyway relevant, but I appreciate the concern on staying as efficient\n> as we can...\n>\n>\nI missed the time complexity of std::unordered_map.\n\nComparing std::unordered_map is O(NlogN) although the following for-loop is\nO(M), where N is the size of idmap_ and M is the size of source.\n\nThis should be O(N) and O(M), respectively.\nSo the original code is better than your proposal one.\n\n>\n> >\n> > > > +\n> > > > + for (const auto &ctrl : source) {\n> > > > + if (contains(ctrl.first)) {\n> > > > + const ControlId *id = idmap_->at(ctrl.first);\n> > > > + LOG(Controls, Warning)\n> > >\n> >\n> > I wonder if this is worth Warning. Debug seems to be natural for me.\n> > Could you explain the reason?\n>\n> Good question, I went for Warning because I think this is a condition\n> that should not happen, as an attempt to overwrite an existing control\n> through merging is suspicious and most probably not desired ? I\n> refrained from making this an error because it might be intentional,\n> but to me it's a condition that developers (mostly pipeline\n> developers) should be aware without going through the extensive Debug\n> log. As Warnings are less frequent, but ordinarily masked, I considered\n> this the right level. I can happily change this is if other debug\n> levels are considered better.\n>\n>\nAck.\n\nReviewed-by: Hirokazu Honda \n\n\n> Thanks\n> j\n>\n> >\n> > Thanks,\n> > -Hiro\n> >\n> > > > + << \"Control \" << id->name() << \" not\n> > > overwritten\";\n> > > > + continue;\n> > > > + }\n> > > > +\n> > > > + set(ctrl.first, ctrl.second);\n> > > > + }\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Check if the list contains a control with the specified\n> \\a id\n> > > > * \\param[in] id The control ID\n> > > >\n> > >\n> > > --\n> > > Regards\n> > > --\n> > > Kieran\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 1BEB9BF81D\n\tfor ;\n\tThu, 6 May 2021 08:08:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D8E268919;\n\tThu, 6 May 2021 10:08:23 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3847368909\n\tfor ;\n\tThu, 6 May 2021 10:08:22 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id t4so7008764ejo.0\n\tfor ;\n\tThu, 06 May 2021 01:08:22 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"QpRyk9k+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=dAQVZY7fW5gg0q+CKXEWDM/bW5/9KFnUAl07quJ+ViU=;\n\tb=QpRyk9k+2U9z3bIjr6dZvjyWxeArQ6zFaPvhR1YApRJCe4F7xbnOLJWMxqam8qFGRO\n\tvHMe/85JAKylaL9QZFOg4MoXyXASaJFv7jQZel+lDUO1kRXOFYngF1KZYfyA3rTP7kYT\n\tjByBU/JrBAViJtiDsrupjvWytvQSc4JFApubk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dAQVZY7fW5gg0q+CKXEWDM/bW5/9KFnUAl07quJ+ViU=;\n\tb=hMLeuL8awOHcPdgJAAJnPW3JsTwVBJRDsfhGF9C7iurJxy+SWUqiZOx+6ix/2LqBFN\n\t9L7ro7Pw6kmCGAKCbbaLex44f0exMo6Zl+/XpYyki8W0GgA816dVzc/MH/S//S6g2mbV\n\tToe4ALwZPbN2md21viOrtq27Qjnli/kuFCJRZ0EblXtv9kWuKKiMuPjKkhlnH4z00VI1\n\tG9di/8MwFXYHnzuMYZFngUhyznIUsSzFq4S9OdmL3A5Sy2bpdkDtpb4o0PCWbnCvgpY+\n\toz912ipKuBUw3SYEsDHVlqn82gkLOIP5xyA15jGrcdMzyfTF3FngKbzwLjA3p54zi7yz\n\tNZVw==","X-Gm-Message-State":"AOAM532K4rxJfd4MgF+UDl66kEW0XdeWr7S2qtJOipVBKTun3uqrre2l\n\tDmvzzSciUdQv9oxslsLk2vHlakmbmRdbrLbKQeS3tg==","X-Google-Smtp-Source":"ABdhPJwicKSREGpa+Sa6xWxSj8BNWhOUNQaDo96jU+yVHM6f9MMWFnDQ9T4T1XSMBjZ9bbIXelzMqNdvyR74VVCQSaA=","X-Received":"by 2002:a17:906:2559:: with SMTP id\n\tj25mr2992669ejb.42.1620288501816; \n\tThu, 06 May 2021 01:08:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503104152.34048-1-jacopo@jmondi.org>\n\t<20210503104152.34048-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506074512.iu4rcon4235plgy5@uno.localdomain>","In-Reply-To":"<20210506074512.iu4rcon4235plgy5@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 6 May 2021 17:08:11 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============6195816429737728221==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16798,"web_url":"https://patchwork.libcamera.org/comment/16798/","msgid":"","date":"2021-05-06T08:50:38","subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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":"On 06/05/2021 09:08, Hirokazu Honda wrote:\n> Hi Jacopo,\n> \n> On Thu, May 6, 2021 at 4:44 PM Jacopo Mondi > wrote:\n> \n> Hi Hiro,\n> \n> On Thu, May 06, 2021 at 01:58:34PM +0900, Hirokazu Honda wrote:\n> > On Tue, May 4, 2021 at 5:11 PM Kieran Bingham <\n> > kieran.bingham@ideasonboard.com\n> > wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > On 03/05/2021 11:41, Jacopo Mondi wrote:\n> > > > From: Laurent Pinchart >\n> > > >\n> > > > Add a new ControlList::merge() function to merge two control\n> 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\n> populate\n> > > > with metadata received from an IPA.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart\n> >\n> > > > [reimplement the function by not using\n> std::unordered_map::merge()]\n> > > > Signed-off-by: Jacopo Mondi >\n> > >\n> > > With the cost/const fixed\n> > >\n> > > Reviewed-by: Kieran Bingham >\n> > >\n> > > > ---\n> > > >  include/libcamera/controls.h |  2 ++\n> > > >  src/libcamera/controls.cpp   | 30 ++++++++++++++++++++++++++++++\n> > > >  2 files changed, 32 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h\n> 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> > > >       bool empty() const { return controls_.empty(); }\n> > > >       std::size_t size() const { return controls_.size(); }\n> > > > +\n> > > >       void clear() { controls_.clear(); }\n> > > > +     void merge(const ControlList &source);\n> > > >\n> > > >       bool contains(const ControlId &id) const;\n> > > >       bool contains(unsigned int id) const;\n> > > > diff --git a/src/libcamera/controls.cpp\n> b/src/libcamera/controls.cpp\n> > > > index c58ed3946f3b..d8f104e3734b 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -874,6 +874,36 @@ ControlList::ControlList(const ControlInfoMap\n> > > &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\n> 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> > > > + *\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> > > > + *\n> > > > + * \\todo Reimplement or implement an overloaded version which\n> > > internally uses\n> > > > + * std::unordered_map::merge() and accepts a non-cost argument.\n> > > > + */\n> > > > +void ControlList::merge(const ControlList &source)\n> > > > +{\n> > > > +     ASSERT(idmap_ == source.idmap_);\n> > >\n> >\n> > Comparing std::unordered_map is O(NlogN) although the following\n> for-loop is\n> > O(M), where N is the size of idmap_ and M is the size of source.\n> > Would you consider if this is worth doing?\n> \n> Not sure I got this. Is your concern due to the idmap comparison ? Do\n> you wonder if it is necessary ?\n> \n> It seems to me if we skip the comparison, things might seem to work\n> well, as the control values mapped to ids are stored in controls_ not\n> in idmap_.\n> \n> Thing is, if you construct two control lists from two different\n> controls maps you have a risk of id collision, in two different\n> maps the same ControlId can be mapped to a different unsigned int, or\n> the other way around, two different ControlId can be mapped to the\n> same numeric identifier.\n> \n> How likely is this to happen at the current development state in\n> mainline libcamera ? Close to 0 I would say.\n> \n> However we should consider downstream implementation, which might use\n> custom controls and control maps, especially in IPA. They could be\n> tempted to mix them with lists constructed with the canonical\n> controls::controls and if they're not particularly careful bad things\n> can happen and can be quite difficult to debug.\n> \n> Considering the usual length of the control lists we deal with, I\n> would say this check is worth it, but maybe I'm being paranoid and we\n> can safely skip it.\n> \n> Or there might be better way to handle this, like\n> \n>         for (const auto &ctrl : source) {\n>                 if (idmap_[ctrl.first] != source.idmap_[ctrl.first])\n>                         /* Error out ludly */\n> \n> \n>         }\n> \n> Although operator[] on an unordered map is linear in complexity in the\n> worst case, so we would get a O(NM) complexity, which is slightly\n> better than O(NlogNM). We're really talking details here, as with\n> lists of up to a few tens of items, I don't think the difference is\n> anyway relevant, but I appreciate the concern on staying as efficient\n> as we can...\n> \n> \n> I missed the time complexity of std::unordered_map.\n> \n> Comparing std::unordered_map is O(NlogN) although the following\n> for-loop is\n> O(M), where N is the size of idmap_ and M is the size of source.\n> \n> This should be O(N) and O(M), respectively.\n> So the original code is better than your proposal one.\n\nI like the thorough investigations to find out the best way to compare a\nmap - but I'm weary it's a bit moot.\n\nIsn't this an incredibly cheap pointer comparison?\n\n\tASSERT(idmap_ == source.idmap_);\n\n\nOr is this not what we're querying here (it's hard to tell).\n\nThe idmap_ of a ControlList is of type:\n\nclass ControlList\n{\nprivate:\n\tconst ControlIdMap *idmap_;\n}\n\nSo the assert is guaranteeing that both control lists are using /exactly\nthe same/ id map, not that the contents of each idmap is the same.\n\n\n\n> \n> >\n> >\n> > > > +\n> > > > +     for (const auto &ctrl : source) {\n> > > > +             if (contains(ctrl.first)) {\n> > > > +                     const ControlId *id =\n> idmap_->at(ctrl.first);\n> > > > +                     LOG(Controls, Warning)\n> > >\n> >\n> > I wonder if this is worth Warning. Debug seems to be natural for me.\n> > Could you explain the reason?\n> \n> Good question, I went for Warning because I think this is a condition\n> that should not happen, as an attempt to overwrite an existing control\n> through merging is suspicious and most probably not desired ? I\n> refrained from making this an error because it might be intentional,\n> but to me it's a condition that developers (mostly pipeline\n> developers) should be aware without going through the extensive Debug\n> log. As Warnings are less frequent, but ordinarily masked, I considered\n> this the right level. I can happily change this is if other debug\n> levels are considered better.\n> \n\nI agree here, I think it's better to be louder here, because otherwise\nthe user could get behaviour they weren't expecting.\n\n\n> \n> Ack.\n> \n> Reviewed-by: Hirokazu Honda >\n>  \n> \n> Thanks\n>    j\n> \n> >\n> > Thanks,\n> > -Hiro\n> >\n> > > > +                             << \"Control \" << id->name() << \" not\n> > > overwritten\";\n> > > > +                     continue;\n> > > > +             }\n> > > > +\n> > > > +             set(ctrl.first, ctrl.second);\n> > > > +     }\n> > > > +}\n> > > > +\n> > > >  /**\n> > > >   * \\brief Check if the list contains a control with the\n> specified \\a id\n> > > >   * \\param[in] id The control ID\n> > > >\n> > >\n> > > --\n> > > Regards\n> > > --\n> > > Kieran\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> \n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 B0A8ABDE7F\n\tfor ;\n\tThu, 6 May 2021 08:50:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3458768921;\n\tThu, 6 May 2021 10:50:44 +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 914FC6891B\n\tfor ;\n\tThu, 6 May 2021 10:50:42 +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 E4C114A5;\n\tThu, 6 May 2021 10:50:41 +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=\"O0Xt7leN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620291042;\n\tbh=xmxrlQ2JHY3fUt/8ks+UxkHaO17hjtpP3tVFeJixdlY=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=O0Xt7leNiGICZUqOvIC1SZotRWbh2U/DFIK4OxAGitQNrCg9LvPK9H9DNXYdg1qNp\n\tE4SBcLnsb9X5WJ9C/oEzT4QjpW5JP8RoGdctML1NRkZPgnqPaihjP7oWC3gwPxsdDk\n\tI04f8iRSd1XL0QPnoKL5Beaa1oMrhBp6fuPy3i1Y=","To":"Hirokazu Honda , Jacopo Mondi ","References":"<20210503104152.34048-1-jacopo@jmondi.org>\n\t<20210503104152.34048-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210506074512.iu4rcon4235plgy5@uno.localdomain>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 6 May 2021 09:50:38 +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":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v5 01/17] 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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]