[{"id":4360,"web_url":"https://patchwork.libcamera.org/comment/4360/","msgid":"<39437562-627d-c486-0669-398f2fc0e933@ideasonboard.com>","date":"2020-04-01T11:35:56","subject":"Re: [libcamera-devel] [PATCH] libcamera: PixelFormat: Replace hex\n\twith 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 31/03/2020 22:05, 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> Write the tests for them as well.\n> \n> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>\n> ---\n>  src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-\n>  test/meson.build               |   1 +\n>  test/pixel-format.cpp          |  55 ++++++++++\n>  3 files changed, 245 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 87557d9..398dbb2 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -6,6 +6,8 @@\n>   */\n>  \n>  #include <libcamera/pixelformats.h>\n> +#include <map>\n> +#include <string.h>\n>  \n>  /**\n>   * \\file pixelformats.h\n> @@ -108,9 +110,193 @@ 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_ == 0)\n\nAs DRM_FORMAT_INVALID == 0, I'd express this as:\n\n\tif (fourcc_ == DRM_FORMAT_INVALID)\n\n\n> +\t\treturn \"<INVALID>\";\n> +\n> +\tchar ss[8] = { static_cast<char>(fourcc_ & 0x7f),\n> +\t\t       static_cast<char>((fourcc_ >> 8) & 0x7f),\n> +\t\t       static_cast<char>((fourcc_ >> 16) & 0x7f),\n> +\t\t       static_cast<char>((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> +\tstd::string modifier;\n> +\n\nAs we continually reference the 'first' modifier only, perhaps it's\nworth taking copy of it to a local var rather than referencing the\n*modifiers_.begin() each time.\n\nOr alternatively, convert modifiers_ from being a set to a single value\nbefore this patch.\n\n> +\tif (*modifiers_.begin() == 72057594037927935) {\n\nTry to avoid magic numbers, What is 72057594037927935 ?\nIf this is DRM_FORMAT_MOD_INVALID, then just use that directly.\n\n> +\t\tmodifier = \" <INVALID> modifier\";\n> +\t\treturn ss + modifier;\n> +\t}\n> +\n> +\tif (*modifiers_.begin() == 0xf) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_MASK\";\n> +\t\treturn ss + modifier;\n\n\nThis 'mask' is not a value to express, but it is stating the 'mask'\nwhich can be used to extract the 'MOD_BLOCK_SIZE'.\n\nSo you could express somethign like:\n\nuint8_t block_size = modifierValue & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;\n\nBut note that the AFBC modifiers appear to be ARM vendor specific, so\nthey should be handled in that vendor code switch statement.\n\n\n> +\t} else if (*modifiers_.begin() == 1ULL) {\n> +\t\tmodifier = \"AFBC_FORMAT_MOD_BLOCK_SIZE_16x16\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == 2ULL) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_32x8\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == 3ULL) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_64x4\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == 4ULL) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 4)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_YTR\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 5)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_SPLIT\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 6)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_SPARS\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 7)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_CBR\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 8)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_TILED\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 9)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_SC\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 10)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_DB\";\n> +\t\treturn ss + modifier;\n> +\t} else if (*modifiers_.begin() == (1ULL << 11)) {\n> +\t\tmodifier = \" AFBC_FORMAT_MOD_BCH\";\n> +\t\treturn ss + modifier;\n> +\t}\n\n\nI suspect that we could simplify all of the conversions by using tables\nsuch as the vendor table below (populated with a similar macro to make\nsure the values and strings come directly from the defines given in\ndrm_fourcc.h).\n\nWould you be able to experiment with that a little to see if you can\nsimplify the code?\n\n\n> +\n> +\tstd::map<long int, std::string> vendors = {\n\nI would suggest we use the preprocessor to populate this table from the\ndefines directly:\n\n#define PIXELFORMAT_VENDOR(vendor) \\\n\t{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }\n\nstd::map<long int, std::string> vendors = {\n\tPIXELFORMAT_VENDOR(NONE),\n\tPIXELFORMAT_VENDOR(INTEL),\n\tPIXELFORMAT_VENDOR(AMD),\n\tPIXELFORMAT_VENDOR(NVIDIA),\n\t... and the rest ...\n}\t\n\n\n> +\t\t{ 0, \"NONE\" },\n> +\t\t{ 0x01, \"INTEL\" },\n> +\t\t{ 0x02, \"AMD\" },\n> +\t\t{ 0x03, \"NVIDIA\" },\n> +\t\t{ 0x04, \"SAMSUNG\" },\n> +\t\t{ 0x05, \"QCOM\" },\n> +\t\t{ 0x06, \"VIVANTE\" },\n> +\t\t{ 0x07, \"BROADCOM\" },\n> +\t\t{ 0x08, \"ARM\" },\n> +\t\t{ 0x09, \"ALLWINNER\" },\n> +\t\t{ 0x0a, \"MIPI\" }\n> +\t};\n> +\n> +\tlong int vendorCode = (*modifiers_.begin() >> 56) & 0xff;\n> +\n> +\tstd::string vendor = vendors[vendorCode];\n> +\n> +\tint modifierValue = *modifiers_.begin() & 0xff;\n\nSome vendors can utilise more bits, I think the modifierValue should be\na uint64_t, with the vendor bits masked out.\n\n\n> +\tstd::string vendorSpecification;\n> +\n> +\tswitch (vendorCode) {\n> +\tcase 0: {\n\nThe switch statements should be more expressive, and not use magic\nnumbers either:\n\n\tcase DRM_FORMAT_MOD_VENDOR_NONE: {\n\n\n> +\t\tif (modifierValue == 0)\n> +\t\t\tvendorSpecification = \"Linear Layout\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x01: {\n\n\tcase DRM_FORMAT_MOD_VENDOR_INTEL: {\n\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"X-tiled\";\n> +\t\telse if (modifierValue == 2)\n> +\t\t\tvendorSpecification = \"Y-tiled\";\n> +\t\telse if (modifierValue == 3)\n> +\t\t\tvendorSpecification = \"Yf-tiled\";\n> +\t\telse if (modifierValue == 4)\n> +\t\t\tvendorSpecification = \"Y-tiled CCS\";\n> +\t\telse if (modifierValue == 5)\n> +\t\t\tvendorSpecification = \"Yf-tiled CSS\";\n> +\t\telse if (modifierValue == 8)\n> +\t\t\tvendorSpecification = \"Bayer packed\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x02: {\n> +\t\t// no specifications\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x03: {\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"Tegra-tiled\";\n> +\t\telse if (modifierValue == 0x10)\n> +\t\t\tvendorSpecification = \"ONE_GOB | v = 0\";\n> +\t\telse if (modifierValue == 0x11)\n> +\t\t\tvendorSpecification = \"TWO_GOB | v = 1\";\n> +\t\telse if (modifierValue == 0x12)\n> +\t\t\tvendorSpecification = \"FOUR_GOB | v = 2\";\n> +\t\telse if (modifierValue == 0x13)\n> +\t\t\tvendorSpecification = \"EIGHT_GOB | v = 3\";\n> +\t\telse if (modifierValue == 0x14)\n> +\t\t\tvendorSpecification = \"SIXTEEN_GOB | v = 4\";\n> +\t\telse if (modifierValue == 0x15)\n> +\t\t\tvendorSpecification = \"THIRTYTWO_GOB | v = 5\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x04: {\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"Tiled, 64 (pixels) x 32 (lines)\";\n> +\t\telse if (modifierValue == 2)\n> +\t\t\tvendorSpecification = \"Tiled, 16 (pixels) x 16 (lines)\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x05: {\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"Compressed\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x06: {\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"4 x 4 tiled\";\n> +\t\telse if (modifierValue == 2)\n> +\t\t\tvendorSpecification = \"64 x 64 super-tiled\";\n> +\t\telse if (modifierValue == 3)\n> +\t\t\tvendorSpecification = \"4 x 4 split-tiled\";\n> +\t\telse if (modifierValue == 4)\n> +\t\t\tvendorSpecification = \"64 x 64 split-super-tiled\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x07: {\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"VC4-T-Tiled\";\n> +\t\telse if (modifierValue == 6)\n> +\t\t\tvendorSpecification = \"UIF\";\n> +\t\telse if (modifierValue == 2)\n> +\t\t\tvendorSpecification = \"SAND32\";\n> +\t\telse if (modifierValue == 3)\n> +\t\t\tvendorSpecification = \"SAND64\";\n> +\t\telse if (modifierValue == 4)\n> +\t\t\tvendorSpecification = \"SAND128\";\n> +\t\telse if (modifierValue == 5)\n> +\t\t\tvendorSpecification = \"SAND256\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x08: {\n> +\t\tvendorSpecification = \"DRM_FORMAT_MOD_ARM_AFBC(\" + modifierValue;\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x09: {\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"Tiled\";\n> +\t\tbreak;\n> +\t}\n> +\tcase 0x0a: {\n> +\t\tif (modifierValue == 1)\n> +\t\t\tvendorSpecification = \"CSI-2 packed\";\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tbreak;\n> +\t}\n> +\n> +\tmodifier = vendor + ' ' + vendorSpecification;\n> +\tstd::string formatString(ss);\n> +\n> +\treturn formatString + ' ' + modifier;\n>  }\n>  \n>  } /* namespace libcamera */\n> diff --git a/test/meson.build b/test/meson.build\n> index 8ab58ac..3f97278 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -30,6 +30,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..9676f27\n> --- /dev/null\n> +++ b/test/pixel-format.cpp\n> @@ -0,0 +1,55 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n\nThis is a new test being added, so the year is 2020, and I think you can\nown the copyright on files you create from scratch...\n\n Copyright (C) 2020, Kaaira Gupta\n\n> + *\n> + * libcamera pixel format handling test\n> + */\n> +\n> +#include <iostream>\n> +#include <vector>\n> +\n> +#include \"libcamera/pixelformats.h\"\n> +#include \"utils.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<std::pair<PixelFormat, const char *>> formats{\n> +\t\t\t{ PixelFormat(0, { DRM_FORMAT_MOD_INVALID }), \"<INVALID>\" },\n> +\t\t\t{ PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),\n> +\t\t\t  \"RGGB NONE Linear Layout\" },\n> +\t\t\t{ PixelFormat(DRM_FORMAT_C8,\n> +\t\t\t{ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),\n> +\t\t\t  \"C8   BROADCOM SAND32\" },\n> +\t\t\t{ PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),\n> +\t\t\t  \"YUV9 AFBC_FORMAT_MOD_SPLIT\" },\n> +\t\t\t{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),\n> +\t\t\t  \"....-BE <INVALID> modifier\" }\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() != \"<INVALID>\") {\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":"<kieran.bingham@ideasonboard.com>","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 05CE060105\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Apr 2020 13:35:59 +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 44BDCA2A;\n\tWed,  1 Apr 2020 13:35:58 +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=\"bKk8r2Vk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585740958;\n\tbh=SHoCEr2Ke0mEJmgDWT2BhRfRschJmeCSu7kkDP9tBaY=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=bKk8r2VkxSt2Wr61NEIDzcwBm7R7w5xcQ7k/zEE0+hS/EJbLBYggDf+rbu/AEydRF\n\tNf2AJvf3kA1Oz4GkesBCqKovCj7fAYhSQpvZdOGyF8/9XR20zVXtqicfdbkuDU7Asa\n\tIzv5+XFtuBU1uc3EMMi5qUsRYi/QLA1/aihqxxuc=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta <kgupta@es.iitr.ac.in>, libcamera-devel@lists.libcamera.org,\n\tHelen Koike <helen.koike@collabora.com>,\n\tVaishali Thakkar <vthakkar@vaishalithakkar.in>","References":"<20200331210525.GA4378@kaaira-HP-Pavilion-Notebook>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","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":"<39437562-627d-c486-0669-398f2fc0e933@ideasonboard.com>","Date":"Wed, 1 Apr 2020 12:35:56 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200331210525.GA4378@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: PixelFormat: Replace hex\n\twith fourcc and modifiers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 01 Apr 2020 11:35:59 -0000"}},{"id":4368,"web_url":"https://patchwork.libcamera.org/comment/4368/","msgid":"<20200402125620.GA5391@kaaira-HP-Pavilion-Notebook>","date":"2020-04-02T12:56:20","subject":"Re: [libcamera-devel] [PATCH] libcamera: PixelFormat: Replace hex\n\twith fourcc and modifiers","submitter":{"id":39,"url":"https://patchwork.libcamera.org/api/people/39/","name":"Kaaira Gupta","email":"kgupta@es.iitr.ac.in"},"content":"On Wed, Apr 01, 2020 at 12:35:56PM +0100, Kieran Bingham wrote:\n> Hi Kaaira,\n> \n> On 31/03/2020 22:05, 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> > Write the tests for them as well.\n> > \n> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>\n> > ---\n> >  src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-\n> >  test/meson.build               |   1 +\n> >  test/pixel-format.cpp          |  55 ++++++++++\n> >  3 files changed, 245 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 87557d9..398dbb2 100644\n> > --- a/src/libcamera/pixelformats.cpp\n> > +++ b/src/libcamera/pixelformats.cpp\n> > @@ -6,6 +6,8 @@\n> >   */\n> >  \n> >  #include <libcamera/pixelformats.h>\n> > +#include <map>\n> > +#include <string.h>\n> >  \n> >  /**\n> >   * \\file pixelformats.h\n> > @@ -108,9 +110,193 @@ 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_ == 0)\n> \n> As DRM_FORMAT_INVALID == 0, I'd express this as:\n> \n> \tif (fourcc_ == DRM_FORMAT_INVALID)\n> \n> \n> > +\t\treturn \"<INVALID>\";\n> > +\n> > +\tchar ss[8] = { static_cast<char>(fourcc_ & 0x7f),\n> > +\t\t       static_cast<char>((fourcc_ >> 8) & 0x7f),\n> > +\t\t       static_cast<char>((fourcc_ >> 16) & 0x7f),\n> > +\t\t       static_cast<char>((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> > +\tstd::string modifier;\n> > +\n> \n> As we continually reference the 'first' modifier only, perhaps it's\n> worth taking copy of it to a local var rather than referencing the\n> *modifiers_.begin() each time.\n> \n> Or alternatively, convert modifiers_ from being a set to a single value\n> before this patch.\n\nOkay, I'll do this first :)\n\n> \n> > +\tif (*modifiers_.begin() == 72057594037927935) {\n> \n> Try to avoid magic numbers, What is 72057594037927935 ?\n> If this is DRM_FORMAT_MOD_INVALID, then just use that directly.\n> \n> > +\t\tmodifier = \" <INVALID> modifier\";\n> > +\t\treturn ss + modifier;\n> > +\t}\n> > +\n> > +\tif (*modifiers_.begin() == 0xf) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_MASK\";\n> > +\t\treturn ss + modifier;\n> \n> \n> This 'mask' is not a value to express, but it is stating the 'mask'\n> which can be used to extract the 'MOD_BLOCK_SIZE'.\n\nI didn't understand you well, can you elaborate please?\n\n> \n> So you could express somethign like:\n> \n> uint8_t block_size = modifierValue & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;\n> \n> But note that the AFBC modifiers appear to be ARM vendor specific, so\n> they should be handled in that vendor code switch statement.\n\nOh, okay.\n\n> \n> \n> > +\t} else if (*modifiers_.begin() == 1ULL) {\n> > +\t\tmodifier = \"AFBC_FORMAT_MOD_BLOCK_SIZE_16x16\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == 2ULL) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_32x8\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == 3ULL) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_64x4\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == 4ULL) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 4)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_YTR\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 5)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_SPLIT\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 6)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_SPARS\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 7)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_CBR\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 8)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_TILED\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 9)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_SC\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 10)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_DB\";\n> > +\t\treturn ss + modifier;\n> > +\t} else if (*modifiers_.begin() == (1ULL << 11)) {\n> > +\t\tmodifier = \" AFBC_FORMAT_MOD_BCH\";\n> > +\t\treturn ss + modifier;\n> > +\t}\n> \n> \n> I suspect that we could simplify all of the conversions by using tables\n> such as the vendor table below (populated with a similar macro to make\n> sure the values and strings come directly from the defines given in\n> drm_fourcc.h).\n\nAgain, I don't understand what exactly you want here? :3\nCan you elaborate please?\n\n> \n> Would you be able to experiment with that a little to see if you can\n> simplify the code?\n> \n> \n> > +\n> > +\tstd::map<long int, std::string> vendors = {\n> \n> I would suggest we use the preprocessor to populate this table from the\n> defines directly:\n\nYes\n\n\n> \n> #define PIXELFORMAT_VENDOR(vendor) \\\n> \t{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }\n> \n> std::map<long int, std::string> vendors = {\n> \tPIXELFORMAT_VENDOR(NONE),\n> \tPIXELFORMAT_VENDOR(INTEL),\n> \tPIXELFORMAT_VENDOR(AMD),\n> \tPIXELFORMAT_VENDOR(NVIDIA),\n> \t... and the rest ...\n> }\t\n> \n> \n> > +\t\t{ 0, \"NONE\" },\n> > +\t\t{ 0x01, \"INTEL\" },\n> > +\t\t{ 0x02, \"AMD\" },\n> > +\t\t{ 0x03, \"NVIDIA\" },\n> > +\t\t{ 0x04, \"SAMSUNG\" },\n> > +\t\t{ 0x05, \"QCOM\" },\n> > +\t\t{ 0x06, \"VIVANTE\" },\n> > +\t\t{ 0x07, \"BROADCOM\" },\n> > +\t\t{ 0x08, \"ARM\" },\n> > +\t\t{ 0x09, \"ALLWINNER\" },\n> > +\t\t{ 0x0a, \"MIPI\" }\n> > +\t};\n> > +\n> > +\tlong int vendorCode = (*modifiers_.begin() >> 56) & 0xff;\n> > +\n> > +\tstd::string vendor = vendors[vendorCode];\n> > +\n> > +\tint modifierValue = *modifiers_.begin() & 0xff;\n> \n> Some vendors can utilise more bits, I think the modifierValue should be\n> a uint64_t, with the vendor bits masked out.\n\nYes you are right..I'll change this\n\n> \n> \n> > +\tstd::string vendorSpecification;\n> > +\n> > +\tswitch (vendorCode) {\n> > +\tcase 0: {\n> \n> The switch statements should be more expressive, and not use magic\n> numbers either:\n\nOkay, will make the changes\n\n\n> \n> \tcase DRM_FORMAT_MOD_VENDOR_NONE: {\n> \n> \n> > +\t\tif (modifierValue == 0)\n> > +\t\t\tvendorSpecification = \"Linear Layout\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x01: {\n> \n> \tcase DRM_FORMAT_MOD_VENDOR_INTEL: {\n> \n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"X-tiled\";\n> > +\t\telse if (modifierValue == 2)\n> > +\t\t\tvendorSpecification = \"Y-tiled\";\n> > +\t\telse if (modifierValue == 3)\n> > +\t\t\tvendorSpecification = \"Yf-tiled\";\n> > +\t\telse if (modifierValue == 4)\n> > +\t\t\tvendorSpecification = \"Y-tiled CCS\";\n> > +\t\telse if (modifierValue == 5)\n> > +\t\t\tvendorSpecification = \"Yf-tiled CSS\";\n> > +\t\telse if (modifierValue == 8)\n> > +\t\t\tvendorSpecification = \"Bayer packed\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x02: {\n> > +\t\t// no specifications\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x03: {\n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"Tegra-tiled\";\n> > +\t\telse if (modifierValue == 0x10)\n> > +\t\t\tvendorSpecification = \"ONE_GOB | v = 0\";\n> > +\t\telse if (modifierValue == 0x11)\n> > +\t\t\tvendorSpecification = \"TWO_GOB | v = 1\";\n> > +\t\telse if (modifierValue == 0x12)\n> > +\t\t\tvendorSpecification = \"FOUR_GOB | v = 2\";\n> > +\t\telse if (modifierValue == 0x13)\n> > +\t\t\tvendorSpecification = \"EIGHT_GOB | v = 3\";\n> > +\t\telse if (modifierValue == 0x14)\n> > +\t\t\tvendorSpecification = \"SIXTEEN_GOB | v = 4\";\n> > +\t\telse if (modifierValue == 0x15)\n> > +\t\t\tvendorSpecification = \"THIRTYTWO_GOB | v = 5\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x04: {\n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"Tiled, 64 (pixels) x 32 (lines)\";\n> > +\t\telse if (modifierValue == 2)\n> > +\t\t\tvendorSpecification = \"Tiled, 16 (pixels) x 16 (lines)\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x05: {\n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"Compressed\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x06: {\n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"4 x 4 tiled\";\n> > +\t\telse if (modifierValue == 2)\n> > +\t\t\tvendorSpecification = \"64 x 64 super-tiled\";\n> > +\t\telse if (modifierValue == 3)\n> > +\t\t\tvendorSpecification = \"4 x 4 split-tiled\";\n> > +\t\telse if (modifierValue == 4)\n> > +\t\t\tvendorSpecification = \"64 x 64 split-super-tiled\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x07: {\n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"VC4-T-Tiled\";\n> > +\t\telse if (modifierValue == 6)\n> > +\t\t\tvendorSpecification = \"UIF\";\n> > +\t\telse if (modifierValue == 2)\n> > +\t\t\tvendorSpecification = \"SAND32\";\n> > +\t\telse if (modifierValue == 3)\n> > +\t\t\tvendorSpecification = \"SAND64\";\n> > +\t\telse if (modifierValue == 4)\n> > +\t\t\tvendorSpecification = \"SAND128\";\n> > +\t\telse if (modifierValue == 5)\n> > +\t\t\tvendorSpecification = \"SAND256\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x08: {\n> > +\t\tvendorSpecification = \"DRM_FORMAT_MOD_ARM_AFBC(\" + modifierValue;\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x09: {\n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"Tiled\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase 0x0a: {\n> > +\t\tif (modifierValue == 1)\n> > +\t\t\tvendorSpecification = \"CSI-2 packed\";\n> > +\t\tbreak;\n> > +\t}\n> > +\tdefault:\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tmodifier = vendor + ' ' + vendorSpecification;\n> > +\tstd::string formatString(ss);\n> > +\n> > +\treturn formatString + ' ' + modifier;\n> >  }\n> >  \n> >  } /* namespace libcamera */\n> > diff --git a/test/meson.build b/test/meson.build\n> > index 8ab58ac..3f97278 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -30,6 +30,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..9676f27\n> > --- /dev/null\n> > +++ b/test/pixel-format.cpp\n> > @@ -0,0 +1,55 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> \n> This is a new test being added, so the year is 2020, and I think you can\n> own the copyright on files you create from scratch...\n\nOkayy..I didn't know how this thing works, so I just copied it from a\nfile :P\n\n> \n>  Copyright (C) 2020, Kaaira Gupta\n> \n> > + *\n> > + * libcamera pixel format handling test\n> > + */\n> > +\n> > +#include <iostream>\n> > +#include <vector>\n> > +\n> > +#include \"libcamera/pixelformats.h\"\n> > +#include \"utils.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<std::pair<PixelFormat, const char *>> formats{\n> > +\t\t\t{ PixelFormat(0, { DRM_FORMAT_MOD_INVALID }), \"<INVALID>\" },\n> > +\t\t\t{ PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),\n> > +\t\t\t  \"RGGB NONE Linear Layout\" },\n> > +\t\t\t{ PixelFormat(DRM_FORMAT_C8,\n> > +\t\t\t{ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),\n> > +\t\t\t  \"C8   BROADCOM SAND32\" },\n> > +\t\t\t{ PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),\n> > +\t\t\t  \"YUV9 AFBC_FORMAT_MOD_SPLIT\" },\n> > +\t\t\t{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),\n> > +\t\t\t  \"....-BE <INVALID> modifier\" }\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() != \"<INVALID>\") {\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> > \n\nThanks for the time!\n\n> \n> -- \n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<kgupta@es.iitr.ac.in>","Received":["from mail-pg1-x544.google.com (mail-pg1-x544.google.com\n\t[IPv6:2607:f8b0:4864:20::544])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C186629B9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  2 Apr 2020 14:56:35 +0200 (CEST)","by mail-pg1-x544.google.com with SMTP id h8so1793350pgs.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 02 Apr 2020 05:56:35 -0700 (PDT)","from kaaira-HP-Pavilion-Notebook\n\t([2401:4900:4167:4023:410f:e301:de32:617e])\n\tby smtp.gmail.com with ESMTPSA id\n\th11sm3752479pfq.56.2020.04.02.05.56.30\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 02 Apr 2020 05:56:33 -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=\"Cfi9KA7/\"; \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:in-reply-to:user-agent;\n\tbh=qOszxlpU0RKbEnCvILa8oZDcLPxSNGwh9bXjpsApedE=;\n\tb=Cfi9KA7/5eAnybzZKtoVa/fM3gmkpKzDL4ZQW4Un+f90AlWNKgcgLiTWhU+svTVQm+\n\t8C/FHeTX/e9w/vvDYsjQy5+uXcH9Bufr3aHTk8Ijo0gk3nLcbRsHhu9kH5lGbem5087k\n\tgqwCsv4flJhg0+fdKynWO2V7s+gz4uyAJRLcE603LuKqMsnt+apnpT/WLaF41VJekfFY\n\t4yRtbrIGYj4wiuFs59CdK8EqSRO6XIfmMLCb6wZ87IstAgAX0Gx0SIhxW1K6tvUpPx/P\n\tDBxFOAhCcLBltolz7fmelciWUkaAMwGjyMRwrRye873/JKbexkbuGkyxkVn6ZBnaqjRa\n\t6INw==","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:in-reply-to:user-agent;\n\tbh=qOszxlpU0RKbEnCvILa8oZDcLPxSNGwh9bXjpsApedE=;\n\tb=q8NNaKTqXZulQr1aaW1esrPP/8gD9GZ9m58M2m8FxKnr+1+0hG08Ah4C81vJPjA+MD\n\thma/f8TdMbQBzxqv1XXhzzMZgOe3CE97sL5O81LaCR+AP/6lhVNQnbXYDlx5mj5bJLBn\n\t879Qedr9WsPXmRZV0shWV7X4dv3yDbZMzsReLNtvyzX7Hdn1KqgdPn7r7XCMY5Ko4iv6\n\tHIGR4bD8Tq2Zr/aFHclDIUNQYPQbLpfFT5AW2OlPt3o/Ws7Zth1rh/eBgwuNtZjIYFYc\n\tOI6yRqKmjmMe6DAs4VUKhMuGMh7+K2XH3l00HJLCmBKwGO72USBzjMGUSi5jF65M8sE0\n\tQwJA==","X-Gm-Message-State":"AGi0PuZgpU2Bo35Ru3kspiUOTMpy73GsoA1WIM/f7A4g3A2UjJOwpK6Y\n\tl1D20HpFNdFuqVvBI03Wb/hrAA==","X-Google-Smtp-Source":"APiQypI8OZ1TFeXnISJJb1VqFl/f1xfaP5Clq1kzbmuBmDmmieTS+Uphj7PiwQPO2CPArZKfvXSVDQ==","X-Received":"by 2002:a63:5023:: with SMTP id\n\te35mr3132478pgb.165.1585832193811; \n\tThu, 02 Apr 2020 05:56:33 -0700 (PDT)","From":"Kaaira Gupta <kgupta@es.iitr.ac.in>","X-Google-Original-From":"Kaaira Gupta <Kaairakgupta@es.iitr.ac.in>","Date":"Thu, 2 Apr 2020 18:26:20 +0530","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Kaaira Gupta <kgupta@es.iitr.ac.in>, libcamera-devel@lists.libcamera.org,\n\tHelen Koike <helen.koike@collabora.com>,\n\tVaishali Thakkar <vthakkar@vaishalithakkar.in>","Message-ID":"<20200402125620.GA5391@kaaira-HP-Pavilion-Notebook>","References":"<20200331210525.GA4378@kaaira-HP-Pavilion-Notebook>\n\t<39437562-627d-c486-0669-398f2fc0e933@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<39437562-627d-c486-0669-398f2fc0e933@ideasonboard.com>","User-Agent":"Mutt/1.9.4 (2018-02-28)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: PixelFormat: Replace hex\n\twith fourcc and modifiers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 02 Apr 2020 12:56:41 -0000"}},{"id":4369,"web_url":"https://patchwork.libcamera.org/comment/4369/","msgid":"<173e05a0-f137-24eb-318e-ebc4d11b8090@ideasonboard.com>","date":"2020-04-03T10:07:08","subject":"Re: [libcamera-devel] [PATCH] libcamera: PixelFormat: Replace hex\n\twith 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 02/04/2020 13:56, Kaaira Gupta wrote:\n> On Wed, Apr 01, 2020 at 12:35:56PM +0100, Kieran Bingham wrote:\n>> Hi Kaaira,\n>>\n>> On 31/03/2020 22:05, 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>>> Write the tests for them as well.\n>>>\n>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>\n>>> ---\n>>>  src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-\n>>>  test/meson.build               |   1 +\n>>>  test/pixel-format.cpp          |  55 ++++++++++\n>>>  3 files changed, 245 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 87557d9..398dbb2 100644\n>>> --- a/src/libcamera/pixelformats.cpp\n>>> +++ b/src/libcamera/pixelformats.cpp\n>>> @@ -6,6 +6,8 @@\n>>>   */\n>>>  \n>>>  #include <libcamera/pixelformats.h>\n>>> +#include <map>\n>>> +#include <string.h>\n>>>  \n>>>  /**\n>>>   * \\file pixelformats.h\n>>> @@ -108,9 +110,193 @@ 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_ == 0)\n>>\n>> As DRM_FORMAT_INVALID == 0, I'd express this as:\n>>\n>> \tif (fourcc_ == DRM_FORMAT_INVALID)\n>>\n>>\n>>> +\t\treturn \"<INVALID>\";\n>>> +\n>>> +\tchar ss[8] = { static_cast<char>(fourcc_ & 0x7f),\n>>> +\t\t       static_cast<char>((fourcc_ >> 8) & 0x7f),\n>>> +\t\t       static_cast<char>((fourcc_ >> 16) & 0x7f),\n>>> +\t\t       static_cast<char>((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>>> +\tstd::string modifier;\n>>> +\n>>\n>> As we continually reference the 'first' modifier only, perhaps it's\n>> worth taking copy of it to a local var rather than referencing the\n>> *modifiers_.begin() each time.\n>>\n>> Or alternatively, convert modifiers_ from being a set to a single value\n>> before this patch.\n> \n> Okay, I'll do this first :)\n> \n>>\n>>> +\tif (*modifiers_.begin() == 72057594037927935) {\n>>\n>> Try to avoid magic numbers, What is 72057594037927935 ?\n>> If this is DRM_FORMAT_MOD_INVALID, then just use that directly.\n>>\n>>> +\t\tmodifier = \" <INVALID> modifier\";\n>>> +\t\treturn ss + modifier;\n>>> +\t}\n>>> +\n>>> +\tif (*modifiers_.begin() == 0xf) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_MASK\";\n>>> +\t\treturn ss + modifier;\n>>\n>>\n>> This 'mask' is not a value to express, but it is stating the 'mask'\n>> which can be used to extract the 'MOD_BLOCK_SIZE'.\n> \n> I didn't understand you well, can you elaborate please?\n\nThe value here, is not one that would be expected to be printed itself\nas a string.\n\nWhen dealing with bitfields (which is what the modifier essentially is),\nit is often the case that a mask is provided to define which bits are\nutilised.\n\nIn this case, the AFBC_FORMAT_MOD_BLOCK_SIZE_MASK == 0xF which\nrepresents 4 bits. (b00001111)\n\nNow, for example sake, imagine we have an 8 bit value (i.e. an 'unsigned\nchar') with the value 0x32.., we can use the 'BLOCK_SIZE_MASK' to\nextract the appropriate 'block-size', without being affected by the\nother bits that are set in the original value.\n\n\nTry building and playing with the following code if you want to\nexperiment: (online at http://cpp.sh/8toy if you want too)\n\n\n#include <stdio.h>\n\n#define AFBC_FORMAT_MOD_BLOCK_SIZE_MASK         0xf\n#define AFBC_FORMAT_MOD_BLOCK_SIZE_32x8         (2ULL)\n#define AFBC_FORMAT_MOD_CBR                     (1ULL <<  7)\n\nint main(int argc, char **argv)\n{\n  /* val = 0x82 */\n  unsigned char val = AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | AFBC_FORMAT_MOD_CBR;\n  unsigned int block_size = val & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;\n\n  printf(\"Original Value = 0x%x\\n\", val); /* = 0x82 */\n  printf(\"Block size = %d\\n\", block_size); /* = 2 */\n\n  switch (block_size) {\n  case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:\n        printf(\"AFBC_FORMAT_MOD_BLOCK_SIZE_32x8\\n\");\n        break;\n\n  default:\n     printf(\"Unknown format\\n\");\n  }\n\n  return 0;\n}\n\nFor more reading material if this is new to you:\nhttps://www.coranac.com/documents/working-with-bits-and-bitfields/\n\nSection 3 on that write up shows quite nicely what we're doing here.\n\nWe don't use native bitfields support from the C compiler in cases like\nthis, because they are not portable and are susceptible to endianness\nissues which are important to consider when working with the kernel,\nwhich runs on multiple architectures.\n\n\n\n>> So you could express somethign like:\n>>\n>> uint8_t block_size = modifierValue & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;\n>>\n>> But note that the AFBC modifiers appear to be ARM vendor specific, so\n>> they should be handled in that vendor code switch statement.\n> \n> Oh, okay.\n> \n>>\n>>\n>>> +\t} else if (*modifiers_.begin() == 1ULL) {\n>>> +\t\tmodifier = \"AFBC_FORMAT_MOD_BLOCK_SIZE_16x16\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == 2ULL) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_32x8\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == 3ULL) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_64x4\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == 4ULL) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 4)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_YTR\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 5)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_SPLIT\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 6)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_SPARS\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 7)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_CBR\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 8)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_TILED\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 9)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_SC\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 10)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_DB\";\n>>> +\t\treturn ss + modifier;\n>>> +\t} else if (*modifiers_.begin() == (1ULL << 11)) {\n>>> +\t\tmodifier = \" AFBC_FORMAT_MOD_BCH\";\n>>> +\t\treturn ss + modifier;\n>>> +\t}\n>>\n>>\n>> I suspect that we could simplify all of the conversions by using tables\n>> such as the vendor table below (populated with a similar macro to make\n>> sure the values and strings come directly from the defines given in\n>> drm_fourcc.h).\n> \n> Again, I don't understand what exactly you want here? :3\n> Can you elaborate please?\n\nFor each define, you require 3 lines of code:\n\n +\t} else if (*modifiers_.begin() == (1ULL << 10)) {\n +\t\tmodifier = \" AFBC_FORMAT_MOD_DB\";\n +\t\treturn ss + modifier;\n\nThis also references 'magic' values where you have to specify the value\ndirectly in the code. That can be prone to errors, and has the effect of\nlooking like 'magic numbers' so we try to avoid it.\n\nA solution to simplifying this code is to use a table, which is then\niterated.\n\nTry to see if you can generate a table like the following:\n\nstd::map<long int, std::string> afbc_format_mods = {\n \tAFBC_FORMAT_MOD(BLOCK_SIZE_16x16),\n\t...\n \tAFBC_FORMAT_MOD(BLOCK_SIZE_32x8_64x4),\n}\t\n\nThat will then take only one line per 'define' of code, and ensure that\nthe value provided by the define gets converted to the appropriate\nstring, like the way you do with the vendor string. You might need some\nadditional validation to first ensure that the value is in the map\ntable, and produce an 'unknown-bit-field' or such string if it isn't given.\n\n\nOnce you've got the table coded for the block-sizes, have a look at how\nthe other fields are used.\n\nWhere they use a construct like :\n\n   1ULL << 4\n\nThat means they are specifying a single bit within the bitfields, (I\nprefer it when people use the syntax BIT(4) in those cases),\n\nSo it's perfectly valid to have as we did above in our code sample:\n\n  uint8_t val = AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | AFBC_FORMAT_MOD_CBR;\n\n\nAnd in that event, we would want to return a string containing both parts:\n\ni.e. like:\n \"ARM: BLOCK_SIZE_32x8 | CBR\"\n\nFeel free to post an initial rework as you go if you want help (i.e.\ndon't worry if it doesn't print all fields just yet)\n\n\n>> Would you be able to experiment with that a little to see if you can\n>> simplify the code?\n>>\n>>\n>>> +\n>>> +\tstd::map<long int, std::string> vendors = {\n>>\n>> I would suggest we use the preprocessor to populate this table from the\n>> defines directly:\n> \n> Yes\n> \n> \n>>\n>> #define PIXELFORMAT_VENDOR(vendor) \\\n>> \t{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }\n>>\n>> std::map<long int, std::string> vendors = {\n>> \tPIXELFORMAT_VENDOR(NONE),\n>> \tPIXELFORMAT_VENDOR(INTEL),\n>> \tPIXELFORMAT_VENDOR(AMD),\n>> \tPIXELFORMAT_VENDOR(NVIDIA),\n>> \t... and the rest ...\n>> }\t\n>>\n>>\n>>> +\t\t{ 0, \"NONE\" },\n>>> +\t\t{ 0x01, \"INTEL\" },\n>>> +\t\t{ 0x02, \"AMD\" },\n>>> +\t\t{ 0x03, \"NVIDIA\" },\n>>> +\t\t{ 0x04, \"SAMSUNG\" },\n>>> +\t\t{ 0x05, \"QCOM\" },\n>>> +\t\t{ 0x06, \"VIVANTE\" },\n>>> +\t\t{ 0x07, \"BROADCOM\" },\n>>> +\t\t{ 0x08, \"ARM\" },\n>>> +\t\t{ 0x09, \"ALLWINNER\" },\n>>> +\t\t{ 0x0a, \"MIPI\" }\n>>> +\t};\n>>> +\n>>> +\tlong int vendorCode = (*modifiers_.begin() >> 56) & 0xff;\n>>> +\n>>> +\tstd::string vendor = vendors[vendorCode];\n>>> +\n>>> +\tint modifierValue = *modifiers_.begin() & 0xff;\n>>\n>> Some vendors can utilise more bits, I think the modifierValue should be\n>> a uint64_t, with the vendor bits masked out.\n> \n> Yes you are right..I'll change this\n> \n>>\n>>\n>>> +\tstd::string vendorSpecification;\n>>> +\n>>> +\tswitch (vendorCode) {\n>>> +\tcase 0: {\n>>\n>> The switch statements should be more expressive, and not use magic\n>> numbers either:\n> \n> Okay, will make the changes\n> \n> \n>>\n>> \tcase DRM_FORMAT_MOD_VENDOR_NONE: {\n>>\n>>\n>>> +\t\tif (modifierValue == 0)\n>>> +\t\t\tvendorSpecification = \"Linear Layout\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x01: {\n>>\n>> \tcase DRM_FORMAT_MOD_VENDOR_INTEL: {\n>>\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"X-tiled\";\n>>> +\t\telse if (modifierValue == 2)\n>>> +\t\t\tvendorSpecification = \"Y-tiled\";\n>>> +\t\telse if (modifierValue == 3)\n>>> +\t\t\tvendorSpecification = \"Yf-tiled\";\n>>> +\t\telse if (modifierValue == 4)\n>>> +\t\t\tvendorSpecification = \"Y-tiled CCS\";\n>>> +\t\telse if (modifierValue == 5)\n>>> +\t\t\tvendorSpecification = \"Yf-tiled CSS\";\n>>> +\t\telse if (modifierValue == 8)\n>>> +\t\t\tvendorSpecification = \"Bayer packed\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x02: {\n>>> +\t\t// no specifications\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x03: {\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"Tegra-tiled\";\n>>> +\t\telse if (modifierValue == 0x10)\n>>> +\t\t\tvendorSpecification = \"ONE_GOB | v = 0\";\n>>> +\t\telse if (modifierValue == 0x11)\n>>> +\t\t\tvendorSpecification = \"TWO_GOB | v = 1\";\n>>> +\t\telse if (modifierValue == 0x12)\n>>> +\t\t\tvendorSpecification = \"FOUR_GOB | v = 2\";\n>>> +\t\telse if (modifierValue == 0x13)\n>>> +\t\t\tvendorSpecification = \"EIGHT_GOB | v = 3\";\n>>> +\t\telse if (modifierValue == 0x14)\n>>> +\t\t\tvendorSpecification = \"SIXTEEN_GOB | v = 4\";\n>>> +\t\telse if (modifierValue == 0x15)\n>>> +\t\t\tvendorSpecification = \"THIRTYTWO_GOB | v = 5\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x04: {\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"Tiled, 64 (pixels) x 32 (lines)\";\n>>> +\t\telse if (modifierValue == 2)\n>>> +\t\t\tvendorSpecification = \"Tiled, 16 (pixels) x 16 (lines)\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x05: {\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"Compressed\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x06: {\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"4 x 4 tiled\";\n>>> +\t\telse if (modifierValue == 2)\n>>> +\t\t\tvendorSpecification = \"64 x 64 super-tiled\";\n>>> +\t\telse if (modifierValue == 3)\n>>> +\t\t\tvendorSpecification = \"4 x 4 split-tiled\";\n>>> +\t\telse if (modifierValue == 4)\n>>> +\t\t\tvendorSpecification = \"64 x 64 split-super-tiled\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x07: {\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"VC4-T-Tiled\";\n>>> +\t\telse if (modifierValue == 6)\n>>> +\t\t\tvendorSpecification = \"UIF\";\n>>> +\t\telse if (modifierValue == 2)\n>>> +\t\t\tvendorSpecification = \"SAND32\";\n>>> +\t\telse if (modifierValue == 3)\n>>> +\t\t\tvendorSpecification = \"SAND64\";\n>>> +\t\telse if (modifierValue == 4)\n>>> +\t\t\tvendorSpecification = \"SAND128\";\n>>> +\t\telse if (modifierValue == 5)\n>>> +\t\t\tvendorSpecification = \"SAND256\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x08: {\n>>> +\t\tvendorSpecification = \"DRM_FORMAT_MOD_ARM_AFBC(\" + modifierValue;\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x09: {\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"Tiled\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase 0x0a: {\n>>> +\t\tif (modifierValue == 1)\n>>> +\t\t\tvendorSpecification = \"CSI-2 packed\";\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tdefault:\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\n>>> +\tmodifier = vendor + ' ' + vendorSpecification;\n>>> +\tstd::string formatString(ss);\n>>> +\n>>> +\treturn formatString + ' ' + modifier;\n>>>  }\n>>>  \n>>>  } /* namespace libcamera */\n>>> diff --git a/test/meson.build b/test/meson.build\n>>> index 8ab58ac..3f97278 100644\n>>> --- a/test/meson.build\n>>> +++ b/test/meson.build\n>>> @@ -30,6 +30,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..9676f27\n>>> --- /dev/null\n>>> +++ b/test/pixel-format.cpp\n>>> @@ -0,0 +1,55 @@\n>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> +/*\n>>> + * Copyright (C) 2019, Google Inc.\n>>\n>> This is a new test being added, so the year is 2020, and I think you can\n>> own the copyright on files you create from scratch...\n> \n> Okayy..I didn't know how this thing works, so I just copied it from a\n> file :P\n\nThat's perfectly understandable :-)\n\nNormally if you make a copy of an existing file and retain a portion of\nit's content, you would retain the existing copyright ... but in this\ninstance I think excepting boilerplate stuff - this is your code addition.\n\n\n>>  Copyright (C) 2020, Kaaira Gupta\n>>\n>>> + *\n>>> + * libcamera pixel format handling test\n>>> + */\n>>> +\n>>> +#include <iostream>\n>>> +#include <vector>\n>>> +\n>>> +#include \"libcamera/pixelformats.h\"\n>>> +#include \"utils.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<std::pair<PixelFormat, const char *>> formats{\n>>> +\t\t\t{ PixelFormat(0, { DRM_FORMAT_MOD_INVALID }), \"<INVALID>\" },\n>>> +\t\t\t{ PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),\n>>> +\t\t\t  \"RGGB NONE Linear Layout\" },\n>>> +\t\t\t{ PixelFormat(DRM_FORMAT_C8,\n>>> +\t\t\t{ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),\n>>> +\t\t\t  \"C8   BROADCOM SAND32\" },\n>>> +\t\t\t{ PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),\n>>> +\t\t\t  \"YUV9 AFBC_FORMAT_MOD_SPLIT\" },\n>>> +\t\t\t{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),\n>>> +\t\t\t  \"....-BE <INVALID> modifier\" }\n\nBecause that's a 'table', I would be tempted to go beyond the 80 char\nlimit (we have a soft 80 char, and a 'harder' 120 char width limit) to\ntry to get those on one line. I always think tables should look like a\nspreadsheet in columns where possible - but that can take a lot of space\nsometimes :-) So it's probably a controversial stance (like how people\nargue between tabs vs spaces).\n\nIt looks like however the last DRM_FORMAT_BIG_ENDIAN,\nDRM_FORMAT_MOD_INVALID test possibly goes over 120 though so it might\ncause more hassle than it's worth :-)\n\n\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() != \"<INVALID>\") {\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>>>\n> \n> Thanks for the time!\n\nYou're welcome - thanks for all of your efforts :-)\n\n>>\n>> -- \n>> Regards\n>> --\n>> Kieran","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","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 81BFB600FA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  3 Apr 2020 12:07:11 +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 A4C7C321;\n\tFri,  3 Apr 2020 12:07:10 +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=\"JfH4mOKW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585908431;\n\tbh=62gPMeC3VEca3jQRo5deqJProDxBqeEs40ADY3GP3/g=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=JfH4mOKWcGyBeQLK4prMp/AR/Uu8ZBsWdHJAFhULlxmJv+XO0cy0bKR5G6yIDI0od\n\tT9Oxnzl3VNs9ZbGmtAt1J4lexgMhlluTolxXF+9TLOL0VFLS+03pysPjLp4zrevUWc\n\tRFXVI3SJGXUK0gdus/g8a9r+r5ygiXAFU2HpFwBw=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta <kgupta@es.iitr.ac.in>","Cc":"libcamera-devel@lists.libcamera.org,\n\tHelen Koike <helen.koike@collabora.com>,\n\tVaishali Thakkar <vthakkar@vaishalithakkar.in>","References":"<20200331210525.GA4378@kaaira-HP-Pavilion-Notebook>\n\t<39437562-627d-c486-0669-398f2fc0e933@ideasonboard.com>\n\t<20200402125620.GA5391@kaaira-HP-Pavilion-Notebook>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","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":"<173e05a0-f137-24eb-318e-ebc4d11b8090@ideasonboard.com>","Date":"Fri, 3 Apr 2020 11:07:08 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200402125620.GA5391@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: PixelFormat: Replace hex\n\twith fourcc and modifiers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 03 Apr 2020 10:07:11 -0000"}}]