[{"id":2726,"web_url":"https://patchwork.libcamera.org/comment/2726/","msgid":"<20190929101025.b3pcuipnpqd3cvzy@uno.localdomain>","date":"2019-09-29T10:10:25","subject":"Re: [libcamera-devel] [PATCH 08/12] libcamera: v4l2_controls: Use\n\tthe ControlValue class for data storage","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Sep 28, 2019 at 06:22:34PM +0300, Laurent Pinchart wrote:\n> Use the ControlValue class to replace the manually crafted data storage\n> in V4L2Control. This will help sharing code when more data types will be\n> supported.\n>\n\nHappy indeed to see that at third attempt we might be getting there\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/v4l2_controls.h | 15 +++++++++------\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/v4l2_controls.cpp | 19 ++++++++++---------\n> src/libcamera/v4l2_device.cpp | 8 ++++----\n> 5 files changed, 25 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> index 10b726504951..f2b67c5d44e1 100644\n> --- a/src/libcamera/include/v4l2_controls.h\n> +++ b/src/libcamera/include/v4l2_controls.h\n> @@ -16,6 +16,8 @@\n> #include \n> #include \n>\n> +#include \n> +\n> namespace libcamera {\n>\n> class V4L2ControlInfo\n> @@ -46,17 +48,18 @@ using V4L2ControlInfoMap = std::map;\n> class V4L2Control\n> {\n> public:\n> -\tV4L2Control(unsigned int id, int value = 0)\n> -\t\t: id_(id), value_(value) {}\n> -\n> -\tint64_t value() const { return value_; }\n> -\tvoid setValue(int64_t value) { value_ = value; }\n> +\tV4L2Control(unsigned int id, const ControlValue &value = ControlValue())\n> +\t\t: id_(id), value_(value)\n\nThe default copy constructor is fine so far, as it copies the union\ncontent. With compound controls this will fall short I fear.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\t{\n> +\t}\n>\n> \tunsigned int id() const { return id_; }\n> +\tconst ControlValue &value() const { return value_; }\n> +\tControlValue &value() { return value_; }\n>\n> private:\n> \tunsigned int id_;\n> -\tint64_t value_;\n> +\tControlValue value_;\n> };\n>\n> class V4L2ControlList\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 3635ea83a7b1..a80f8a43a116 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -252,7 +252,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> \t\tLOG(UVC, Debug)\n> \t\t\t<< \"Setting control 0x\"\n> \t\t\t<< std::hex << std::setw(8) << ctrl.id() << std::dec\n> -\t\t\t<< \" to \" << ctrl.value();\n> +\t\t\t<< \" to \" << ctrl.value().toString();\n>\n> \tint ret = data->video_->setControls(&controls);\n> \tif (ret) {\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index d9dd031e173c..5d6909f58cf9 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -300,7 +300,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> \t\tLOG(VIMC, Debug)\n> \t\t\t<< \"Setting control 0x\"\n> \t\t\t<< std::hex << std::setw(8) << ctrl.id() << std::dec\n> -\t\t\t<< \" to \" << ctrl.value();\n> +\t\t\t<< \" to \" << ctrl.value().toString();\n>\n> \tint ret = data->sensor_->setControls(&controls);\n> \tif (ret) {\n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> index 84258d9954d0..64f0555fff7c 100644\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ b/src/libcamera/v4l2_controls.cpp\n> @@ -143,6 +143,16 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> * \\param value The control value\n> */\n>\n> +/**\n> + * \\fn V4L2Control::value() const\n> + * \\brief Retrieve the value of the control\n> + *\n> + * This method is a const version of V4L2Control::value(), returning a const\n> + * reference to the value.\n> + *\n> + * \\return The V4L2 control value\n> + */\n> +\n> /**\n> * \\fn V4L2Control::value()\n> * \\brief Retrieve the value of the control\n> @@ -154,15 +164,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> * \\return The V4L2 control value\n> */\n>\n> -/**\n> - * \\fn V4L2Control::setValue()\n> - * \\brief Set the value of the control\n> - * \\param value The new V4L2 control value\n> - *\n> - * This method stores the control value, which will be applied to the\n> - * device when calling V4L2Device::setControls().\n> - */\n> -\n> /**\n> * \\fn V4L2Control::id()\n> * \\brief Retrieve the control ID this instance refers to\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 349bf2d29704..fd4b9c6d5c62 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -264,14 +264,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n> \t\t/* Set the v4l2_ext_control value for the write operation. */\n> \t\tswitch (info->type()) {\n> \t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> -\t\t\tv4l2Ctrls[i].value64 = ctrl->value();\n> +\t\t\tv4l2Ctrls[i].value64 = ctrl->value().get();\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\t/*\n> \t\t\t * \\todo To be changed when support for string and\n> \t\t\t * compound controls will be added.\n> \t\t\t */\n> -\t\t\tv4l2Ctrls[i].value = ctrl->value();\n> +\t\t\tv4l2Ctrls[i].value = ctrl->value().get();\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> @@ -393,14 +393,14 @@ void V4L2Device::updateControls(V4L2ControlList *ctrls,\n>\n> \t\tswitch (info->type()) {\n> \t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> -\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> +\t\t\tctrl->value().set(v4l2Ctrl->value64);\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\t/*\n> \t\t\t * \\todo To be changed when support for string and\n> \t\t\t * compound controls will be added.\n> \t\t\t */\n> -\t\t\tctrl->setValue(v4l2Ctrl->value);\n> +\t\t\tctrl->value().set(v4l2Ctrl->value);\n> \t\t\tbreak;\n> \t\t}\n> \t}\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 relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9221761654\n\tfor ;\n\tSun, 29 Sep 2019 12:08:43 +0200 (CEST)","from uno.localdomain\n\t(host7-199-dynamic.171-212-r.retail.telecomitalia.it [212.171.199.7])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id AB0F0FF804;\n\tSun, 29 Sep 2019 10:08:42 +0000 (UTC)"],"X-Originating-IP":"212.171.199.7","Date":"Sun, 29 Sep 2019 12:10:25 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190929101025.b3pcuipnpqd3cvzy@uno.localdomain>","References":"<20190928152238.23752-1-laurent.pinchart@ideasonboard.com>\n\t<20190928152238.23752-9-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"5w4y3rx4o6bf4pns\"","Content-Disposition":"inline","In-Reply-To":"<20190928152238.23752-9-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 08/12] libcamera: v4l2_controls: Use\n\tthe ControlValue class for data storage","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":"Sun, 29 Sep 2019 10:08:43 -0000"}},{"id":2734,"web_url":"https://patchwork.libcamera.org/comment/2734/","msgid":"<20190929133049.GF4827@pendragon.ideasonboard.com>","date":"2019-09-29T13:30:49","subject":"Re: [libcamera-devel] [PATCH 08/12] libcamera: v4l2_controls: Use\n\tthe ControlValue class for data storage","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Sep 29, 2019 at 12:10:25PM +0200, Jacopo Mondi wrote:\n> On Sat, Sep 28, 2019 at 06:22:34PM +0300, Laurent Pinchart wrote:\n> > Use the ControlValue class to replace the manually crafted data storage\n> > in V4L2Control. This will help sharing code when more data types will be\n> > supported.\n> \n> Happy indeed to see that at third attempt we might be getting there\n\nIt's nice to share code, but I'd like to make it clear that my first\nfocus is a good API towards applications, my second focus a good API\ntowards pipeline handlers and IPAs, and internal code sharing is only my\nthird focus. As long as libcamera and V4L2 controls are similar enough\nthat code can be shared, I have no issue with that. If we figure out\nlater that differences are needed, I will choose separate\nimplementations over any regression in the public API for the purpose of\ncode sharing.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/include/v4l2_controls.h | 15 +++++++++------\n> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > src/libcamera/v4l2_controls.cpp | 19 ++++++++++---------\n> > src/libcamera/v4l2_device.cpp | 8 ++++----\n> > 5 files changed, 25 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > index 10b726504951..f2b67c5d44e1 100644\n> > --- a/src/libcamera/include/v4l2_controls.h\n> > +++ b/src/libcamera/include/v4l2_controls.h\n> > @@ -16,6 +16,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > namespace libcamera {\n> >\n> > class V4L2ControlInfo\n> > @@ -46,17 +48,18 @@ using V4L2ControlInfoMap = std::map;\n> > class V4L2Control\n> > {\n> > public:\n> > -\tV4L2Control(unsigned int id, int value = 0)\n> > -\t\t: id_(id), value_(value) {}\n> > -\n> > -\tint64_t value() const { return value_; }\n> > -\tvoid setValue(int64_t value) { value_ = value; }\n> > +\tV4L2Control(unsigned int id, const ControlValue &value = ControlValue())\n> > +\t\t: id_(id), value_(value)\n> \n> The default copy constructor is fine so far, as it copies the union\n> content. With compound controls this will fall short I fear.\n\nYes, we will then need an explicit copy constructor.\n\n> Reviewed-by: Jacopo Mondi \n> \n> > +\t{\n> > +\t}\n> >\n> > \tunsigned int id() const { return id_; }\n> > +\tconst ControlValue &value() const { return value_; }\n> > +\tControlValue &value() { return value_; }\n> >\n> > private:\n> > \tunsigned int id_;\n> > -\tint64_t value_;\n> > +\tControlValue value_;\n> > };\n> >\n> > class V4L2ControlList\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 3635ea83a7b1..a80f8a43a116 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -252,7 +252,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > \t\tLOG(UVC, Debug)\n> > \t\t\t<< \"Setting control 0x\"\n> > \t\t\t<< std::hex << std::setw(8) << ctrl.id() << std::dec\n> > -\t\t\t<< \" to \" << ctrl.value();\n> > +\t\t\t<< \" to \" << ctrl.value().toString();\n> >\n> > \tint ret = data->video_->setControls(&controls);\n> > \tif (ret) {\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index d9dd031e173c..5d6909f58cf9 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -300,7 +300,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> > \t\tLOG(VIMC, Debug)\n> > \t\t\t<< \"Setting control 0x\"\n> > \t\t\t<< std::hex << std::setw(8) << ctrl.id() << std::dec\n> > -\t\t\t<< \" to \" << ctrl.value();\n> > +\t\t\t<< \" to \" << ctrl.value().toString();\n> >\n> > \tint ret = data->sensor_->setControls(&controls);\n> > \tif (ret) {\n> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > index 84258d9954d0..64f0555fff7c 100644\n> > --- a/src/libcamera/v4l2_controls.cpp\n> > +++ b/src/libcamera/v4l2_controls.cpp\n> > @@ -143,6 +143,16 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > * \\param value The control value\n> > */\n> >\n> > +/**\n> > + * \\fn V4L2Control::value() const\n> > + * \\brief Retrieve the value of the control\n> > + *\n> > + * This method is a const version of V4L2Control::value(), returning a const\n> > + * reference to the value.\n> > + *\n> > + * \\return The V4L2 control value\n> > + */\n> > +\n> > /**\n> > * \\fn V4L2Control::value()\n> > * \\brief Retrieve the value of the control\n> > @@ -154,15 +164,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > * \\return The V4L2 control value\n> > */\n> >\n> > -/**\n> > - * \\fn V4L2Control::setValue()\n> > - * \\brief Set the value of the control\n> > - * \\param value The new V4L2 control value\n> > - *\n> > - * This method stores the control value, which will be applied to the\n> > - * device when calling V4L2Device::setControls().\n> > - */\n> > -\n> > /**\n> > * \\fn V4L2Control::id()\n> > * \\brief Retrieve the control ID this instance refers to\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 349bf2d29704..fd4b9c6d5c62 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -264,14 +264,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n> > \t\t/* Set the v4l2_ext_control value for the write operation. */\n> > \t\tswitch (info->type()) {\n> > \t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > -\t\t\tv4l2Ctrls[i].value64 = ctrl->value();\n> > +\t\t\tv4l2Ctrls[i].value64 = ctrl->value().get();\n> > \t\t\tbreak;\n> > \t\tdefault:\n> > \t\t\t/*\n> > \t\t\t * \\todo To be changed when support for string and\n> > \t\t\t * compound controls will be added.\n> > \t\t\t */\n> > -\t\t\tv4l2Ctrls[i].value = ctrl->value();\n> > +\t\t\tv4l2Ctrls[i].value = ctrl->value().get();\n> > \t\t\tbreak;\n> > \t\t}\n> > \t}\n> > @@ -393,14 +393,14 @@ void V4L2Device::updateControls(V4L2ControlList *ctrls,\n> >\n> > \t\tswitch (info->type()) {\n> > \t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > -\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> > +\t\t\tctrl->value().set(v4l2Ctrl->value64);\n> > \t\t\tbreak;\n> > \t\tdefault:\n> > \t\t\t/*\n> > \t\t\t * \\todo To be changed when support for string and\n> > \t\t\t * compound controls will be added.\n> > \t\t\t */\n> > -\t\t\tctrl->setValue(v4l2Ctrl->value);\n> > +\t\t\tctrl->value().set(v4l2Ctrl->value);\n> > \t\t\tbreak;\n> > \t\t}\n> > \t}","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0B8561654\n\tfor ;\n\tSun, 29 Sep 2019 15:31:08 +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 5171D320;\n\tSun, 29 Sep 2019 15:31:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1569763868;\n\tbh=rAYX6Oc2jVpp+K7R7/DfobruzP7ppiHdou5VZFIM7fs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XgYMc8hKMIJaj5T178yvIh4V6e5YLexT4uvpS58meUygnFMFGNZyJU9nNnCvOVVzT\n\tgDhsRwxv0Jkj5JU7XVfi7HCL0tr/TWWvLUQ62E9jXU/zBRvVTAs2QC/MywWbTC7OxD\n\tjSRmEOAfVIOH+r7cBmlKwWpcbQCxEd9fnHybDa38=","Date":"Sun, 29 Sep 2019 16:30:49 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190929133049.GF4827@pendragon.ideasonboard.com>","References":"<20190928152238.23752-1-laurent.pinchart@ideasonboard.com>\n\t<20190928152238.23752-9-laurent.pinchart@ideasonboard.com>\n\t<20190929101025.b3pcuipnpqd3cvzy@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190929101025.b3pcuipnpqd3cvzy@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 08/12] libcamera: v4l2_controls: Use\n\tthe ControlValue class for data storage","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":"Sun, 29 Sep 2019 13:31:09 -0000"}}]