[{"id":11635,"web_url":"https://patchwork.libcamera.org/comment/11635/","msgid":"<3e1cfde0-c7ca-246b-1cc2-9b569d3c89d3@ideasonboard.com>","date":"2020-07-27T15:30:01","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn 27/07/2020 16:18, Kaaira Gupta wrote:\n> Add a function which returns a format, given its name as a string.\n> \n> Signed-off-by: Kaaira Gupta \n> ---\n> include/libcamera/internal/formats.h | 1 +\n> src/libcamera/formats.cpp | 20 ++++++++++++++++++++\n> 2 files changed, 21 insertions(+)\n> \n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index 0bb1510..2589e88 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -38,6 +38,7 @@ public:\n> \n> \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> +\tstatic const PixelFormat &info(const std::string &name);\n\nVery close, but I think this is a layering violation.\n\nNote the return type of the other two info() calls, I would expect this\nfunction to have the same type.\n\n> \tunsigned int stride(unsigned int width, unsigned int plane,\n> \t\t\t unsigned int align = 1) const;\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 11774b0..8ea5461 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -23,6 +23,8 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Formats)\n> \n> +const PixelFormat invalidPixelFormat = PixelFormat();\n> +\n> /**\n> * \\class PixelFormatPlaneInfo\n> * \\brief Information about a single plane of a pixel format\n> @@ -663,6 +665,24 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> \treturn info->second;\n> }\n> \n> +/**\n> + * \\brief Retrieve pixel format from its name\n> + * \\param[in] name The name of pixel format\n> + * \\return The pixel format corresponding to \\a name if known, or an invalid\n> + * pixel format otherwise\n> + */\n> +const PixelFormat &PixelFormatInfo::info(const std::string &name)\n> +{\n> +\tauto it = pixelFormatInfo.begin();\n> +\twhile (it != pixelFormatInfo.end()) {\n\nCan this be written as:\n\n \tfor (const PixelFormatInfo &info : pixelFormatInfo) {\n\t...\n\t}\n\nThere's probably not much in it, except for not needing to use manual\niterators then.\n\n\n> +\t\tif (it->second.name == name) {\n> +\t\t\treturn it->first;\n> +\t\t}\n> +\t\tit++;\n> +\t}\n> +\treturn invalidPixelFormat;\n\nThis function should return either the correct PixelFormatInfo, or the\ninvalidPixelFormatInfo.\n\nthen it's up to the caller to extract the PixelFormat from that const\nreference.\n\nIf invalidPixelFormatInfo is returned, it will contain an\n'invalidPixelFormat'.\n\n--\nKieran\n\n\n\n> +}\n> +\n> /**\n> * \\brief Compute the stride\n> * \\param[in] width The width of the line, in pixels\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8EF60BD86F\n\tfor ;\n\tMon, 27 Jul 2020 15:30:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F24E56138C;\n\tMon, 27 Jul 2020 17:30:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCD2D6118A\n\tfor ;\n\tMon, 27 Jul 2020 17:30:04 +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 4ED91556;\n\tMon, 27 Jul 2020 17:30:04 +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=\"qUK8AS7e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595863804;\n\tbh=5wA/edfn8ZUjMTuPA/Gl/XEO9Q/xuFt4BJhzEH4lekc=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=qUK8AS7eBsPkl6j023ibHxn2fk6mj4uUumlBlLHeqMhCuPZXFABR3pc5I6lfAupeP\n\tE4r69Q0XohqSKWuD+KHJz6m59hn2ht4oaShZbueOWU8NuD5WiDz7ySAn7lQpPrfx0j\n\t2aGkNa41Awz0BIaSVEtOW2a7PmOkzBG9wXHemOD0=","To":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\t=?utf-8?q?Niklas_S=C3=B6derlund?= ","References":"<20200727151820.24466-1-kgupta@es.iitr.ac.in>\n\t<20200727151820.24466-2-kgupta@es.iitr.ac.in>","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":"<3e1cfde0-c7ca-246b-1cc2-9b569d3c89d3@ideasonboard.com>","Date":"Mon, 27 Jul 2020 16:30:01 +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":"<20200727151820.24466-2-kgupta@es.iitr.ac.in>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11638,"web_url":"https://patchwork.libcamera.org/comment/11638/","msgid":"<20200727153341.GG20890@pendragon.ideasonboard.com>","date":"2020-07-27T15:33:41","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jul 27, 2020 at 04:30:01PM +0100, Kieran Bingham wrote:\n> On 27/07/2020 16:18, Kaaira Gupta wrote:\n> > Add a function which returns a format, given its name as a string.\n> > \n> > Signed-off-by: Kaaira Gupta \n> > ---\n> > include/libcamera/internal/formats.h | 1 +\n> > src/libcamera/formats.cpp | 20 ++++++++++++++++++++\n> > 2 files changed, 21 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > index 0bb1510..2589e88 100644\n> > --- a/include/libcamera/internal/formats.h\n> > +++ b/include/libcamera/internal/formats.h\n> > @@ -38,6 +38,7 @@ public:\n> > \n> > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> > +\tstatic const PixelFormat &info(const std::string &name);\n> \n> Very close, but I think this is a layering violation.\n> \n> Note the return type of the other two info() calls, I would expect this\n> function to have the same type.\n\nI would expect this function to be part of the PixelFormat class\n\n\tstatic PixelFormat fromString(const std::string &name);\n\nand be called with\n\n\tPixelFormat format = PixelFormat::fromString(\"YUYV\");\n\n> > \tunsigned int stride(unsigned int width, unsigned int plane,\n> > \t\t\t unsigned int align = 1) const;\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 11774b0..8ea5461 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -23,6 +23,8 @@ namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(Formats)\n> > \n> > +const PixelFormat invalidPixelFormat = PixelFormat();\n> > +\n> > /**\n> > * \\class PixelFormatPlaneInfo\n> > * \\brief Information about a single plane of a pixel format\n> > @@ -663,6 +665,24 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> > \treturn info->second;\n> > }\n> > \n> > +/**\n> > + * \\brief Retrieve pixel format from its name\n> > + * \\param[in] name The name of pixel format\n> > + * \\return The pixel format corresponding to \\a name if known, or an invalid\n> > + * pixel format otherwise\n> > + */\n> > +const PixelFormat &PixelFormatInfo::info(const std::string &name)\n> > +{\n> > +\tauto it = pixelFormatInfo.begin();\n> > +\twhile (it != pixelFormatInfo.end()) {\n> \n> Can this be written as:\n> \n> \tfor (const PixelFormatInfo &info : pixelFormatInfo) {\n> \t...\n> \t}\n> \n> There's probably not much in it, except for not needing to use manual\n> iterators then.\n\nAnd it looks nicer (to me at least :-)).\n\n> > +\t\tif (it->second.name == name) {\n> > +\t\t\treturn it->first;\n> > +\t\t}\n> > +\t\tit++;\n> > +\t}\n> > +\treturn invalidPixelFormat;\n> \n> This function should return either the correct PixelFormatInfo, or the\n> invalidPixelFormatInfo.\n> \n> then it's up to the caller to extract the PixelFormat from that const\n> reference.\n> \n> If invalidPixelFormatInfo is returned, it will contain an\n> 'invalidPixelFormat'.\n> \n> > +}\n> > +\n> > /**\n> > * \\brief Compute the stride\n> > * \\param[in] width The width of the line, in pixels","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 0E33CBD878\n\tfor ;\n\tMon, 27 Jul 2020 15:33:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EE0F6118A;\n\tMon, 27 Jul 2020 17:33:55 +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 BB316605B2\n\tfor ;\n\tMon, 27 Jul 2020 17:33:53 +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 1388E556;\n\tMon, 27 Jul 2020 17:33:49 +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=\"J9LlzAy8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595864029;\n\tbh=LcP1+e+iL3MvDQm9x74ou57MY0f9u8ZEYy+uU3rlcSU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J9LlzAy8Pv0WFbPXnNdKkJjbi+xWN2egLtJi674FUU8Lz7JKjAe+7ClUrisOn2NTV\n\tsjJi5UuAqVszgrog3sJFnrlntNJLvgHfdvuvRy2oZEHD0YY8fAd8IOjjkty9+S9rZf\n\tHbmcFzf0D5zSqhpvZrZJd6mi4JAsyRmWw7UPckKk=","Date":"Mon, 27 Jul 2020 18:33:41 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200727153341.GG20890@pendragon.ideasonboard.com>","References":"<20200727151820.24466-1-kgupta@es.iitr.ac.in>\n\t<20200727151820.24466-2-kgupta@es.iitr.ac.in>\n\t<3e1cfde0-c7ca-246b-1cc2-9b569d3c89d3@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<3e1cfde0-c7ca-246b-1cc2-9b569d3c89d3@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11642,"web_url":"https://patchwork.libcamera.org/comment/11642/","msgid":"<20200727153820.GH20890@pendragon.ideasonboard.com>","date":"2020-07-27T15:38:20","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jul 27, 2020 at 06:33:41PM +0300, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Jul 27, 2020 at 04:30:01PM +0100, Kieran Bingham wrote:\n> > On 27/07/2020 16:18, Kaaira Gupta wrote:\n> > > Add a function which returns a format, given its name as a string.\n> > > \n> > > Signed-off-by: Kaaira Gupta \n> > > ---\n> > > include/libcamera/internal/formats.h | 1 +\n> > > src/libcamera/formats.cpp | 20 ++++++++++++++++++++\n> > > 2 files changed, 21 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > > index 0bb1510..2589e88 100644\n> > > --- a/include/libcamera/internal/formats.h\n> > > +++ b/include/libcamera/internal/formats.h\n> > > @@ -38,6 +38,7 @@ public:\n> > > \n> > > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > > \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> > > +\tstatic const PixelFormat &info(const std::string &name);\n> > \n> > Very close, but I think this is a layering violation.\n> > \n> > Note the return type of the other two info() calls, I would expect this\n> > function to have the same type.\n> \n> I would expect this function to be part of the PixelFormat class\n> \n> \tstatic PixelFormat fromString(const std::string &name);\n> \n> and be called with\n> \n> \tPixelFormat format = PixelFormat::fromString(\"YUYV\");\n\nI see that's what patch 2/2 implements :-) I wonder if we need a\nname-based lookup in the PixelFormatInfo class, it should be enough to\nimplement it in PixelFormat only. However, as the PixelFormat class\ndoesn't have access to the pixelFormatInfo map, that's not something we\ncould implement without some refactoring. I'm thus fine with a lookup\nfunction here.\n\n> > > \tunsigned int stride(unsigned int width, unsigned int plane,\n> > > \t\t\t unsigned int align = 1) const;\n> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > > index 11774b0..8ea5461 100644\n> > > --- a/src/libcamera/formats.cpp\n> > > +++ b/src/libcamera/formats.cpp\n> > > @@ -23,6 +23,8 @@ namespace libcamera {\n> > > \n> > > LOG_DEFINE_CATEGORY(Formats)\n> > > \n> > > +const PixelFormat invalidPixelFormat = PixelFormat();\n> > > +\n> > > /**\n> > > * \\class PixelFormatPlaneInfo\n> > > * \\brief Information about a single plane of a pixel format\n> > > @@ -663,6 +665,24 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> > > \treturn info->second;\n> > > }\n> > > \n> > > +/**\n> > > + * \\brief Retrieve pixel format from its name\n> > > + * \\param[in] name The name of pixel format\n> > > + * \\return The pixel format corresponding to \\a name if known, or an invalid\n> > > + * pixel format otherwise\n> > > + */\n> > > +const PixelFormat &PixelFormatInfo::info(const std::string &name)\n> > > +{\n> > > +\tauto it = pixelFormatInfo.begin();\n> > > +\twhile (it != pixelFormatInfo.end()) {\n> > \n> > Can this be written as:\n> > \n> > \tfor (const PixelFormatInfo &info : pixelFormatInfo) {\n> > \t...\n> > \t}\n> > \n> > There's probably not much in it, except for not needing to use manual\n> > iterators then.\n> \n> And it looks nicer (to me at least :-)).\n> \n> > > +\t\tif (it->second.name == name) {\n> > > +\t\t\treturn it->first;\n> > > +\t\t}\n> > > +\t\tit++;\n> > > +\t}\n> > > +\treturn invalidPixelFormat;\n> > \n> > This function should return either the correct PixelFormatInfo, or the\n> > invalidPixelFormatInfo.\n> > \n> > then it's up to the caller to extract the PixelFormat from that const\n> > reference.\n> > \n> > If invalidPixelFormatInfo is returned, it will contain an\n> > 'invalidPixelFormat'.\n> > \n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Compute the stride\n> > > * \\param[in] width The width of the line, in pixels","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 119BCBD86F\n\tfor ;\n\tMon, 27 Jul 2020 15:38:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D401D61253;\n\tMon, 27 Jul 2020 17:38:38 +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 BBEC0605B2\n\tfor ;\n\tMon, 27 Jul 2020 17:38:36 +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 3A69E556;\n\tMon, 27 Jul 2020 17:38: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=\"cDmAFTs8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595864308;\n\tbh=9d8TyTn+u8XXaI+Z9yVfdPsWnT5hnMR6zqc9GRbugCY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cDmAFTs8ITr+RSPfDZ3MTZYKGaq0gLPKcOBQI26r/2EJZzbXxsRcDF5fEfq6OdLRg\n\tzji5PvuJdjboF7kK7FExBbgpJnEAZ9s/0sPiERqED9xekX2nQdYhVE1/y+BsIqrQFz\n\tgKfDE75ev+tlkyD1SzLZs57GDgYandYc5d1pBqj4=","Date":"Mon, 27 Jul 2020 18:38:20 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200727153820.GH20890@pendragon.ideasonboard.com>","References":"<20200727151820.24466-1-kgupta@es.iitr.ac.in>\n\t<20200727151820.24466-2-kgupta@es.iitr.ac.in>\n\t<3e1cfde0-c7ca-246b-1cc2-9b569d3c89d3@ideasonboard.com>\n\t<20200727153341.GG20890@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200727153341.GG20890@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11653,"web_url":"https://patchwork.libcamera.org/comment/11653/","msgid":"<0ec38112-257a-34ac-4143-e9fe5f8e9141@ideasonboard.com>","date":"2020-07-27T16:10:35","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 27/07/2020 16:38, Laurent Pinchart wrote:\n> On Mon, Jul 27, 2020 at 06:33:41PM +0300, Laurent Pinchart wrote:\n>> Hi Kieran,\n>>\n>> On Mon, Jul 27, 2020 at 04:30:01PM +0100, Kieran Bingham wrote:\n>>> On 27/07/2020 16:18, Kaaira Gupta wrote:\n>>>> Add a function which returns a format, given its name as a string.\n>>>>\n>>>> Signed-off-by: Kaaira Gupta \n>>>> ---\n>>>> include/libcamera/internal/formats.h | 1 +\n>>>> src/libcamera/formats.cpp | 20 ++++++++++++++++++++\n>>>> 2 files changed, 21 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n>>>> index 0bb1510..2589e88 100644\n>>>> --- a/include/libcamera/internal/formats.h\n>>>> +++ b/include/libcamera/internal/formats.h\n>>>> @@ -38,6 +38,7 @@ public:\n>>>> \n>>>> \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n>>>> \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n>>>> +\tstatic const PixelFormat &info(const std::string &name);\n>>>\n>>> Very close, but I think this is a layering violation.\n>>>\n>>> Note the return type of the other two info() calls, I would expect this\n>>> function to have the same type.\n>>\n>> I would expect this function to be part of the PixelFormat class\n>>\n>> \tstatic PixelFormat fromString(const std::string &name);\n>>\n>> and be called with\n>>\n>> \tPixelFormat format = PixelFormat::fromString(\"YUYV\");\n> \n> I see that's what patch 2/2 implements :-) I wonder if we need a\n> name-based lookup in the PixelFormatInfo class, it should be enough to\n> implement it in PixelFormat only. However, as the PixelFormat class\n> doesn't have access to the pixelFormatInfo map, that's not something we\n> could implement without some refactoring. I'm thus fine with a lookup\n> function here.\n\nIndeed, that's why I think we should have a PixelFormatInfo\n static const PixelFormatInfo &info(const std::string &name);\n\n\nand a separate\n static PixelFormat fromString(const std::string &name);\n\nas is implemented in 2/3 indeed.\n\n--\nKieran\n\n\n>>>> \tunsigned int stride(unsigned int width, unsigned int plane,\n>>>> \t\t\t unsigned int align = 1) const;\n>>>> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n>>>> index 11774b0..8ea5461 100644\n>>>> --- a/src/libcamera/formats.cpp\n>>>> +++ b/src/libcamera/formats.cpp\n>>>> @@ -23,6 +23,8 @@ namespace libcamera {\n>>>> \n>>>> LOG_DEFINE_CATEGORY(Formats)\n>>>> \n>>>> +const PixelFormat invalidPixelFormat = PixelFormat();\n>>>> +\n>>>> /**\n>>>> * \\class PixelFormatPlaneInfo\n>>>> * \\brief Information about a single plane of a pixel format\n>>>> @@ -663,6 +665,24 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n>>>> \treturn info->second;\n>>>> }\n>>>> \n>>>> +/**\n>>>> + * \\brief Retrieve pixel format from its name\n>>>> + * \\param[in] name The name of pixel format\n>>>> + * \\return The pixel format corresponding to \\a name if known, or an invalid\n>>>> + * pixel format otherwise\n>>>> + */\n>>>> +const PixelFormat &PixelFormatInfo::info(const std::string &name)\n>>>> +{\n>>>> +\tauto it = pixelFormatInfo.begin();\n>>>> +\twhile (it != pixelFormatInfo.end()) {\n>>>\n>>> Can this be written as:\n>>>\n>>> \tfor (const PixelFormatInfo &info : pixelFormatInfo) {\n>>> \t...\n>>> \t}\n>>>\n>>> There's probably not much in it, except for not needing to use manual\n>>> iterators then.\n>>\n>> And it looks nicer (to me at least :-)).\n>>\n>>>> +\t\tif (it->second.name == name) {\n>>>> +\t\t\treturn it->first;\n>>>> +\t\t}\n>>>> +\t\tit++;\n>>>> +\t}\n>>>> +\treturn invalidPixelFormat;\n>>>\n>>> This function should return either the correct PixelFormatInfo, or the\n>>> invalidPixelFormatInfo.\n>>>\n>>> then it's up to the caller to extract the PixelFormat from that const\n>>> reference.\n>>>\n>>> If invalidPixelFormatInfo is returned, it will contain an\n>>> 'invalidPixelFormat'.\n>>>\n>>>> +}\n>>>> +\n>>>> /**\n>>>> * \\brief Compute the stride\n>>>> * \\param[in] width The width of the line, in pixels\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 27E5FBD878\n\tfor ;\n\tMon, 27 Jul 2020 16:10:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AF9961253;\n\tMon, 27 Jul 2020 18:10:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1805E605B2\n\tfor ;\n\tMon, 27 Jul 2020 18:10:39 +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 6C241556;\n\tMon, 27 Jul 2020 18:10:38 +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=\"ueX9o6Kt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595866238;\n\tbh=28NObwpyU6zViZAmo5xjHqdxKwmIzCXnwLuHi+c1Vu4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ueX9o6KtcpOu4SaVsSsethZF81qLPwknneGDRw8hoX9Ogsif4HJHS1OSWdO6k+skh\n\tfqdpvaqkUH+vf7pO5N+OPiKY6lrBfvjRAhoxhQkcU82ybpvMhw1qpLiT5WuJtRTwxp\n\tOXFP9Vwkzc1XHeqCtCAVomLK+auw5XpTAi6TgXxg=","To":"Laurent Pinchart ","References":"<20200727151820.24466-1-kgupta@es.iitr.ac.in>\n\t<20200727151820.24466-2-kgupta@es.iitr.ac.in>\n\t<3e1cfde0-c7ca-246b-1cc2-9b569d3c89d3@ideasonboard.com>\n\t<20200727153341.GG20890@pendragon.ideasonboard.com>\n\t<20200727153820.GH20890@pendragon.ideasonboard.com>","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":"<0ec38112-257a-34ac-4143-e9fe5f8e9141@ideasonboard.com>","Date":"Mon, 27 Jul 2020 17:10:35 +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":"<20200727153820.GH20890@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: formats:\n\tPixelFormatInfo: Add name lookup function","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]