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

[Laurent Pinchart]

Hi Kaaira,

On Wed, Jun 03, 2020 at 06:20:23PM +0530, Kaaira Gupta wrote:
> On Tue, Jun 02, 2020 at 11:33:09PM +0300, Laurent Pinchart wrote:
> > 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?

A FourCC is indeed easier to remember, but I've found myself in
situations where one tool (a display test tool for instance) would give
me a hex value, it would be nice to be able to pass it directly to cam
without having to convert it manually.

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