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 781270 - qtdemux, dash: segmentation fault on representation change
qtdemux, dash: segmentation fault on representation change
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.11.90
Other Linux
: Normal blocker
: 1.11.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-04-13 13:25 UTC by Juergen Sachs
Modified: 2017-04-27 09:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
testcode to verify the root cause of the problem (1.57 KB, patch)
2017-04-13 13:41 UTC, Juergen Sachs
none Details | Review

Description Juergen Sachs 2017-04-13 13:25:58 UTC
When playing the live DASH stream 
   'http://eotv-live.dash.adaptive.level3.net/ses/eotv/manifest.mpd' 
we get a SEGFAULT in qtdemux_parse_moof()
The reason is a NULL pointer for 'stream' in 
----
      GST_DEBUG_OBJECT (qtdemux, "decode time %" G_GINT64_FORMAT
          " (%" GST_TIME_FORMAT ")", decode_time,
          GST_TIME_ARGS (QTSTREAMTIME_TO_GSTTIME (stream, decode_time)));

      /* Discard the fragment buffer timestamp info to avoid using it.
----
The null-pointer-access is an inherited error of an issue in this particular DASH stream: This stream uses seperate init-segments for each representation and the moov-nodes in the init segments have _different track IDs_. This leads to skip the track in qtdemux_parse_trak():
----
        GST_WARNING_OBJECT (qtdemux, "Stream not found, going to ignore it");
        goto skip_track;
----
So something gets wrong with qtdemux and the stream is no longer parsed. 

A minimal test case to reproduce is: 
  gst-launch-1.0 souphttpsrc location=http://eotv-live.dash.adaptive.level3.net/ses/eotv/manifest.mpd ! dashdemux name=demux \
    demux.video_00 ! multiqueue ! qtdemux ! fakesink \
    demux.audio_00 ! fakesink
Comment 1 Juergen Sachs 2017-04-13 13:41:54 UTC
Created attachment 349798 [details] [review]
testcode to verify the root cause of the problem

The patch demonstrates that the root cause of the problem is the changing track_id. But I don't have enough knowledge about the qtdemux to be sure that this does not break other situations.
I would like to start a discussion, how this special DASH situation should be handled correctly:
1. we get an init-segment with moov and taht a lot of moof and mdat
2. after a while, we get another moov with different track id.

I assumed a different track id means a complete different track, like another language for audio. Here is the new track a "continuation" for the old one and should replace this. But I could not found a safe indication, that the track is _changed_ and the stream can be re-used.
Comment 2 Edward Hervey 2017-04-13 14:39:04 UTC
To be honest, I don't get why in push-mode we don't just remove/re-expose everything ...
Comment 3 Edward Hervey 2017-04-13 14:39:59 UTC
An alternative could be
* parse once all traks, i.e. updating those already present with the same trak_id.
* Remove streams that weren't present in the new moov
* Create/add streams that weren't present in the old moov
Comment 4 Seungha Yang 2017-04-16 03:29:46 UTC
Hello Juergen Sachs

I'm happy to meet since you seems to have interest in similar issue which I have :)

As Edward suggested, and like TSdemux is doing when PMT change is detected, Creating/Reusing/Removing streams per moov seems to reasonable way.
I did some works for it in bug #684790, but it's quite old, I'll rebase it and do some more cleanup.
Comment 5 Juergen Sachs 2017-04-18 07:16:03 UTC
I'm not sure, whether I will be happy when a new pad is created: In our particular case we have a hardware decoder at the sink end of the pipeline. When this decoder is closed and reopened, we get a black screen on the transition and this is not allowed in DASH.
But if "New moov means new stream/new pad" is the general intention, we will handle this in our decoder somehow.
Comment 6 Sebastian Dröge (slomo) 2017-04-25 13:25:29 UTC
This sounds indeed like a duplicate of https://bugzilla.gnome.org/show_bug.cgi?id=684790 . Whenever a new moov is received, I don't think we have a way of knowing that anything in there is a continuation of any moov that was seen before.

It would be good to not crash though. So maybe in this bug we can try to find a workaround, or at least error out cleanly instead of crashing. And handling it properly would be handled in the other bug.


If nobody comes up with another solution, I would make this an real error instead of a crash before the 1.12 release.
Comment 7 Juergen Sachs 2017-04-25 13:45:37 UTC
For _our_ purposes the patch above fixes the problem and does not kill any of our test cases. But I don't dare to recomment this for 1.12 release. 
I agree to make the crash to error for 1.12 and than look what we can do with the changes coming from https://bugzilla.gnome.org/show_bug.cgi?id=684790.
Comment 8 Sebastian Dröge (slomo) 2017-04-27 09:58:55 UTC
The code in question already handles stream==NULL, just not in the debug output. This should make the crash disappear at least:

commit 875fc630d567a17f6b0fc0b6bf6a1bd90464e589
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Apr 27 12:56:27 2017 +0300

    qtdemux: Don't crash in debug output if stream==NULL
    
    That case is correctly handled below but not in the debug output.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=781270


Let's handle the remaining problem in the other bug.