[{"id":32415,"web_url":"https://patchwork.libcamera.org/comment/32415/","msgid":"<20241127135512.GJ31095@pendragon.ideasonboard.com>","date":"2024-11-27T13:55:12","subject":"Re: [PATCH v2 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 Wed, Nov 27, 2024 at 05:50:16PM +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> ---\n> Changes in v2:\n> - simplify code\n> ---\n> include/libcamera/controls.h | 20 ++++++++++++++-\n> src/libcamera/control_ids.cpp.in | 4 +--\n> src/libcamera/controls.cpp | 43 ++++++++++++++++++++++++++++++--\n> 3 files changed, 62 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 3cfe2de5973c..74a566a05db4 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 DirectionFlags direction =\n> +\t\t\t static_cast(Direction::In) |\n> +\t\t\t static_cast(Direction::Out),\n\nI think I would still prefer not having a default value here, as all\ncontrols should have a direction. This will require setting the\ndirection explicitly when creating ControlId instances for V4L2 controls\nor in the control serializer, but I think that's better, as it will\nforce us to look at how we handle those instead of forgetting about them\nbecause they don't appear in this patch.\n\n> \t\t const std::map &enumStrMap = {});\n> \n> \tunsigned int id() const { return id_; }\n> @@ -245,6 +256,8 @@ 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 { return !!(direction_ & Direction::In); }\n> +\tbool isOutput() const { return !!(direction_ & Direction::Out); }\n> \tconst std::map &enumerators() const { return reverseMap_; }\n> \n> private:\n> @@ -255,10 +268,13 @@ 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> \n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n> +\n> static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> {\n> \treturn lhs == rhs.id();\n> @@ -286,9 +302,11 @@ public:\n> \tusing type = T;\n> \n> \tControl(unsigned int id, const char *name, const char *vendor,\n> +\t\tControlId::DirectionFlags direction = ControlId::Direction::In\n> +\t\t\t\t\t\t | ControlId::Direction::Out,\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..f221a7e621ab 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> * \\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, 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\nMaybe \"as input in controls\" to match the \"in metadata\" ?\n\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> + * 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\nThis sounds a bit weird.\n\n> + *\n> + * \\var ControlId::Direction::In\n> + * \\brief The control can be passed as input in controls\n> + *\n> + * \\var ControlId::Direction::Out\n> + * \\brief The control can be returned as output in metadata\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,8 @@ 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\n> + * 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 A2351C3200\n\tfor ;\n\tWed, 27 Nov 2024 13:55:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E21A3660CD;\n\tWed, 27 Nov 2024 14:55:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AFE1A660C4\n\tfor ;\n\tWed, 27 Nov 2024 14:55:22 +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 5EAAA78C;\n\tWed, 27 Nov 2024 14:54:59 +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=\"oZX1T2V0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732715699;\n\tbh=ZKyxTgZN/15IscIeqOv7weNAvkUTj/cvlLzKiXhrRRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oZX1T2V0m0j9fE7WaC7Yog3dNs0fJZhSHfBYuyHMt+RGF+0dDGGLdSGNcbkY/rKu4\n\tUQC0/0k3QhhSbUXu1RJ5r++DdyAAeZNmQtvfK+WRsJmtnftP602HAyjbHdI98eQ7xy\n\t5ikqMKbX9n7f5/93ZCmXWZ6sAViigZuZGkToE6m0=","Date":"Wed, 27 Nov 2024 15:55:12 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"<20241127135512.GJ31095@pendragon.ideasonboard.com>","References":"<20241127085017.2192069-1-paul.elder@ideasonboard.com>\n\t<20241127085017.2192069-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241127085017.2192069-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":32514,"web_url":"https://patchwork.libcamera.org/comment/32514/","msgid":"<173331279996.3135963.14552005197695159480@ping.linuxembedded.co.uk>","date":"2024-12-04T11:46:39","subject":"Re: [PATCH v2 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-11-27 13:55:12)\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Wed, Nov 27, 2024 at 05:50:16PM +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> > ---\n> > Changes in v2:\n> > - simplify code\n> > ---\n> > include/libcamera/controls.h | 20 ++++++++++++++-\n> > src/libcamera/control_ids.cpp.in | 4 +--\n> > src/libcamera/controls.cpp | 43 ++++++++++++++++++++++++++++++--\n> > 3 files changed, 62 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 3cfe2de5973c..74a566a05db4 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> > + enum class Direction {\n> > + In = (1 << 0),\n> > + Out = (1 << 1),\n> > + };\n> > +\n> > + using DirectionFlags = Flags;\n> > +\n> > ControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> > ControlType type, std::size_t size = 0,\n> > + DirectionFlags direction =\n> > + static_cast(Direction::In) |\n> > + static_cast(Direction::Out),\n> \n> I think I would still prefer not having a default value here, as all\n> controls should have a direction. This will require setting the\n> direction explicitly when creating ControlId instances for V4L2 controls\n> or in the control serializer, but I think that's better, as it will\n> force us to look at how we handle those instead of forgetting about them\n> because they don't appear in this patch.\n\nDoes that mean we'll be able to use this to convey 'ReadOnly' controls\nfrom V4L2 ?\n\nI assume we can construct controls from V4L2 accordingly with this now?\nand superseed\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=4064 and\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=3654 with\nthis implementation?\n\n--\nKieran\n\n> \n> > const std::map &enumStrMap = {});\n> > \n> > unsigned int id() const { return id_; }\n> > @@ -245,6 +256,8 @@ public:\n> > ControlType type() const { return type_; }\n> > bool isArray() const { return size_ > 0; }\n> > std::size_t size() const { return size_; }\n> > + bool isInput() const { return !!(direction_ & Direction::In); }\n> > + bool isOutput() const { return !!(direction_ & Direction::Out); }\n> > const std::map &enumerators() const { return reverseMap_; }\n> > \n> > private:\n> > @@ -255,10 +268,13 @@ private:\n> > std::string vendor_;\n> > ControlType type_;\n> > std::size_t size_;\n> > + DirectionFlags direction_;\n> > std::map enumStrMap_;\n> > std::map reverseMap_;\n> > };\n> > \n> > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n> > +\n> > static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> > {\n> > return lhs == rhs.id();\n> > @@ -286,9 +302,11 @@ public:\n> > using type = T;\n> > \n> > Control(unsigned int id, const char *name, const char *vendor,\n> > + ControlId::DirectionFlags direction = ControlId::Direction::In\n> > + | ControlId::Direction::Out,\n> > const std::map &enumStrMap = {})\n> > : ControlId(id, name, vendor, details::control_type>::value,\n> > - details::control_type>::size, enumStrMap)\n> > + details::control_type>::size, direction, enumStrMap)\n> > {\n> > }\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> > { \"{{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..f221a7e621ab 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> > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > */\n> > ControlId::ControlId(unsigned int id, const std::string &name,\n> > const std::string &vendor, ControlType type,\n> > - std::size_t size,\n> > + std::size_t size, DirectionFlags direction,\n> > const std::map &enumStrMap)\n> > : id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> > - enumStrMap_(enumStrMap)\n> > + direction_(direction), enumStrMap_(enumStrMap)\n> > {\n> > for (const auto &pair : enumStrMap_)\n> > reverseMap_[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> \n> Maybe \"as input in controls\" to match the \"in metadata\" ?\n> \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> > + * 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> This sounds a bit weird.\n> \n> > + *\n> > + * \\var ControlId::Direction::In\n> > + * \\brief The control can be passed as input in controls\n> > + *\n> > + * \\var ControlId::Direction::Out\n> > + * \\brief The control can be returned as output in metadata\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,8 @@ 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\n> > + * 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.\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 1F685BDB1C\n\tfor ;\n\tWed, 4 Dec 2024 11:46:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7A82660BB;\n\tWed, 4 Dec 2024 12:46:44 +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 660F3660A9\n\tfor ;\n\tWed, 4 Dec 2024 12:46:43 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38E2D4D4;\n\tWed, 4 Dec 2024 12:46:15 +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=\"GHAY4fFq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733312775;\n\tbh=Zgpf0diOeghDD1DqNyCSTo+g5X8cGr8KhScWIjonBTo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GHAY4fFqo/inVkmYZkblqxYgtWV93rJnPnHfuc7XoJGXH3chz7y15nPmjlZBS80l/\n\trtP012t2640lhg46ETtvSumirNsaL+AjRA62ZVSAdJYyWTpZNwAO3kwM9d7JO126n9\n\tRMTSDX4j5Z60QYkehjPkt209L3eKd/p+Udgv4ltM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241127135512.GJ31095@pendragon.ideasonboard.com>","References":"<20241127085017.2192069-1-paul.elder@ideasonboard.com>\n\t<20241127085017.2192069-4-paul.elder@ideasonboard.com>\n\t<20241127135512.GJ31095@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2 3/4] libcamera: controls: Add support for querying\n\tdirection information","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tPaul Elder ","Date":"Wed, 04 Dec 2024 11:46:39 +0000","Message-ID":"<173331279996.3135963.14552005197695159480@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":32523,"web_url":"https://patchwork.libcamera.org/comment/32523/","msgid":"<20241204163805.GD10736@pendragon.ideasonboard.com>","date":"2024-12-04T16:38:05","subject":"Re: [PATCH v2 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, Dec 04, 2024 at 11:46:39AM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-11-27 13:55:12)\n> > Hi Paul,\n> > \n> > Thank you for the patch.\n> > \n> > On Wed, Nov 27, 2024 at 05:50:16PM +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> > > ---\n> > > Changes in v2:\n> > > - simplify code\n> > > ---\n> > > include/libcamera/controls.h | 20 ++++++++++++++-\n> > > src/libcamera/control_ids.cpp.in | 4 +--\n> > > src/libcamera/controls.cpp | 43 ++++++++++++++++++++++++++++++--\n> > > 3 files changed, 62 insertions(+), 5 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 3cfe2de5973c..74a566a05db4 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> > > + enum class Direction {\n> > > + In = (1 << 0),\n> > > + Out = (1 << 1),\n> > > + };\n> > > +\n> > > + using DirectionFlags = Flags;\n> > > +\n> > > ControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> > > ControlType type, std::size_t size = 0,\n> > > + DirectionFlags direction =\n> > > + static_cast(Direction::In) |\n> > > + static_cast(Direction::Out),\n> > \n> > I think I would still prefer not having a default value here, as all\n> > controls should have a direction. This will require setting the\n> > direction explicitly when creating ControlId instances for V4L2 controls\n> > or in the control serializer, but I think that's better, as it will\n> > force us to look at how we handle those instead of forgetting about them\n> > because they don't appear in this patch.\n> \n> Does that mean we'll be able to use this to convey 'ReadOnly' controls\n> from V4L2 ?\n> \n> I assume we can construct controls from V4L2 accordingly with this now?\n> and superseed\n> https://patchwork.libcamera.org/project/libcamera/list/?series=4064 and\n> https://patchwork.libcamera.org/project/libcamera/list/?series=3654 with\n> this implementation?\n\nWe may need a few adjustements, but I think it's worth checking. I like\nwhen new APIs have a positive unexpected impact on other parts of\nlibcamera, it feels clean and right :-)\n\n> > > const std::map &enumStrMap = {});\n> > > \n> > > unsigned int id() const { return id_; }\n> > > @@ -245,6 +256,8 @@ public:\n> > > ControlType type() const { return type_; }\n> > > bool isArray() const { return size_ > 0; }\n> > > std::size_t size() const { return size_; }\n> > > + bool isInput() const { return !!(direction_ & Direction::In); }\n> > > + bool isOutput() const { return !!(direction_ & Direction::Out); }\n> > > const std::map &enumerators() const { return reverseMap_; }\n> > > \n> > > private:\n> > > @@ -255,10 +268,13 @@ private:\n> > > std::string vendor_;\n> > > ControlType type_;\n> > > std::size_t size_;\n> > > + DirectionFlags direction_;\n> > > std::map enumStrMap_;\n> > > std::map reverseMap_;\n> > > };\n> > > \n> > > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n> > > +\n> > > static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> > > {\n> > > return lhs == rhs.id();\n> > > @@ -286,9 +302,11 @@ public:\n> > > using type = T;\n> > > \n> > > Control(unsigned int id, const char *name, const char *vendor,\n> > > + ControlId::DirectionFlags direction = ControlId::Direction::In\n> > > + | ControlId::Direction::Out,\n> > > const std::map &enumStrMap = {})\n> > > : ControlId(id, name, vendor, details::control_type>::value,\n> > > - details::control_type>::size, enumStrMap)\n> > > + details::control_type>::size, direction, enumStrMap)\n> > > {\n> > > }\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> > > { \"{{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..f221a7e621ab 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> > > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > > */\n> > > ControlId::ControlId(unsigned int id, const std::string &name,\n> > > const std::string &vendor, ControlType type,\n> > > - std::size_t size,\n> > > + std::size_t size, DirectionFlags direction,\n> > > const std::map &enumStrMap)\n> > > : id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> > > - enumStrMap_(enumStrMap)\n> > > + direction_(direction), enumStrMap_(enumStrMap)\n> > > {\n> > > for (const auto &pair : enumStrMap_)\n> > > reverseMap_[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> > \n> > Maybe \"as input in controls\" to match the \"in metadata\" ?\n> > \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> > > + * 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> > This sounds a bit weird.\n> > \n> > > + *\n> > > + * \\var ControlId::Direction::In\n> > > + * \\brief The control can be passed as input in controls\n> > > + *\n> > > + * \\var ControlId::Direction::Out\n> > > + * \\brief The control can be returned as output in metadata\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,8 @@ 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\n> > > + * 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 96008BDB1C\n\tfor ;\n\tWed, 4 Dec 2024 16:38:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28FE4660C8;\n\tWed, 4 Dec 2024 17:38:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6EAF618B5\n\tfor ;\n\tWed, 4 Dec 2024 17:38:18 +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 83D4A6D6;\n\tWed, 4 Dec 2024 17:37:48 +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=\"H2GiKND1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733330268;\n\tbh=U5RjmI53LXlnqu56Z8BVhr6A3DPm/Mzgfk1mhBLC/w4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H2GiKND1R8q3Xg0CTXPz5qwUzzXrOPDSFdNjAOExq0AcBbhcK8LyvCMyFsjC6+yxl\n\tfMY6X4JxrwnlDdQfCStQRB0KJ2WI/MpF48vP2IpWCwTm2YajkNhOTEKYcROB+Yq+o5\n\toiDhGuIHp2ZyklFtn7MqXXRcuMXU0EisiDM0C3t4=","Date":"Wed, 4 Dec 2024 18:38:05 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"<20241204163805.GD10736@pendragon.ideasonboard.com>","References":"<20241127085017.2192069-1-paul.elder@ideasonboard.com>\n\t<20241127085017.2192069-4-paul.elder@ideasonboard.com>\n\t<20241127135512.GJ31095@pendragon.ideasonboard.com>\n\t<173331279996.3135963.14552005197695159480@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173331279996.3135963.14552005197695159480@ping.linuxembedded.co.uk>","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\" "}}]