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

[Niklas Söderlund]

Hi Kaaira and Kieran,

On 2020-06-22 21:01:11 +0100, Kieran Bingham wrote:
> 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.

I think we crossed that threshold when we introduced the format:: 
namespace. As we already give a "libcamera name" to a DRM fourcc + a 
modifier I think there is little harm in exposing this name in string 
form to applications.

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

-- 
Regards,
Niklas Söderlund