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

[Kieran Bingham]

Hi Kaaira,

Reviving this thread,

On 22/06/2020 21:48, Niklas Söderlund wrote:
> 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.

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


--
Kieran


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

-- 
Regards
--
Kieran