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

[Laurent Pinchart]

Hi Kieran,

On Thu, Mar 26, 2020 at 03:49:04PM +0000, Kieran Bingham wrote:
> On 26/03/2020 15:37, Kieran Bingham wrote:
> > On 26/03/2020 09:33, 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 at es.iitr.ac.in>
> > 
> > Thank you, This looks good to me.
> > 
> > Only a couple of quite minor formatting points, which if there are no
> > further issues, and you're happy with; I can handle when applying.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> >> ---
> >>
> >> 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 | 19 ++++++++++++++++---
> >>  test/v4l2_videodevice/formats.cpp  | 20 ++++++++++++++++++++
> >>  2 files changed, 36 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >> index b778181..bb049ec 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";

This will result in messages such as "Pixel format set to Invalid". I
wonder if "INVALID", or maybe something even more distinct from
surrounding text such as "[INV]" or "<INV>" would make sense.

> >> +
> >> +	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");
> > 
> > Probably a blank line here to make sure the return statement is distinct.
> > 
> >> +	return ss;
> >>  }
> >>  
> >>  /**
> >> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp
> >> index d504d17..4e02afc 100644
> >> --- a/test/v4l2_videodevice/formats.cpp
> >> +++ b/test/v4l2_videodevice/formats.cpp
> >> @@ -47,6 +47,26 @@ protected:
> >>  			return TestFail;
> >>  		}
> >>  
> >> +		std::vector<std::pair<uint32_t, const char*>> formats{
> > 
> > Missing space between formats and {
> 
> Ahh - checkstyle correctly points out I'm wrong here, as this is hugged
> due to it being an initialiser, rather than a scope block.
> 
> I guess the alternative is:
> 
>  formats = {
> 
> But it doesn't make much differnce I don't think.
> 
> Let's keep it as you posted without the space.

Just wanted to note that we'll likely need to go through the code to
make consistent usage of different initialization types at some point.
This is certainly fine for now.

> >> +			{ V4L2_PIX_FMT_YUYV, "YUYV" }, { 0, "Invalid" },
> >> +			{ v4l2_fourcc( 0, 1, 2, 3 ), "...." },
> > 
> > Because this is a table, I would probably put each entry on it's own
> > line, rather than condensing to fit 80chars or such.
> > 
> > 
> > i.e.
> > 			{ V4L2_PIX_FMT_YUYV, "YUYV" },
> > 			{ 0, "Invalid" },
> > 			{ v4l2_fourcc( 0, 1, 2, 3 ), "...." },
> > ...

I was about to propose that too :-)

> >> +			{ V4L2_PIX_FMT_Y16_BE, "Y16 -BE" }
> >> +		};
> >> +
> >> +		for (const auto &format : formats) {
> >> +			if (V4L2PixelFormat(format.first).toString() != format.second) {
> >> +				cerr << "Failed to convert V4L2PixelFormat to its fourcc"
> >> +				     << " string" << endl;

How about
				cerr << "Failed to convert V4L2PixelFormat "
				     << utils::hex(format.first) << "to string"
				     << endl;

to tell which one failed ?

> >> +				return TestFail;
> >> +			}
> >> +		}
> >> +
> >> +		if (V4L2PixelFormat().toString() != "Invalid") {
> >> +			cerr << "Failed to convert default V4L2PixelFormat to 'Invalid'"
> >> +			     << " string" << endl;

This could then be

			cerr << "Failed to convert default V4L2PixelFormat to string"
			     << endl;

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

-- 
Regards,

Laurent Pinchart