After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 700505 - Video corruption when dashdemux changes bitrate
Video corruption when dashdemux changes bitrate
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 701750 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-05-17 08:38 UTC by A Ashley
Modified: 2015-09-14 10:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Console output and dot graphs for failed dashdemux usages (15.22 KB, application/zip)
2013-05-17 08:38 UTC, A Ashley
  Details
debug log with dashdemux:5 (37.39 KB, text/plain)
2013-05-21 07:59 UTC, A Ashley
  Details
Avoid skipping moov atoms for fragmented MP4 files (1.91 KB, patch)
2013-06-11 14:11 UTC, A Ashley
committed Details | Review
qtdemux patch to demonstrate stream-start event issue (5.38 KB, patch)
2013-06-14 22:50 UTC, Greg Rutz
none Details | Review
Patch to make sending of stream-start event conditional (5.36 KB, patch)
2013-06-18 18:00 UTC, Greg Rutz
reviewed Details | Review
MPD to reproduce this issue in MPEG2 TS (1.59 KB, application/octet-stream)
2013-06-25 20:56 UTC, Greg Rutz
  Details

Description A Ashley 2013-05-17 08:38:04 UTC
Created attachment 244493 [details]
Console output and dot graphs for failed dashdemux usages

I have been trying out dashdemux from its new home in gst-plugins-bad.

I am having a problem when dashdemux switches representations (i.e. moves between bitrates). After a representation change the video output is corrupted and there are lots of error messages from the video decoder.

The command I am using to launch the MPD file is:

gst-launch-1.0 playbin uri='http://www-itec.uni-klu.ac.at/ftp/datasets/mmsys12/RedBullPlayStreets/redbull_4s/RedBullPlayStreets_4s_isoffmain_DIS_23009_1_v_2_1c2_2011_08_30.mpd'


If I edit the MPD file to remove everything but a single bitrate, the file plays correctly. For example if you download the following file and host it locally:

https://www.dropbox.com/s/a5s9fwg2epuh56y/redbull.mpd

the file will play correctly using:
gst-launch-1.0 playbin uri='http://127.0.0.1/redbull.mpd'

Note that in both cases, the video assets are being served from www-itec.uni-klu.ac.at

I have attached a zip file with the output from gstreamer plus the dot files giving the connection graphs.

I am using the master branch from git of gstreamer, gst-plugins-base, gst-plugins-good and gst-plugins-bad on a Fedora 17 install. The git log for gst-plugins-bad shows the last change as:
commit f0a1935119c6572bd320c7605d53815fbc4c8092
Author: Sebastian Dröge
Date:   Wed May 15 11:38:35 2013 +0200
Comment 1 A Ashley 2013-05-21 07:59:41 UTC
Created attachment 244898 [details]
debug log with dashdemux:5

Debug log with --gst-debug-no-color --gst-debug-level=dashdemux:5
Comment 2 Greg Rutz 2013-05-27 14:50:21 UTC
I actually think this has to do with a resolution change rather than a bitrate change.  I have another dataset that contains multiple representations of different bitrates, but all of which have the same video resolution.  I can play this dataset and see representation changes without problems.  It is possible that the libav h264 decoder can not handle resolution changes without a decoder reset.

When a representation changes in dashdemux, I think we need to trigger a decode group switch by adding new pads, sending no-more-pads, then removing the old pads.  This should setup a fresh decode chain for the new representation.  Currently, the code only does this when it switches Periods and it signals new decode chains for *all* active streams.

I am working on a fix where we can signal a decode group switch for a single stream when a representation switch is needed.
Comment 3 Thiago Sousa Santos 2013-06-02 15:59:36 UTC
(In reply to comment #2)
> 
> I am working on a fix where we can signal a decode group switch for a single
> stream when a representation switch is needed.

mssdemux has this same scenario, and it works because qtdemux knows hows to handle a bitrate/resolution switch by reconfiguring itself and pushing a new caps event downstream, informing the decoders/parsers of the change.

In the dashdemux case, qtdemux is simply ignoring the switch and continues pushing data and the decoders/parsers will interpret the new data with the old resolution/bitrate, leading to corrupted video/audio. The difference is that MSS doesn't use 'moov' atoms with the track information, the track information is sent directly in the caps event. Dashdemux, OTOH, sends another moov everytime it switches bitrates, but qtdemux currently ignores all but the first moov it receives.
Comment 4 Greg Rutz 2013-06-02 21:38:44 UTC
(In reply to comment #3)
> In the dashdemux case, qtdemux is simply ignoring the switch and continues
> pushing data and the decoders/parsers will interpret the new data with the old
> resolution/bitrate, leading to corrupted video/audio.

tsdemux suffers from this same problem.  We have to examine the elementary stream packet headers to get the resolution and send an new caps event.

Until these problem are fixed with demuxers, I'm assuming you will be OK with making dashdemux just switch decode groups when it detects a new Representation (which is what my incoming patch will do).  Please let me know if this fix would not be acceptable at this time.
Comment 5 Thiago Sousa Santos 2013-06-03 02:09:51 UTC
I believe the issue is at qtdemux. Dashdemux already detects the bitrate change and pushes a new caps event and an initialization segment (a new moov atom) to the downstream qtdemux, but it doesn't handle this new moov yet, it will only parse the first one it receives. I think the solution is to make qtdemux accept a new moov in a dash bitstream switching scenario.

While decode groups switching would work, they aren't needed as a simple renegotiation (pushing a new caps event downstream) would be enough and cheaper.
Comment 6 A Ashley 2013-06-05 16:09:18 UTC
(In reply to comment #5)
> While decode groups switching would work, they aren't needed as a simple
> renegotiation (pushing a new caps event downstream) would be enough and
> cheaper.

That sounds like a better option, because it saves having to build a new decode chain. The whole point of HTTP adaptive streaming is that it allows seamless adaptation to network conditions. Trying to tear down and create new decoders without causing stuttering in the video display might be rather difficult on "low end" CPU devices like set top boxes or fanless HTPCs.

Looking at qtdemux.c gst_qtdemux_loop_state_header:
  case FOURCC_moov:
    {
      GstBuffer *moov = NULL;
      if (qtdemux->got_moov) {
        GST_DEBUG_OBJECT (qtdemux, "Skipping moov atom as we have one already");
        qtdemux->offset += length;
        goto beach;
      }

and gst_qtdemux_reset, it should be fairly easy to try a quick hack to see if this fixes the resolution change problem.

I will have a play and add a comment if I find out anything.
Comment 7 Greg Rutz 2013-06-09 04:03:28 UTC
In the case of MP4 containers, it looks like we will have a relatively simple way to get the demux to detect a change and emit a new caps event to the decoders downstream (since video resolution is mandatory in the container metadata).  However, in the case of MPEG2 transport streams, the container itself does not have information about video resolution.  The elementary streams probably carry this information, but you would have to have codec-specific code in the demux to parse into the video headers to find the resolution.

I am not an expert in MPEG2 TS, but is there a way to get the video resolution so that a new caps event can be sent by the demux?
Comment 8 A Ashley 2013-06-10 14:41:04 UTC
Yes, the information is in the MPEG2 TS, but as you say it would require parsing the PAT & PTM from the TS to find the video PID and then codec specific parsing of the video PID. 

Can we use the resolution information in the Representation element of the MPD file? For example:

<Representation bandwidth="808000" id="S1" frameRate="25" scanType="progressive"
                       width="352" height="288" sar="16:11"/>
<Representation bandwidth="1166000" id="S2" frameRate="25" scanType="interlaced"
                       width="544" height="576" sar="64:33"/>
<Representation bandwidth="1452000" id="S3" frameRate="25" scanType="interlaced"
                       width="544" height="576" sar="64:33"/>
<Representation bandwidth="1755000" id="S4" frameRate="25" scanType="interlaced"
                       width="704" height="576" sar="16:11"/>
Comment 9 Olivier Crête 2013-06-10 14:59:38 UTC
Won't the codec specific parser that will be between the demuxer and the decoder figure out the resolution change and push new caps?
Comment 10 David Corvoysier 2013-06-11 08:58:31 UTC
*** Bug 701750 has been marked as a duplicate of this bug. ***
Comment 11 A Ashley 2013-06-11 08:59:22 UTC
I am inclined to agree with Thiago that the initialization data for a
resolution change (SPS and PPS for H.264) is not making it as far as the parser
and decoder. The codec specific parser can't generate the change event if it
has not seen new initialization data. Sending compressed data to the decoder
for a different resolution will produce decoding errors if the decoder has not
been reconfigured to the new resolution. 

What I am not sure about is if there are additional requirements beyond sending
SPS+PPS (such as an event being pushed to the decoder) to cause them to
reconfigure for the new resolution.

I have tried patching qtdemux to handle more than one moov atom, but this is
causing the H.264 decoder to segfault after a representation switch.

I am also trying with an AVC3 encoded DASH file, because in avc3 the SPS+PPS
are in each fragment. However that is also failing because something downstream
of qtdemux is failing the caps negotiation (I am getting "peer ALLOCATION query
failed" in qtdemux_do_allocation, followed by a "Error pushing buffer:
not-negotiated" from dashdemux).
Comment 12 Thiago Sousa Santos 2013-06-11 12:04:59 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=702003

Just fixed a renegotiation issue in gst-libav that was causing the corrupted video. If we forcefully make qtdemux accept new moovs, the video should play correctly now. There is still something wrong with the timestamping, though. (Next item on my list)
Comment 13 A Ashley 2013-06-11 14:11:30 UTC
Created attachment 246527 [details] [review]
Avoid skipping moov atoms for fragmented MP4 files

This patch changes qtdemux to accept a new moov in a dash bitstream
switching scenario.
Comment 14 A Ashley 2013-06-11 14:14:36 UTC
(In reply to comment #12)
> https://bugzilla.gnome.org/show_bug.cgi?id=702003
> 
> Just fixed a renegotiation issue in gst-libav that was causing the corrupted
> video. If we forcefully make qtdemux accept new moovs, the video should play
> correctly now. There is still something wrong with the timestamping, though.
> (Next item on my list)

I've just uploaded a quick patch to qtdemux that causes it to parse subsequent moov atoms. For me this allows a DASH ISO BMFF stream to correctly decode following a representation change. However this isn't the final solution because it seems to cause a pause after the first representation change.
Comment 15 Greg Rutz 2013-06-11 15:27:35 UTC
(In reply to comment #8)
> Can we use the resolution information in the Representation element of the MPD
> file?

These attributes are optional and will not be present in every MPD, so we can not rely on them for generating new caps
Comment 16 Greg Rutz 2013-06-11 15:29:37 UTC
(In reply to comment #9)
> Won't the codec specific parser that will be between the demuxer and the
> decoder figure out the resolution change and push new caps?

I have always been a little confused about the role a "parser" plays in the pipeline.  I will try to educate myself a little bit more about parsers in this particular MPEG2 TS - based pipeline and get back to you.
Comment 17 Greg Rutz 2013-06-11 15:29:48 UTC
(In reply to comment #9)
> Won't the codec specific parser that will be between the demuxer and the
> decoder figure out the resolution change and push new caps?

I have always been a little confused about the role a "parser" plays in the pipeline.  I will try to educate myself a little bit more about parsers in this particular MPEG2 TS - based pipeline and get back to you.
Comment 18 Greg Rutz 2013-06-14 22:48:37 UTC
Using A Ashley's patch, I was able to make one additional change to get improved performance.  The problem you are seeing is that the qtdemux sends a stream-start event for all streams after it parses the new MOOV.  I am not a complete expert on clocks and timestamping in GStreamer, so I will probably fumble this explanation a little bit.

The stream-start event causes the pipeline clock to reset.  However, the buffer timestamps continue on from where they left off.  I see the following message in the log from base_sink shortly after it receives the stream-start event:

0:00:14.543808931 12759 0xac7f6320 DEBUG               basesink gstbasesink.c:3261:gst_base_sink_chain_unlocked:<xvimagesink0> got times start: 0:00:07.770433333, end: 0:00:07.803800000
0:00:14.543822958 12759 0xac7f6320 DEBUG               basesink gstbasesink.c:1877:gst_base_sink_get_sync_times:<xvimagesink0> got times start: 0:00:07.770433333, stop: 0:00:07.803800000, do_sync 1
0:00:14.543836497 12759 0xac7f6320 DEBUG               basesink gstbasesink.c:2450:gst_base_sink_do_sync:<xvimagesink0> reset rc_time to time 0:00:15.540866666
0:00:14.543845940 12759 0xac7f6320 DEBUG               basesink gstbasesink.c:2462:gst_base_sink_do_sync:<xvimagesink0> possibly waiting for clock to reach 0:00:15.540866666, adjusted 0:00:15.540866666
0

I am attaching a patch that is a TOTAL HACK.  But I think it at least proves where the problem is.  I'll probably need some guidance from some of you out there about where to go from here.  I'm not sure if the stream-start event is really the problem, or if the buffer timestamps are the ones that need altering.
Comment 19 Greg Rutz 2013-06-14 22:50:24 UTC
Created attachment 246863 [details] [review]
qtdemux patch to demonstrate stream-start event issue
Comment 20 David Corvoysier 2013-06-17 07:33:16 UTC
Having looked into this particular timestamping issue in details, and not sure it is even related, but this reminds me of a patch I submitted a patch about a year ago to make sure that the timeline offset contained in the tfdt box was taken into account when parsing a new moov:

https://bugzilla.gnome.org/show_bug.cgi?id=677535
Comment 21 Thiago Sousa Santos 2013-06-18 03:58:20 UTC
Bisecting core took me here:

commit f0a3d8bb096c301134af45eeb04db9fa3d118d4d
Author: Brendan Long <b.long@cablelabs.com>
Date:   Fri May 31 09:39:55 2013 -0600

    input-selector: send notify::active signal for input-selector pads.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=701319


I still don't understand how it is related.
Comment 22 Greg Rutz 2013-06-18 18:00:00 UTC
Created attachment 247193 [details] [review]
Patch to make sending of stream-start event conditional

This patch replaces my previous one to be more clean to apply.  It should be applied on top of A Ashleys patch to allow multiple MOOV atoms.
Comment 23 Thiago Sousa Santos 2013-06-19 13:41:03 UTC
(In reply to comment #22)
> Created an attachment (id=247193) [details] [review]
> Patch to make sending of stream-start event conditional
> 
> This patch replaces my previous one to be more clean to apply.  It should be
> applied on top of A Ashleys patch to allow multiple MOOV atoms.

I'm sorry, I ended up pushing a different version of this fix. Also got A Ashley's patch pushed.

I would like to close this issue, but I still see a problem about MPEG TS in the comments and I'd like to understand it before doing so. Do we have a stream to reproduce the issue?
Comment 24 Greg Rutz 2013-06-25 20:53:19 UTC
(In reply to comment #23)
> I would like to close this issue, but I still see a problem about MPEG TS in
> the comments and I'd like to understand it before doing so. Do we have a stream
> to reproduce the issue?

The content in bug 702677 will reproduce this.  However, it will fail to play because of the issue described in that bug.  If you download that content, I will attach a modified MPD to this bug that will reproduce the MPEG TS video corruption issue without running in the the problem in 702677.
Comment 25 Greg Rutz 2013-06-25 20:56:19 UTC
Created attachment 247776 [details]
MPD to reproduce this issue in MPEG2 TS

Use this MPD with the content in bug 702677 to reproduce the issue for MPEG2 TS.
Comment 26 Sebastian Dröge (slomo) 2013-10-07 09:14:11 UTC
Review of attachment 247193 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +4615,3 @@
                 QtDemuxStream *stream = demux->streams[n];
 
+                gst_qtdemux_configure_stream (demux, stream, send_stream_start);

So this here is the only place where no new stream-start is sent sometimes. Why exactly, what does it break how?

@@ +5632,3 @@
         GST_ELEMENT_CAST (qtdemux), "%03u", stream->track_id);
+    if (send_stream_start)
+      gst_pad_push_event (stream->pad, gst_event_new_stream_start (stream_id));

While looking at this... dashdemux should set the same group-id on the stream-start events of all streams that belong together. And when a new group starts set a new group-id on all of them.

(A group means here the set of streams that should be played together)
Comment 27 Sebastian Dröge (slomo) 2013-10-07 09:18:28 UTC
A new stream-start event should be sent whenever a new stream starts btw, for dashdemux that shouldn't probably happen at all unless a different mpd is selected. You'll still need to make sure that any added new streams get a stream-start event with a unique stream id.
Comment 28 Shamik 2013-10-09 09:46:01 UTC
I was trying DashDemux in a Raspberry PI board.

I built Gstreamer 1.2 for raspberry and tried using some mpd content. As dash-demux is part of gst-plugins-bad, my expectation was it should work as it is.

But I faced the same problem as mentioned in the Bug. When start the gst-launch:

gst-launch-1.0 playbin uri=http://<url>/sample.mpd (this consists of some mp4 files)

To start with it took the lowest resolution Audio and Video and started showing that. But after sometime when it tried to switch the representation to higher resolution Video it crashed. Audio Switch does happen.

Also I tried with individual content (having only one representation in the MPD) and PI could successfully play all the different video content including 1080p.

But is gets stuck when it is trying to switch the representation (different resolution video)
Comment 29 Thiago Sousa Santos 2014-12-02 01:34:44 UTC
Can you retry with latest git? Lots of fixes landed since 1.2
Comment 30 Philippe Normand 2015-08-16 15:12:03 UTC
I can't reproduce this bug on my RPi with current git master, the modified redbull.mpd provided by Alex plays fine here.
Comment 31 Vincent Penquerc'h 2015-09-14 10:57:20 UTC
Seems fine here too (x86_64). Both videos play the same, and dashdemux logs on the first one show a number of representation switches happening.

Also, a related bug, https://bugzilla.gnome.org/show_bug.cgi?id=753869, was fixed recently.