[libcamera-devel] [RFC PATCH] libcamera: stream_option: use format name to set cam/qcam format

[Laurent Pinchart]

On Wed, Jun 24, 2020 at 09:23:18PM +0200, Niklas Söderlund wrote:
> Hi Kaaira,
> 
> Thanks for your work.
> 
> On 2020-06-23 19:10:16 +0530, Kaaira Gupta wrote:
> > Replace hex input for pixelformats with their format names, for input in
> > cam and qcam.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> > ---
> >  src/cam/stream_options.cpp | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> > index bd12c8f..b163177 100644
> > --- a/src/cam/stream_options.cpp
> > +++ b/src/cam/stream_options.cpp
> > @@ -7,6 +7,7 @@
> >  #include "stream_options.h"
> >  
> >  #include <iostream>
> > +#include <iterator>
> >  
> >  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 name",
> >  		  ArgumentRequired);
> >  }
> >  
> > @@ -96,8 +97,16 @@ 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")) {
> > +			const StreamFormats &formats = cfg.formats();
> > +			std::vector<PixelFormat> pixelFormats = formats.pixelformats();
> > +			std::vector<PixelFormat>::iterator ptr;
> > +			for (ptr = pixelFormats.begin(); ptr < pixelFormats.end(); ptr++) {
> 
> This will only iterate over the formats exposed by the 
> Camera::generateConfiguration(). In itself that is not so bad as the 
> formats returned form it are the only possible ones.  But it will 
> prevent the user from detecting that the configuration asked for have 
> been Adjusted. I think you should iterate over all formats in the 
> formats:: namespace here.

I think that's why a PixelFormat constructor taking a string has been
proposed, it would internally use a bew PixelFormatInfo::info(const char
*).

> Small nit: You could have simplified the loop to something like this 
> (not testd):
> 
>     for (const PixelFormat &format : formats.pixelformats())
>         if (opts["pixelformat"].toString() == format.toString())
>             cfg.pixelFormat = format;
> 
> > +				if (opts["pixelformat"].toString() == ptr->toString()) {
> > +					cfg.pixelFormat = PixelFormat(*ptr);
> > +				}
> > +			}
> > +		}
> >  	}
> >  
> >  	return 0;

-- 
Regards,

Laurent Pinchart