[{"id":5289,"web_url":"https://patchwork.libcamera.org/comment/5289/","msgid":"","date":"2020-06-19T21:40:15","subject":"Re: [libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn 19/06/2020 20:06, Kaaira Gupta wrote:\n> Print format names defined in formats namespace instead of the hex\n> values in toString() as they are easier to comprehend. For this add\n> a property of 'name' in PixelFormatInfo so as to map the formats\n> with their names. Print fourcc for formats which are not used in\n> libcamera.\n\n\nExcellent, this is really readable now - and ties in nicely with the new\nformats namespacing:\n\n>> Using camera VIMC Sensor B\n>> [66:02:15.314555780] [1159464] INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888\n>> [66:02:15.314729922] [1159464] INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888\n>> Capture until user interrupts by SIGINT\n\n\n> Signed-off-by: Kaaira Gupta \n> ---\n> \n> Changes since v4:\n> \t-Print libcamera defined names instead of fourcc.\n> \n> Changes since v3:\n> \t-shortened the texts.\n> \t-Removed default case as well.\n> \t-changed commit message and tests to reflect the changes.\n> \n> Changes since v2:\n> - Remove description for all vendors except for MIPI\n> - Change commit message to reflect this change.\n> - Change tests accordingly.\n> \n> Changes since v1:\n> - Replaced magic numbers with expressive values.\n> - Re-wrote ARM vendor's modifiers\n> - Re-wrote the vendors' map with a macro.\n> - Changed the copyrights in test file.\n> - Changed the tests.\n> \n> include/libcamera/internal/formats.h | 1 +\n> src/libcamera/formats.cpp | 40 +++++++++++++++++++++--\n> src/libcamera/pixel_format.cpp | 27 +++++++++++++--\n> test/meson.build | 1 +\n> test/pixel-format.cpp | 49 ++++++++++++++++++++++++++++\n> 5 files changed, 113 insertions(+), 5 deletions(-)\n> create mode 100644 test/pixel-format.cpp\n> \n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index 4b172ef..8012721 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -46,6 +46,7 @@ public:\n> \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> \n> \t/* \\todo Add support for non-contiguous memory planes */\n> +\tstd::string name;\n\nI think this could be \"const char * name\" ?\n\nIf that's the only thing to update, (and if it should be) then I can\nupdate this when applying if we get more RB tags, so no need to repost\nunless there are other comments.\n\nReviewed-by: Kieran Bingham \n\nAlthough I do have a question below about std::hex vs utils::hex too..\n\n> \tPixelFormat format;\n> \tV4L2PixelFormat v4l2Format;\n> \tunsigned int bitsPerPixel;\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 97e9867..2908409 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -169,6 +169,7 @@ namespace {\n> const std::map pixelFormatInfo{\n> \t/* RGB formats. */\n> \t{ formats::BGR888, {\n> +\t\t.name = \"BGR888\",\n> \t\t.format = formats::BGR888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),\n> \t\t.bitsPerPixel = 24,\n> @@ -176,6 +177,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::RGB888, {\n> +\t\t.name = \"RGB888\",\n> \t\t.format = formats::RGB888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),\n> \t\t.bitsPerPixel = 24,\n> @@ -183,6 +185,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::ABGR8888, {\n> +\t\t.name = \"ABGR8888\",\n> \t\t.format = formats::ABGR8888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),\n> \t\t.bitsPerPixel = 32,\n> @@ -190,6 +193,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::ARGB8888, {\n> +\t\t.name = \"ARGB8888\",\n> \t\t.format = formats::ARGB8888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),\n> \t\t.bitsPerPixel = 32,\n> @@ -197,6 +201,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::BGRA8888, {\n> +\t\t.name = \"BGRA8888\",\n> \t\t.format = formats::BGRA8888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),\n> \t\t.bitsPerPixel = 32,\n> @@ -204,6 +209,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::RGBA8888, {\n> +\t\t.name = \"RGBA8888\",\n> \t\t.format = formats::RGBA8888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),\n> \t\t.bitsPerPixel = 32,\n> @@ -213,6 +219,7 @@ const std::map pixelFormatInfo{\n> \n> \t/* YUV packed formats. */\n> \t{ formats::YUYV, {\n> +\t\t.name = \"YUYV\",\n> \t\t.format = formats::YUYV,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),\n> \t\t.bitsPerPixel = 16,\n> @@ -220,6 +227,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::YVYU, {\n> +\t\t.name = \"YVYU\",\n> \t\t.format = formats::YVYU,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),\n> \t\t.bitsPerPixel = 16,\n> @@ -227,6 +235,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::UYVY, {\n> +\t\t.name = \"UYVY\",\n> \t\t.format = formats::UYVY,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),\n> \t\t.bitsPerPixel = 16,\n> @@ -234,6 +243,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::VYUY, {\n> +\t\t.name = \"VYUY\",\n> \t\t.format = formats::VYUY,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),\n> \t\t.bitsPerPixel = 16,\n> @@ -243,6 +253,7 @@ const std::map pixelFormatInfo{\n> \n> \t/* YUV planar formats. */\n> \t{ formats::NV16, {\n> +\t\t.name = \"NV16\",\n> \t\t.format = formats::NV16,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),\n> \t\t.bitsPerPixel = 16,\n> @@ -250,6 +261,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::NV61, {\n> +\t\t.name = \"NV61\",\n> \t\t.format = formats::NV61,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),\n> \t\t.bitsPerPixel = 16,\n> @@ -257,6 +269,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::NV12, {\n> +\t\t.name = \"NV12\",\n> \t\t.format = formats::NV12,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),\n> \t\t.bitsPerPixel = 12,\n> @@ -264,6 +277,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::NV21, {\n> +\t\t.name = \"NV21\",\n> \t\t.format = formats::NV21,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),\n> \t\t.bitsPerPixel = 12,\n> @@ -273,6 +287,7 @@ const std::map pixelFormatInfo{\n> \n> \t/* Greyscale formats. */\n> \t{ formats::R8, {\n> +\t\t.name = \"R8\",\n> \t\t.format = formats::R8,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),\n> \t\t.bitsPerPixel = 8,\n> @@ -282,6 +297,7 @@ const std::map pixelFormatInfo{\n> \n> \t/* Bayer formats. */\n> \t{ formats::SBGGR8, {\n> +\t\t.name = \"SBGGR8\",\n> \t\t.format = formats::SBGGR8,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),\n> \t\t.bitsPerPixel = 8,\n> @@ -289,6 +305,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SGBRG8, {\n> +\t\t.name = \"SGBRG8\",\n> \t\t.format = formats::SGBRG8,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),\n> \t\t.bitsPerPixel = 8,\n> @@ -296,6 +313,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SGRBG8, {\n> +\t\t.name = \"SGRBG8\",\n> \t\t.format = formats::SGRBG8,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),\n> \t\t.bitsPerPixel = 8,\n> @@ -303,6 +321,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SRGGB8, {\n> +\t\t.name = \"SRGGB8\",\n> \t\t.format = formats::SRGGB8,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),\n> \t\t.bitsPerPixel = 8,\n> @@ -310,6 +329,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SBGGR10, {\n> +\t\t.name = \"SBGGR10\",\n> \t\t.format = formats::SBGGR10,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),\n> \t\t.bitsPerPixel = 10,\n> @@ -317,6 +337,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SGBRG10, {\n> +\t\t.name = \"SGBRG10\",\n> \t\t.format = formats::SGBRG10,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),\n> \t\t.bitsPerPixel = 10,\n> @@ -324,6 +345,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SGRBG10, {\n> +\t\t.name = \"SGRBG10\",\n> \t\t.format = formats::SGRBG10,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),\n> \t\t.bitsPerPixel = 10,\n> @@ -331,6 +353,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SRGGB10, {\n> +\t\t.name = \"SRGGB10\",\n> \t\t.format = formats::SRGGB10,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),\n> \t\t.bitsPerPixel = 10,\n> @@ -338,6 +361,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SBGGR10_CSI2P, {\n> +\t\t.name = \"SBGGR10_CSI2P\",\n> \t\t.format = formats::SBGGR10_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),\n> \t\t.bitsPerPixel = 10,\n> @@ -345,6 +369,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = true,\n> \t} },\n> \t{ formats::SGBRG10_CSI2P, {\n> +\t\t.name = \"SGBRG10_CSI2P\",\n> \t\t.format = formats::SGBRG10_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),\n> \t\t.bitsPerPixel = 10,\n> @@ -352,6 +377,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = true,\n> \t} },\n> \t{ formats::SGRBG10_CSI2P, {\n> +\t\t.name = \"SGRBG10_CSI2P\",\n> \t\t.format = formats::SGRBG10_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),\n> \t\t.bitsPerPixel = 10,\n> @@ -359,6 +385,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = true,\n> \t} },\n> \t{ formats::SRGGB10_CSI2P, {\n> +\t\t.name = \"SRGGB10_CSI2P\",\n> \t\t.format = formats::SRGGB10_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),\n> \t\t.bitsPerPixel = 10,\n> @@ -366,6 +393,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = true,\n> \t} },\n> \t{ formats::SBGGR12, {\n> +\t\t.name = \"SBGGR12\",\n> \t\t.format = formats::SBGGR12,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),\n> \t\t.bitsPerPixel = 12,\n> @@ -373,6 +401,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SGBRG12, {\n> +\t\t.name = \"SGBRG12\",\n> \t\t.format = formats::SGBRG12,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),\n> \t\t.bitsPerPixel = 12,\n> @@ -380,6 +409,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SGRBG12, {\n> +\t\t.name = \"SGRBG12\",\n> \t\t.format = formats::SGRBG12,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),\n> \t\t.bitsPerPixel = 12,\n> @@ -387,6 +417,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SRGGB12, {\n> +\t\t.name = \"SRGGB12\",\n> \t\t.format = formats::SRGGB12,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),\n> \t\t.bitsPerPixel = 12,\n> @@ -394,6 +425,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = false,\n> \t} },\n> \t{ formats::SBGGR12_CSI2P, {\n> +\t\t.name = \"SBGGR12_CSI2P\",\n> \t\t.format = formats::SBGGR12_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),\n> \t\t.bitsPerPixel = 12,\n> @@ -401,6 +433,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = true,\n> \t} },\n> \t{ formats::SGBRG12_CSI2P, {\n> +\t\t.name = \"SGBRG12_CSI2P\",\n> \t\t.format = formats::SGBRG12_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),\n> \t\t.bitsPerPixel = 12,\n> @@ -408,6 +441,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = true,\n> \t} },\n> \t{ formats::SGRBG12_CSI2P, {\n> +\t\t.name = \"SGRBG12_CSI2P\",\n> \t\t.format = formats::SGRBG12_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),\n> \t\t.bitsPerPixel = 12,\n> @@ -415,6 +449,7 @@ const std::map pixelFormatInfo{\n> \t\t.packed = true,\n> \t} },\n> \t{ formats::SRGGB12_CSI2P, {\n> +\t\t.name = \"SRGGB12_CSI2P\",\n> \t\t.format = formats::SRGGB12_CSI2P,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),\n> \t\t.bitsPerPixel = 12,\n> @@ -424,6 +459,7 @@ const std::map pixelFormatInfo{\n> \n> \t/* Compressed formats. */\n> \t{ formats::MJPEG, {\n> +\t\t.name = \"MJPEG\",\n> \t\t.format = formats::MJPEG,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> \t\t.bitsPerPixel = 0,\n> @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)\n> \tconst auto iter = pixelFormatInfo.find(format);\n> \tif (iter == pixelFormatInfo.end()) {\n> \t\tLOG(Formats, Warning)\n> -\t\t\t<< \"Unsupported pixel format \"\n> -\t\t\t<< format.toString();\n> +\t\t\t<< \"Unsupported pixel format 0x\"\n> +\t\t\t<< std::hex << format.fourcc();\n\nDoes utils::hex(format.fourcc()) make any difference here?\n\n\n> \t\treturn invalid;\n> \t}\n> \n> diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp\n> index f191851..07e7af0 100644\n> --- a/src/libcamera/pixel_format.cpp\n> +++ b/src/libcamera/pixel_format.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \n> +#include \"libcamera/internal/formats.h\"\n> #include \n> \n> /**\n> @@ -104,9 +105,29 @@ 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> +\tPixelFormat format = PixelFormat(fourcc_, modifier_);\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> +\n> +\tif (!info.isValid()) {\n> +\t\tif (format == PixelFormat())\n> +\t\t\treturn \"\";\n> +\n> +\t\tchar fourcc[7] = { '<',\n> +\t\t\t\t static_cast(fourcc_ & 0x7f),\n> +\t\t\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> +\t\t\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> +\t\t\t\t static_cast((fourcc_ >> 24) & 0x7f),\n> +\t\t\t\t '>' };\n> +\n> +\t\tfor (unsigned int i = 1; i < 5; i++) {\n> +\t\t\tif (!isprint(fourcc[i]))\n> +\t\t\t\tfourcc[i] = '.';\n> +\t\t}\n> +\n> +\t\treturn fourcc;\n> +\t}\n> +\n> +\treturn info.name;\n> }\n> \n> } /* namespace libcamera */\n> diff --git a/test/meson.build b/test/meson.build\n> index a868813..7808a26 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -34,6 +34,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..5b9cdc9\n> --- /dev/null\n> +++ b/test/pixel-format.cpp\n> @@ -0,0 +1,49 @@\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 \n> +#include \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> formatsMap{\n> +\t\t\t{ formats::R8, \"R8\" },\n> +\t\t\t{ formats::SRGGB10_CSI2P, \"SRGGB10_CSI2P\" },\n> +\t\t\t{ PixelFormat(0, 0), \"\" },\n> +\t\t\t{ PixelFormat(0x20203843), \"\" }\n> +\t\t};\n> +\n> +\t\tfor (const auto &format : formatsMap) {\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[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A1CD600FE\n\tfor ;\n\tFri, 19 Jun 2020 23:40:18 +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 BDC69556;\n\tFri, 19 Jun 2020 23:40:17 +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=\"qSMpkoC8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592602817;\n\tbh=7CJg4z8CatFIpUz+O2UeW0CropoEHqQ9qD1a6qRmiqM=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=qSMpkoC8QcjqIUmcvvd3gaY+RFWhKDk6Z5jzdsDXu71VnykTYrCJkLP1ieyxJZ/i6\n\tc7mrZUPfGOA7ma40SAsjzHUS1Lahs/M7a8YdW+WKhQjyPKWh36OIXZdSkkpL91VRlr\n\tFyxMNN39QTC14GizmB9SZJmEXScfh9yhB2ueqEcE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta , libcamera-devel@lists.libcamera.org","References":"<20200619190631.GA13950@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":"Fri, 19 Jun 2020 22:40:15 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200619190631.GA13950@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","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":"Fri, 19 Jun 2020 21:40:18 -0000"}},{"id":5290,"web_url":"https://patchwork.libcamera.org/comment/5290/","msgid":"<20200619231056.GN5823@pendragon.ideasonboard.com>","date":"2020-06-19T23:10:56","subject":"Re: [libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Jun 19, 2020 at 10:40:15PM +0100, Kieran Bingham wrote:\n> On 19/06/2020 20:06, Kaaira Gupta wrote:\n> > Print format names defined in formats namespace instead of the hex\n> > values in toString() as they are easier to comprehend. For this add\n> > a property of 'name' in PixelFormatInfo so as to map the formats\n> > with their names. Print fourcc for formats which are not used in\n> > libcamera.\n> \n> Excellent, this is really readable now - and ties in nicely with the new\n> formats namespacing:\n> \n> >> Using camera VIMC Sensor B\n> >> [66:02:15.314555780] [1159464] INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888\n> >> [66:02:15.314729922] [1159464] INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888\n> >> Capture until user interrupts by SIGINT\n\nMuch nicer indeed ! Thank you Kaaira for your perseverance :-)\n\n> > Signed-off-by: Kaaira Gupta \n> > ---\n> > \n> > Changes since v4:\n> > \t-Print libcamera defined names instead of fourcc.\n> > \n> > Changes since v3:\n> > \t-shortened the texts.\n> > \t-Removed default case as well.\n> > \t-changed commit message and tests to reflect the changes.\n> > \n> > Changes since v2:\n> > - Remove description for all vendors except for MIPI\n> > - Change commit message to reflect this change.\n> > - Change tests accordingly.\n> > \n> > Changes since v1:\n> > - Replaced magic numbers with expressive values.\n> > - Re-wrote ARM vendor's modifiers\n> > - Re-wrote the vendors' map with a macro.\n> > - Changed the copyrights in test file.\n> > - Changed the tests.\n> > \n> > include/libcamera/internal/formats.h | 1 +\n> > src/libcamera/formats.cpp | 40 +++++++++++++++++++++--\n> > src/libcamera/pixel_format.cpp | 27 +++++++++++++--\n> > test/meson.build | 1 +\n> > test/pixel-format.cpp | 49 ++++++++++++++++++++++++++++\n> > 5 files changed, 113 insertions(+), 5 deletions(-)\n> > create mode 100644 test/pixel-format.cpp\n> > \n> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > index 4b172ef..8012721 100644\n> > --- a/include/libcamera/internal/formats.h\n> > +++ b/include/libcamera/internal/formats.h\n> > @@ -46,6 +46,7 @@ public:\n> > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > \n> > \t/* \\todo Add support for non-contiguous memory planes */\n> > +\tstd::string name;\n> \n> I think this could be \"const char * name\" ?\n\ns/\\* /*/\n\n> If that's the only thing to update, (and if it should be) then I can\n> update this when applying if we get more RB tags, so no need to repost\n> unless there are other comments.\n> \n> Reviewed-by: Kieran Bingham \n> \n> Although I do have a question below about std::hex vs utils::hex too..\n> \n> > \tPixelFormat format;\n> > \tV4L2PixelFormat v4l2Format;\n> > \tunsigned int bitsPerPixel;\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 97e9867..2908409 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -169,6 +169,7 @@ namespace {\n> > const std::map pixelFormatInfo{\n> > \t/* RGB formats. */\n> > \t{ formats::BGR888, {\n> > +\t\t.name = \"BGR888\",\n> > \t\t.format = formats::BGR888,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),\n> > \t\t.bitsPerPixel = 24,\n\nUnrelated to this patch, but I wonder if we should add more information\nto formats.yaml and generate this map.\n\n> > @@ -176,6 +177,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::RGB888, {\n> > +\t\t.name = \"RGB888\",\n> > \t\t.format = formats::RGB888,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),\n> > \t\t.bitsPerPixel = 24,\n> > @@ -183,6 +185,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::ABGR8888, {\n> > +\t\t.name = \"ABGR8888\",\n> > \t\t.format = formats::ABGR8888,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),\n> > \t\t.bitsPerPixel = 32,\n> > @@ -190,6 +193,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::ARGB8888, {\n> > +\t\t.name = \"ARGB8888\",\n> > \t\t.format = formats::ARGB8888,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),\n> > \t\t.bitsPerPixel = 32,\n> > @@ -197,6 +201,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::BGRA8888, {\n> > +\t\t.name = \"BGRA8888\",\n> > \t\t.format = formats::BGRA8888,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),\n> > \t\t.bitsPerPixel = 32,\n> > @@ -204,6 +209,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::RGBA8888, {\n> > +\t\t.name = \"RGBA8888\",\n> > \t\t.format = formats::RGBA8888,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),\n> > \t\t.bitsPerPixel = 32,\n> > @@ -213,6 +219,7 @@ const std::map pixelFormatInfo{\n> > \n> > \t/* YUV packed formats. */\n> > \t{ formats::YUYV, {\n> > +\t\t.name = \"YUYV\",\n> > \t\t.format = formats::YUYV,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),\n> > \t\t.bitsPerPixel = 16,\n> > @@ -220,6 +227,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::YVYU, {\n> > +\t\t.name = \"YVYU\",\n> > \t\t.format = formats::YVYU,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),\n> > \t\t.bitsPerPixel = 16,\n> > @@ -227,6 +235,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::UYVY, {\n> > +\t\t.name = \"UYVY\",\n> > \t\t.format = formats::UYVY,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),\n> > \t\t.bitsPerPixel = 16,\n> > @@ -234,6 +243,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::VYUY, {\n> > +\t\t.name = \"VYUY\",\n> > \t\t.format = formats::VYUY,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),\n> > \t\t.bitsPerPixel = 16,\n> > @@ -243,6 +253,7 @@ const std::map pixelFormatInfo{\n> > \n> > \t/* YUV planar formats. */\n> > \t{ formats::NV16, {\n> > +\t\t.name = \"NV16\",\n> > \t\t.format = formats::NV16,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),\n> > \t\t.bitsPerPixel = 16,\n> > @@ -250,6 +261,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::NV61, {\n> > +\t\t.name = \"NV61\",\n> > \t\t.format = formats::NV61,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),\n> > \t\t.bitsPerPixel = 16,\n> > @@ -257,6 +269,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::NV12, {\n> > +\t\t.name = \"NV12\",\n> > \t\t.format = formats::NV12,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -264,6 +277,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::NV21, {\n> > +\t\t.name = \"NV21\",\n> > \t\t.format = formats::NV21,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -273,6 +287,7 @@ const std::map pixelFormatInfo{\n> > \n> > \t/* Greyscale formats. */\n> > \t{ formats::R8, {\n> > +\t\t.name = \"R8\",\n> > \t\t.format = formats::R8,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),\n> > \t\t.bitsPerPixel = 8,\n> > @@ -282,6 +297,7 @@ const std::map pixelFormatInfo{\n> > \n> > \t/* Bayer formats. */\n> > \t{ formats::SBGGR8, {\n> > +\t\t.name = \"SBGGR8\",\n> > \t\t.format = formats::SBGGR8,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),\n> > \t\t.bitsPerPixel = 8,\n> > @@ -289,6 +305,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SGBRG8, {\n> > +\t\t.name = \"SGBRG8\",\n> > \t\t.format = formats::SGBRG8,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),\n> > \t\t.bitsPerPixel = 8,\n> > @@ -296,6 +313,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SGRBG8, {\n> > +\t\t.name = \"SGRBG8\",\n> > \t\t.format = formats::SGRBG8,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),\n> > \t\t.bitsPerPixel = 8,\n> > @@ -303,6 +321,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SRGGB8, {\n> > +\t\t.name = \"SRGGB8\",\n> > \t\t.format = formats::SRGGB8,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),\n> > \t\t.bitsPerPixel = 8,\n> > @@ -310,6 +329,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SBGGR10, {\n> > +\t\t.name = \"SBGGR10\",\n> > \t\t.format = formats::SBGGR10,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -317,6 +337,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SGBRG10, {\n> > +\t\t.name = \"SGBRG10\",\n> > \t\t.format = formats::SGBRG10,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -324,6 +345,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SGRBG10, {\n> > +\t\t.name = \"SGRBG10\",\n> > \t\t.format = formats::SGRBG10,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -331,6 +353,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SRGGB10, {\n> > +\t\t.name = \"SRGGB10\",\n> > \t\t.format = formats::SRGGB10,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -338,6 +361,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SBGGR10_CSI2P, {\n> > +\t\t.name = \"SBGGR10_CSI2P\",\n> > \t\t.format = formats::SBGGR10_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -345,6 +369,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = true,\n> > \t} },\n> > \t{ formats::SGBRG10_CSI2P, {\n> > +\t\t.name = \"SGBRG10_CSI2P\",\n> > \t\t.format = formats::SGBRG10_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -352,6 +377,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = true,\n> > \t} },\n> > \t{ formats::SGRBG10_CSI2P, {\n> > +\t\t.name = \"SGRBG10_CSI2P\",\n> > \t\t.format = formats::SGRBG10_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -359,6 +385,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = true,\n> > \t} },\n> > \t{ formats::SRGGB10_CSI2P, {\n> > +\t\t.name = \"SRGGB10_CSI2P\",\n> > \t\t.format = formats::SRGGB10_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),\n> > \t\t.bitsPerPixel = 10,\n> > @@ -366,6 +393,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = true,\n> > \t} },\n> > \t{ formats::SBGGR12, {\n> > +\t\t.name = \"SBGGR12\",\n> > \t\t.format = formats::SBGGR12,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -373,6 +401,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SGBRG12, {\n> > +\t\t.name = \"SGBRG12\",\n> > \t\t.format = formats::SGBRG12,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -380,6 +409,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SGRBG12, {\n> > +\t\t.name = \"SGRBG12\",\n> > \t\t.format = formats::SGRBG12,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -387,6 +417,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SRGGB12, {\n> > +\t\t.name = \"SRGGB12\",\n> > \t\t.format = formats::SRGGB12,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -394,6 +425,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = false,\n> > \t} },\n> > \t{ formats::SBGGR12_CSI2P, {\n> > +\t\t.name = \"SBGGR12_CSI2P\",\n> > \t\t.format = formats::SBGGR12_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -401,6 +433,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = true,\n> > \t} },\n> > \t{ formats::SGBRG12_CSI2P, {\n> > +\t\t.name = \"SGBRG12_CSI2P\",\n> > \t\t.format = formats::SGBRG12_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -408,6 +441,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = true,\n> > \t} },\n> > \t{ formats::SGRBG12_CSI2P, {\n> > +\t\t.name = \"SGRBG12_CSI2P\",\n> > \t\t.format = formats::SGRBG12_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -415,6 +449,7 @@ const std::map pixelFormatInfo{\n> > \t\t.packed = true,\n> > \t} },\n> > \t{ formats::SRGGB12_CSI2P, {\n> > +\t\t.name = \"SRGGB12_CSI2P\",\n> > \t\t.format = formats::SRGGB12_CSI2P,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),\n> > \t\t.bitsPerPixel = 12,\n> > @@ -424,6 +459,7 @@ const std::map pixelFormatInfo{\n> > \n> > \t/* Compressed formats. */\n> > \t{ formats::MJPEG, {\n> > +\t\t.name = \"MJPEG\",\n> > \t\t.format = formats::MJPEG,\n> > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> > \t\t.bitsPerPixel = 0,\n> > @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)\n> > \tconst auto iter = pixelFormatInfo.find(format);\n> > \tif (iter == pixelFormatInfo.end()) {\n> > \t\tLOG(Formats, Warning)\n> > -\t\t\t<< \"Unsupported pixel format \"\n> > -\t\t\t<< format.toString();\n> > +\t\t\t<< \"Unsupported pixel format 0x\"\n> > +\t\t\t<< std::hex << format.fourcc();\n> \n> Does utils::hex(format.fourcc()) make any difference here?\n\nutils::hex() will automatically pad the output string to the size of the\nfield (8 characters in this case). I think it would be nicer to use it\nhere. It also resets the formatting options of the stream to avoid\nhaving to add a std::dec after the argument, but that won't make a\ndifference here as the FourCC is last.\n\n> > \t\treturn invalid;\n> > \t}\n> > \n> > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp\n> > index f191851..07e7af0 100644\n> > --- a/src/libcamera/pixel_format.cpp\n> > +++ b/src/libcamera/pixel_format.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> > \n> > #include \n> > +#include \"libcamera/internal/formats.h\"\n> > #include \n\nWe put the internal headers after the external ones, with a blank line\nbetween the two groups.\n\n> > \n> > /**\n> > @@ -104,9 +105,29 @@ 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> > +\tPixelFormat format = PixelFormat(fourcc_, modifier_);\n> > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n\nHow about\n\n\tconst PixelFormatInfo &info = PixelFormatInfo::info(*this);\n\n> > +\n> > +\tif (!info.isValid()) {\n> > +\t\tif (format == PixelFormat())\n\nAnd here,\n\n\t\tif (!isValid())\n\nWith that, you can drop the local format variable.\n\n> > +\t\t\treturn \"\";\n> > +\n> > +\t\tchar fourcc[7] = { '<',\n> > +\t\t\t\t static_cast(fourcc_ & 0x7f),\n> > +\t\t\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> > +\t\t\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> > +\t\t\t\t static_cast((fourcc_ >> 24) & 0x7f),\n\nThe isprint() should take care of non-printable characters, do we need\nthe & 0x7f ?\n\n> > +\t\t\t\t '>' };\n> > +\n> > +\t\tfor (unsigned int i = 1; i < 5; i++) {\n> > +\t\t\tif (!isprint(fourcc[i]))\n> > +\t\t\t\tfourcc[i] = '.';\n> > +\t\t}\n> > +\n> > +\t\treturn fourcc;\n> > +\t}\n> > +\n> > +\treturn info.name;\n> > }\n> > \n> > } /* namespace libcamera */\n> > diff --git a/test/meson.build b/test/meson.build\n> > index a868813..7808a26 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -34,6 +34,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..5b9cdc9\n> > --- /dev/null\n> > +++ b/test/pixel-format.cpp\n> > @@ -0,0 +1,49 @@\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 \n> > +#include \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> formatsMap{\n> > +\t\t\t{ formats::R8, \"R8\" },\n> > +\t\t\t{ formats::SRGGB10_CSI2P, \"SRGGB10_CSI2P\" },\n> > +\t\t\t{ PixelFormat(0, 0), \"\" },\n> > +\t\t\t{ PixelFormat(0x20203843), \"\" }\n> > +\t\t};\n> > +\n> > +\t\tfor (const auto &format : formatsMap) {\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\nMaybe utils::hex(format.first.fourcc()) ?\n\nOnly minor changes, with these addressed,\n\nReviewed-by: Laurent Pinchart \n\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)","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 15ED360710\n\tfor ;\n\tSat, 20 Jun 2020 01:11:21 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69D0D552;\n\tSat, 20 Jun 2020 01:11:20 +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=\"QZxIlUQB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592608280;\n\tbh=weTFhCVPmMtuhh6LFSLmqRcSYXXSD92ZLHv3H7G4SOM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QZxIlUQBJ1gyQyWxBvmZiZthzm4v/ZdBY5BPKPGvaN+VeaY4u99bq+79CcFuaujeJ\n\tE+f/rk6pj6bVS7OXFFi/POGCtFQ5HTlczxA6BUYe5UDpmhEhEEoDCrFi8VhAvQDx+A\n\tGuRWa2Z/yptSJV/Fc5mSDpvtTnLenw2BexEj4m5Y=","Date":"Sat, 20 Jun 2020 02:10:56 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Kaaira Gupta , libcamera-devel@lists.libcamera.org","Message-ID":"<20200619231056.GN5823@pendragon.ideasonboard.com>","References":"<20200619190631.GA13950@kaaira-HP-Pavilion-Notebook>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","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":"Fri, 19 Jun 2020 23:11:21 -0000"}},{"id":5306,"web_url":"https://patchwork.libcamera.org/comment/5306/","msgid":"<20200621201433.GA11144@kaaira-HP-Pavilion-Notebook>","date":"2020-06-21T20:14:33","subject":"[libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","submitter":{"id":39,"url":"https://patchwork.libcamera.org/api/people/39/","name":"Kaaira Gupta","email":"kgupta@es.iitr.ac.in"},"content":"On Sat, Jun 20, 2020 at 02:10:56AM +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Fri, Jun 19, 2020 at 10:40:15PM +0100, Kieran Bingham wrote:\n> > On 19/06/2020 20:06, Kaaira Gupta wrote:\n> > > Print format names defined in formats namespace instead of the hex\n> > > values in toString() as they are easier to comprehend. For this add\n> > > a property of 'name' in PixelFormatInfo so as to map the formats\n> > > with their names. Print fourcc for formats which are not used in\n> > > libcamera.\n> > \n> > Excellent, this is really readable now - and ties in nicely with the new\n> > formats namespacing:\n> > \n> > >> Using camera VIMC Sensor B\n> > >> [66:02:15.314555780] [1159464] INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888\n> > >> [66:02:15.314729922] [1159464] INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888\n> > >> Capture until user interrupts by SIGINT\n> \n> Much nicer indeed ! Thank you Kaaira for your perseverance :-)\n> \n> > > Signed-off-by: Kaaira Gupta \n> > > ---\n> > > \n> > > Changes since v4:\n> > > \t-Print libcamera defined names instead of fourcc.\n> > > \n> > > Changes since v3:\n> > > \t-shortened the texts.\n> > > \t-Removed default case as well.\n> > > \t-changed commit message and tests to reflect the changes.\n> > > \n> > > Changes since v2:\n> > > - Remove description for all vendors except for MIPI\n> > > - Change commit message to reflect this change.\n> > > - Change tests accordingly.\n> > > \n> > > Changes since v1:\n> > > - Replaced magic numbers with expressive values.\n> > > - Re-wrote ARM vendor's modifiers\n> > > - Re-wrote the vendors' map with a macro.\n> > > - Changed the copyrights in test file.\n> > > - Changed the tests.\n> > > \n> > > include/libcamera/internal/formats.h | 1 +\n> > > src/libcamera/formats.cpp | 40 +++++++++++++++++++++--\n> > > src/libcamera/pixel_format.cpp | 27 +++++++++++++--\n> > > test/meson.build | 1 +\n> > > test/pixel-format.cpp | 49 ++++++++++++++++++++++++++++\n> > > 5 files changed, 113 insertions(+), 5 deletions(-)\n> > > create mode 100644 test/pixel-format.cpp\n> > > \n> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > > index 4b172ef..8012721 100644\n> > > --- a/include/libcamera/internal/formats.h\n> > > +++ b/include/libcamera/internal/formats.h\n> > > @@ -46,6 +46,7 @@ public:\n> > > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > > \n> > > \t/* \\todo Add support for non-contiguous memory planes */\n> > > +\tstd::string name;\n> > \n> > I think this could be \"const char * name\" ?\n> \n> s/\\* /*/\n> \n> > If that's the only thing to update, (and if it should be) then I can\n> > update this when applying if we get more RB tags, so no need to repost\n> > unless there are other comments.\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > Although I do have a question below about std::hex vs utils::hex too..\n\nI didn't know the difference (Now I do), I was just used to using std:: \nhence I used that\n\n> > \n> > > \tPixelFormat format;\n> > > \tV4L2PixelFormat v4l2Format;\n> > > \tunsigned int bitsPerPixel;\n> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > > index 97e9867..2908409 100644\n> > > --- a/src/libcamera/formats.cpp\n> > > +++ b/src/libcamera/formats.cpp\n> > > @@ -169,6 +169,7 @@ namespace {\n> > > const std::map pixelFormatInfo{\n> > > \t/* RGB formats. */\n> > > \t{ formats::BGR888, {\n> > > +\t\t.name = \"BGR888\",\n> > > \t\t.format = formats::BGR888,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),\n> > > \t\t.bitsPerPixel = 24,\n> \n> Unrelated to this patch, but I wonder if we should add more information\n> to formats.yaml and generate this map.\n> \n> > > @@ -176,6 +177,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::RGB888, {\n> > > +\t\t.name = \"RGB888\",\n> > > \t\t.format = formats::RGB888,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),\n> > > \t\t.bitsPerPixel = 24,\n> > > @@ -183,6 +185,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::ABGR8888, {\n> > > +\t\t.name = \"ABGR8888\",\n> > > \t\t.format = formats::ABGR8888,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),\n> > > \t\t.bitsPerPixel = 32,\n> > > @@ -190,6 +193,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::ARGB8888, {\n> > > +\t\t.name = \"ARGB8888\",\n> > > \t\t.format = formats::ARGB8888,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),\n> > > \t\t.bitsPerPixel = 32,\n> > > @@ -197,6 +201,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::BGRA8888, {\n> > > +\t\t.name = \"BGRA8888\",\n> > > \t\t.format = formats::BGRA8888,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),\n> > > \t\t.bitsPerPixel = 32,\n> > > @@ -204,6 +209,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::RGBA8888, {\n> > > +\t\t.name = \"RGBA8888\",\n> > > \t\t.format = formats::RGBA8888,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),\n> > > \t\t.bitsPerPixel = 32,\n> > > @@ -213,6 +219,7 @@ const std::map pixelFormatInfo{\n> > > \n> > > \t/* YUV packed formats. */\n> > > \t{ formats::YUYV, {\n> > > +\t\t.name = \"YUYV\",\n> > > \t\t.format = formats::YUYV,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),\n> > > \t\t.bitsPerPixel = 16,\n> > > @@ -220,6 +227,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::YVYU, {\n> > > +\t\t.name = \"YVYU\",\n> > > \t\t.format = formats::YVYU,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),\n> > > \t\t.bitsPerPixel = 16,\n> > > @@ -227,6 +235,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::UYVY, {\n> > > +\t\t.name = \"UYVY\",\n> > > \t\t.format = formats::UYVY,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),\n> > > \t\t.bitsPerPixel = 16,\n> > > @@ -234,6 +243,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::VYUY, {\n> > > +\t\t.name = \"VYUY\",\n> > > \t\t.format = formats::VYUY,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),\n> > > \t\t.bitsPerPixel = 16,\n> > > @@ -243,6 +253,7 @@ const std::map pixelFormatInfo{\n> > > \n> > > \t/* YUV planar formats. */\n> > > \t{ formats::NV16, {\n> > > +\t\t.name = \"NV16\",\n> > > \t\t.format = formats::NV16,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),\n> > > \t\t.bitsPerPixel = 16,\n> > > @@ -250,6 +261,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::NV61, {\n> > > +\t\t.name = \"NV61\",\n> > > \t\t.format = formats::NV61,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),\n> > > \t\t.bitsPerPixel = 16,\n> > > @@ -257,6 +269,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::NV12, {\n> > > +\t\t.name = \"NV12\",\n> > > \t\t.format = formats::NV12,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -264,6 +277,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::NV21, {\n> > > +\t\t.name = \"NV21\",\n> > > \t\t.format = formats::NV21,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -273,6 +287,7 @@ const std::map pixelFormatInfo{\n> > > \n> > > \t/* Greyscale formats. */\n> > > \t{ formats::R8, {\n> > > +\t\t.name = \"R8\",\n> > > \t\t.format = formats::R8,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),\n> > > \t\t.bitsPerPixel = 8,\n> > > @@ -282,6 +297,7 @@ const std::map pixelFormatInfo{\n> > > \n> > > \t/* Bayer formats. */\n> > > \t{ formats::SBGGR8, {\n> > > +\t\t.name = \"SBGGR8\",\n> > > \t\t.format = formats::SBGGR8,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),\n> > > \t\t.bitsPerPixel = 8,\n> > > @@ -289,6 +305,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SGBRG8, {\n> > > +\t\t.name = \"SGBRG8\",\n> > > \t\t.format = formats::SGBRG8,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),\n> > > \t\t.bitsPerPixel = 8,\n> > > @@ -296,6 +313,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SGRBG8, {\n> > > +\t\t.name = \"SGRBG8\",\n> > > \t\t.format = formats::SGRBG8,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),\n> > > \t\t.bitsPerPixel = 8,\n> > > @@ -303,6 +321,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SRGGB8, {\n> > > +\t\t.name = \"SRGGB8\",\n> > > \t\t.format = formats::SRGGB8,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),\n> > > \t\t.bitsPerPixel = 8,\n> > > @@ -310,6 +329,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SBGGR10, {\n> > > +\t\t.name = \"SBGGR10\",\n> > > \t\t.format = formats::SBGGR10,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -317,6 +337,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SGBRG10, {\n> > > +\t\t.name = \"SGBRG10\",\n> > > \t\t.format = formats::SGBRG10,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -324,6 +345,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SGRBG10, {\n> > > +\t\t.name = \"SGRBG10\",\n> > > \t\t.format = formats::SGRBG10,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -331,6 +353,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SRGGB10, {\n> > > +\t\t.name = \"SRGGB10\",\n> > > \t\t.format = formats::SRGGB10,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -338,6 +361,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SBGGR10_CSI2P, {\n> > > +\t\t.name = \"SBGGR10_CSI2P\",\n> > > \t\t.format = formats::SBGGR10_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -345,6 +369,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = true,\n> > > \t} },\n> > > \t{ formats::SGBRG10_CSI2P, {\n> > > +\t\t.name = \"SGBRG10_CSI2P\",\n> > > \t\t.format = formats::SGBRG10_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -352,6 +377,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = true,\n> > > \t} },\n> > > \t{ formats::SGRBG10_CSI2P, {\n> > > +\t\t.name = \"SGRBG10_CSI2P\",\n> > > \t\t.format = formats::SGRBG10_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -359,6 +385,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = true,\n> > > \t} },\n> > > \t{ formats::SRGGB10_CSI2P, {\n> > > +\t\t.name = \"SRGGB10_CSI2P\",\n> > > \t\t.format = formats::SRGGB10_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),\n> > > \t\t.bitsPerPixel = 10,\n> > > @@ -366,6 +393,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = true,\n> > > \t} },\n> > > \t{ formats::SBGGR12, {\n> > > +\t\t.name = \"SBGGR12\",\n> > > \t\t.format = formats::SBGGR12,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -373,6 +401,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SGBRG12, {\n> > > +\t\t.name = \"SGBRG12\",\n> > > \t\t.format = formats::SGBRG12,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -380,6 +409,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SGRBG12, {\n> > > +\t\t.name = \"SGRBG12\",\n> > > \t\t.format = formats::SGRBG12,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -387,6 +417,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SRGGB12, {\n> > > +\t\t.name = \"SRGGB12\",\n> > > \t\t.format = formats::SRGGB12,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -394,6 +425,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = false,\n> > > \t} },\n> > > \t{ formats::SBGGR12_CSI2P, {\n> > > +\t\t.name = \"SBGGR12_CSI2P\",\n> > > \t\t.format = formats::SBGGR12_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -401,6 +433,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = true,\n> > > \t} },\n> > > \t{ formats::SGBRG12_CSI2P, {\n> > > +\t\t.name = \"SGBRG12_CSI2P\",\n> > > \t\t.format = formats::SGBRG12_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -408,6 +441,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = true,\n> > > \t} },\n> > > \t{ formats::SGRBG12_CSI2P, {\n> > > +\t\t.name = \"SGRBG12_CSI2P\",\n> > > \t\t.format = formats::SGRBG12_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -415,6 +449,7 @@ const std::map pixelFormatInfo{\n> > > \t\t.packed = true,\n> > > \t} },\n> > > \t{ formats::SRGGB12_CSI2P, {\n> > > +\t\t.name = \"SRGGB12_CSI2P\",\n> > > \t\t.format = formats::SRGGB12_CSI2P,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),\n> > > \t\t.bitsPerPixel = 12,\n> > > @@ -424,6 +459,7 @@ const std::map pixelFormatInfo{\n> > > \n> > > \t/* Compressed formats. */\n> > > \t{ formats::MJPEG, {\n> > > +\t\t.name = \"MJPEG\",\n> > > \t\t.format = formats::MJPEG,\n> > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> > > \t\t.bitsPerPixel = 0,\n> > > @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)\n> > > \tconst auto iter = pixelFormatInfo.find(format);\n> > > \tif (iter == pixelFormatInfo.end()) {\n> > > \t\tLOG(Formats, Warning)\n> > > -\t\t\t<< \"Unsupported pixel format \"\n> > > -\t\t\t<< format.toString();\n> > > +\t\t\t<< \"Unsupported pixel format 0x\"\n> > > +\t\t\t<< std::hex << format.fourcc();\n> > \n> > Does utils::hex(format.fourcc()) make any difference here?\n> \n> utils::hex() will automatically pad the output string to the size of the\n> field (8 characters in this case). I think it would be nicer to use it\n> here. It also resets the formatting options of the stream to avoid\n> having to add a std::dec after the argument, but that won't make a\n> difference here as the FourCC is last.\n\nOkay i'll make the change\n\n> \n> > > \t\treturn invalid;\n> > > \t}\n> > > \n> > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp\n> > > index f191851..07e7af0 100644\n> > > --- a/src/libcamera/pixel_format.cpp\n> > > +++ b/src/libcamera/pixel_format.cpp\n> > > @@ -6,6 +6,7 @@\n> > > */\n> > > \n> > > #include \n> > > +#include \"libcamera/internal/formats.h\"\n> > > #include \n> \n> We put the internal headers after the external ones, with a blank line\n> between the two groups.\n\nNoted\n\n> \n> > > \n> > > /**\n> > > @@ -104,9 +105,29 @@ 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> > > +\tPixelFormat format = PixelFormat(fourcc_, modifier_);\n> > > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> \n> How about\n> \n> \tconst PixelFormatInfo &info = PixelFormatInfo::info(*this);\n> \n> > > +\n> > > +\tif (!info.isValid()) {\n> > > +\t\tif (format == PixelFormat())\n> \n> And here,\n> \n> \t\tif (!isValid())\n> \n> With that, you can drop the local format variable.\n\nI didn't think that way. Thanks, will change it\n\n> \n> > > +\t\t\treturn \"\";\n> > > +\n> > > +\t\tchar fourcc[7] = { '<',\n> > > +\t\t\t\t static_cast(fourcc_ & 0x7f),\n> > > +\t\t\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> > > +\t\t\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> > > +\t\t\t\t static_cast((fourcc_ >> 24) & 0x7f),\n> \n> The isprint() should take care of non-printable characters, do we need\n> the & 0x7f ?\n\nDon't we need & 0x7f to check for ascii character? I think we can't\ncheck for non-printable non-ascii characters with isprint(), so won't it\nbreak while checking if the passed character is non-ascii?\nidk I am not sure\n\n> \n> > > +\t\t\t\t '>' };\n> > > +\n> > > +\t\tfor (unsigned int i = 1; i < 5; i++) {\n> > > +\t\t\tif (!isprint(fourcc[i]))\n> > > +\t\t\t\tfourcc[i] = '.';\n> > > +\t\t}\n> > > +\n> > > +\t\treturn fourcc;\n> > > +\t}\n> > > +\n> > > +\treturn info.name;\n> > > }\n> > > \n> > > } /* namespace libcamera */\n> > > diff --git a/test/meson.build b/test/meson.build\n> > > index a868813..7808a26 100644\n> > > --- a/test/meson.build\n> > > +++ b/test/meson.build\n> > > @@ -34,6 +34,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..5b9cdc9\n> > > --- /dev/null\n> > > +++ b/test/pixel-format.cpp\n> > > @@ -0,0 +1,49 @@\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 \n> > > +#include \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> formatsMap{\n> > > +\t\t\t{ formats::R8, \"R8\" },\n> > > +\t\t\t{ formats::SRGGB10_CSI2P, \"SRGGB10_CSI2P\" },\n> > > +\t\t\t{ PixelFormat(0, 0), \"\" },\n> > > +\t\t\t{ PixelFormat(0x20203843), \"\" }\n> > > +\t\t};\n> > > +\n> > > +\t\tfor (const auto &format : formatsMap) {\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> \n> Maybe utils::hex(format.first.fourcc()) ?\n> \n> Only minor changes, with these addressed,\n> \n> Reviewed-by: Laurent Pinchart \n> \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> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-pf1-x429.google.com (mail-pf1-x429.google.com\n\t[IPv6:2607:f8b0:4864:20::429])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3576A609A5\n\tfor ;\n\tSun, 21 Jun 2020 22:14:43 +0200 (CEST)","by mail-pf1-x429.google.com with SMTP id b201so7373382pfb.0\n\tfor ;\n\tSun, 21 Jun 2020 13:14:43 -0700 (PDT)","from kaaira-HP-Pavilion-Notebook ([103.113.213.178])\n\tby smtp.gmail.com with ESMTPSA id\n\tj16sm3042279pgb.33.2020.06.21.13.14.37\n\t(version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 21 Jun 2020 13:14:40 -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=\"VsGd5S19\"; \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=yAUcf6lCxgpusgXkToHHojs0Lq9VOOCs7wASsvmGm7Y=;\n\tb=VsGd5S19Eo6YYHLJNZXckil1PrZC3dbYJ3deZC68A/uhrEXv+nVDtlO/0mmBoroVwH\n\tL0gvqfsGT76Xe01WG3f2iLbiWuiukwiMwPwWxok6lp1szWRfh5NWM87EobaYpbDDyJLa\n\tmdGyzuuT3+MFC9BDjO8T69OmpmpqYbM+6BXRriKG5MgNKDnjM4E5Yrj2bd2FBpPgWto8\n\t8UHGnURL1MePQ/KlLcrIO2gq3jk5B1lhQcH0Z8GKzRhnEa7LEd1Xom9Z2VwYmiXGqHy2\n\tBmOXFN4ZMjcrOwSbbSaWvNN/asIpQzR6QjtfUFksCuPqXS5KDfeAYudsW+12l1nMs84w\n\ttEmA==","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=yAUcf6lCxgpusgXkToHHojs0Lq9VOOCs7wASsvmGm7Y=;\n\tb=XdtikwXWBFa4QacXCu5c0UUwABiIN9ruPM2YCe2lqqN9jhdP5q4JQEF578xbC6ehfz\n\tdbe0k/J9SuYVeoBYh8/k776s/HnBWRtFxfYDNmk7n/jgUUInVFkbvPXQ2fQVbbCBdrhg\n\tdPGsXMjbRIRq5+Rs74OQnQcZ3EftRpXjvUT2m6U21VYvv7W3jvCatdM0FgkobzpOepbX\n\tn8Q86xboiu6zqRsVikK+1U5B69rLLEFVgEzFiNmQFT8KotCARg0oCutFeoKFHUcG5Z1w\n\tVGqw9viTB8pcjXnpVdjWASb4oJjC3WPalQdN87Zo1YN2aapdCv8mO9p5DkLgyi0+v58J\n\tvM/Q==","X-Gm-Message-State":"AOAM531MnyS2bdaOIN9Pt1T0XNGXDqJ4vdcHCJAk1Zjv6GRLCDvvWOyk\n\t5ibbXsrrNAtnK1ArlWQzRcIbvx3q8X0mpA==","X-Google-Smtp-Source":"ABdhPJzpYKKZ3LZI8+EhElaVNEN0pAAIqLWE6zXiy/O3//mIB6og+J7nVX5MOJe99X5Zog7eVX6z6g==","X-Received":"by 2002:a62:c105:: with SMTP id\n\ti5mr17680722pfg.250.1592770480998; \n\tSun, 21 Jun 2020 13:14:40 -0700 (PDT)","From":"Kaaira Gupta ","X-Google-Original-From":"Kaaira Gupta ","Date":"Mon, 22 Jun 2020 01:44:33 +0530","To":"Laurent Pinchart ,\n\tKieran Bingham ","Cc":"Kaaira Gupta , libcamera-devel@lists.libcamera.org","Message-ID":"<20200621201433.GA11144@kaaira-HP-Pavilion-Notebook>","References":"<20200619190631.GA13950@kaaira-HP-Pavilion-Notebook>\n\t\n\t<20200619231056.GN5823@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20200619231056.GN5823@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.9.4 (2018-02-28)","Subject":"[libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","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":"Sun, 21 Jun 2020 20:14:44 -0000"}},{"id":5307,"web_url":"https://patchwork.libcamera.org/comment/5307/","msgid":"<20200621221040.GC5962@pendragon.ideasonboard.com>","date":"2020-06-21T22:10:40","subject":"Re: [libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn Mon, Jun 22, 2020 at 01:44:33AM +0530, Kaaira Gupta wrote:\n> On Sat, Jun 20, 2020 at 02:10:56AM +0300, Laurent Pinchart wrote:\n> > On Fri, Jun 19, 2020 at 10:40:15PM +0100, Kieran Bingham wrote:\n> > > On 19/06/2020 20:06, Kaaira Gupta wrote:\n> > > > Print format names defined in formats namespace instead of the hex\n> > > > values in toString() as they are easier to comprehend. For this add\n> > > > a property of 'name' in PixelFormatInfo so as to map the formats\n> > > > with their names. Print fourcc for formats which are not used in\n> > > > libcamera.\n> > > \n> > > Excellent, this is really readable now - and ties in nicely with the new\n> > > formats namespacing:\n> > > \n> > > >> Using camera VIMC Sensor B\n> > > >> [66:02:15.314555780] [1159464] INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888\n> > > >> [66:02:15.314729922] [1159464] INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888\n> > > >> Capture until user interrupts by SIGINT\n> > \n> > Much nicer indeed ! Thank you Kaaira for your perseverance :-)\n> > \n> > > > Signed-off-by: Kaaira Gupta \n> > > > ---\n> > > > \n> > > > Changes since v4:\n> > > > \t-Print libcamera defined names instead of fourcc.\n> > > > \n> > > > Changes since v3:\n> > > > \t-shortened the texts.\n> > > > \t-Removed default case as well.\n> > > > \t-changed commit message and tests to reflect the changes.\n> > > > \n> > > > Changes since v2:\n> > > > - Remove description for all vendors except for MIPI\n> > > > - Change commit message to reflect this change.\n> > > > - Change tests accordingly.\n> > > > \n> > > > Changes since v1:\n> > > > - Replaced magic numbers with expressive values.\n> > > > - Re-wrote ARM vendor's modifiers\n> > > > - Re-wrote the vendors' map with a macro.\n> > > > - Changed the copyrights in test file.\n> > > > - Changed the tests.\n> > > > \n> > > > include/libcamera/internal/formats.h | 1 +\n> > > > src/libcamera/formats.cpp | 40 +++++++++++++++++++++--\n> > > > src/libcamera/pixel_format.cpp | 27 +++++++++++++--\n> > > > test/meson.build | 1 +\n> > > > test/pixel-format.cpp | 49 ++++++++++++++++++++++++++++\n> > > > 5 files changed, 113 insertions(+), 5 deletions(-)\n> > > > create mode 100644 test/pixel-format.cpp\n> > > > \n> > > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > > > index 4b172ef..8012721 100644\n> > > > --- a/include/libcamera/internal/formats.h\n> > > > +++ b/include/libcamera/internal/formats.h\n> > > > @@ -46,6 +46,7 @@ public:\n> > > > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > > > \n> > > > \t/* \\todo Add support for non-contiguous memory planes */\n> > > > +\tstd::string name;\n> > > \n> > > I think this could be \"const char * name\" ?\n> > \n> > s/\\* /*/\n> > \n> > > If that's the only thing to update, (and if it should be) then I can\n> > > update this when applying if we get more RB tags, so no need to repost\n> > > unless there are other comments.\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > Although I do have a question below about std::hex vs utils::hex too..\n> \n> I didn't know the difference (Now I do), I was just used to using std:: \n> hence I used that\n> \n> > > \n> > > > \tPixelFormat format;\n> > > > \tV4L2PixelFormat v4l2Format;\n> > > > \tunsigned int bitsPerPixel;\n> > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > > > index 97e9867..2908409 100644\n> > > > --- a/src/libcamera/formats.cpp\n> > > > +++ b/src/libcamera/formats.cpp\n> > > > @@ -169,6 +169,7 @@ namespace {\n> > > > const std::map pixelFormatInfo{\n> > > > \t/* RGB formats. */\n> > > > \t{ formats::BGR888, {\n> > > > +\t\t.name = \"BGR888\",\n> > > > \t\t.format = formats::BGR888,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),\n> > > > \t\t.bitsPerPixel = 24,\n> > \n> > Unrelated to this patch, but I wonder if we should add more information\n> > to formats.yaml and generate this map.\n> > \n> > > > @@ -176,6 +177,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::RGB888, {\n> > > > +\t\t.name = \"RGB888\",\n> > > > \t\t.format = formats::RGB888,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),\n> > > > \t\t.bitsPerPixel = 24,\n> > > > @@ -183,6 +185,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::ABGR8888, {\n> > > > +\t\t.name = \"ABGR8888\",\n> > > > \t\t.format = formats::ABGR8888,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),\n> > > > \t\t.bitsPerPixel = 32,\n> > > > @@ -190,6 +193,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::ARGB8888, {\n> > > > +\t\t.name = \"ARGB8888\",\n> > > > \t\t.format = formats::ARGB8888,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),\n> > > > \t\t.bitsPerPixel = 32,\n> > > > @@ -197,6 +201,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::BGRA8888, {\n> > > > +\t\t.name = \"BGRA8888\",\n> > > > \t\t.format = formats::BGRA8888,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),\n> > > > \t\t.bitsPerPixel = 32,\n> > > > @@ -204,6 +209,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::RGBA8888, {\n> > > > +\t\t.name = \"RGBA8888\",\n> > > > \t\t.format = formats::RGBA8888,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),\n> > > > \t\t.bitsPerPixel = 32,\n> > > > @@ -213,6 +219,7 @@ const std::map pixelFormatInfo{\n> > > > \n> > > > \t/* YUV packed formats. */\n> > > > \t{ formats::YUYV, {\n> > > > +\t\t.name = \"YUYV\",\n> > > > \t\t.format = formats::YUYV,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),\n> > > > \t\t.bitsPerPixel = 16,\n> > > > @@ -220,6 +227,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::YVYU, {\n> > > > +\t\t.name = \"YVYU\",\n> > > > \t\t.format = formats::YVYU,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),\n> > > > \t\t.bitsPerPixel = 16,\n> > > > @@ -227,6 +235,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::UYVY, {\n> > > > +\t\t.name = \"UYVY\",\n> > > > \t\t.format = formats::UYVY,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),\n> > > > \t\t.bitsPerPixel = 16,\n> > > > @@ -234,6 +243,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::VYUY, {\n> > > > +\t\t.name = \"VYUY\",\n> > > > \t\t.format = formats::VYUY,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),\n> > > > \t\t.bitsPerPixel = 16,\n> > > > @@ -243,6 +253,7 @@ const std::map pixelFormatInfo{\n> > > > \n> > > > \t/* YUV planar formats. */\n> > > > \t{ formats::NV16, {\n> > > > +\t\t.name = \"NV16\",\n> > > > \t\t.format = formats::NV16,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),\n> > > > \t\t.bitsPerPixel = 16,\n> > > > @@ -250,6 +261,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::NV61, {\n> > > > +\t\t.name = \"NV61\",\n> > > > \t\t.format = formats::NV61,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),\n> > > > \t\t.bitsPerPixel = 16,\n> > > > @@ -257,6 +269,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::NV12, {\n> > > > +\t\t.name = \"NV12\",\n> > > > \t\t.format = formats::NV12,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -264,6 +277,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::NV21, {\n> > > > +\t\t.name = \"NV21\",\n> > > > \t\t.format = formats::NV21,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -273,6 +287,7 @@ const std::map pixelFormatInfo{\n> > > > \n> > > > \t/* Greyscale formats. */\n> > > > \t{ formats::R8, {\n> > > > +\t\t.name = \"R8\",\n> > > > \t\t.format = formats::R8,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),\n> > > > \t\t.bitsPerPixel = 8,\n> > > > @@ -282,6 +297,7 @@ const std::map pixelFormatInfo{\n> > > > \n> > > > \t/* Bayer formats. */\n> > > > \t{ formats::SBGGR8, {\n> > > > +\t\t.name = \"SBGGR8\",\n> > > > \t\t.format = formats::SBGGR8,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),\n> > > > \t\t.bitsPerPixel = 8,\n> > > > @@ -289,6 +305,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SGBRG8, {\n> > > > +\t\t.name = \"SGBRG8\",\n> > > > \t\t.format = formats::SGBRG8,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),\n> > > > \t\t.bitsPerPixel = 8,\n> > > > @@ -296,6 +313,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SGRBG8, {\n> > > > +\t\t.name = \"SGRBG8\",\n> > > > \t\t.format = formats::SGRBG8,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),\n> > > > \t\t.bitsPerPixel = 8,\n> > > > @@ -303,6 +321,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SRGGB8, {\n> > > > +\t\t.name = \"SRGGB8\",\n> > > > \t\t.format = formats::SRGGB8,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),\n> > > > \t\t.bitsPerPixel = 8,\n> > > > @@ -310,6 +329,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SBGGR10, {\n> > > > +\t\t.name = \"SBGGR10\",\n> > > > \t\t.format = formats::SBGGR10,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -317,6 +337,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SGBRG10, {\n> > > > +\t\t.name = \"SGBRG10\",\n> > > > \t\t.format = formats::SGBRG10,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -324,6 +345,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SGRBG10, {\n> > > > +\t\t.name = \"SGRBG10\",\n> > > > \t\t.format = formats::SGRBG10,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -331,6 +353,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SRGGB10, {\n> > > > +\t\t.name = \"SRGGB10\",\n> > > > \t\t.format = formats::SRGGB10,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -338,6 +361,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SBGGR10_CSI2P, {\n> > > > +\t\t.name = \"SBGGR10_CSI2P\",\n> > > > \t\t.format = formats::SBGGR10_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -345,6 +369,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = true,\n> > > > \t} },\n> > > > \t{ formats::SGBRG10_CSI2P, {\n> > > > +\t\t.name = \"SGBRG10_CSI2P\",\n> > > > \t\t.format = formats::SGBRG10_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -352,6 +377,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = true,\n> > > > \t} },\n> > > > \t{ formats::SGRBG10_CSI2P, {\n> > > > +\t\t.name = \"SGRBG10_CSI2P\",\n> > > > \t\t.format = formats::SGRBG10_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -359,6 +385,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = true,\n> > > > \t} },\n> > > > \t{ formats::SRGGB10_CSI2P, {\n> > > > +\t\t.name = \"SRGGB10_CSI2P\",\n> > > > \t\t.format = formats::SRGGB10_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),\n> > > > \t\t.bitsPerPixel = 10,\n> > > > @@ -366,6 +393,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = true,\n> > > > \t} },\n> > > > \t{ formats::SBGGR12, {\n> > > > +\t\t.name = \"SBGGR12\",\n> > > > \t\t.format = formats::SBGGR12,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -373,6 +401,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SGBRG12, {\n> > > > +\t\t.name = \"SGBRG12\",\n> > > > \t\t.format = formats::SGBRG12,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -380,6 +409,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SGRBG12, {\n> > > > +\t\t.name = \"SGRBG12\",\n> > > > \t\t.format = formats::SGRBG12,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -387,6 +417,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SRGGB12, {\n> > > > +\t\t.name = \"SRGGB12\",\n> > > > \t\t.format = formats::SRGGB12,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -394,6 +425,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = false,\n> > > > \t} },\n> > > > \t{ formats::SBGGR12_CSI2P, {\n> > > > +\t\t.name = \"SBGGR12_CSI2P\",\n> > > > \t\t.format = formats::SBGGR12_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -401,6 +433,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = true,\n> > > > \t} },\n> > > > \t{ formats::SGBRG12_CSI2P, {\n> > > > +\t\t.name = \"SGBRG12_CSI2P\",\n> > > > \t\t.format = formats::SGBRG12_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -408,6 +441,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = true,\n> > > > \t} },\n> > > > \t{ formats::SGRBG12_CSI2P, {\n> > > > +\t\t.name = \"SGRBG12_CSI2P\",\n> > > > \t\t.format = formats::SGRBG12_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -415,6 +449,7 @@ const std::map pixelFormatInfo{\n> > > > \t\t.packed = true,\n> > > > \t} },\n> > > > \t{ formats::SRGGB12_CSI2P, {\n> > > > +\t\t.name = \"SRGGB12_CSI2P\",\n> > > > \t\t.format = formats::SRGGB12_CSI2P,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),\n> > > > \t\t.bitsPerPixel = 12,\n> > > > @@ -424,6 +459,7 @@ const std::map pixelFormatInfo{\n> > > > \n> > > > \t/* Compressed formats. */\n> > > > \t{ formats::MJPEG, {\n> > > > +\t\t.name = \"MJPEG\",\n> > > > \t\t.format = formats::MJPEG,\n> > > > \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),\n> > > > \t\t.bitsPerPixel = 0,\n> > > > @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)\n> > > > \tconst auto iter = pixelFormatInfo.find(format);\n> > > > \tif (iter == pixelFormatInfo.end()) {\n> > > > \t\tLOG(Formats, Warning)\n> > > > -\t\t\t<< \"Unsupported pixel format \"\n> > > > -\t\t\t<< format.toString();\n> > > > +\t\t\t<< \"Unsupported pixel format 0x\"\n> > > > +\t\t\t<< std::hex << format.fourcc();\n> > > \n> > > Does utils::hex(format.fourcc()) make any difference here?\n> > \n> > utils::hex() will automatically pad the output string to the size of the\n> > field (8 characters in this case). I think it would be nicer to use it\n> > here. It also resets the formatting options of the stream to avoid\n> > having to add a std::dec after the argument, but that won't make a\n> > difference here as the FourCC is last.\n> \n> Okay i'll make the change\n> \n> > \n> > > > \t\treturn invalid;\n> > > > \t}\n> > > > \n> > > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp\n> > > > index f191851..07e7af0 100644\n> > > > --- a/src/libcamera/pixel_format.cpp\n> > > > +++ b/src/libcamera/pixel_format.cpp\n> > > > @@ -6,6 +6,7 @@\n> > > > */\n> > > > \n> > > > #include \n> > > > +#include \"libcamera/internal/formats.h\"\n> > > > #include \n> > \n> > We put the internal headers after the external ones, with a blank line\n> > between the two groups.\n> \n> Noted\n> \n> > \n> > > > \n> > > > /**\n> > > > @@ -104,9 +105,29 @@ 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> > > > +\tPixelFormat format = PixelFormat(fourcc_, modifier_);\n> > > > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> > \n> > How about\n> > \n> > \tconst PixelFormatInfo &info = PixelFormatInfo::info(*this);\n> > \n> > > > +\n> > > > +\tif (!info.isValid()) {\n> > > > +\t\tif (format == PixelFormat())\n> > \n> > And here,\n> > \n> > \t\tif (!isValid())\n> > \n> > With that, you can drop the local format variable.\n> \n> I didn't think that way. Thanks, will change it\n> \n> > > > +\t\t\treturn \"\";\n> > > > +\n> > > > +\t\tchar fourcc[7] = { '<',\n> > > > +\t\t\t\t static_cast(fourcc_ & 0x7f),\n> > > > +\t\t\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> > > > +\t\t\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> > > > +\t\t\t\t static_cast((fourcc_ >> 24) & 0x7f),\n> > \n> > The isprint() should take care of non-printable characters, do we need\n> > the & 0x7f ?\n> \n> Don't we need & 0x7f to check for ascii character? I think we can't\n> check for non-printable non-ascii characters with isprint(), so won't it\n> break while checking if the passed character is non-ascii?\n> idk I am not sure\n\nThe behaviour of isprint() is locale-specific, but in any case it should\nguarantee that the character is printable, and it accepts any value\nbetween 0 and 255 (technically any value that is representable by an\nunsigned char). It should thus be safe.\n\n> > > > +\t\t\t\t '>' };\n> > > > +\n> > > > +\t\tfor (unsigned int i = 1; i < 5; i++) {\n> > > > +\t\t\tif (!isprint(fourcc[i]))\n> > > > +\t\t\t\tfourcc[i] = '.';\n> > > > +\t\t}\n> > > > +\n> > > > +\t\treturn fourcc;\n> > > > +\t}\n> > > > +\n> > > > +\treturn info.name;\n> > > > }\n> > > > \n> > > > } /* namespace libcamera */\n> > > > diff --git a/test/meson.build b/test/meson.build\n> > > > index a868813..7808a26 100644\n> > > > --- a/test/meson.build\n> > > > +++ b/test/meson.build\n> > > > @@ -34,6 +34,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..5b9cdc9\n> > > > --- /dev/null\n> > > > +++ b/test/pixel-format.cpp\n> > > > @@ -0,0 +1,49 @@\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 \n> > > > +#include \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> formatsMap{\n> > > > +\t\t\t{ formats::R8, \"R8\" },\n> > > > +\t\t\t{ formats::SRGGB10_CSI2P, \"SRGGB10_CSI2P\" },\n> > > > +\t\t\t{ PixelFormat(0, 0), \"\" },\n> > > > +\t\t\t{ PixelFormat(0x20203843), \"\" }\n> > > > +\t\t};\n> > > > +\n> > > > +\t\tfor (const auto &format : formatsMap) {\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> > \n> > Maybe utils::hex(format.first.fourcc()) ?\n> > \n> > Only minor changes, with these addressed,\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \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)","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 83B6A609A3\n\tfor ;\n\tMon, 22 Jun 2020 00:11:05 +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 D0D45A5E;\n\tMon, 22 Jun 2020 00:11:04 +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=\"a8gPiF3r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592777465;\n\tbh=/4vvA+D3MJnT4YUNFmUiAkYrx29+u9p6pg9hrs2pa68=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a8gPiF3r3SpCQzIlW7uvefc9TpBvqh1cibVHCqF37zDbZvaJHjfmlIzS1yRLLTNPn\n\t4pqIBlPxJfGXOdCShgZ8TrBFoY5hP69DguAJ8bANGF4TI9ByFtZ1oWjoiPdOGCXEc0\n\tWz5JZJxfyd/a9QwSxm7XjNzAr9q0bdCbwu38i4iM=","Date":"Mon, 22 Jun 2020 01:10:40 +0300","From":"Laurent Pinchart ","To":"Kaaira Gupta ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200621221040.GC5962@pendragon.ideasonboard.com>","References":"<20200619190631.GA13950@kaaira-HP-Pavilion-Notebook>\n\t\n\t<20200619231056.GN5823@pendragon.ideasonboard.com>\n\t<20200621201433.GA11144@kaaira-HP-Pavilion-Notebook>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200621201433.GA11144@kaaira-HP-Pavilion-Notebook>","Subject":"Re: [libcamera-devel] [PATCH v5] libcamera:PixelFormat: Replace hex\n\twith format names","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":"Sun, 21 Jun 2020 22:11:05 -0000"}}]