[{"id":4528,"web_url":"https://patchwork.libcamera.org/comment/4528/","msgid":"<20200426142454.bcs22uomcf2eo2ys@uno.localdomain>","date":"2020-04-26T14:24:54","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: controls: Return\n\tControlValue reference from ControlList::set()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Apr 26, 2020 at 02:16:22AM +0300, Laurent Pinchart wrote:\n> It is useful for the caller of ControlList::set() to get a reference to\n> the ControlValue that stores the control in the control list, in order\n> to then modify that value directly. This allows creating an entry in\n> the ControlList and then reserving memory for the control when getting\n> V4L2 controls from a device. This is already possible by calling the\n> get() function right after set(), but results in two lookups. Extend the\n> id-based set() function to return the reference to the inserted\n> ControlValue to avoid the second lookup.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/controls.h | 2 +-\n> src/libcamera/controls.cpp | 7 +++++--\n> 2 files changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 80944efc133a..5a5cfc3e52ca 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -394,7 +394,7 @@ public:\n> \t}\n>\n> \tconst ControlValue &get(unsigned int id) const;\n> -\tvoid set(unsigned int id, const ControlValue &value);\n> +\tControlValue &set(unsigned int id, const ControlValue &value);\n>\n> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n>\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 08df7f29e938..12822e87a4d7 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -930,14 +930,17 @@ const ControlValue &ControlList::get(unsigned int id) const\n> *\n> * The behaviour is undefined if the control \\a id is not supported by the\n> * object that the list refers to.\n> + *\n> + * \\return A reference to the ControlValue stored in the control list\n> */\n> -void ControlList::set(unsigned int id, const ControlValue &value)\n> +ControlValue &ControlList::set(unsigned int id, const ControlValue &value)\n> {\n> \tControlValue *val = find(id);\n> \tif (!val)\n> -\t\treturn;\n> +\t\treturn *val;\n\nWouldn't you dereference a nullptr ? Should we create a\n static ControlValue empty{};\n\nat function begin and return that one ?\n\nOverall, I think I'm still missing the use case for this tbh, but\nwe're not losing anything, as the previous return type was void, so\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> \t*val = value;\n> +\treturn *val;\n> }\n>\n> /**\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE068603FA\n\tfor ;\n\tSun, 26 Apr 2020 16:21:54 +0200 (CEST)","from uno.localdomain\n\t(host170-48-dynamic.3-87-r.retail.telecomitalia.it [87.3.48.170])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 8E6D040002;\n\tSun, 26 Apr 2020 14:21:53 +0000 (UTC)"],"X-Originating-IP":"87.3.48.170","Date":"Sun, 26 Apr 2020 16:24:54 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426142454.bcs22uomcf2eo2ys@uno.localdomain>","References":"<20200425231623.6505-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425231623.6505-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: controls: Return\n\tControlValue reference from ControlList::set()","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, 26 Apr 2020 14:21:55 -0000"}},{"id":4538,"web_url":"https://patchwork.libcamera.org/comment/4538/","msgid":"<20200426173052.GF5950@pendragon.ideasonboard.com>","date":"2020-04-26T17:30:52","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: controls: Return\n\tControlValue reference from ControlList::set()","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, Apr 26, 2020 at 04:24:54PM +0200, Jacopo Mondi wrote:\n> On Sun, Apr 26, 2020 at 02:16:22AM +0300, Laurent Pinchart wrote:\n> > It is useful for the caller of ControlList::set() to get a reference to\n> > the ControlValue that stores the control in the control list, in order\n> > to then modify that value directly. This allows creating an entry in\n> > the ControlList and then reserving memory for the control when getting\n> > V4L2 controls from a device. This is already possible by calling the\n> > get() function right after set(), but results in two lookups. Extend the\n> > id-based set() function to return the reference to the inserted\n> > ControlValue to avoid the second lookup.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/controls.h | 2 +-\n> > src/libcamera/controls.cpp | 7 +++++--\n> > 2 files changed, 6 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 80944efc133a..5a5cfc3e52ca 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -394,7 +394,7 @@ public:\n> > \t}\n> >\n> > \tconst ControlValue &get(unsigned int id) const;\n> > -\tvoid set(unsigned int id, const ControlValue &value);\n> > +\tControlValue &set(unsigned int id, const ControlValue &value);\n> >\n> > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> >\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 08df7f29e938..12822e87a4d7 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -930,14 +930,17 @@ const ControlValue &ControlList::get(unsigned int id) const\n> > *\n> > * The behaviour is undefined if the control \\a id is not supported by the\n> > * object that the list refers to.\n> > + *\n> > + * \\return A reference to the ControlValue stored in the control list\n> > */\n> > -void ControlList::set(unsigned int id, const ControlValue &value)\n> > +ControlValue &ControlList::set(unsigned int id, const ControlValue &value)\n> > {\n> > \tControlValue *val = find(id);\n> > \tif (!val)\n> > -\t\treturn;\n> > +\t\treturn *val;\n> \n> Wouldn't you dereference a nullptr ? Should we create a\n\nI agree. I actually dreamt about libcamera last night and figured out\nthis issue in my sleep. I wonder how bad that is :-)\n\n> static ControlValue empty{};\n> \n> at function begin and return that one ?\n\nThe set() function is documented as having an undefined behaviour if the\ncontrol isn't supported, so this is within the range of possible\nundefined behaviours (so would rm -rf /*). It's not very nice though.\nReturning a reference to a local static variable isn't nice either, as\nthe caller won't know that an error occurred. I'm leaning towards\nreturning a pointer, until we refactor this API (getting and setting\ncontrols by numerical ID isn't nice in general). I'll see what I can do.\n\n> Overall, I think I'm still missing the use case for this tbh, but\n\nIt's to avoid a second lookup in patch 2/2.\n\n> we're not losing anything, as the previous return type was void, so\n> Reviewed-by: Jacopo Mondi \n> \n> >\n> > \t*val = value;\n> > +\treturn *val;\n> > }\n> >\n> > /**","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 505F262E55\n\tfor ;\n\tSun, 26 Apr 2020 19:31:08 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BB60F4F7;\n\tSun, 26 Apr 2020 19:31:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XYORzm1/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587922267;\n\tbh=uHHmd2S48YbZ6S/YtvuTpHxk1/mbsj5zNkQauOuyJcQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XYORzm1/BO49O+V1X3oSA8SLrGeEF4N2fCErPvdHL2caC2JLIu3BD6vC6MMttVxM0\n\tMWXHPHzR0gbPnG1AdZMsCb7x0O80yzsIMQQahBHQbYJ8Rv2X02SppCHXGIq4uo/iXd\n\thdm3wvgQGVIJBz+y7dZ42bMqo9T4ZvvIKos+xlYE=","Date":"Sun, 26 Apr 2020 20:30:52 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426173052.GF5950@pendragon.ideasonboard.com>","References":"<20200425231623.6505-1-laurent.pinchart@ideasonboard.com>\n\t<20200426142454.bcs22uomcf2eo2ys@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426142454.bcs22uomcf2eo2ys@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: controls: Return\n\tControlValue reference from ControlList::set()","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, 26 Apr 2020 17:31:08 -0000"}},{"id":4567,"web_url":"https://patchwork.libcamera.org/comment/4567/","msgid":"<20200427120825.nk5cd5wmfhvgk6qq@uno.localdomain>","date":"2020-04-27T12:08:25","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: controls: Return\n\tControlValue reference from ControlList::set()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Apr 26, 2020 at 08:30:52PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sun, Apr 26, 2020 at 04:24:54PM +0200, Jacopo Mondi wrote:\n> > On Sun, Apr 26, 2020 at 02:16:22AM +0300, Laurent Pinchart wrote:\n> > > It is useful for the caller of ControlList::set() to get a reference to\n> > > the ControlValue that stores the control in the control list, in order\n> > > to then modify that value directly. This allows creating an entry in\n> > > the ControlList and then reserving memory for the control when getting\n> > > V4L2 controls from a device. This is already possible by calling the\n> > > get() function right after set(), but results in two lookups. Extend the\n> > > id-based set() function to return the reference to the inserted\n> > > ControlValue to avoid the second lookup.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/controls.h | 2 +-\n> > > src/libcamera/controls.cpp | 7 +++++--\n> > > 2 files changed, 6 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 80944efc133a..5a5cfc3e52ca 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -394,7 +394,7 @@ public:\n> > > \t}\n> > >\n> > > \tconst ControlValue &get(unsigned int id) const;\n> > > -\tvoid set(unsigned int id, const ControlValue &value);\n> > > +\tControlValue &set(unsigned int id, const ControlValue &value);\n> > >\n> > > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > >\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 08df7f29e938..12822e87a4d7 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -930,14 +930,17 @@ const ControlValue &ControlList::get(unsigned int id) const\n> > > *\n> > > * The behaviour is undefined if the control \\a id is not supported by the\n> > > * object that the list refers to.\n> > > + *\n> > > + * \\return A reference to the ControlValue stored in the control list\n> > > */\n> > > -void ControlList::set(unsigned int id, const ControlValue &value)\n> > > +ControlValue &ControlList::set(unsigned int id, const ControlValue &value)\n> > > {\n> > > \tControlValue *val = find(id);\n> > > \tif (!val)\n> > > -\t\treturn;\n> > > +\t\treturn *val;\n> >\n> > Wouldn't you dereference a nullptr ? Should we create a\n>\n> I agree. I actually dreamt about libcamera last night and figured out\n> this issue in my sleep. I wonder how bad that is :-)\n>\n> > static ControlValue empty{};\n> >\n> > at function begin and return that one ?\n>\n> The set() function is documented as having an undefined behaviour if the\n> control isn't supported, so this is within the range of possible\n> undefined behaviours (so would rm -rf /*). It's not very nice though.\n> Returning a reference to a local static variable isn't nice either, as\n> the caller won't know that an error occurred. I'm leaning towards\n> returning a pointer, until we refactor this API (getting and setting\n> controls by numerical ID isn't nice in general). I'll see what I can do.\n\nPlease be aware the same happens already in\n const ControlValue &ControlList::get(unsigned int id) const\n\n>\n> > Overall, I think I'm still missing the use case for this tbh, but\n>\n> It's to avoid a second lookup in patch 2/2.\n>\n> > we're not losing anything, as the previous return type was void, so\n> > Reviewed-by: Jacopo Mondi \n> >\n> > >\n> > > \t*val = value;\n> > > +\treturn *val;\n> > > }\n> > >\n> > > /**\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 C17AC603F9\n\tfor ;\n\tMon, 27 Apr 2020 14:05:15 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id D96C0FF818;\n\tMon, 27 Apr 2020 12:05:14 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Mon, 27 Apr 2020 14:08:25 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427120825.nk5cd5wmfhvgk6qq@uno.localdomain>","References":"<20200425231623.6505-1-laurent.pinchart@ideasonboard.com>\n\t<20200426142454.bcs22uomcf2eo2ys@uno.localdomain>\n\t<20200426173052.GF5950@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426173052.GF5950@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: controls: Return\n\tControlValue reference from ControlList::set()","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":"Mon, 27 Apr 2020 12:05:15 -0000"}}]