[{"id":16696,"web_url":"https://patchwork.libcamera.org/comment/16696/","msgid":"<20210430085810.u6e375tptodxkati@uno.localdomain>","date":"2021-04-30T08:58:10","subject":"Re: [libcamera-devel] [PATCH v3 2/7] libcamera: controls: Add extra\n\tcontrol values to ControlInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n + Laurent for a digression at the end of the email\n\nOn Wed, Apr 28, 2021 at 04:36:12PM +0900, Hirokazu Honda wrote:\n> The v4l2 menu contains not only index (int32_t) but also either\n> name (string) or value (int64_t). To support it keeping\n> ControlValue simple, this adds the extra ControlValues to\n> ControlInfo. With the ControlInfo, indeices are stored in\n> ControlInfo::values, and names (or values) are stored in\n> ControlInfo::extraValues.\n>\n\nI feel it will be hard to assign a proper semantic to what \"values\"\nand \"extra values\" are for a control.\n\nLet's take a step back and look at the issue at hand and ask ourselves\nif we really care about strings that are used to describe a test\npattern to users.\n\nWhat do we care about ?\n1) Being able to enumerate all the test patter modes supported by a\nsensor\n2) Map the sensor test patterns to a libcamera test pattern\n3) Map the libcamera test pattern to the Android one\n\nLooking at the rest of the series it seems the string test pattern\ndescription is used to map the sensor's test pattern mode to the\nlibcamera one. Is this necessary ? Can't we map indexes to indexes and\nleave strings out ?\n\nThis would allow us to:\n1) Augment V4L2Device::listControls() to add support for menu controls\nby only collecting indexes. This would work for menu and int_menu\ncontrols. The indexes can be collected one by one calls to\nVIDIOC_QUERYMENU. Once we have a list of indexes, V4L2ControlInfo\ncan be augmented with a contructor similar to\n\n ControlInfo::ControlInfo(Span values,\n const ControlValue &def)\n\nSo that it can transport a list of int32 (the indexes)\n\n2) The sensor db requires a bit more work, not that much actually.\nIn your series you add entries like:\n\n { \"Disabled\", controls::draft::TestPatternModeOff },\n\nCould this just be\n\n { 0, controls::draft::TestPatternModeOff },\n?\n\nOf course we are now tying the driver's patter listing order to the\nlibcamera's control, if a driver changes the ordering (why would it ?)\nthe sensor db will have to be updated. But this is not different.. if\nthe driver changes a pattern name (why would it?) we'll have to update\nthe db anyhow (possibly with versioning \\o/ )\n\nSetting a menu control goes by index anyhow, so it won't really make a\ndifference (when and if we'll have to support setting menu controls).\n\nThis might work for test pattern modes, as we don't actually care\nabout the mode name, if not for debugging purposes. It won't work that\nwell with INT_MENU controls, the most notable being CID_LINK_FREQ\n(even if its usage from userspace is limited, even not required\npossibly). In that case the index is less representitive than its\ncontent, so might chose for INT_MENU to collect the actual values\ninstead of the indexes, and in that case we'll need a way to reverse\nthe value->index association when we'll need to set an INT_MENU\ncontrol as I assume ph will want to say \"set LINK_FREQ to 192000000\"\nand not \"set LINK_FREQ to index 0\")\n\n--------- Digressions not related to this patch ----------------------\n--------- Mostly for discussions and not blocking -------------------\n\nA question remains when setting a menu control: where does the\nvalue->index reversing happens ?\n\nApplications/Android will tell \"set TEST_PATTERN to\nlibcamera::controls::TestPatternXYZ\" to the pipeline\nhandler that will receive a control list with\n\n { TestPatternMode, TestPatternModeOff }\n\nand it will have to set it in the video device or the camera sensor.\n\nFor video devices, we have a pure v4l2 controls based interface, so\nthe ph will have to do the reversing:\n\n TestPatternModeOff = index 0\n\nby itself as it knows its video devices and can create a map for that\npurpose.\n\nFor camera sensor we have a v4l2 controls based interface, but that's\nnot really finalized, and if we want the ph to do the value->index\nreversing it mean we'll have to expose the sensor db. Otherwise we\nintroduce a libcamera::Controls based interface in CameraSensor and\nkeep the reversing internal, but this ofc has other drawbacks (the\nmost notable will be the fact we'll have two interfaces (one v4l2, the\nother libcamera) or we drop the v4l2 interface from CameraSensor\n(which will require IPAs to speak libcamera::controls when they send\nsensor's controls to the ph).\n\nWhat do you think ?\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/controls.h | 5 +++++\n> src/libcamera/controls.cpp | 22 ++++++++++++++++++++++\n> 2 files changed, 27 insertions(+)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1a5690a5..a8deb16a 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -271,11 +271,15 @@ public:\n> \t\t\t const ControlValue &def = 0);\n> \texplicit ControlInfo(Span values,\n> \t\t\t const ControlValue &def = {});\n> +\texplicit ControlInfo(Span values,\n> +\t\t\t Span extraValues,\n> +\t\t\t const ControlValue &def = {});\n>\n> \tconst ControlValue &min() const { return min_; }\n> \tconst ControlValue &max() const { return max_; }\n> \tconst ControlValue &def() const { return def_; }\n> \tconst std::vector &values() const { return values_; }\n> +\tconst std::vector &extraValues() const { return extraValues_; }\n>\n> \tstd::string toString() const;\n>\n> @@ -294,6 +298,7 @@ private:\n> \tControlValue max_;\n> \tControlValue def_;\n> \tstd::vector values_;\n> +\tstd::vector extraValues_;\n> };\n>\n> using ControlIdMap = std::unordered_map;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index c58ed394..e2e8619a 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span values,\n> \t\tvalues_.push_back(value);\n> }\n>\n> +/**\n> + * \\brief Construct a ControlInfo from the list of valid values and extra values\n> + * \\param[in] values The control valid values\n> + * \\param[in] extraValues The control valid extra values associated with \\a values\n> + * \\param[in] def The control default value\n> + *\n> + * Construct a ControlInfo from a list of valid values and extra values. The\n> + * ControlInfo minimum and maximum values are set to the first and last members\n> + * of the values list respectively. The default value is set to \\a def if\n> + * provided, or to the minimum value otherwise. The extra values are associated\n> + * with \\a values and in the same order as \\a values.\n> + *\n> + */\n> +ControlInfo::ControlInfo(Span values,\n> +\t\t\t Span extraValues,\n> +\t\t\t const ControlValue &def)\n> +\t: ControlInfo(values, def)\n> +{\n> +\tfor (const ControlValue &extraValue : extraValues)\n> +\t\textraValues_.push_back(extraValue);\n> +}\n> +\n> /**\n> * \\fn ControlInfo::min()\n> * \\brief Retrieve the minimum value of the control\n> --\n> 2.31.1.498.g6c1eba8ee3d-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 DD7A9BDE47\n\tfor ;\n\tFri, 30 Apr 2021 08:57:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DE866890E;\n\tFri, 30 Apr 2021 10:57:31 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED33E688AF\n\tfor ;\n\tFri, 30 Apr 2021 10:57:28 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 0EACF100004;\n\tFri, 30 Apr 2021 08:57:27 +0000 (UTC)"],"Date":"Fri, 30 Apr 2021 10:58:10 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","Message-ID":"<20210430085810.u6e375tptodxkati@uno.localdomain>","References":"<20210428073617.373422-1-hiroh@chromium.org>\n\t<20210428073617.373422-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210428073617.373422-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 2/7] libcamera: controls: Add extra\n\tcontrol values to ControlInfo","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16697,"web_url":"https://patchwork.libcamera.org/comment/16697/","msgid":"","date":"2021-04-30T11:44:13","subject":"Re: [libcamera-devel] [PATCH v3 2/7] libcamera: controls: Add extra\n\tcontrol values to ControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Apr 30, 2021 at 10:58:10AM +0200, Jacopo Mondi wrote:\n> Hi Hiro,\n> + Laurent for a digression at the end of the email\n> \n> On Wed, Apr 28, 2021 at 04:36:12PM +0900, Hirokazu Honda wrote:\n> > The v4l2 menu contains not only index (int32_t) but also either\n> > name (string) or value (int64_t). To support it keeping\n> > ControlValue simple, this adds the extra ControlValues to\n> > ControlInfo. With the ControlInfo, indeices are stored in\n> > ControlInfo::values, and names (or values) are stored in\n> > ControlInfo::extraValues.\n> >\n> \n> I feel it will be hard to assign a proper semantic to what \"values\"\n> and \"extra values\" are for a control.\n> \n> Let's take a step back and look at the issue at hand and ask ourselves\n> if we really care about strings that are used to describe a test\n> pattern to users.\n> \n> What do we care about ?\n> 1) Being able to enumerate all the test patter modes supported by a\n> sensor\n> 2) Map the sensor test patterns to a libcamera test pattern\n> 3) Map the libcamera test pattern to the Android one\n> \n> Looking at the rest of the series it seems the string test pattern\n> description is used to map the sensor's test pattern mode to the\n> libcamera one. Is this necessary ? Can't we map indexes to indexes and\n> leave strings out ?\n\nAgreed on all that. The semantics of \"extraValues()\" is very\nill-defined, and it would be hard to fix that. It's a hack, and we don't\nwant hacks in the public API, we want a properly designed solution.\n\nMenu strings are not needed in ControlList, and ControlValue is mostly\nmeant as the storage for variant entries in ControlList. It is also\nreused in ControlInfo, but that's not its main purpose. If we want to\nhandle menu strings, they should go to ControlInfo directly. Even then,\nit would need to be done in a clean way from a public API point of view.\nIn particular, the issue of internationalisation needs to be taken into\naccount, so I'm really, really not keen to have menu strings, anywhere.\n\n> This would allow us to:\n> 1) Augment V4L2Device::listControls() to add support for menu controls\n> by only collecting indexes. This would work for menu and int_menu\n> controls. The indexes can be collected one by one calls to\n> VIDIOC_QUERYMENU. Once we have a list of indexes, V4L2ControlInfo\n> can be augmented with a contructor similar to\n> \n> ControlInfo::ControlInfo(Span values,\n> const ControlValue &def)\n> \n> So that it can transport a list of int32 (the indexes)\n> \n> 2) The sensor db requires a bit more work, not that much actually.\n> In your series you add entries like:\n> \n> { \"Disabled\", controls::draft::TestPatternModeOff },\n> \n> Could this just be\n> \n> { 0, controls::draft::TestPatternModeOff },\n> ?\n> \n> Of course we are now tying the driver's patter listing order to the\n> libcamera's control, if a driver changes the ordering (why would it ?)\n> the sensor db will have to be updated. But this is not different.. if\n> the driver changes a pattern name (why would it?) we'll have to update\n> the db anyhow (possibly with versioning \\o/ )\n> \n> Setting a menu control goes by index anyhow, so it won't really make a\n> difference (when and if we'll have to support setting menu controls).\n> \n> This might work for test pattern modes, as we don't actually care\n> about the mode name, if not for debugging purposes. It won't work that\n> well with INT_MENU controls, the most notable being CID_LINK_FREQ\n> (even if its usage from userspace is limited, even not required\n> possibly). In that case the index is less representitive than its\n> content, so might chose for INT_MENU to collect the actual values\n> instead of the indexes, and in that case we'll need a way to reverse\n> the value->index association when we'll need to set an INT_MENU\n> control as I assume ph will want to say \"set LINK_FREQ to 192000000\"\n> and not \"set LINK_FREQ to index 0\")\n> \n> --------- Digressions not related to this patch ----------------------\n> --------- Mostly for discussions and not blocking -------------------\n> \n> A question remains when setting a menu control: where does the\n> value->index reversing happens ?\n> \n> Applications/Android will tell \"set TEST_PATTERN to\n> libcamera::controls::TestPatternXYZ\" to the pipeline\n> handler that will receive a control list with\n> \n> { TestPatternMode, TestPatternModeOff }\n> \n> and it will have to set it in the video device or the camera sensor.\n> \n> For video devices, we have a pure v4l2 controls based interface, so\n> the ph will have to do the reversing:\n> \n> TestPatternModeOff = index 0\n> \n> by itself as it knows its video devices and can create a map for that\n> purpose.\n> \n> For camera sensor we have a v4l2 controls based interface, but that's\n> not really finalized, and if we want the ph to do the value->index\n> reversing it mean we'll have to expose the sensor db. Otherwise we\n> introduce a libcamera::Controls based interface in CameraSensor and\n> keep the reversing internal, but this ofc has other drawbacks (the\n> most notable will be the fact we'll have two interfaces (one v4l2, the\n> other libcamera) or we drop the v4l2 interface from CameraSensor\n> (which will require IPAs to speak libcamera::controls when they send\n> sensor's controls to the ph).\n> \n> What do you think ?\n> \n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/controls.h | 5 +++++\n> > src/libcamera/controls.cpp | 22 ++++++++++++++++++++++\n> > 2 files changed, 27 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1a5690a5..a8deb16a 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -271,11 +271,15 @@ public:\n> > \t\t\t const ControlValue &def = 0);\n> > \texplicit ControlInfo(Span values,\n> > \t\t\t const ControlValue &def = {});\n> > +\texplicit ControlInfo(Span values,\n> > +\t\t\t Span extraValues,\n> > +\t\t\t const ControlValue &def = {});\n> >\n> > \tconst ControlValue &min() const { return min_; }\n> > \tconst ControlValue &max() const { return max_; }\n> > \tconst ControlValue &def() const { return def_; }\n> > \tconst std::vector &values() const { return values_; }\n> > +\tconst std::vector &extraValues() const { return extraValues_; }\n> >\n> > \tstd::string toString() const;\n> >\n> > @@ -294,6 +298,7 @@ private:\n> > \tControlValue max_;\n> > \tControlValue def_;\n> > \tstd::vector values_;\n> > +\tstd::vector extraValues_;\n> > };\n> >\n> > using ControlIdMap = std::unordered_map;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index c58ed394..e2e8619a 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span values,\n> > \t\tvalues_.push_back(value);\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a ControlInfo from the list of valid values and extra values\n> > + * \\param[in] values The control valid values\n> > + * \\param[in] extraValues The control valid extra values associated with \\a values\n> > + * \\param[in] def The control default value\n> > + *\n> > + * Construct a ControlInfo from a list of valid values and extra values. The\n> > + * ControlInfo minimum and maximum values are set to the first and last members\n> > + * of the values list respectively. The default value is set to \\a def if\n> > + * provided, or to the minimum value otherwise. The extra values are associated\n> > + * with \\a values and in the same order as \\a values.\n> > + *\n> > + */\n> > +ControlInfo::ControlInfo(Span values,\n> > +\t\t\t Span extraValues,\n> > +\t\t\t const ControlValue &def)\n> > +\t: ControlInfo(values, def)\n> > +{\n> > +\tfor (const ControlValue &extraValue : extraValues)\n> > +\t\textraValues_.push_back(extraValue);\n> > +}\n> > +\n> > /**\n> > * \\fn ControlInfo::min()\n> > * \\brief Retrieve the minimum value of the control","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 20B98BDE48\n\tfor ;\n\tFri, 30 Apr 2021 11:44:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D351A68901;\n\tFri, 30 Apr 2021 13:44:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE4C2688A5\n\tfor ;\n\tFri, 30 Apr 2021 13:44:14 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4FAE41027;\n\tFri, 30 Apr 2021 13:44:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"l4v8sfXk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619783054;\n\tbh=RvX0eEp/zgKTZ4M7pY17doj0Kd83HifAXl7IY+1SEU4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l4v8sfXkU7dklpYaVHAKOE7ZSPxmgYriDGW3tI1r49cpMqUrmZ4ZpR0xE59KDk6TU\n\tQFQlsEU88Z4BiX85OSdknad234PVZgLy/HzYXYASyC6gsyEiwy1uw4XrPz+zc5F6jt\n\tO4ebDpfQ5CiGUESDcDZII/arfPJB62LiQRfKEdMc=","Date":"Fri, 30 Apr 2021 14:44:13 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210428073617.373422-1-hiroh@chromium.org>\n\t<20210428073617.373422-3-hiroh@chromium.org>\n\t<20210430085810.u6e375tptodxkati@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210430085810.u6e375tptodxkati@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 2/7] libcamera: controls: Add extra\n\tcontrol values to ControlInfo","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16790,"web_url":"https://patchwork.libcamera.org/comment/16790/","msgid":"","date":"2021-05-06T06:12:12","subject":"Re: [libcamera-devel] [PATCH v3 2/7] libcamera: controls: Add extra\n\tcontrol values to ControlInfo","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo and Laurent,\n\nOn Fri, Apr 30, 2021 at 8:44 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Fri, Apr 30, 2021 at 10:58:10AM +0200, Jacopo Mondi wrote:\n> > Hi Hiro,\n> > + Laurent for a digression at the end of the email\n> >\n> > On Wed, Apr 28, 2021 at 04:36:12PM +0900, Hirokazu Honda wrote:\n> > > The v4l2 menu contains not only index (int32_t) but also either\n> > > name (string) or value (int64_t). To support it keeping\n> > > ControlValue simple, this adds the extra ControlValues to\n> > > ControlInfo. With the ControlInfo, indeices are stored in\n> > > ControlInfo::values, and names (or values) are stored in\n> > > ControlInfo::extraValues.\n> > >\n> >\n> > I feel it will be hard to assign a proper semantic to what \"values\"\n> > and \"extra values\" are for a control.\n> >\n> > Let's take a step back and look at the issue at hand and ask ourselves\n> > if we really care about strings that are used to describe a test\n> > pattern to users.\n> >\n> > What do we care about ?\n> > 1) Being able to enumerate all the test patter modes supported by a\n> > sensor\n> > 2) Map the sensor test patterns to a libcamera test pattern\n> > 3) Map the libcamera test pattern to the Android one\n> >\n> > Looking at the rest of the series it seems the string test pattern\n> > description is used to map the sensor's test pattern mode to the\n> > libcamera one. Is this necessary ? Can't we map indexes to indexes and\n> > leave strings out ?\n>\n> Agreed on all that. The semantics of \"extraValues()\" is very\n> ill-defined, and it would be hard to fix that. It's a hack, and we don't\n> want hacks in the public API, we want a properly designed solution.\n>\n> Menu strings are not needed in ControlList, and ControlValue is mostly\n> meant as the storage for variant entries in ControlList. It is also\n> reused in ControlInfo, but that's not its main purpose. If we want to\n> handle menu strings, they should go to ControlInfo directly. Even then,\n> it would need to be done in a clean way from a public API point of view.\n> In particular, the issue of internationalisation needs to be taken into\n> account, so I'm really, really not keen to have menu strings, anywhere.\n>\n>\nLaurent, is the implementation of this patch what you suggested me?\nI will abandon this and re-implement with Jacopo's idea. But I would like\nto make sure if I didn't miss something.\n\n-Hiro\n\n\n> > This would allow us to:\n> > 1) Augment V4L2Device::listControls() to add support for menu controls\n> > by only collecting indexes. This would work for menu and int_menu\n> > controls. The indexes can be collected one by one calls to\n> > VIDIOC_QUERYMENU. Once we have a list of indexes, V4L2ControlInfo\n> > can be augmented with a contructor similar to\n> >\n> > ControlInfo::ControlInfo(Span values,\n> > const ControlValue &def)\n> >\n> > So that it can transport a list of int32 (the indexes)\n> >\n> > 2) The sensor db requires a bit more work, not that much actually.\n> > In your series you add entries like:\n> >\n> > { \"Disabled\", controls::draft::TestPatternModeOff },\n> >\n> > Could this just be\n> >\n> > { 0, controls::draft::TestPatternModeOff },\n> > ?\n> >\n> > Of course we are now tying the driver's patter listing order to the\n> > libcamera's control, if a driver changes the ordering (why would it ?)\n> > the sensor db will have to be updated. But this is not different.. if\n> > the driver changes a pattern name (why would it?) we'll have to update\n> > the db anyhow (possibly with versioning \\o/ )\n> >\n> > Setting a menu control goes by index anyhow, so it won't really make a\n> > difference (when and if we'll have to support setting menu controls).\n> >\n> > This might work for test pattern modes, as we don't actually care\n> > about the mode name, if not for debugging purposes. It won't work that\n> > well with INT_MENU controls, the most notable being CID_LINK_FREQ\n> > (even if its usage from userspace is limited, even not required\n> > possibly). In that case the index is less representitive than its\n> > content, so might chose for INT_MENU to collect the actual values\n> > instead of the indexes, and in that case we'll need a way to reverse\n> > the value->index association when we'll need to set an INT_MENU\n> > control as I assume ph will want to say \"set LINK_FREQ to 192000000\"\n> > and not \"set LINK_FREQ to index 0\")\n> >\n> > --------- Digressions not related to this patch ----------------------\n> > --------- Mostly for discussions and not blocking -------------------\n> >\n> > A question remains when setting a menu control: where does the\n> > value->index reversing happens ?\n> >\n> > Applications/Android will tell \"set TEST_PATTERN to\n> > libcamera::controls::TestPatternXYZ\" to the pipeline\n> > handler that will receive a control list with\n> >\n> > { TestPatternMode, TestPatternModeOff }\n> >\n> > and it will have to set it in the video device or the camera sensor.\n> >\n> > For video devices, we have a pure v4l2 controls based interface, so\n> > the ph will have to do the reversing:\n> >\n> > TestPatternModeOff = index 0\n> >\n> > by itself as it knows its video devices and can create a map for that\n> > purpose.\n> >\n> > For camera sensor we have a v4l2 controls based interface, but that's\n> > not really finalized, and if we want the ph to do the value->index\n> > reversing it mean we'll have to expose the sensor db. Otherwise we\n> > introduce a libcamera::Controls based interface in CameraSensor and\n> > keep the reversing internal, but this ofc has other drawbacks (the\n> > most notable will be the fact we'll have two interfaces (one v4l2, the\n> > other libcamera) or we drop the v4l2 interface from CameraSensor\n> > (which will require IPAs to speak libcamera::controls when they send\n> > sensor's controls to the ph).\n> >\n> > What do you think ?\n> >\n\n\nI agreed.\nIt sounds like the current control implementation is enough for menu\nbecause we should only store the index.\n\n\n>\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/controls.h | 5 +++++\n> > > src/libcamera/controls.cpp | 22 ++++++++++++++++++++++\n> > > 2 files changed, 27 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/controls.h\n> b/include/libcamera/controls.h\n> > > index 1a5690a5..a8deb16a 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -271,11 +271,15 @@ public:\n> > > const ControlValue &def = 0);\n> > > explicit ControlInfo(Span values,\n> > > const ControlValue &def = {});\n> > > + explicit ControlInfo(Span values,\n> > > + Span extraValues,\n> > > + const ControlValue &def = {});\n> > >\n> > > const ControlValue &min() const { return min_; }\n> > > const ControlValue &max() const { return max_; }\n> > > const ControlValue &def() const { return def_; }\n> > > const std::vector &values() const { return values_; }\n> > > + const std::vector &extraValues() const { return\n> extraValues_; }\n> > >\n> > > std::string toString() const;\n> > >\n> > > @@ -294,6 +298,7 @@ private:\n> > > ControlValue max_;\n> > > ControlValue def_;\n> > > std::vector values_;\n> > > + std::vector extraValues_;\n> > > };\n> > >\n> > > using ControlIdMap = std::unordered_map *>;\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index c58ed394..e2e8619a 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span\n> values,\n> > > values_.push_back(value);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Construct a ControlInfo from the list of valid values and\n> extra values\n> > > + * \\param[in] values The control valid values\n> > > + * \\param[in] extraValues The control valid extra values associated\n> with \\a values\n> > > + * \\param[in] def The control default value\n> > > + *\n> > > + * Construct a ControlInfo from a list of valid values and extra\n> values. The\n> > > + * ControlInfo minimum and maximum values are set to the first and\n> last members\n> > > + * of the values list respectively. The default value is set to \\a\n> def if\n> > > + * provided, or to the minimum value otherwise. The extra values are\n> associated\n> > > + * with \\a values and in the same order as \\a values.\n> > > + *\n> > > + */\n> > > +ControlInfo::ControlInfo(Span values,\n> > > + Span extraValues,\n> > > + const ControlValue &def)\n> > > + : ControlInfo(values, def)\n> > > +{\n> > > + for (const ControlValue &extraValue : extraValues)\n> > > + extraValues_.push_back(extraValue);\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn ControlInfo::min()\n> > > * \\brief Retrieve the minimum value of the control\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","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 2D1A5BDE7E\n\tfor ;\n\tThu, 6 May 2021 06:12:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DBEC68909;\n\tThu, 6 May 2021 08:12:24 +0200 (CEST)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D57CC688E5\n\tfor ;\n\tThu, 6 May 2021 08:12:22 +0200 (CEST)","by mail-ej1-x62f.google.com with SMTP id l4so6473164ejc.10\n\tfor ;\n\tWed, 05 May 2021 23:12:22 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"YZYcNa38\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=KPGhdK5YKP0uHM5o0qijlGzYv5cv3zfZNJFe2h2Pfyo=;\n\tb=YZYcNa38Ds1NTQWZh5bvrOYFH29Nn37eodNBoOP4GFB/PaH31Rr5VPbcwAljv+Rzol\n\t+YeyPCEvFDKaVkotIIvHdE2TQ+BzIDdPJ9YSBNINxxGSZsye1EEDtCKfEg6yNFyTGmNf\n\tVpV3szSU0UecB1FbB2W+5Sv4FJQkw3TYpXIqY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=KPGhdK5YKP0uHM5o0qijlGzYv5cv3zfZNJFe2h2Pfyo=;\n\tb=EXRXZ/C4vl+jTtLAtp94AHKJ2BIrhPyPc9O7IN6tj3TwKnalENM3aGxJnSfa97vHeT\n\ty1Q1K/xLgOla73PQ8Db/TG1uvDqmaYREROfVcgZ4mSdH5ntfpLIRPuRrYAq3H6zukW48\n\tuqkXyOCOj1u6DjilGAei1OF3buEwmJAFg7+ZeUSk8kfWpQlbO1egKglVtrovvvIlCIC8\n\t3K7KBGFMsE4V9WZy/s2htH0J7p+l+M9T9UFSsramEUbB3/4uhydbmFXVQsENCCO50HyO\n\t7OcBlP6EuaL9rwMDXUNJjOqQOmMS7Ey+Kcp4JzfakhwVC8Cxa0FwYSknTDGHbxCcgXtu\n\teUDg==","X-Gm-Message-State":"AOAM530nlp9PDLiiCrBi0+hq2cC0z75q1tPNNVv20C6qcVCtk7+eH7lH\n\tQhSEjlqOjEGYYU7afCZbACFSXSPBSxTUpaRXlW7XJw==","X-Google-Smtp-Source":"ABdhPJy90CDN2w3UPygFPWwBawkRuSmSDsTEX7Dj1An9u+nfqmkIOjVeCPXmOJmLkJtBWBxQY5Mbi5ZBmp84DgRde3E=","X-Received":"by 2002:a17:906:5acd:: with SMTP id\n\tx13mr2471600ejs.243.1620281542535; \n\tWed, 05 May 2021 23:12:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20210428073617.373422-1-hiroh@chromium.org>\n\t<20210428073617.373422-3-hiroh@chromium.org>\n\t<20210430085810.u6e375tptodxkati@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 6 May 2021 15:12:12 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 2/7] libcamera: controls: Add extra\n\tcontrol values to ControlInfo","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","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============2658626646277714152==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]