[libcamera-devel] libcamera: PixelFormat: Replace hex with fourcc and modifiers
diff mbox series

Message ID 20200331210525.GA4378@kaaira-HP-Pavilion-Notebook
State Superseded
Headers show
Series
  • [libcamera-devel] libcamera: PixelFormat: Replace hex with fourcc and modifiers
Related show

Commit Message

Kaaira Gupta March 31, 2020, 9:05 p.m. UTC 
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>
---
 src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-
 test/meson.build               |   1 +
 test/pixel-format.cpp          |  55 ++++++++++
 3 files changed, 245 insertions(+), 3 deletions(-)
 create mode 100644 test/pixel-format.cpp

Comments

Kieran Bingham April 1, 2020, 11:35 a.m. UTC | #1 
Hi Kaaira,

On 31/03/2020 22:05, 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>
> ---
>  src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-
>  test/meson.build               |   1 +
>  test/pixel-format.cpp          |  55 ++++++++++
>  3 files changed, 245 insertions(+), 3 deletions(-)
>  create mode 100644 test/pixel-format.cpp
> 
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index 87557d9..398dbb2 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -6,6 +6,8 @@
>   */
>  
>  #include <libcamera/pixelformats.h>
> +#include <map>
> +#include <string.h>
>  
>  /**
>   * \file pixelformats.h
> @@ -108,9 +110,193 @@ 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_ == 0)

As DRM_FORMAT_INVALID == 0, I'd express this as:

	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;
> +

As we continually reference the 'first' modifier only, perhaps it's
worth taking copy of it to a local var rather than referencing the
*modifiers_.begin() each time.

Or alternatively, convert modifiers_ from being a set to a single value
before this patch.

> +	if (*modifiers_.begin() == 72057594037927935) {

Try to avoid magic numbers, What is 72057594037927935 ?
If this is DRM_FORMAT_MOD_INVALID, then just use that directly.

> +		modifier = " <INVALID> modifier";
> +		return ss + modifier;
> +	}
> +
> +	if (*modifiers_.begin() == 0xf) {
> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_MASK";
> +		return ss + modifier;


This 'mask' is not a value to express, but it is stating the 'mask'
which can be used to extract the 'MOD_BLOCK_SIZE'.

So you could express somethign like:

uint8_t block_size = modifierValue & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;

But note that the AFBC modifiers appear to be ARM vendor specific, so
they should be handled in that vendor code switch statement.


> +	} else if (*modifiers_.begin() == 1ULL) {
> +		modifier = "AFBC_FORMAT_MOD_BLOCK_SIZE_16x16";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == 2ULL) {
> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == 3ULL) {
> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_64x4";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == 4ULL) {
> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 4)) {
> +		modifier = " AFBC_FORMAT_MOD_YTR";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 5)) {
> +		modifier = " AFBC_FORMAT_MOD_SPLIT";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 6)) {
> +		modifier = " AFBC_FORMAT_MOD_SPARS";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 7)) {
> +		modifier = " AFBC_FORMAT_MOD_CBR";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 8)) {
> +		modifier = " AFBC_FORMAT_MOD_TILED";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 9)) {
> +		modifier = " AFBC_FORMAT_MOD_SC";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 10)) {
> +		modifier = " AFBC_FORMAT_MOD_DB";
> +		return ss + modifier;
> +	} else if (*modifiers_.begin() == (1ULL << 11)) {
> +		modifier = " AFBC_FORMAT_MOD_BCH";
> +		return ss + modifier;
> +	}


I suspect that we could simplify all of the conversions by using tables
such as the vendor table below (populated with a similar macro to make
sure the values and strings come directly from the defines given in
drm_fourcc.h).

Would you be able to experiment with that a little to see if you can
simplify the code?


> +
> +	std::map<long int, std::string> vendors = {

I would suggest we use the preprocessor to populate this table from the
defines directly:

#define PIXELFORMAT_VENDOR(vendor) \
	{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }

std::map<long int, std::string> vendors = {
	PIXELFORMAT_VENDOR(NONE),
	PIXELFORMAT_VENDOR(INTEL),
	PIXELFORMAT_VENDOR(AMD),
	PIXELFORMAT_VENDOR(NVIDIA),
	... and the rest ...
}	


> +		{ 0, "NONE" },
> +		{ 0x01, "INTEL" },
> +		{ 0x02, "AMD" },
> +		{ 0x03, "NVIDIA" },
> +		{ 0x04, "SAMSUNG" },
> +		{ 0x05, "QCOM" },
> +		{ 0x06, "VIVANTE" },
> +		{ 0x07, "BROADCOM" },
> +		{ 0x08, "ARM" },
> +		{ 0x09, "ALLWINNER" },
> +		{ 0x0a, "MIPI" }
> +	};
> +
> +	long int vendorCode = (*modifiers_.begin() >> 56) & 0xff;
> +
> +	std::string vendor = vendors[vendorCode];
> +
> +	int modifierValue = *modifiers_.begin() & 0xff;

Some vendors can utilise more bits, I think the modifierValue should be
a uint64_t, with the vendor bits masked out.


> +	std::string vendorSpecification;
> +
> +	switch (vendorCode) {
> +	case 0: {

The switch statements should be more expressive, and not use magic
numbers either:

	case DRM_FORMAT_MOD_VENDOR_NONE: {


> +		if (modifierValue == 0)
> +			vendorSpecification = "Linear Layout";
> +		break;
> +	}
> +	case 0x01: {

	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 0x02: {
> +		// no specifications
> +		break;
> +	}
> +	case 0x03: {
> +		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 0x04: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "Tiled, 64 (pixels) x 32 (lines)";
> +		else if (modifierValue == 2)
> +			vendorSpecification = "Tiled, 16 (pixels) x 16 (lines)";
> +		break;
> +	}
> +	case 0x05: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "Compressed";
> +		break;
> +	}
> +	case 0x06: {
> +		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 0x07: {
> +		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 0x08: {
> +		vendorSpecification = "DRM_FORMAT_MOD_ARM_AFBC(" + modifierValue;
> +		break;
> +	}
> +	case 0x09: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "Tiled";
> +		break;
> +	}
> +	case 0x0a: {
> +		if (modifierValue == 1)
> +			vendorSpecification = "CSI-2 packed";
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +
> +	modifier = vendor + ' ' + vendorSpecification;
> +	std::string formatString(ss);
> +
> +	return formatString + ' ' + modifier;
>  }
>  
>  } /* 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..9676f27
> --- /dev/null
> +++ b/test/pixel-format.cpp
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.

This is a new test being added, so the year is 2020, and I think you can
own the copyright on files you create from scratch...

 Copyright (C) 2020, Kaaira Gupta

> + *
> + * libcamera pixel format handling test
> + */
> +
> +#include <iostream>
> +#include <vector>
> +
> +#include "libcamera/pixelformats.h"
> +#include "utils.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(0, { DRM_FORMAT_MOD_INVALID }), "<INVALID>" },
> +			{ PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),
> +			  "RGGB NONE Linear Layout" },
> +			{ PixelFormat(DRM_FORMAT_C8,
> +			{ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),
> +			  "C8   BROADCOM SAND32" },
> +			{ PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),
> +			  "YUV9 AFBC_FORMAT_MOD_SPLIT" },
> +			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),
> +			  "....-BE <INVALID> modifier" }
> +		};
> +		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)
>
Kaaira Gupta April 2, 2020, 12:56 p.m. UTC | #2 
On Wed, Apr 01, 2020 at 12:35:56PM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> On 31/03/2020 22:05, 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>
> > ---
> >  src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-
> >  test/meson.build               |   1 +
> >  test/pixel-format.cpp          |  55 ++++++++++
> >  3 files changed, 245 insertions(+), 3 deletions(-)
> >  create mode 100644 test/pixel-format.cpp
> > 
> > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> > index 87557d9..398dbb2 100644
> > --- a/src/libcamera/pixelformats.cpp
> > +++ b/src/libcamera/pixelformats.cpp
> > @@ -6,6 +6,8 @@
> >   */
> >  
> >  #include <libcamera/pixelformats.h>
> > +#include <map>
> > +#include <string.h>
> >  
> >  /**
> >   * \file pixelformats.h
> > @@ -108,9 +110,193 @@ 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_ == 0)
> 
> As DRM_FORMAT_INVALID == 0, I'd express this as:
> 
> 	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;
> > +
> 
> As we continually reference the 'first' modifier only, perhaps it's
> worth taking copy of it to a local var rather than referencing the
> *modifiers_.begin() each time.
> 
> Or alternatively, convert modifiers_ from being a set to a single value
> before this patch.

Okay, I'll do this first :)

> 
> > +	if (*modifiers_.begin() == 72057594037927935) {
> 
> Try to avoid magic numbers, What is 72057594037927935 ?
> If this is DRM_FORMAT_MOD_INVALID, then just use that directly.
> 
> > +		modifier = " <INVALID> modifier";
> > +		return ss + modifier;
> > +	}
> > +
> > +	if (*modifiers_.begin() == 0xf) {
> > +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_MASK";
> > +		return ss + modifier;
> 
> 
> This 'mask' is not a value to express, but it is stating the 'mask'
> which can be used to extract the 'MOD_BLOCK_SIZE'.

I didn't understand you well, can you elaborate please?

> 
> So you could express somethign like:
> 
> uint8_t block_size = modifierValue & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;
> 
> But note that the AFBC modifiers appear to be ARM vendor specific, so
> they should be handled in that vendor code switch statement.

Oh, okay.

> 
> 
> > +	} else if (*modifiers_.begin() == 1ULL) {
> > +		modifier = "AFBC_FORMAT_MOD_BLOCK_SIZE_16x16";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == 2ULL) {
> > +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == 3ULL) {
> > +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_64x4";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == 4ULL) {
> > +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 4)) {
> > +		modifier = " AFBC_FORMAT_MOD_YTR";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 5)) {
> > +		modifier = " AFBC_FORMAT_MOD_SPLIT";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 6)) {
> > +		modifier = " AFBC_FORMAT_MOD_SPARS";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 7)) {
> > +		modifier = " AFBC_FORMAT_MOD_CBR";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 8)) {
> > +		modifier = " AFBC_FORMAT_MOD_TILED";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 9)) {
> > +		modifier = " AFBC_FORMAT_MOD_SC";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 10)) {
> > +		modifier = " AFBC_FORMAT_MOD_DB";
> > +		return ss + modifier;
> > +	} else if (*modifiers_.begin() == (1ULL << 11)) {
> > +		modifier = " AFBC_FORMAT_MOD_BCH";
> > +		return ss + modifier;
> > +	}
> 
> 
> I suspect that we could simplify all of the conversions by using tables
> such as the vendor table below (populated with a similar macro to make
> sure the values and strings come directly from the defines given in
> drm_fourcc.h).

Again, I don't understand what exactly you want here? :3
Can you elaborate please?

> 
> Would you be able to experiment with that a little to see if you can
> simplify the code?
> 
> 
> > +
> > +	std::map<long int, std::string> vendors = {
> 
> I would suggest we use the preprocessor to populate this table from the
> defines directly:

Yes


> 
> #define PIXELFORMAT_VENDOR(vendor) \
> 	{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }
> 
> std::map<long int, std::string> vendors = {
> 	PIXELFORMAT_VENDOR(NONE),
> 	PIXELFORMAT_VENDOR(INTEL),
> 	PIXELFORMAT_VENDOR(AMD),
> 	PIXELFORMAT_VENDOR(NVIDIA),
> 	... and the rest ...
> }	
> 
> 
> > +		{ 0, "NONE" },
> > +		{ 0x01, "INTEL" },
> > +		{ 0x02, "AMD" },
> > +		{ 0x03, "NVIDIA" },
> > +		{ 0x04, "SAMSUNG" },
> > +		{ 0x05, "QCOM" },
> > +		{ 0x06, "VIVANTE" },
> > +		{ 0x07, "BROADCOM" },
> > +		{ 0x08, "ARM" },
> > +		{ 0x09, "ALLWINNER" },
> > +		{ 0x0a, "MIPI" }
> > +	};
> > +
> > +	long int vendorCode = (*modifiers_.begin() >> 56) & 0xff;
> > +
> > +	std::string vendor = vendors[vendorCode];
> > +
> > +	int modifierValue = *modifiers_.begin() & 0xff;
> 
> Some vendors can utilise more bits, I think the modifierValue should be
> a uint64_t, with the vendor bits masked out.

Yes you are right..I'll change this

> 
> 
> > +	std::string vendorSpecification;
> > +
> > +	switch (vendorCode) {
> > +	case 0: {
> 
> The switch statements should be more expressive, and not use magic
> numbers either:

Okay, will make the changes


> 
> 	case DRM_FORMAT_MOD_VENDOR_NONE: {
> 
> 
> > +		if (modifierValue == 0)
> > +			vendorSpecification = "Linear Layout";
> > +		break;
> > +	}
> > +	case 0x01: {
> 
> 	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 0x02: {
> > +		// no specifications
> > +		break;
> > +	}
> > +	case 0x03: {
> > +		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 0x04: {
> > +		if (modifierValue == 1)
> > +			vendorSpecification = "Tiled, 64 (pixels) x 32 (lines)";
> > +		else if (modifierValue == 2)
> > +			vendorSpecification = "Tiled, 16 (pixels) x 16 (lines)";
> > +		break;
> > +	}
> > +	case 0x05: {
> > +		if (modifierValue == 1)
> > +			vendorSpecification = "Compressed";
> > +		break;
> > +	}
> > +	case 0x06: {
> > +		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 0x07: {
> > +		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 0x08: {
> > +		vendorSpecification = "DRM_FORMAT_MOD_ARM_AFBC(" + modifierValue;
> > +		break;
> > +	}
> > +	case 0x09: {
> > +		if (modifierValue == 1)
> > +			vendorSpecification = "Tiled";
> > +		break;
> > +	}
> > +	case 0x0a: {
> > +		if (modifierValue == 1)
> > +			vendorSpecification = "CSI-2 packed";
> > +		break;
> > +	}
> > +	default:
> > +		break;
> > +	}
> > +
> > +	modifier = vendor + ' ' + vendorSpecification;
> > +	std::string formatString(ss);
> > +
> > +	return formatString + ' ' + modifier;
> >  }
> >  
> >  } /* 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..9676f27
> > --- /dev/null
> > +++ b/test/pixel-format.cpp
> > @@ -0,0 +1,55 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> 
> This is a new test being added, so the year is 2020, and I think you can
> own the copyright on files you create from scratch...

Okayy..I didn't know how this thing works, so I just copied it from a
file :P

> 
>  Copyright (C) 2020, Kaaira Gupta
> 
> > + *
> > + * libcamera pixel format handling test
> > + */
> > +
> > +#include <iostream>
> > +#include <vector>
> > +
> > +#include "libcamera/pixelformats.h"
> > +#include "utils.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(0, { DRM_FORMAT_MOD_INVALID }), "<INVALID>" },
> > +			{ PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),
> > +			  "RGGB NONE Linear Layout" },
> > +			{ PixelFormat(DRM_FORMAT_C8,
> > +			{ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),
> > +			  "C8   BROADCOM SAND32" },
> > +			{ PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),
> > +			  "YUV9 AFBC_FORMAT_MOD_SPLIT" },
> > +			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),
> > +			  "....-BE <INVALID> modifier" }
> > +		};
> > +		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)
> > 

Thanks for the time!

> 
> -- 
> Regards
> --
> Kieran
Kieran Bingham April 3, 2020, 10:07 a.m. UTC | #3 
Hi Kaaira,

On 02/04/2020 13:56, Kaaira Gupta wrote:
> On Wed, Apr 01, 2020 at 12:35:56PM +0100, Kieran Bingham wrote:
>> Hi Kaaira,
>>
>> On 31/03/2020 22:05, 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>
>>> ---
>>>  src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-
>>>  test/meson.build               |   1 +
>>>  test/pixel-format.cpp          |  55 ++++++++++
>>>  3 files changed, 245 insertions(+), 3 deletions(-)
>>>  create mode 100644 test/pixel-format.cpp
>>>
>>> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
>>> index 87557d9..398dbb2 100644
>>> --- a/src/libcamera/pixelformats.cpp
>>> +++ b/src/libcamera/pixelformats.cpp
>>> @@ -6,6 +6,8 @@
>>>   */
>>>  
>>>  #include <libcamera/pixelformats.h>
>>> +#include <map>
>>> +#include <string.h>
>>>  
>>>  /**
>>>   * \file pixelformats.h
>>> @@ -108,9 +110,193 @@ 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_ == 0)
>>
>> As DRM_FORMAT_INVALID == 0, I'd express this as:
>>
>> 	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;
>>> +
>>
>> As we continually reference the 'first' modifier only, perhaps it's
>> worth taking copy of it to a local var rather than referencing the
>> *modifiers_.begin() each time.
>>
>> Or alternatively, convert modifiers_ from being a set to a single value
>> before this patch.
> 
> Okay, I'll do this first :)
> 
>>
>>> +	if (*modifiers_.begin() == 72057594037927935) {
>>
>> Try to avoid magic numbers, What is 72057594037927935 ?
>> If this is DRM_FORMAT_MOD_INVALID, then just use that directly.
>>
>>> +		modifier = " <INVALID> modifier";
>>> +		return ss + modifier;
>>> +	}
>>> +
>>> +	if (*modifiers_.begin() == 0xf) {
>>> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_MASK";
>>> +		return ss + modifier;
>>
>>
>> This 'mask' is not a value to express, but it is stating the 'mask'
>> which can be used to extract the 'MOD_BLOCK_SIZE'.
> 
> I didn't understand you well, can you elaborate please?

The value here, is not one that would be expected to be printed itself
as a string.

When dealing with bitfields (which is what the modifier essentially is),
it is often the case that a mask is provided to define which bits are
utilised.

In this case, the AFBC_FORMAT_MOD_BLOCK_SIZE_MASK == 0xF which
represents 4 bits. (b00001111)

Now, for example sake, imagine we have an 8 bit value (i.e. an 'unsigned
char') with the value 0x32.., we can use the 'BLOCK_SIZE_MASK' to
extract the appropriate 'block-size', without being affected by the
other bits that are set in the original value.


Try building and playing with the following code if you want to
experiment: (online at http://cpp.sh/8toy if you want too)


#include <stdio.h>

#define AFBC_FORMAT_MOD_BLOCK_SIZE_MASK         0xf
#define AFBC_FORMAT_MOD_BLOCK_SIZE_32x8         (2ULL)
#define AFBC_FORMAT_MOD_CBR                     (1ULL <<  7)

int main(int argc, char **argv)
{
  /* val = 0x82 */
  unsigned char val = AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | AFBC_FORMAT_MOD_CBR;
  unsigned int block_size = val & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;

  printf("Original Value = 0x%x\n", val); /* = 0x82 */
  printf("Block size = %d\n", block_size); /* = 2 */

  switch (block_size) {
  case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
        printf("AFBC_FORMAT_MOD_BLOCK_SIZE_32x8\n");
        break;

  default:
     printf("Unknown format\n");
  }

  return 0;
}

For more reading material if this is new to you:
https://www.coranac.com/documents/working-with-bits-and-bitfields/

Section 3 on that write up shows quite nicely what we're doing here.

We don't use native bitfields support from the C compiler in cases like
this, because they are not portable and are susceptible to endianness
issues which are important to consider when working with the kernel,
which runs on multiple architectures.



>> So you could express somethign like:
>>
>> uint8_t block_size = modifierValue & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;
>>
>> But note that the AFBC modifiers appear to be ARM vendor specific, so
>> they should be handled in that vendor code switch statement.
> 
> Oh, okay.
> 
>>
>>
>>> +	} else if (*modifiers_.begin() == 1ULL) {
>>> +		modifier = "AFBC_FORMAT_MOD_BLOCK_SIZE_16x16";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == 2ULL) {
>>> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == 3ULL) {
>>> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_64x4";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == 4ULL) {
>>> +		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 4)) {
>>> +		modifier = " AFBC_FORMAT_MOD_YTR";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 5)) {
>>> +		modifier = " AFBC_FORMAT_MOD_SPLIT";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 6)) {
>>> +		modifier = " AFBC_FORMAT_MOD_SPARS";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 7)) {
>>> +		modifier = " AFBC_FORMAT_MOD_CBR";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 8)) {
>>> +		modifier = " AFBC_FORMAT_MOD_TILED";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 9)) {
>>> +		modifier = " AFBC_FORMAT_MOD_SC";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 10)) {
>>> +		modifier = " AFBC_FORMAT_MOD_DB";
>>> +		return ss + modifier;
>>> +	} else if (*modifiers_.begin() == (1ULL << 11)) {
>>> +		modifier = " AFBC_FORMAT_MOD_BCH";
>>> +		return ss + modifier;
>>> +	}
>>
>>
>> I suspect that we could simplify all of the conversions by using tables
>> such as the vendor table below (populated with a similar macro to make
>> sure the values and strings come directly from the defines given in
>> drm_fourcc.h).
> 
> Again, I don't understand what exactly you want here? :3
> Can you elaborate please?

For each define, you require 3 lines of code:

 +	} else if (*modifiers_.begin() == (1ULL << 10)) {
 +		modifier = " AFBC_FORMAT_MOD_DB";
 +		return ss + modifier;

This also references 'magic' values where you have to specify the value
directly in the code. That can be prone to errors, and has the effect of
looking like 'magic numbers' so we try to avoid it.

A solution to simplifying this code is to use a table, which is then
iterated.

Try to see if you can generate a table like the following:

std::map<long int, std::string> afbc_format_mods = {
 	AFBC_FORMAT_MOD(BLOCK_SIZE_16x16),
	...
 	AFBC_FORMAT_MOD(BLOCK_SIZE_32x8_64x4),
}	

That will then take only one line per 'define' of code, and ensure that
the value provided by the define gets converted to the appropriate
string, like the way you do with the vendor string. You might need some
additional validation to first ensure that the value is in the map
table, and produce an 'unknown-bit-field' or such string if it isn't given.


Once you've got the table coded for the block-sizes, have a look at how
the other fields are used.

Where they use a construct like :

   1ULL << 4

That means they are specifying a single bit within the bitfields, (I
prefer it when people use the syntax BIT(4) in those cases),

So it's perfectly valid to have as we did above in our code sample:

  uint8_t val = AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | AFBC_FORMAT_MOD_CBR;


And in that event, we would want to return a string containing both parts:

i.e. like:
 "ARM: BLOCK_SIZE_32x8 | CBR"

Feel free to post an initial rework as you go if you want help (i.e.
don't worry if it doesn't print all fields just yet)


>> Would you be able to experiment with that a little to see if you can
>> simplify the code?
>>
>>
>>> +
>>> +	std::map<long int, std::string> vendors = {
>>
>> I would suggest we use the preprocessor to populate this table from the
>> defines directly:
> 
> Yes
> 
> 
>>
>> #define PIXELFORMAT_VENDOR(vendor) \
>> 	{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }
>>
>> std::map<long int, std::string> vendors = {
>> 	PIXELFORMAT_VENDOR(NONE),
>> 	PIXELFORMAT_VENDOR(INTEL),
>> 	PIXELFORMAT_VENDOR(AMD),
>> 	PIXELFORMAT_VENDOR(NVIDIA),
>> 	... and the rest ...
>> }	
>>
>>
>>> +		{ 0, "NONE" },
>>> +		{ 0x01, "INTEL" },
>>> +		{ 0x02, "AMD" },
>>> +		{ 0x03, "NVIDIA" },
>>> +		{ 0x04, "SAMSUNG" },
>>> +		{ 0x05, "QCOM" },
>>> +		{ 0x06, "VIVANTE" },
>>> +		{ 0x07, "BROADCOM" },
>>> +		{ 0x08, "ARM" },
>>> +		{ 0x09, "ALLWINNER" },
>>> +		{ 0x0a, "MIPI" }
>>> +	};
>>> +
>>> +	long int vendorCode = (*modifiers_.begin() >> 56) & 0xff;
>>> +
>>> +	std::string vendor = vendors[vendorCode];
>>> +
>>> +	int modifierValue = *modifiers_.begin() & 0xff;
>>
>> Some vendors can utilise more bits, I think the modifierValue should be
>> a uint64_t, with the vendor bits masked out.
> 
> Yes you are right..I'll change this
> 
>>
>>
>>> +	std::string vendorSpecification;
>>> +
>>> +	switch (vendorCode) {
>>> +	case 0: {
>>
>> The switch statements should be more expressive, and not use magic
>> numbers either:
> 
> Okay, will make the changes
> 
> 
>>
>> 	case DRM_FORMAT_MOD_VENDOR_NONE: {
>>
>>
>>> +		if (modifierValue == 0)
>>> +			vendorSpecification = "Linear Layout";
>>> +		break;
>>> +	}
>>> +	case 0x01: {
>>
>> 	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 0x02: {
>>> +		// no specifications
>>> +		break;
>>> +	}
>>> +	case 0x03: {
>>> +		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 0x04: {
>>> +		if (modifierValue == 1)
>>> +			vendorSpecification = "Tiled, 64 (pixels) x 32 (lines)";
>>> +		else if (modifierValue == 2)
>>> +			vendorSpecification = "Tiled, 16 (pixels) x 16 (lines)";
>>> +		break;
>>> +	}
>>> +	case 0x05: {
>>> +		if (modifierValue == 1)
>>> +			vendorSpecification = "Compressed";
>>> +		break;
>>> +	}
>>> +	case 0x06: {
>>> +		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 0x07: {
>>> +		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 0x08: {
>>> +		vendorSpecification = "DRM_FORMAT_MOD_ARM_AFBC(" + modifierValue;
>>> +		break;
>>> +	}
>>> +	case 0x09: {
>>> +		if (modifierValue == 1)
>>> +			vendorSpecification = "Tiled";
>>> +		break;
>>> +	}
>>> +	case 0x0a: {
>>> +		if (modifierValue == 1)
>>> +			vendorSpecification = "CSI-2 packed";
>>> +		break;
>>> +	}
>>> +	default:
>>> +		break;
>>> +	}
>>> +
>>> +	modifier = vendor + ' ' + vendorSpecification;
>>> +	std::string formatString(ss);
>>> +
>>> +	return formatString + ' ' + modifier;
>>>  }
>>>  
>>>  } /* 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..9676f27
>>> --- /dev/null
>>> +++ b/test/pixel-format.cpp
>>> @@ -0,0 +1,55 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright (C) 2019, Google Inc.
>>
>> This is a new test being added, so the year is 2020, and I think you can
>> own the copyright on files you create from scratch...
> 
> Okayy..I didn't know how this thing works, so I just copied it from a
> file :P

That's perfectly understandable :-)

Normally if you make a copy of an existing file and retain a portion of
it's content, you would retain the existing copyright ... but in this
instance I think excepting boilerplate stuff - this is your code addition.


>>  Copyright (C) 2020, Kaaira Gupta
>>
>>> + *
>>> + * libcamera pixel format handling test
>>> + */
>>> +
>>> +#include <iostream>
>>> +#include <vector>
>>> +
>>> +#include "libcamera/pixelformats.h"
>>> +#include "utils.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(0, { DRM_FORMAT_MOD_INVALID }), "<INVALID>" },
>>> +			{ PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),
>>> +			  "RGGB NONE Linear Layout" },
>>> +			{ PixelFormat(DRM_FORMAT_C8,
>>> +			{ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),
>>> +			  "C8   BROADCOM SAND32" },
>>> +			{ PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),
>>> +			  "YUV9 AFBC_FORMAT_MOD_SPLIT" },
>>> +			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),
>>> +			  "....-BE <INVALID> modifier" }

Because that's a 'table', I would be tempted to go beyond the 80 char
limit (we have a soft 80 char, and a 'harder' 120 char width limit) to
try to get those on one line. I always think tables should look like a
spreadsheet in columns where possible - but that can take a lot of space
sometimes :-) So it's probably a controversial stance (like how people
argue between tabs vs spaces).

It looks like however the last DRM_FORMAT_BIG_ENDIAN,
DRM_FORMAT_MOD_INVALID test possibly goes over 120 though so it might
cause more hassle than it's worth :-)


>>> +		};
>>> +		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)
>>>
> 
> Thanks for the time!

You're welcome - thanks for all of your efforts :-)

>>
>> -- 
>> Regards
>> --
>> Kieran

Patch
diff mbox series 

diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
index 87557d9..398dbb2 100644
--- a/src/libcamera/pixelformats.cpp
+++ b/src/libcamera/pixelformats.cpp
@@ -6,6 +6,8 @@ 
  */
 
 #include <libcamera/pixelformats.h>
+#include <map>
+#include <string.h>
 
 /**
  * \file pixelformats.h
@@ -108,9 +110,193 @@  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_ == 0)
+		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 (*modifiers_.begin() == 72057594037927935) {
+		modifier = " <INVALID> modifier";
+		return ss + modifier;
+	}
+
+	if (*modifiers_.begin() == 0xf) {
+		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_MASK";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == 1ULL) {
+		modifier = "AFBC_FORMAT_MOD_BLOCK_SIZE_16x16";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == 2ULL) {
+		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == 3ULL) {
+		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_64x4";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == 4ULL) {
+		modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 4)) {
+		modifier = " AFBC_FORMAT_MOD_YTR";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 5)) {
+		modifier = " AFBC_FORMAT_MOD_SPLIT";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 6)) {
+		modifier = " AFBC_FORMAT_MOD_SPARS";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 7)) {
+		modifier = " AFBC_FORMAT_MOD_CBR";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 8)) {
+		modifier = " AFBC_FORMAT_MOD_TILED";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 9)) {
+		modifier = " AFBC_FORMAT_MOD_SC";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 10)) {
+		modifier = " AFBC_FORMAT_MOD_DB";
+		return ss + modifier;
+	} else if (*modifiers_.begin() == (1ULL << 11)) {
+		modifier = " AFBC_FORMAT_MOD_BCH";
+		return ss + modifier;
+	}
+
+	std::map<long int, std::string> vendors = {
+		{ 0, "NONE" },
+		{ 0x01, "INTEL" },
+		{ 0x02, "AMD" },
+		{ 0x03, "NVIDIA" },
+		{ 0x04, "SAMSUNG" },
+		{ 0x05, "QCOM" },
+		{ 0x06, "VIVANTE" },
+		{ 0x07, "BROADCOM" },
+		{ 0x08, "ARM" },
+		{ 0x09, "ALLWINNER" },
+		{ 0x0a, "MIPI" }
+	};
+
+	long int vendorCode = (*modifiers_.begin() >> 56) & 0xff;
+
+	std::string vendor = vendors[vendorCode];
+
+	int modifierValue = *modifiers_.begin() & 0xff;
+	std::string vendorSpecification;
+
+	switch (vendorCode) {
+	case 0: {
+		if (modifierValue == 0)
+			vendorSpecification = "Linear Layout";
+		break;
+	}
+	case 0x01: {
+		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 0x02: {
+		// no specifications
+		break;
+	}
+	case 0x03: {
+		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 0x04: {
+		if (modifierValue == 1)
+			vendorSpecification = "Tiled, 64 (pixels) x 32 (lines)";
+		else if (modifierValue == 2)
+			vendorSpecification = "Tiled, 16 (pixels) x 16 (lines)";
+		break;
+	}
+	case 0x05: {
+		if (modifierValue == 1)
+			vendorSpecification = "Compressed";
+		break;
+	}
+	case 0x06: {
+		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 0x07: {
+		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 0x08: {
+		vendorSpecification = "DRM_FORMAT_MOD_ARM_AFBC(" + modifierValue;
+		break;
+	}
+	case 0x09: {
+		if (modifierValue == 1)
+			vendorSpecification = "Tiled";
+		break;
+	}
+	case 0x0a: {
+		if (modifierValue == 1)
+			vendorSpecification = "CSI-2 packed";
+		break;
+	}
+	default:
+		break;
+	}
+
+	modifier = vendor + ' ' + vendorSpecification;
+	std::string formatString(ss);
+
+	return formatString + ' ' + modifier;
 }
 
 } /* 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..9676f27
--- /dev/null
+++ b/test/pixel-format.cpp
@@ -0,0 +1,55 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2019, Google Inc.
+ *
+ * libcamera pixel format handling test
+ */
+
+#include <iostream>
+#include <vector>
+
+#include "libcamera/pixelformats.h"
+#include "utils.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(0, { DRM_FORMAT_MOD_INVALID }), "<INVALID>" },
+			{ PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),
+			  "RGGB NONE Linear Layout" },
+			{ PixelFormat(DRM_FORMAT_C8,
+			{ DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),
+			  "C8   BROADCOM SAND32" },
+			{ PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),
+			  "YUV9 AFBC_FORMAT_MOD_SPLIT" },
+			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),
+			  "....-BE <INVALID> modifier" }
+		};
+		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)