[libcamera-devel,v2] libcamera: v4l2PixelFormat: Replace hex with fourCC

3322 diff mbox series

Print fourCC characters instead of the hex value in toString() as they are
more informative. Also, write the tests for this in formats.cpp

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---

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 | 19 ++++++++++++++++---
 test/v4l2_videodevice/formats.cpp  | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

Hi Kaaira,

Thank you for the patch.

On Thu, Mar 26, 2020 at 03:32:52AM +0530, 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
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
> 
> 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 | 19 ++++++++++++++++---
>  test/v4l2_videodevice/formats.cpp  | 15 +++++++++++++++
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index b778181..f5c14aa 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -336,9 +336,22 @@ 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[7] = {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 < strlen(ss); i++){

This will stop at the first 0 byte. If you replace the badfourcc below
with (0, 1, 2, 3) you will see the test failing.

We consider any issue in the code that isn't caught by a test to imply
that there's a missing test. In this case I think using (0, 1, 2, 3)
instead of (1, 2, 3, 4) in the test case is a good test coverage
extension. As for the fix in the code, i < 4 seems to be the simplest
solution.

> +		if (!isprint(ss[i]))
> +			ss[i]= '.';
> +	}
> +
> +	if (fourcc_ & (1 << 31))
> +		strcat(ss, "-BE");

This overflows the buffer, as you have space for 7 characters only, and
need an eight character for the termating '\0'.

> +	return ss;
>  }

There are lots of formatting issues here and below that are caught by
checkstyle.py. Could you install it as a post-commit git hook ?
Instructions are available in utils/hooks/post-commit. You don't have to
take all the flagged issues into account (in particular we haven't found
a way yet to tell the underlying code formatter, clang-format, about the
80 columns soft limit and the 120 columns hard limit, so it will often
unwrap lines up to the 120 columns limit).

>  
>  /**
> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp
> index d504d17..f129c5c 100644
> --- a/test/v4l2_videodevice/formats.cpp
> +++ b/test/v4l2_videodevice/formats.cpp
> @@ -47,6 +47,21 @@ protected:
>  			return TestFail;
>  		}
>  
> +		auto badfourcc = v4l2_fourcc( 1, 2, 3, 4 );

We usually discourage the usage of auto when the type is simple, to
avoid moving towards an untyped language. auto can introduce subtle bugs
when the developers and reviewers don't have all the C++ type deduction
rules in memory. In this case uin32_t is an appropriate type. Or,
possibly even better, you could use the v4l2_fourcc macro directly below
instead of badfourcc.

> +		std::vector<std::pair<uint32_t, const char*>> formats{
> +			{V4L2_PIX_FMT_YUYV,"YUYV"},{0, "Invalid"}, {badfourcc, "...."},
> +			{V4L2_PIX_FMT_Y16_BE,"Y16 -BE"}};
> +
> +		for (const auto &format :formats){
> +			if (V4L2PixelFormat(format.first).toString()!=format.second){

Please print a message to cerr to tell what failed, otherwise test
failures are difficult to investigate.

> +				return TestFail;
> +			}
> +		}
> +
> +		if (V4L2PixelFormat().toString() != "Invalid"){

Same here.

> +			return TestFail;
> +		}
> +
>  		return TestPass;
>  	}
>  };