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

[Laurent Pinchart]

Hi Kieran,

On Fri, Jul 24, 2020 at 03:35:33PM +0100, Kieran Bingham wrote:
> On 22/06/2020 21:48, Niklas Söderlund wrote:
> > On 2020-06-22 21:01:11 +0100, Kieran Bingham wrote:
> >> 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.
> 
> My qeustion was more about 'how much' should we expose to applications?
> I.e. the content of include/libcamera/internal/formats.h
> 
> or...
> 
> >> [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?
> 
> Should we add a PixelFormat(std::string) which will search for a named
> format, and return it if available.
> 
> I would prefer this option that having applications encode a table of
> all supported strings/PixelFormats each time, but I also think there's
> value in all of the information stored in PixelFormatInfo, but I don't
> currently think we want to make that part of the public-api ... so a
> string constructor is a the best solution I can currently see...

Or a static function ?

class PixelFormat
{
	...
	static PixelFormat fromString(const std::string &name);
	...
};

> >>> +		}
> >>>  	}
> >>>  
> >>>  	return 0;

-- 
Regards,

Laurent Pinchart