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

[Kaaira Gupta]

On Wed, Mar 25, 2020 at 02:39:33PM +0200, Laurent Pinchart wrote:
> Hi Kaaira,
> 
> Thank you for the patch.
> 
> 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?

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