[{"id":2105,"web_url":"https://patchwork.libcamera.org/comment/2105/","msgid":"<20190702000632.GG9228@bigcity.dyn.berto.se>","date":"2019-07-02T00:06:32","subject":"Re: [libcamera-devel] [PATCH v4 05/13] libcamera: controls: Extend\n\tControlList to access controls by ID","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-07-01 23:14:56 +0300, Laurent Pinchart wrote:\n> The ControlList class implements a map from control specifier to control\n> ID. To avoid constant lookups of ControlInfo when using the class in the\n> libcamera core or in pipeline handlers, the map uses ControlInfo\n> pointers instead of ControlId values. This is however not very\n> convenient for applications or pipeline handlers, as they would be\n> forced to first look up the ControlInfo pointers for the controls they\n> want to access. Facilitate ease of use of ControlLists by implementing\n> an internal lookup of the ControlInfo from the controls provided by the\n> Camera.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Jacopo Mondi \n> ---\n> Changes since v3:\n> \n> - Typo fixes\n> ---\n> include/libcamera/controls.h | 2 ++\n> src/libcamera/controls.cpp | 54 ++++++++++++++++++++++++++++++++++++\n> 2 files changed, 56 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 18293c9462cf..fbb3a62274c6 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -117,11 +117,13 @@ public:\n> \tconst_iterator begin() const { return controls_.begin(); }\n> \tconst_iterator end() const { return controls_.end(); }\n> \n> +\tbool contains(ControlId id) const;\n> \tbool contains(const ControlInfo *info) const;\n> \tbool empty() const { return controls_.empty(); }\n> \tstd::size_t size() const { return controls_.size(); }\n> \tvoid clear() { controls_.clear(); }\n> \n> +\tControlValue &operator[](ControlId id);\n> \tControlValue &operator[](const ControlInfo *info) { return controls_[info]; }\n> \n> \tvoid update(const ControlList &list);\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index cd2cf337b379..42a2f8990bf6 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"log.h\"\n> #include \"utils.h\"\n> \n> @@ -387,6 +389,30 @@ ControlList::ControlList(Camera *camera)\n> * list\n> */\n> \n> +/**\n> + * \\brief Check if the list contains a control with the specified \\a id\n> + * \\param[in] id The control ID\n> + *\n> + * The behaviour is undefined if the control \\a id is not supported by the\n> + * camera that the ControlList refers to.\n\nIs it undefined ? It looks like an Error is be printed and false is \nreturned, right? Same comment bellow.\n\nWith or without this nit fixed,\n\nReviewed-by: Niklas Söderlund \n\n> + *\n> + * \\return True if the list contains a matching control, false otherwise\n> + */\n> +bool ControlList::contains(ControlId id) const\n> +{\n> +\tconst ControlInfoMap &controls = camera_->controls();\n> +\tconst auto iter = controls.find(id);\n> +\tif (iter == controls.end()) {\n> +\t\tLOG(Controls, Error)\n> +\t\t\t<< \"Camera \" << camera_->name()\n> +\t\t\t<< \" does not support control \" << id;\n> +\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn controls_.find(&iter->second) != controls_.end();\n> +}\n> +\n> /**\n> * \\brief Check if the list contains a control with the specified \\a info\n> * \\param[in] info The control info\n> @@ -414,6 +440,34 @@ bool ControlList::contains(const ControlInfo *info) const\n> * \\brief Removes all controls from the list\n> */\n> \n> +/**\n> + * \\brief Access or insert the control specified by \\a id\n> + * \\param[in] id The control ID\n> + *\n> + * This method returns a reference to the control identified by \\a id, inserting\n> + * it in the list if the ID is not already present.\n> + *\n> + * The behaviour is undefined if the control \\a id is not supported by the\n> + * camera that the ControlList refers to.\n> + *\n> + * \\return A reference to the value of the control identified by \\a id\n> + */\n> +ControlValue &ControlList::operator[](ControlId id)\n> +{\n> +\tconst ControlInfoMap &controls = camera_->controls();\n> +\tconst auto iter = controls.find(id);\n> +\tif (iter == controls.end()) {\n> +\t\tLOG(Controls, Error)\n> +\t\t\t<< \"Camera \" << camera_->name()\n> +\t\t\t<< \" does not support control \" << id;\n> +\n> +\t\tstatic ControlValue empty;\n> +\t\treturn empty;\n> +\t}\n> +\n> +\treturn controls_[&iter->second];\n> +}\n> +\n> /**\n> * \\fn ControlList::operator[](const ControlInfo *info)\n> * \\brief Access or insert the control specified by \\a info\n> -- \n> Regards,\n> \n> Laurent Pinchart\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-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3023760C2B\n\tfor ;\n\tTue, 2 Jul 2019 02:06:34 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id q26so10012083lfc.3\n\tfor ;\n\tMon, 01 Jul 2019 17:06:34 -0700 (PDT)","from localhost (customer-145-14-112-32.stosn.net. [145.14.112.32])\n\tby smtp.gmail.com with ESMTPSA id\n\tn10sm2785843lfe.24.2019.07.01.17.06.32\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tMon, 01 Jul 2019 17:06:32 -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=XEL8FXZ8M4Ymey2HhRoYUlKIgQ0P88Logs5TnPnJYfw=;\n\tb=KEGpHV+fN0k+W6yOiozOKT87tq7ByXVLxrUnWtiIceeslH+tLksYWWCtSwWdv2pJmW\n\tKw9QVAC7SNcg7FEKrRw0NNNFWaEnCakv4p7d+s4uonDaayZPKy3YSnfL1/8lALocdHIx\n\tVG8PoesZkrEg72BMxGhyO83IB7h5XB0/x43HMQSoCc2o+3hMlH94zVUN6q7MtntAlqqQ\n\tA1WsAIoOWCkWFw+h+vYDjcNEbPrKIn16iKUt1gIOTRsex/WuND9/KWrYJOj/u7UPmzfe\n\tePAx6JiHXINR8R+DqVNTrRTUu1U7lfsVRVUETwr3NYVJYXLEgEMgyNewju0vtVEAX1P2\n\tNinQ==","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=XEL8FXZ8M4Ymey2HhRoYUlKIgQ0P88Logs5TnPnJYfw=;\n\tb=SNWGkGCPQ7tvUhScK5Xcn0cqi/ygeatJ9P0DwfuuQ4nEw7uiwg8RhhwcSECFK3ac6u\n\tz52vE/DUpbR4lr+/HDn7r82ij+8SNa3fP8OKE1iRtHV5YZ4vR3PSiwEAI6AGb8Y+QYfJ\n\tL/y3CZBFJSJtrBkYYaA2TkI7Q2Fbqsxu1JXoItLTWLNyKHOyVy6AwaY5UYQZkhRhvetU\n\tAOHHK2ao3ZF9H24OY0RYc+2TDOU6j6aQsXfIgcwx4e3c9FwtRpajqWOc8WL6/9KWyz5K\n\tjNypdyIn7H/xMG41noI6tOXDKwV8S1L0hsKnv8Vpvauo52tRVoG9QUKqUG6hm3i4Dzqp\n\tKv6w==","X-Gm-Message-State":"APjAAAUbDhuz3Wd8tBDaaGdm5fD9ElAQlPgr7yMId9RAXRVIMYEw/igS\n\tFiRxMeGhKSICp4Ih0FcnBye4TQ==","X-Google-Smtp-Source":"APXvYqzsh7LnU830Mz0YG9a6A7Ln1qEyr5KWXbPmqhuFX+c5VhQGkyWJuvslxLwrbYW3SmFVxaRPpw==","X-Received":"by 2002:ac2:5212:: with SMTP id\n\ta18mr12804649lfl.50.1562025993463; \n\tMon, 01 Jul 2019 17:06:33 -0700 (PDT)","Date":"Tue, 2 Jul 2019 02:06:32 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190702000632.GG9228@bigcity.dyn.berto.se>","References":"<20190701201504.28487-1-laurent.pinchart@ideasonboard.com>\n\t<20190701201504.28487-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190701201504.28487-6-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v4 05/13] libcamera: controls: Extend\n\tControlList to access controls by ID","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":"Tue, 02 Jul 2019 00:06:34 -0000"}},{"id":2106,"web_url":"https://patchwork.libcamera.org/comment/2106/","msgid":"<20190702000858.GE24839@pendragon.ideasonboard.com>","date":"2019-07-02T00:08:58","subject":"Re: [libcamera-devel] [PATCH v4 05/13] libcamera: controls: Extend\n\tControlList to access controls by ID","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Jul 02, 2019 at 02:06:32AM +0200, Niklas Söderlund wrote:\n> On 2019-07-01 23:14:56 +0300, Laurent Pinchart wrote:\n> > The ControlList class implements a map from control specifier to control\n> > ID. To avoid constant lookups of ControlInfo when using the class in the\n> > libcamera core or in pipeline handlers, the map uses ControlInfo\n> > pointers instead of ControlId values. This is however not very\n> > convenient for applications or pipeline handlers, as they would be\n> > forced to first look up the ControlInfo pointers for the controls they\n> > want to access. Facilitate ease of use of ControlLists by implementing\n> > an internal lookup of the ControlInfo from the controls provided by the\n> > Camera.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > Changes since v3:\n> > \n> > - Typo fixes\n> > ---\n> > include/libcamera/controls.h | 2 ++\n> > src/libcamera/controls.cpp | 54 ++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 56 insertions(+)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 18293c9462cf..fbb3a62274c6 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -117,11 +117,13 @@ public:\n> > \tconst_iterator begin() const { return controls_.begin(); }\n> > \tconst_iterator end() const { return controls_.end(); }\n> > \n> > +\tbool contains(ControlId id) const;\n> > \tbool contains(const ControlInfo *info) const;\n> > \tbool empty() const { return controls_.empty(); }\n> > \tstd::size_t size() const { return controls_.size(); }\n> > \tvoid clear() { controls_.clear(); }\n> > \n> > +\tControlValue &operator[](ControlId id);\n> > \tControlValue &operator[](const ControlInfo *info) { return controls_[info]; }\n> > \n> > \tvoid update(const ControlList &list);\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index cd2cf337b379..42a2f8990bf6 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -10,6 +10,8 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \"log.h\"\n> > #include \"utils.h\"\n> > \n> > @@ -387,6 +389,30 @@ ControlList::ControlList(Camera *camera)\n> > * list\n> > */\n> > \n> > +/**\n> > + * \\brief Check if the list contains a control with the specified \\a id\n> > + * \\param[in] id The control ID\n> > + *\n> > + * The behaviour is undefined if the control \\a id is not supported by the\n> > + * camera that the ControlList refers to.\n> \n> Is it undefined ? It looks like an Error is be printed and false is \n> returned, right? Same comment bellow.\n\nUndefined means we don't guarantee what will happen, and don't guarantee\nthat the current behaviour will remain stable. Applications must not do\nthis.\n\nFor the contains() method we may specify that the function will return\nfalse. For the operator[], however, I think we should keep the behaviour\nunspecified as we don't do much.\n\n> With or without this nit fixed,\n> \n> Reviewed-by: Niklas Söderlund \n> \n> > + *\n> > + * \\return True if the list contains a matching control, false otherwise\n> > + */\n> > +bool ControlList::contains(ControlId id) const\n> > +{\n> > +\tconst ControlInfoMap &controls = camera_->controls();\n> > +\tconst auto iter = controls.find(id);\n> > +\tif (iter == controls.end()) {\n> > +\t\tLOG(Controls, Error)\n> > +\t\t\t<< \"Camera \" << camera_->name()\n> > +\t\t\t<< \" does not support control \" << id;\n> > +\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn controls_.find(&iter->second) != controls_.end();\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the list contains a control with the specified \\a info\n> > * \\param[in] info The control info\n> > @@ -414,6 +440,34 @@ bool ControlList::contains(const ControlInfo *info) const\n> > * \\brief Removes all controls from the list\n> > */\n> > \n> > +/**\n> > + * \\brief Access or insert the control specified by \\a id\n> > + * \\param[in] id The control ID\n> > + *\n> > + * This method returns a reference to the control identified by \\a id, inserting\n> > + * it in the list if the ID is not already present.\n> > + *\n> > + * The behaviour is undefined if the control \\a id is not supported by the\n> > + * camera that the ControlList refers to.\n> > + *\n> > + * \\return A reference to the value of the control identified by \\a id\n> > + */\n> > +ControlValue &ControlList::operator[](ControlId id)\n> > +{\n> > +\tconst ControlInfoMap &controls = camera_->controls();\n> > +\tconst auto iter = controls.find(id);\n> > +\tif (iter == controls.end()) {\n> > +\t\tLOG(Controls, Error)\n> > +\t\t\t<< \"Camera \" << camera_->name()\n> > +\t\t\t<< \" does not support control \" << id;\n> > +\n> > +\t\tstatic ControlValue empty;\n> > +\t\treturn empty;\n> > +\t}\n> > +\n> > +\treturn controls_[&iter->second];\n> > +}\n> > +\n> > /**\n> > * \\fn ControlList::operator[](const ControlInfo *info)\n> > * \\brief Access or insert the control specified by \\a info","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 E3F9660C2B\n\tfor ;\n\tTue, 2 Jul 2019 02:09:17 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 87A00E34;\n\tTue, 2 Jul 2019 02:09:17 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562026157;\n\tbh=bLe+f8rXMTbehlcOn+KGsnliRjr6yu8orQ+xPEli4wQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r9QO503FHFiTvk0EWPtygu8/eImBNmtXnXmZeh6giyCOLpaJzkwn2LdLvrmkw/xkB\n\tABp/RmkEom/n4BdMG7+fhzTU6VnR5JCyfRCap7691avDmHkpYwzzLxBrOOHWQNJXU6\n\tZWZxEAixtpP3o/R8k287OCeR4rcOXCRcJ4oBt0GE=","Date":"Tue, 2 Jul 2019 03:08:58 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190702000858.GE24839@pendragon.ideasonboard.com>","References":"<20190701201504.28487-1-laurent.pinchart@ideasonboard.com>\n\t<20190701201504.28487-6-laurent.pinchart@ideasonboard.com>\n\t<20190702000632.GG9228@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":"<20190702000632.GG9228@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 05/13] libcamera: controls: Extend\n\tControlList to access controls by ID","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":"Tue, 02 Jul 2019 00:09:18 -0000"}}]