[{"id":5330,"web_url":"https://patchwork.libcamera.org/comment/5330/","msgid":"<647b3893-9eff-0a73-e321-456f4a29b384@ideasonboard.com>","date":"2020-06-22T20:01:11","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn 22/06/2020 15:42, Kaaira Gupta wrote:\n> Replace hex input for pixelformats with their format names, for input in\n> cam and qcam.\n> \n> Signed-off-by: Kaaira Gupta \n> ---\n> \n> Changes since v1:\n> \t-use format names, according to formats namespace, instead of\n> \tFourCC\n> \n> src/cam/stream_options.cpp | 52 +++++++++++++++++++++++++++++++++++---\n> 1 file changed, 49 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> index bd12c8f..9fc428a 100644\n> --- a/src/cam/stream_options.cpp\n> +++ b/src/cam/stream_options.cpp\n> @@ -6,6 +6,8 @@\n> */\n> #include \"stream_options.h\"\n> \n> +#include \n> +\n> #include \n> \n> using namespace libcamera;\n> @@ -19,7 +21,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 name\",\n> \t\t ArgumentRequired);\n> }\n> \n> @@ -96,8 +98,52 @@ 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::map pixelFormatNames{\n> +\t\t\t\t{ \"BGR888\", formats::BGR888 },\n> +\t\t\t\t{ \"RGB888\", formats::RGB888 },\n> +\t\t\t\t{ \"ABGR8888\", formats::ABGR8888 },\n> +\t\t\t\t{ \"ARGB8888\", formats::ARGB8888 },\n> +\t\t\t\t{ \"BGRA8888\", formats::BGRA8888 },\n> +\t\t\t\t{ \"RGBA8888\", formats::RGBA8888 },\n> +\t\t\t\t{ \"YUYV\", formats::YUYV },\n> +\t\t\t\t{ \"YVYU\", formats::YVYU },\n> +\t\t\t\t{ \"UYVY\", formats::UYVY },\n> +\t\t\t\t{ \"VYUY\", formats::VYUY },\n> +\t\t\t\t{ \"NV16\", formats::NV16 },\n> +\t\t\t\t{ \"NV61\", formats::NV61 },\n> +\t\t\t\t{ \"NV12\", formats::NV12 },\n> +\t\t\t\t{ \"NV21\", formats::NV21 },\n> +\t\t\t\t{ \"YUV420\", formats::YUV420 },\n> +\t\t\t\t{ \"YUV422\", formats::YUV422 },\n> +\t\t\t\t{ \"R8\", formats::R8 },\n> +\t\t\t\t{ \"SBGGR8\", formats::SBGGR8 },\n> +\t\t\t\t{ \"SGBRG8\", formats::SGBRG8 },\n> +\t\t\t\t{ \"SGRBG8\", formats::SGRBG8 },\n> +\t\t\t\t{ \"SRGGB8\", formats::SRGGB8 },\n> +\t\t\t\t{ \"SBGGR10\", formats::SBGGR10 },\n> +\t\t\t\t{ \"SGBRG10\", formats::SGBRG10 },\n> +\t\t\t\t{ \"SGRBG10\", formats::SGRBG10 },\n> +\t\t\t\t{ \"SRGGB10\", formats::SRGGB10 },\n> +\t\t\t\t{ \"SBGGR10_CSI2P\", formats::SBGGR10_CSI2P },\n> +\t\t\t\t{ \"SGBRG10_CSI2P\", formats::SGBRG10_CSI2P },\n> +\t\t\t\t{ \"SGRBG10_CSI2P\", formats::SGRBG10_CSI2P },\n> +\t\t\t\t{ \"SRGGB10_CSI2P\", formats::SRGGB10_CSI2P },\n> +\t\t\t\t{ \"SBGGR12\", formats::SBGGR12 },\n> +\t\t\t\t{ \"SGBRG12\", formats::SGBRG12 },\n> +\t\t\t\t{ \"SGRBG12\", formats::SGRBG12 },\n> +\t\t\t\t{ \"SRGGB12\", formats::SRGGB12 },\n> +\t\t\t\t{ \"SBGGR12_CSI2P\", formats::SBGGR12_CSI2P },\n> +\t\t\t\t{ \"SGBRG12_CSI2P\", formats::SGBRG12_CSI2P },\n> +\t\t\t\t{ \"SGRBG12_CSI2P\", formats::SGRBG12_CSI2P },\n> +\t\t\t\t{ \"SRGGB12_CSI2P\", formats::SRGGB12_CSI2P },\n> +\t\t\t\t{ \"MJPEG\", formats::MJPEG },\n> +\t\t\t};\n> +\n> +\t\t\tstd::map::iterator it;\n> +\t\t\tit = pixelFormatNames.find(opts[\"pixelformat\"]);\n> +\t\t\tcfg.pixelFormat = it->second;\n\nThis doesn't deal with what would happen if the format can not be found.\n\nThere needs to be an error check to make sure that if the given string\nis not in the map, then an error is returned, and at least the iterator\nshould not be dereferenced.\n\n\nAt the moment, libcamera does not expose any of the internal format\ninformation, so there's no way to handle the comparison without this\nmap, so this indeed looks like the only way it can be handled currently[0].\n\nI think just for readability, and clarity, I would move the map outside\nof the scope of this function and somewhere to the top of the file as a\ngeneric 'data table'.\n\nIt would be nice if applications had more information on each of the\npixel-formats, to prevent having to duplicate all the information\neverywhere, but I fear that the consensus would likely be that\ninformation doesn't live in libcamera.\n\n\n[0]: But, we 'could' choose to expose a PixelFormat(std::string)\nconstructor, which could construct a PixelFormat by searching the\ninternal PixelFormatInfo tables... or return an if not found...\n\nAny thoughts on a string constructor for PixelFormat anyone?\n\n\n\n> +\t\t}\n> \t}\n> \n> \treturn 0;\n>","headers":{"Return-Path":"","Received":["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 E5EDF603B9\n\tfor ;\n\tMon, 22 Jun 2020 22:01:14 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 472CC327;\n\tMon, 22 Jun 2020 22:01:14 +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=\"sb3P0xJX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592856074;\n\tbh=l0VKdicd9+RtyM1AT3NKS6rZ5WhGzcE4tU1g5XzO7kU=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=sb3P0xJXxWFPI5gpTy56Yd62MGZH9uQFw8ADvS6NMYXgzJkdvj+ep9RMDqsLUWRtU\n\tLBbdeuDuWvM0dXLknqnVoVsoXHRKBHhfzaQPzfls2Su5CsDSyeDvfAYW6kWeEIVCCu\n\tyGXempLTeTi2XMgowXqWcOkqHMk8lU0Z7iwYPsdQ=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\t=?utf-8?q?Niklas_S=C3=B6derlund?= ","References":"<20200622144257.GA27697@kaaira-HP-Pavilion-Notebook>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<647b3893-9eff-0a73-e321-456f4a29b384@ideasonboard.com>","Date":"Mon, 22 Jun 2020 21:01:11 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200622144257.GA27697@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 22 Jun 2020 20:01:15 -0000"}},{"id":5331,"web_url":"https://patchwork.libcamera.org/comment/5331/","msgid":"<20200622204817.GA691357@oden.dyn.berto.se>","date":"2020-06-22T20:48:17","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kaaira and Kieran,\n\nOn 2020-06-22 21:01:11 +0100, Kieran Bingham wrote:\n> Hi Kaaira,\n> \n> On 22/06/2020 15:42, Kaaira Gupta wrote:\n> > Replace hex input for pixelformats with their format names, for input in\n> > cam and qcam.\n> > \n> > Signed-off-by: Kaaira Gupta \n> > ---\n> > \n> > Changes since v1:\n> > \t-use format names, according to formats namespace, instead of\n> > \tFourCC\n> > \n> > src/cam/stream_options.cpp | 52 +++++++++++++++++++++++++++++++++++---\n> > 1 file changed, 49 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> > index bd12c8f..9fc428a 100644\n> > --- a/src/cam/stream_options.cpp\n> > +++ b/src/cam/stream_options.cpp\n> > @@ -6,6 +6,8 @@\n> > */\n> > #include \"stream_options.h\"\n> > \n> > +#include \n> > +\n> > #include \n> > \n> > using namespace libcamera;\n> > @@ -19,7 +21,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 name\",\n> > \t\t ArgumentRequired);\n> > }\n> > \n> > @@ -96,8 +98,52 @@ 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::map pixelFormatNames{\n> > +\t\t\t\t{ \"BGR888\", formats::BGR888 },\n> > +\t\t\t\t{ \"RGB888\", formats::RGB888 },\n> > +\t\t\t\t{ \"ABGR8888\", formats::ABGR8888 },\n> > +\t\t\t\t{ \"ARGB8888\", formats::ARGB8888 },\n> > +\t\t\t\t{ \"BGRA8888\", formats::BGRA8888 },\n> > +\t\t\t\t{ \"RGBA8888\", formats::RGBA8888 },\n> > +\t\t\t\t{ \"YUYV\", formats::YUYV },\n> > +\t\t\t\t{ \"YVYU\", formats::YVYU },\n> > +\t\t\t\t{ \"UYVY\", formats::UYVY },\n> > +\t\t\t\t{ \"VYUY\", formats::VYUY },\n> > +\t\t\t\t{ \"NV16\", formats::NV16 },\n> > +\t\t\t\t{ \"NV61\", formats::NV61 },\n> > +\t\t\t\t{ \"NV12\", formats::NV12 },\n> > +\t\t\t\t{ \"NV21\", formats::NV21 },\n> > +\t\t\t\t{ \"YUV420\", formats::YUV420 },\n> > +\t\t\t\t{ \"YUV422\", formats::YUV422 },\n> > +\t\t\t\t{ \"R8\", formats::R8 },\n> > +\t\t\t\t{ \"SBGGR8\", formats::SBGGR8 },\n> > +\t\t\t\t{ \"SGBRG8\", formats::SGBRG8 },\n> > +\t\t\t\t{ \"SGRBG8\", formats::SGRBG8 },\n> > +\t\t\t\t{ \"SRGGB8\", formats::SRGGB8 },\n> > +\t\t\t\t{ \"SBGGR10\", formats::SBGGR10 },\n> > +\t\t\t\t{ \"SGBRG10\", formats::SGBRG10 },\n> > +\t\t\t\t{ \"SGRBG10\", formats::SGRBG10 },\n> > +\t\t\t\t{ \"SRGGB10\", formats::SRGGB10 },\n> > +\t\t\t\t{ \"SBGGR10_CSI2P\", formats::SBGGR10_CSI2P },\n> > +\t\t\t\t{ \"SGBRG10_CSI2P\", formats::SGBRG10_CSI2P },\n> > +\t\t\t\t{ \"SGRBG10_CSI2P\", formats::SGRBG10_CSI2P },\n> > +\t\t\t\t{ \"SRGGB10_CSI2P\", formats::SRGGB10_CSI2P },\n> > +\t\t\t\t{ \"SBGGR12\", formats::SBGGR12 },\n> > +\t\t\t\t{ \"SGBRG12\", formats::SGBRG12 },\n> > +\t\t\t\t{ \"SGRBG12\", formats::SGRBG12 },\n> > +\t\t\t\t{ \"SRGGB12\", formats::SRGGB12 },\n> > +\t\t\t\t{ \"SBGGR12_CSI2P\", formats::SBGGR12_CSI2P },\n> > +\t\t\t\t{ \"SGBRG12_CSI2P\", formats::SGBRG12_CSI2P },\n> > +\t\t\t\t{ \"SGRBG12_CSI2P\", formats::SGRBG12_CSI2P },\n> > +\t\t\t\t{ \"SRGGB12_CSI2P\", formats::SRGGB12_CSI2P },\n> > +\t\t\t\t{ \"MJPEG\", formats::MJPEG },\n> > +\t\t\t};\n> > +\n> > +\t\t\tstd::map::iterator it;\n> > +\t\t\tit = pixelFormatNames.find(opts[\"pixelformat\"]);\n> > +\t\t\tcfg.pixelFormat = it->second;\n> \n> This doesn't deal with what would happen if the format can not be found.\n> \n> There needs to be an error check to make sure that if the given string\n> is not in the map, then an error is returned, and at least the iterator\n> should not be dereferenced.\n> \n> \n> At the moment, libcamera does not expose any of the internal format\n> information, so there's no way to handle the comparison without this\n> map, so this indeed looks like the only way it can be handled currently[0].\n> \n> I think just for readability, and clarity, I would move the map outside\n> of the scope of this function and somewhere to the top of the file as a\n> generic 'data table'.\n> \n> It would be nice if applications had more information on each of the\n> pixel-formats, to prevent having to duplicate all the information\n> everywhere, but I fear that the consensus would likely be that\n> information doesn't live in libcamera.\n\nI think we crossed that threshold when we introduced the format:: \nnamespace. As we already give a \"libcamera name\" to a DRM fourcc + a \nmodifier I think there is little harm in exposing this name in string \nform to applications.\n\n> \n> \n> [0]: But, we 'could' choose to expose a PixelFormat(std::string)\n> constructor, which could construct a PixelFormat by searching the\n> internal PixelFormatInfo tables... or return an if not found...\n> \n> Any thoughts on a string constructor for PixelFormat anyone?\n> \n> \n> \n> > +\t\t}\n> > \t}\n> > \n> > \treturn 0;\n> > \n> \n> -- \n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","Received":["from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 95FAF603B9\n\tfor ;\n\tMon, 22 Jun 2020 22:48:19 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id c21so10424364lfb.3\n\tfor ;\n\tMon, 22 Jun 2020 13:48:19 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tx30sm3324688lfn.3.2020.06.22.13.48.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 22 Jun 2020 13:48:17 -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=\"01THYJ6l\"; \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=u2EnHUrloO9LZyt2wDt4ZbPsjy7TtaNV2wVTrXY+0bs=;\n\tb=01THYJ6llbO2Pdss/2QJSNnoR429BfoVMv4D2Bmzzy66MqQSXWuN5nTqRD4w09FkRh\n\tErq6JnAOx574TBq1ctGCUbKJMhppOrRbMz+RL9z70V4PgCAUByh/G4RxrUCs3+G7QkbG\n\tF9AymxDOo1kKva0WDnkCXx+p6/oGHc79bhDmO1dfpcLqFxScLq1/XEdAc81hoz6VFO4k\n\t3TTjciDTaR+0HCKq0Z31PBouMQuFWYIx770vmvn/uEATCMg3mg+CIKJAorwvo8tzwcg7\n\tQMmCx0379oH0iEqey6cPHosrlbMd5JRL93+m6sMBFhzv1YRl0EXbr/KegMlxNygBCJok\n\tEE5A==","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=u2EnHUrloO9LZyt2wDt4ZbPsjy7TtaNV2wVTrXY+0bs=;\n\tb=sgRp2KDVy/HKvScZH371R5EMl416t5V1cMt0wwtDjDSo4Kv6AWO3ECTo80olSru3Ej\n\tjYyfJlevLARoTThQCdcoXRcIqEAwnd+hvZ9Pr7ltRWeHNpv/TFQisubGU4u8tVtUjGmd\n\t7b/dt4/7BUCiLPTkuQHl6rcDEQw7SRob0EaexJxt3VnlbFMnjYJ8UR5cm9K8OHejME2R\n\tOTRmD+7wDgHeiOqgMwTFePZVujxN9nmWgQudVk1OvXQ2dADPeKEqQnWruksWObpOzH7A\n\txgsxwiaOmF8/S6F+tfuDR46d+CIDHjCEY/2YUGE8DrH2sjUznWjOB5agSZb+vRTYg3cC\n\tcfcA==","X-Gm-Message-State":"AOAM5312HQqDQhDiQZi60tFwa0tauQ26j/DaqLVCSbLtXSXrr9ASB2c8\n\tq7na2RNHGaJ6ODY/Kd8s8AvzE2CTQCc=","X-Google-Smtp-Source":"ABdhPJzw+e4PclvpJyu6Rvg4MplBkLK3h7eAjKJD2RznAAjuHCzk20I7zbTWisCITIy5r1+z2F2WcA==","X-Received":"by 2002:a19:6a02:: with SMTP id u2mr10881174lfu.9.1592858898740; \n\tMon, 22 Jun 2020 13:48:18 -0700 (PDT)","Date":"Mon, 22 Jun 2020 22:48:17 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Cc":"Kaaira Gupta , libcamera-devel@lists.libcamera.org","Message-ID":"<20200622204817.GA691357@oden.dyn.berto.se>","References":"<20200622144257.GA27697@kaaira-HP-Pavilion-Notebook>\n\t<647b3893-9eff-0a73-e321-456f4a29b384@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<647b3893-9eff-0a73-e321-456f4a29b384@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 22 Jun 2020 20:48:20 -0000"}},{"id":11558,"web_url":"https://patchwork.libcamera.org/comment/11558/","msgid":"<5f4e09da-e293-6b25-ed20-1d993fbcb1f4@ideasonboard.com>","date":"2020-07-24T14:35:33","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nReviving this thread,\n\nOn 22/06/2020 21:48, Niklas Söderlund wrote:\n> Hi Kaaira and Kieran,\n> \n> On 2020-06-22 21:01:11 +0100, Kieran Bingham wrote:\n>> Hi Kaaira,\n>>\n>> On 22/06/2020 15:42, Kaaira Gupta wrote:\n>>> Replace hex input for pixelformats with their format names, for input in\n>>> cam and qcam.\n>>>\n>>> Signed-off-by: Kaaira Gupta \n>>> ---\n>>>\n>>> Changes since v1:\n>>> \t-use format names, according to formats namespace, instead of\n>>> \tFourCC\n>>>\n>>> src/cam/stream_options.cpp | 52 +++++++++++++++++++++++++++++++++++---\n>>> 1 file changed, 49 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n>>> index bd12c8f..9fc428a 100644\n>>> --- a/src/cam/stream_options.cpp\n>>> +++ b/src/cam/stream_options.cpp\n>>> @@ -6,6 +6,8 @@\n>>> */\n>>> #include \"stream_options.h\"\n>>> \n>>> +#include \n>>> +\n>>> #include \n>>> \n>>> using namespace libcamera;\n>>> @@ -19,7 +21,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 name\",\n>>> \t\t ArgumentRequired);\n>>> }\n>>> \n>>> @@ -96,8 +98,52 @@ 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::map pixelFormatNames{\n>>> +\t\t\t\t{ \"BGR888\", formats::BGR888 },\n>>> +\t\t\t\t{ \"RGB888\", formats::RGB888 },\n>>> +\t\t\t\t{ \"ABGR8888\", formats::ABGR8888 },\n>>> +\t\t\t\t{ \"ARGB8888\", formats::ARGB8888 },\n>>> +\t\t\t\t{ \"BGRA8888\", formats::BGRA8888 },\n>>> +\t\t\t\t{ \"RGBA8888\", formats::RGBA8888 },\n>>> +\t\t\t\t{ \"YUYV\", formats::YUYV },\n>>> +\t\t\t\t{ \"YVYU\", formats::YVYU },\n>>> +\t\t\t\t{ \"UYVY\", formats::UYVY },\n>>> +\t\t\t\t{ \"VYUY\", formats::VYUY },\n>>> +\t\t\t\t{ \"NV16\", formats::NV16 },\n>>> +\t\t\t\t{ \"NV61\", formats::NV61 },\n>>> +\t\t\t\t{ \"NV12\", formats::NV12 },\n>>> +\t\t\t\t{ \"NV21\", formats::NV21 },\n>>> +\t\t\t\t{ \"YUV420\", formats::YUV420 },\n>>> +\t\t\t\t{ \"YUV422\", formats::YUV422 },\n>>> +\t\t\t\t{ \"R8\", formats::R8 },\n>>> +\t\t\t\t{ \"SBGGR8\", formats::SBGGR8 },\n>>> +\t\t\t\t{ \"SGBRG8\", formats::SGBRG8 },\n>>> +\t\t\t\t{ \"SGRBG8\", formats::SGRBG8 },\n>>> +\t\t\t\t{ \"SRGGB8\", formats::SRGGB8 },\n>>> +\t\t\t\t{ \"SBGGR10\", formats::SBGGR10 },\n>>> +\t\t\t\t{ \"SGBRG10\", formats::SGBRG10 },\n>>> +\t\t\t\t{ \"SGRBG10\", formats::SGRBG10 },\n>>> +\t\t\t\t{ \"SRGGB10\", formats::SRGGB10 },\n>>> +\t\t\t\t{ \"SBGGR10_CSI2P\", formats::SBGGR10_CSI2P },\n>>> +\t\t\t\t{ \"SGBRG10_CSI2P\", formats::SGBRG10_CSI2P },\n>>> +\t\t\t\t{ \"SGRBG10_CSI2P\", formats::SGRBG10_CSI2P },\n>>> +\t\t\t\t{ \"SRGGB10_CSI2P\", formats::SRGGB10_CSI2P },\n>>> +\t\t\t\t{ \"SBGGR12\", formats::SBGGR12 },\n>>> +\t\t\t\t{ \"SGBRG12\", formats::SGBRG12 },\n>>> +\t\t\t\t{ \"SGRBG12\", formats::SGRBG12 },\n>>> +\t\t\t\t{ \"SRGGB12\", formats::SRGGB12 },\n>>> +\t\t\t\t{ \"SBGGR12_CSI2P\", formats::SBGGR12_CSI2P },\n>>> +\t\t\t\t{ \"SGBRG12_CSI2P\", formats::SGBRG12_CSI2P },\n>>> +\t\t\t\t{ \"SGRBG12_CSI2P\", formats::SGRBG12_CSI2P },\n>>> +\t\t\t\t{ \"SRGGB12_CSI2P\", formats::SRGGB12_CSI2P },\n>>> +\t\t\t\t{ \"MJPEG\", formats::MJPEG },\n>>> +\t\t\t};\n>>> +\n>>> +\t\t\tstd::map::iterator it;\n>>> +\t\t\tit = pixelFormatNames.find(opts[\"pixelformat\"]);\n>>> +\t\t\tcfg.pixelFormat = it->second;\n>>\n>> This doesn't deal with what would happen if the format can not be found.\n>>\n>> There needs to be an error check to make sure that if the given string\n>> is not in the map, then an error is returned, and at least the iterator\n>> should not be dereferenced.\n>>\n>>\n>> At the moment, libcamera does not expose any of the internal format\n>> information, so there's no way to handle the comparison without this\n>> map, so this indeed looks like the only way it can be handled currently[0].\n>>\n>> I think just for readability, and clarity, I would move the map outside\n>> of the scope of this function and somewhere to the top of the file as a\n>> generic 'data table'.\n>>\n>> It would be nice if applications had more information on each of the\n>> pixel-formats, to prevent having to duplicate all the information\n>> everywhere, but I fear that the consensus would likely be that\n>> information doesn't live in libcamera.\n> \n> I think we crossed that threshold when we introduced the format:: \n> namespace. As we already give a \"libcamera name\" to a DRM fourcc + a \n> modifier I think there is little harm in exposing this name in string \n> form to applications.\n\nMy qeustion was more about 'how much' should we expose to applications?\nI.e. the content of include/libcamera/internal/formats.h\n\nor...\n\n\n> \n>>\n>>\n>> [0]: But, we 'could' choose to expose a PixelFormat(std::string)\n>> constructor, which could construct a PixelFormat by searching the\n>> internal PixelFormatInfo tables... or return an if not found...\n>>\n>> Any thoughts on a string constructor for PixelFormat anyone?\n\nShould we add a PixelFormat(std::string) which will search for a named\nformat, and return it if available.\n\nI would prefer this option that having applications encode a table of\nall supported strings/PixelFormats each time, but I also think there's\nvalue in all of the information stored in PixelFormatInfo, but I don't\ncurrently think we want to make that part of the public-api ... so a\nstring constructor is a the best solution I can currently see...\n\n\n--\nKieran\n\n\n>>\n>>\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 BE22EBD878\n\tfor ;\n\tFri, 24 Jul 2020 14:35:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E69E611B0;\n\tFri, 24 Jul 2020 16:35:37 +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 56A4C60535\n\tfor ;\n\tFri, 24 Jul 2020 16:35:36 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B31C2538;\n\tFri, 24 Jul 2020 16:35:35 +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=\"B4rkF4XY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595601335;\n\tbh=IbFDcXgeUCdcOy1dNJBIeMvOpgamyXINcMqjgzw15VQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=B4rkF4XYFTIHqUk778SFkUzDAWWieOuYIFwpBse8yuTeFyw6J6dgvnkIpnvy7nNZ4\n\tHp2Yh/bvadu6aSEQnBEfZk/nMEMbYtkJhatlHyJxUuFmbHyfejSsJgXu7ZLQhuzabm\n\twomzVL8JQRQXogTT4LMeLUl/aT/s6puaPHWh3Cng=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","References":"<20200622144257.GA27697@kaaira-HP-Pavilion-Notebook>\n\t<647b3893-9eff-0a73-e321-456f4a29b384@ideasonboard.com>\n\t<20200622204817.GA691357@oden.dyn.berto.se>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<5f4e09da-e293-6b25-ed20-1d993fbcb1f4@ideasonboard.com>","Date":"Fri, 24 Jul 2020 15:35:33 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200622204817.GA691357@oden.dyn.berto.se>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"Kaaira Gupta , 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\" "}},{"id":11593,"web_url":"https://patchwork.libcamera.org/comment/11593/","msgid":"<20200725001521.GN5921@pendragon.ideasonboard.com>","date":"2020-07-25T00:15:21","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Jul 24, 2020 at 03:35:33PM +0100, Kieran Bingham wrote:\n> On 22/06/2020 21:48, Niklas Söderlund wrote:\n> > On 2020-06-22 21:01:11 +0100, Kieran Bingham wrote:\n> >> On 22/06/2020 15:42, Kaaira Gupta wrote:\n> >>> Replace hex input for pixelformats with their format names, for input in\n> >>> cam and qcam.\n> >>>\n> >>> Signed-off-by: Kaaira Gupta \n> >>> ---\n> >>>\n> >>> Changes since v1:\n> >>> \t-use format names, according to formats namespace, instead of\n> >>> \tFourCC\n> >>>\n> >>> src/cam/stream_options.cpp | 52 +++++++++++++++++++++++++++++++++++---\n> >>> 1 file changed, 49 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> >>> index bd12c8f..9fc428a 100644\n> >>> --- a/src/cam/stream_options.cpp\n> >>> +++ b/src/cam/stream_options.cpp\n> >>> @@ -6,6 +6,8 @@\n> >>> */\n> >>> #include \"stream_options.h\"\n> >>> \n> >>> +#include \n> >>> +\n> >>> #include \n> >>> \n> >>> using namespace libcamera;\n> >>> @@ -19,7 +21,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 name\",\n> >>> \t\t ArgumentRequired);\n> >>> }\n> >>> \n> >>> @@ -96,8 +98,52 @@ 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::map pixelFormatNames{\n> >>> +\t\t\t\t{ \"BGR888\", formats::BGR888 },\n> >>> +\t\t\t\t{ \"RGB888\", formats::RGB888 },\n> >>> +\t\t\t\t{ \"ABGR8888\", formats::ABGR8888 },\n> >>> +\t\t\t\t{ \"ARGB8888\", formats::ARGB8888 },\n> >>> +\t\t\t\t{ \"BGRA8888\", formats::BGRA8888 },\n> >>> +\t\t\t\t{ \"RGBA8888\", formats::RGBA8888 },\n> >>> +\t\t\t\t{ \"YUYV\", formats::YUYV },\n> >>> +\t\t\t\t{ \"YVYU\", formats::YVYU },\n> >>> +\t\t\t\t{ \"UYVY\", formats::UYVY },\n> >>> +\t\t\t\t{ \"VYUY\", formats::VYUY },\n> >>> +\t\t\t\t{ \"NV16\", formats::NV16 },\n> >>> +\t\t\t\t{ \"NV61\", formats::NV61 },\n> >>> +\t\t\t\t{ \"NV12\", formats::NV12 },\n> >>> +\t\t\t\t{ \"NV21\", formats::NV21 },\n> >>> +\t\t\t\t{ \"YUV420\", formats::YUV420 },\n> >>> +\t\t\t\t{ \"YUV422\", formats::YUV422 },\n> >>> +\t\t\t\t{ \"R8\", formats::R8 },\n> >>> +\t\t\t\t{ \"SBGGR8\", formats::SBGGR8 },\n> >>> +\t\t\t\t{ \"SGBRG8\", formats::SGBRG8 },\n> >>> +\t\t\t\t{ \"SGRBG8\", formats::SGRBG8 },\n> >>> +\t\t\t\t{ \"SRGGB8\", formats::SRGGB8 },\n> >>> +\t\t\t\t{ \"SBGGR10\", formats::SBGGR10 },\n> >>> +\t\t\t\t{ \"SGBRG10\", formats::SGBRG10 },\n> >>> +\t\t\t\t{ \"SGRBG10\", formats::SGRBG10 },\n> >>> +\t\t\t\t{ \"SRGGB10\", formats::SRGGB10 },\n> >>> +\t\t\t\t{ \"SBGGR10_CSI2P\", formats::SBGGR10_CSI2P },\n> >>> +\t\t\t\t{ \"SGBRG10_CSI2P\", formats::SGBRG10_CSI2P },\n> >>> +\t\t\t\t{ \"SGRBG10_CSI2P\", formats::SGRBG10_CSI2P },\n> >>> +\t\t\t\t{ \"SRGGB10_CSI2P\", formats::SRGGB10_CSI2P },\n> >>> +\t\t\t\t{ \"SBGGR12\", formats::SBGGR12 },\n> >>> +\t\t\t\t{ \"SGBRG12\", formats::SGBRG12 },\n> >>> +\t\t\t\t{ \"SGRBG12\", formats::SGRBG12 },\n> >>> +\t\t\t\t{ \"SRGGB12\", formats::SRGGB12 },\n> >>> +\t\t\t\t{ \"SBGGR12_CSI2P\", formats::SBGGR12_CSI2P },\n> >>> +\t\t\t\t{ \"SGBRG12_CSI2P\", formats::SGBRG12_CSI2P },\n> >>> +\t\t\t\t{ \"SGRBG12_CSI2P\", formats::SGRBG12_CSI2P },\n> >>> +\t\t\t\t{ \"SRGGB12_CSI2P\", formats::SRGGB12_CSI2P },\n> >>> +\t\t\t\t{ \"MJPEG\", formats::MJPEG },\n> >>> +\t\t\t};\n> >>> +\n> >>> +\t\t\tstd::map::iterator it;\n> >>> +\t\t\tit = pixelFormatNames.find(opts[\"pixelformat\"]);\n> >>> +\t\t\tcfg.pixelFormat = it->second;\n> >>\n> >> This doesn't deal with what would happen if the format can not be found.\n> >>\n> >> There needs to be an error check to make sure that if the given string\n> >> is not in the map, then an error is returned, and at least the iterator\n> >> should not be dereferenced.\n> >>\n> >>\n> >> At the moment, libcamera does not expose any of the internal format\n> >> information, so there's no way to handle the comparison without this\n> >> map, so this indeed looks like the only way it can be handled currently[0].\n> >>\n> >> I think just for readability, and clarity, I would move the map outside\n> >> of the scope of this function and somewhere to the top of the file as a\n> >> generic 'data table'.\n> >>\n> >> It would be nice if applications had more information on each of the\n> >> pixel-formats, to prevent having to duplicate all the information\n> >> everywhere, but I fear that the consensus would likely be that\n> >> information doesn't live in libcamera.\n> > \n> > I think we crossed that threshold when we introduced the format:: \n> > namespace. As we already give a \"libcamera name\" to a DRM fourcc + a \n> > modifier I think there is little harm in exposing this name in string \n> > form to applications.\n> \n> My qeustion was more about 'how much' should we expose to applications?\n> I.e. the content of include/libcamera/internal/formats.h\n> \n> or...\n> \n> >> [0]: But, we 'could' choose to expose a PixelFormat(std::string)\n> >> constructor, which could construct a PixelFormat by searching the\n> >> internal PixelFormatInfo tables... or return an if not found...\n> >>\n> >> Any thoughts on a string constructor for PixelFormat anyone?\n> \n> Should we add a PixelFormat(std::string) which will search for a named\n> format, and return it if available.\n> \n> I would prefer this option that having applications encode a table of\n> all supported strings/PixelFormats each time, but I also think there's\n> value in all of the information stored in PixelFormatInfo, but I don't\n> currently think we want to make that part of the public-api ... so a\n> string constructor is a the best solution I can currently see...\n\nOr a static function ?\n\nclass PixelFormat\n{\n\t...\n\tstatic PixelFormat fromString(const std::string &name);\n\t...\n};\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 E02F9BD86F\n\tfor ;\n\tSat, 25 Jul 2020 00:15:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 665BC60939;\n\tSat, 25 Jul 2020 02:15:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FF3B60496\n\tfor ;\n\tSat, 25 Jul 2020 02:15:29 +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 A1400538;\n\tSat, 25 Jul 2020 02:15:28 +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=\"OXvh3bZ3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595636128;\n\tbh=dGP4v4wPZnRRizcRDuaCRgG/QR1FgDW0pAW9AR+z6XU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OXvh3bZ3v1D4rynj+5GWlqbISWvWs34enB9wWME5CB+mUFgJHi6UpgQZ5VcIzt9Xe\n\t8tvfaXxsA77sDtcC/1YDLyjzMMT02HrQVDPFuDVPtcB0IjJakBl9dTwohxo9AekKGc\n\tcnU3bAqmgxvFFQRWrLn+hXD4YtC3bSMeEHB+lfps=","Date":"Sat, 25 Jul 2020 03:15:21 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200725001521.GN5921@pendragon.ideasonboard.com>","References":"<20200622144257.GA27697@kaaira-HP-Pavilion-Notebook>\n\t<647b3893-9eff-0a73-e321-456f4a29b384@ideasonboard.com>\n\t<20200622204817.GA691357@oden.dyn.berto.se>\n\t<5f4e09da-e293-6b25-ed20-1d993fbcb1f4@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<5f4e09da-e293-6b25-ed20-1d993fbcb1f4@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: stream_option: use\n\tformat name to set cam/qcam format","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":"Kaaira Gupta , 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\" "}}]