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

[Laurent Pinchart]

Hi Kaaira,

On Thu, Mar 26, 2020 at 03:41:25AM +0530, Kaaira Gupta wrote:
> On Wed, Mar 25, 2020 at 09:02:29PM +0200, Laurent Pinchart wrote:
> > 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?

There's no current test case to ensure that V4L2PixelFormat() creates an
instance with fourcc == 0, that's why I've proposed a separate case.
It's hijacking your work a little bit :-)

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

-- 
Regards,

Laurent Pinchart