[{"id":2837,"web_url":"https://patchwork.libcamera.org/comment/2837/","msgid":"<20191009083520.fwujwwxejr2x7uzr@uno.localdomain>","date":"2019-10-09T08:35:20","subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: v4l2_controls: Add\n\tV4L2ControlId","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Oct 08, 2019 at 01:46:39AM +0300, Laurent Pinchart wrote:\n> Add a V4L2 specialisation of the ControlId class, in order to construct\n> a ControlId from a v4l2_query_ext_ctrl. This is needed in order to use\n> ControlList for V4L2 controls.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/v4l2_controls.h | 12 +++--\n> src/libcamera/v4l2_controls.cpp | 67 +++++++++++++++++++++++----\n> src/libcamera/v4l2_device.cpp | 4 +-\n> 3 files changed, 68 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> index b39370b2e90e..71ce377fe4c5 100644\n> --- a/src/libcamera/include/v4l2_controls.h\n> +++ b/src/libcamera/include/v4l2_controls.h\n> @@ -20,23 +20,27 @@\n>\n> namespace libcamera {\n>\n> +class V4L2ControlId : public ControlId\n> +{\n> +public:\n> +\tV4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);\n> +};\n> +\n\nThis only add a constructor without any field. Do we need inheritance\nor can we just add a constructor to ControlId ?\n\n> class V4L2ControlInfo\n> {\n> public:\n> \tV4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n>\n> -\tunsigned int id() const { return id_; }\n> +\tconst ControlId &id() const { return id_; }\n> \tunsigned int type() const { return type_; }\n> \tsize_t size() const { return size_; }\n> -\tconst std::string &name() const { return name_; }\n>\n> \tconst ControlRange &range() const { return range_; }\n>\n> private:\n> -\tunsigned int id_;\n> +\tV4L2ControlId id_;\n> \tunsigned int type_;\n> \tsize_t size_;\n> -\tstd::string name_;\n>\n> \tControlRange range_;\n> };\n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> index 6f5f1578b139..a630a2583d33 100644\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ b/src/libcamera/v4l2_controls.cpp\n> @@ -7,6 +7,8 @@\n>\n> #include \"v4l2_controls.h\"\n>\n> +#include \n> +\n> /**\n> * \\file v4l2_controls.h\n> * \\brief Support for V4L2 Controls using the V4L2 Extended Controls APIs\n> @@ -47,6 +49,60 @@\n>\n> namespace libcamera {\n>\n> +namespace {\n> +\n> +std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> +{\n> +\tsize_t len = strnlen(ctrl.name, sizeof(ctrl.name));\n> +\treturn std::string(static_cast(ctrl.name), len);\n> +}\n> +\n> +ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)\n> +{\n> +\tswitch (ctrl.type) {\n> +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\t\treturn ControlTypeBool;\n> +\n> +\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\t\treturn ControlTypeInteger32;\n> +\n> +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\treturn ControlTypeInteger64;\n> +\n> +\tcase V4L2_CTRL_TYPE_MENU:\n> +\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\tcase V4L2_CTRL_TYPE_BITMASK:\n> +\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> +\t\t/*\n> +\t\t * More precise types may be needed, for now use a 32-bit\n> +\t\t * integer type.\n> +\t\t */\n> +\t\treturn ControlTypeInteger32;\n> +\n> +\tdefault:\n> +\t\treturn ControlTypeNone;\n> +\t}\n> +}\n\nProbably it's ok to have inheritance as we need this in\nv4l2_controls.cpp\n\nThis considered\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\class V4L2ControlId\n> + * \\brief V4L2 control static metadata\n> + *\n> + * The V4L2ControlId class is a specialisation of the ControlId for V4L2\n> + * controls.\n> + */\n> +\n> +/**\n> + * \\brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl\n> + * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> + */\n> +V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)\n> +\t: ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))\n> +{\n> +}\n> +\n> /**\n> * \\class V4L2ControlInfo\n> * \\brief Information on a V4L2 control\n> @@ -66,13 +122,12 @@ namespace libcamera {\n>\n> /**\n> * \\brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl\n> - * \\param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> + * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> */\n> V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> +\t: id_(ctrl)\n> {\n> -\tid_ = ctrl.id;\n> \ttype_ = ctrl.type;\n> -\tname_ = static_cast(ctrl.name);\n> \tsize_ = ctrl.elem_size * ctrl.elems;\n>\n> \tif (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)\n> @@ -101,12 +156,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> * \\return The V4L2 control value data size\n> */\n>\n> -/**\n> - * \\fn V4L2ControlInfo::name()\n> - * \\brief Retrieve the control user readable name\n> - * \\return The V4L2 control user readable name\n> - */\n> -\n> /**\n> * \\fn V4L2ControlInfo::range()\n> * \\brief Retrieve the control value range\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 3bd82ff23212..466c3d41f6e3 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -184,7 +184,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)\n>\n> \t\tconst V4L2ControlInfo *info = &iter->second;\n> \t\tcontrolInfo[i] = info;\n> -\t\tv4l2Ctrls[i].id = info->id();\n> +\t\tv4l2Ctrls[i].id = ctrl->id();\n> \t}\n>\n> \tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> @@ -259,7 +259,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n>\n> \t\tconst V4L2ControlInfo *info = &iter->second;\n> \t\tcontrolInfo[i] = info;\n> -\t\tv4l2Ctrls[i].id = info->id();\n> +\t\tv4l2Ctrls[i].id = ctrl->id();\n>\n> \t\t/* Set the v4l2_ext_control value for the write operation. */\n> \t\tswitch (info->type()) {\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 relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 084696157B\n\tfor ;\n\tWed, 9 Oct 2019 10:33:35 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 922DE100015;\n\tWed, 9 Oct 2019 08:33:34 +0000 (UTC)"],"Date":"Wed, 9 Oct 2019 10:35:20 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191009083520.fwujwwxejr2x7uzr@uno.localdomain>","References":"<20191007224642.6597-1-laurent.pinchart@ideasonboard.com>\n\t<20191007224642.6597-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"iqqltksf7gzlb4bt\"","Content-Disposition":"inline","In-Reply-To":"<20191007224642.6597-7-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: v4l2_controls: Add\n\tV4L2ControlId","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","X-List-Received-Date":"Wed, 09 Oct 2019 08:33:35 -0000"}},{"id":2839,"web_url":"https://patchwork.libcamera.org/comment/2839/","msgid":"<20191009090549.GG22998@pendragon.ideasonboard.com>","date":"2019-10-09T09:05:49","subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: v4l2_controls: Add\n\tV4L2ControlId","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Oct 09, 2019 at 10:35:20AM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 08, 2019 at 01:46:39AM +0300, Laurent Pinchart wrote:\n> > Add a V4L2 specialisation of the ControlId class, in order to construct\n> > a ControlId from a v4l2_query_ext_ctrl. This is needed in order to use\n> > ControlList for V4L2 controls.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/include/v4l2_controls.h | 12 +++--\n> > src/libcamera/v4l2_controls.cpp | 67 +++++++++++++++++++++++----\n> > src/libcamera/v4l2_device.cpp | 4 +-\n> > 3 files changed, 68 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > index b39370b2e90e..71ce377fe4c5 100644\n> > --- a/src/libcamera/include/v4l2_controls.h\n> > +++ b/src/libcamera/include/v4l2_controls.h\n> > @@ -20,23 +20,27 @@\n> >\n> > namespace libcamera {\n> >\n> > +class V4L2ControlId : public ControlId\n> > +{\n> > +public:\n> > +\tV4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);\n> > +};\n> > +\n> \n> This only add a constructor without any field. Do we need inheritance\n> or can we just add a constructor to ControlId ?\n\nWe could, but I think it's best to keep the ControlId constructor\nprotected, otherwise applications could create new ControlId instances.\n\n> > class V4L2ControlInfo\n> > {\n> > public:\n> > \tV4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n> >\n> > -\tunsigned int id() const { return id_; }\n> > +\tconst ControlId &id() const { return id_; }\n> > \tunsigned int type() const { return type_; }\n> > \tsize_t size() const { return size_; }\n> > -\tconst std::string &name() const { return name_; }\n> >\n> > \tconst ControlRange &range() const { return range_; }\n> >\n> > private:\n> > -\tunsigned int id_;\n> > +\tV4L2ControlId id_;\n> > \tunsigned int type_;\n> > \tsize_t size_;\n> > -\tstd::string name_;\n> >\n> > \tControlRange range_;\n> > };\n> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > index 6f5f1578b139..a630a2583d33 100644\n> > --- a/src/libcamera/v4l2_controls.cpp\n> > +++ b/src/libcamera/v4l2_controls.cpp\n> > @@ -7,6 +7,8 @@\n> >\n> > #include \"v4l2_controls.h\"\n> >\n> > +#include \n> > +\n> > /**\n> > * \\file v4l2_controls.h\n> > * \\brief Support for V4L2 Controls using the V4L2 Extended Controls APIs\n> > @@ -47,6 +49,60 @@\n> >\n> > namespace libcamera {\n> >\n> > +namespace {\n> > +\n> > +std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> > +{\n> > +\tsize_t len = strnlen(ctrl.name, sizeof(ctrl.name));\n> > +\treturn std::string(static_cast(ctrl.name), len);\n> > +}\n> > +\n> > +ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)\n> > +{\n> > +\tswitch (ctrl.type) {\n> > +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > +\t\treturn ControlTypeBool;\n> > +\n> > +\tcase V4L2_CTRL_TYPE_INTEGER:\n> > +\t\treturn ControlTypeInteger32;\n> > +\n> > +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\treturn ControlTypeInteger64;\n> > +\n> > +\tcase V4L2_CTRL_TYPE_MENU:\n> > +\tcase V4L2_CTRL_TYPE_BUTTON:\n> > +\tcase V4L2_CTRL_TYPE_BITMASK:\n> > +\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> > +\t\t/*\n> > +\t\t * More precise types may be needed, for now use a 32-bit\n> > +\t\t * integer type.\n> > +\t\t */\n> > +\t\treturn ControlTypeInteger32;\n> > +\n> > +\tdefault:\n> > +\t\treturn ControlTypeNone;\n> > +\t}\n> > +}\n> \n> Probably it's ok to have inheritance as we need this in\n> v4l2_controls.cpp\n> \n> This considered\n> Reviewed-by: Jacopo Mondi \n> \n> > +\n> > +} /* namespace */\n> > +\n> > +/**\n> > + * \\class V4L2ControlId\n> > + * \\brief V4L2 control static metadata\n> > + *\n> > + * The V4L2ControlId class is a specialisation of the ControlId for V4L2\n> > + * controls.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl\n> > + * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> > + */\n> > +V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)\n> > +\t: ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))\n> > +{\n> > +}\n> > +\n> > /**\n> > * \\class V4L2ControlInfo\n> > * \\brief Information on a V4L2 control\n> > @@ -66,13 +122,12 @@ namespace libcamera {\n> >\n> > /**\n> > * \\brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl\n> > - * \\param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> > + * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> > */\n> > V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > +\t: id_(ctrl)\n> > {\n> > -\tid_ = ctrl.id;\n> > \ttype_ = ctrl.type;\n> > -\tname_ = static_cast(ctrl.name);\n> > \tsize_ = ctrl.elem_size * ctrl.elems;\n> >\n> > \tif (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)\n> > @@ -101,12 +156,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > * \\return The V4L2 control value data size\n> > */\n> >\n> > -/**\n> > - * \\fn V4L2ControlInfo::name()\n> > - * \\brief Retrieve the control user readable name\n> > - * \\return The V4L2 control user readable name\n> > - */\n> > -\n> > /**\n> > * \\fn V4L2ControlInfo::range()\n> > * \\brief Retrieve the control value range\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 3bd82ff23212..466c3d41f6e3 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -184,7 +184,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)\n> >\n> > \t\tconst V4L2ControlInfo *info = &iter->second;\n> > \t\tcontrolInfo[i] = info;\n> > -\t\tv4l2Ctrls[i].id = info->id();\n> > +\t\tv4l2Ctrls[i].id = ctrl->id();\n> > \t}\n> >\n> > \tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > @@ -259,7 +259,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n> >\n> > \t\tconst V4L2ControlInfo *info = &iter->second;\n> > \t\tcontrolInfo[i] = info;\n> > -\t\tv4l2Ctrls[i].id = info->id();\n> > +\t\tv4l2Ctrls[i].id = ctrl->id();\n> >\n> > \t\t/* Set the v4l2_ext_control value for the write operation. */\n> > \t\tswitch (info->type()) {","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 32A076157B\n\tfor ;\n\tWed, 9 Oct 2019 11:05:51 +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 86FD84FF;\n\tWed, 9 Oct 2019 11:05:50 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570611950;\n\tbh=qxrNIln+bImTnj0jocdYYjN4zNfU3ABank/p9zqxXrg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z6by/3qpydxZ66u1eyLBexUJ7DGfMVMwe0qpvQkfRk/uZ0OmdNFlOP/kKvGXdEAIh\n\t0pquKtl/NeBqFxL+5+p8VADAcTNWMM2uZMx7km2ypzI13eQskSRK9ZMP6MdLsxe6fN\n\tq4HGs/AvKQ/9XMFHYt5YfoPChyQXXiJdl1Pf5vcA=","Date":"Wed, 9 Oct 2019 12:05:49 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191009090549.GG22998@pendragon.ideasonboard.com>","References":"<20191007224642.6597-1-laurent.pinchart@ideasonboard.com>\n\t<20191007224642.6597-7-laurent.pinchart@ideasonboard.com>\n\t<20191009083520.fwujwwxejr2x7uzr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191009083520.fwujwwxejr2x7uzr@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 6/9] libcamera: v4l2_controls: Add\n\tV4L2ControlId","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","X-List-Received-Date":"Wed, 09 Oct 2019 09:05:51 -0000"}}]