[libcamera-devel] [PATCH] libcamera: stream_option: use fourcc values to set cam/qcam formats

[Laurent Pinchart]

Hello,

On Tue, Jun 02, 2020 at 05:20:14PM +0200, Niklas Söderlund wrote:
> On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote:
> > Replace hex input for pixelformats with their fourcc values,
> > in cam and qcam.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> > ---
> >  src/cam/stream_options.cpp | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> > index bd12c8f..9f9536e 100644
> > --- a/src/cam/stream_options.cpp
> > +++ b/src/cam/stream_options.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >  #include "stream_options.h"
> >  
> > +#include <bits/stdc++.h>

What is this header used for ? It doesn't seem to be standard.

> >  #include <iostream>
> >  
> >  using namespace libcamera;
> > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser()
> >  		  ArgumentRequired);
> >  	addOption("height", OptionInteger, "Height in pixels",
> >  		  ArgumentRequired);
> > -	addOption("pixelformat", OptionInteger, "Pixel format",
> > +	addOption("pixelformat", OptionString, "Pixel format fourcc",

s/fourcc/FourCC/

> >  		  ArgumentRequired);
> >  }
> >  
> > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> >  		}
> >  
> >  		/* \todo Translate 4CC string to pixelformat with modifier. */
> > -		if (opts.isSet("pixelformat"))
> > -			cfg.pixelFormat = PixelFormat(opts["pixelformat"]);
> > +		if (opts.isSet("pixelformat")) {
> > +			std::string fourcc = opts["pixelformat"];
> > +			transform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper);
> 
> I fear we don't know all fourcc codes will be uppercase. It's currently 
> true all DRM defined ones are, but might change. As an example look at 
> V4L2 fourcc codes where lower/upper case is already mixed.
> 
> > +			char char_array[5];
> > +			strcpy(char_array, fourcc.c_str());
> 
> I think you can simplify this, or something similar (not compile 
> tested).
> 
>     const char *fourcc = opts["pixelformat"].toString().c_str();

OptionValue::toString() returns a temporary object, you'll have a
use-after-free.

> 
> > +			cfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) |
> > +						      ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24));

static_cast<uint32_t>() instead of (__u32), to use C++-style casts, and
because __u32 is a kernel type. Let's also split this to shorten the
lines.

This change goes in the right direction, but I think we need to expand
it a little bit:

- Some FourCC contain spaces at the end. It would be nice to write

	-s pixelformat=R8

  instead of

	-s 'pixelformat=R8  '

- Should we support both the numerical value and the 4CC representation
  ? It could be useful in some cases, to avoid having to convert
  manually from hex to 4CC first, but maybe there are only few use cases
  for that feature.

- With a 4CC we can't easily handle the formats that use
  DRM_FORMAT_BIG_ENDIAN. Maybe that's no big deal though, as we don't
  any use such format at the moment, and DRM_FORMAT_BIG_ENDIAN seems
  more like a hack than something that should be used going forward.

Not all this need to be addressed now.

> > +		}
> >  	}
> >  
> >  	return 0;

-- 
Regards,

Laurent Pinchart