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

[Niklas Söderlund]

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.

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
> 

-- 
Regards,
Niklas Söderlund