[{"id":16601,"web_url":"https://patchwork.libcamera.org/comment/16601/","msgid":"","date":"2021-04-27T03:31:16","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Wed, Apr 21, 2021 at 01:23:41PM +0900, Hirokazu Honda wrote:\n> This adds control type for v4l2 menu.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++\n> src/libcamera/controls.cpp | 7 +++++++\n> 2 files changed, 35 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1a5690a5..357819e7 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -9,6 +9,7 @@\n> #define __LIBCAMERA_CONTROLS_H__\n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -32,6 +33,28 @@ enum ControlType {\n> \tControlTypeString,\n> \tControlTypeRectangle,\n> \tControlTypeSize,\n> +\tControlTypeMenu,\n> +};\n> +\n> +struct ControlMenu {\n> +\tuint32_t index;\n> +\tbool isName = false;\n> +\n> +\tunion {\n> +\t\tchar name[28];\n> +\t\tint64_t value;\n> +\t};\n> +\n> +\tstd::string toString() const\n> +\t{\n> +\t\tstd::stringstream ss;\n> +\t\tss << \"index: \" << index;\n> +\t\tif (isName)\n> +\t\t\tss << \"name: \" << name;\n> +\t\telse\n> +\t\t\tss << \"value: \" << value;\n> +\t\treturn ss.str();\n> +\t}\n> };\n\nAs this is used for V4L2 controls only, I'm really not keen on adding\nsupport for named menu entries :-( Especially given that 28 is a\ncompletely arbitrary limit. I'd like to instead store the value as an\ninteger, and, if we really need support for named menu entries, to add\nthem to the ControlInfo class instead ? The mapping between numerical\nvalues and names should be constant, it shouldn't be a property of the\nControlValue.\n\n> \n> namespace details {\n> @@ -85,6 +108,11 @@ struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeSize;\n> };\n> \n> +template<>\n> +struct control_type {\n> +\tstatic constexpr ControlType value = ControlTypeMenu;\n> +};\n> +\n> template\n> struct control_type> : public control_type> {\n> };\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index c58ed394..8ab5ac92 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -60,6 +60,7 @@ static constexpr size_t ControlValueSize[] = {\n> \t[ControlTypeString]\t\t= sizeof(char),\n> \t[ControlTypeRectangle]\t\t= sizeof(Rectangle),\n> \t[ControlTypeSize]\t\t= sizeof(Size),\n> +\t[ControlTypeMenu]\t\t= sizeof(ControlMenu),\n> };\n> \n> } /* namespace */\n> @@ -254,6 +255,12 @@ std::string ControlValue::toString() const\n> \t\t\tstr += value->toString();\n> \t\t\tbreak;\n> \t\t}\n> +\t\tcase ControlTypeMenu: {\n> +\t\t\tconst ControlMenu *value =\n> +\t\t\t\treinterpret_cast(data);\n> +\t\t\tstr += value->toString();\n> +\t\t\tbreak;\n> +\t\t}\n> \t\tcase ControlTypeNone:\n> \t\tcase ControlTypeString:\n> \t\t\tbreak;","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 E4D07BDCA5\n\tfor ;\n\tTue, 27 Apr 2021 03:31:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3532A68878;\n\tTue, 27 Apr 2021 05:31:23 +0200 (CEST)","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 7415660512\n\tfor ;\n\tTue, 27 Apr 2021 05:31:22 +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 E10E6E9;\n\tTue, 27 Apr 2021 05:31:21 +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=\"Zur+y3JZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619494282;\n\tbh=gvymEMIQVsSCl2ySt0cpcqwD0CVIPJMzXNesBtQPYz8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zur+y3JZJ1L4n2XzaxeJN/umg7rLlxIUN7AMslTaJX9Kb2TaM19uw1KO3n7XnsYZ4\n\tet5EAxOkuxcKf1SMPMS0FhK44fjPeu3MyJDdYV1U0L8A14ROxdvOPh1NtTwZ7ImoiR\n\tnP2USeTyxQK6RK9zU4WUPE77o827r6Ssigvv8FYE=","Date":"Tue, 27 Apr 2021 06:31:16 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210421042346.312854-1-hiroh@chromium.org>\n\t<20210421042346.312854-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421042346.312854-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","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":16604,"web_url":"https://patchwork.libcamera.org/comment/16604/","msgid":"","date":"2021-04-27T03:46:30","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 27, 2021 at 12:31 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 21, 2021 at 01:23:41PM +0900, Hirokazu Honda wrote:\n> > This adds control type for v4l2 menu.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++\n> > src/libcamera/controls.cpp | 7 +++++++\n> > 2 files changed, 35 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1a5690a5..357819e7 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -9,6 +9,7 @@\n> > #define __LIBCAMERA_CONTROLS_H__\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -32,6 +33,28 @@ enum ControlType {\n> > ControlTypeString,\n> > ControlTypeRectangle,\n> > ControlTypeSize,\n> > + ControlTypeMenu,\n> > +};\n> > +\n> > +struct ControlMenu {\n> > + uint32_t index;\n> > + bool isName = false;\n> > +\n> > + union {\n> > + char name[28];\n> > + int64_t value;\n> > + };\n> > +\n> > + std::string toString() const\n> > + {\n> > + std::stringstream ss;\n> > + ss << \"index: \" << index;\n> > + if (isName)\n> > + ss << \"name: \" << name;\n> > + else\n> > + ss << \"value: \" << value;\n> > + return ss.str();\n> > + }\n> > };\n>\n> As this is used for V4L2 controls only, I'm really not keen on adding\n> support for named menu entries :-( Especially given that 28 is a\n> completely arbitrary limit. I'd like to instead store the value as an\n> integer, and, if we really need support for named menu entries, to add\n> them to the ControlInfo class instead ? The mapping between numerical\n> values and names should be constant, it shouldn't be a property of the\n> ControlValue.\n>\n\nI see. Actually the named menu is necessary for test pattern mode.\nWe need a class for string. The problem is ControlValue uses memcpy\nfor copy, but it doesn't obviously work for std::string.\nCould you tell me your prefered solution?\nMy idea is only to replace memcpy by copy assignment operator.\n\n-Hiro\n> >\n> > namespace details {\n> > @@ -85,6 +108,11 @@ struct control_type {\n> > static constexpr ControlType value = ControlTypeSize;\n> > };\n> >\n> > +template<>\n> > +struct control_type {\n> > + static constexpr ControlType value = ControlTypeMenu;\n> > +};\n> > +\n> > template\n> > struct control_type> : public control_type> {\n> > };\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index c58ed394..8ab5ac92 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -60,6 +60,7 @@ static constexpr size_t ControlValueSize[] = {\n> > [ControlTypeString] = sizeof(char),\n> > [ControlTypeRectangle] = sizeof(Rectangle),\n> > [ControlTypeSize] = sizeof(Size),\n> > + [ControlTypeMenu] = sizeof(ControlMenu),\n> > };\n> >\n> > } /* namespace */\n> > @@ -254,6 +255,12 @@ std::string ControlValue::toString() const\n> > str += value->toString();\n> > break;\n> > }\n> > + case ControlTypeMenu: {\n> > + const ControlMenu *value =\n> > + reinterpret_cast(data);\n> > + str += value->toString();\n> > + break;\n> > + }\n> > case ControlTypeNone:\n> > case ControlTypeString:\n> > break;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 DB312BDCA6\n\tfor ;\n\tTue, 27 Apr 2021 03:46:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9745168878;\n\tTue, 27 Apr 2021 05:46:41 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8CE4760512\n\tfor ;\n\tTue, 27 Apr 2021 05:46:40 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id n2so87523665ejy.7\n\tfor ;\n\tMon, 26 Apr 2021 20:46:40 -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=\"ibkjnf/v\"; 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=s3Zl0aMR+mlzkM3oe/WC2nxUMO7dlbV5p0EwTocUR8Y=;\n\tb=ibkjnf/vHhjukb2+Vic3tE+kRJJo4f0f4kP0NAtQszzYQUKSpIWJz0N06KbCsb7C6p\n\t0cGsbw8WL0Hk7YAphsSR/42O0kLyXX5QkesDJjzACAx+bLL/8iNfH5opnmhQnN8YorWt\n\t1R2UI2IS0lj9yPHtwoOV0jt9K8sU35klB+L/E=","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=s3Zl0aMR+mlzkM3oe/WC2nxUMO7dlbV5p0EwTocUR8Y=;\n\tb=TgEZG3MXXgtWNMzRQbgenoaUpoEMg0rCMWFRnBcL4JjAxUydUe+n0NDwUWscshRd04\n\tJxUMlKVlX6gRVNo+HtonlnrAFfyDCpthPCc8k6TFyCiVUVAlMwyZGjabiyeG9uRu7dAp\n\tbCE7L4ZTAZz0I1zmTTCZsekjFB/uekWKeo+hmlmB3WDw1psdgvpQ/DsX3zrs9m2MEjBp\n\tcP+yBFaWbDoaSBZrSpYVaiV98RNpj/PyIqGYNzDaabP7UD1yArBLOebLdMzP5t2YwdwR\n\t6hn7vq4TyUXYbZv1gXdDiJondro30LqGScmrWlcy1F1ERn4Pl9Vc0VhdtC6D6jRzra6p\n\tGhxQ==","X-Gm-Message-State":"AOAM5335wsqCEOm9Gyg7qj28clJDS2nlW7UTjCiBoYCbZ4UP/uwyYfV1\n\tpPa2HDISwT8CKq2dI3IJ7uu/T9+eoT5h9uE0FhLSzcbGPF4=","X-Google-Smtp-Source":"ABdhPJzaUF6Hca2PiO3bxaDd38v/pVkXceJZhFV49XSU6JargYJbi+0gYIVJZacb1eW2bcFVvpu23ZzvWffSIJEc0xE=","X-Received":"by 2002:a17:906:5acd:: with SMTP id\n\tx13mr962348ejs.243.1619495200308; \n\tMon, 26 Apr 2021 20:46:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421042346.312854-1-hiroh@chromium.org>\n\t<20210421042346.312854-3-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 27 Apr 2021 12:46:30 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16605,"web_url":"https://patchwork.libcamera.org/comment/16605/","msgid":"","date":"2021-04-27T03:56:17","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Apr 27, 2021 at 12:46:30PM +0900, Hirokazu Honda wrote:\n> On Tue, Apr 27, 2021 at 12:31 PM Laurent Pinchart wrote:\n> > On Wed, Apr 21, 2021 at 01:23:41PM +0900, Hirokazu Honda wrote:\n> > > This adds control type for v4l2 menu.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++\n> > > src/libcamera/controls.cpp | 7 +++++++\n> > > 2 files changed, 35 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 1a5690a5..357819e7 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -9,6 +9,7 @@\n> > > #define __LIBCAMERA_CONTROLS_H__\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -32,6 +33,28 @@ enum ControlType {\n> > > ControlTypeString,\n> > > ControlTypeRectangle,\n> > > ControlTypeSize,\n> > > + ControlTypeMenu,\n> > > +};\n> > > +\n> > > +struct ControlMenu {\n> > > + uint32_t index;\n> > > + bool isName = false;\n> > > +\n> > > + union {\n> > > + char name[28];\n> > > + int64_t value;\n> > > + };\n> > > +\n> > > + std::string toString() const\n> > > + {\n> > > + std::stringstream ss;\n> > > + ss << \"index: \" << index;\n> > > + if (isName)\n> > > + ss << \"name: \" << name;\n> > > + else\n> > > + ss << \"value: \" << value;\n> > > + return ss.str();\n> > > + }\n> > > };\n> >\n> > As this is used for V4L2 controls only, I'm really not keen on adding\n> > support for named menu entries :-( Especially given that 28 is a\n> > completely arbitrary limit. I'd like to instead store the value as an\n> > integer, and, if we really need support for named menu entries, to add\n> > them to the ControlInfo class instead ? The mapping between numerical\n> > values and names should be constant, it shouldn't be a property of the\n> > ControlValue.\n> \n> I see. Actually the named menu is necessary for test pattern mode.\n> We need a class for string. The problem is ControlValue uses memcpy\n\nWhy do we need strings, why can't we use the numerical values ?\n\n> for copy, but it doesn't obviously work for std::string.\n> Could you tell me your prefered solution?\n> My idea is only to replace memcpy by copy assignment operator.\n\nI don't know, I likely won't have time to try and design a solution for\nthis in the near future, all I know is that we should do better :-)\n\n> > > namespace details {\n> > > @@ -85,6 +108,11 @@ struct control_type {\n> > > static constexpr ControlType value = ControlTypeSize;\n> > > };\n> > >\n> > > +template<>\n> > > +struct control_type {\n> > > + static constexpr ControlType value = ControlTypeMenu;\n> > > +};\n> > > +\n> > > template\n> > > struct control_type> : public control_type> {\n> > > };\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index c58ed394..8ab5ac92 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -60,6 +60,7 @@ static constexpr size_t ControlValueSize[] = {\n> > > [ControlTypeString] = sizeof(char),\n> > > [ControlTypeRectangle] = sizeof(Rectangle),\n> > > [ControlTypeSize] = sizeof(Size),\n> > > + [ControlTypeMenu] = sizeof(ControlMenu),\n> > > };\n> > >\n> > > } /* namespace */\n> > > @@ -254,6 +255,12 @@ std::string ControlValue::toString() const\n> > > str += value->toString();\n> > > break;\n> > > }\n> > > + case ControlTypeMenu: {\n> > > + const ControlMenu *value =\n> > > + reinterpret_cast(data);\n> > > + str += value->toString();\n> > > + break;\n> > > + }\n> > > case ControlTypeNone:\n> > > case ControlTypeString:\n> > > break;","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 7AD80BDCA1\n\tfor ;\n\tTue, 27 Apr 2021 03:56:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B46AA68880;\n\tTue, 27 Apr 2021 05:56:25 +0200 (CEST)","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 C3AD760512\n\tfor ;\n\tTue, 27 Apr 2021 05:56:23 +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 38194E9;\n\tTue, 27 Apr 2021 05:56:23 +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=\"WpxUKHOI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619495783;\n\tbh=2shkaYVzSw8MzeOr9Qk1w1QGV14v85VwJPo4w/VA1tM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WpxUKHOI2b1rXzmeQKfR6AhwOjAG/tedN0RqNfQtPFERZp3KT7NW9mA3QVyYGhC3R\n\tyzlbsnjSirZ1d/y2yiTLyY6EEbWF5vq0UxXjunJxWMBXZ4hccgTggP40ktxF7W0NxM\n\tEJZoH2r9kk1B/UNqp6Q9RlVWLQxNY/XT/yVuRghs=","Date":"Tue, 27 Apr 2021 06:56:17 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210421042346.312854-1-hiroh@chromium.org>\n\t<20210421042346.312854-3-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16611,"web_url":"https://patchwork.libcamera.org/comment/16611/","msgid":"","date":"2021-04-27T04:13:48","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Apr 27, 2021 at 12:56 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Apr 27, 2021 at 12:46:30PM +0900, Hirokazu Honda wrote:\n> > On Tue, Apr 27, 2021 at 12:31 PM Laurent Pinchart wrote:\n> > > On Wed, Apr 21, 2021 at 01:23:41PM +0900, Hirokazu Honda wrote:\n> > > > This adds control type for v4l2 menu.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++\n> > > > src/libcamera/controls.cpp | 7 +++++++\n> > > > 2 files changed, 35 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > index 1a5690a5..357819e7 100644\n> > > > --- a/include/libcamera/controls.h\n> > > > +++ b/include/libcamera/controls.h\n> > > > @@ -9,6 +9,7 @@\n> > > > #define __LIBCAMERA_CONTROLS_H__\n> > > >\n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -32,6 +33,28 @@ enum ControlType {\n> > > > ControlTypeString,\n> > > > ControlTypeRectangle,\n> > > > ControlTypeSize,\n> > > > + ControlTypeMenu,\n> > > > +};\n> > > > +\n> > > > +struct ControlMenu {\n> > > > + uint32_t index;\n> > > > + bool isName = false;\n> > > > +\n> > > > + union {\n> > > > + char name[28];\n> > > > + int64_t value;\n> > > > + };\n> > > > +\n> > > > + std::string toString() const\n> > > > + {\n> > > > + std::stringstream ss;\n> > > > + ss << \"index: \" << index;\n> > > > + if (isName)\n> > > > + ss << \"name: \" << name;\n> > > > + else\n> > > > + ss << \"value: \" << value;\n> > > > + return ss.str();\n> > > > + }\n> > > > };\n> > >\n> > > As this is used for V4L2 controls only, I'm really not keen on adding\n> > > support for named menu entries :-( Especially given that 28 is a\n> > > completely arbitrary limit. I'd like to instead store the value as an\n> > > integer, and, if we really need support for named menu entries, to add\n> > > them to the ControlInfo class instead ? The mapping between numerical\n> > > values and names should be constant, it shouldn't be a property of the\n> > > ControlValue.\n> >\n> > I see. Actually the named menu is necessary for test pattern mode.\n> > We need a class for string. The problem is ControlValue uses memcpy\n>\n> Why do we need strings, why can't we use the numerical values ?\n>\n\nI misunderstood you don't like to have the 28 length limitation.\nBy the way, for v4l2 menu, we need to store index and either string or integer.\nAre you okay to introduce two new control types, std::pair and std::pair?\n\n-Hiro\n\n> > for copy, but it doesn't obviously work for std::string.\n> > Could you tell me your prefered solution?\n> > My idea is only to replace memcpy by copy assignment operator.\n>\n> I don't know, I likely won't have time to try and design a solution for\n> this in the near future, all I know is that we should do better :-)\n>\n> > > > namespace details {\n> > > > @@ -85,6 +108,11 @@ struct control_type {\n> > > > static constexpr ControlType value = ControlTypeSize;\n> > > > };\n> > > >\n> > > > +template<>\n> > > > +struct control_type {\n> > > > + static constexpr ControlType value = ControlTypeMenu;\n> > > > +};\n> > > > +\n> > > > template\n> > > > struct control_type> : public control_type> {\n> > > > };\n> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > index c58ed394..8ab5ac92 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -60,6 +60,7 @@ static constexpr size_t ControlValueSize[] = {\n> > > > [ControlTypeString] = sizeof(char),\n> > > > [ControlTypeRectangle] = sizeof(Rectangle),\n> > > > [ControlTypeSize] = sizeof(Size),\n> > > > + [ControlTypeMenu] = sizeof(ControlMenu),\n> > > > };\n> > > >\n> > > > } /* namespace */\n> > > > @@ -254,6 +255,12 @@ std::string ControlValue::toString() const\n> > > > str += value->toString();\n> > > > break;\n> > > > }\n> > > > + case ControlTypeMenu: {\n> > > > + const ControlMenu *value =\n> > > > + reinterpret_cast(data);\n> > > > + str += value->toString();\n> > > > + break;\n> > > > + }\n> > > > case ControlTypeNone:\n> > > > case ControlTypeString:\n> > > > break;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 8AB67BDCA1\n\tfor ;\n\tTue, 27 Apr 2021 04:13:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47A5868880;\n\tTue, 27 Apr 2021 06:13:59 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 740CA60512\n\tfor ;\n\tTue, 27 Apr 2021 06:13:58 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id h10so68279593edt.13\n\tfor ;\n\tMon, 26 Apr 2021 21:13:58 -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=\"BNywhvfq\"; 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=TunrUU7l1FBEUBiXlXMbdcQNXv+A44QZtPT/rq3/nkk=;\n\tb=BNywhvfq52Z5j5gsprosgdmCIDmtCPogENUphC1ARK+08bDCaRPLYncv1HSL1xdwH9\n\tdhga+Y0vOYYH4LxVkJpby7zoLdM4wm5oU5923GMCgwb1KcUvubmtiAypS+1kGsucQpSj\n\tomKe/0FnDfD1pdkFJI9ylIzFvqS3AHjRBqI7s=","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=TunrUU7l1FBEUBiXlXMbdcQNXv+A44QZtPT/rq3/nkk=;\n\tb=iA8kYHzOEdWhG1p334cjgKHSgjZ/tR2obyDHpdqcARodtmjwQLF/pmaP2nH6Ya0N/I\n\toK1HoDP+SpUN5gu6wGKrKkqkSpMBqzII5nxCqT5x5Jjt1AdhksfXWwaQ6bYMsRzurKDP\n\txwCaNxyiC8RPht/K9jZ66JX46KZJk1AA3f/AqWFq5VqNNTObNLeB+bhroaR0DSHxMESy\n\tb1hepfSIBAfKUeDun8+LY+Fj6xixZSLGZX7LgZUofICDoh2HQ/l1dvNQwzuz7QbTnQc6\n\tQ7uHBz0E1MRoaO6Y0PExvTEcw8VEwmVkF7HgYbh5Pm1JFLwsX2cmxSqwDfzJZSI2XHcj\n\tlbaQ==","X-Gm-Message-State":"AOAM532aRxgHbBDChfWjNFJ3jVnPlIiZmKb576RFeC3rDwGr2JqqgHod\n\tphIjSmPY+9DVAECnyOhJYiwDIzazzFEPye+Xxnnt8jAZnnQ=","X-Google-Smtp-Source":"ABdhPJwvcaQ8Djj/yvKKJ2E90Alv+sn7DRIeBmvLPLuXNxG/7VahpWc4mtnD71zFFmb9agedQTYFq8OEPVhFj4gYIoE=","X-Received":"by 2002:a05:6402:488:: with SMTP id\n\tk8mr2000374edv.233.1619496838225; \n\tMon, 26 Apr 2021 21:13:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421042346.312854-1-hiroh@chromium.org>\n\t<20210421042346.312854-3-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 27 Apr 2021 13:13:48 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16618,"web_url":"https://patchwork.libcamera.org/comment/16618/","msgid":"","date":"2021-04-27T05:06:28","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Apr 27, 2021 at 01:13:48PM +0900, Hirokazu Honda wrote:\n> On Tue, Apr 27, 2021 at 12:56 PM Laurent Pinchart wrote:\n> > On Tue, Apr 27, 2021 at 12:46:30PM +0900, Hirokazu Honda wrote:\n> > > On Tue, Apr 27, 2021 at 12:31 PM Laurent Pinchart wrote:\n> > > > On Wed, Apr 21, 2021 at 01:23:41PM +0900, Hirokazu Honda wrote:\n> > > > > This adds control type for v4l2 menu.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > include/libcamera/controls.h | 28 ++++++++++++++++++++++++++++\n> > > > > src/libcamera/controls.cpp | 7 +++++++\n> > > > > 2 files changed, 35 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > > index 1a5690a5..357819e7 100644\n> > > > > --- a/include/libcamera/controls.h\n> > > > > +++ b/include/libcamera/controls.h\n> > > > > @@ -9,6 +9,7 @@\n> > > > > #define __LIBCAMERA_CONTROLS_H__\n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -32,6 +33,28 @@ enum ControlType {\n> > > > > ControlTypeString,\n> > > > > ControlTypeRectangle,\n> > > > > ControlTypeSize,\n> > > > > + ControlTypeMenu,\n> > > > > +};\n> > > > > +\n> > > > > +struct ControlMenu {\n> > > > > + uint32_t index;\n> > > > > + bool isName = false;\n> > > > > +\n> > > > > + union {\n> > > > > + char name[28];\n> > > > > + int64_t value;\n> > > > > + };\n> > > > > +\n> > > > > + std::string toString() const\n> > > > > + {\n> > > > > + std::stringstream ss;\n> > > > > + ss << \"index: \" << index;\n> > > > > + if (isName)\n> > > > > + ss << \"name: \" << name;\n> > > > > + else\n> > > > > + ss << \"value: \" << value;\n> > > > > + return ss.str();\n> > > > > + }\n> > > > > };\n> > > >\n> > > > As this is used for V4L2 controls only, I'm really not keen on adding\n> > > > support for named menu entries :-( Especially given that 28 is a\n> > > > completely arbitrary limit. I'd like to instead store the value as an\n> > > > integer, and, if we really need support for named menu entries, to add\n> > > > them to the ControlInfo class instead ? The mapping between numerical\n> > > > values and names should be constant, it shouldn't be a property of the\n> > > > ControlValue.\n> > >\n> > > I see. Actually the named menu is necessary for test pattern mode.\n> > > We need a class for string. The problem is ControlValue uses memcpy\n> >\n> > Why do we need strings, why can't we use the numerical values ?\n> \n> I misunderstood you don't like to have the 28 length limitation.\n> By the way, for v4l2 menu, we need to store index and either string or integer.\n> Are you okay to introduce two new control types, std::pair uint8_t[28]> and std::pair?\n\nIn ControlValue I would like to store the index only. If we need to\nstore strings in ControlInfo, I think we'll need to extend the\nControlInfo class to store other information than just a ControlValue. I\nwant to avoid as much as possible extending the types of ControlValue we\nsupport, as you would otherwise need to plumb them through the IPC\nserialization code too. ControlValue is designed as the container to be\nstored in ControlList. It's reused in ControlInfo, but that's not what\nit's optimized for, it's main target is ControlList.\n\n> > > for copy, but it doesn't obviously work for std::string.\n> > > Could you tell me your prefered solution?\n> > > My idea is only to replace memcpy by copy assignment operator.\n> >\n> > I don't know, I likely won't have time to try and design a solution for\n> > this in the near future, all I know is that we should do better :-)\n> >\n> > > > > namespace details {\n> > > > > @@ -85,6 +108,11 @@ struct control_type {\n> > > > > static constexpr ControlType value = ControlTypeSize;\n> > > > > };\n> > > > >\n> > > > > +template<>\n> > > > > +struct control_type {\n> > > > > + static constexpr ControlType value = ControlTypeMenu;\n> > > > > +};\n> > > > > +\n> > > > > template\n> > > > > struct control_type> : public control_type> {\n> > > > > };\n> > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > > index c58ed394..8ab5ac92 100644\n> > > > > --- a/src/libcamera/controls.cpp\n> > > > > +++ b/src/libcamera/controls.cpp\n> > > > > @@ -60,6 +60,7 @@ static constexpr size_t ControlValueSize[] = {\n> > > > > [ControlTypeString] = sizeof(char),\n> > > > > [ControlTypeRectangle] = sizeof(Rectangle),\n> > > > > [ControlTypeSize] = sizeof(Size),\n> > > > > + [ControlTypeMenu] = sizeof(ControlMenu),\n> > > > > };\n> > > > >\n> > > > > } /* namespace */\n> > > > > @@ -254,6 +255,12 @@ std::string ControlValue::toString() const\n> > > > > str += value->toString();\n> > > > > break;\n> > > > > }\n> > > > > + case ControlTypeMenu: {\n> > > > > + const ControlMenu *value =\n> > > > > + reinterpret_cast(data);\n> > > > > + str += value->toString();\n> > > > > + break;\n> > > > > + }\n> > > > > case ControlTypeNone:\n> > > > > case ControlTypeString:\n> > > > > break;","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 E9D22BDCC3\n\tfor ;\n\tTue, 27 Apr 2021 05:06:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69AE868883;\n\tTue, 27 Apr 2021 07:06:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EEB760512\n\tfor ;\n\tTue, 27 Apr 2021 07:06:35 +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 9D4C2E9;\n\tTue, 27 Apr 2021 07:06:34 +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=\"RfTgPtsE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619499994;\n\tbh=t15dp2ZqstT5/Xk3t7aC0OLgHMR+93bCwFrFjOluI9A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RfTgPtsE+s18c9S45GUPAl8c/qYlHhVPoLkZasrVDJEoNLCd+s2zEq2KeZ9H5/ls3\n\tMlNym/d+ue5TjrSyJK0RCTs3uC+FQt1OeM/X8JQcl8L7WiLmZxrDAysPya1Hi4b7WS\n\tLb7CUUHeLto/JG/It49jXLvVXnNolSeb5hYsyi28=","Date":"Tue, 27 Apr 2021 08:06:28 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210421042346.312854-1-hiroh@chromium.org>\n\t<20210421042346.312854-3-hiroh@chromium.org>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: Controls: Add\n\tControlTypeMenu","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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]