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

[Laurent Pinchart]

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 ?

> +	   << 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 ?

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

Could you also please add a test case for this function ?

> +
> +	if (fourcc_ & (1 << 31))
> +		ss << "-BE";
> +	return ss.str();
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart