[{"id":4923,"web_url":"https://patchwork.libcamera.org/comment/4923/","msgid":"","date":"2020-05-28T09:24:13","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: PixelFormat: Replace\n\thex with fourcc and modifiers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn 25/05/2020 18:35, Kaaira Gupta wrote:\n> Print fourCC characters instead of the hex values in toString() as they\n> are easier to comprehend. Also, print the corresponding modifiers of the\n> DRM formats so that it can be more specific.\n> \n> Describe the modifiers for MIPI vendor as it is mostly used in\n> libcamera. Print digits for the rest.\n\n\nThis prints the following for me now:\n\n> [1:21:53.958400669] [50957] INFO VIMC vimc.cpp:188 Skipping unsupported pixel format BA24 - NONE : Linear Layout\n> [1:21:53.958672633] [50957] INFO VIMC vimc.cpp:188 Skipping unsupported pixel format RG24 - NONE : Linear Layout\n> Camera configuration adjusted\n> [1:21:53.958935662] [50957] INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BG24 - NONE : Linear Layout\n\nI think we will need to shorten the default case which adds a lot of\ntext there:\n \" - NONE : Linear Layout\"\n\nis 23 extra characters on top of the fourcc, which is the default case\nfor most PixelFormats.\n\nPerhaps we should simply not print anything for those simple cases.\n\n\n\n> Signed-off-by: Kaaira Gupta \n> ---\n> \n> Changes since v2:\n> \t- Remove description for all vendors except for MIPI\n> \t- Change commit message to reflect this change.\n> \t- Change tests accordingly.\n> \n> Changes since v1:\n> \t- Replaced magic numbers with expressive values.\n> \t- Re-wrote ARM vendor's modifiers\n> \t- Re-wrote the vendors' map with a macro.\n> \t- Changed the copyrights in test file.\n> \t- Changed the tests.\n> \n> src/libcamera/pixelformats.cpp | 65 ++++++++++++++++++++++++++++++++--\n> test/meson.build | 1 +\n> test/pixel-format.cpp | 47 ++++++++++++++++++++++++\n> 3 files changed, 110 insertions(+), 3 deletions(-)\n> create mode 100644 test/pixel-format.cpp\n> \n> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> index 1330dc5..e16f5ba 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -6,6 +6,11 @@\n> */\n> \n> #include \n> +#include \n> +#include \n> +\n> +#define PIXELFORMAT_VENDOR(vendor)\t\t \\\n> +\t{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }\n> \n> /**\n> * \\file pixelformats.h\n> @@ -108,9 +113,63 @@ bool PixelFormat::operator<(const PixelFormat &other) const\n> */\n> std::string PixelFormat::toString() const\n> {\n> -\tchar str[11];\n> -\tsnprintf(str, 11, \"0x%08x\", fourcc_);\n> -\treturn str;\n> +\tif (fourcc_ == DRM_FORMAT_INVALID)\n> +\t\treturn \"\";\n> +\n> +\tchar ss[8] = { static_cast(fourcc_ & 0x7f),\n> +\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> +\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> +\t\t static_cast((fourcc_ >> 24) & 0x7f) };\n> +\n> +\tfor (unsigned int i = 0; i < 4; i++) {\n> +\t\tif (!isprint(ss[i]))\n> +\t\t\tss[i] = '.';\n> +\t}\n> +\n> +\tif (fourcc_ & (1 << 31))\n> +\t\tstrcat(ss, \"-BE\");\n> +\n> +\tstd::string modifier;\n> +\n> +\tif (modifier_ == DRM_FORMAT_MOD_INVALID) {\n> +\t\tmodifier = \" - modifier\";\n> +\t\treturn ss + modifier;\n> +\t}\n> +\n> +\t/* Map vendors with their Ids: */\n> +\tstd::map vendors = {\n> +\t\tPIXELFORMAT_VENDOR(NONE),\n> +\t\tPIXELFORMAT_VENDOR(MIPI)\n> +\t};\n> +\n> +\t/* Get the vendor name using its Id */\n> +\tlong int vendorCode = (modifier_ >> 56) & 0xff;\n> +\tstd::string vendor;\n> +\tstd::string vendorSpecification;\n> +\n> +\tswitch (vendorCode) {\n> +\tcase DRM_FORMAT_MOD_VENDOR_NONE: {\n> +\t\tvendor = vendors[vendorCode];\n> +\t\tif (modifier_ == DRM_FORMAT_MOD_LINEAR)\n> +\t\t\tvendorSpecification = \"Linear Layout\";\n\nSo Vendor-None mod-linear turns out to be the simple 'default' case,\nwhich we could consider as 'no modifier', so it becomes a bit redundant\nto display that there is no modifier.\n\nIt seems we should avoid printing this one altogether.\n\nIf you keep the conditional, then the case statement should be kept to\nprevent printing the values here.\n\n\n\n> +\t\tbreak;\n> +\t}\n> +\tcase DRM_FORMAT_MOD_VENDOR_MIPI: {\n> +\t\tvendor = vendors[vendorCode];\n> +\t\tif (modifier_ == MIPI_FORMAT_MOD_CSI2_PACKED)\n> +\t\t\tvendorSpecification = \"CSI-2 packed\";\n> +\t\tbreak;\n\nThis one is worth displaying though in some form.\n\nTalking with Laurent he has queried if it would be simpler to provide an\noptional 'modifier' string to the PixelFormatInfo tables.\n\nThen we could keep the fourcc handling above, but for modifiers - only\nprint a string if one is provided in the internal pixelformat tables.\n\nHowever, looking at that, we would then put the same modifier string in\neach of the PixelFormatInfo structures that set .packed == true, which\nwould be the same as handling the case here when modifier_ ==\nMOD_CSI2_PACKED...\n\n\n> +\t}\n> +\tdefault: {\n> +\t\tvendor = std::to_string(vendorCode);\n> +\t\tvendorSpecification = std::to_string(modifier_ & 0xff);\n> +\t}\n> +\t}\n> +\n> +\tmodifier = vendor + \" : \" + vendorSpecification;\n\nWe need these strings to be short, so we shouldn't have spaces around\nseparators.\n\n> +\tstd::string formatString(ss);\n> +\n> +\treturn formatString + \" - \" + modifier;\n\n\nPerhaps as we only really have a single modifier to support, we could\ntake the modifiers down to the simple case as a single 'if'...\n\nstd::string formatString(ss);\n\nif (modifier_ == MIPI_FORMAT_MOD_CSI2_PACKED)\n\tformatString += \":packed\";\n\nreturn formatString;\n\n\n\n\n> }\n> \n> } /* namespace libcamera */\n> diff --git a/test/meson.build b/test/meson.build\n> index bd7da14..591d848 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -33,6 +33,7 @@ internal_tests = [\n> ['message', 'message.cpp'],\n> ['object', 'object.cpp'],\n> ['object-invoke', 'object-invoke.cpp'],\n> + ['pixel-format', 'pixel-format.cpp'],\n> ['signal-threads', 'signal-threads.cpp'],\n> ['threads', 'threads.cpp'],\n> ['timer', 'timer.cpp'],\n> diff --git a/test/pixel-format.cpp b/test/pixel-format.cpp\n> new file mode 100644\n> index 0000000..edada56\n> --- /dev/null\n> +++ b/test/pixel-format.cpp\n> @@ -0,0 +1,47 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Kaaira Gupta\n> + * libcamera pixel format handling test\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/pixelformats.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class PixelFormatTest : public Test\n> +{\n> +protected:\n> +\tint run()\n> +\t{\n> +\t\tstd::vector> formats{\n> +\t\t\t{ PixelFormat(DRM_FORMAT_SRGGB8, DRM_FORMAT_MOD_INVALID), \"RGGB - modifier\" },\n> +\t\t\t{ PixelFormat(DRM_FORMAT_SRGGB8, DRM_FORMAT_MOD_LINEAR), \"RGGB - NONE : Linear Layout\" },\n> +\t\t\t{ PixelFormat(DRM_FORMAT_C8, DRM_FORMAT_MOD_SAMSUNG_64_32_TILE ), \"C8 - 4 : 1\" },\n> +\t\t\t{ PixelFormat(DRM_FORMAT_BIG_ENDIAN,MIPI_FORMAT_MOD_CSI2_PACKED),\"....-BE - MIPI : CSI-2 packed\"}\n> +\t\t};\n> +\t\tfor (const auto &format : formats) {\n> +\t\t\tif ((format.first).toString() != format.second) {\n> +\t\t\t\tcerr << \"Failed to convert PixelFormat \"\n> +\t\t\t\t << format.first.fourcc() << \" to string\"\n> +\t\t\t\t << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (PixelFormat().toString() != \"\") {\n> +\t\t\tcerr << \"Failed to convert default PixelFormat to string\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(PixelFormatTest)\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 037C5603CF\n\tfor ;\n\tThu, 28 May 2020 11:24:16 +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 6B2C12A3;\n\tThu, 28 May 2020 11:24:15 +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=\"BdspLh+X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1590657855;\n\tbh=GSuRgoITHTIHK1zIa4fZvtbUTYWGCpnFmfpv4NHi8b0=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=BdspLh+XKoihhJee13ogTnkBMYwm4ZNqg3AZdhRs7zhNgmSKRIjZ6dzypMtYzxoRs\n\tIYYovMlEBl2LwJrncHuvoEqnz2kaSdaAxSWIMpXRRA0QXddo0Nn5lCzjVHXA9mPZ7Y\n\tHFQ+5gooF+iVqi4bs6tvMzF0SvWtoJybSHFsCkXY=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta , libcamera-devel@lists.libcamera.org","References":"<20200525173540.GA21069@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":"","Date":"Thu, 28 May 2020 10:24:13 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200525173540.GA21069@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] libcamera: PixelFormat: Replace\n\thex with fourcc and modifiers","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":"Thu, 28 May 2020 09:24:16 -0000"}}]