[libcamera-devel,v5] libcamera:PixelFormat: Replace hex with format names
diff mbox series

Message ID 20200619190631.GA13950@kaaira-HP-Pavilion-Notebook
State Superseded
Headers show
Series
  • [libcamera-devel,v5] libcamera:PixelFormat: Replace hex with format names
Related show

Commit Message

Kaaira Gupta June 19, 2020, 7:06 p.m. UTC 
Print format names defined in formats namespace instead of the hex
values in toString() as they are easier to comprehend. For this add
a property of 'name' in PixelFormatInfo so as to map the formats
with their names. Print fourcc for formats which are not used in
libcamera.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---

Changes since v4:
	-Print libcamera defined names instead of fourcc.

Changes since v3:
	-shortened the texts.
	-Removed default case as well.
	-changed commit message and tests to reflect the changes.

Changes since v2:
        - Remove description for all vendors except for MIPI
        - Change commit message to reflect this change.
        - Change tests accordingly.

Changes since v1:
        - Replaced magic numbers with expressive values.
        - Re-wrote ARM vendor's modifiers
        - Re-wrote the vendors' map with a macro.
        - Changed the copyrights in test file.
        - Changed the tests.

 include/libcamera/internal/formats.h |  1 +
 src/libcamera/formats.cpp            | 40 +++++++++++++++++++++--
 src/libcamera/pixel_format.cpp       | 27 +++++++++++++--
 test/meson.build                     |  1 +
 test/pixel-format.cpp                | 49 ++++++++++++++++++++++++++++
 5 files changed, 113 insertions(+), 5 deletions(-)
 create mode 100644 test/pixel-format.cpp

Comments

Kieran Bingham June 19, 2020, 9:40 p.m. UTC | #1 
Hi Kaaira,

On 19/06/2020 20:06, Kaaira Gupta wrote:
> Print format names defined in formats namespace instead of the hex
> values in toString() as they are easier to comprehend. For this add
> a property of 'name' in PixelFormatInfo so as to map the formats
> with their names. Print fourcc for formats which are not used in
> libcamera.


Excellent, this is really readable now - and ties in nicely with the new
formats namespacing:

>> Using camera VIMC Sensor B
>> [66:02:15.314555780] [1159464]  INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888
>> [66:02:15.314729922] [1159464]  INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888
>> Capture until user interrupts by SIGINT


> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
> 
> Changes since v4:
> 	-Print libcamera defined names instead of fourcc.
> 
> Changes since v3:
> 	-shortened the texts.
> 	-Removed default case as well.
> 	-changed commit message and tests to reflect the changes.
> 
> Changes since v2:
>         - Remove description for all vendors except for MIPI
>         - Change commit message to reflect this change.
>         - Change tests accordingly.
> 
> Changes since v1:
>         - Replaced magic numbers with expressive values.
>         - Re-wrote ARM vendor's modifiers
>         - Re-wrote the vendors' map with a macro.
>         - Changed the copyrights in test file.
>         - Changed the tests.
> 
>  include/libcamera/internal/formats.h |  1 +
>  src/libcamera/formats.cpp            | 40 +++++++++++++++++++++--
>  src/libcamera/pixel_format.cpp       | 27 +++++++++++++--
>  test/meson.build                     |  1 +
>  test/pixel-format.cpp                | 49 ++++++++++++++++++++++++++++
>  5 files changed, 113 insertions(+), 5 deletions(-)
>  create mode 100644 test/pixel-format.cpp
> 
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index 4b172ef..8012721 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -46,6 +46,7 @@ public:
>  	static const PixelFormatInfo &info(const PixelFormat &format);
>  
>  	/* \todo Add support for non-contiguous memory planes */
> +	std::string name;

I think this could be "const char * name" ?

If that's the only thing to update, (and if it should be) then I can
update this when applying if we get more RB tags, so no need to repost
unless there are other comments.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Although I do have a question below about std::hex vs utils::hex too..

>  	PixelFormat format;
>  	V4L2PixelFormat v4l2Format;
>  	unsigned int bitsPerPixel;
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 97e9867..2908409 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -169,6 +169,7 @@ namespace {
>  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  	/* RGB formats. */
>  	{ formats::BGR888, {
> +		.name = "BGR888",
>  		.format = formats::BGR888,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),
>  		.bitsPerPixel = 24,
> @@ -176,6 +177,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::RGB888, {
> +		.name = "RGB888",
>  		.format = formats::RGB888,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),
>  		.bitsPerPixel = 24,
> @@ -183,6 +185,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::ABGR8888, {
> +		.name = "ABGR8888",
>  		.format = formats::ABGR8888,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),
>  		.bitsPerPixel = 32,
> @@ -190,6 +193,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::ARGB8888, {
> +		.name = "ARGB8888",
>  		.format = formats::ARGB8888,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),
>  		.bitsPerPixel = 32,
> @@ -197,6 +201,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::BGRA8888, {
> +		.name = "BGRA8888",
>  		.format = formats::BGRA8888,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),
>  		.bitsPerPixel = 32,
> @@ -204,6 +209,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::RGBA8888, {
> +		.name = "RGBA8888",
>  		.format = formats::RGBA8888,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),
>  		.bitsPerPixel = 32,
> @@ -213,6 +219,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  
>  	/* YUV packed formats. */
>  	{ formats::YUYV, {
> +		.name = "YUYV",
>  		.format = formats::YUYV,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),
>  		.bitsPerPixel = 16,
> @@ -220,6 +227,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::YVYU, {
> +		.name = "YVYU",
>  		.format = formats::YVYU,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),
>  		.bitsPerPixel = 16,
> @@ -227,6 +235,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::UYVY, {
> +		.name = "UYVY",
>  		.format = formats::UYVY,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),
>  		.bitsPerPixel = 16,
> @@ -234,6 +243,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::VYUY, {
> +		.name = "VYUY",
>  		.format = formats::VYUY,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),
>  		.bitsPerPixel = 16,
> @@ -243,6 +253,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  
>  	/* YUV planar formats. */
>  	{ formats::NV16, {
> +		.name = "NV16",
>  		.format = formats::NV16,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),
>  		.bitsPerPixel = 16,
> @@ -250,6 +261,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::NV61, {
> +		.name = "NV61",
>  		.format = formats::NV61,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),
>  		.bitsPerPixel = 16,
> @@ -257,6 +269,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::NV12, {
> +		.name = "NV12",
>  		.format = formats::NV12,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),
>  		.bitsPerPixel = 12,
> @@ -264,6 +277,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::NV21, {
> +		.name = "NV21",
>  		.format = formats::NV21,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),
>  		.bitsPerPixel = 12,
> @@ -273,6 +287,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  
>  	/* Greyscale formats. */
>  	{ formats::R8, {
> +		.name = "R8",
>  		.format = formats::R8,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),
>  		.bitsPerPixel = 8,
> @@ -282,6 +297,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  
>  	/* Bayer formats. */
>  	{ formats::SBGGR8, {
> +		.name = "SBGGR8",
>  		.format = formats::SBGGR8,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),
>  		.bitsPerPixel = 8,
> @@ -289,6 +305,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SGBRG8, {
> +		.name = "SGBRG8",
>  		.format = formats::SGBRG8,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),
>  		.bitsPerPixel = 8,
> @@ -296,6 +313,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SGRBG8, {
> +		.name = "SGRBG8",
>  		.format = formats::SGRBG8,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),
>  		.bitsPerPixel = 8,
> @@ -303,6 +321,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SRGGB8, {
> +		.name = "SRGGB8",
>  		.format = formats::SRGGB8,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),
>  		.bitsPerPixel = 8,
> @@ -310,6 +329,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SBGGR10, {
> +		.name = "SBGGR10",
>  		.format = formats::SBGGR10,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),
>  		.bitsPerPixel = 10,
> @@ -317,6 +337,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SGBRG10, {
> +		.name = "SGBRG10",
>  		.format = formats::SGBRG10,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),
>  		.bitsPerPixel = 10,
> @@ -324,6 +345,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SGRBG10, {
> +		.name = "SGRBG10",
>  		.format = formats::SGRBG10,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),
>  		.bitsPerPixel = 10,
> @@ -331,6 +353,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SRGGB10, {
> +		.name = "SRGGB10",
>  		.format = formats::SRGGB10,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),
>  		.bitsPerPixel = 10,
> @@ -338,6 +361,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SBGGR10_CSI2P, {
> +		.name = "SBGGR10_CSI2P",
>  		.format = formats::SBGGR10_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),
>  		.bitsPerPixel = 10,
> @@ -345,6 +369,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = true,
>  	} },
>  	{ formats::SGBRG10_CSI2P, {
> +		.name = "SGBRG10_CSI2P",
>  		.format = formats::SGBRG10_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),
>  		.bitsPerPixel = 10,
> @@ -352,6 +377,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = true,
>  	} },
>  	{ formats::SGRBG10_CSI2P, {
> +		.name = "SGRBG10_CSI2P",
>  		.format = formats::SGRBG10_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),
>  		.bitsPerPixel = 10,
> @@ -359,6 +385,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = true,
>  	} },
>  	{ formats::SRGGB10_CSI2P, {
> +		.name = "SRGGB10_CSI2P",
>  		.format = formats::SRGGB10_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),
>  		.bitsPerPixel = 10,
> @@ -366,6 +393,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = true,
>  	} },
>  	{ formats::SBGGR12, {
> +		.name = "SBGGR12",
>  		.format = formats::SBGGR12,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),
>  		.bitsPerPixel = 12,
> @@ -373,6 +401,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SGBRG12, {
> +		.name = "SGBRG12",
>  		.format = formats::SGBRG12,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),
>  		.bitsPerPixel = 12,
> @@ -380,6 +409,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SGRBG12, {
> +		.name = "SGRBG12",
>  		.format = formats::SGRBG12,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),
>  		.bitsPerPixel = 12,
> @@ -387,6 +417,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SRGGB12, {
> +		.name = "SRGGB12",
>  		.format = formats::SRGGB12,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),
>  		.bitsPerPixel = 12,
> @@ -394,6 +425,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = false,
>  	} },
>  	{ formats::SBGGR12_CSI2P, {
> +		.name = "SBGGR12_CSI2P",
>  		.format = formats::SBGGR12_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),
>  		.bitsPerPixel = 12,
> @@ -401,6 +433,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = true,
>  	} },
>  	{ formats::SGBRG12_CSI2P, {
> +		.name = "SGBRG12_CSI2P",
>  		.format = formats::SGBRG12_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),
>  		.bitsPerPixel = 12,
> @@ -408,6 +441,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = true,
>  	} },
>  	{ formats::SGRBG12_CSI2P, {
> +		.name = "SGRBG12_CSI2P",
>  		.format = formats::SGRBG12_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),
>  		.bitsPerPixel = 12,
> @@ -415,6 +449,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.packed = true,
>  	} },
>  	{ formats::SRGGB12_CSI2P, {
> +		.name = "SRGGB12_CSI2P",
>  		.format = formats::SRGGB12_CSI2P,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),
>  		.bitsPerPixel = 12,
> @@ -424,6 +459,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  
>  	/* Compressed formats. */
>  	{ formats::MJPEG, {
> +		.name = "MJPEG",
>  		.format = formats::MJPEG,
>  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
>  		.bitsPerPixel = 0,
> @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
>  	const auto iter = pixelFormatInfo.find(format);
>  	if (iter == pixelFormatInfo.end()) {
>  		LOG(Formats, Warning)
> -			<< "Unsupported pixel format "
> -			<< format.toString();
> +			<< "Unsupported pixel format 0x"
> +			<< std::hex << format.fourcc();

Does utils::hex(format.fourcc()) make any difference here?


>  		return invalid;
>  	}
>  
> diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
> index f191851..07e7af0 100644
> --- a/src/libcamera/pixel_format.cpp
> +++ b/src/libcamera/pixel_format.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/formats.h>
> +#include "libcamera/internal/formats.h"
>  #include <libcamera/pixel_format.h>
>  
>  /**
> @@ -104,9 +105,29 @@ bool PixelFormat::operator<(const PixelFormat &other) const
>   */
>  std::string PixelFormat::toString() const
>  {
> -	char str[11];
> -	snprintf(str, 11, "0x%08x", fourcc_);
> -	return str;
> +	PixelFormat format = PixelFormat(fourcc_, modifier_);
> +	const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +
> +	if (!info.isValid()) {
> +		if (format == PixelFormat())
> +			return "<INVALID>";
> +
> +		char fourcc[7] = { '<',
> +				   static_cast<char>(fourcc_ & 0x7f),
> +				   static_cast<char>((fourcc_ >> 8) & 0x7f),
> +				   static_cast<char>((fourcc_ >> 16) & 0x7f),
> +				   static_cast<char>((fourcc_ >> 24) & 0x7f),
> +				   '>' };
> +
> +		for (unsigned int i = 1; i < 5; i++) {
> +			if (!isprint(fourcc[i]))
> +				fourcc[i] = '.';
> +		}
> +
> +		return fourcc;
> +	}
> +
> +	return info.name;
>  }
>  
>  } /* namespace libcamera */
> diff --git a/test/meson.build b/test/meson.build
> index a868813..7808a26 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -34,6 +34,7 @@ internal_tests = [
>      ['message',                         'message.cpp'],
>      ['object',                          'object.cpp'],
>      ['object-invoke',                   'object-invoke.cpp'],
> +    ['pixel-format',                    'pixel-format.cpp'],
>      ['signal-threads',                  'signal-threads.cpp'],
>      ['threads',                         'threads.cpp'],
>      ['timer',                           'timer.cpp'],
> diff --git a/test/pixel-format.cpp b/test/pixel-format.cpp
> new file mode 100644
> index 0000000..5b9cdc9
> --- /dev/null
> +++ b/test/pixel-format.cpp
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Kaaira Gupta
> + * libcamera pixel format handling test
> + */
> +
> +#include <iostream>
> +#include <vector>
> +
> +#include <libcamera/formats.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class PixelFormatTest : public Test
> +{
> +protected:
> +	int run()
> +	{
> +		std::vector<std::pair<PixelFormat, const char *>> formatsMap{
> +			{ formats::R8, "R8" },
> +			{ formats::SRGGB10_CSI2P, "SRGGB10_CSI2P" },
> +			{ PixelFormat(0, 0), "<INVALID>" },
> +			{ PixelFormat(0x20203843), "<C8  >" }
> +		};
> +
> +		for (const auto &format : formatsMap) {
> +			if ((format.first).toString() != format.second) {
> +				cerr << "Failed to convert PixelFormat "
> +				     << format.first.fourcc() << " to string"
> +				     << endl;
> +				return TestFail;
> +			}
> +		}
> +
> +		if (PixelFormat().toString() != "<INVALID>") {
> +			cerr << "Failed to convert default PixelFormat to string"
> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +};
> +
> +TEST_REGISTER(PixelFormatTest)
>
Laurent Pinchart June 19, 2020, 11:10 p.m. UTC | #2 
Hello,

On Fri, Jun 19, 2020 at 10:40:15PM +0100, Kieran Bingham wrote:
> On 19/06/2020 20:06, Kaaira Gupta wrote:
> > Print format names defined in formats namespace instead of the hex
> > values in toString() as they are easier to comprehend. For this add
> > a property of 'name' in PixelFormatInfo so as to map the formats
> > with their names. Print fourcc for formats which are not used in
> > libcamera.
> 
> Excellent, this is really readable now - and ties in nicely with the new
> formats namespacing:
> 
> >> Using camera VIMC Sensor B
> >> [66:02:15.314555780] [1159464]  INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888
> >> [66:02:15.314729922] [1159464]  INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888
> >> Capture until user interrupts by SIGINT

Much nicer indeed ! Thank you Kaaira for your perseverance :-)

> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> > 
> > Changes since v4:
> > 	-Print libcamera defined names instead of fourcc.
> > 
> > Changes since v3:
> > 	-shortened the texts.
> > 	-Removed default case as well.
> > 	-changed commit message and tests to reflect the changes.
> > 
> > Changes since v2:
> >         - Remove description for all vendors except for MIPI
> >         - Change commit message to reflect this change.
> >         - Change tests accordingly.
> > 
> > Changes since v1:
> >         - Replaced magic numbers with expressive values.
> >         - Re-wrote ARM vendor's modifiers
> >         - Re-wrote the vendors' map with a macro.
> >         - Changed the copyrights in test file.
> >         - Changed the tests.
> > 
> >  include/libcamera/internal/formats.h |  1 +
> >  src/libcamera/formats.cpp            | 40 +++++++++++++++++++++--
> >  src/libcamera/pixel_format.cpp       | 27 +++++++++++++--
> >  test/meson.build                     |  1 +
> >  test/pixel-format.cpp                | 49 ++++++++++++++++++++++++++++
> >  5 files changed, 113 insertions(+), 5 deletions(-)
> >  create mode 100644 test/pixel-format.cpp
> > 
> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > index 4b172ef..8012721 100644
> > --- a/include/libcamera/internal/formats.h
> > +++ b/include/libcamera/internal/formats.h
> > @@ -46,6 +46,7 @@ public:
> >  	static const PixelFormatInfo &info(const PixelFormat &format);
> >  
> >  	/* \todo Add support for non-contiguous memory planes */
> > +	std::string name;
> 
> I think this could be "const char * name" ?

s/\* /*/

> If that's the only thing to update, (and if it should be) then I can
> update this when applying if we get more RB tags, so no need to repost
> unless there are other comments.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Although I do have a question below about std::hex vs utils::hex too..
> 
> >  	PixelFormat format;
> >  	V4L2PixelFormat v4l2Format;
> >  	unsigned int bitsPerPixel;
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 97e9867..2908409 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -169,6 +169,7 @@ namespace {
> >  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  	/* RGB formats. */
> >  	{ formats::BGR888, {
> > +		.name = "BGR888",
> >  		.format = formats::BGR888,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),
> >  		.bitsPerPixel = 24,

Unrelated to this patch, but I wonder if we should add more information
to formats.yaml and generate this map.

> > @@ -176,6 +177,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::RGB888, {
> > +		.name = "RGB888",
> >  		.format = formats::RGB888,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),
> >  		.bitsPerPixel = 24,
> > @@ -183,6 +185,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::ABGR8888, {
> > +		.name = "ABGR8888",
> >  		.format = formats::ABGR8888,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),
> >  		.bitsPerPixel = 32,
> > @@ -190,6 +193,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::ARGB8888, {
> > +		.name = "ARGB8888",
> >  		.format = formats::ARGB8888,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),
> >  		.bitsPerPixel = 32,
> > @@ -197,6 +201,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::BGRA8888, {
> > +		.name = "BGRA8888",
> >  		.format = formats::BGRA8888,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),
> >  		.bitsPerPixel = 32,
> > @@ -204,6 +209,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::RGBA8888, {
> > +		.name = "RGBA8888",
> >  		.format = formats::RGBA8888,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),
> >  		.bitsPerPixel = 32,
> > @@ -213,6 +219,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  
> >  	/* YUV packed formats. */
> >  	{ formats::YUYV, {
> > +		.name = "YUYV",
> >  		.format = formats::YUYV,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),
> >  		.bitsPerPixel = 16,
> > @@ -220,6 +227,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::YVYU, {
> > +		.name = "YVYU",
> >  		.format = formats::YVYU,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),
> >  		.bitsPerPixel = 16,
> > @@ -227,6 +235,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::UYVY, {
> > +		.name = "UYVY",
> >  		.format = formats::UYVY,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),
> >  		.bitsPerPixel = 16,
> > @@ -234,6 +243,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::VYUY, {
> > +		.name = "VYUY",
> >  		.format = formats::VYUY,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),
> >  		.bitsPerPixel = 16,
> > @@ -243,6 +253,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  
> >  	/* YUV planar formats. */
> >  	{ formats::NV16, {
> > +		.name = "NV16",
> >  		.format = formats::NV16,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),
> >  		.bitsPerPixel = 16,
> > @@ -250,6 +261,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::NV61, {
> > +		.name = "NV61",
> >  		.format = formats::NV61,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),
> >  		.bitsPerPixel = 16,
> > @@ -257,6 +269,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::NV12, {
> > +		.name = "NV12",
> >  		.format = formats::NV12,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),
> >  		.bitsPerPixel = 12,
> > @@ -264,6 +277,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::NV21, {
> > +		.name = "NV21",
> >  		.format = formats::NV21,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),
> >  		.bitsPerPixel = 12,
> > @@ -273,6 +287,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  
> >  	/* Greyscale formats. */
> >  	{ formats::R8, {
> > +		.name = "R8",
> >  		.format = formats::R8,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),
> >  		.bitsPerPixel = 8,
> > @@ -282,6 +297,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  
> >  	/* Bayer formats. */
> >  	{ formats::SBGGR8, {
> > +		.name = "SBGGR8",
> >  		.format = formats::SBGGR8,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),
> >  		.bitsPerPixel = 8,
> > @@ -289,6 +305,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SGBRG8, {
> > +		.name = "SGBRG8",
> >  		.format = formats::SGBRG8,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),
> >  		.bitsPerPixel = 8,
> > @@ -296,6 +313,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SGRBG8, {
> > +		.name = "SGRBG8",
> >  		.format = formats::SGRBG8,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),
> >  		.bitsPerPixel = 8,
> > @@ -303,6 +321,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SRGGB8, {
> > +		.name = "SRGGB8",
> >  		.format = formats::SRGGB8,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),
> >  		.bitsPerPixel = 8,
> > @@ -310,6 +329,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SBGGR10, {
> > +		.name = "SBGGR10",
> >  		.format = formats::SBGGR10,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),
> >  		.bitsPerPixel = 10,
> > @@ -317,6 +337,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SGBRG10, {
> > +		.name = "SGBRG10",
> >  		.format = formats::SGBRG10,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),
> >  		.bitsPerPixel = 10,
> > @@ -324,6 +345,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SGRBG10, {
> > +		.name = "SGRBG10",
> >  		.format = formats::SGRBG10,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),
> >  		.bitsPerPixel = 10,
> > @@ -331,6 +353,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SRGGB10, {
> > +		.name = "SRGGB10",
> >  		.format = formats::SRGGB10,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),
> >  		.bitsPerPixel = 10,
> > @@ -338,6 +361,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SBGGR10_CSI2P, {
> > +		.name = "SBGGR10_CSI2P",
> >  		.format = formats::SBGGR10_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),
> >  		.bitsPerPixel = 10,
> > @@ -345,6 +369,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = true,
> >  	} },
> >  	{ formats::SGBRG10_CSI2P, {
> > +		.name = "SGBRG10_CSI2P",
> >  		.format = formats::SGBRG10_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),
> >  		.bitsPerPixel = 10,
> > @@ -352,6 +377,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = true,
> >  	} },
> >  	{ formats::SGRBG10_CSI2P, {
> > +		.name = "SGRBG10_CSI2P",
> >  		.format = formats::SGRBG10_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),
> >  		.bitsPerPixel = 10,
> > @@ -359,6 +385,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = true,
> >  	} },
> >  	{ formats::SRGGB10_CSI2P, {
> > +		.name = "SRGGB10_CSI2P",
> >  		.format = formats::SRGGB10_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),
> >  		.bitsPerPixel = 10,
> > @@ -366,6 +393,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = true,
> >  	} },
> >  	{ formats::SBGGR12, {
> > +		.name = "SBGGR12",
> >  		.format = formats::SBGGR12,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),
> >  		.bitsPerPixel = 12,
> > @@ -373,6 +401,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SGBRG12, {
> > +		.name = "SGBRG12",
> >  		.format = formats::SGBRG12,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),
> >  		.bitsPerPixel = 12,
> > @@ -380,6 +409,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SGRBG12, {
> > +		.name = "SGRBG12",
> >  		.format = formats::SGRBG12,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),
> >  		.bitsPerPixel = 12,
> > @@ -387,6 +417,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SRGGB12, {
> > +		.name = "SRGGB12",
> >  		.format = formats::SRGGB12,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),
> >  		.bitsPerPixel = 12,
> > @@ -394,6 +425,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = false,
> >  	} },
> >  	{ formats::SBGGR12_CSI2P, {
> > +		.name = "SBGGR12_CSI2P",
> >  		.format = formats::SBGGR12_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),
> >  		.bitsPerPixel = 12,
> > @@ -401,6 +433,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = true,
> >  	} },
> >  	{ formats::SGBRG12_CSI2P, {
> > +		.name = "SGBRG12_CSI2P",
> >  		.format = formats::SGBRG12_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),
> >  		.bitsPerPixel = 12,
> > @@ -408,6 +441,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = true,
> >  	} },
> >  	{ formats::SGRBG12_CSI2P, {
> > +		.name = "SGRBG12_CSI2P",
> >  		.format = formats::SGRBG12_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),
> >  		.bitsPerPixel = 12,
> > @@ -415,6 +449,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.packed = true,
> >  	} },
> >  	{ formats::SRGGB12_CSI2P, {
> > +		.name = "SRGGB12_CSI2P",
> >  		.format = formats::SRGGB12_CSI2P,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),
> >  		.bitsPerPixel = 12,
> > @@ -424,6 +459,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  
> >  	/* Compressed formats. */
> >  	{ formats::MJPEG, {
> > +		.name = "MJPEG",
> >  		.format = formats::MJPEG,
> >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> >  		.bitsPerPixel = 0,
> > @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
> >  	const auto iter = pixelFormatInfo.find(format);
> >  	if (iter == pixelFormatInfo.end()) {
> >  		LOG(Formats, Warning)
> > -			<< "Unsupported pixel format "
> > -			<< format.toString();
> > +			<< "Unsupported pixel format 0x"
> > +			<< std::hex << format.fourcc();
> 
> Does utils::hex(format.fourcc()) make any difference here?

utils::hex() will automatically pad the output string to the size of the
field (8 characters in this case). I think it would be nicer to use it
here. It also resets the formatting options of the stream to avoid
having to add a std::dec after the argument, but that won't make a
difference here as the FourCC is last.

> >  		return invalid;
> >  	}
> >  
> > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
> > index f191851..07e7af0 100644
> > --- a/src/libcamera/pixel_format.cpp
> > +++ b/src/libcamera/pixel_format.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <libcamera/formats.h>
> > +#include "libcamera/internal/formats.h"
> >  #include <libcamera/pixel_format.h>

We put the internal headers after the external ones, with a blank line
between the two groups.

> >  
> >  /**
> > @@ -104,9 +105,29 @@ bool PixelFormat::operator<(const PixelFormat &other) const
> >   */
> >  std::string PixelFormat::toString() const
> >  {
> > -	char str[11];
> > -	snprintf(str, 11, "0x%08x", fourcc_);
> > -	return str;
> > +	PixelFormat format = PixelFormat(fourcc_, modifier_);
> > +	const PixelFormatInfo &info = PixelFormatInfo::info(format);

How about

	const PixelFormatInfo &info = PixelFormatInfo::info(*this);

> > +
> > +	if (!info.isValid()) {
> > +		if (format == PixelFormat())

And here,

		if (!isValid())

With that, you can drop the local format variable.

> > +			return "<INVALID>";
> > +
> > +		char fourcc[7] = { '<',
> > +				   static_cast<char>(fourcc_ & 0x7f),
> > +				   static_cast<char>((fourcc_ >> 8) & 0x7f),
> > +				   static_cast<char>((fourcc_ >> 16) & 0x7f),
> > +				   static_cast<char>((fourcc_ >> 24) & 0x7f),

The isprint() should take care of non-printable characters, do we need
the & 0x7f ?

> > +				   '>' };
> > +
> > +		for (unsigned int i = 1; i < 5; i++) {
> > +			if (!isprint(fourcc[i]))
> > +				fourcc[i] = '.';
> > +		}
> > +
> > +		return fourcc;
> > +	}
> > +
> > +	return info.name;
> >  }
> >  
> >  } /* namespace libcamera */
> > diff --git a/test/meson.build b/test/meson.build
> > index a868813..7808a26 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -34,6 +34,7 @@ internal_tests = [
> >      ['message',                         'message.cpp'],
> >      ['object',                          'object.cpp'],
> >      ['object-invoke',                   'object-invoke.cpp'],
> > +    ['pixel-format',                    'pixel-format.cpp'],
> >      ['signal-threads',                  'signal-threads.cpp'],
> >      ['threads',                         'threads.cpp'],
> >      ['timer',                           'timer.cpp'],
> > diff --git a/test/pixel-format.cpp b/test/pixel-format.cpp
> > new file mode 100644
> > index 0000000..5b9cdc9
> > --- /dev/null
> > +++ b/test/pixel-format.cpp
> > @@ -0,0 +1,49 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Kaaira Gupta
> > + * libcamera pixel format handling test
> > + */
> > +
> > +#include <iostream>
> > +#include <vector>
> > +
> > +#include <libcamera/formats.h>
> > +#include <libcamera/pixel_format.h>
> > +
> > +#include "test.h"
> > +
> > +using namespace std;
> > +using namespace libcamera;
> > +
> > +class PixelFormatTest : public Test
> > +{
> > +protected:
> > +	int run()
> > +	{
> > +		std::vector<std::pair<PixelFormat, const char *>> formatsMap{
> > +			{ formats::R8, "R8" },
> > +			{ formats::SRGGB10_CSI2P, "SRGGB10_CSI2P" },
> > +			{ PixelFormat(0, 0), "<INVALID>" },
> > +			{ PixelFormat(0x20203843), "<C8  >" }
> > +		};
> > +
> > +		for (const auto &format : formatsMap) {
> > +			if ((format.first).toString() != format.second) {
> > +				cerr << "Failed to convert PixelFormat "
> > +				     << format.first.fourcc() << " to string"

Maybe utils::hex(format.first.fourcc()) ?

Only minor changes, with these addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +				     << endl;
> > +				return TestFail;
> > +			}
> > +		}
> > +
> > +		if (PixelFormat().toString() != "<INVALID>") {
> > +			cerr << "Failed to convert default PixelFormat to string"
> > +			     << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +};
> > +
> > +TEST_REGISTER(PixelFormatTest)
Kaaira Gupta June 21, 2020, 8:14 p.m. UTC | #3 
On Sat, Jun 20, 2020 at 02:10:56AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Fri, Jun 19, 2020 at 10:40:15PM +0100, Kieran Bingham wrote:
> > On 19/06/2020 20:06, Kaaira Gupta wrote:
> > > Print format names defined in formats namespace instead of the hex
> > > values in toString() as they are easier to comprehend. For this add
> > > a property of 'name' in PixelFormatInfo so as to map the formats
> > > with their names. Print fourcc for formats which are not used in
> > > libcamera.
> > 
> > Excellent, this is really readable now - and ties in nicely with the new
> > formats namespacing:
> > 
> > >> Using camera VIMC Sensor B
> > >> [66:02:15.314555780] [1159464]  INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888
> > >> [66:02:15.314729922] [1159464]  INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888
> > >> Capture until user interrupts by SIGINT
> 
> Much nicer indeed ! Thank you Kaaira for your perseverance :-)
> 
> > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > > ---
> > > 
> > > Changes since v4:
> > > 	-Print libcamera defined names instead of fourcc.
> > > 
> > > Changes since v3:
> > > 	-shortened the texts.
> > > 	-Removed default case as well.
> > > 	-changed commit message and tests to reflect the changes.
> > > 
> > > Changes since v2:
> > >         - Remove description for all vendors except for MIPI
> > >         - Change commit message to reflect this change.
> > >         - Change tests accordingly.
> > > 
> > > Changes since v1:
> > >         - Replaced magic numbers with expressive values.
> > >         - Re-wrote ARM vendor's modifiers
> > >         - Re-wrote the vendors' map with a macro.
> > >         - Changed the copyrights in test file.
> > >         - Changed the tests.
> > > 
> > >  include/libcamera/internal/formats.h |  1 +
> > >  src/libcamera/formats.cpp            | 40 +++++++++++++++++++++--
> > >  src/libcamera/pixel_format.cpp       | 27 +++++++++++++--
> > >  test/meson.build                     |  1 +
> > >  test/pixel-format.cpp                | 49 ++++++++++++++++++++++++++++
> > >  5 files changed, 113 insertions(+), 5 deletions(-)
> > >  create mode 100644 test/pixel-format.cpp
> > > 
> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > > index 4b172ef..8012721 100644
> > > --- a/include/libcamera/internal/formats.h
> > > +++ b/include/libcamera/internal/formats.h
> > > @@ -46,6 +46,7 @@ public:
> > >  	static const PixelFormatInfo &info(const PixelFormat &format);
> > >  
> > >  	/* \todo Add support for non-contiguous memory planes */
> > > +	std::string name;
> > 
> > I think this could be "const char * name" ?
> 
> s/\* /*/
> 
> > If that's the only thing to update, (and if it should be) then I can
> > update this when applying if we get more RB tags, so no need to repost
> > unless there are other comments.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Although I do have a question below about std::hex vs utils::hex too..

I didn't know the difference (Now I do), I was just used to using std:: 
hence I used that

> > 
> > >  	PixelFormat format;
> > >  	V4L2PixelFormat v4l2Format;
> > >  	unsigned int bitsPerPixel;
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index 97e9867..2908409 100644
> > > --- a/src/libcamera/formats.cpp
> > > +++ b/src/libcamera/formats.cpp
> > > @@ -169,6 +169,7 @@ namespace {
> > >  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  	/* RGB formats. */
> > >  	{ formats::BGR888, {
> > > +		.name = "BGR888",
> > >  		.format = formats::BGR888,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),
> > >  		.bitsPerPixel = 24,
> 
> Unrelated to this patch, but I wonder if we should add more information
> to formats.yaml and generate this map.
> 
> > > @@ -176,6 +177,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::RGB888, {
> > > +		.name = "RGB888",
> > >  		.format = formats::RGB888,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),
> > >  		.bitsPerPixel = 24,
> > > @@ -183,6 +185,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::ABGR8888, {
> > > +		.name = "ABGR8888",
> > >  		.format = formats::ABGR8888,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),
> > >  		.bitsPerPixel = 32,
> > > @@ -190,6 +193,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::ARGB8888, {
> > > +		.name = "ARGB8888",
> > >  		.format = formats::ARGB8888,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),
> > >  		.bitsPerPixel = 32,
> > > @@ -197,6 +201,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::BGRA8888, {
> > > +		.name = "BGRA8888",
> > >  		.format = formats::BGRA8888,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),
> > >  		.bitsPerPixel = 32,
> > > @@ -204,6 +209,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::RGBA8888, {
> > > +		.name = "RGBA8888",
> > >  		.format = formats::RGBA8888,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),
> > >  		.bitsPerPixel = 32,
> > > @@ -213,6 +219,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  
> > >  	/* YUV packed formats. */
> > >  	{ formats::YUYV, {
> > > +		.name = "YUYV",
> > >  		.format = formats::YUYV,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),
> > >  		.bitsPerPixel = 16,
> > > @@ -220,6 +227,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::YVYU, {
> > > +		.name = "YVYU",
> > >  		.format = formats::YVYU,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),
> > >  		.bitsPerPixel = 16,
> > > @@ -227,6 +235,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::UYVY, {
> > > +		.name = "UYVY",
> > >  		.format = formats::UYVY,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),
> > >  		.bitsPerPixel = 16,
> > > @@ -234,6 +243,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::VYUY, {
> > > +		.name = "VYUY",
> > >  		.format = formats::VYUY,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),
> > >  		.bitsPerPixel = 16,
> > > @@ -243,6 +253,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  
> > >  	/* YUV planar formats. */
> > >  	{ formats::NV16, {
> > > +		.name = "NV16",
> > >  		.format = formats::NV16,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),
> > >  		.bitsPerPixel = 16,
> > > @@ -250,6 +261,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::NV61, {
> > > +		.name = "NV61",
> > >  		.format = formats::NV61,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),
> > >  		.bitsPerPixel = 16,
> > > @@ -257,6 +269,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::NV12, {
> > > +		.name = "NV12",
> > >  		.format = formats::NV12,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),
> > >  		.bitsPerPixel = 12,
> > > @@ -264,6 +277,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::NV21, {
> > > +		.name = "NV21",
> > >  		.format = formats::NV21,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),
> > >  		.bitsPerPixel = 12,
> > > @@ -273,6 +287,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  
> > >  	/* Greyscale formats. */
> > >  	{ formats::R8, {
> > > +		.name = "R8",
> > >  		.format = formats::R8,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),
> > >  		.bitsPerPixel = 8,
> > > @@ -282,6 +297,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  
> > >  	/* Bayer formats. */
> > >  	{ formats::SBGGR8, {
> > > +		.name = "SBGGR8",
> > >  		.format = formats::SBGGR8,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),
> > >  		.bitsPerPixel = 8,
> > > @@ -289,6 +305,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SGBRG8, {
> > > +		.name = "SGBRG8",
> > >  		.format = formats::SGBRG8,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),
> > >  		.bitsPerPixel = 8,
> > > @@ -296,6 +313,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SGRBG8, {
> > > +		.name = "SGRBG8",
> > >  		.format = formats::SGRBG8,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),
> > >  		.bitsPerPixel = 8,
> > > @@ -303,6 +321,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SRGGB8, {
> > > +		.name = "SRGGB8",
> > >  		.format = formats::SRGGB8,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),
> > >  		.bitsPerPixel = 8,
> > > @@ -310,6 +329,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SBGGR10, {
> > > +		.name = "SBGGR10",
> > >  		.format = formats::SBGGR10,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),
> > >  		.bitsPerPixel = 10,
> > > @@ -317,6 +337,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SGBRG10, {
> > > +		.name = "SGBRG10",
> > >  		.format = formats::SGBRG10,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),
> > >  		.bitsPerPixel = 10,
> > > @@ -324,6 +345,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SGRBG10, {
> > > +		.name = "SGRBG10",
> > >  		.format = formats::SGRBG10,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),
> > >  		.bitsPerPixel = 10,
> > > @@ -331,6 +353,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SRGGB10, {
> > > +		.name = "SRGGB10",
> > >  		.format = formats::SRGGB10,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),
> > >  		.bitsPerPixel = 10,
> > > @@ -338,6 +361,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SBGGR10_CSI2P, {
> > > +		.name = "SBGGR10_CSI2P",
> > >  		.format = formats::SBGGR10_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),
> > >  		.bitsPerPixel = 10,
> > > @@ -345,6 +369,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = true,
> > >  	} },
> > >  	{ formats::SGBRG10_CSI2P, {
> > > +		.name = "SGBRG10_CSI2P",
> > >  		.format = formats::SGBRG10_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),
> > >  		.bitsPerPixel = 10,
> > > @@ -352,6 +377,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = true,
> > >  	} },
> > >  	{ formats::SGRBG10_CSI2P, {
> > > +		.name = "SGRBG10_CSI2P",
> > >  		.format = formats::SGRBG10_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),
> > >  		.bitsPerPixel = 10,
> > > @@ -359,6 +385,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = true,
> > >  	} },
> > >  	{ formats::SRGGB10_CSI2P, {
> > > +		.name = "SRGGB10_CSI2P",
> > >  		.format = formats::SRGGB10_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),
> > >  		.bitsPerPixel = 10,
> > > @@ -366,6 +393,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = true,
> > >  	} },
> > >  	{ formats::SBGGR12, {
> > > +		.name = "SBGGR12",
> > >  		.format = formats::SBGGR12,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),
> > >  		.bitsPerPixel = 12,
> > > @@ -373,6 +401,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SGBRG12, {
> > > +		.name = "SGBRG12",
> > >  		.format = formats::SGBRG12,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),
> > >  		.bitsPerPixel = 12,
> > > @@ -380,6 +409,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SGRBG12, {
> > > +		.name = "SGRBG12",
> > >  		.format = formats::SGRBG12,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),
> > >  		.bitsPerPixel = 12,
> > > @@ -387,6 +417,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SRGGB12, {
> > > +		.name = "SRGGB12",
> > >  		.format = formats::SRGGB12,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),
> > >  		.bitsPerPixel = 12,
> > > @@ -394,6 +425,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = false,
> > >  	} },
> > >  	{ formats::SBGGR12_CSI2P, {
> > > +		.name = "SBGGR12_CSI2P",
> > >  		.format = formats::SBGGR12_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),
> > >  		.bitsPerPixel = 12,
> > > @@ -401,6 +433,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = true,
> > >  	} },
> > >  	{ formats::SGBRG12_CSI2P, {
> > > +		.name = "SGBRG12_CSI2P",
> > >  		.format = formats::SGBRG12_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),
> > >  		.bitsPerPixel = 12,
> > > @@ -408,6 +441,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = true,
> > >  	} },
> > >  	{ formats::SGRBG12_CSI2P, {
> > > +		.name = "SGRBG12_CSI2P",
> > >  		.format = formats::SGRBG12_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),
> > >  		.bitsPerPixel = 12,
> > > @@ -415,6 +449,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.packed = true,
> > >  	} },
> > >  	{ formats::SRGGB12_CSI2P, {
> > > +		.name = "SRGGB12_CSI2P",
> > >  		.format = formats::SRGGB12_CSI2P,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),
> > >  		.bitsPerPixel = 12,
> > > @@ -424,6 +459,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  
> > >  	/* Compressed formats. */
> > >  	{ formats::MJPEG, {
> > > +		.name = "MJPEG",
> > >  		.format = formats::MJPEG,
> > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > >  		.bitsPerPixel = 0,
> > > @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
> > >  	const auto iter = pixelFormatInfo.find(format);
> > >  	if (iter == pixelFormatInfo.end()) {
> > >  		LOG(Formats, Warning)
> > > -			<< "Unsupported pixel format "
> > > -			<< format.toString();
> > > +			<< "Unsupported pixel format 0x"
> > > +			<< std::hex << format.fourcc();
> > 
> > Does utils::hex(format.fourcc()) make any difference here?
> 
> utils::hex() will automatically pad the output string to the size of the
> field (8 characters in this case). I think it would be nicer to use it
> here. It also resets the formatting options of the stream to avoid
> having to add a std::dec after the argument, but that won't make a
> difference here as the FourCC is last.

Okay i'll make the change

> 
> > >  		return invalid;
> > >  	}
> > >  
> > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
> > > index f191851..07e7af0 100644
> > > --- a/src/libcamera/pixel_format.cpp
> > > +++ b/src/libcamera/pixel_format.cpp
> > > @@ -6,6 +6,7 @@
> > >   */
> > >  
> > >  #include <libcamera/formats.h>
> > > +#include "libcamera/internal/formats.h"
> > >  #include <libcamera/pixel_format.h>
> 
> We put the internal headers after the external ones, with a blank line
> between the two groups.

Noted

> 
> > >  
> > >  /**
> > > @@ -104,9 +105,29 @@ bool PixelFormat::operator<(const PixelFormat &other) const
> > >   */
> > >  std::string PixelFormat::toString() const
> > >  {
> > > -	char str[11];
> > > -	snprintf(str, 11, "0x%08x", fourcc_);
> > > -	return str;
> > > +	PixelFormat format = PixelFormat(fourcc_, modifier_);
> > > +	const PixelFormatInfo &info = PixelFormatInfo::info(format);
> 
> How about
> 
> 	const PixelFormatInfo &info = PixelFormatInfo::info(*this);
> 
> > > +
> > > +	if (!info.isValid()) {
> > > +		if (format == PixelFormat())
> 
> And here,
> 
> 		if (!isValid())
> 
> With that, you can drop the local format variable.

I didn't think that way. Thanks, will change it

> 
> > > +			return "<INVALID>";
> > > +
> > > +		char fourcc[7] = { '<',
> > > +				   static_cast<char>(fourcc_ & 0x7f),
> > > +				   static_cast<char>((fourcc_ >> 8) & 0x7f),
> > > +				   static_cast<char>((fourcc_ >> 16) & 0x7f),
> > > +				   static_cast<char>((fourcc_ >> 24) & 0x7f),
> 
> The isprint() should take care of non-printable characters, do we need
> the & 0x7f ?

Don't we need & 0x7f to check for ascii character? I think we can't
check for non-printable non-ascii characters with isprint(), so won't it
break while checking if the passed character is non-ascii?
idk I am not sure

> 
> > > +				   '>' };
> > > +
> > > +		for (unsigned int i = 1; i < 5; i++) {
> > > +			if (!isprint(fourcc[i]))
> > > +				fourcc[i] = '.';
> > > +		}
> > > +
> > > +		return fourcc;
> > > +	}
> > > +
> > > +	return info.name;
> > >  }
> > >  
> > >  } /* namespace libcamera */
> > > diff --git a/test/meson.build b/test/meson.build
> > > index a868813..7808a26 100644
> > > --- a/test/meson.build
> > > +++ b/test/meson.build
> > > @@ -34,6 +34,7 @@ internal_tests = [
> > >      ['message',                         'message.cpp'],
> > >      ['object',                          'object.cpp'],
> > >      ['object-invoke',                   'object-invoke.cpp'],
> > > +    ['pixel-format',                    'pixel-format.cpp'],
> > >      ['signal-threads',                  'signal-threads.cpp'],
> > >      ['threads',                         'threads.cpp'],
> > >      ['timer',                           'timer.cpp'],
> > > diff --git a/test/pixel-format.cpp b/test/pixel-format.cpp
> > > new file mode 100644
> > > index 0000000..5b9cdc9
> > > --- /dev/null
> > > +++ b/test/pixel-format.cpp
> > > @@ -0,0 +1,49 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Kaaira Gupta
> > > + * libcamera pixel format handling test
> > > + */
> > > +
> > > +#include <iostream>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/formats.h>
> > > +#include <libcamera/pixel_format.h>
> > > +
> > > +#include "test.h"
> > > +
> > > +using namespace std;
> > > +using namespace libcamera;
> > > +
> > > +class PixelFormatTest : public Test
> > > +{
> > > +protected:
> > > +	int run()
> > > +	{
> > > +		std::vector<std::pair<PixelFormat, const char *>> formatsMap{
> > > +			{ formats::R8, "R8" },
> > > +			{ formats::SRGGB10_CSI2P, "SRGGB10_CSI2P" },
> > > +			{ PixelFormat(0, 0), "<INVALID>" },
> > > +			{ PixelFormat(0x20203843), "<C8  >" }
> > > +		};
> > > +
> > > +		for (const auto &format : formatsMap) {
> > > +			if ((format.first).toString() != format.second) {
> > > +				cerr << "Failed to convert PixelFormat "
> > > +				     << format.first.fourcc() << " to string"
> 
> Maybe utils::hex(format.first.fourcc()) ?
> 
> Only minor changes, with these addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > > +				     << endl;
> > > +				return TestFail;
> > > +			}
> > > +		}
> > > +
> > > +		if (PixelFormat().toString() != "<INVALID>") {
> > > +			cerr << "Failed to convert default PixelFormat to string"
> > > +			     << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > > +		return TestPass;
> > > +	}
> > > +};
> > > +
> > > +TEST_REGISTER(PixelFormatTest)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart June 21, 2020, 10:10 p.m. UTC | #4 
Hi Kaaira,

On Mon, Jun 22, 2020 at 01:44:33AM +0530, Kaaira Gupta wrote:
> On Sat, Jun 20, 2020 at 02:10:56AM +0300, Laurent Pinchart wrote:
> > On Fri, Jun 19, 2020 at 10:40:15PM +0100, Kieran Bingham wrote:
> > > On 19/06/2020 20:06, Kaaira Gupta wrote:
> > > > Print format names defined in formats namespace instead of the hex
> > > > values in toString() as they are easier to comprehend. For this add
> > > > a property of 'name' in PixelFormatInfo so as to map the formats
> > > > with their names. Print fourcc for formats which are not used in
> > > > libcamera.
> > > 
> > > Excellent, this is really readable now - and ties in nicely with the new
> > > formats namespacing:
> > > 
> > > >> Using camera VIMC Sensor B
> > > >> [66:02:15.314555780] [1159464]  INFO VIMC vimc.cpp:189 Skipping unsupported pixel format RGB888
> > > >> [66:02:15.314729922] [1159464]  INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BGR888
> > > >> Capture until user interrupts by SIGINT
> > 
> > Much nicer indeed ! Thank you Kaaira for your perseverance :-)
> > 
> > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > > > ---
> > > > 
> > > > Changes since v4:
> > > > 	-Print libcamera defined names instead of fourcc.
> > > > 
> > > > Changes since v3:
> > > > 	-shortened the texts.
> > > > 	-Removed default case as well.
> > > > 	-changed commit message and tests to reflect the changes.
> > > > 
> > > > Changes since v2:
> > > >         - Remove description for all vendors except for MIPI
> > > >         - Change commit message to reflect this change.
> > > >         - Change tests accordingly.
> > > > 
> > > > Changes since v1:
> > > >         - Replaced magic numbers with expressive values.
> > > >         - Re-wrote ARM vendor's modifiers
> > > >         - Re-wrote the vendors' map with a macro.
> > > >         - Changed the copyrights in test file.
> > > >         - Changed the tests.
> > > > 
> > > >  include/libcamera/internal/formats.h |  1 +
> > > >  src/libcamera/formats.cpp            | 40 +++++++++++++++++++++--
> > > >  src/libcamera/pixel_format.cpp       | 27 +++++++++++++--
> > > >  test/meson.build                     |  1 +
> > > >  test/pixel-format.cpp                | 49 ++++++++++++++++++++++++++++
> > > >  5 files changed, 113 insertions(+), 5 deletions(-)
> > > >  create mode 100644 test/pixel-format.cpp
> > > > 
> > > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > > > index 4b172ef..8012721 100644
> > > > --- a/include/libcamera/internal/formats.h
> > > > +++ b/include/libcamera/internal/formats.h
> > > > @@ -46,6 +46,7 @@ public:
> > > >  	static const PixelFormatInfo &info(const PixelFormat &format);
> > > >  
> > > >  	/* \todo Add support for non-contiguous memory planes */
> > > > +	std::string name;
> > > 
> > > I think this could be "const char * name" ?
> > 
> > s/\* /*/
> > 
> > > If that's the only thing to update, (and if it should be) then I can
> > > update this when applying if we get more RB tags, so no need to repost
> > > unless there are other comments.
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > Although I do have a question below about std::hex vs utils::hex too..
> 
> I didn't know the difference (Now I do), I was just used to using std:: 
> hence I used that
> 
> > > 
> > > >  	PixelFormat format;
> > > >  	V4L2PixelFormat v4l2Format;
> > > >  	unsigned int bitsPerPixel;
> > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > > index 97e9867..2908409 100644
> > > > --- a/src/libcamera/formats.cpp
> > > > +++ b/src/libcamera/formats.cpp
> > > > @@ -169,6 +169,7 @@ namespace {
> > > >  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  	/* RGB formats. */
> > > >  	{ formats::BGR888, {
> > > > +		.name = "BGR888",
> > > >  		.format = formats::BGR888,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),
> > > >  		.bitsPerPixel = 24,
> > 
> > Unrelated to this patch, but I wonder if we should add more information
> > to formats.yaml and generate this map.
> > 
> > > > @@ -176,6 +177,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::RGB888, {
> > > > +		.name = "RGB888",
> > > >  		.format = formats::RGB888,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),
> > > >  		.bitsPerPixel = 24,
> > > > @@ -183,6 +185,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::ABGR8888, {
> > > > +		.name = "ABGR8888",
> > > >  		.format = formats::ABGR8888,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),
> > > >  		.bitsPerPixel = 32,
> > > > @@ -190,6 +193,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::ARGB8888, {
> > > > +		.name = "ARGB8888",
> > > >  		.format = formats::ARGB8888,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),
> > > >  		.bitsPerPixel = 32,
> > > > @@ -197,6 +201,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::BGRA8888, {
> > > > +		.name = "BGRA8888",
> > > >  		.format = formats::BGRA8888,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),
> > > >  		.bitsPerPixel = 32,
> > > > @@ -204,6 +209,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::RGBA8888, {
> > > > +		.name = "RGBA8888",
> > > >  		.format = formats::RGBA8888,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),
> > > >  		.bitsPerPixel = 32,
> > > > @@ -213,6 +219,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  
> > > >  	/* YUV packed formats. */
> > > >  	{ formats::YUYV, {
> > > > +		.name = "YUYV",
> > > >  		.format = formats::YUYV,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),
> > > >  		.bitsPerPixel = 16,
> > > > @@ -220,6 +227,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::YVYU, {
> > > > +		.name = "YVYU",
> > > >  		.format = formats::YVYU,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),
> > > >  		.bitsPerPixel = 16,
> > > > @@ -227,6 +235,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::UYVY, {
> > > > +		.name = "UYVY",
> > > >  		.format = formats::UYVY,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),
> > > >  		.bitsPerPixel = 16,
> > > > @@ -234,6 +243,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::VYUY, {
> > > > +		.name = "VYUY",
> > > >  		.format = formats::VYUY,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),
> > > >  		.bitsPerPixel = 16,
> > > > @@ -243,6 +253,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  
> > > >  	/* YUV planar formats. */
> > > >  	{ formats::NV16, {
> > > > +		.name = "NV16",
> > > >  		.format = formats::NV16,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),
> > > >  		.bitsPerPixel = 16,
> > > > @@ -250,6 +261,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::NV61, {
> > > > +		.name = "NV61",
> > > >  		.format = formats::NV61,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),
> > > >  		.bitsPerPixel = 16,
> > > > @@ -257,6 +269,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::NV12, {
> > > > +		.name = "NV12",
> > > >  		.format = formats::NV12,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -264,6 +277,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::NV21, {
> > > > +		.name = "NV21",
> > > >  		.format = formats::NV21,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -273,6 +287,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  
> > > >  	/* Greyscale formats. */
> > > >  	{ formats::R8, {
> > > > +		.name = "R8",
> > > >  		.format = formats::R8,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),
> > > >  		.bitsPerPixel = 8,
> > > > @@ -282,6 +297,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  
> > > >  	/* Bayer formats. */
> > > >  	{ formats::SBGGR8, {
> > > > +		.name = "SBGGR8",
> > > >  		.format = formats::SBGGR8,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),
> > > >  		.bitsPerPixel = 8,
> > > > @@ -289,6 +305,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SGBRG8, {
> > > > +		.name = "SGBRG8",
> > > >  		.format = formats::SGBRG8,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),
> > > >  		.bitsPerPixel = 8,
> > > > @@ -296,6 +313,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SGRBG8, {
> > > > +		.name = "SGRBG8",
> > > >  		.format = formats::SGRBG8,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),
> > > >  		.bitsPerPixel = 8,
> > > > @@ -303,6 +321,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SRGGB8, {
> > > > +		.name = "SRGGB8",
> > > >  		.format = formats::SRGGB8,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),
> > > >  		.bitsPerPixel = 8,
> > > > @@ -310,6 +329,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SBGGR10, {
> > > > +		.name = "SBGGR10",
> > > >  		.format = formats::SBGGR10,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -317,6 +337,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SGBRG10, {
> > > > +		.name = "SGBRG10",
> > > >  		.format = formats::SGBRG10,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -324,6 +345,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SGRBG10, {
> > > > +		.name = "SGRBG10",
> > > >  		.format = formats::SGRBG10,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -331,6 +353,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SRGGB10, {
> > > > +		.name = "SRGGB10",
> > > >  		.format = formats::SRGGB10,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -338,6 +361,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SBGGR10_CSI2P, {
> > > > +		.name = "SBGGR10_CSI2P",
> > > >  		.format = formats::SBGGR10_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -345,6 +369,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = true,
> > > >  	} },
> > > >  	{ formats::SGBRG10_CSI2P, {
> > > > +		.name = "SGBRG10_CSI2P",
> > > >  		.format = formats::SGBRG10_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -352,6 +377,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = true,
> > > >  	} },
> > > >  	{ formats::SGRBG10_CSI2P, {
> > > > +		.name = "SGRBG10_CSI2P",
> > > >  		.format = formats::SGRBG10_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -359,6 +385,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = true,
> > > >  	} },
> > > >  	{ formats::SRGGB10_CSI2P, {
> > > > +		.name = "SRGGB10_CSI2P",
> > > >  		.format = formats::SRGGB10_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),
> > > >  		.bitsPerPixel = 10,
> > > > @@ -366,6 +393,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = true,
> > > >  	} },
> > > >  	{ formats::SBGGR12, {
> > > > +		.name = "SBGGR12",
> > > >  		.format = formats::SBGGR12,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -373,6 +401,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SGBRG12, {
> > > > +		.name = "SGBRG12",
> > > >  		.format = formats::SGBRG12,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -380,6 +409,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SGRBG12, {
> > > > +		.name = "SGRBG12",
> > > >  		.format = formats::SGRBG12,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -387,6 +417,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SRGGB12, {
> > > > +		.name = "SRGGB12",
> > > >  		.format = formats::SRGGB12,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -394,6 +425,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = false,
> > > >  	} },
> > > >  	{ formats::SBGGR12_CSI2P, {
> > > > +		.name = "SBGGR12_CSI2P",
> > > >  		.format = formats::SBGGR12_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -401,6 +433,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = true,
> > > >  	} },
> > > >  	{ formats::SGBRG12_CSI2P, {
> > > > +		.name = "SGBRG12_CSI2P",
> > > >  		.format = formats::SGBRG12_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -408,6 +441,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = true,
> > > >  	} },
> > > >  	{ formats::SGRBG12_CSI2P, {
> > > > +		.name = "SGRBG12_CSI2P",
> > > >  		.format = formats::SGRBG12_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -415,6 +449,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  		.packed = true,
> > > >  	} },
> > > >  	{ formats::SRGGB12_CSI2P, {
> > > > +		.name = "SRGGB12_CSI2P",
> > > >  		.format = formats::SRGGB12_CSI2P,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),
> > > >  		.bitsPerPixel = 12,
> > > > @@ -424,6 +459,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >  
> > > >  	/* Compressed formats. */
> > > >  	{ formats::MJPEG, {
> > > > +		.name = "MJPEG",
> > > >  		.format = formats::MJPEG,
> > > >  		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > >  		.bitsPerPixel = 0,
> > > > @@ -453,8 +489,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
> > > >  	const auto iter = pixelFormatInfo.find(format);
> > > >  	if (iter == pixelFormatInfo.end()) {
> > > >  		LOG(Formats, Warning)
> > > > -			<< "Unsupported pixel format "
> > > > -			<< format.toString();
> > > > +			<< "Unsupported pixel format 0x"
> > > > +			<< std::hex << format.fourcc();
> > > 
> > > Does utils::hex(format.fourcc()) make any difference here?
> > 
> > utils::hex() will automatically pad the output string to the size of the
> > field (8 characters in this case). I think it would be nicer to use it
> > here. It also resets the formatting options of the stream to avoid
> > having to add a std::dec after the argument, but that won't make a
> > difference here as the FourCC is last.
> 
> Okay i'll make the change
> 
> > 
> > > >  		return invalid;
> > > >  	}
> > > >  
> > > > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
> > > > index f191851..07e7af0 100644
> > > > --- a/src/libcamera/pixel_format.cpp
> > > > +++ b/src/libcamera/pixel_format.cpp
> > > > @@ -6,6 +6,7 @@
> > > >   */
> > > >  
> > > >  #include <libcamera/formats.h>
> > > > +#include "libcamera/internal/formats.h"
> > > >  #include <libcamera/pixel_format.h>
> > 
> > We put the internal headers after the external ones, with a blank line
> > between the two groups.
> 
> Noted
> 
> > 
> > > >  
> > > >  /**
> > > > @@ -104,9 +105,29 @@ bool PixelFormat::operator<(const PixelFormat &other) const
> > > >   */
> > > >  std::string PixelFormat::toString() const
> > > >  {
> > > > -	char str[11];
> > > > -	snprintf(str, 11, "0x%08x", fourcc_);
> > > > -	return str;
> > > > +	PixelFormat format = PixelFormat(fourcc_, modifier_);
> > > > +	const PixelFormatInfo &info = PixelFormatInfo::info(format);
> > 
> > How about
> > 
> > 	const PixelFormatInfo &info = PixelFormatInfo::info(*this);
> > 
> > > > +
> > > > +	if (!info.isValid()) {
> > > > +		if (format == PixelFormat())
> > 
> > And here,
> > 
> > 		if (!isValid())
> > 
> > With that, you can drop the local format variable.
> 
> I didn't think that way. Thanks, will change it
> 
> > > > +			return "<INVALID>";
> > > > +
> > > > +		char fourcc[7] = { '<',
> > > > +				   static_cast<char>(fourcc_ & 0x7f),
> > > > +				   static_cast<char>((fourcc_ >> 8) & 0x7f),
> > > > +				   static_cast<char>((fourcc_ >> 16) & 0x7f),
> > > > +				   static_cast<char>((fourcc_ >> 24) & 0x7f),
> > 
> > The isprint() should take care of non-printable characters, do we need
> > the & 0x7f ?
> 
> Don't we need & 0x7f to check for ascii character? I think we can't
> check for non-printable non-ascii characters with isprint(), so won't it
> break while checking if the passed character is non-ascii?
> idk I am not sure

The behaviour of isprint() is locale-specific, but in any case it should
guarantee that the character is printable, and it accepts any value
between 0 and 255 (technically any value that is representable by an
unsigned char). It should thus be safe.

> > > > +				   '>' };
> > > > +
> > > > +		for (unsigned int i = 1; i < 5; i++) {
> > > > +			if (!isprint(fourcc[i]))
> > > > +				fourcc[i] = '.';
> > > > +		}
> > > > +
> > > > +		return fourcc;
> > > > +	}
> > > > +
> > > > +	return info.name;
> > > >  }
> > > >  
> > > >  } /* namespace libcamera */
> > > > diff --git a/test/meson.build b/test/meson.build
> > > > index a868813..7808a26 100644
> > > > --- a/test/meson.build
> > > > +++ b/test/meson.build
> > > > @@ -34,6 +34,7 @@ internal_tests = [
> > > >      ['message',                         'message.cpp'],
> > > >      ['object',                          'object.cpp'],
> > > >      ['object-invoke',                   'object-invoke.cpp'],
> > > > +    ['pixel-format',                    'pixel-format.cpp'],
> > > >      ['signal-threads',                  'signal-threads.cpp'],
> > > >      ['threads',                         'threads.cpp'],
> > > >      ['timer',                           'timer.cpp'],
> > > > diff --git a/test/pixel-format.cpp b/test/pixel-format.cpp
> > > > new file mode 100644
> > > > index 0000000..5b9cdc9
> > > > --- /dev/null
> > > > +++ b/test/pixel-format.cpp
> > > > @@ -0,0 +1,49 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2020, Kaaira Gupta
> > > > + * libcamera pixel format handling test
> > > > + */
> > > > +
> > > > +#include <iostream>
> > > > +#include <vector>
> > > > +
> > > > +#include <libcamera/formats.h>
> > > > +#include <libcamera/pixel_format.h>
> > > > +
> > > > +#include "test.h"
> > > > +
> > > > +using namespace std;
> > > > +using namespace libcamera;
> > > > +
> > > > +class PixelFormatTest : public Test
> > > > +{
> > > > +protected:
> > > > +	int run()
> > > > +	{
> > > > +		std::vector<std::pair<PixelFormat, const char *>> formatsMap{
> > > > +			{ formats::R8, "R8" },
> > > > +			{ formats::SRGGB10_CSI2P, "SRGGB10_CSI2P" },
> > > > +			{ PixelFormat(0, 0), "<INVALID>" },
> > > > +			{ PixelFormat(0x20203843), "<C8  >" }
> > > > +		};
> > > > +
> > > > +		for (const auto &format : formatsMap) {
> > > > +			if ((format.first).toString() != format.second) {
> > > > +				cerr << "Failed to convert PixelFormat "
> > > > +				     << format.first.fourcc() << " to string"
> > 
> > Maybe utils::hex(format.first.fourcc()) ?
> > 
> > Only minor changes, with these addressed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > > +				     << endl;
> > > > +				return TestFail;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		if (PixelFormat().toString() != "<INVALID>") {
> > > > +			cerr << "Failed to convert default PixelFormat to string"
> > > > +			     << endl;
> > > > +			return TestFail;
> > > > +		}
> > > > +
> > > > +		return TestPass;
> > > > +	}
> > > > +};
> > > > +
> > > > +TEST_REGISTER(PixelFormatTest)

Patch
diff mbox series 

diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
index 4b172ef..8012721 100644
--- a/include/libcamera/internal/formats.h
+++ b/include/libcamera/internal/formats.h
@@ -46,6 +46,7 @@  public:
 	static const PixelFormatInfo &info(const PixelFormat &format);
 
 	/* \todo Add support for non-contiguous memory planes */
+	std::string name;
 	PixelFormat format;
 	V4L2PixelFormat v4l2Format;
 	unsigned int bitsPerPixel;
diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
index 97e9867..2908409 100644
--- a/src/libcamera/formats.cpp
+++ b/src/libcamera/formats.cpp
@@ -169,6 +169,7 @@  namespace {
 const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 	/* RGB formats. */
 	{ formats::BGR888, {
+		.name = "BGR888",
 		.format = formats::BGR888,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),
 		.bitsPerPixel = 24,
@@ -176,6 +177,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::RGB888, {
+		.name = "RGB888",
 		.format = formats::RGB888,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),
 		.bitsPerPixel = 24,
@@ -183,6 +185,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::ABGR8888, {
+		.name = "ABGR8888",
 		.format = formats::ABGR8888,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),
 		.bitsPerPixel = 32,
@@ -190,6 +193,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::ARGB8888, {
+		.name = "ARGB8888",
 		.format = formats::ARGB8888,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),
 		.bitsPerPixel = 32,
@@ -197,6 +201,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::BGRA8888, {
+		.name = "BGRA8888",
 		.format = formats::BGRA8888,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),
 		.bitsPerPixel = 32,
@@ -204,6 +209,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::RGBA8888, {
+		.name = "RGBA8888",
 		.format = formats::RGBA8888,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),
 		.bitsPerPixel = 32,
@@ -213,6 +219,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 
 	/* YUV packed formats. */
 	{ formats::YUYV, {
+		.name = "YUYV",
 		.format = formats::YUYV,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),
 		.bitsPerPixel = 16,
@@ -220,6 +227,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::YVYU, {
+		.name = "YVYU",
 		.format = formats::YVYU,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),
 		.bitsPerPixel = 16,
@@ -227,6 +235,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::UYVY, {
+		.name = "UYVY",
 		.format = formats::UYVY,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),
 		.bitsPerPixel = 16,
@@ -234,6 +243,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::VYUY, {
+		.name = "VYUY",
 		.format = formats::VYUY,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),
 		.bitsPerPixel = 16,
@@ -243,6 +253,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 
 	/* YUV planar formats. */
 	{ formats::NV16, {
+		.name = "NV16",
 		.format = formats::NV16,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),
 		.bitsPerPixel = 16,
@@ -250,6 +261,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::NV61, {
+		.name = "NV61",
 		.format = formats::NV61,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),
 		.bitsPerPixel = 16,
@@ -257,6 +269,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::NV12, {
+		.name = "NV12",
 		.format = formats::NV12,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),
 		.bitsPerPixel = 12,
@@ -264,6 +277,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::NV21, {
+		.name = "NV21",
 		.format = formats::NV21,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),
 		.bitsPerPixel = 12,
@@ -273,6 +287,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 
 	/* Greyscale formats. */
 	{ formats::R8, {
+		.name = "R8",
 		.format = formats::R8,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),
 		.bitsPerPixel = 8,
@@ -282,6 +297,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 
 	/* Bayer formats. */
 	{ formats::SBGGR8, {
+		.name = "SBGGR8",
 		.format = formats::SBGGR8,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),
 		.bitsPerPixel = 8,
@@ -289,6 +305,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SGBRG8, {
+		.name = "SGBRG8",
 		.format = formats::SGBRG8,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),
 		.bitsPerPixel = 8,
@@ -296,6 +313,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SGRBG8, {
+		.name = "SGRBG8",
 		.format = formats::SGRBG8,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),
 		.bitsPerPixel = 8,
@@ -303,6 +321,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SRGGB8, {
+		.name = "SRGGB8",
 		.format = formats::SRGGB8,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),
 		.bitsPerPixel = 8,
@@ -310,6 +329,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SBGGR10, {
+		.name = "SBGGR10",
 		.format = formats::SBGGR10,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),
 		.bitsPerPixel = 10,
@@ -317,6 +337,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SGBRG10, {
+		.name = "SGBRG10",
 		.format = formats::SGBRG10,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),
 		.bitsPerPixel = 10,
@@ -324,6 +345,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SGRBG10, {
+		.name = "SGRBG10",
 		.format = formats::SGRBG10,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),
 		.bitsPerPixel = 10,
@@ -331,6 +353,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SRGGB10, {
+		.name = "SRGGB10",
 		.format = formats::SRGGB10,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),
 		.bitsPerPixel = 10,
@@ -338,6 +361,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SBGGR10_CSI2P, {
+		.name = "SBGGR10_CSI2P",
 		.format = formats::SBGGR10_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),
 		.bitsPerPixel = 10,
@@ -345,6 +369,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = true,
 	} },
 	{ formats::SGBRG10_CSI2P, {
+		.name = "SGBRG10_CSI2P",
 		.format = formats::SGBRG10_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),
 		.bitsPerPixel = 10,
@@ -352,6 +377,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = true,
 	} },
 	{ formats::SGRBG10_CSI2P, {
+		.name = "SGRBG10_CSI2P",
 		.format = formats::SGRBG10_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),
 		.bitsPerPixel = 10,
@@ -359,6 +385,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = true,
 	} },
 	{ formats::SRGGB10_CSI2P, {
+		.name = "SRGGB10_CSI2P",
 		.format = formats::SRGGB10_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),
 		.bitsPerPixel = 10,
@@ -366,6 +393,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = true,
 	} },
 	{ formats::SBGGR12, {
+		.name = "SBGGR12",
 		.format = formats::SBGGR12,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),
 		.bitsPerPixel = 12,
@@ -373,6 +401,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SGBRG12, {
+		.name = "SGBRG12",
 		.format = formats::SGBRG12,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),
 		.bitsPerPixel = 12,
@@ -380,6 +409,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SGRBG12, {
+		.name = "SGRBG12",
 		.format = formats::SGRBG12,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),
 		.bitsPerPixel = 12,
@@ -387,6 +417,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SRGGB12, {
+		.name = "SRGGB12",
 		.format = formats::SRGGB12,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),
 		.bitsPerPixel = 12,
@@ -394,6 +425,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = false,
 	} },
 	{ formats::SBGGR12_CSI2P, {
+		.name = "SBGGR12_CSI2P",
 		.format = formats::SBGGR12_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),
 		.bitsPerPixel = 12,
@@ -401,6 +433,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = true,
 	} },
 	{ formats::SGBRG12_CSI2P, {
+		.name = "SGBRG12_CSI2P",
 		.format = formats::SGBRG12_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),
 		.bitsPerPixel = 12,
@@ -408,6 +441,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = true,
 	} },
 	{ formats::SGRBG12_CSI2P, {
+		.name = "SGRBG12_CSI2P",
 		.format = formats::SGRBG12_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),
 		.bitsPerPixel = 12,
@@ -415,6 +449,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.packed = true,
 	} },
 	{ formats::SRGGB12_CSI2P, {
+		.name = "SRGGB12_CSI2P",
 		.format = formats::SRGGB12_CSI2P,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),
 		.bitsPerPixel = 12,
@@ -424,6 +459,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 
 	/* Compressed formats. */
 	{ formats::MJPEG, {
+		.name = "MJPEG",
 		.format = formats::MJPEG,
 		.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
 		.bitsPerPixel = 0,
@@ -453,8 +489,8 @@  const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
 	const auto iter = pixelFormatInfo.find(format);
 	if (iter == pixelFormatInfo.end()) {
 		LOG(Formats, Warning)
-			<< "Unsupported pixel format "
-			<< format.toString();
+			<< "Unsupported pixel format 0x"
+			<< std::hex << format.fourcc();
 		return invalid;
 	}
 
diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
index f191851..07e7af0 100644
--- a/src/libcamera/pixel_format.cpp
+++ b/src/libcamera/pixel_format.cpp
@@ -6,6 +6,7 @@ 
  */
 
 #include <libcamera/formats.h>
+#include "libcamera/internal/formats.h"
 #include <libcamera/pixel_format.h>
 
 /**
@@ -104,9 +105,29 @@  bool PixelFormat::operator<(const PixelFormat &other) const
  */
 std::string PixelFormat::toString() const
 {
-	char str[11];
-	snprintf(str, 11, "0x%08x", fourcc_);
-	return str;
+	PixelFormat format = PixelFormat(fourcc_, modifier_);
+	const PixelFormatInfo &info = PixelFormatInfo::info(format);
+
+	if (!info.isValid()) {
+		if (format == PixelFormat())
+			return "<INVALID>";
+
+		char fourcc[7] = { '<',
+				   static_cast<char>(fourcc_ & 0x7f),
+				   static_cast<char>((fourcc_ >> 8) & 0x7f),
+				   static_cast<char>((fourcc_ >> 16) & 0x7f),
+				   static_cast<char>((fourcc_ >> 24) & 0x7f),
+				   '>' };
+
+		for (unsigned int i = 1; i < 5; i++) {
+			if (!isprint(fourcc[i]))
+				fourcc[i] = '.';
+		}
+
+		return fourcc;
+	}
+
+	return info.name;
 }
 
 } /* namespace libcamera */
diff --git a/test/meson.build b/test/meson.build
index a868813..7808a26 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -34,6 +34,7 @@  internal_tests = [
     ['message',                         'message.cpp'],
     ['object',                          'object.cpp'],
     ['object-invoke',                   'object-invoke.cpp'],
+    ['pixel-format',                    'pixel-format.cpp'],
     ['signal-threads',                  'signal-threads.cpp'],
     ['threads',                         'threads.cpp'],
     ['timer',                           'timer.cpp'],
diff --git a/test/pixel-format.cpp b/test/pixel-format.cpp
new file mode 100644
index 0000000..5b9cdc9
--- /dev/null
+++ b/test/pixel-format.cpp
@@ -0,0 +1,49 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2020, Kaaira Gupta
+ * libcamera pixel format handling test
+ */
+
+#include <iostream>
+#include <vector>
+
+#include <libcamera/formats.h>
+#include <libcamera/pixel_format.h>
+
+#include "test.h"
+
+using namespace std;
+using namespace libcamera;
+
+class PixelFormatTest : public Test
+{
+protected:
+	int run()
+	{
+		std::vector<std::pair<PixelFormat, const char *>> formatsMap{
+			{ formats::R8, "R8" },
+			{ formats::SRGGB10_CSI2P, "SRGGB10_CSI2P" },
+			{ PixelFormat(0, 0), "<INVALID>" },
+			{ PixelFormat(0x20203843), "<C8  >" }
+		};
+
+		for (const auto &format : formatsMap) {
+			if ((format.first).toString() != format.second) {
+				cerr << "Failed to convert PixelFormat "
+				     << format.first.fourcc() << " to string"
+				     << endl;
+				return TestFail;
+			}
+		}
+
+		if (PixelFormat().toString() != "<INVALID>") {
+			cerr << "Failed to convert default PixelFormat to string"
+			     << endl;
+			return TestFail;
+		}
+
+		return TestPass;
+	}
+};
+
+TEST_REGISTER(PixelFormatTest)