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

[Kaaira Gupta]

On Tue, Jun 02, 2020 at 11:33:09PM +0300, Laurent Pinchart wrote:
> 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.

it is used for "toupper" functionality, I added it so that we can write,
for example, ba24 instead of BA24..but as pointed out DRM formats can
have a mixture of both upper and lower cases in future, so I'll remove
that.

> 
> > >  #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  '

Noted

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

What are the use cases for direct hex values? I think FourCC are easier
to remember?

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