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

[Laurent Pinchart]

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 at 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 at 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)

-- 
Regards,

Laurent Pinchart