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

3870 diff mbox series

Print fourCC characters instead of the hex values in toString() as they
are easier to comprehend. Also, print the corresponding modifiers for
MIPI vendor as it is mostly used in libcamera.

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

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.

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

Hi Kaaira,

On 28/05/2020 14: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 for
> MIPI vendor as it is mostly used in libcamera.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
> 
> 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.
> 
>  src/libcamera/pixelformats.cpp | 28 +++++++++++++++++---
>  test/meson.build               |  1 +
>  test/pixel-format.cpp          | 47 ++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 3 deletions(-)
>  create mode 100644 test/pixel-format.cpp
> 
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index 1330dc5..16aa7e4 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/pixelformats.h>
> +#include <string.h>
>  
>  /**
>   * \file pixelformats.h
> @@ -108,9 +109,30 @@ 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 fourcc[5] = { 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(fourcc[i]))
> +			fourcc[i] = '.';
> +	}
> +
> +	std::string formatString(fourcc);
> +
> +	if (fourcc_ & (1 << 31))
> +		formatString += "-BE";
> +
> +	if (modifier_ == DRM_FORMAT_MOD_INVALID)
> +		formatString += ":<INVALID> modifier";

I think this is the only part I find slightly distasteful, as I don't
think we need to explicitly mention the " modifier".

With this removed

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

So if you're happy, I can simply remove the extra word when applying.



> +	else 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..84062b7
> --- /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" },
> +			{ PixelFormat(DRM_FORMAT_C8, DRM_FORMAT_MOD_SAMSUNG_64_32_TILE), "C8  " },
> +			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, MIPI_FORMAT_MOD_CSI2_PACKED), "....-BE: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)
>

Hello,

On Tue, Jun 02, 2020 at 11:35:43AM +0100, Kieran Bingham wrote:
> On 28/05/2020 14: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 for
> > MIPI vendor as it is mostly used in libcamera.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> > 
> > 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.
> > 
> >  src/libcamera/pixelformats.cpp | 28 +++++++++++++++++---
> >  test/meson.build               |  1 +
> >  test/pixel-format.cpp          | 47 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 73 insertions(+), 3 deletions(-)
> >  create mode 100644 test/pixel-format.cpp
> > 
> > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> > index 1330dc5..16aa7e4 100644
> > --- a/src/libcamera/pixelformats.cpp
> > +++ b/src/libcamera/pixelformats.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <libcamera/pixelformats.h>
> > +#include <string.h>
> >  
> >  /**
> >   * \file pixelformats.h
> > @@ -108,9 +109,30 @@ 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 fourcc[5] = { 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(fourcc[i]))
> > +			fourcc[i] = '.';
> > +	}
> > +
> > +	std::string formatString(fourcc);
> > +
> > +	if (fourcc_ & (1 << 31))
> > +		formatString += "-BE";
> > +
> > +	if (modifier_ == DRM_FORMAT_MOD_INVALID)
> > +		formatString += ":<INVALID> modifier";
> 
> I think this is the only part I find slightly distasteful, as I don't
> think we need to explicitly mention the " modifier".
> 
> With this removed
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> So if you're happy, I can simply remove the extra word when applying.

I think we need to consider the inverse problem, creating a PixelFormat
from a string, to support passing pixel formats on the command line in
cam and qcam (Kaaira has also posted a patch for that). If we don't
consider the two issues together, there's a high chance this function
will need to be changed later (and quite soon). I wonder if, considering
the libcamera::formats:: namespace proposal, and the PixelFormatInfo
structure, adding the format name to PixelFormatInfo and looking it up
wouldn't be a good alternative approach. I'm not sure what the best
option is though (I would otherwise propose it :-)), but I feel we're
not there yet.

> > +	else 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..84062b7
> > --- /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" },
> > +			{ PixelFormat(DRM_FORMAT_C8, DRM_FORMAT_MOD_SAMSUNG_64_32_TILE), "C8  " },
> > +			{ PixelFormat(DRM_FORMAT_BIG_ENDIAN, MIPI_FORMAT_MOD_CSI2_PACKED), "....-BE: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)