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

[Kaaira Gupta]

On Wed, Mar 25, 2020 at 09:02:29PM +0200, Laurent Pinchart wrote:
> 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.

Yes, done and submitted..but I don't understand why we need to check for
'0' separately, when we are already checking for the default
constructor's value?

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