[libcamera-devel] [PATCH v4] libcamera: v4l2PixelFormat: Replace hex with fourCC

[Kieran Bingham]

Hi Kaaira,

On 26/03/2020 20:14, Kaaira Gupta wrote:
> Print fourCC characters instead of the hex value in toString() as they are
> more informative. Also, write the tests for this in formats.cpp

Whenever someone gives you a tag, hang on to it - And keep it in further
versions. Tags are really valuable (to you, the submitter) as evidence
of existing reviews etc. Don't lose them :-)

If code substantially changes beyond review comments, then it might be a
time to drop given tags, but in this instance my tag from v3 certainly
still applies.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


Thank you for your work. We'll merge this today :-)

--
Kieran


> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
> 
> Changes since v3:
> 	- Reformatted the code
> 	- Changed 'invalid' to "<INVALID>"
> 	- Changed cerr message.
> 
> Changes since v2:
>         - reformatted the code
>         - added cerr messages before TestFail
>         - Changed test case to accomodate edge case
>         - Increased buffer size of char array
>         - Changed the maximum length of for loop.
> 
> Changes since v1:
>         - Add tests for checking this function.
>         - use char[] instead of stringstream.
>         - add checks for default value.
>         - Print '.' for non-printable characters.
> 
>  src/libcamera/v4l2_videodevice.cpp | 20 +++++++++++++++++---
>  test/v4l2_videodevice/formats.cpp  | 22 ++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index b778181..eb33a68 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -336,9 +336,23 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
>   */
>  std::string V4L2PixelFormat::toString() const
>  {
> -	char str[11];
> -	snprintf(str, 11, "0x%08x", fourcc_);
> -	return str;
> +	if (fourcc_ == 0)
> +		return "<INVALID>";
> +
> +	char ss[8] = { static_cast<char>(fourcc_ & 0x7f),
> +		       static_cast<char>((fourcc_ >> 8) & 0x7f),
> +		       static_cast<char>((fourcc_ >> 16) & 0x7f),
> +		       static_cast<char>((fourcc_ >> 24) & 0x7f) };
> +
> +	for (unsigned int i = 0; i < 4; i++) {
> +		if (!isprint(ss[i]))
> +			ss[i] = '.';
> +	}
> +
> +	if (fourcc_ & (1 << 31))
> +		strcat(ss, "-BE");
> +
> +	return ss;
>  }
>  
>  /**
> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp
> index d504d17..d01eccd 100644
> --- a/test/v4l2_videodevice/formats.cpp
> +++ b/test/v4l2_videodevice/formats.cpp
> @@ -47,6 +47,28 @@ protected:
>  			return TestFail;
>  		}
>  
> +		std::vector<std::pair<uint32_t, const char*>> formats{
> +			{ V4L2_PIX_FMT_YUYV, "YUYV" },
> +			{ 0, "<INVALID>" },
> +			{ v4l2_fourcc(0, 1, 2, 3), "...." },
> +			{ V4L2_PIX_FMT_Y16_BE, "Y16 -BE" }
> +		};
> +
> +		for (const auto &format : formats) {
> +			if (V4L2PixelFormat(format.first).toString() != format.second) {
> +				cerr << "Failed to convert V4L2PixelFormat"
> +				     << utils::hex(format.first) << "to string"
> +				     << endl;
> +				return TestFail;
> +			}
> +		}
> +
> +		if (V4L2PixelFormat().toString() != "<INVALID>") {
> +			cerr << "Failed to convert default V4L2PixelFormat to string"
> +			     << endl;
> +			return TestFail;
> +		}
> +
>  		return TestPass;
>  	}
>  };
> 

-- 
Regards
--
Kieran