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

[Laurent Pinchart]

Hi Kaaira,

On Wed, Mar 25, 2020 at 06:43:27PM +0530, Kaaira Gupta wrote:
> On Wed, Mar 25, 2020 at 02:39:33PM +0200, Laurent Pinchart wrote:
> > 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.
> 
> Yes, I'll do that
> 
> > > +	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 ?
> 
> Yes, I'll add these cases too
> 
> > 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 ?
> 
> How should I do that? When I made the changes and wanted to check my
> outputs, I just printed format.fourcc.toString() in 
> test/v4l2_videodevice/v4l2_videodevice_test.cpp...what is the correct
> way to write a test?

That's not very nice, as it requires manually looking at the output.
When need automated tests :-)

You can either add a test case to an existing test, or create a new one.
For this specific case we have test/v4l2_videodevice/formats.cpp that
seems to be a good enough option.

The test should create V4L2PixelFormat instance, and compare the output
of their .toString() method with expected output. You could do something
like

	std::vector<std::pair<uint32_t, const char *>> formats{
		{ V4L2_PIX_FMT_..., "..." },
	};

and use a loop for the tests:

	for (const auto &format : formats) {
		if (V4L2PixelFormat(format.first).toString() != format.second) {
			...
		}
	}

You should include in the vector a few different corner cases (a valid
format, 0, a big-endian format, a numerical value that will result in
non-printable characters, ...). You should also add one check for the
default constructor with V4L2PixelFormat().toString() != "...." outside
of the loop.

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

-- 
Regards,

Laurent Pinchart