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

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 25, 2020 at 01:12:09PM +0000, Kieran Bingham wrote:
> On 25/03/2020 12:39, 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.
> > 
> >> +	ss << static_cast<char>(fourcc_ & 0x7f)
> > 
> > Why 7 bits only ?
> 
> This code follows the patch submitted by Sakari/Hans which was destined
> to go into the V4L2 UAPI header - but seems stalled for no apparent
> reason. It has only positive feedback but didn't progress:
> 
> https://lore.kernel.org/linux-media/20190916100433.24367-2-hverkuil-cisco@xs4all.nl/
> 
> >> +	   << 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 ?
> 
> I don't think so no. V4L2PixelFormat is *only* an internal thing. It
> should never be exposed to an application, or to user input.
> 
> If A V4L2 device has a NULL, newline or unprintable character, I suspect
> it's a bug in the kernel.

Right, but what should we then do ? :-) Maybe replacing the
non-printable character (as reported by isprint() maybe ?) with a dot ?

> > There's also the case of a default-constructed V4L2PixelFormat, fourcc_
> > will be 0 in that case.
> 
> fourcc_ == 0 should indeed be guarded though, and return something like
> <NONE> or <EMPTY>

It's documented as invalid ;-)

> > Could you also please add a test case for this function ?
> >>> +
> >> +	if (fourcc_ & (1 << 31))
> >> +		ss << "-BE";
> >> +	return ss.str();
> >>  }
> >>  
> >>  /**

-- 
Regards,

Laurent Pinchart