[{"id":4970,"web_url":"https://patchwork.libcamera.org/comment/4970/","msgid":"<20200602152014.GA94018@oden.dyn.berto.se>","date":"2020-06-02T15:20:14","subject":"Re: [libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kaaira,\n\nThanks for your patch.\n\nOn 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote:\n> Replace hex input for pixelformats with their fourcc values,\n> in cam and qcam.\n> \n> Signed-off-by: Kaaira Gupta \n> ---\n> src/cam/stream_options.cpp | 13 ++++++++++---\n> 1 file changed, 10 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> index bd12c8f..9f9536e 100644\n> --- a/src/cam/stream_options.cpp\n> +++ b/src/cam/stream_options.cpp\n> @@ -6,6 +6,7 @@\n> */\n> #include \"stream_options.h\"\n> \n> +#include \n> #include \n> \n> using namespace libcamera;\n> @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser()\n> \t\t ArgumentRequired);\n> \taddOption(\"height\", OptionInteger, \"Height in pixels\",\n> \t\t ArgumentRequired);\n> -\taddOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> +\taddOption(\"pixelformat\", OptionString, \"Pixel format fourcc\",\n> \t\t ArgumentRequired);\n> }\n> \n> @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> \t\t}\n> \n> \t\t/* \\todo Translate 4CC string to pixelformat with modifier. */\n> -\t\tif (opts.isSet(\"pixelformat\"))\n> -\t\t\tcfg.pixelFormat = PixelFormat(opts[\"pixelformat\"]);\n> +\t\tif (opts.isSet(\"pixelformat\")) {\n> +\t\t\tstd::string fourcc = opts[\"pixelformat\"];\n> +\t\t\ttransform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper);\n\nI fear we don't know all fourcc codes will be uppercase. It's currently \ntrue all DRM defined ones are, but might change. As an example look at \nV4L2 fourcc codes where lower/upper case is already mixed.\n\n> +\t\t\tchar char_array[5];\n> +\t\t\tstrcpy(char_array, fourcc.c_str());\n\nI think you can simplify this, or something similar (not compile \ntested).\n\n const char *fourcc = opts[\"pixelformat\"].toString().c_str();\n\n> +\t\t\tcfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) |\n> +\t\t\t\t\t\t ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24));\n> +\t\t}\n> \t}\n> \n> \treturn 0;\n> -- \n> 2.17.1\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 mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81D2861012\n\tfor ;\n\tTue, 2 Jun 2020 17:20:16 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id n24so2524012lji.10\n\tfor ;\n\tTue, 02 Jun 2020 08:20:16 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tw144sm762745lff.67.2020.06.02.08.20.14\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Jun 2020 08:20:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"mJ2iMVqp\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=Iz8JRYDzz64ks9ExRXRR1uazgcWwKexgYHDEjDm+Ft4=;\n\tb=mJ2iMVqp7jPvImmxh0tlfQtakKcx0QEmkPOu8UxusIXD2+goslL83yYKVIEIDMQ15S\n\txd+tcRZAkEhyr3qiKwolHE1anH6pWgHqqRHLwpBAU4jbFJMRZKWMgfJBjpTbxL3Fxu7f\n\ta1DXKlg6RPfLeTLf/vL9iqiy8RPKKfbbTH8tEpDNmrDtlQ1uOeoLM7a6u2dbbHHHi1jG\n\tZF205m9SP1moE6ljhH61k1s21WBGnkWxiiREHe2hRCz+3WefkhgoE9bDY1gbIL9kqW6F\n\tkQ/ni5OHYAzMhoYzM0ryBT/CS4FWyPh4Bm1Czj5fFUKwc2DkQzke2SVJGu2WLQprInh2\n\th2mw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=Iz8JRYDzz64ks9ExRXRR1uazgcWwKexgYHDEjDm+Ft4=;\n\tb=c9QBpV8ryfGqnQ+CFc3jUgWKzEgSBqXNsYMkIreo4UdDt7TqUgqIeZIARztbhi0H+C\n\t8yga9GuRWXmBnhqO+m4KC4rk519XQQWOKYtjQ4Cu6Of2DSHYtUme5idTRTDI7wV0p64k\n\tnUb5NcakJxEzGYHgpAjB5NYbK4mXUppQAAJWlxH3ixe4WQbkHIyL1qcQtn3GcwHWthIg\n\tHMabJZOJJvQFtEQFqG0tfgEaXSLg9XNRKwQK+gJQG0umNznEs/D/26HZFK8rVL64wRuF\n\tf7wxSD1NDmKE3XPG0cUWSslnch9BZZhLVS1nH7SPbr7xd6Qirs69BdINIp5/qccViftW\n\tv/ig==","X-Gm-Message-State":"AOAM5328F7umSxtqYZV90cCxo3+mI/7FEfaiVsFDLS0xVbdbJBb4I6+i\n\tiR6uaoyTI673R5xJJV5XlJ4rTg==","X-Google-Smtp-Source":"ABdhPJwFpZMRA9kBS+/ZMFOEUwkgCiAchYzCiBb7SFK6ecNHI0QLjYvDw+90peiH7bSKsq4WU4IkAg==","X-Received":"by 2002:a2e:9141:: with SMTP id q1mr5794092ljg.196.1591111215677;\n\tTue, 02 Jun 2020 08:20:15 -0700 (PDT)","Date":"Tue, 2 Jun 2020 17:20:14 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kaaira Gupta ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Message-ID":"<20200602152014.GA94018@oden.dyn.berto.se>","References":"<20200529140433.GA18070@kaaira-HP-Pavilion-Notebook>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200529140433.GA18070@kaaira-HP-Pavilion-Notebook>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","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":"Tue, 02 Jun 2020 15:20:16 -0000"}},{"id":4973,"web_url":"https://patchwork.libcamera.org/comment/4973/","msgid":"<20200602203309.GB6547@pendragon.ideasonboard.com>","date":"2020-06-02T20:33:09","subject":"Re: [libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Jun 02, 2020 at 05:20:14PM +0200, Niklas Söderlund wrote:\n> On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote:\n> > Replace hex input for pixelformats with their fourcc values,\n> > in cam and qcam.\n> > \n> > Signed-off-by: Kaaira Gupta \n> > ---\n> > src/cam/stream_options.cpp | 13 ++++++++++---\n> > 1 file changed, 10 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> > index bd12c8f..9f9536e 100644\n> > --- a/src/cam/stream_options.cpp\n> > +++ b/src/cam/stream_options.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> > #include \"stream_options.h\"\n> > \n> > +#include \n\nWhat is this header used for ? It doesn't seem to be standard.\n\n> > #include \n> > \n> > using namespace libcamera;\n> > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser()\n> > \t\t ArgumentRequired);\n> > \taddOption(\"height\", OptionInteger, \"Height in pixels\",\n> > \t\t ArgumentRequired);\n> > -\taddOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> > +\taddOption(\"pixelformat\", OptionString, \"Pixel format fourcc\",\n\ns/fourcc/FourCC/\n\n> > \t\t ArgumentRequired);\n> > }\n> > \n> > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> > \t\t}\n> > \n> > \t\t/* \\todo Translate 4CC string to pixelformat with modifier. */\n> > -\t\tif (opts.isSet(\"pixelformat\"))\n> > -\t\t\tcfg.pixelFormat = PixelFormat(opts[\"pixelformat\"]);\n> > +\t\tif (opts.isSet(\"pixelformat\")) {\n> > +\t\t\tstd::string fourcc = opts[\"pixelformat\"];\n> > +\t\t\ttransform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper);\n> \n> I fear we don't know all fourcc codes will be uppercase. It's currently \n> true all DRM defined ones are, but might change. As an example look at \n> V4L2 fourcc codes where lower/upper case is already mixed.\n> \n> > +\t\t\tchar char_array[5];\n> > +\t\t\tstrcpy(char_array, fourcc.c_str());\n> \n> I think you can simplify this, or something similar (not compile \n> tested).\n> \n> const char *fourcc = opts[\"pixelformat\"].toString().c_str();\n\nOptionValue::toString() returns a temporary object, you'll have a\nuse-after-free.\n\n> \n> > +\t\t\tcfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) |\n> > +\t\t\t\t\t\t ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24));\n\nstatic_cast() instead of (__u32), to use C++-style casts, and\nbecause __u32 is a kernel type. Let's also split this to shorten the\nlines.\n\nThis change goes in the right direction, but I think we need to expand\nit a little bit:\n\n- Some FourCC contain spaces at the end. It would be nice to write\n\n\t-s pixelformat=R8\n\n instead of\n\n\t-s 'pixelformat=R8 '\n\n- Should we support both the numerical value and the 4CC representation\n ? It could be useful in some cases, to avoid having to convert\n manually from hex to 4CC first, but maybe there are only few use cases\n for that feature.\n\n- With a 4CC we can't easily handle the formats that use\n DRM_FORMAT_BIG_ENDIAN. Maybe that's no big deal though, as we don't\n any use such format at the moment, and DRM_FORMAT_BIG_ENDIAN seems\n more like a hack than something that should be used going forward.\n\nNot all this need to be addressed now.\n\n> > +\t\t}\n> > \t}\n> > \n> > \treturn 0;","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 53336603D0\n\tfor ;\n\tTue, 2 Jun 2020 22:33:25 +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 AADC32A4;\n\tTue, 2 Jun 2020 22:33:24 +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=\"gSH0K4ri\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591130004;\n\tbh=AJGV4LUWixGAWLrxP/pimKFyyDg8kHOfKBUojw7kH+8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gSH0K4riIhbRtB6fdsPkYaSwrj8KMUJC9uQCPY5pgxIOOggbr/P2Adv4DG0nZ/a10\n\tv63rm2ddltEIkkSs7fbNcNfxJb6ONVYTgKCs5prMJ5dOlJcCuMHrHQj2ZSCNqlya03\n\tk51UH19LeO4mFDw1D6zDh5ZGfPgtqqwyvNcPjBls=","Date":"Tue, 2 Jun 2020 23:33:09 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Kaaira Gupta , libcamera-devel@lists.libcamera.org","Message-ID":"<20200602203309.GB6547@pendragon.ideasonboard.com>","References":"<20200529140433.GA18070@kaaira-HP-Pavilion-Notebook>\n\t<20200602152014.GA94018@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200602152014.GA94018@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","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":"Tue, 02 Jun 2020 20:33:25 -0000"}},{"id":4982,"web_url":"https://patchwork.libcamera.org/comment/4982/","msgid":"<20200603125023.GA4967@kaaira-HP-Pavilion-Notebook>","date":"2020-06-03T12:50:23","subject":"[libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","submitter":{"id":39,"url":"https://patchwork.libcamera.org/api/people/39/","name":"Kaaira Gupta","email":"kgupta@es.iitr.ac.in"},"content":"On Tue, Jun 02, 2020 at 11:33:09PM +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Tue, Jun 02, 2020 at 05:20:14PM +0200, Niklas Söderlund wrote:\n> > On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote:\n> > > Replace hex input for pixelformats with their fourcc values,\n> > > in cam and qcam.\n> > > \n> > > Signed-off-by: Kaaira Gupta \n> > > ---\n> > > src/cam/stream_options.cpp | 13 ++++++++++---\n> > > 1 file changed, 10 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> > > index bd12c8f..9f9536e 100644\n> > > --- a/src/cam/stream_options.cpp\n> > > +++ b/src/cam/stream_options.cpp\n> > > @@ -6,6 +6,7 @@\n> > > */\n> > > #include \"stream_options.h\"\n> > > \n> > > +#include \n> \n> What is this header used for ? It doesn't seem to be standard.\n\nit is used for \"toupper\" functionality, I added it so that we can write,\nfor example, ba24 instead of BA24..but as pointed out DRM formats can\nhave a mixture of both upper and lower cases in future, so I'll remove\nthat.\n\n> \n> > > #include \n> > > \n> > > using namespace libcamera;\n> > > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser()\n> > > \t\t ArgumentRequired);\n> > > \taddOption(\"height\", OptionInteger, \"Height in pixels\",\n> > > \t\t ArgumentRequired);\n> > > -\taddOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> > > +\taddOption(\"pixelformat\", OptionString, \"Pixel format fourcc\",\n> \n> s/fourcc/FourCC/\n> \n> > > \t\t ArgumentRequired);\n> > > }\n> > > \n> > > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> > > \t\t}\n> > > \n> > > \t\t/* \\todo Translate 4CC string to pixelformat with modifier. */\n> > > -\t\tif (opts.isSet(\"pixelformat\"))\n> > > -\t\t\tcfg.pixelFormat = PixelFormat(opts[\"pixelformat\"]);\n> > > +\t\tif (opts.isSet(\"pixelformat\")) {\n> > > +\t\t\tstd::string fourcc = opts[\"pixelformat\"];\n> > > +\t\t\ttransform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper);\n> > \n> > I fear we don't know all fourcc codes will be uppercase. It's currently \n> > true all DRM defined ones are, but might change. As an example look at \n> > V4L2 fourcc codes where lower/upper case is already mixed.\n> > \n> > > +\t\t\tchar char_array[5];\n> > > +\t\t\tstrcpy(char_array, fourcc.c_str());\n> > \n> > I think you can simplify this, or something similar (not compile \n> > tested).\n> > \n> > const char *fourcc = opts[\"pixelformat\"].toString().c_str();\n> \n> OptionValue::toString() returns a temporary object, you'll have a\n> use-after-free.\n> \n> > \n> > > +\t\t\tcfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) |\n> > > +\t\t\t\t\t\t ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24));\n> \n> static_cast() instead of (__u32), to use C++-style casts, and\n> because __u32 is a kernel type. Let's also split this to shorten the\n> lines.\n> \n> This change goes in the right direction, but I think we need to expand\n> it a little bit:\n> \n> - Some FourCC contain spaces at the end. It would be nice to write\n> \n> \t-s pixelformat=R8\n> \n> instead of\n> \n> \t-s 'pixelformat=R8 '\n\nNoted\n\n> \n> - Should we support both the numerical value and the 4CC representation\n> ? It could be useful in some cases, to avoid having to convert\n> manually from hex to 4CC first, but maybe there are only few use cases\n> for that feature.\n\nWhat are the use cases for direct hex values? I think FourCC are easier\nto remember?\n\n> \n> - With a 4CC we can't easily handle the formats that use\n> DRM_FORMAT_BIG_ENDIAN. Maybe that's no big deal though, as we don't\n> any use such format at the moment, and DRM_FORMAT_BIG_ENDIAN seems\n> more like a hack than something that should be used going forward.\n> \n> Not all this need to be addressed now.\n> \n> > > +\t\t}\n> > > \t}\n> > > \n> > > \treturn 0;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-pg1-x532.google.com (mail-pg1-x532.google.com\n\t[IPv6:2607:f8b0:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C5F461027\n\tfor ;\n\tWed, 3 Jun 2020 14:50:32 +0200 (CEST)","by mail-pg1-x532.google.com with SMTP id s10so1736419pgm.0\n\tfor ;\n\tWed, 03 Jun 2020 05:50:32 -0700 (PDT)","from kaaira-HP-Pavilion-Notebook ([103.113.213.174])\n\tby smtp.gmail.com with ESMTPSA id\n\td8sm1683532pgb.42.2020.06.03.05.50.27\n\t(version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 03 Jun 2020 05:50:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=es-iitr-ac-in.20150623.gappssmtp.com\n\theader.i=@es-iitr-ac-in.20150623.gappssmtp.com header.b=\"MipCnlun\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=es-iitr-ac-in.20150623.gappssmtp.com; s=20150623;\n\th=from:date:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=aMKzAw9rVrNptxcaUqOGFinaIb7hytj26feZLK1zmBg=;\n\tb=MipCnluneure6qAwoyhUveLo5hqsb2ObgKWBCZ/SSepEy4LB6QDG2B03YcY4IC+8Ov\n\to6lfnb3dRtStn2GipvKL1mci3Y331nfOaL2dPw/kBv/T4G8l3bgHiknidEkil3kssfDH\n\t97fL6d3R0spMhZdCO3wVbZnZJX34wxjk0iTN9XfXuTnbjlsvPXtkw0RRZFskf+36N2+f\n\tv+t7HTagYuNyKtDGqWVtL27n98yvLdiWIcpKSHvsB9CQ9EgW13HoWMCNCcbKtKN8Bs/m\n\tOj3CY6hTZq8vl+32ftQXXCdew/8JQknyiBbJH67eGXxVgEjKwfU34S1gVp288U48vjGc\n\tn07w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:date:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=aMKzAw9rVrNptxcaUqOGFinaIb7hytj26feZLK1zmBg=;\n\tb=ApMIZ7XhpGYSfXGsQrR+K3YU9RP+7yeB4Jl7G0UCu85zFipFW0t2/7cJ6B5aO9aoPK\n\thfNZUtn2SSfvb6t2IyY+DMnq7SluYFEGQyt0kMDm6gz1WVGs1xLM6JjSUFSNZv5PV9K2\n\tL6oxoulIZVxWXN6AjxfplfebT5uHQPXL59S9W0YY26Ze4sFLnPW1NLxYsgcX04e2Dzmw\n\tzlUe6a/PcbR6WqzvB2tEC2SgRz3SeHbaSLu/iLO0pfroWiKSigJilLz6MySaus3M2Y6Y\n\tEQ//352vvTE1XG/lv5twe7hVRl2dCDSu9pIWzpwMW5LqgQaQNPfDgQ0YCo3bjjM1SpkM\n\t/HPA==","X-Gm-Message-State":"AOAM530Rra+6d9TBEsih92Ho1/sxjf7kHabDCHpLbugPVNxCi7eDeQOK\n\tqdNOvJIxLiDS0AnE3N0JrpyGVYMxAImPmg==","X-Google-Smtp-Source":"ABdhPJzEo/4XTMOWbp8Y9nmWtxpsm0i5LG/3duCuGVxR8hiu3OoLl+R2jeUcMudKqYNt+yEonr6zKQ==","X-Received":"by 2002:aa7:93b4:: with SMTP id x20mr15285904pff.9.1591188630333;\n\tWed, 03 Jun 2020 05:50:30 -0700 (PDT)","From":"Kaaira Gupta ","X-Google-Original-From":"Kaaira Gupta ","Date":"Wed, 3 Jun 2020 18:20:23 +0530","To":"Laurent Pinchart ","Cc":"Niklas =?iso-8859-1?q?S=F6derlund?= ,\n\tKaaira Gupta , libcamera-devel@lists.libcamera.org","Message-ID":"<20200603125023.GA4967@kaaira-HP-Pavilion-Notebook>","References":"<20200529140433.GA18070@kaaira-HP-Pavilion-Notebook>\n\t<20200602152014.GA94018@oden.dyn.berto.se>\n\t<20200602203309.GB6547@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200602203309.GB6547@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.9.4 (2018-02-28)","Subject":"[libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","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, 03 Jun 2020 12:50:32 -0000"}},{"id":5396,"web_url":"https://patchwork.libcamera.org/comment/5396/","msgid":"<20200625034219.GH5980@pendragon.ideasonboard.com>","date":"2020-06-25T03:42:19","subject":"Re: [libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn Wed, Jun 03, 2020 at 06:20:23PM +0530, Kaaira Gupta wrote:\n> On Tue, Jun 02, 2020 at 11:33:09PM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 02, 2020 at 05:20:14PM +0200, Niklas Söderlund wrote:\n> > > On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote:\n> > > > Replace hex input for pixelformats with their fourcc values,\n> > > > in cam and qcam.\n> > > > \n> > > > Signed-off-by: Kaaira Gupta \n> > > > ---\n> > > > src/cam/stream_options.cpp | 13 ++++++++++---\n> > > > 1 file changed, 10 insertions(+), 3 deletions(-)\n> > > > \n> > > > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> > > > index bd12c8f..9f9536e 100644\n> > > > --- a/src/cam/stream_options.cpp\n> > > > +++ b/src/cam/stream_options.cpp\n> > > > @@ -6,6 +6,7 @@\n> > > > */\n> > > > #include \"stream_options.h\"\n> > > > \n> > > > +#include \n> > \n> > What is this header used for ? It doesn't seem to be standard.\n> \n> it is used for \"toupper\" functionality, I added it so that we can write,\n> for example, ba24 instead of BA24..but as pointed out DRM formats can\n> have a mixture of both upper and lower cases in future, so I'll remove\n> that.\n> \n> > > > #include \n> > > > \n> > > > using namespace libcamera;\n> > > > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser()\n> > > > \t\t ArgumentRequired);\n> > > > \taddOption(\"height\", OptionInteger, \"Height in pixels\",\n> > > > \t\t ArgumentRequired);\n> > > > -\taddOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> > > > +\taddOption(\"pixelformat\", OptionString, \"Pixel format fourcc\",\n> > \n> > s/fourcc/FourCC/\n> > \n> > > > \t\t ArgumentRequired);\n> > > > }\n> > > > \n> > > > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> > > > \t\t}\n> > > > \n> > > > \t\t/* \\todo Translate 4CC string to pixelformat with modifier. */\n> > > > -\t\tif (opts.isSet(\"pixelformat\"))\n> > > > -\t\t\tcfg.pixelFormat = PixelFormat(opts[\"pixelformat\"]);\n> > > > +\t\tif (opts.isSet(\"pixelformat\")) {\n> > > > +\t\t\tstd::string fourcc = opts[\"pixelformat\"];\n> > > > +\t\t\ttransform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper);\n> > > \n> > > I fear we don't know all fourcc codes will be uppercase. It's currently \n> > > true all DRM defined ones are, but might change. As an example look at \n> > > V4L2 fourcc codes where lower/upper case is already mixed.\n> > > \n> > > > +\t\t\tchar char_array[5];\n> > > > +\t\t\tstrcpy(char_array, fourcc.c_str());\n> > > \n> > > I think you can simplify this, or something similar (not compile \n> > > tested).\n> > > \n> > > const char *fourcc = opts[\"pixelformat\"].toString().c_str();\n> > \n> > OptionValue::toString() returns a temporary object, you'll have a\n> > use-after-free.\n> > \n> > > \n> > > > +\t\t\tcfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) |\n> > > > +\t\t\t\t\t\t ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24));\n> > \n> > static_cast() instead of (__u32), to use C++-style casts, and\n> > because __u32 is a kernel type. Let's also split this to shorten the\n> > lines.\n> > \n> > This change goes in the right direction, but I think we need to expand\n> > it a little bit:\n> > \n> > - Some FourCC contain spaces at the end. It would be nice to write\n> > \n> > \t-s pixelformat=R8\n> > \n> > instead of\n> > \n> > \t-s 'pixelformat=R8 '\n> \n> Noted\n> \n> > \n> > - Should we support both the numerical value and the 4CC representation\n> > ? It could be useful in some cases, to avoid having to convert\n> > manually from hex to 4CC first, but maybe there are only few use cases\n> > for that feature.\n> \n> What are the use cases for direct hex values? I think FourCC are easier\n> to remember?\n\nA FourCC is indeed easier to remember, but I've found myself in\nsituations where one tool (a display test tool for instance) would give\nme a hex value, it would be nice to be able to pass it directly to cam\nwithout having to convert it manually.\n\n> > - With a 4CC we can't easily handle the formats that use\n> > DRM_FORMAT_BIG_ENDIAN. Maybe that's no big deal though, as we don't\n> > any use such format at the moment, and DRM_FORMAT_BIG_ENDIAN seems\n> > more like a hack than something that should be used going forward.\n> > \n> > Not all this need to be addressed now.\n> > \n> > > > +\t\t}\n> > > > \t}\n> > > > \n> > > > \treturn 0;","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 251F6C0100\n\tfor ;\n\tThu, 25 Jun 2020 03:42:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 831FB609A5;\n\tThu, 25 Jun 2020 05:42: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 9ABF4603BB\n\tfor ;\n\tThu, 25 Jun 2020 05:42:21 +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 02844521;\n\tThu, 25 Jun 2020 05:42:20 +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=\"ulUaFru3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593056541;\n\tbh=44lBLc3TaGR7LDivYjR71Y0xuAeCMNsbKp18Wy8sY4U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ulUaFru3pRfKvskk6ZN4IRYIo103T0JRKgr0uK25fGxYahVkIwA+pokcwPOpJ/Q3f\n\tYL6KhcuIuE1P8VnNx6hiMoJf3UifZbs7ya7qwuMhIND1Nn+OvgnX3SOPy4ceQiROw8\n\tz5W8wAYmsu3frJ6ykYcWTNu7rL6FIr7FN/3+wgD4=","Date":"Thu, 25 Jun 2020 06:42:19 +0300","From":"Laurent Pinchart ","To":"Kaaira Gupta ","Message-ID":"<20200625034219.GH5980@pendragon.ideasonboard.com>","References":"<20200529140433.GA18070@kaaira-HP-Pavilion-Notebook>\n\t<20200602152014.GA94018@oden.dyn.berto.se>\n\t<20200602203309.GB6547@pendragon.ideasonboard.com>\n\t<20200603125023.GA4967@kaaira-HP-Pavilion-Notebook>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200603125023.GA4967@kaaira-HP-Pavilion-Notebook>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: stream_option: use fourcc\n\tvalues to set cam/qcam formats","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]