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

4157 diff mbox series

Replace hex input for pixelformats with their format names, for input in
cam and qcam.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 src/cam/stream_options.cpp | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

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

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

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