GNOME Bugzilla – Bug 756897
qtdemux: cenc aux_info_type endianness issue
Last modified: 2016-04-14 17:42:45 UTC
Fragments available through this dash manifest expose a 'cnec' aux_info_type according to qtdemux: qtdemux.c:3323:qtdemux_parse_saio:<qtdemux1> aux_info_type: 'cnec', aux_info_type_parameter: 000000 Perhaps we should use the ntohl() function to convert the value to host endianness or is this fragment simply invalid?
Instead of nothl(), rather use GST_READ_UINT32_BE() or similar. For the actual bug, I don't know :) Someone will have to check the spec.
Well, maybe we should rather use the QT_FOURCC macro? Since this is interpreted as a FOURCC field in the end.
Could you make available a hexdump of the relevant part of the fragment? (i.e. the saio box). Is this log from a little endian system or a big endian system?
Nevermind, the URL of the initdata is in the URL field above.
(In reply to Tim-Philipp Müller from comment #3) > Could you make available a hexdump of the relevant part of the fragment? > (i.e. the saio box). > > Is this log from a little endian system or a big endian system? Little endian :)
At first glance this looks like the fourcc was written wrongly, but I didn't really look at it properly.
Adding Brendan in CC, as this manifest is hosted by CableLabs, maybe he can shed some light on this issue.
Created attachment 321674 [details] Screenshot from a hex editor showing seemingly fine siao box Which segment has this problem? I downloaded a few segments and the 'saio' box looked fine in a hex editor (screenshot attached). I wasn't able to find any example of 'cnec' in the files.
CC'ing Greg Rutz, since he created this file and I don't see to have a copy of the MPEG-CE spec.
The current implementation of GST_FOURCC_ARGS() macro expects the fourcc data to be a guint32 in LE format. Looks to me like there is one of two problems: 1) GST_FOURCC_ARGS macro that is used in the offending debug statement is wrong and should be expecting a BE guint32. I am doubtful that this is the case, since I'm sure this macro is used in other places. 2) GST_FOURCC_ARGS is correct and the guint32 aux_info_type should be read as LE using gst_byte_reader_get_uint32_le()
Also note that other areas of the code use QT_FOURCC to read fourcc byte data which is implemented as: #define QT_FOURCC(a) (GST_READ_UINT32_LE(a))
Does 2) fix it? Can you provide a patch? :)
And if it does, why doesn't it fail generally on all files?
It doesn't look like the aux_info_type parameter is really used (just one comparison to 0U). So, it is likely that the problem has always produced the log statement reported in this bug, it just doesn't affect the pipeline's ability to play the content.
Phil, can you compare that with what you see in the stream?
(In reply to Greg Rutz from comment #14) > It doesn't look like the aux_info_type parameter is really used (just one > comparison to 0U). So, it is likely that the problem has always produced > the log statement reported in this bug, it just doesn't affect the > pipeline's ability to play the content. IIRC this isn't right. When I reported this bug, playback was actually prevented by this issue, because the parse_saio() function sets the info_type wrongly and that triggers an error in the parse_moof() function because info_type is not FOURCC_cenc. (In reply to Greg Rutz from comment #11) > Also note that other areas of the code use QT_FOURCC to read fourcc byte > data which is implemented as: > > #define QT_FOURCC(a) (GST_READ_UINT32_LE(a)) This is what I was suggesting to use in comment 2. And I've been using it without issues so far :)
Can you provide a patch for that then?
Created attachment 321811 [details] [review] proposed patch
Review of attachment 321811 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +3379,2 @@ if (!gst_byte_reader_get_uint32_be (br, &aux_info_type_parameter)) return FALSE; So the type is a fourcc in LE, the other parts are still in big endian? Can someone check if this actually makes sense and is what the spec says? I'll push it then.
All multi-byte fields, including FOURCCs, in ISOBMFF files (MOV, MP4, QT, etc) are big-endian. FWIW, the comment in the definition for GST_MAKE_FOURCC says: "Transform four characters into a #guint32 fourcc value with host endianness" If this were true, I would expect that this function would be different on platforms with different endianness. But I don't think that is the case. Seems like the decision was made at some point to always store FOURCC values in GStreamer as little-endian.
So is the patch correct then and the documentation also should be updated? Can someone provide part of a file with this box for reproducing and having it here for historical reasons?
(In reply to Sebastian Dröge (slomo) from comment #21) > So is the patch correct then and the documentation also should be updated? That is my opinion, but I would sure like someone else to jump in to say that they agree with me. I only had time to do a very brief examination of the code. > Can someone provide part of a file with this box for reproducing and having > it here for historical reasons? I can try. The smallest file I have right now is 1.4M. I can't remember if Bugzilla will let me attach a file that size.
You could put it on dropbox or similar :)
Created attachment 321982 [details] Test file that exhibits the problem
commit 459ef195bbc77cee5e407facc3172b77cb29d9a3 Author: Philippe Normand <philn@igalia.com> Date: Wed Oct 21 16:21:45 2015 +0200 qtdemux: read saio aux_info_type as a FOURCC https://bugzilla.gnome.org/show_bug.cgi?id=756897
(I'm sure I looked at the hexdump when looking at it originally, sorry for any noise! :))