Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

[Mauro Carvalho Chehab <mchehab@kernel.org>]

Em Fri, 3 Apr 2020 13:47:02 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> > > +static noinline_for_stack
> > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > +		    struct printf_spec spec, const char *fmt)
> > > +{
> > > +#define FOURCC_STRING_BE	"-BE"
> > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > > +
> > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > +		return buf;
> > > +
> > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +		return error_string(buf, end, "(%p4?)", spec);
> > > +
> > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > > +
> > > +	if (*fourcc & BIT(31))
> > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > +			sizeof(FOURCC_STRING_BE));
> > > +
> > > +	return string(buf, end, s, spec);  
> > 
> > Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
> > (without quotes). There are other 4CCs that contain spaces and would
> > suffer from a similar issue. Even in little-endian format, it would
> > result in additional spaces in the output string. Is this what we want ?
> > Should the caller always enclose the 4CC in quotes or brackets for
> > clarity ? Or should still be done here ?  
> 
> Good question. Space is indeed a valid character in a 4cc code.
> 
> If I omit one or more spaces, it will no longer be a 4cc, but a 3cc or even
> a 2cc. Jokes aside, there are probably fair arguments both ways.
> 
> I presume there's no 4cc code where the location of a space would make a
> difference but all of the spaces are trailing spaces.

Yes. I guess it doesn't make any sense to allow a 4cc code with an
space before or in the middle.

Btw, on a quick search at the Internet for non-Linux definitions,
a Fourcc code "Y8  " is actually shown at the lists as just "Y8", 
e. g. removing the leading spaces:

	https://www.fourcc.org/codecs.php
	http://abcavi.kibi.ru/fourcc.php
	https://softron.zendesk.com/hc/en-us/articles/207695697-List-of-FourCC-codes-for-video-codecs
	https://www.free-codecs.com/guides/guides.php?f=fourcc

One interesting detail there is that some tables show some codes 
like "BGR(15)". While I'm not sure how this is encoded, I suspect
that the fourcc is actually "BGR\x15".

We don't do that on V4L, nor we have plans to do so. Not sure if
DRM would accept something like that. Of so, then the logic should
have some special handler if the code is below 32.	

> It's also worth noting that the formats printed are mostly for debugging
> purpose and thus even getting a hypothetical case wrong is not a grave
> issue. This would also support just printing them as-is though.
> 
> I'm leaning slightly towards omitting any spaces if the code has them. 

I would just remove trailing spaces, and then use a loop from the end
to remove trailing spaces (and optionally handle codes ending with a
value below 32, if are there any such case with DRM fourcc codes).

On the other hand, I don't mind if you prefer to use just one for()
loop and just trip any spaces inside it.

> This is something that couldn't be done by using a macro...

Well, I suspect that it might be possible to write a macro
for doing that too, for example using preprocessor concatenation
logic that could produce the same results. If you do something 
like that, however, I suspect that te macro would face some 
portability issues, as, as far as I know, not all C compilers
would handle string concatenation the same way.

Thanks,
Mauro