GNOME Bugzilla – Bug 735070
asfdemux: ASF file with H.264 video not playing back properly
Last modified: 2014-08-27 14:05:07 UTC
Some ASF files are not properly played back by GStreamer 1.4.0. For example, the following: https://www.dropbox.com/s/uc1ba26kfqolj7z/trailer_1080p_640x480-libx264-2m_aac-128k-4410.asf It plays back fine in VLC and mplayer Debug log from GStreamer: 0:00:00.067988812 5562 0x7f61a40ea770 LOG asfdemux gstasfdemux.c:1710:gst_asf_demux_push_complete_payloads:asfdemux0:video_0 pushing buffer, ts=0:00:03.560000000, dur=99:99:99.999999999 size=107594 0:00:00.068000126 5562 0x7f61a40ea770 DEBUG asfdemux gstasfdemux.c:1901:gst_asf_demux_loop: pushing complete payloads failed 0:00:00.068003449 5562 0x7f61a40ea770 DEBUG asfdemux gstasfdemux.c:1950:gst_asf_demux_loop: pausing task, flow return: not-negotiated 0:00:00.068021544 5562 0x7f61a40ea770 WARN asfdemux gstasfdemux.c:1962:gst_asf_demux_loop: error: Internal data stream error.
It seems that this ASF has h264 in bytestream format and the SPS/PPS are provided only by codec_data. This is being ignored currently in h264parse, let me try to patch it. Did this work in some previous 1.x version?
Are SPS/PPS in byte-stream format here or avcc format? Let's not put avcc style codec_data on byte-stream h264 caps please.
It is in byte-stream format, I have a patch that keeps it in codec_data in bytestream format. It does 2 things: 1) Parse the codec_data NAL looking only for PPS and SPS 2) Relay the codec_data just like it is to downstream
Created attachment 284086 [details] [review] h264parse: handle codec_data from bytestream streams The codec_data might contain the SPS and PPS that are needed to decode the stream that doesn't have those entries repeated in it.
I'm really not keen on having byte-stream style SPS/PPS in a codec_data field. I think it's wrong, and we should fix it at the producer, even if it's awkward to have codec-specific code there. I think we do something similar already somewhere else (avidemux? matroskademux?). Two options IMHO: - asfdemux just puts the byte-stream style codec data as header buffers into the stream before pushing the first packets (after checking for sync markers to be sure) - asfdemux puts the byte-stream codec data into the caps as "streamheader" field (but I think it should additionally put them into the stream as buffers as well).
I think the streamheader makes sense to allow decoding the stream from any point in it (seeks or late linked/receivers). Trying to understand your opinion better. How is it different from codec-data? Just semantics as it is in byte-stream format?
codec_data is used for out-of-band codec data. For h264 it is pretty much defined to be in avcc format, and it is also only defined for stream-format=avc. I'm sure there's code out there that will decode whether something is avc based on the presence of the codec_data field too (for historical reasons mostly). I think making codec_data for byte-stream a thing would be confusing and probably break stuff. I also think making codec_data with a different representation of the content a thing is bad. The difference between streamheader and codec_data fields is that codec_data is out-of-band and streamheader can be prepended to the actual stream data (when written to file or sent across the network, for example), so I think in this case streamheader is the best fit, and the correct thing to do.
Created attachment 284428 [details] [review] asfdemux: if video is h264, check the codec_data for bytestream data For bytestream we don't want to expose it as codec_data but rather as streamheader as it is not out-of-band data but data that should be appended to the beginning of the stream before the other buffers.
Created attachment 284429 [details] [review] asfdemux: push streamheaders as first buffers Store the streamheaders in streams and push them before the first buffer of the stream to allow downstream to process them before handling the rest of the stream
Created attachment 284430 [details] [review] baseparse: handle streamheaders by prepending them to the stream Add a first_buffer boolean state flag to have baseparse do actions before pushing data. This is used to check the caps for streamheader buffers that are prepended to the stream, but only if the first buffer isn't already marked with the _HEADER flag. In this case, it is assumed that the _HEADER marked buffer is the same as the streamheader.
Looks fine to me. Not sure the non-OK flow checking and bailing out is really needed in asfdemux, even if it's more correct of course. The first patch should have s/append/prepend/ in the commit message. One could probably unify the streamheader member and the new first_buffer member, but maybe keeping them separate is cleaner.
Thanks for the review. Updated the patches and pushed. commit 59c34a8ff75d8cbfb24759711a2c35599f9ae52d Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Mon Aug 25 13:44:30 2014 -0300 baseparse: handle streamheaders by prepending them to the stream Add a first_buffer boolean state flag to have baseparse do actions before pushing data. This is used to check the caps for streamheader buffers that are prepended to the stream, but only if the first buffer isn't already marked with the _HEADER flag. In this case, it is assumed that the _HEADER marked buffer is the same as the streamheader. https://bugzilla.gnome.org/show_bug.cgi?id=735070 commit 2863d9ae0039ddd27b4b98ceaaa1f4ed3a2eb3d6 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Thu Aug 21 12:09:23 2014 -0300 asfdemux: if video is h264, check the codec_data for bytestream data For bytestream we don't want to expose it as codec_data but rather as streamheader as it is not out-of-band data but data that should be prepended to the beginning of the stream before the other buffers. https://bugzilla.gnome.org/show_bug.cgi?id=735070