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

3853 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.

Describe the modifiers for MIPI vendor as it is mostly used in
libcamera. Print digits for the rest.

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

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.

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

Hi Kaaira,

On 25/05/2020 18:35, 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.
> 
> Describe the modifiers for MIPI vendor as it is mostly used in
> libcamera. Print digits for the rest.


This prints the following for me now:

> [1:21:53.958400669] [50957]  INFO VIMC vimc.cpp:188 Skipping unsupported pixel format BA24 - NONE : Linear Layout
> [1:21:53.958672633] [50957]  INFO VIMC vimc.cpp:188 Skipping unsupported pixel format RG24 - NONE : Linear Layout
> Camera configuration adjusted
> [1:21:53.958935662] [50957]  INFO Camera camera.cpp:770 configuring streams: (0) 1920x1080-BG24 - NONE : Linear Layout

I think we will need to shorten the default case which adds a lot of
text there:
   " - NONE : Linear Layout"

is 23 extra characters on top of the fourcc, which is the default case
for most PixelFormats.

Perhaps we should simply not print anything for those simple cases.



> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
> 
> 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.
> 
>  src/libcamera/pixelformats.cpp | 65 ++++++++++++++++++++++++++++++++--
>  test/meson.build               |  1 +
>  test/pixel-format.cpp          | 47 ++++++++++++++++++++++++
>  3 files changed, 110 insertions(+), 3 deletions(-)
>  create mode 100644 test/pixel-format.cpp
> 
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index 1330dc5..e16f5ba 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -6,6 +6,11 @@
>   */
>  
>  #include <libcamera/pixelformats.h>
> +#include <map>
> +#include <string.h>
> +
> +#define PIXELFORMAT_VENDOR(vendor)		    \
> +	{ DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }
>  
>  /**
>   * \file pixelformats.h
> @@ -108,9 +113,63 @@ 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 vendors with their Ids: */
> +	std::map<long int, std::string> vendors = {
> +		PIXELFORMAT_VENDOR(NONE),
> +		PIXELFORMAT_VENDOR(MIPI)
> +	};
> +
> +	/* Get the vendor name using its Id */
> +	long int vendorCode = (modifier_ >> 56) & 0xff;
> +	std::string vendor;
> +	std::string vendorSpecification;
> +
> +	switch (vendorCode) {
> +	case DRM_FORMAT_MOD_VENDOR_NONE: {
> +		vendor = vendors[vendorCode];
> +		if (modifier_ == DRM_FORMAT_MOD_LINEAR)
> +			vendorSpecification = "Linear Layout";

So Vendor-None mod-linear turns out to be the simple 'default' case,
which we could consider as 'no modifier', so it becomes a bit redundant
to display that there is no modifier.

It seems we should avoid printing this one altogether.

If you keep the conditional, then the case statement should be kept to
prevent printing the values here.



> +		break;
> +	}
> +	case DRM_FORMAT_MOD_VENDOR_MIPI: {
> +		vendor = vendors[vendorCode];
> +		if (modifier_ == MIPI_FORMAT_MOD_CSI2_PACKED)
> +			vendorSpecification = "CSI-2 packed";
> +		break;

This one is worth displaying though in some form.

Talking with Laurent he has queried if it would be simpler to provide an
optional 'modifier' string to the PixelFormatInfo tables.

Then we could keep the fourcc handling above, but for modifiers - only
print a string if one is provided in the internal pixelformat tables.

However, looking at that, we would then put the same modifier string in
each of the PixelFormatInfo structures that set .packed == true, which
would be the same as handling the case here when modifier_ ==
MOD_CSI2_PACKED...


> +	}
> +	default: {
> +		vendor = std::to_string(vendorCode);
> +		vendorSpecification = std::to_string(modifier_ & 0xff);
> +	}
> +	}
> +
> +	modifier = vendor + " : " + vendorSpecification;

We need these strings to be short, so we shouldn't have spaces around
separators.

> +	std::string formatString(ss);
> +
> +	return formatString + " - " + modifier;


Perhaps as we only really have a single modifier to support, we could
take the modifiers down to the simple case as a single 'if'...

std::string formatString(ss);

if (modifier_ == MIPI_FORMAT_MOD_CSI2_PACKED)
	formatString += ":packed";

return formatString;




>  }
>  
>  } /* namespace libcamera */
> diff --git a/test/meson.build b/test/meson.build
> index bd7da14..591d848 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -33,6 +33,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..edada56
> --- /dev/null
> +++ b/test/pixel-format.cpp
> @@ -0,0 +1,47 @@
> +/* 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_SAMSUNG_64_32_TILE ), "C8   - 4 : 1" },
> +			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN,MIPI_FORMAT_MOD_CSI2_PACKED),"....-BE - MIPI : CSI-2 packed"}
> +		};
> +		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)
>