[{"id":32378,"web_url":"https://patchwork.libcamera.org/comment/32378/","msgid":"<20241125231910.GA19381@pendragon.ideasonboard.com>","date":"2024-11-25T23:19:10","subject":"Re: [PATCH 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Nov 26, 2024 at 12:30:02AM +0900, Paul Elder wrote:\n> Add support to ControlId for querying direction information. This allows\n> applications to query whether a ControlId is meant for being set in\n> controls or to be returned in metadata or both. This also has a side\n> effect of properly encoding this information, as previously it was only\n> mentioned losely and inconsistently in the control id definition.\n> \n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/controls.h | 27 +++++++++++++++++++-\n> src/libcamera/control_ids.cpp.in | 4 +--\n> src/libcamera/controls.cpp | 42 ++++++++++++++++++++++++++++++--\n> 3 files changed, 68 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 3cfe2de5973c..cd338ac0d653 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -235,8 +236,18 @@ private:\n> class ControlId\n> {\n> public:\n> +\tenum class Direction {\n> +\t\tIn = (1 << 0),\n> +\t\tOut = (1 << 1),\n> +\t};\n> +\n> +\tusing DirectionFlags = Flags;\n> +\n> \tControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> \t\t ControlType type, std::size_t size = 0,\n> +\t\t const DirectionFlags &direction =\n> +\t\t\t static_cast(Direction::In) |\n> +\t\t\t static_cast(Direction::Out),\n\nI think we could make this argument explicit, without a default value,\nas all controls have a direction.\n\n> \t\t const std::map &enumStrMap = {});\n> \n> \tunsigned int id() const { return id_; }\n> @@ -245,6 +256,16 @@ public:\n> \tControlType type() const { return type_; }\n> \tbool isArray() const { return size_ > 0; }\n> \tstd::size_t size() const { return size_; }\n> +\tbool isInput() const\n\nWith the changes proposed in the cover letter this now holds on a single\nline:\n\n\tbool isInput() const { return !!(direction_ & Direction::In); }\n\n> +\t{\n> +\t\treturn static_cast(\n> +\t\t\tdirection_ & static_cast(Direction::In));\n> +\t}\n> +\tbool isOutput() const\n\nSame here.\n\n> +\t{\n> +\t\treturn static_cast(\n> +\t\t\tdirection_ & static_cast(Direction::Out));\n> +\t}\n> \tconst std::map &enumerators() const { return reverseMap_; }\n> \n> private:\n> @@ -255,6 +276,7 @@ private:\n> \tstd::string vendor_;\n> \tControlType type_;\n> \tstd::size_t size_;\n> +\tDirectionFlags direction_;\n> \tstd::map enumStrMap_;\n> \tstd::map reverseMap_;\n> };\n> @@ -286,9 +308,12 @@ public:\n> \tusing type = T;\n> \n> \tControl(unsigned int id, const char *name, const char *vendor,\n> +\t\tconst ControlId::DirectionFlags &direction =\n> +\t\t\tstatic_cast(ControlId::Direction::In) |\n> +\t\t\tstatic_cast(ControlId::Direction::Out),\n\nDo we want to pass this as an argument to the constructor, or as a\ntemplate argument ? The latter would allow handling the direction at\ncompile time (not sure what the use case would be though).\n\n> \t\tconst std::map &enumStrMap = {})\n> \t\t: ControlId(id, name, vendor, details::control_type>::value,\n> -\t\t\t details::control_type>::size, enumStrMap)\n> +\t\t\t details::control_type>::size, direction, enumStrMap)\n> \t{\n> \t}\n> \n> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> index afe9e2c96610..65668d486dbc 100644\n> --- a/src/libcamera/control_ids.cpp.in\n> +++ b/src/libcamera/control_ids.cpp.in\n> @@ -89,9 +89,9 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> \t{ \"{{enum.name}}\", {{enum.name}} },\n> {%- endfor %}\n> };\n> -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.name}}NameValueMap);\n> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);\n> {% else -%}\n> -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\");\n> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}});\n> {% endif -%}\n> {%- endfor %}\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 2efecf0fc52b..30eb17e7f064 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -397,14 +397,15 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> * \\param[in] vendor The vendor name\n> * \\param[in] type The control data type\n> * \\param[in] size The size of the array control, or 0 if scalar control\n> + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n\nLine wrap ?\n\n> * \\param[in] enumStrMap The map from enum names to values (optional)\n> */\n> ControlId::ControlId(unsigned int id, const std::string &name,\n> \t\t const std::string &vendor, ControlType type,\n> -\t\t std::size_t size,\n> +\t\t std::size_t size, const DirectionFlags &direction,\n> \t\t const std::map &enumStrMap)\n> \t: id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> -\t enumStrMap_(enumStrMap)\n> +\t direction_(direction), enumStrMap_(enumStrMap)\n> {\n> \tfor (const auto &pair : enumStrMap_)\n> \t\treverseMap_[pair.second] = pair.first;\n> @@ -440,6 +441,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\return True if the control is an array control, false otherwise\n> */\n> \n> +/**\n> + * \\fn bool ControlId::isInput() const\n> + * \\brief Determine if the control is available to be used as an input control\n> + *\n> + * Controls can be used either as input as a control, or as output in metadata.\n> + * This function checks if the control is allowed to be used as the former.\n> + *\n> + * \\return True if the control can be used as an input control, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn bool ControlId::isOutput() const\n> + * \\brief Determine if the control is available to be used in output metadata\n> + *\n> + * Controls can be used either as input as a control, or as output in metadata.\n\n\"Controls can be used [...] as a control\"\n\n:S I don't have a better suggestion for now that wouldn't involve a\ntree-wide rename, so I'm OK with this.\n\n> + * This function checks if the control is allowed to be used as the latter.\n> + *\n> + * \\return True if the control can be returned in output metadata, false otherwise\n> + */\n> +\n> /**\n> * \\fn std::size_t ControlId::size() const\n> * \\brief Retrieve the size of the control if it is an array control\n> @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\return True if \\a lhs.id() is equal to \\a rhs, false otherwise\n> */\n> \n> +/**\n> + * \\enum ControlId::Direction\n> + * \\brief The direction that a control of the ControlId is capable of being passed from/to\n> + *\n> + * \\var ControlId::Direction::In\n> + * \\brief The control can be passed in controls as input\n> + *\n> + * \\var ControlId::Direction::Out\n> + * \\brief The control can be returned in output as metadata\n\nThe two definitions are not consistent: \"in controls as input\" vs. \"in\noutput as metadata\".\n\n> + */\n> +\n> +/**\n> + * \\typedef ControlId::DirectionFlags\n> + * \\brief A wrapper for ControlId::Direction so that it can be used as flags\n> + */\n> +\n> /**\n> * \\class Control\n> * \\brief Describe a control and its intrinsic properties\n> @@ -504,6 +541,7 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\param[in] id The control numerical ID\n> * \\param[in] name The control name\n> * \\param[in] vendor The vendor name\n> + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> * \\param[in] enumStrMap The map from enum names to values (optional)\n> *\n> * The control data type is automatically deduced from the template type T.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C1CF8BD160\n\tfor ;\n\tMon, 25 Nov 2024 23:19:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8BF66605E;\n\tTue, 26 Nov 2024 00:19:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 714DA66047\n\tfor ;\n\tTue, 26 Nov 2024 00:19:20 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4A81E526;\n\tTue, 26 Nov 2024 00:18:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VDEAz0BK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732576738;\n\tbh=H9AS7rtZ2IpG1+gbTliVZMrDkypsbNR8cyD2wORWzbg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VDEAz0BKSlITBxDrbMixXerZxZF7d40nC732YDzZRibJcqXPOEM2YGKAUWOm/ekHS\n\txAaxGlk+LMfwxW6VKNqJ0PiiimX4b8KWY7Hv90vAwMxzQfGCCG+I4Z7ZyFmPeeAFLT\n\tkvnHDHN+GMqkBuDU84Be6WDGXmv9hYinZdy5BdMY=","Date":"Tue, 26 Nov 2024 01:19:10 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"<20241125231910.GA19381@pendragon.ideasonboard.com>","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241125153003.3309066-4-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32406,"web_url":"https://patchwork.libcamera.org/comment/32406/","msgid":"","date":"2024-11-27T08:40:43","subject":"Re: [PATCH 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Tue, Nov 26, 2024 at 01:19:10AM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Tue, Nov 26, 2024 at 12:30:02AM +0900, Paul Elder wrote:\n> > Add support to ControlId for querying direction information. This allows\n> > applications to query whether a ControlId is meant for being set in\n> > controls or to be returned in metadata or both. This also has a side\n> > effect of properly encoding this information, as previously it was only\n> > mentioned losely and inconsistently in the control id definition.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/controls.h | 27 +++++++++++++++++++-\n> > src/libcamera/control_ids.cpp.in | 4 +--\n> > src/libcamera/controls.cpp | 42 ++++++++++++++++++++++++++++++--\n> > 3 files changed, 68 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 3cfe2de5973c..cd338ac0d653 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -235,8 +236,18 @@ private:\n> > class ControlId\n> > {\n> > public:\n> > +\tenum class Direction {\n> > +\t\tIn = (1 << 0),\n> > +\t\tOut = (1 << 1),\n> > +\t};\n> > +\n> > +\tusing DirectionFlags = Flags;\n> > +\n> > \tControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> > \t\t ControlType type, std::size_t size = 0,\n> > +\t\t const DirectionFlags &direction =\n> > +\t\t\t static_cast(Direction::In) |\n> > +\t\t\t static_cast(Direction::Out),\n> \n> I think we could make this argument explicit, without a default value,\n> as all controls have a direction.\n\nIt was causing problems in generating v4l2 controls in V4L2Device, and\nin ControlSerializer, so I'll leave it in.\n\n> \n> > \t\t const std::map &enumStrMap = {});\n> > \n> > \tunsigned int id() const { return id_; }\n> > @@ -245,6 +256,16 @@ public:\n> > \tControlType type() const { return type_; }\n> > \tbool isArray() const { return size_ > 0; }\n> > \tstd::size_t size() const { return size_; }\n> > +\tbool isInput() const\n> \n> With the changes proposed in the cover letter this now holds on a single\n> line:\n> \n> \tbool isInput() const { return !!(direction_ & Direction::In); }\n> \n> > +\t{\n> > +\t\treturn static_cast(\n> > +\t\t\tdirection_ & static_cast(Direction::In));\n> > +\t}\n> > +\tbool isOutput() const\n> \n> Same here.\n> \n> > +\t{\n> > +\t\treturn static_cast(\n> > +\t\t\tdirection_ & static_cast(Direction::Out));\n> > +\t}\n> > \tconst std::map &enumerators() const { return reverseMap_; }\n> > \n> > private:\n> > @@ -255,6 +276,7 @@ private:\n> > \tstd::string vendor_;\n> > \tControlType type_;\n> > \tstd::size_t size_;\n> > +\tDirectionFlags direction_;\n> > \tstd::map enumStrMap_;\n> > \tstd::map reverseMap_;\n> > };\n> > @@ -286,9 +308,12 @@ public:\n> > \tusing type = T;\n> > \n> > \tControl(unsigned int id, const char *name, const char *vendor,\n> > +\t\tconst ControlId::DirectionFlags &direction =\n> > +\t\t\tstatic_cast(ControlId::Direction::In) |\n> > +\t\t\tstatic_cast(ControlId::Direction::Out),\n> \n> Do we want to pass this as an argument to the constructor, or as a\n> template argument ? The latter would allow handling the direction at\n> compile time (not sure what the use case would be though).\n\nI think I'll keep it as regular parameters.\n\n> \n> > \t\tconst std::map &enumStrMap = {})\n> > \t\t: ControlId(id, name, vendor, details::control_type>::value,\n> > -\t\t\t details::control_type>::size, enumStrMap)\n> > +\t\t\t details::control_type>::size, direction, enumStrMap)\n> > \t{\n> > \t}\n> > \n> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > index afe9e2c96610..65668d486dbc 100644\n> > --- a/src/libcamera/control_ids.cpp.in\n> > +++ b/src/libcamera/control_ids.cpp.in\n> > @@ -89,9 +89,9 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> > \t{ \"{{enum.name}}\", {{enum.name}} },\n> > {%- endfor %}\n> > };\n> > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.name}}NameValueMap);\n> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);\n> > {% else -%}\n> > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\");\n> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}});\n> > {% endif -%}\n> > {%- endfor %}\n> > \n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 2efecf0fc52b..30eb17e7f064 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -397,14 +397,15 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> > * \\param[in] vendor The vendor name\n> > * \\param[in] type The control data type\n> > * \\param[in] size The size of the array control, or 0 if scalar control\n> > + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> \n> Line wrap ?\n> \n> > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > */\n> > ControlId::ControlId(unsigned int id, const std::string &name,\n> > \t\t const std::string &vendor, ControlType type,\n> > -\t\t std::size_t size,\n> > +\t\t std::size_t size, const DirectionFlags &direction,\n> > \t\t const std::map &enumStrMap)\n> > \t: id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> > -\t enumStrMap_(enumStrMap)\n> > +\t direction_(direction), enumStrMap_(enumStrMap)\n> > {\n> > \tfor (const auto &pair : enumStrMap_)\n> > \t\treverseMap_[pair.second] = pair.first;\n> > @@ -440,6 +441,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\return True if the control is an array control, false otherwise\n> > */\n> > \n> > +/**\n> > + * \\fn bool ControlId::isInput() const\n> > + * \\brief Determine if the control is available to be used as an input control\n> > + *\n> > + * Controls can be used either as input as a control, or as output in metadata.\n> > + * This function checks if the control is allowed to be used as the former.\n> > + *\n> > + * \\return True if the control can be used as an input control, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn bool ControlId::isOutput() const\n> > + * \\brief Determine if the control is available to be used in output metadata\n> > + *\n> > + * Controls can be used either as input as a control, or as output in metadata.\n> \n> \"Controls can be used [...] as a control\"\n\nI'm not sure I understand the change that you want here. I think this is\nmore explicit and informative. It also appeases all the bikeshedders\nthat want an explicit mention somewhere and we don't have any mention of\ncontrols in the application developer guide so this might be the second\nbest place to put it.\n\n> \n> :S I don't have a better suggestion for now that wouldn't involve a\n> tree-wide rename, so I'm OK with this.\n> \n> > + * This function checks if the control is allowed to be used as the latter.\n> > + *\n> > + * \\return True if the control can be returned in output metadata, false otherwise\n> > + */\n> > +\n> > /**\n> > * \\fn std::size_t ControlId::size() const\n> > * \\brief Retrieve the size of the control if it is an array control\n> > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\return True if \\a lhs.id() is equal to \\a rhs, false otherwise\n> > */\n> > \n> > +/**\n> > + * \\enum ControlId::Direction\n> > + * \\brief The direction that a control of the ControlId is capable of being passed from/to\n> > + *\n> > + * \\var ControlId::Direction::In\n> > + * \\brief The control can be passed in controls as input\n> > + *\n> > + * \\var ControlId::Direction::Out\n> > + * \\brief The control can be returned in output as metadata\n> \n> The two definitions are not consistent: \"in controls as input\" vs. \"in\n> output as metadata\".\n\nOops, right.\n\n\nThanks,\n\nPaul\n\n> \n> > + */\n> > +\n> > +/**\n> > + * \\typedef ControlId::DirectionFlags\n> > + * \\brief A wrapper for ControlId::Direction so that it can be used as flags\n> > + */\n> > +\n> > /**\n> > * \\class Control\n> > * \\brief Describe a control and its intrinsic properties\n> > @@ -504,6 +541,7 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\param[in] id The control numerical ID\n> > * \\param[in] name The control name\n> > * \\param[in] vendor The vendor name\n> > + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > *\n> > * The control data type is automatically deduced from the template type T.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8194BC3200\n\tfor ;\n\tWed, 27 Nov 2024 08:40:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1E186609E;\n\tWed, 27 Nov 2024 09:40:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3560A65FD2\n\tfor ;\n\tWed, 27 Nov 2024 09:40:51 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:7dcc:a4ea:e361:d355])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 495FC78C;\n\tWed, 27 Nov 2024 09:40:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FJvRVC8h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732696828;\n\tbh=jV+pZXGGFRNGvXyF2BjHk3ovIBa3N80h2kh2o2i3J8U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FJvRVC8h8Y2sEV9HmpEznKnhaIvA9Jh7K9QyhcBTPndShgYQgI+lJGvU72BHKN5Ri\n\tcglvJ/jnHgKEFGYK1XQfw94baJHhlbOb70EHYgB7ihcIlUFYuUoa5EBqKByB8+lfub\n\tJ3pxVrjXDngzcAhsxn9zEQU2QRrjE2X9BML/uLhw=","Date":"Wed, 27 Nov 2024 17:40:43 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-4-paul.elder@ideasonboard.com>\n\t<20241125231910.GA19381@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241125231910.GA19381@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32408,"web_url":"https://patchwork.libcamera.org/comment/32408/","msgid":"<20241127085350.GX5461@pendragon.ideasonboard.com>","date":"2024-11-27T08:53:50","subject":"Re: [PATCH 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Nov 27, 2024 at 05:40:43PM +0900, Paul Elder wrote:\n> On Tue, Nov 26, 2024 at 01:19:10AM +0200, Laurent Pinchart wrote:\n> > On Tue, Nov 26, 2024 at 12:30:02AM +0900, Paul Elder wrote:\n> > > Add support to ControlId for querying direction information. This allows\n> > > applications to query whether a ControlId is meant for being set in\n> > > controls or to be returned in metadata or both. This also has a side\n> > > effect of properly encoding this information, as previously it was only\n> > > mentioned losely and inconsistently in the control id definition.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > include/libcamera/controls.h | 27 +++++++++++++++++++-\n> > > src/libcamera/control_ids.cpp.in | 4 +--\n> > > src/libcamera/controls.cpp | 42 ++++++++++++++++++++++++++++++--\n> > > 3 files changed, 68 insertions(+), 5 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 3cfe2de5973c..cd338ac0d653 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \n> > > @@ -235,8 +236,18 @@ private:\n> > > class ControlId\n> > > {\n> > > public:\n> > > +\tenum class Direction {\n> > > +\t\tIn = (1 << 0),\n> > > +\t\tOut = (1 << 1),\n> > > +\t};\n> > > +\n> > > +\tusing DirectionFlags = Flags;\n> > > +\n> > > \tControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> > > \t\t ControlType type, std::size_t size = 0,\n> > > +\t\t const DirectionFlags &direction =\n> > > +\t\t\t static_cast(Direction::In) |\n> > > +\t\t\t static_cast(Direction::Out),\n> > \n> > I think we could make this argument explicit, without a default value,\n> > as all controls have a direction.\n> \n> It was causing problems in generating v4l2 controls in V4L2Device, and\n> in ControlSerializer, so I'll leave it in.\n\nBoth could easily pass {} for the parameter if they don't know the\ndirection.\n\nFor the control serializer that may actually be an issue, if it can't\nset a direction, it means we will have controls within libcamera where\nthe isInput() and isOutput() functions will be unreliable, possibly\nleading to difficult to debug issues.\n\n> > > \t\t const std::map &enumStrMap = {});\n> > > \n> > > \tunsigned int id() const { return id_; }\n> > > @@ -245,6 +256,16 @@ public:\n> > > \tControlType type() const { return type_; }\n> > > \tbool isArray() const { return size_ > 0; }\n> > > \tstd::size_t size() const { return size_; }\n> > > +\tbool isInput() const\n> > \n> > With the changes proposed in the cover letter this now holds on a single\n> > line:\n> > \n> > \tbool isInput() const { return !!(direction_ & Direction::In); }\n> > \n> > > +\t{\n> > > +\t\treturn static_cast(\n> > > +\t\t\tdirection_ & static_cast(Direction::In));\n> > > +\t}\n> > > +\tbool isOutput() const\n> > \n> > Same here.\n> > \n> > > +\t{\n> > > +\t\treturn static_cast(\n> > > +\t\t\tdirection_ & static_cast(Direction::Out));\n> > > +\t}\n> > > \tconst std::map &enumerators() const { return reverseMap_; }\n> > > \n> > > private:\n> > > @@ -255,6 +276,7 @@ private:\n> > > \tstd::string vendor_;\n> > > \tControlType type_;\n> > > \tstd::size_t size_;\n> > > +\tDirectionFlags direction_;\n> > > \tstd::map enumStrMap_;\n> > > \tstd::map reverseMap_;\n> > > };\n> > > @@ -286,9 +308,12 @@ public:\n> > > \tusing type = T;\n> > > \n> > > \tControl(unsigned int id, const char *name, const char *vendor,\n> > > +\t\tconst ControlId::DirectionFlags &direction =\n> > > +\t\t\tstatic_cast(ControlId::Direction::In) |\n> > > +\t\t\tstatic_cast(ControlId::Direction::Out),\n> > \n> > Do we want to pass this as an argument to the constructor, or as a\n> > template argument ? The latter would allow handling the direction at\n> > compile time (not sure what the use case would be though).\n> \n> I think I'll keep it as regular parameters.\n\nFine with me until we have a use case.\n\n> > > \t\tconst std::map &enumStrMap = {})\n> > > \t\t: ControlId(id, name, vendor, details::control_type>::value,\n> > > -\t\t\t details::control_type>::size, enumStrMap)\n> > > +\t\t\t details::control_type>::size, direction, enumStrMap)\n> > > \t{\n> > > \t}\n> > > \n> > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > > index afe9e2c96610..65668d486dbc 100644\n> > > --- a/src/libcamera/control_ids.cpp.in\n> > > +++ b/src/libcamera/control_ids.cpp.in\n> > > @@ -89,9 +89,9 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> > > \t{ \"{{enum.name}}\", {{enum.name}} },\n> > > {%- endfor %}\n> > > };\n> > > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.name}}NameValueMap);\n> > > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);\n> > > {% else -%}\n> > > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\");\n> > > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}});\n> > > {% endif -%}\n> > > {%- endfor %}\n> > > \n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 2efecf0fc52b..30eb17e7f064 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -397,14 +397,15 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> > > * \\param[in] vendor The vendor name\n> > > * \\param[in] type The control data type\n> > > * \\param[in] size The size of the array control, or 0 if scalar control\n> > > + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> > \n> > Line wrap ?\n> > \n> > > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > > */\n> > > ControlId::ControlId(unsigned int id, const std::string &name,\n> > > \t\t const std::string &vendor, ControlType type,\n> > > -\t\t std::size_t size,\n> > > +\t\t std::size_t size, const DirectionFlags &direction,\n> > > \t\t const std::map &enumStrMap)\n> > > \t: id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> > > -\t enumStrMap_(enumStrMap)\n> > > +\t direction_(direction), enumStrMap_(enumStrMap)\n> > > {\n> > > \tfor (const auto &pair : enumStrMap_)\n> > > \t\treverseMap_[pair.second] = pair.first;\n> > > @@ -440,6 +441,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > > * \\return True if the control is an array control, false otherwise\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn bool ControlId::isInput() const\n> > > + * \\brief Determine if the control is available to be used as an input control\n> > > + *\n> > > + * Controls can be used either as input as a control, or as output in metadata.\n> > > + * This function checks if the control is allowed to be used as the former.\n> > > + *\n> > > + * \\return True if the control can be used as an input control, false otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn bool ControlId::isOutput() const\n> > > + * \\brief Determine if the control is available to be used in output metadata\n> > > + *\n> > > + * Controls can be used either as input as a control, or as output in metadata.\n> > \n> > \"Controls can be used [...] as a control\"\n> \n> I'm not sure I understand the change that you want here. I think this is\n\nI'm not proposing any change. I find the wording weird, saying that\n\"controls can be used as a control\". That's nothing new, it's due to\nthe control vs. metadata naming issue that we should solve at some\npoint.\n\n> more explicit and informative. It also appeases all the bikeshedders\n> that want an explicit mention somewhere and we don't have any mention of\n> controls in the application developer guide so this might be the second\n> best place to put it.\n> \n> > :S I don't have a better suggestion for now that wouldn't involve a\n> > tree-wide rename, so I'm OK with this.\n> > \n> > > + * This function checks if the control is allowed to be used as the latter.\n> > > + *\n> > > + * \\return True if the control can be returned in output metadata, false otherwise\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn std::size_t ControlId::size() const\n> > > * \\brief Retrieve the size of the control if it is an array control\n> > > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > > * \\return True if \\a lhs.id() is equal to \\a rhs, false otherwise\n> > > */\n> > > \n> > > +/**\n> > > + * \\enum ControlId::Direction\n> > > + * \\brief The direction that a control of the ControlId is capable of being passed from/to\n> > > + *\n> > > + * \\var ControlId::Direction::In\n> > > + * \\brief The control can be passed in controls as input\n> > > + *\n> > > + * \\var ControlId::Direction::Out\n> > > + * \\brief The control can be returned in output as metadata\n> > \n> > The two definitions are not consistent: \"in controls as input\" vs. \"in\n> > output as metadata\".\n> \n> Oops, right.\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\typedef ControlId::DirectionFlags\n> > > + * \\brief A wrapper for ControlId::Direction so that it can be used as flags\n> > > + */\n> > > +\n> > > /**\n> > > * \\class Control\n> > > * \\brief Describe a control and its intrinsic properties\n> > > @@ -504,6 +541,7 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > > * \\param[in] id The control numerical ID\n> > > * \\param[in] name The control name\n> > > * \\param[in] vendor The vendor name\n> > > + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> > > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > > *\n> > > * The control data type is automatically deduced from the template type T.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 063FEC3213\n\tfor ;\n\tWed, 27 Nov 2024 08:54:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D33E4660AD;\n\tWed, 27 Nov 2024 09:54:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B03C6609E\n\tfor ;\n\tWed, 27 Nov 2024 09:54:01 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A4D178C;\n\tWed, 27 Nov 2024 09:53:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AEiIi9Bl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732697618;\n\tbh=FPWuSgOep03siTTVDPrtTE5X9gB5bP38nbdCP5O81Ew=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AEiIi9BlYzJ1MvVcIsopix1iPoyaDvu2EYKlzVWxjo/l4KuSY/ViRXAaeV/tI0lLC\n\tYVbjSFAlJVhVWhopz81JsX9bKQvwLbb7L++6p7N1aWoDLeqeNcx/3Hj0zGcPfcAAdc\n\tgiiENvY/KbzyfsC6OcZOEQ+NnfLXzaI2Bwloo8q8=","Date":"Wed, 27 Nov 2024 10:53:50 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"<20241127085350.GX5461@pendragon.ideasonboard.com>","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>\n\t<20241125153003.3309066-4-paul.elder@ideasonboard.com>\n\t<20241125231910.GA19381@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]