[{"id":1976,"web_url":"https://patchwork.libcamera.org/comment/1976/","msgid":"<20190623153501.GD32581@bigcity.dyn.berto.se>","date":"2019-06-23T15:35:01","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nThanks for your patch.\n\nOn 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n> Define a ControlList which wraps the STL std::unordered_map to contain\n> a list of associative pairs of ControlInfo and Value items.\n> \n> A ControlInfo (or represented by its ControlId) together with a Value\n> provide a control item which can be interpreted by the internal IPA and\n> PipelineHandler components.\n> \n> To pass a set of controls around, a ControlList can be added to a\n> Request or stored locally to establish a set of default values for later\n> reuse.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/controls.h | 41 +++++++++++\n> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 169 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 95198d41c4cf..5835b840a31b 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -4,6 +4,9 @@\n> *\n> * controls.h - Control handling\n> */\n> +\n> +#include \n> +\n> #ifndef __LIBCAMERA_CONTROLS_H__\n> #define __LIBCAMERA_CONTROLS_H__\n> \n> @@ -53,6 +56,44 @@ private:\n> \n> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> \n> +class ControlList\n> +{\n> +private:\n> +\tstruct ControlInfoHasher {\n> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n> +\t\t{\n> +\t\t\treturn std::hash()(ci.id());\n> +\t\t}\n> +\t};\n> +\n> +\ttypedef std::unordered_map ControlListMap;\n\nIs this needed to go first or can it be moved down to the other private \nsection?\n\n> +\n> +public:\n> +\tControlList();\n> +\n> +\tusing iterator = ControlListMap::iterator;\n> +\tusing const_iterator = ControlListMap::const_iterator;\n> +\n> +\titerator begin() { return controls_.begin(); }\n> +\titerator end() { return controls_.end(); }\n> +\tconst_iterator begin() const { return controls_.begin(); }\n> +\tconst_iterator end() const { return controls_.end(); }\n> +\n> +\tbool contains(ControlId id) const { return controls_.count(id); };\n\nWould not return controls_.find(id) != controls_.end() be better?\n\n> +\tbool empty() const { return controls_.empty(); }\n> +\tsize_t size() const { return controls_.size(); }\n> +\tvoid reset() { controls_.clear(); }\n> +\n> +\tValue &operator[](ControlId id) { return controls_[id]; }\n> +\n> +\tvoid update(const ControlList &list);\n> +\tvoid update(enum ControlId id, const Value &value);\n> +\tvoid update(const ControlInfo &info, const Value &value);\n> +\n> +private:\n> +\tControlListMap controls_;\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_CONTROLS_H__ */\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index b1be46ddb55e..7c55afffe4ca 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> \treturn stream << info.toString();\n> }\n> \n> +/**\n> + * \\class ControlList\n> + * \\brief Associates a list of ControlIds with their values.\n> + *\n> + * The ControlList stores a pair of ControlInfo and Value classes as an\n> + * unordered map. Entries can be updated using array syntax such as\n> + * myControlList[MyControlId] = myValue;\n> + * or\n> + * myNewValue = myControlList[MyControlId];\n\nI don't think this will update the value.\n\nI think you can drop the examples and the last sentence from the \ndocumentation.\n\nWith these issues addressed,\n\nReviewed-by: Niklas Söderlund \n\n> + */\n> +\n> +/**\n> + * \\brief Construct a ControlList\n> + */\n> +ControlList::ControlList()\n> +{\n> +}\n> +\n> +/**\n> + * \\typedef ControlList::iterator\n> + * \\brief Iterator for the controls contained within the list.\n> + */\n> +\n> +/**\n> + * \\typedef ControlList::const_iterator\n> + * \\brief Const iterator for the controls contained within the list.\n> + */\n> +\n> +/**\n> + * \\fn iterator ControlList::begin()\n> + * \\brief Retrieve an iterator to the first Control in the list\n> + * \\return An iterator to the first Control in the list\n> + */\n> +\n> +/**\n> + * \\fn iterator ControlList::end()\n> + * \\brief Retrieve an iterator to the next element after the last controls in\n> + * the instance.\n> + * \\return An iterator to the element following the last control in the instance\n> + */\n> +\n> +/**\n> + * \\fn const_iterator ControlList::begin() const\n> + * \\brief Retrieve a const_iterator to the first Control in the list\n> + * \\return A const_iterator to the first Control in the list\n> + */\n> +\n> +/**\n> + * \\fn const_iterator ControlList::end() const\n> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n> + * \\return A const iterator to the element following the last control in the\n> + * instance\n> + */\n> +\n> +/**\n> + * \\fn ControlList::contains()\n> + * \\brief Identify if the List contains a control with the specified ControlId\n> + * \\return True if the list contains a matching control, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn ControlList::empty()\n> + * \\brief Identify if the list is empty\n> + * \\return True if the list does not contain any control, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn ControlList::size()\n> + * \\brief Retrieve the number of controls in the list\n> + * \\return The number of Control entries stored in the list\n> + */\n> +\n> +/**\n> + * \\fn ControlList::reset()\n> + * \\brief Removes all controls from the list\n> + */\n> +\n> +/**\n> + * \\fn ControlList::operator[]()\n> + * \\brief Access the control with the given Id\n> + * \\param id The id of the control to access\n> + * \\return The Value of the control with a ControlId of \\a Id\n> + */\n> +\n> +/**\n> + * \\brief Update all Control values with the value from the given \\a list\n> + * \\param list The list of controls to update or append to this list\n> + *\n> + * Update all controls in the ControlList, by the values given by \\a list\n> + * If the list already contains a control of this ID then it will be overwritten\n> + */\n> +void ControlList::update(const ControlList &list)\n> +{\n> +\tfor (auto it : list) {\n> +\t\tconst ControlInfo &info = it.first;\n> +\t\tconst Value &value = it.second;\n> +\n> +\t\tcontrols_[info] = value;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Update a control value in the list\n> + * \\param id The ControlId of the control to update\n> + * \\param value The new value for the updated control\n> + *\n> + * Set the Control value in the list to the appropriate value\n> + * If the list already contains a control of this ID then it will be overwritten\n> + */\n> +void ControlList::update(enum ControlId id, const Value &value)\n> +{\n> +\tcontrols_[id] = value;\n> +}\n> +\n> +/**\n> + * \\brief Update a control value in the list.\n> + * \\param info The ControlInfo of the control to update\n> + * \\param value The new value for the updated control\n> + *\n> + * Set the Control value in the list to the appropriate value.\n> + * If the list already contains a control of the Id matching the ControlInfo\n> + * then it will be overwritten.\n> + */\n> +void ControlList::update(const ControlInfo &info, const Value &value)\n> +{\n> +\tcontrols_[info] = value;\n> +}\n> +\n> } /* namespace libcamera */\n> -- \n> 2.20.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":"","Received":["from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F2DD6157E\n\tfor ;\n\tSun, 23 Jun 2019 17:35:03 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id v24so10165435ljg.13\n\tfor ;\n\tSun, 23 Jun 2019 08:35:03 -0700 (PDT)","from localhost (customer-145-14-112-32.stosn.net. [145.14.112.32])\n\tby smtp.gmail.com with ESMTPSA id\n\tw1sm1507088ljm.81.2019.06.23.08.35.01\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSun, 23 Jun 2019 08:35:01 -0700 (PDT)"],"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\t:user-agent; bh=aNVAdE+yq8qDkSFqfVI4lhcg+Euj076udhqlL5GRF84=;\n\tb=X7MgvqPL8iBTGmOk4dq+a0OxJZ5xOG+jR8EzFmf5Ebd9M7dqPCg/MUbITTcUj8qTbj\n\tx5ubj5vHzfzH+ZOIj/UbL+whxrVCDeTNFNQE/C64bjPLUV1mGLounW4PCcmqzz/XVKIZ\n\tdz2SaPxfhceYgiLi62CLk0n5xpinFt6bG/FZ3h+jizIjG3fplEjV2UKEggnOGrIeXUNi\n\t9QXjmZnfSl4pGdg+wI760lQL3Ppd9MRhD5TNnvsfLIrW4JoQMJ3WZVF09LLlhlloeyfl\n\tvLspoXoGiHceg9dJOUzdTi/NShzsC/NIQO3TkzmJYHzo/16rTRVPdn1ZdfaZkXZzJAYM\n\t6pJg==","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:user-agent;\n\tbh=aNVAdE+yq8qDkSFqfVI4lhcg+Euj076udhqlL5GRF84=;\n\tb=LSAmmROA5a18nQHiLM1OdTL0pXONEJg6vLzifZKwlm0vH/EXoWCmqwHpN9D6dj0Cxr\n\tNVu2PO0g4kHdPuePdAnYih2JmTYCogf24QXRs93iKTtb8/euSxzw+5rTXzbu+0wDKH7s\n\t51YiJrLaGkdp2g+ogbSrWzZEuzTFdEhmWdjbRrmhgugGa2DiiTRdlTFyXZQ+u3fN1PQz\n\tURPiaKjBSdc9rnXK8i/4lyodrM4rOrmSk5HXdR1Xkr192PqqdgP8a0XqR+6iug+axFzs\n\tZNpen/Uek4wfC2rYHeZq65O0Sa+T63w9L+vUlJ6x6b2/i+Yzq5A/ALa3koa0uAq7OJTf\n\tBVog==","X-Gm-Message-State":"APjAAAVAVjBtmoxwDxoXeZdo8QyTzI4LB2pBVOYFuwIzRPWBmvOINEUm\n\tNRZtqKKvl/w9Psral4wlj1gbiA==","X-Google-Smtp-Source":"APXvYqxMo0VLCo2T5D3BusIy8Y8DGdwQfdv8vlJP+8XpnmMXhJgny1yT+l2RXFaXHUVxbPCNA2msoA==","X-Received":"by 2002:a2e:298a:: with SMTP id\n\tp10mr49115477ljp.74.1561304102486; \n\tSun, 23 Jun 2019 08:35:02 -0700 (PDT)","Date":"Sun, 23 Jun 2019 17:35:01 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190623153501.GD32581@bigcity.dyn.berto.se>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190621161401.28337-6-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 23 Jun 2019 15:35:03 -0000"}},{"id":1982,"web_url":"https://patchwork.libcamera.org/comment/1982/","msgid":"<20190623222132.GE6124@pendragon.ideasonboard.com>","date":"2019-06-23T22:21:32","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Sun, Jun 23, 2019 at 05:35:01PM +0200, Niklas Söderlund wrote:\n> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n> > Define a ControlList which wraps the STL std::unordered_map to contain\n> > a list of associative pairs of ControlInfo and Value items.\n> > \n> > A ControlInfo (or represented by its ControlId) together with a Value\n> > provide a control item which can be interpreted by the internal IPA and\n> > PipelineHandler components.\n> > \n> > To pass a set of controls around, a ControlList can be added to a\n\n\ns/ / /\n\n> > Request or stored locally to establish a set of default values for later\n> > reuse.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/controls.h | 41 +++++++++++\n> > src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n> > 2 files changed, 169 insertions(+)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 95198d41c4cf..5835b840a31b 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -4,6 +4,9 @@\n> > *\n> > * controls.h - Control handling\n> > */\n> > +\n> > +#include \n> > +\n> > #ifndef __LIBCAMERA_CONTROLS_H__\n> > #define __LIBCAMERA_CONTROLS_H__\n> > \n> > @@ -53,6 +56,44 @@ private:\n> > \n> > std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> > \n> > +class ControlList\n> > +{\n> > +private:\n> > +\tstruct ControlInfoHasher {\n> > +\t\tstd::size_t operator()(const ControlInfo &ci) const\n> > +\t\t{\n> > +\t\t\treturn std::hash()(ci.id());\n> > +\t\t}\n> > +\t};\n> > +\n> > +\ttypedef std::unordered_map ControlListMap;\n\nWe usually use the C++11 syntax\n\n\tusing ControlListMap = std::unordered_map;\n\nI don't think you should copy the ControlInfo in the ControlList. If I\nunderstand your patch series correctly, ControlInfo should be created\nwith the camera, and remain constant for the lifetime of the camera. The\nmap should thus use either the control ID or a pointer to the control\ninfo as the key.\n\n> Is this needed to go first or can it be moved down to the other private \n> section?\n> \n> > +\n> > +public:\n> > +\tControlList();\n> > +\n> > +\tusing iterator = ControlListMap::iterator;\n> > +\tusing const_iterator = ControlListMap::const_iterator;\n> > +\n> > +\titerator begin() { return controls_.begin(); }\n> > +\titerator end() { return controls_.end(); }\n> > +\tconst_iterator begin() const { return controls_.begin(); }\n> > +\tconst_iterator end() const { return controls_.end(); }\n> > +\n> > +\tbool contains(ControlId id) const { return controls_.count(id); };\n> \n> Would not return controls_.find(id) != controls_.end() be better?\n> \n> > +\tbool empty() const { return controls_.empty(); }\n> > +\tsize_t size() const { return controls_.size(); }\n\nstd::size_t\n\n> > +\tvoid reset() { controls_.clear(); }\n\nShould we call this clear() ?\n\n> > +\n> > +\tValue &operator[](ControlId id) { return controls_[id]; }\n> > +\n> > +\tvoid update(const ControlList &list);\n> > +\tvoid update(enum ControlId id, const Value &value);\n> > +\tvoid update(const ControlInfo &info, const Value &value);\n> > +\n> > +private:\n> > +\tControlListMap controls_;\n> > +};\n> > +\n> > } /* namespace libcamera */\n> > \n> > #endif /* __LIBCAMERA_CONTROLS_H__ */\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index b1be46ddb55e..7c55afffe4ca 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> > \treturn stream << info.toString();\n> > }\n> > \n> > +/**\n> > + * \\class ControlList\n> > + * \\brief Associates a list of ControlIds with their values.\n> > + *\n> > + * The ControlList stores a pair of ControlInfo and Value classes as an\n> > + * unordered map. Entries can be updated using array syntax such as\n> > + * myControlList[MyControlId] = myValue;\n> > + * or\n> > + * myNewValue = myControlList[MyControlId];\n> \n> I don't think this will update the value.\n> \n> I think you can drop the examples and the last sentence from the \n> documentation.\n> \n> With these issues addressed,\n> \n> Reviewed-by: Niklas Söderlund \n> \n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a ControlList\n> > + */\n> > +ControlList::ControlList()\n> > +{\n> > +}\n> > +\n\nIf the constructor is empty and is the only one you can omit it, the\ncompiler will generate one for your.\n\n> > +/**\n> > + * \\typedef ControlList::iterator\n> > + * \\brief Iterator for the controls contained within the list.\n> > + */\n> > +\n> > +/**\n> > + * \\typedef ControlList::const_iterator\n> > + * \\brief Const iterator for the controls contained within the list.\n> > + */\n> > +\n> > +/**\n> > + * \\fn iterator ControlList::begin()\n> > + * \\brief Retrieve an iterator to the first Control in the list\n> > + * \\return An iterator to the first Control in the list\n> > + */\n> > +\n> > +/**\n> > + * \\fn iterator ControlList::end()\n> > + * \\brief Retrieve an iterator to the next element after the last controls in\n> > + * the instance.\n> > + * \\return An iterator to the element following the last control in the instance\n> > + */\n> > +\n> > +/**\n> > + * \\fn const_iterator ControlList::begin() const\n> > + * \\brief Retrieve a const_iterator to the first Control in the list\n> > + * \\return A const_iterator to the first Control in the list\n> > + */\n> > +\n> > +/**\n> > + * \\fn const_iterator ControlList::end() const\n> > + * \\brief Retrieve a constant iterator pointing to an empty element after the\n> > + * \\return A const iterator to the element following the last control in the\n> > + * instance\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlList::contains()\n> > + * \\brief Identify if the List contains a control with the specified ControlId\n> > + * \\return True if the list contains a matching control, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlList::empty()\n> > + * \\brief Identify if the list is empty\n> > + * \\return True if the list does not contain any control, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlList::size()\n> > + * \\brief Retrieve the number of controls in the list\n> > + * \\return The number of Control entries stored in the list\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlList::reset()\n> > + * \\brief Removes all controls from the list\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlList::operator[]()\n> > + * \\brief Access the control with the given Id\n> > + * \\param id The id of the control to access\n> > + * \\return The Value of the control with a ControlId of \\a Id\n> > + */\n> > +\n> > +/**\n> > + * \\brief Update all Control values with the value from the given \\a list\n> > + * \\param list The list of controls to update or append to this list\n> > + *\n> > + * Update all controls in the ControlList, by the values given by \\a list\n> > + * If the list already contains a control of this ID then it will be overwritten\n> > + */\n> > +void ControlList::update(const ControlList &list)\n> > +{\n> > +\tfor (auto it : list) {\n> > +\t\tconst ControlInfo &info = it.first;\n> > +\t\tconst Value &value = it.second;\n> > +\n> > +\t\tcontrols_[info] = value;\n> > +\t}\n> > +}\n> > +\n> > +/**\n> > + * \\brief Update a control value in the list\n> > + * \\param id The ControlId of the control to update\n> > + * \\param value The new value for the updated control\n> > + *\n> > + * Set the Control value in the list to the appropriate value\n> > + * If the list already contains a control of this ID then it will be overwritten\n> > + */\n> > +void ControlList::update(enum ControlId id, const Value &value)\n> > +{\n> > +\tcontrols_[id] = value;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Update a control value in the list.\n> > + * \\param info The ControlInfo of the control to update\n> > + * \\param value The new value for the updated control\n> > + *\n> > + * Set the Control value in the list to the appropriate value.\n> > + * If the list already contains a control of the Id matching the ControlInfo\n> > + * then it will be overwritten.\n> > + */\n> > +void ControlList::update(const ControlInfo &info, const Value &value)\n> > +{\n> > +\tcontrols_[info] = value;\n> > +}\n\nDo we really need this class ? Can't we just expose \n\n\tusing ControlList = std::unordered_map;\n\n? The last two update functions would be replaced by usage of\noperator[], and the first one with\n\ntemplate< class InputIt >\nvoid std::unordered_map::insert( InputIt first, InputIt last );\n\n> > +\n> > } /* namespace libcamera */","headers":{"Return-Path":"","Received":["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 BD2BC6157E\n\tfor ;\n\tMon, 24 Jun 2019 00:24:01 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33AD9500;\n\tMon, 24 Jun 2019 00:24:01 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561328641;\n\tbh=wnkKGmWeXASSewy14VgMfqTeMrP+jZjwAjUj2iU3HuA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oqNmgBxfQHHPmsOc2Ffvq+kkB5iQFbPcLR+tYotAATTX63AG0jxGiqlkzONxkziOg\n\ti3QxW32zBTX/2xauClkL72+ARLA3MaC5SsgU3GHvFruzaqgc5V2GKHhtW+cY+EA9aA\n\tPCQje/mZ7a+78dxljhVUF+dFIs0W7DCTfpk/yrsM=","Date":"Mon, 24 Jun 2019 01:21:32 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tLibCamera Devel ","Message-ID":"<20190623222132.GE6124@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190623153501.GD32581@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 23 Jun 2019 22:24:02 -0000"}},{"id":2006,"web_url":"https://patchwork.libcamera.org/comment/2006/","msgid":"","date":"2019-06-24T15:48:05","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 23/06/2019 16:35, Niklas Söderlund wrote:\n> Hi Kieran,\n> \n> Thanks for your patch.\n> \n> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n>> Define a ControlList which wraps the STL std::unordered_map to contain\n>> a list of associative pairs of ControlInfo and Value items.\n>>\n>> A ControlInfo (or represented by its ControlId) together with a Value\n>> provide a control item which can be interpreted by the internal IPA and\n>> PipelineHandler components.\n>>\n>> To pass a set of controls around, a ControlList can be added to a\n>> Request or stored locally to establish a set of default values for later\n>> reuse.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/controls.h | 41 +++++++++++\n>> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n>> 2 files changed, 169 insertions(+)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 95198d41c4cf..5835b840a31b 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -4,6 +4,9 @@\n>> *\n>> * controls.h - Control handling\n>> */\n>> +\n>> +#include \n>> +\n>> #ifndef __LIBCAMERA_CONTROLS_H__\n>> #define __LIBCAMERA_CONTROLS_H__\n>> \n>> @@ -53,6 +56,44 @@ private:\n>> \n>> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n>> \n>> +class ControlList\n>> +{\n>> +private:\n>> +\tstruct ControlInfoHasher {\n>> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n>> +\t\t{\n>> +\t\t\treturn std::hash()(ci.id());\n>> +\t\t}\n>> +\t};\n>> +\n>> +\ttypedef std::unordered_map ControlListMap;\n> \n> Is this needed to go first or can it be moved down to the other private \n> section?\n\nIt has to be defined before the usages below.\n\n> \n>> +\n>> +public:\n>> +\tControlList();\n>> +\n>> +\tusing iterator = ControlListMap::iterator;\n>> +\tusing const_iterator = ControlListMap::const_iterator;\n>> +\n>> +\titerator begin() { return controls_.begin(); }\n>> +\titerator end() { return controls_.end(); }\n>> +\tconst_iterator begin() const { return controls_.begin(); }\n>> +\tconst_iterator end() const { return controls_.end(); }\n>> +\n>> +\tbool contains(ControlId id) const { return controls_.count(id); };\n> \n> Would not return controls_.find(id) != controls_.end() be better?\n\nI thought this was elegant.\n\nCount == 0 = false,\ncount > 0 = true...\n\n\n>> +\tbool empty() const { return controls_.empty(); }\n>> +\tsize_t size() const { return controls_.size(); }\n>> +\tvoid reset() { controls_.clear(); }\n>> +\n>> +\tValue &operator[](ControlId id) { return controls_[id]; }\n>> +\n>> +\tvoid update(const ControlList &list);\n>> +\tvoid update(enum ControlId id, const Value &value);\n>> +\tvoid update(const ControlInfo &info, const Value &value);\n>> +\n>> +private:\n>> +\tControlListMap controls_;\n>> +};\n>> +\n>> } /* namespace libcamera */\n>> \n>> #endif /* __LIBCAMERA_CONTROLS_H__ */\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index b1be46ddb55e..7c55afffe4ca 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n>> \treturn stream << info.toString();\n>> }\n>> \n>> +/**\n>> + * \\class ControlList\n>> + * \\brief Associates a list of ControlIds with their values.\n>> + *\n>> + * The ControlList stores a pair of ControlInfo and Value classes as an\n>> + * unordered map. Entries can be updated using array syntax such as\n>> + * myControlList[MyControlId] = myValue;\n>> + * or\n>> + * myNewValue = myControlList[MyControlId];\n> \n> I don't think this will update the value.\n\nNo - it updates myNewValue. I'll just remove this. I was trying to put\nin that the the [] is the accessor.\n\n\n> \n> I think you can drop the examples and the last sentence from the \n> documentation.\n> \n> With these issues addressed,\n> \n> Reviewed-by: Niklas Söderlund \n> \n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a ControlList\n>> + */\n>> +ControlList::ControlList()\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\typedef ControlList::iterator\n>> + * \\brief Iterator for the controls contained within the list.\n>> + */\n>> +\n>> +/**\n>> + * \\typedef ControlList::const_iterator\n>> + * \\brief Const iterator for the controls contained within the list.\n>> + */\n>> +\n>> +/**\n>> + * \\fn iterator ControlList::begin()\n>> + * \\brief Retrieve an iterator to the first Control in the list\n>> + * \\return An iterator to the first Control in the list\n>> + */\n>> +\n>> +/**\n>> + * \\fn iterator ControlList::end()\n>> + * \\brief Retrieve an iterator to the next element after the last controls in\n>> + * the instance.\n>> + * \\return An iterator to the element following the last control in the instance\n>> + */\n>> +\n>> +/**\n>> + * \\fn const_iterator ControlList::begin() const\n>> + * \\brief Retrieve a const_iterator to the first Control in the list\n>> + * \\return A const_iterator to the first Control in the list\n>> + */\n>> +\n>> +/**\n>> + * \\fn const_iterator ControlList::end() const\n>> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n>> + * \\return A const iterator to the element following the last control in the\n>> + * instance\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlList::contains()\n>> + * \\brief Identify if the List contains a control with the specified ControlId\n>> + * \\return True if the list contains a matching control, false otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlList::empty()\n>> + * \\brief Identify if the list is empty\n>> + * \\return True if the list does not contain any control, false otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlList::size()\n>> + * \\brief Retrieve the number of controls in the list\n>> + * \\return The number of Control entries stored in the list\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlList::reset()\n>> + * \\brief Removes all controls from the list\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlList::operator[]()\n>> + * \\brief Access the control with the given Id\n>> + * \\param id The id of the control to access\n>> + * \\return The Value of the control with a ControlId of \\a Id\n>> + */\n>> +\n>> +/**\n>> + * \\brief Update all Control values with the value from the given \\a list\n>> + * \\param list The list of controls to update or append to this list\n>> + *\n>> + * Update all controls in the ControlList, by the values given by \\a list\n>> + * If the list already contains a control of this ID then it will be overwritten\n>> + */\n>> +void ControlList::update(const ControlList &list)\n>> +{\n>> +\tfor (auto it : list) {\n>> +\t\tconst ControlInfo &info = it.first;\n>> +\t\tconst Value &value = it.second;\n>> +\n>> +\t\tcontrols_[info] = value;\n>> +\t}\n>> +}\n>> +\n>> +/**\n>> + * \\brief Update a control value in the list\n>> + * \\param id The ControlId of the control to update\n>> + * \\param value The new value for the updated control\n>> + *\n>> + * Set the Control value in the list to the appropriate value\n>> + * If the list already contains a control of this ID then it will be overwritten\n>> + */\n>> +void ControlList::update(enum ControlId id, const Value &value)\n>> +{\n>> +\tcontrols_[id] = value;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Update a control value in the list.\n>> + * \\param info The ControlInfo of the control to update\n>> + * \\param value The new value for the updated control\n>> + *\n>> + * Set the Control value in the list to the appropriate value.\n>> + * If the list already contains a control of the Id matching the ControlInfo\n>> + * then it will be overwritten.\n>> + */\n>> +void ControlList::update(const ControlInfo &info, const Value &value)\n>> +{\n>> +\tcontrols_[info] = value;\n>> +}\n>> +\n>> } /* namespace libcamera */\n>> -- \n>> 2.20.1\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","Received":["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 011AD61580\n\tfor ;\n\tMon, 24 Jun 2019 17:48:08 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 74A642C6;\n\tMon, 24 Jun 2019 17:48:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561391288;\n\tbh=NA9f+mlSeM/B7X7HfV/rShTV31QP9eAX6XhKGBXmuPI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=R3qT38UUatsv1i2b9FWZnQfZ3iY7gW+aJ0qNkJH6WDFcyrIAF1o3A6xMmSt+1oYx9\n\tRPPU3H+yAuscTHFv86zpGPRLxej2tedatacvftjT6BfbTZswCuANhaIYLJlegO0xpt\n\tOuhN3UAuBuWUxVhf53WJdjwDDpT8ua2p4kKiq67w=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Cc":"LibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 24 Jun 2019 16:48:05 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190623153501.GD32581@bigcity.dyn.berto.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 15:48:09 -0000"}},{"id":2008,"web_url":"https://patchwork.libcamera.org/comment/2008/","msgid":"","date":"2019-06-24T16:02:55","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 23/06/2019 23:21, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Sun, Jun 23, 2019 at 05:35:01PM +0200, Niklas Söderlund wrote:\n>> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n>>> Define a ControlList which wraps the STL std::unordered_map to contain\n>>> a list of associative pairs of ControlInfo and Value items.\n>>>\n>>> A ControlInfo (or represented by its ControlId) together with a Value\n>>> provide a control item which can be interpreted by the internal IPA and\n>>> PipelineHandler components.\n>>>\n>>> To pass a set of controls around, a ControlList can be added to a\n> \n> \n> s/ / /\n\nI wish vim reflow/automatic wrap would do that....\n\n>>> Request or stored locally to establish a set of default values for later\n>>> reuse.\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> include/libcamera/controls.h | 41 +++++++++++\n>>> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n>>> 2 files changed, 169 insertions(+)\n>>>\n>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>> index 95198d41c4cf..5835b840a31b 100644\n>>> --- a/include/libcamera/controls.h\n>>> +++ b/include/libcamera/controls.h\n>>> @@ -4,6 +4,9 @@\n>>> *\n>>> * controls.h - Control handling\n>>> */\n>>> +\n>>> +#include \n>>> +\n>>> #ifndef __LIBCAMERA_CONTROLS_H__\n>>> #define __LIBCAMERA_CONTROLS_H__\n>>> \n>>> @@ -53,6 +56,44 @@ private:\n>>> \n>>> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n>>> \n>>> +class ControlList\n>>> +{\n>>> +private:\n>>> +\tstruct ControlInfoHasher {\n>>> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n>>> +\t\t{\n>>> +\t\t\treturn std::hash()(ci.id());\n>>> +\t\t}\n>>> +\t};\n>>> +\n>>> +\ttypedef std::unordered_map ControlListMap;\n> \n> We usually use the C++11 syntax\n> \n> \tusing ControlListMap = std::unordered_map;\n\nI prefer this :-D\n\nUpdated.\n\n> \n> I don't think you should copy the ControlInfo in the ControlList. \n\nI was trying not to - It was supposed to be a ControlInfo & but creating\nthe original ControlInfo's has caused me headache ...\n\n> If I\n> understand your patch series correctly, ControlInfo should be created\n> with the camera, and remain constant for the lifetime of the camera. \n\nYes, I want to create ControlInfo structures which are constant for the\ncamera lifetime, and contain any values appropriate to that camera instance.\n\n\n> The\n> map should thus use either the control ID or a pointer to the control\n> info as the key.\n\n\nAn ID should be easy to support, but means code will have to look up the\nControlInfo later, whereas a pointer to the ControlInfo needs to look it\nup internally.\n\nThis is essentially why this topic is still RFC for me.\n\n\n> \n>> Is this needed to go first or can it be moved down to the other private \n>> section?\n>>\n>>> +\n>>> +public:\n>>> +\tControlList();\n>>> +\n>>> +\tusing iterator = ControlListMap::iterator;\n>>> +\tusing const_iterator = ControlListMap::const_iterator;\n>>> +\n>>> +\titerator begin() { return controls_.begin(); }\n>>> +\titerator end() { return controls_.end(); }\n>>> +\tconst_iterator begin() const { return controls_.begin(); }\n>>> +\tconst_iterator end() const { return controls_.end(); }\n>>> +\n>>> +\tbool contains(ControlId id) const { return controls_.count(id); };\n>>\n>> Would not return controls_.find(id) != controls_.end() be better?\n>>\n>>> +\tbool empty() const { return controls_.empty(); }\n>>> +\tsize_t size() const { return controls_.size(); }\n> \n> std::size_t\n\n\nSure.\n\n> \n>>> +\tvoid reset() { controls_.clear(); }\n> \n> Should we call this clear() ?\n\n\nSure.\n\n\n> \n>>> +\n>>> +\tValue &operator[](ControlId id) { return controls_[id]; }\n>>> +\n>>> +\tvoid update(const ControlList &list);\n>>> +\tvoid update(enum ControlId id, const Value &value);\n>>> +\tvoid update(const ControlInfo &info, const Value &value);\n>>> +\n>>> +private:\n>>> +\tControlListMap controls_;\n>>> +};\n>>> +\n>>> } /* namespace libcamera */\n>>> \n>>> #endif /* __LIBCAMERA_CONTROLS_H__ */\n>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>> index b1be46ddb55e..7c55afffe4ca 100644\n>>> --- a/src/libcamera/controls.cpp\n>>> +++ b/src/libcamera/controls.cpp\n>>> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n>>> \treturn stream << info.toString();\n>>> }\n>>> \n>>> +/**\n>>> + * \\class ControlList\n>>> + * \\brief Associates a list of ControlIds with their values.\n>>> + *\n>>> + * The ControlList stores a pair of ControlInfo and Value classes as an\n>>> + * unordered map. Entries can be updated using array syntax such as\n>>> + * myControlList[MyControlId] = myValue;\n>>> + * or\n>>> + * myNewValue = myControlList[MyControlId];\n>>\n>> I don't think this will update the value.\n>>\n>> I think you can drop the examples and the last sentence from the \n>> documentation.\n>>\n>> With these issues addressed,\n>>\n>> Reviewed-by: Niklas Söderlund \n>>\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct a ControlList\n>>> + */\n>>> +ControlList::ControlList()\n>>> +{\n>>> +}\n>>> +\n> \n> If the constructor is empty and is the only one you can omit it, the\n> compiler will generate one for your.\n\n\nGood point, removed.\n\n> \n>>> +/**\n>>> + * \\typedef ControlList::iterator\n>>> + * \\brief Iterator for the controls contained within the list.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\typedef ControlList::const_iterator\n>>> + * \\brief Const iterator for the controls contained within the list.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn iterator ControlList::begin()\n>>> + * \\brief Retrieve an iterator to the first Control in the list\n>>> + * \\return An iterator to the first Control in the list\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn iterator ControlList::end()\n>>> + * \\brief Retrieve an iterator to the next element after the last controls in\n>>> + * the instance.\n>>> + * \\return An iterator to the element following the last control in the instance\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn const_iterator ControlList::begin() const\n>>> + * \\brief Retrieve a const_iterator to the first Control in the list\n>>> + * \\return A const_iterator to the first Control in the list\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn const_iterator ControlList::end() const\n>>> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n>>> + * \\return A const iterator to the element following the last control in the\n>>> + * instance\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlList::contains()\n>>> + * \\brief Identify if the List contains a control with the specified ControlId\n>>> + * \\return True if the list contains a matching control, false otherwise\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlList::empty()\n>>> + * \\brief Identify if the list is empty\n>>> + * \\return True if the list does not contain any control, false otherwise\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlList::size()\n>>> + * \\brief Retrieve the number of controls in the list\n>>> + * \\return The number of Control entries stored in the list\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlList::reset()\n>>> + * \\brief Removes all controls from the list\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlList::operator[]()\n>>> + * \\brief Access the control with the given Id\n>>> + * \\param id The id of the control to access\n>>> + * \\return The Value of the control with a ControlId of \\a Id\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Update all Control values with the value from the given \\a list\n>>> + * \\param list The list of controls to update or append to this list\n>>> + *\n>>> + * Update all controls in the ControlList, by the values given by \\a list\n>>> + * If the list already contains a control of this ID then it will be overwritten\n>>> + */\n>>> +void ControlList::update(const ControlList &list)\n>>> +{\n>>> +\tfor (auto it : list) {\n>>> +\t\tconst ControlInfo &info = it.first;\n>>> +\t\tconst Value &value = it.second;\n>>> +\n>>> +\t\tcontrols_[info] = value;\n>>> +\t}\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Update a control value in the list\n>>> + * \\param id The ControlId of the control to update\n>>> + * \\param value The new value for the updated control\n>>> + *\n>>> + * Set the Control value in the list to the appropriate value\n>>> + * If the list already contains a control of this ID then it will be overwritten\n>>> + */\n>>> +void ControlList::update(enum ControlId id, const Value &value)\n>>> +{\n>>> +\tcontrols_[id] = value;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Update a control value in the list.\n>>> + * \\param info The ControlInfo of the control to update\n>>> + * \\param value The new value for the updated control\n>>> + *\n>>> + * Set the Control value in the list to the appropriate value.\n>>> + * If the list already contains a control of the Id matching the ControlInfo\n>>> + * then it will be overwritten.\n>>> + */\n>>> +void ControlList::update(const ControlInfo &info, const Value &value)\n>>> +{\n>>> +\tcontrols_[info] = value;\n>>> +}\n> \n> Do we really need this class ? Can't we just expose \n> \n> \tusing ControlList = std::unordered_map;\n\nHrm ... well I think the intention was to be able to add some extra\nvalidation later... but I now wonder if just the ControlList 'using'\nmight be easier, and remove a lot of rubbish passthrough code ...\n\n\nI'll give it a go - but I don't want to throw away a whole bunch of code\nto re-add it again soon after ...\n\nBest save this as a branch (well although it's saved on list at least)....\n\n\n> ? The last two update functions would be replaced by usage of\n> operator[], and the first one with\n> \n> template< class InputIt >\n> void std::unordered_map::insert( InputIt first, InputIt last );\n\n'just like that' ?\n\nI'll play there next.\n\nRemoving the ControlList class would remove my desire to have all the\ntests that you disagree with later in the series ...\n\n\n> \n>>> +\n>>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D688A61580\n\tfor ;\n\tMon, 24 Jun 2019 18:02:58 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2FCA92E7;\n\tMon, 24 Jun 2019 18:02:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561392178;\n\tbh=uT2rhqrqzf3l2RoLFNQyw/9ciX2KDjH1j9gWUq1thic=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qJ9JFg2DYZQa3pljjUOdMFBRTTGJ2T95PMofaETm5tnlmpsatWELYiGZ1GFY8Qzzc\n\tiD7Q5OTOBI11LNG7GnCi4QO2D+pblM+YYqlbhJUkwVrw0y596BdpYeGibnEmEIwQd0\n\tg2ieswwLPeCpOy5Aavd92fPXEfAaL4IyJUc6LKOo=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tLibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t<20190623222132.GE6124@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 24 Jun 2019 17:02:55 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190623222132.GE6124@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 16:02:59 -0000"}},{"id":2012,"web_url":"https://patchwork.libcamera.org/comment/2012/","msgid":"<20190624162715.GQ5737@pendragon.ideasonboard.com>","date":"2019-06-24T16:27:15","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 24, 2019 at 04:48:05PM +0100, Kieran Bingham wrote:\n> On 23/06/2019 16:35, Niklas Söderlund wrote:\n> > On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n> >> Define a ControlList which wraps the STL std::unordered_map to contain\n> >> a list of associative pairs of ControlInfo and Value items.\n> >>\n> >> A ControlInfo (or represented by its ControlId) together with a Value\n> >> provide a control item which can be interpreted by the internal IPA and\n> >> PipelineHandler components.\n> >>\n> >> To pass a set of controls around, a ControlList can be added to a\n> >> Request or stored locally to establish a set of default values for later\n> >> reuse.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> include/libcamera/controls.h | 41 +++++++++++\n> >> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n> >> 2 files changed, 169 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >> index 95198d41c4cf..5835b840a31b 100644\n> >> --- a/include/libcamera/controls.h\n> >> +++ b/include/libcamera/controls.h\n> >> @@ -4,6 +4,9 @@\n> >> *\n> >> * controls.h - Control handling\n> >> */\n> >> +\n> >> +#include \n> >> +\n> >> #ifndef __LIBCAMERA_CONTROLS_H__\n> >> #define __LIBCAMERA_CONTROLS_H__\n> >> \n> >> @@ -53,6 +56,44 @@ private:\n> >> \n> >> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> >> \n> >> +class ControlList\n> >> +{\n> >> +private:\n> >> +\tstruct ControlInfoHasher {\n> >> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n> >> +\t\t{\n> >> +\t\t\treturn std::hash()(ci.id());\n> >> +\t\t}\n> >> +\t};\n> >> +\n> >> +\ttypedef std::unordered_map ControlListMap;\n> > \n> > Is this needed to go first or can it be moved down to the other private \n> > section?\n> \n> It has to be defined before the usages below.\n> \n> >> +\n> >> +public:\n> >> +\tControlList();\n> >> +\n> >> +\tusing iterator = ControlListMap::iterator;\n> >> +\tusing const_iterator = ControlListMap::const_iterator;\n> >> +\n> >> +\titerator begin() { return controls_.begin(); }\n> >> +\titerator end() { return controls_.end(); }\n> >> +\tconst_iterator begin() const { return controls_.begin(); }\n> >> +\tconst_iterator end() const { return controls_.end(); }\n> >> +\n> >> +\tbool contains(ControlId id) const { return controls_.count(id); };\n> > \n> > Would not return controls_.find(id) != controls_.end() be better?\n> \n> I thought this was elegant.\n> \n> Count == 0 = false,\n> count > 0 = true...\n\nBut it's less efficient as it has to go through the whole list in all\ncases.\n\n> >> +\tbool empty() const { return controls_.empty(); }\n> >> +\tsize_t size() const { return controls_.size(); }\n> >> +\tvoid reset() { controls_.clear(); }\n> >> +\n> >> +\tValue &operator[](ControlId id) { return controls_[id]; }\n> >> +\n> >> +\tvoid update(const ControlList &list);\n> >> +\tvoid update(enum ControlId id, const Value &value);\n> >> +\tvoid update(const ControlInfo &info, const Value &value);\n> >> +\n> >> +private:\n> >> +\tControlListMap controls_;\n> >> +};\n> >> +\n> >> } /* namespace libcamera */\n> >> \n> >> #endif /* __LIBCAMERA_CONTROLS_H__ */\n> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >> index b1be46ddb55e..7c55afffe4ca 100644\n> >> --- a/src/libcamera/controls.cpp\n> >> +++ b/src/libcamera/controls.cpp\n> >> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> >> \treturn stream << info.toString();\n> >> }\n> >> \n> >> +/**\n> >> + * \\class ControlList\n> >> + * \\brief Associates a list of ControlIds with their values.\n> >> + *\n> >> + * The ControlList stores a pair of ControlInfo and Value classes as an\n> >> + * unordered map. Entries can be updated using array syntax such as\n> >> + * myControlList[MyControlId] = myValue;\n> >> + * or\n> >> + * myNewValue = myControlList[MyControlId];\n> > \n> > I don't think this will update the value.\n> \n> No - it updates myNewValue. I'll just remove this. I was trying to put\n> in that the the [] is the accessor.\n> \n> > I think you can drop the examples and the last sentence from the \n> > documentation.\n> > \n> > With these issues addressed,\n> > \n> > Reviewed-by: Niklas Söderlund \n> > \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Construct a ControlList\n> >> + */\n> >> +ControlList::ControlList()\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\typedef ControlList::iterator\n> >> + * \\brief Iterator for the controls contained within the list.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\typedef ControlList::const_iterator\n> >> + * \\brief Const iterator for the controls contained within the list.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn iterator ControlList::begin()\n> >> + * \\brief Retrieve an iterator to the first Control in the list\n> >> + * \\return An iterator to the first Control in the list\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn iterator ControlList::end()\n> >> + * \\brief Retrieve an iterator to the next element after the last controls in\n> >> + * the instance.\n> >> + * \\return An iterator to the element following the last control in the instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn const_iterator ControlList::begin() const\n> >> + * \\brief Retrieve a const_iterator to the first Control in the list\n> >> + * \\return A const_iterator to the first Control in the list\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn const_iterator ControlList::end() const\n> >> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n> >> + * \\return A const iterator to the element following the last control in the\n> >> + * instance\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlList::contains()\n> >> + * \\brief Identify if the List contains a control with the specified ControlId\n> >> + * \\return True if the list contains a matching control, false otherwise\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlList::empty()\n> >> + * \\brief Identify if the list is empty\n> >> + * \\return True if the list does not contain any control, false otherwise\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlList::size()\n> >> + * \\brief Retrieve the number of controls in the list\n> >> + * \\return The number of Control entries stored in the list\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlList::reset()\n> >> + * \\brief Removes all controls from the list\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlList::operator[]()\n> >> + * \\brief Access the control with the given Id\n> >> + * \\param id The id of the control to access\n> >> + * \\return The Value of the control with a ControlId of \\a Id\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Update all Control values with the value from the given \\a list\n> >> + * \\param list The list of controls to update or append to this list\n> >> + *\n> >> + * Update all controls in the ControlList, by the values given by \\a list\n> >> + * If the list already contains a control of this ID then it will be overwritten\n> >> + */\n> >> +void ControlList::update(const ControlList &list)\n> >> +{\n> >> +\tfor (auto it : list) {\n> >> +\t\tconst ControlInfo &info = it.first;\n> >> +\t\tconst Value &value = it.second;\n> >> +\n> >> +\t\tcontrols_[info] = value;\n> >> +\t}\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Update a control value in the list\n> >> + * \\param id The ControlId of the control to update\n> >> + * \\param value The new value for the updated control\n> >> + *\n> >> + * Set the Control value in the list to the appropriate value\n> >> + * If the list already contains a control of this ID then it will be overwritten\n> >> + */\n> >> +void ControlList::update(enum ControlId id, const Value &value)\n> >> +{\n> >> +\tcontrols_[id] = value;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Update a control value in the list.\n> >> + * \\param info The ControlInfo of the control to update\n> >> + * \\param value The new value for the updated control\n> >> + *\n> >> + * Set the Control value in the list to the appropriate value.\n> >> + * If the list already contains a control of the Id matching the ControlInfo\n> >> + * then it will be overwritten.\n> >> + */\n> >> +void ControlList::update(const ControlInfo &info, const Value &value)\n> >> +{\n> >> +\tcontrols_[info] = value;\n> >> +}\n> >> +\n> >> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["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 496DA61580\n\tfor ;\n\tMon, 24 Jun 2019 18:29:46 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5DED2E7;\n\tMon, 24 Jun 2019 18:29:45 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561393785;\n\tbh=vDjqVt6Y+y04B0BFrw4lYDJwhJVHnAcfmDFAO1/MtWU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=idOMyA4f8XGHSo5FRpA4rZQX4VM9AbmMLiWAT27Z7ZnXk+F4yKCe+Ic9cNURFc/Fg\n\tGp7vXrJEWnqUa7egsXLbYyWqC2TwUNfzgFhriHOTMbqqJVimZIb6ecptNivKJRjakb\n\taBoCFZQERR0QHhZnMel84WQkHcfo/CVYrIEp3GHM=","Date":"Mon, 24 Jun 2019 19:27:15 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tLibCamera Devel ","Message-ID":"<20190624162715.GQ5737@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 16:29:46 -0000"}},{"id":2013,"web_url":"https://patchwork.libcamera.org/comment/2013/","msgid":"<20190624163045.GA24965@pendragon.ideasonboard.com>","date":"2019-06-24T16:30:45","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 24, 2019 at 05:02:55PM +0100, Kieran Bingham wrote:\n> On 23/06/2019 23:21, Laurent Pinchart wrote:\n> > On Sun, Jun 23, 2019 at 05:35:01PM +0200, Niklas Söderlund wrote:\n> >> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n> >>> Define a ControlList which wraps the STL std::unordered_map to contain\n> >>> a list of associative pairs of ControlInfo and Value items.\n> >>>\n> >>> A ControlInfo (or represented by its ControlId) together with a Value\n> >>> provide a control item which can be interpreted by the internal IPA and\n> >>> PipelineHandler components.\n> >>>\n> >>> To pass a set of controls around, a ControlList can be added to a\n> > \n> > s/ / /\n> \n> I wish vim reflow/automatic wrap would do that....\n> \n> >>> Request or stored locally to establish a set of default values for later\n> >>> reuse.\n> >>>\n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>> include/libcamera/controls.h | 41 +++++++++++\n> >>> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n> >>> 2 files changed, 169 insertions(+)\n> >>>\n> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>> index 95198d41c4cf..5835b840a31b 100644\n> >>> --- a/include/libcamera/controls.h\n> >>> +++ b/include/libcamera/controls.h\n> >>> @@ -4,6 +4,9 @@\n> >>> *\n> >>> * controls.h - Control handling\n> >>> */\n> >>> +\n> >>> +#include \n> >>> +\n> >>> #ifndef __LIBCAMERA_CONTROLS_H__\n> >>> #define __LIBCAMERA_CONTROLS_H__\n> >>> \n> >>> @@ -53,6 +56,44 @@ private:\n> >>> \n> >>> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> >>> \n> >>> +class ControlList\n> >>> +{\n> >>> +private:\n> >>> +\tstruct ControlInfoHasher {\n> >>> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n> >>> +\t\t{\n> >>> +\t\t\treturn std::hash()(ci.id());\n> >>> +\t\t}\n> >>> +\t};\n> >>> +\n> >>> +\ttypedef std::unordered_map ControlListMap;\n> > \n> > We usually use the C++11 syntax\n> > \n> > \tusing ControlListMap = std::unordered_map;\n> \n> I prefer this :-D\n> \n> Updated.\n> \n> > I don't think you should copy the ControlInfo in the ControlList. \n> \n> I was trying not to - It was supposed to be a ControlInfo & but creating\n> the original ControlInfo's has caused me headache ...\n> \n> > If I\n> > understand your patch series correctly, ControlInfo should be created\n> > with the camera, and remain constant for the lifetime of the camera. \n> \n> Yes, I want to create ControlInfo structures which are constant for the\n> camera lifetime, and contain any values appropriate to that camera instance.\n> \n> > The\n> > map should thus use either the control ID or a pointer to the control\n> > info as the key.\n> \n> An ID should be easy to support, but means code will have to look up the\n> ControlInfo later, whereas a pointer to the ControlInfo needs to look it\n> up internally.\n> \n> This is essentially why this topic is still RFC for me.\n> \n> >> Is this needed to go first or can it be moved down to the other private \n> >> section?\n> >>\n> >>> +\n> >>> +public:\n> >>> +\tControlList();\n> >>> +\n> >>> +\tusing iterator = ControlListMap::iterator;\n> >>> +\tusing const_iterator = ControlListMap::const_iterator;\n> >>> +\n> >>> +\titerator begin() { return controls_.begin(); }\n> >>> +\titerator end() { return controls_.end(); }\n> >>> +\tconst_iterator begin() const { return controls_.begin(); }\n> >>> +\tconst_iterator end() const { return controls_.end(); }\n> >>> +\n> >>> +\tbool contains(ControlId id) const { return controls_.count(id); };\n> >>\n> >> Would not return controls_.find(id) != controls_.end() be better?\n> >>\n> >>> +\tbool empty() const { return controls_.empty(); }\n> >>> +\tsize_t size() const { return controls_.size(); }\n> > \n> > std::size_t\n> \n> Sure.\n> \n> >>> +\tvoid reset() { controls_.clear(); }\n> > \n> > Should we call this clear() ?\n> \n> Sure.\n> \n> >>> +\n> >>> +\tValue &operator[](ControlId id) { return controls_[id]; }\n> >>> +\n> >>> +\tvoid update(const ControlList &list);\n> >>> +\tvoid update(enum ControlId id, const Value &value);\n> >>> +\tvoid update(const ControlInfo &info, const Value &value);\n> >>> +\n> >>> +private:\n> >>> +\tControlListMap controls_;\n> >>> +};\n> >>> +\n> >>> } /* namespace libcamera */\n> >>> \n> >>> #endif /* __LIBCAMERA_CONTROLS_H__ */\n> >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>> index b1be46ddb55e..7c55afffe4ca 100644\n> >>> --- a/src/libcamera/controls.cpp\n> >>> +++ b/src/libcamera/controls.cpp\n> >>> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> >>> \treturn stream << info.toString();\n> >>> }\n> >>> \n> >>> +/**\n> >>> + * \\class ControlList\n> >>> + * \\brief Associates a list of ControlIds with their values.\n> >>> + *\n> >>> + * The ControlList stores a pair of ControlInfo and Value classes as an\n> >>> + * unordered map. Entries can be updated using array syntax such as\n> >>> + * myControlList[MyControlId] = myValue;\n> >>> + * or\n> >>> + * myNewValue = myControlList[MyControlId];\n> >>\n> >> I don't think this will update the value.\n> >>\n> >> I think you can drop the examples and the last sentence from the \n> >> documentation.\n> >>\n> >> With these issues addressed,\n> >>\n> >> Reviewed-by: Niklas Söderlund \n> >>\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Construct a ControlList\n> >>> + */\n> >>> +ControlList::ControlList()\n> >>> +{\n> >>> +}\n> >>> +\n> > \n> > If the constructor is empty and is the only one you can omit it, the\n> > compiler will generate one for your.\n> \n> Good point, removed.\n> \n> >>> +/**\n> >>> + * \\typedef ControlList::iterator\n> >>> + * \\brief Iterator for the controls contained within the list.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\typedef ControlList::const_iterator\n> >>> + * \\brief Const iterator for the controls contained within the list.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn iterator ControlList::begin()\n> >>> + * \\brief Retrieve an iterator to the first Control in the list\n> >>> + * \\return An iterator to the first Control in the list\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn iterator ControlList::end()\n> >>> + * \\brief Retrieve an iterator to the next element after the last controls in\n> >>> + * the instance.\n> >>> + * \\return An iterator to the element following the last control in the instance\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn const_iterator ControlList::begin() const\n> >>> + * \\brief Retrieve a const_iterator to the first Control in the list\n> >>> + * \\return A const_iterator to the first Control in the list\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn const_iterator ControlList::end() const\n> >>> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n> >>> + * \\return A const iterator to the element following the last control in the\n> >>> + * instance\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn ControlList::contains()\n> >>> + * \\brief Identify if the List contains a control with the specified ControlId\n> >>> + * \\return True if the list contains a matching control, false otherwise\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn ControlList::empty()\n> >>> + * \\brief Identify if the list is empty\n> >>> + * \\return True if the list does not contain any control, false otherwise\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn ControlList::size()\n> >>> + * \\brief Retrieve the number of controls in the list\n> >>> + * \\return The number of Control entries stored in the list\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn ControlList::reset()\n> >>> + * \\brief Removes all controls from the list\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn ControlList::operator[]()\n> >>> + * \\brief Access the control with the given Id\n> >>> + * \\param id The id of the control to access\n> >>> + * \\return The Value of the control with a ControlId of \\a Id\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Update all Control values with the value from the given \\a list\n> >>> + * \\param list The list of controls to update or append to this list\n> >>> + *\n> >>> + * Update all controls in the ControlList, by the values given by \\a list\n> >>> + * If the list already contains a control of this ID then it will be overwritten\n> >>> + */\n> >>> +void ControlList::update(const ControlList &list)\n> >>> +{\n> >>> +\tfor (auto it : list) {\n> >>> +\t\tconst ControlInfo &info = it.first;\n> >>> +\t\tconst Value &value = it.second;\n> >>> +\n> >>> +\t\tcontrols_[info] = value;\n> >>> +\t}\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Update a control value in the list\n> >>> + * \\param id The ControlId of the control to update\n> >>> + * \\param value The new value for the updated control\n> >>> + *\n> >>> + * Set the Control value in the list to the appropriate value\n> >>> + * If the list already contains a control of this ID then it will be overwritten\n> >>> + */\n> >>> +void ControlList::update(enum ControlId id, const Value &value)\n> >>> +{\n> >>> +\tcontrols_[id] = value;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Update a control value in the list.\n> >>> + * \\param info The ControlInfo of the control to update\n> >>> + * \\param value The new value for the updated control\n> >>> + *\n> >>> + * Set the Control value in the list to the appropriate value.\n> >>> + * If the list already contains a control of the Id matching the ControlInfo\n> >>> + * then it will be overwritten.\n> >>> + */\n> >>> +void ControlList::update(const ControlInfo &info, const Value &value)\n> >>> +{\n> >>> +\tcontrols_[info] = value;\n> >>> +}\n> > \n> > Do we really need this class ? Can't we just expose \n> > \n> > \tusing ControlList = std::unordered_map;\n> \n> Hrm ... well I think the intention was to be able to add some extra\n> validation later... but I now wonder if just the ControlList 'using'\n> might be easier, and remove a lot of rubbish passthrough code ...\n> \n> I'll give it a go - but I don't want to throw away a whole bunch of code\n> to re-add it again soon after ...\n\ngit history to the rescue :-) (and mailing list archives)\n\n> Best save this as a branch (well although it's saved on list at least)....\n> \n> > ? The last two update functions would be replaced by usage of\n> > operator[], and the first one with\n> > \n> > template< class InputIt >\n> > void std::unordered_map::insert( InputIt first, InputIt last );\n> \n> 'just like that' ?\n\ncontrols.insert(otherControls.begin(), otherControls.end());\n\n> I'll play there next.\n> \n> Removing the ControlList class would remove my desire to have all the\n> tests that you disagree with later in the series ...\n\nI swear the proposal has nothing to do with the test discussion :-)\n\n> >>> +\n> >>> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["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 2761D61580\n\tfor ;\n\tMon, 24 Jun 2019 18:33:15 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E3832E7;\n\tMon, 24 Jun 2019 18:33:14 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561393994;\n\tbh=UhZ5XPNg/TJImJUgI8pobuyU6I9qR2lVzD6rtu37mwE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lU93JB2xy3tIdJSwbk/ShCeUYw3pNq4Q+Ig0vz4UKIA3oGOVNmqDURkrX8eVPC7nx\n\tW3RRhVNmqRTREZufcLX6j63rcTRfakj8Y3VwKdf9siT1ZVDMQKNHTOzkJAm2nBQqmq\n\tpLPBiEuPJK+DDCf/+nJ8aN+G4E85ShtIFsNRs+2s=","Date":"Mon, 24 Jun 2019 19:30:45 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tLibCamera Devel ","Message-ID":"<20190624163045.GA24965@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t<20190623222132.GE6124@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 16:33:15 -0000"}},{"id":2035,"web_url":"https://patchwork.libcamera.org/comment/2035/","msgid":"","date":"2019-06-26T10:59:00","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent, Niklas,\n\nOn 24/06/2019 17:27, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Jun 24, 2019 at 04:48:05PM +0100, Kieran Bingham wrote:\n>> On 23/06/2019 16:35, Niklas Söderlund wrote:\n>>> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n>>>> Define a ControlList which wraps the STL std::unordered_map to contain\n>>>> a list of associative pairs of ControlInfo and Value items.\n>>>>\n>>>> A ControlInfo (or represented by its ControlId) together with a Value\n>>>> provide a control item which can be interpreted by the internal IPA and\n>>>> PipelineHandler components.\n>>>>\n>>>> To pass a set of controls around, a ControlList can be added to a\n>>>> Request or stored locally to establish a set of default values for later\n>>>> reuse.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> include/libcamera/controls.h | 41 +++++++++++\n>>>> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n>>>> 2 files changed, 169 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>> index 95198d41c4cf..5835b840a31b 100644\n>>>> --- a/include/libcamera/controls.h\n>>>> +++ b/include/libcamera/controls.h\n>>>> @@ -4,6 +4,9 @@\n>>>> *\n>>>> * controls.h - Control handling\n>>>> */\n>>>> +\n>>>> +#include \n>>>> +\n>>>> #ifndef __LIBCAMERA_CONTROLS_H__\n>>>> #define __LIBCAMERA_CONTROLS_H__\n>>>> \n>>>> @@ -53,6 +56,44 @@ private:\n>>>> \n>>>> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n>>>> \n>>>> +class ControlList\n>>>> +{\n>>>> +private:\n>>>> +\tstruct ControlInfoHasher {\n>>>> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n>>>> +\t\t{\n>>>> +\t\t\treturn std::hash()(ci.id());\n>>>> +\t\t}\n>>>> +\t};\n>>>> +\n>>>> +\ttypedef std::unordered_map ControlListMap;\n>>>\n>>> Is this needed to go first or can it be moved down to the other private \n>>> section?\n>>\n>> It has to be defined before the usages below.\n>>\n>>>> +\n>>>> +public:\n>>>> +\tControlList();\n>>>> +\n>>>> +\tusing iterator = ControlListMap::iterator;\n>>>> +\tusing const_iterator = ControlListMap::const_iterator;\n>>>> +\n>>>> +\titerator begin() { return controls_.begin(); }\n>>>> +\titerator end() { return controls_.end(); }\n>>>> +\tconst_iterator begin() const { return controls_.begin(); }\n>>>> +\tconst_iterator end() const { return controls_.end(); }\n>>>> +\n>>>> +\tbool contains(ControlId id) const { return controls_.count(id); };\n>>>\n>>> Would not return controls_.find(id) != controls_.end() be better?\n>>\n>> I thought this was elegant.\n>>\n>> Count == 0 = false,\n>> count > 0 = true...\n> \n> But it's less efficient as it has to go through the whole list in all\n> cases.\n\nHrm... I thought with the guaranteed single key entry, and known hash\nalgorithm this would be optimised internally : However:\n\nwww.cplusplus.com/reference/unordered_map/unordered_map/{count,find}:\n\nCount elements with a specific key\nSearches the container for elements whose key is k and returns the\nnumber of elements found. Because unordered_map containers do not allow\nfor duplicate keys, this means that the function actually returns 1 if\nan element with that key exists in the container, and zero otherwise.\n\nBut:\n\nComplexity[Count]\nAverage case: linear in the number of elements counted.\nWorst case: linear in container size.\n\nvs\n\nComplexity[Find]\nAverage case: constant.\nWorst case: linear in container size.\n\n\nYup... that certainly points to using find();.\n\n\n>>>> +\tbool empty() const { return controls_.empty(); }\n>>>> +\tsize_t size() const { return controls_.size(); }\n>>>> +\tvoid reset() { controls_.clear(); }\n>>>> +\n>>>> +\tValue &operator[](ControlId id) { return controls_[id]; }\n>>>> +\n>>>> +\tvoid update(const ControlList &list);\n>>>> +\tvoid update(enum ControlId id, const Value &value);\n>>>> +\tvoid update(const ControlInfo &info, const Value &value);\n>>>> +\n>>>> +private:\n>>>> +\tControlListMap controls_;\n>>>> +};\n>>>> +\n>>>> } /* namespace libcamera */\n>>>> \n>>>> #endif /* __LIBCAMERA_CONTROLS_H__ */\n>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>> index b1be46ddb55e..7c55afffe4ca 100644\n>>>> --- a/src/libcamera/controls.cpp\n>>>> +++ b/src/libcamera/controls.cpp\n>>>> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n>>>> \treturn stream << info.toString();\n>>>> }\n>>>> \n>>>> +/**\n>>>> + * \\class ControlList\n>>>> + * \\brief Associates a list of ControlIds with their values.\n>>>> + *\n>>>> + * The ControlList stores a pair of ControlInfo and Value classes as an\n>>>> + * unordered map. Entries can be updated using array syntax such as\n>>>> + * myControlList[MyControlId] = myValue;\n>>>> + * or\n>>>> + * myNewValue = myControlList[MyControlId];\n>>>\n>>> I don't think this will update the value.\n>>\n>> No - it updates myNewValue. I'll just remove this. I was trying to put\n>> in that the the [] is the accessor.\n>>\n>>> I think you can drop the examples and the last sentence from the \n>>> documentation.\n>>>\n>>> With these issues addressed,\n>>>\n>>> Reviewed-by: Niklas Söderlund \n\nThankyou, currently trying to work out if I can drop the whole wrapper\neasily though :S\n\n>>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Construct a ControlList\n>>>> + */\n>>>> +ControlList::ControlList()\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\typedef ControlList::iterator\n>>>> + * \\brief Iterator for the controls contained within the list.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\typedef ControlList::const_iterator\n>>>> + * \\brief Const iterator for the controls contained within the list.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn iterator ControlList::begin()\n>>>> + * \\brief Retrieve an iterator to the first Control in the list\n>>>> + * \\return An iterator to the first Control in the list\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn iterator ControlList::end()\n>>>> + * \\brief Retrieve an iterator to the next element after the last controls in\n>>>> + * the instance.\n>>>> + * \\return An iterator to the element following the last control in the instance\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn const_iterator ControlList::begin() const\n>>>> + * \\brief Retrieve a const_iterator to the first Control in the list\n>>>> + * \\return A const_iterator to the first Control in the list\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn const_iterator ControlList::end() const\n>>>> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n>>>> + * \\return A const iterator to the element following the last control in the\n>>>> + * instance\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ControlList::contains()\n>>>> + * \\brief Identify if the List contains a control with the specified ControlId\n>>>> + * \\return True if the list contains a matching control, false otherwise\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ControlList::empty()\n>>>> + * \\brief Identify if the list is empty\n>>>> + * \\return True if the list does not contain any control, false otherwise\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ControlList::size()\n>>>> + * \\brief Retrieve the number of controls in the list\n>>>> + * \\return The number of Control entries stored in the list\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ControlList::reset()\n>>>> + * \\brief Removes all controls from the list\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ControlList::operator[]()\n>>>> + * \\brief Access the control with the given Id\n>>>> + * \\param id The id of the control to access\n>>>> + * \\return The Value of the control with a ControlId of \\a Id\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Update all Control values with the value from the given \\a list\n>>>> + * \\param list The list of controls to update or append to this list\n>>>> + *\n>>>> + * Update all controls in the ControlList, by the values given by \\a list\n>>>> + * If the list already contains a control of this ID then it will be overwritten\n>>>> + */\n>>>> +void ControlList::update(const ControlList &list)\n>>>> +{\n>>>> +\tfor (auto it : list) {\n>>>> +\t\tconst ControlInfo &info = it.first;\n>>>> +\t\tconst Value &value = it.second;\n>>>> +\n>>>> +\t\tcontrols_[info] = value;\n>>>> +\t}\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Update a control value in the list\n>>>> + * \\param id The ControlId of the control to update\n>>>> + * \\param value The new value for the updated control\n>>>> + *\n>>>> + * Set the Control value in the list to the appropriate value\n>>>> + * If the list already contains a control of this ID then it will be overwritten\n>>>> + */\n>>>> +void ControlList::update(enum ControlId id, const Value &value)\n>>>> +{\n>>>> +\tcontrols_[id] = value;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Update a control value in the list.\n>>>> + * \\param info The ControlInfo of the control to update\n>>>> + * \\param value The new value for the updated control\n>>>> + *\n>>>> + * Set the Control value in the list to the appropriate value.\n>>>> + * If the list already contains a control of the Id matching the ControlInfo\n>>>> + * then it will be overwritten.\n>>>> + */\n>>>> +void ControlList::update(const ControlInfo &info, const Value &value)\n>>>> +{\n>>>> +\tcontrols_[info] = value;\n>>>> +}\n>>>> +\n>>>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A3546157F\n\tfor ;\n\tWed, 26 Jun 2019 12:59:03 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B099F510;\n\tWed, 26 Jun 2019 12:59:02 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561546743;\n\tbh=kra4xoNdbsWZ4hMnFpT8SxOnnGFA00jgNHOq2KxeYaw=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=fPOiUik1+gCn4VX6g2RbLgf9t0ZeKWWqwh0YbWWitZLHMTFM81eCOd2KtaVbKJM+N\n\tC273kFgLpuFUNJVbAdk6F7SGVNBsSeea1Qr7e1MH6IRIUD/V9pVNACdohrf0O3u+7x\n\tfxw01FkI6L62ttTzXo+zfWuxB1mR/NG6NKBV6hkc=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tLibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t\n\t<20190624162715.GQ5737@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 26 Jun 2019 11:59:00 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190624162715.GQ5737@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 26 Jun 2019 10:59:03 -0000"}},{"id":2037,"web_url":"https://patchwork.libcamera.org/comment/2037/","msgid":"","date":"2019-06-26T13:14:34","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nA whole bunch of my development thoughts as I went through\nconverting/removing the ControlList abstraction.\n\nTLDR: I'm removing the Class abstraction and just using the\nstd::unordered_map directly through the typedef/using.\n\nSeems like it might be worth going back to being a map of straight\nControlID, {Control}Value pairs too - and keep the ControlInfo entirely\nseparated. Otherwise anytime a control is added to the list directly - a\nlookup will have to be done on the ControlInfo even if it's not needed...\n\n\nOn 24/06/2019 17:30, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Jun 24, 2019 at 05:02:55PM +0100, Kieran Bingham wrote:\n>> On 23/06/2019 23:21, Laurent Pinchart wrote:\n>>> On Sun, Jun 23, 2019 at 05:35:01PM +0200, Niklas Söderlund wrote:\n>>>> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n>>>>> Define a ControlList which wraps the STL std::unordered_map to contain\n>>>>> a list of associative pairs of ControlInfo and Value items.\n>>>>>\n>>>>> A ControlInfo (or represented by its ControlId) together with a Value\n>>>>> provide a control item which can be interpreted by the internal IPA and\n>>>>> PipelineHandler components.\n>>>>>\n>>>>> To pass a set of controls around, a ControlList can be added to a\n>>>\n>>> s/ / /\n>>\n>> I wish vim reflow/automatic wrap would do that....\n>>\n>>>>> Request or stored locally to establish a set of default values for later\n>>>>> reuse.\n>>>>>\n>>>>> Signed-off-by: Kieran Bingham \n>>>>> ---\n>>>>> include/libcamera/controls.h | 41 +++++++++++\n>>>>> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n>>>>> 2 files changed, 169 insertions(+)\n>>>>>\n>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>> index 95198d41c4cf..5835b840a31b 100644\n>>>>> --- a/include/libcamera/controls.h\n>>>>> +++ b/include/libcamera/controls.h\n>>>>> @@ -4,6 +4,9 @@\n>>>>> *\n>>>>> * controls.h - Control handling\n>>>>> */\n>>>>> +\n>>>>> +#include \n>>>>> +\n>>>>> #ifndef __LIBCAMERA_CONTROLS_H__\n>>>>> #define __LIBCAMERA_CONTROLS_H__\n>>>>> \n>>>>> @@ -53,6 +56,44 @@ private:\n>>>>> \n>>>>> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n>>>>> \n>>>>> +class ControlList\n>>>>> +{\n>>>>> +private:\n>>>>> +\tstruct ControlInfoHasher {\n>>>>> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n>>>>> +\t\t{\n>>>>> +\t\t\treturn std::hash()(ci.id());\n>>>>> +\t\t}\n>>>>> +\t};\n>>>>> +\n>>>>> +\ttypedef std::unordered_map ControlListMap;\n>>>\n>>> We usually use the C++11 syntax\n>>>\n>>> \tusing ControlListMap = std::unordered_map;\n>>\n>> I prefer this :-D\n>>\n>> Updated.\n>>\n>>> I don't think you should copy the ControlInfo in the ControlList. \n>>\n>> I was trying not to - It was supposed to be a ControlInfo & but creating\n>> the original ControlInfo's has caused me headache ...\n>>\n>>> If I\n>>> understand your patch series correctly, ControlInfo should be created\n>>> with the camera, and remain constant for the lifetime of the camera. \n>>\n>> Yes, I want to create ControlInfo structures which are constant for the\n>> camera lifetime, and contain any values appropriate to that camera instance.\n>>\n>>> The\n>>> map should thus use either the control ID or a pointer to the control\n>>> info as the key.\n>>\n>> An ID should be easy to support, but means code will have to look up the\n>> ControlInfo later, whereas a pointer to the ControlInfo needs to look it\n>> up internally.\n>>\n>> This is essentially why this topic is still RFC for me.\n>>\n>>>> Is this needed to go first or can it be moved down to the other private \n>>>> section?\n>>>>\n>>>>> +\n>>>>> +public:\n>>>>> +\tControlList();\n>>>>> +\n>>>>> +\tusing iterator = ControlListMap::iterator;\n>>>>> +\tusing const_iterator = ControlListMap::const_iterator;\n>>>>> +\n>>>>> +\titerator begin() { return controls_.begin(); }\n>>>>> +\titerator end() { return controls_.end(); }\n>>>>> +\tconst_iterator begin() const { return controls_.begin(); }\n>>>>> +\tconst_iterator end() const { return controls_.end(); }\n>>>>> +\n>>>>> +\tbool contains(ControlId id) const { return controls_.count(id); };\n>>>>\n>>>> Would not return controls_.find(id) != controls_.end() be better?\n>>>>\n>>>>> +\tbool empty() const { return controls_.empty(); }\n>>>>> +\tsize_t size() const { return controls_.size(); }\n>>>\n>>> std::size_t\n>>\n>> Sure.\n>>\n>>>>> +\tvoid reset() { controls_.clear(); }\n>>>\n>>> Should we call this clear() ?\n>>\n>> Sure.\n>>\n>>>>> +\n>>>>> +\tValue &operator[](ControlId id) { return controls_[id]; }\n>>>>> +\n>>>>> +\tvoid update(const ControlList &list);\n>>>>> +\tvoid update(enum ControlId id, const Value &value);\n>>>>> +\tvoid update(const ControlInfo &info, const Value &value);\n>>>>> +\n>>>>> +private:\n>>>>> +\tControlListMap controls_;\n>>>>> +};\n>>>>> +\n>>>>> } /* namespace libcamera */\n>>>>> \n>>>>> #endif /* __LIBCAMERA_CONTROLS_H__ */\n>>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>>> index b1be46ddb55e..7c55afffe4ca 100644\n>>>>> --- a/src/libcamera/controls.cpp\n>>>>> +++ b/src/libcamera/controls.cpp\n>>>>> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n>>>>> \treturn stream << info.toString();\n>>>>> }\n>>>>> \n>>>>> +/**\n>>>>> + * \\class ControlList\n>>>>> + * \\brief Associates a list of ControlIds with their values.\n>>>>> + *\n>>>>> + * The ControlList stores a pair of ControlInfo and Value classes as an\n>>>>> + * unordered map. Entries can be updated using array syntax such as\n>>>>> + * myControlList[MyControlId] = myValue;\n>>>>> + * or\n>>>>> + * myNewValue = myControlList[MyControlId];\n>>>>\n>>>> I don't think this will update the value.\n>>>>\n>>>> I think you can drop the examples and the last sentence from the \n>>>> documentation.\n>>>>\n>>>> With these issues addressed,\n>>>>\n>>>> Reviewed-by: Niklas Söderlund \n>>>>\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\brief Construct a ControlList\n>>>>> + */\n>>>>> +ControlList::ControlList()\n>>>>> +{\n>>>>> +}\n>>>>> +\n>>>\n>>> If the constructor is empty and is the only one you can omit it, the\n>>> compiler will generate one for your.\n>>\n>> Good point, removed.\n>>\n>>>>> +/**\n>>>>> + * \\typedef ControlList::iterator\n>>>>> + * \\brief Iterator for the controls contained within the list.\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\typedef ControlList::const_iterator\n>>>>> + * \\brief Const iterator for the controls contained within the list.\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn iterator ControlList::begin()\n>>>>> + * \\brief Retrieve an iterator to the first Control in the list\n>>>>> + * \\return An iterator to the first Control in the list\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn iterator ControlList::end()\n>>>>> + * \\brief Retrieve an iterator to the next element after the last controls in\n>>>>> + * the instance.\n>>>>> + * \\return An iterator to the element following the last control in the instance\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn const_iterator ControlList::begin() const\n>>>>> + * \\brief Retrieve a const_iterator to the first Control in the list\n>>>>> + * \\return A const_iterator to the first Control in the list\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn const_iterator ControlList::end() const\n>>>>> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n>>>>> + * \\return A const iterator to the element following the last control in the\n>>>>> + * instance\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn ControlList::contains()\n>>>>> + * \\brief Identify if the List contains a control with the specified ControlId\n>>>>> + * \\return True if the list contains a matching control, false otherwise\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn ControlList::empty()\n>>>>> + * \\brief Identify if the list is empty\n>>>>> + * \\return True if the list does not contain any control, false otherwise\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn ControlList::size()\n>>>>> + * \\brief Retrieve the number of controls in the list\n>>>>> + * \\return The number of Control entries stored in the list\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn ControlList::reset()\n>>>>> + * \\brief Removes all controls from the list\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn ControlList::operator[]()\n>>>>> + * \\brief Access the control with the given Id\n>>>>> + * \\param id The id of the control to access\n>>>>> + * \\return The Value of the control with a ControlId of \\a Id\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\brief Update all Control values with the value from the given \\a list\n>>>>> + * \\param list The list of controls to update or append to this list\n>>>>> + *\n>>>>> + * Update all controls in the ControlList, by the values given by \\a list\n>>>>> + * If the list already contains a control of this ID then it will be overwritten\n>>>>> + */\n>>>>> +void ControlList::update(const ControlList &list)\n>>>>> +{\n>>>>> +\tfor (auto it : list) {\n>>>>> +\t\tconst ControlInfo &info = it.first;\n>>>>> +\t\tconst Value &value = it.second;\n>>>>> +\n>>>>> +\t\tcontrols_[info] = value;\n>>>>> +\t}\n>>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\brief Update a control value in the list\n>>>>> + * \\param id The ControlId of the control to update\n>>>>> + * \\param value The new value for the updated control\n>>>>> + *\n>>>>> + * Set the Control value in the list to the appropriate value\n>>>>> + * If the list already contains a control of this ID then it will be overwritten\n>>>>> + */\n>>>>> +void ControlList::update(enum ControlId id, const Value &value)\n>>>>> +{\n>>>>> +\tcontrols_[id] = value;\n>>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\brief Update a control value in the list.\n>>>>> + * \\param info The ControlInfo of the control to update\n>>>>> + * \\param value The new value for the updated control\n>>>>> + *\n>>>>> + * Set the Control value in the list to the appropriate value.\n>>>>> + * If the list already contains a control of the Id matching the ControlInfo\n>>>>> + * then it will be overwritten.\n>>>>> + */\n>>>>> +void ControlList::update(const ControlInfo &info, const Value &value)\n>>>>> +{\n>>>>> +\tcontrols_[info] = value;\n>>>>> +}\n>>>\n>>> Do we really need this class ? Can't we just expose \n>>>\n>>> \tusing ControlList = std::unordered_map;\n>>\n>> Hrm ... well I think the intention was to be able to add some extra\n>> validation later... but I now wonder if just the ControlList 'using'\n>> might be easier, and remove a lot of rubbish passthrough code ...\n>>\n>> I'll give it a go - but I don't want to throw away a whole bunch of code\n>> to re-add it again soon after ...\n> \n> git history to the rescue :-) (and mailing list archives)\n> \n>> Best save this as a branch (well although it's saved on list at least)....\n>>\n>>> ? The last two update functions would be replaced by usage of\n>>> operator[], and the first one with\n>>>\n>>> template< class InputIt >\n>>> void std::unordered_map::insert( InputIt first, InputIt last );\n>>\n>> 'just like that' ?\n> \n> controls.insert(otherControls.begin(), otherControls.end());\n\nI wonder if some helpers would make it 'nicer', but because there is no\nclass - it feels a bit more ugly (loses it's OO?):\n\n void controlListUpdate(targetList, sourceList) {\n\ttargetList.insert(sourceList.begin(), sourceList.end());\n }\n\nAny ideas or suggestions on how to do these helpers nicely? Ideally I'd\nwant the ControlList to look like a class object - but I don't think I\ncan just extend the std::unordered_map() interface ...\n\nI'd also want a helper for the .contains() method\n\n(which is a shame, because in C++20 .contains() is available directly on\nthe unordered_map)\n\n\nPerhaps it's a non-issue anyway, as if we only care about initialising a\nlist with a 'preset' set of controls, then that can be done with the\noperator=():\n\n ControlList newList = defaultList;\n\nBut that removes any existing items in the new list, so can't be used to\nupdate an existing list.\n\nStill if that becomes necessary - then we can worry about adding the helper.\n\nLikewise, perhaps the usage of .contains() won't be really necessary\njust yet, and it would be more optimal to use the find() directly so\nthat in the event that it is found, you don't then have to 'search again'.\n\n\nTLDR: I've cancelled out my concerns :) - dropping the Class and\nreplacing with the 'using' typedef.\n\n\n>> I'll play there next.\n>>\n>> Removing the ControlList class would remove my desire to have all the\n>> tests that you disagree with later in the series ...\n> \n> I swear the proposal has nothing to do with the test discussion :-)\n\nHahah - of course, I actually like the idea of just using the typdef on\nthe map itself. I don't want to pointlessly wrap the standard library.\n\nIf there's a good way to implement helpers - then this looks like a good\napproach ...\n\nOr we just open-code the .insert() for update() and .find() for\ncontains() but it would be nice if those were friendlier.\n\n\nThis doesn't seem as nice anymore:\n\n\tif ((newList.find(ManualGain) == newList.end()) ||\n\t (newList.find(ManualBrightness) == newList.end())) {}\n\nwas:\n\tif (!(newList.contains(ManualGain) &&\n\t (newList.contains(ManualBrightness))) {}\n\n>>>>> +\n>>>>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["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 023276157F\n\tfor ;\n\tWed, 26 Jun 2019 15:14:37 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 417EE510;\n\tWed, 26 Jun 2019 15:14:37 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561554877;\n\tbh=HnoBks6Yz60lfzQ4mxM5TJUb8x2FUElYTff/fpRYIpU=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=u142g3bIN+Y6JNf/3/DywBHktGbxeBfG1zqvxZ3TO6PLAOXsPwUkl2PoxkEp1O+Mz\n\tcka+fSmYwatlvJMwlSrlPZXCANwKFHZZVtih7o/1jilr0bCWW5tRENXA0KQ8tcn9oM\n\tCU2aeyLvupGyW9wD2tALSTJvyHDe5NEfVGh/ZgUs=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tLibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t<20190623222132.GE6124@pendragon.ideasonboard.com>\n\t\n\t<20190624163045.GA24965@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 26 Jun 2019 14:14:34 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190624163045.GA24965@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 26 Jun 2019 13:14:38 -0000"}},{"id":2039,"web_url":"https://patchwork.libcamera.org/comment/2039/","msgid":"<20190626135108.GA6118@pendragon.ideasonboard.com>","date":"2019-06-26T13:51:08","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jun 26, 2019 at 02:14:34PM +0100, Kieran Bingham wrote:\n> Hi Laurent,\n> \n> A whole bunch of my development thoughts as I went through\n> converting/removing the ControlList abstraction.\n> \n> TLDR: I'm removing the Class abstraction and just using the\n> std::unordered_map directly through the typedef/using.\n> \n> Seems like it might be worth going back to being a map of straight\n> ControlID, {Control}Value pairs too - and keep the ControlInfo entirely\n> separated. Otherwise anytime a control is added to the list directly - a\n> lookup will have to be done on the ControlInfo even if it's not needed...\n\nI think that will depend on the usage patterns, but my gut feeling at\nthe moment is that you're right.\n\n> On 24/06/2019 17:30, Laurent Pinchart wrote:\n> > On Mon, Jun 24, 2019 at 05:02:55PM +0100, Kieran Bingham wrote:\n> >> On 23/06/2019 23:21, Laurent Pinchart wrote:\n> >>> On Sun, Jun 23, 2019 at 05:35:01PM +0200, Niklas Söderlund wrote:\n> >>>> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:\n> >>>>> Define a ControlList which wraps the STL std::unordered_map to contain\n> >>>>> a list of associative pairs of ControlInfo and Value items.\n> >>>>>\n> >>>>> A ControlInfo (or represented by its ControlId) together with a Value\n> >>>>> provide a control item which can be interpreted by the internal IPA and\n> >>>>> PipelineHandler components.\n> >>>>>\n> >>>>> To pass a set of controls around, a ControlList can be added to a\n> >>>\n> >>> s/ / /\n> >>\n> >> I wish vim reflow/automatic wrap would do that....\n> >>\n> >>>>> Request or stored locally to establish a set of default values for later\n> >>>>> reuse.\n> >>>>>\n> >>>>> Signed-off-by: Kieran Bingham \n> >>>>> ---\n> >>>>> include/libcamera/controls.h | 41 +++++++++++\n> >>>>> src/libcamera/controls.cpp | 128 +++++++++++++++++++++++++++++++++++\n> >>>>> 2 files changed, 169 insertions(+)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>>>> index 95198d41c4cf..5835b840a31b 100644\n> >>>>> --- a/include/libcamera/controls.h\n> >>>>> +++ b/include/libcamera/controls.h\n> >>>>> @@ -4,6 +4,9 @@\n> >>>>> *\n> >>>>> * controls.h - Control handling\n> >>>>> */\n> >>>>> +\n> >>>>> +#include \n> >>>>> +\n> >>>>> #ifndef __LIBCAMERA_CONTROLS_H__\n> >>>>> #define __LIBCAMERA_CONTROLS_H__\n> >>>>> \n> >>>>> @@ -53,6 +56,44 @@ private:\n> >>>>> \n> >>>>> std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> >>>>> \n> >>>>> +class ControlList\n> >>>>> +{\n> >>>>> +private:\n> >>>>> +\tstruct ControlInfoHasher {\n> >>>>> +\t\tstd::size_t operator()(const ControlInfo &ci) const\n> >>>>> +\t\t{\n> >>>>> +\t\t\treturn std::hash()(ci.id());\n> >>>>> +\t\t}\n> >>>>> +\t};\n> >>>>> +\n> >>>>> +\ttypedef std::unordered_map ControlListMap;\n> >>>\n> >>> We usually use the C++11 syntax\n> >>>\n> >>> \tusing ControlListMap = std::unordered_map;\n> >>\n> >> I prefer this :-D\n> >>\n> >> Updated.\n> >>\n> >>> I don't think you should copy the ControlInfo in the ControlList. \n> >>\n> >> I was trying not to - It was supposed to be a ControlInfo & but creating\n> >> the original ControlInfo's has caused me headache ...\n> >>\n> >>> If I\n> >>> understand your patch series correctly, ControlInfo should be created\n> >>> with the camera, and remain constant for the lifetime of the camera. \n> >>\n> >> Yes, I want to create ControlInfo structures which are constant for the\n> >> camera lifetime, and contain any values appropriate to that camera instance.\n> >>\n> >>> The\n> >>> map should thus use either the control ID or a pointer to the control\n> >>> info as the key.\n> >>\n> >> An ID should be easy to support, but means code will have to look up the\n> >> ControlInfo later, whereas a pointer to the ControlInfo needs to look it\n> >> up internally.\n> >>\n> >> This is essentially why this topic is still RFC for me.\n> >>\n> >>>> Is this needed to go first or can it be moved down to the other private \n> >>>> section?\n> >>>>\n> >>>>> +\n> >>>>> +public:\n> >>>>> +\tControlList();\n> >>>>> +\n> >>>>> +\tusing iterator = ControlListMap::iterator;\n> >>>>> +\tusing const_iterator = ControlListMap::const_iterator;\n> >>>>> +\n> >>>>> +\titerator begin() { return controls_.begin(); }\n> >>>>> +\titerator end() { return controls_.end(); }\n> >>>>> +\tconst_iterator begin() const { return controls_.begin(); }\n> >>>>> +\tconst_iterator end() const { return controls_.end(); }\n> >>>>> +\n> >>>>> +\tbool contains(ControlId id) const { return controls_.count(id); };\n> >>>>\n> >>>> Would not return controls_.find(id) != controls_.end() be better?\n> >>>>\n> >>>>> +\tbool empty() const { return controls_.empty(); }\n> >>>>> +\tsize_t size() const { return controls_.size(); }\n> >>>\n> >>> std::size_t\n> >>\n> >> Sure.\n> >>\n> >>>>> +\tvoid reset() { controls_.clear(); }\n> >>>\n> >>> Should we call this clear() ?\n> >>\n> >> Sure.\n> >>\n> >>>>> +\n> >>>>> +\tValue &operator[](ControlId id) { return controls_[id]; }\n> >>>>> +\n> >>>>> +\tvoid update(const ControlList &list);\n> >>>>> +\tvoid update(enum ControlId id, const Value &value);\n> >>>>> +\tvoid update(const ControlInfo &info, const Value &value);\n> >>>>> +\n> >>>>> +private:\n> >>>>> +\tControlListMap controls_;\n> >>>>> +};\n> >>>>> +\n> >>>>> } /* namespace libcamera */\n> >>>>> \n> >>>>> #endif /* __LIBCAMERA_CONTROLS_H__ */\n> >>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>>>> index b1be46ddb55e..7c55afffe4ca 100644\n> >>>>> --- a/src/libcamera/controls.cpp\n> >>>>> +++ b/src/libcamera/controls.cpp\n> >>>>> @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> >>>>> \treturn stream << info.toString();\n> >>>>> }\n> >>>>> \n> >>>>> +/**\n> >>>>> + * \\class ControlList\n> >>>>> + * \\brief Associates a list of ControlIds with their values.\n> >>>>> + *\n> >>>>> + * The ControlList stores a pair of ControlInfo and Value classes as an\n> >>>>> + * unordered map. Entries can be updated using array syntax such as\n> >>>>> + * myControlList[MyControlId] = myValue;\n> >>>>> + * or\n> >>>>> + * myNewValue = myControlList[MyControlId];\n> >>>>\n> >>>> I don't think this will update the value.\n> >>>>\n> >>>> I think you can drop the examples and the last sentence from the \n> >>>> documentation.\n> >>>>\n> >>>> With these issues addressed,\n> >>>>\n> >>>> Reviewed-by: Niklas Söderlund \n> >>>>\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Construct a ControlList\n> >>>>> + */\n> >>>>> +ControlList::ControlList()\n> >>>>> +{\n> >>>>> +}\n> >>>>> +\n> >>>\n> >>> If the constructor is empty and is the only one you can omit it, the\n> >>> compiler will generate one for your.\n> >>\n> >> Good point, removed.\n> >>\n> >>>>> +/**\n> >>>>> + * \\typedef ControlList::iterator\n> >>>>> + * \\brief Iterator for the controls contained within the list.\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\typedef ControlList::const_iterator\n> >>>>> + * \\brief Const iterator for the controls contained within the list.\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn iterator ControlList::begin()\n> >>>>> + * \\brief Retrieve an iterator to the first Control in the list\n> >>>>> + * \\return An iterator to the first Control in the list\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn iterator ControlList::end()\n> >>>>> + * \\brief Retrieve an iterator to the next element after the last controls in\n> >>>>> + * the instance.\n> >>>>> + * \\return An iterator to the element following the last control in the instance\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn const_iterator ControlList::begin() const\n> >>>>> + * \\brief Retrieve a const_iterator to the first Control in the list\n> >>>>> + * \\return A const_iterator to the first Control in the list\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn const_iterator ControlList::end() const\n> >>>>> + * \\brief Retrieve a constant iterator pointing to an empty element after the\n> >>>>> + * \\return A const iterator to the element following the last control in the\n> >>>>> + * instance\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn ControlList::contains()\n> >>>>> + * \\brief Identify if the List contains a control with the specified ControlId\n> >>>>> + * \\return True if the list contains a matching control, false otherwise\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn ControlList::empty()\n> >>>>> + * \\brief Identify if the list is empty\n> >>>>> + * \\return True if the list does not contain any control, false otherwise\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn ControlList::size()\n> >>>>> + * \\brief Retrieve the number of controls in the list\n> >>>>> + * \\return The number of Control entries stored in the list\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn ControlList::reset()\n> >>>>> + * \\brief Removes all controls from the list\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\fn ControlList::operator[]()\n> >>>>> + * \\brief Access the control with the given Id\n> >>>>> + * \\param id The id of the control to access\n> >>>>> + * \\return The Value of the control with a ControlId of \\a Id\n> >>>>> + */\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Update all Control values with the value from the given \\a list\n> >>>>> + * \\param list The list of controls to update or append to this list\n> >>>>> + *\n> >>>>> + * Update all controls in the ControlList, by the values given by \\a list\n> >>>>> + * If the list already contains a control of this ID then it will be overwritten\n> >>>>> + */\n> >>>>> +void ControlList::update(const ControlList &list)\n> >>>>> +{\n> >>>>> +\tfor (auto it : list) {\n> >>>>> +\t\tconst ControlInfo &info = it.first;\n> >>>>> +\t\tconst Value &value = it.second;\n> >>>>> +\n> >>>>> +\t\tcontrols_[info] = value;\n> >>>>> +\t}\n> >>>>> +}\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Update a control value in the list\n> >>>>> + * \\param id The ControlId of the control to update\n> >>>>> + * \\param value The new value for the updated control\n> >>>>> + *\n> >>>>> + * Set the Control value in the list to the appropriate value\n> >>>>> + * If the list already contains a control of this ID then it will be overwritten\n> >>>>> + */\n> >>>>> +void ControlList::update(enum ControlId id, const Value &value)\n> >>>>> +{\n> >>>>> +\tcontrols_[id] = value;\n> >>>>> +}\n> >>>>> +\n> >>>>> +/**\n> >>>>> + * \\brief Update a control value in the list.\n> >>>>> + * \\param info The ControlInfo of the control to update\n> >>>>> + * \\param value The new value for the updated control\n> >>>>> + *\n> >>>>> + * Set the Control value in the list to the appropriate value.\n> >>>>> + * If the list already contains a control of the Id matching the ControlInfo\n> >>>>> + * then it will be overwritten.\n> >>>>> + */\n> >>>>> +void ControlList::update(const ControlInfo &info, const Value &value)\n> >>>>> +{\n> >>>>> +\tcontrols_[info] = value;\n> >>>>> +}\n> >>>\n> >>> Do we really need this class ? Can't we just expose \n> >>>\n> >>> \tusing ControlList = std::unordered_map;\n> >>\n> >> Hrm ... well I think the intention was to be able to add some extra\n> >> validation later... but I now wonder if just the ControlList 'using'\n> >> might be easier, and remove a lot of rubbish passthrough code ...\n> >>\n> >> I'll give it a go - but I don't want to throw away a whole bunch of code\n> >> to re-add it again soon after ...\n> > \n> > git history to the rescue :-) (and mailing list archives)\n> > \n> >> Best save this as a branch (well although it's saved on list at least)....\n> >>\n> >>> ? The last two update functions would be replaced by usage of\n> >>> operator[], and the first one with\n> >>>\n> >>> template< class InputIt >\n> >>> void std::unordered_map::insert( InputIt first, InputIt last );\n> >>\n> >> 'just like that' ?\n> > \n> > controls.insert(otherControls.begin(), otherControls.end());\n> \n> I wonder if some helpers would make it 'nicer', but because there is no\n> class - it feels a bit more ugly (loses it's OO?):\n> \n> void controlListUpdate(targetList, sourceList) {\n> \ttargetList.insert(sourceList.begin(), sourceList.end());\n> }\n> \n> Any ideas or suggestions on how to do these helpers nicely? Ideally I'd\n> want the ControlList to look like a class object - but I don't think I\n> can just extend the std::unordered_map() interface ...\n> \n> I'd also want a helper for the .contains() method\n> \n> (which is a shame, because in C++20 .contains() is available directly on\n> the unordered_map)\n\nHow about keeping the ControlList class, but making it derive from the\ncontainer ? That would allow you to add the few helpers you need without\na need to proxy everything else. You also won't need to document or test\nthe proxy methods.\n\n> Perhaps it's a non-issue anyway, as if we only care about initialising a\n> list with a 'preset' set of controls, then that can be done with the\n> operator=():\n> \n> ControlList newList = defaultList;\n> \n> But that removes any existing items in the new list, so can't be used to\n> update an existing list.\n> \n> Still if that becomes necessary - then we can worry about adding the helper.\n> \n> Likewise, perhaps the usage of .contains() won't be really necessary\n> just yet, and it would be more optimal to use the find() directly so\n> that in the event that it is found, you don't then have to 'search again'.\n> \n> \n> TLDR: I've cancelled out my concerns :) - dropping the Class and\n> replacing with the 'using' typedef.\n> \n> \n> >> I'll play there next.\n> >>\n> >> Removing the ControlList class would remove my desire to have all the\n> >> tests that you disagree with later in the series ...\n> > \n> > I swear the proposal has nothing to do with the test discussion :-)\n> \n> Hahah - of course, I actually like the idea of just using the typdef on\n> the map itself. I don't want to pointlessly wrap the standard library.\n> \n> If there's a good way to implement helpers - then this looks like a good\n> approach ...\n> \n> Or we just open-code the .insert() for update() and .find() for\n> contains() but it would be nice if those were friendlier.\n> \n> \n> This doesn't seem as nice anymore:\n> \n> \tif ((newList.find(ManualGain) == newList.end()) ||\n> \t (newList.find(ManualBrightness) == newList.end())) {}\n> \n> was:\n> \tif (!(newList.contains(ManualGain) &&\n> \t (newList.contains(ManualBrightness))) {}\n> \n> >>>>> +\n> >>>>> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["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 732866157F\n\tfor ;\n\tWed, 26 Jun 2019 15:53:38 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3DC7510;\n\tWed, 26 Jun 2019 15:53:37 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561557218;\n\tbh=HKhZ9IX0s7nG8yJA1VZ28f/HQa9TPKLXbOtsDdF9mfU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j3aOwWX0rClmJlSHiqBQZ4qLJhuRSk4pp0+H5rggnB7Cytm704iY+nFmv44tJTWQ2\n\tQojDvcXIf5yMt3utVu+M66bmi3SnoX+V7VtRBiVXVPMvPi5e6OCIhi1HLUb04STW+8\n\tihvekl+tbVd8cBmRzFUQnArhAkzvE3OF1Nj7GyB0=","Date":"Wed, 26 Jun 2019 16:51:08 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tLibCamera Devel ","Message-ID":"<20190626135108.GA6118@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t<20190623222132.GE6124@pendragon.ideasonboard.com>\n\t\n\t<20190624163045.GA24965@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 26 Jun 2019 13:53:38 -0000"}},{"id":2041,"web_url":"https://patchwork.libcamera.org/comment/2041/","msgid":"<1ea505ea-e46c-110b-608c-eef16ba90686@ideasonboard.com>","date":"2019-06-26T14:02:59","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 26/06/2019 14:51, Laurent Pinchart wrote:\n \n\n>> Any ideas or suggestions on how to do these helpers nicely? Ideally I'd\n>> want the ControlList to look like a class object - but I don't think I\n>> can just extend the std::unordered_map() interface ...\n>>\n>> I'd also want a helper for the .contains() method\n>>\n>> (which is a shame, because in C++20 .contains() is available directly on\n>> the unordered_map)\n> \n> How about keeping the ControlList class, but making it derive from the\n> container ? That would allow you to add the few helpers you need without\n> a need to proxy everything else. You also won't need to document or test\n> the proxy methods.\n\nI sort of want to do exactly that - but too many comments online say\n\"Never derive the STL containers\":\n\nhttps://stackoverflow.com/questions/10477839/c-inheriting-from-stdmap\nhttps://www.researchgate.net/post/I_really_want_to_understand_if_there_is_any_reason_why_one_should_not_inherit_from_the_stl_vector_class_in_c2\n\n\nI think the reasoning is that the STL does not implement virtual\ndestructors, but does this matter if I don't implement a custom destructor?\n\n","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E9156157F\n\tfor ;\n\tWed, 26 Jun 2019 16:03:03 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F515510;\n\tWed, 26 Jun 2019 16:03:02 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561557782;\n\tbh=swGWiZ4XD9615Kc0ck+ls3VZnzcGECuuzFgreqPCEs8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=W5uhoz9+r4vl6xc80tuMzkueIHONIL58imV+3Y9xey9WB0xOxQ02wxlGsK+E0T5Jl\n\tBYW3F+KD4bHmkt6j+JshRpGJ8ZO1ADRe+wDnEDRjKh1MkmibvPz32g+QoSYusIcHty\n\tLuwMkpORD+HnhmRL7D2q6gk1HXH4dryjvQVqmH2o=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tLibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t<20190623222132.GE6124@pendragon.ideasonboard.com>\n\t\n\t<20190624163045.GA24965@pendragon.ideasonboard.com>\n\t\n\t<20190626135108.GA6118@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<1ea505ea-e46c-110b-608c-eef16ba90686@ideasonboard.com>","Date":"Wed, 26 Jun 2019 15:02:59 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190626135108.GA6118@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 26 Jun 2019 14:03:03 -0000"}},{"id":2042,"web_url":"https://patchwork.libcamera.org/comment/2042/","msgid":"<20190626145353.GD5015@pendragon.ideasonboard.com>","date":"2019-06-26T14:53:53","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jun 26, 2019 at 03:02:59PM +0100, Kieran Bingham wrote:\n> On 26/06/2019 14:51, Laurent Pinchart wrote:\n> \n> \n> >> Any ideas or suggestions on how to do these helpers nicely? Ideally I'd\n> >> want the ControlList to look like a class object - but I don't think I\n> >> can just extend the std::unordered_map() interface ...\n> >>\n> >> I'd also want a helper for the .contains() method\n> >>\n> >> (which is a shame, because in C++20 .contains() is available directly on\n> >> the unordered_map)\n> > \n> > How about keeping the ControlList class, but making it derive from the\n> > container ? That would allow you to add the few helpers you need without\n> > a need to proxy everything else. You also won't need to document or test\n> > the proxy methods.\n> \n> I sort of want to do exactly that - but too many comments online say\n> \"Never derive the STL containers\":\n> \n> https://stackoverflow.com/questions/10477839/c-inheriting-from-stdmap\n> https://www.researchgate.net/post/I_really_want_to_understand_if_there_is_any_reason_why_one_should_not_inherit_from_the_stl_vector_class_in_c2\n> \n> I think the reasoning is that the STL does not implement virtual\n> destructors, but does this matter if I don't implement a custom destructor?\n\nIt's one of the reasons. Another one seems that the standard says not to\ndo so, but I believe the reasons behind it are not relevant in this\ncase. If the only purpose of this inheritance is to add helper methods\nto the container, without adding data members, I think it should be\nfine. If we have to add data members or want to override existing\nmethods, then we're reaching grey waters, and I wouldn't be comfortable\ngoing there.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A46FC6157F\n\tfor ;\n\tWed, 26 Jun 2019 16:56:22 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 10982510;\n\tWed, 26 Jun 2019 16:56:22 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561560982;\n\tbh=3oWihyxbMEKbIleeFpk968ZuGriYatyp/JqppnPPayU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DH6EMDCdqI2m3ecr4jQY85N/s+GR249RoExRjJIOq4GbGGAGdtLIegJK1B/+rvq9I\n\tHH5z+GsRVxfO+uWgp6omkfoWDFmUjE2hugMbhBEHwAdARq6Wtka7+pX0f4G0dEGUF9\n\trthV1fN+xOC8ZlKI8LQf7qlUJN3vU7bvfg+dRTWk=","Date":"Wed, 26 Jun 2019 17:53:53 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tLibCamera Devel ","Message-ID":"<20190626145353.GD5015@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-6-kieran.bingham@ideasonboard.com>\n\t<20190623153501.GD32581@bigcity.dyn.berto.se>\n\t<20190623222132.GE6124@pendragon.ideasonboard.com>\n\t\n\t<20190624163045.GA24965@pendragon.ideasonboard.com>\n\t\n\t<20190626135108.GA6118@pendragon.ideasonboard.com>\n\t<1ea505ea-e46c-110b-608c-eef16ba90686@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1ea505ea-e46c-110b-608c-eef16ba90686@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a\n\tControlList","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 26 Jun 2019 14:56:22 -0000"}}]