GNOME Bugzilla – Bug 621282
Display of short fourccs is in hex instead of text
Last modified: 2010-06-12 06:09:00 UTC
Created attachment 163373 [details] [review] A patch fixing the bug Short fourccs like "Y8 " or "Y16 " are not displayed correctly when debugging caps. This is because the function decides if it displays the text or the hex value of the string by checking is all characters are alnum. However, short fourccs also contain spaces. The simple attached patch fixes it.
Could you add a unit test for this? Also please test if this causes confusion for the parser if the fourcc contains characters used for other things, e.g. paranthesis, brackets, etc. Best would be to check if gst_caps_{from,to}_string() works as expected with all kinds of fourcc and other fields combinations (and fourccs in lists, ...)
I basically just changed the function gst_value_serialize_fourcc() so that it has the same behavior as gst_value_transform_fourcc_string() just a couple of lines higher. I tested a working pipeline using a capfilter with caps "video/x-raw-yuv, format=(fourcc) ..." in the following case: - ... = Y800 (normal fourcc): works fine - ... = Y80x (wrong fourcc with alnum character): pipeline not able to link, excepted behavior - ... = Y80# or Y80) (wrong fourcc with non-alnum character): pipeline not even able to set the property on the element. I don't know exactly why. The behavior that I was going to write would have been to change "is_alnum" for something like "is_alnum(c) || c == ' '" (would that be ok?), but when I saw that gst_value_transform_fourcc_string(), I used the same implementation. I could write a more thorough test. I'm not familiar with GStreamer test architecture yet, I can look into that.
Actually, any character for which GST_ASCII_IS_STRING(c) returns true is fine, and gets included in the fourcc. All other characters, such as parenthesis or brackets, parsing stops. So the patch preserves behavior for characters such as ) or }.
Ok, well, the test would be in gstreamer/tests/check/gst/gstvalue.c , take a look at the other tests there for examples :) A test would definitely be good to have. Are you going to use GST_ASCII_IS_STRING() in a later patch?
Created attachment 163445 [details] [review] A patch fixing the bug This patch fixes the bug by considering only alnum AND spaces, which should be the appropriate behavior. Characters like - and +, allowed by G_ASCII_IS_STRING, should not be in fourcc formats. Therefore they should be displayed in hexadecimal.
commit 4a3dde96e70bd1748a4273c5e7ab85f64b082c17 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Sat Jun 12 08:04:47 2010 +0200 value: Add test for deserializing fourccs commit ab0763f0e8461c569dfbff8a0f049454070d14b5 Author: Martin Bisson <martin.bisson@gmail.com> Date: Fri Jun 11 22:56:13 2010 +0000 value: Fixed serialization for short fourccs. "Y16 " and "Y8 " were not displayed properly because the space character is not alnum. A unit test is also included. Fixes bug #621282.