[libcamera-devel] [PATCH] libcamera: v4l2PixelFormat: replace hex with 4CC

[Kieran Bingham]

Hi Laurent,

On 25/03/2020 12:39, Laurent Pinchart wrote:
> Hi Kaaira,
> 
> Thank you for the patch.
> 
> On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote:
>> Print 4CC characters instead of the hex value in toString() as they are
>> more informative.
>>
>> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
>> ---
>>  src/libcamera/v4l2_videodevice.cpp | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index b778181..ea8ee50 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
>>   */
>>  std::string V4L2PixelFormat::toString() const
>>  {
>> -	char str[11];
>> -	snprintf(str, 11, "0x%08x", fourcc_);
>> -	return str;
>> +	std::stringstream ss;
> 
> Given that the string will be at mose 7 characters long, and the first
> four characters are individually computed, a stringstream seems
> overkill. You can use a plain char[] buffer and write to it directly.
> 
>> +	ss << static_cast<char>(fourcc_ & 0x7f)
> 
> Why 7 bits only ?


This code follows the patch submitted by Sakari/Hans which was destined
to go into the V4L2 UAPI header - but seems stalled for no apparent
reason. It has only positive feedback but didn't progress:

https://lore.kernel.org/linux-media/20190916100433.24367-2-hverkuil-cisco@xs4all.nl/



> 
>> +	   << static_cast<char>((fourcc_ >> 8) & 0x7f)
>> +	   << static_cast<char>((fourcc_ >> 16) & 0x7f)
>> +	   << static_cast<char>((fourcc_ >> 24) & 0x7f);
> 
> What happens if any of these values is non-printable ? What if it's a
> NUL character ? Or a newline ? We shouldn't have such cases normally in
> V4L2 4CCs, but should we protect against that ?

I don't think so no. V4L2PixelFormat is *only* an internal thing. It
should never be exposed to an application, or to user input.

If A V4L2 device has a NULL, newline or unprintable character, I suspect
it's a bug in the kernel.


> There's also the case of a default-constructed V4L2PixelFormat, fourcc_
> will be 0 in that case.

fourcc_ == 0 should indeed be guarded though, and return something like
<NONE> or <EMPTY>


> Could you also please add a test case for this function ?
>>> +
>> +	if (fourcc_ & (1 << 31))
>> +		ss << "-BE";
>> +	return ss.str();
>>  }
>>  
>>  /**
> 

-- 
Regards
--
Kieran