[{"id":2834,"web_url":"https://patchwork.libcamera.org/comment/2834/","msgid":"<20191009081822.iwatv43jl56rjkuw@uno.localdomain>","date":"2019-10-09T08:18:22","subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: controls: Store\n\tcontrol name in ControlId","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:36AM +0300, Laurent Pinchart wrote:\n> The ControlId class stores a pointer to the control name. This works\n> fine for statically-defined controls, but requires code that allocates\n> controls dynamically (for instance based on control discovery on a V4L2\n> device) to keep a list of control names in separate storage. To ease\n> usage of dynamically allocated controls, store a copy of the control\n> name string in the ControlId class.\n>\n\nI would go as far as removing this from the ControlId, as it will need\nto be serialized and it is used for debug purposes only.\n\nIn case we want to retrieve it for debug, we can use the controls\nglobal map you have reintroduced in patch 1\n\nIf you like best to keep it:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/controls.h | 6 +++---\n> src/libcamera/controls.cpp | 2 +-\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 49ff1a6f747f..a5a6a135ec16 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -54,11 +54,11 @@ class ControlId\n> {\n> public:\n> \tunsigned int id() const { return id_; }\n> -\tconst char *name() const { return name_; }\n> +\tconst std::string &name() const { return name_; }\n> \tControlType type() const { return type_; }\n>\n> protected:\n> -\tControlId(unsigned int id, const char *name, ControlType type)\n> +\tControlId(unsigned int id, const std::string &name, ControlType type)\n> \t\t: id_(id), name_(name), type_(type)\n> \t{\n> \t}\n> @@ -68,7 +68,7 @@ private:\n> \tControlId &operator=(const ControlId &) = delete;\n>\n> \tunsigned int id_;\n> -\tconst char *name_;\n> +\tstd::string name_;\n> \tControlType type_;\n> };\n>\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index f862a09adef9..e528dd80a2a7 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -205,7 +205,7 @@ std::string ControlValue::toString() const\n> */\n>\n> /**\n> - * \\fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)\n> + * \\fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)\n> * \\brief Construct a ControlId instance\n> * \\param[in] id The control numerical ID\n> * \\param[in] name The control name\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 C683A6157B\n\tfor ;\n\tWed, 9 Oct 2019 10:16:42 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 61130FF813;\n\tWed, 9 Oct 2019 08:16:42 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 9 Oct 2019 10:18:22 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191009081822.iwatv43jl56rjkuw@uno.localdomain>","References":"<20191007224642.6597-1-laurent.pinchart@ideasonboard.com>\n\t<20191007224642.6597-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"wlureviawl6yzmsr\"","Content-Disposition":"inline","In-Reply-To":"<20191007224642.6597-4-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: controls: Store\n\tcontrol name in ControlId","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:16:42 -0000"}},{"id":2836,"web_url":"https://patchwork.libcamera.org/comment/2836/","msgid":"<20191009082310.GF22998@pendragon.ideasonboard.com>","date":"2019-10-09T08:23:10","subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: controls: Store\n\tcontrol name in ControlId","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:18:22AM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 08, 2019 at 01:46:36AM +0300, Laurent Pinchart wrote:\n> > The ControlId class stores a pointer to the control name. This works\n> > fine for statically-defined controls, but requires code that allocates\n> > controls dynamically (for instance based on control discovery on a V4L2\n> > device) to keep a list of control names in separate storage. To ease\n> > usage of dynamically allocated controls, store a copy of the control\n> > name string in the ControlId class.\n> \n> I would go as far as removing this from the ControlId, as it will need\n> to be serialized and it is used for debug purposes only.\n> \n> In case we want to retrieve it for debug, we can use the controls\n> global map you have reintroduced in patch 1\n\nBut that map stores the name in ControlId :-) Where would you store it ?\n\n> If you like best to keep it:\n> Reviewed-by: Jacopo Mondi \n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/controls.h | 6 +++---\n> > src/libcamera/controls.cpp | 2 +-\n> > 2 files changed, 4 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 49ff1a6f747f..a5a6a135ec16 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -54,11 +54,11 @@ class ControlId\n> > {\n> > public:\n> > \tunsigned int id() const { return id_; }\n> > -\tconst char *name() const { return name_; }\n> > +\tconst std::string &name() const { return name_; }\n> > \tControlType type() const { return type_; }\n> >\n> > protected:\n> > -\tControlId(unsigned int id, const char *name, ControlType type)\n> > +\tControlId(unsigned int id, const std::string &name, ControlType type)\n> > \t\t: id_(id), name_(name), type_(type)\n> > \t{\n> > \t}\n> > @@ -68,7 +68,7 @@ private:\n> > \tControlId &operator=(const ControlId &) = delete;\n> >\n> > \tunsigned int id_;\n> > -\tconst char *name_;\n> > +\tstd::string name_;\n> > \tControlType type_;\n> > };\n> >\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index f862a09adef9..e528dd80a2a7 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -205,7 +205,7 @@ std::string ControlValue::toString() const\n> > */\n> >\n> > /**\n> > - * \\fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)\n> > + * \\fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)\n> > * \\brief Construct a ControlId instance\n> > * \\param[in] id The control numerical ID\n> > * \\param[in] name The control name","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 2963C6157B\n\tfor ;\n\tWed, 9 Oct 2019 10:23:13 +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 34E7A4FF;\n\tWed, 9 Oct 2019 10:23:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570609392;\n\tbh=Agoe0obNPZOX5Yq685dsgtGpF+IMF5vA5iOf9eEP/48=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rRQJDtEuhqKZ+KLW8oGqPJqOgochRkjiHnQql++Jbb/p2TAMcZugs1kgz4LgON17l\n\tZgkhEXfnAV2/n4cfQ65yqYpS4lBp8+v2hnw3mnLu3xBKh6wT/dkDZ+RP+6ZMB5ztxO\n\tT+DeVgHX0Wjj8iVKnNwRRKSgNg7tXopXlcXlkIcs=","Date":"Wed, 9 Oct 2019 11:23:10 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191009082310.GF22998@pendragon.ideasonboard.com>","References":"<20191007224642.6597-1-laurent.pinchart@ideasonboard.com>\n\t<20191007224642.6597-4-laurent.pinchart@ideasonboard.com>\n\t<20191009081822.iwatv43jl56rjkuw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191009081822.iwatv43jl56rjkuw@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: controls: Store\n\tcontrol name in ControlId","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:23:13 -0000"}},{"id":2838,"web_url":"https://patchwork.libcamera.org/comment/2838/","msgid":"<20191009083814.4nuuym6vxpnjpiyu@uno.localdomain>","date":"2019-10-09T08:38:14","subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: controls: Store\n\tcontrol name in ControlId","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Oct 09, 2019 at 11:23:10AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Oct 09, 2019 at 10:18:22AM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 08, 2019 at 01:46:36AM +0300, Laurent Pinchart wrote:\n> > > The ControlId class stores a pointer to the control name. This works\n> > > fine for statically-defined controls, but requires code that allocates\n> > > controls dynamically (for instance based on control discovery on a V4L2\n> > > device) to keep a list of control names in separate storage. To ease\n> > > usage of dynamically allocated controls, store a copy of the control\n> > > name string in the ControlId class.\n> >\n> > I would go as far as removing this from the ControlId, as it will need\n> > to be serialized and it is used for debug purposes only.\n> >\n> > In case we want to retrieve it for debug, we can use the controls\n> > global map you have reintroduced in patch 1\n>\n> But that map stores the name in ControlId :-) Where would you store it ?\n>\n\nA derived class only used there?\n\nAnyway, scratch this, too complex now and not really something that\nconcerns this series.\n\nPlease take my tag in.\n\nThanks\n j\n\n> > If you like best to keep it:\n> > Reviewed-by: Jacopo Mondi \n> >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/controls.h | 6 +++---\n> > > src/libcamera/controls.cpp | 2 +-\n> > > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 49ff1a6f747f..a5a6a135ec16 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -54,11 +54,11 @@ class ControlId\n> > > {\n> > > public:\n> > > \tunsigned int id() const { return id_; }\n> > > -\tconst char *name() const { return name_; }\n> > > +\tconst std::string &name() const { return name_; }\n> > > \tControlType type() const { return type_; }\n> > >\n> > > protected:\n> > > -\tControlId(unsigned int id, const char *name, ControlType type)\n> > > +\tControlId(unsigned int id, const std::string &name, ControlType type)\n> > > \t\t: id_(id), name_(name), type_(type)\n> > > \t{\n> > > \t}\n> > > @@ -68,7 +68,7 @@ private:\n> > > \tControlId &operator=(const ControlId &) = delete;\n> > >\n> > > \tunsigned int id_;\n> > > -\tconst char *name_;\n> > > +\tstd::string name_;\n> > > \tControlType type_;\n> > > };\n> > >\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index f862a09adef9..e528dd80a2a7 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -205,7 +205,7 @@ std::string ControlValue::toString() const\n> > > */\n> > >\n> > > /**\n> > > - * \\fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)\n> > > + * \\fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)\n> > > * \\brief Construct a ControlId instance\n> > > * \\param[in] id The control numerical ID\n> > > * \\param[in] name The control name\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8431B6157B\n\tfor ;\n\tWed, 9 Oct 2019 10:36:29 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id B02EAC0008;\n\tWed, 9 Oct 2019 08:36:28 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 9 Oct 2019 10:38:14 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191009083814.4nuuym6vxpnjpiyu@uno.localdomain>","References":"<20191007224642.6597-1-laurent.pinchart@ideasonboard.com>\n\t<20191007224642.6597-4-laurent.pinchart@ideasonboard.com>\n\t<20191009081822.iwatv43jl56rjkuw@uno.localdomain>\n\t<20191009082310.GF22998@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"c23zkk7dzuugn26j\"","Content-Disposition":"inline","In-Reply-To":"<20191009082310.GF22998@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/9] libcamera: controls: Store\n\tcontrol name in ControlId","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:36:29 -0000"}}]