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

[Kieran Bingham]

Hi Kaaira,

On 22/06/2020 15:42, 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>
> ---
> 
> Changes since v1:
> 	-use format names, according to formats namespace, instead of
> 	FourCC
> 
>  src/cam/stream_options.cpp | 52 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> index bd12c8f..9fc428a 100644
> --- a/src/cam/stream_options.cpp
> +++ b/src/cam/stream_options.cpp
> @@ -6,6 +6,8 @@
>   */
>  #include "stream_options.h"
>  
> +#include <libcamera/formats.h>
> +
>  #include <iostream>
>  
>  using namespace libcamera;
> @@ -19,7 +21,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 +98,52 @@ 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::map<std::string, PixelFormat> pixelFormatNames{
> +				{ "BGR888", formats::BGR888 },
> +				{ "RGB888", formats::RGB888 },
> +				{ "ABGR8888", formats::ABGR8888 },
> +				{ "ARGB8888", formats::ARGB8888 },
> +				{ "BGRA8888", formats::BGRA8888 },
> +				{ "RGBA8888", formats::RGBA8888 },
> +				{ "YUYV", formats::YUYV },
> +				{ "YVYU", formats::YVYU },
> +				{ "UYVY", formats::UYVY },
> +				{ "VYUY", formats::VYUY },
> +				{ "NV16", formats::NV16 },
> +				{ "NV61", formats::NV61 },
> +				{ "NV12", formats::NV12 },
> +				{ "NV21", formats::NV21 },
> +				{ "YUV420", formats::YUV420 },
> +				{ "YUV422", formats::YUV422 },
> +				{ "R8", formats::R8 },
> +				{ "SBGGR8", formats::SBGGR8 },
> +				{ "SGBRG8", formats::SGBRG8 },
> +				{ "SGRBG8", formats::SGRBG8 },
> +				{ "SRGGB8", formats::SRGGB8 },
> +				{ "SBGGR10", formats::SBGGR10 },
> +				{ "SGBRG10", formats::SGBRG10 },
> +				{ "SGRBG10", formats::SGRBG10 },
> +				{ "SRGGB10", formats::SRGGB10 },
> +				{ "SBGGR10_CSI2P", formats::SBGGR10_CSI2P },
> +				{ "SGBRG10_CSI2P", formats::SGBRG10_CSI2P },
> +				{ "SGRBG10_CSI2P", formats::SGRBG10_CSI2P },
> +				{ "SRGGB10_CSI2P", formats::SRGGB10_CSI2P },
> +				{ "SBGGR12", formats::SBGGR12 },
> +				{ "SGBRG12", formats::SGBRG12 },
> +				{ "SGRBG12", formats::SGRBG12 },
> +				{ "SRGGB12", formats::SRGGB12 },
> +				{ "SBGGR12_CSI2P", formats::SBGGR12_CSI2P },
> +				{ "SGBRG12_CSI2P", formats::SGBRG12_CSI2P },
> +				{ "SGRBG12_CSI2P", formats::SGRBG12_CSI2P },
> +				{ "SRGGB12_CSI2P", formats::SRGGB12_CSI2P },
> +				{ "MJPEG", formats::MJPEG },
> +			};
> +
> +			std::map<std::string, PixelFormat>::iterator it;
> +			it = pixelFormatNames.find(opts["pixelformat"]);
> +			cfg.pixelFormat = it->second;

This doesn't deal with what would happen if the format can not be found.

There needs to be an error check to make sure that if the given string
is not in the map, then an error is returned, and at least the iterator
should not be dereferenced.


At the moment, libcamera does not expose any of the internal format
information, so there's no way to handle the comparison without this
map, so this indeed looks like the only way it can be handled currently[0].

I think just for readability, and clarity, I would move the map outside
of the scope of this function and somewhere to the top of the file as a
generic 'data table'.

It would be nice if applications had more information on each of the
pixel-formats, to prevent having to duplicate all the information
everywhere, but I fear that the consensus would likely be that
information doesn't live in libcamera.


[0]: But, we 'could' choose to expose a PixelFormat(std::string)
constructor, which could construct a PixelFormat by searching the
internal PixelFormatInfo tables... or return an <INVALID> if not found...

Any thoughts on a string constructor for PixelFormat anyone?



> +		}
>  	}
>  
>  	return 0;
> 

-- 
Regards
--
Kieran