[libcamera-devel,v2] libcamera: PixelFormat: Replace hex with fourcc and modifiers

3402 diff mbox series

Print fourCC characters instead of the hex values in toString() as they
are easier to comprehend. Also, print the corresponding modifiers of the
DRM formats so that it can be more specific.

Write the tests for them as well.

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

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.

 src/libcamera/pixelformats.cpp | 197 ++++++++++++++++++++++++++++++++-
 test/meson.build               |   1 +
 test/pixel-format.cpp          |  49 ++++++++
 3 files changed, 244 insertions(+), 3 deletions(-)
 create mode 100644 test/pixel-format.cpp

Hi Kaaira,

On 04/04/2020 15:31, Kaaira Gupta wrote:
> Print fourCC characters instead of the hex values in toString() as they
> are easier to comprehend. Also, print the corresponding modifiers of the
> DRM formats so that it can be more specific.
> 
> Write the tests for them as well.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
> 
> 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.
> 
>  src/libcamera/pixelformats.cpp | 197 ++++++++++++++++++++++++++++++++-
>  test/meson.build               |   1 +
>  test/pixel-format.cpp          |  49 ++++++++
>  3 files changed, 244 insertions(+), 3 deletions(-)
>  create mode 100644 test/pixel-format.cpp
> 
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index 1330dc5..81159ee 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -6,6 +6,17 @@
>   */
>  
>  #include <libcamera/pixelformats.h>
> +#include <map>
> +#include <string.h>
> +
> +#define PIXELFORMAT_VENDOR(vendor)		    \
> +        { DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }
> +
> +#define AFBC_FORMAT_MOD_BLOCK(afbcBlock)		    \
> +		{ AFBC_FORMAT_MOD_## afbcBlock, #afbcBlock }
> +
> +#define AFBC_FORMAT_MOD(modifier, mod)			     \
> +		{ modifier & (AFBC_FORMAT_MOD_## mod), #mod }

I'm sorry - as it's likely my fault, but I think we went a little too
deep here.

The only vendor code modifier we are likely to use is the
DRM_FORMAT_MOD_VENDOR_MIPI, so we probably want to greatly simplify this
conversion and only handle that vendor code.

We should still print the other vendor codes if they are given as a
hint, but theres no need to decode those directly.



>  
>  /**
>   * \file pixelformats.h
> @@ -108,9 +119,189 @@ bool PixelFormat::operator<(const PixelFormat &other) const
>   */
>  std::string PixelFormat::toString() const
>  {
> -	char str[11];
> -	snprintf(str, 11, "0x%08x", fourcc_);
> -	return str;
> +	if (fourcc_ == DRM_FORMAT_INVALID)
> +		return "<INVALID>";
> +
> +	char ss[8] = { 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 = 0; i < 4; i++) {
> +		if (!isprint(ss[i]))
> +			ss[i] = '.';
> +	}
> +
> +	if (fourcc_ & (1 << 31))
> +		strcat(ss, "-BE");
> +
> +	std::string modifier;
> +
> +	if (modifier_ == DRM_FORMAT_MOD_INVALID) {
> +		modifier = " - <INVALID> modifier";
> +		return ss + modifier;
> +	}
> +
> +	/* Map of all the vendors with their Ids: */
> +	std::map<long int, std::string> vendors = {
> +		PIXELFORMAT_VENDOR(NONE),
> +		PIXELFORMAT_VENDOR(INTEL),
> +		PIXELFORMAT_VENDOR(AMD),
> +		PIXELFORMAT_VENDOR(NVIDIA),
> +		PIXELFORMAT_VENDOR(SAMSUNG),
> +		PIXELFORMAT_VENDOR(QCOM),
> +		PIXELFORMAT_VENDOR(VIVANTE),
> +		PIXELFORMAT_VENDOR(BROADCOM),
> +		PIXELFORMAT_VENDOR(ARM),
> +		PIXELFORMAT_VENDOR(ALLWINNER),
> +		PIXELFORMAT_VENDOR(MIPI)
> +	};
> +
> +	std::map<long int, std::string> afbc_format_blocks = {
> +		AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_16x16),
> +		AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_32x8),
> +		AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_64x4),
> +		AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_32x8_64x4),
> +
> +	};
> +
> +	std::map<long int, std::string> afbc_format_mod = {
> +		AFBC_FORMAT_MOD(modifier_, YTR),
> +		AFBC_FORMAT_MOD(modifier_, SPLIT),
> +		AFBC_FORMAT_MOD(modifier_, SPARSE),
> +		AFBC_FORMAT_MOD(modifier_, CBR),
> +		AFBC_FORMAT_MOD(modifier_, TILED),
> +		AFBC_FORMAT_MOD(modifier_, SC),
> +		AFBC_FORMAT_MOD(modifier_, DB),
> +		AFBC_FORMAT_MOD(modifier_, BCH),
> +	};
> +
> +	/* Get the vendor name using its Id */
> +	long int vendorCode = (modifier_ >> 56) & 0xff;
> +	std::string vendor = vendors[vendorCode];
> +
> +	uint64_t modifierValue = modifier_ & 0xff;
> +	std::string vendorSpecification;
> +
> +	switch (vendorCode) {
> +	case DRM_FORMAT_MOD_VENDOR_NONE: {
> +		if (modifierValue == 0)
> +			vendorSpecification = "Linear Layout";
> +		break;
> +	}

We'd keep DRM_FORMAT_MOD_VENDOR_NONE of course ;-)


> +	case DRM_FORMAT_MOD_VENDOR_INTEL: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "X-tiled";
> +		else if (modifierValue == 2)
> +			vendorSpecification = "Y-tiled";
> +		else if (modifierValue == 3)
> +			vendorSpecification = "Yf-tiled";
> +		else if (modifierValue == 4)
> +			vendorSpecification = "Y-tiled CCS";
> +		else if (modifierValue == 5)
> +			vendorSpecification = "Yf-tiled CSS";
> +		else if (modifierValue == 8)
> +			vendorSpecification = "Bayer packed";
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_AMD: {
> +		// no specifications
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_NVIDIA: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "Tegra-tiled";
> +		else if (modifierValue == 0x10)
> +			vendorSpecification = "ONE_GOB | v = 0";
> +		else if (modifierValue == 0x11)
> +			vendorSpecification = "TWO_GOB | v = 1";
> +		else if (modifierValue == 0x12)
> +			vendorSpecification = "FOUR_GOB | v = 2";
> +		else if (modifierValue == 0x13)
> +			vendorSpecification = "EIGHT_GOB | v = 3";
> +		else if (modifierValue == 0x14)
> +			vendorSpecification = "SIXTEEN_GOB | v = 4";
> +		else if (modifierValue == 0x15)
> +			vendorSpecification = "THIRTYTWO_GOB | v = 5";
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_SAMSUNG: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "Tiled, 64 (pixels) x 32 (lines)";
> +		else if (modifierValue == 2)
> +			vendorSpecification = "Tiled, 16 (pixels) x 16 (lines)";
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_QCOM: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "Compressed";
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_VIVANTE: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "4 x 4 tiled";
> +		else if (modifierValue == 2)
> +			vendorSpecification = "64 x 64 super-tiled";
> +		else if (modifierValue == 3)
> +			vendorSpecification = "4 x 4 split-tiled";
> +		else if (modifierValue == 4)
> +			vendorSpecification = "64 x 64 split-super-tiled";
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_BROADCOM: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "VC4-T-Tiled";
> +		else if (modifierValue == 6)
> +			vendorSpecification = "UIF";
> +		else if (modifierValue == 2)
> +			vendorSpecification = "SAND32";
> +		else if (modifierValue == 3)
> +			vendorSpecification = "SAND64";
> +		else if (modifierValue == 4)
> +			vendorSpecification = "SAND128";
> +		else if (modifierValue == 5)
> +			vendorSpecification = "SAND256";
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_ARM: {
> +		uint8_t block_size = modifier_ & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;
> +
> +		if (afbc_format_blocks.count(block_size)) {
> +			vendorSpecification = afbc_format_blocks[block_size] + " | ";
> +		};
> +
> +		std::map<long int, std::string>::iterator it;
> +		for (it = afbc_format_mod.begin(); it != afbc_format_mod.end(); it++) {
> +			if (it->first != 0) {
> +				vendorSpecification += it->second + " | ";
> +			}
> +		}
> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_ALLWINNER: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "Tiled";
> +		break;
> +	}

So for all the other vendor, printing just the hex value should be fine,
as a default catch all (and it would be in the default case statement below)

> +	case DRM_FORMAT_MOD_VENDOR_MIPI: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "CSI-2 packed";
> +		break;
> +	}

And we should keep that one, but using modifierValue == 1 is a 'magic
value', so we should get that directly from the modifier code. We might
need to use a bitfield mask.


> +	default:
> +		break;
> +	}
> +
> +	modifier = vendor + " : " + vendorSpecification;
> +	std::string formatString(ss);
> +	formatString = formatString + " - " + modifier;
> +
> +	/* Trim last three characters of the string if last char is a space. */
> +	if (isspace(formatString.back())) {
> +		formatString = formatString.substr(0, formatString.size() - 3);
> +	}

Try to see if you can prevent adding the characters in the first place,
so they don't need to be trimmed back.


> +
> +	return formatString;
>  }
>  
>  } /* namespace libcamera */
> diff --git a/test/meson.build b/test/meson.build
> index 8ab58ac..3f97278 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -30,6 +30,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..55cd916
> --- /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/pixelformats.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class PixelFormatTest : public Test
> +{
> +protected:
> +	int run()
> +	{
> +		std::vector<std::pair<PixelFormat, const char *>> formats{
> +			{ PixelFormat(DRM_FORMAT_SRGGB8, DRM_FORMAT_MOD_INVALID), "RGGB - <INVALID> modifier" },
> +			{ PixelFormat(DRM_FORMAT_SRGGB8, DRM_FORMAT_MOD_LINEAR), "RGGB - NONE : Linear Layout" },
> +			{ PixelFormat(DRM_FORMAT_C8, DRM_FORMAT_MOD_ARM_AFBC(4)), "C8   - ARM : BLOCK_SIZE_32x8_64x4" },
> +			{ PixelFormat(DRM_FORMAT_YUV410, DRM_FORMAT_MOD_ARM_AFBC(4|1ULL << 11)), "YUV9 - ARM : BLOCK_SIZE_32x8_64x4 | BCH"},
> +			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, DRM_FORMAT_MOD_ALLWINNER_TILED), "....-BE - ALLWINNER : Tiled" }
> +		};
> +		for (const auto &format : formats) {
> +			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)
>