[Bug 783028] gstmpdparser : Crash when playing some of the Dash LIVE URL's with Gstreamer v1.12

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #2 from Edward Hervey <***@bilboed.com> ---
Note: It also fails with
http://vm2.dashif.org/livesim/segtimeline_1/testpic_6s/Manifest.mpd

Confirms it's not a race, but a real issue. Might be related to segment
timeline support ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #4 from Edward Hervey <***@bilboed.com> ---
Also fails with 1.12

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Seungha Yang <***@lge.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@lge.com

--- Comment #5 from Seungha Yang <***@lge.com> ---
I'm also investigating this bug. This special mpd has zero minimumUpdatePeriod
which is mentioned as special case by spec. Not only handle this crash, we
might add handle for this zero minimumUpdatePeriod IMHO.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Thiago Sousa Santos <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

--- Comment #3 from Thiago Sousa Santos <***@gmail.com> ---
What should happen is that the AdaptiveDemux streams should remain the same and
the internal dash streams are replaced. What I believe is the issue that the
old ones are removed and not replaced. It seems to be related to the [streams,
next_streams, prepared_streams] thing as dashdemux expects them to be in
streams but I think they are as "prepared_streams" at that stage. Will continue
debugging.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@bilboed.com
Version|1.12.x |git master

--- Comment #1 from Edward Hervey <***@bilboed.com> ---
The problem seems to be the following:
* Manifest is initially parsed
* GstMpdClient gets created along with a list of GstActiveStreams
* A GstActiveStream gets assigned to each GstAdaptiveDemuxStream in
setup_streams()
* Late the manifest gets updated
* A new GstMpdClient gets created along with a list of GstActiveStreams
* That client replaces the previous one
* The previous one gets destroyed along with the list of GstActiveStreams

...

And eventually code tries accessing the GstAdaptiveDemuxStream's
active_stream which ... is from the previous GstMpdClient and therefore
invalid.

Not 100% sure who's at fault, but should gst_dash_demux_setup_streams() be
called again when updating the manifest ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #7 from Thiago Sousa Santos <***@gmail.com> ---
Created attachment 352850
--> https://bugzilla.gnome.org/attachment.cgi?id=352850&action=edit
adaptivedemux: do not erase data while updates-loop is running

Make sure the manifest update loop is stopped before proceeding with the
resetting of the manifest data. Otherwise, the updates loop will try to
use it and it leads to a segfault

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #8 from Thiago Sousa Santos <***@gmail.com> ---
Created attachment 352851
--> https://bugzilla.gnome.org/attachment.cgi?id=352851&action=edit
dashdemux: update manifest streams correctly if pads aren't exposed

In some cases, it is possible that we need to update the manifest before
pads have been exposed at all. If there are no current pads, just expose
the next prepared streams. This doesn't handle the case where a manifest
update would happen while a live streams is changing periods, which is a
type of use case that we're unaware of real usages yet.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #6 from Thiago Sousa Santos <***@gmail.com> ---
Created attachment 352849
--> https://bugzilla.gnome.org/attachment.cgi?id=352849&action=edit
mpdparser: remove duplicate free of client data

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #9 from Thiago Sousa Santos <***@gmail.com> ---
This still has issues after those fixes. Right now it tries to start at 'now'
time but that is in the future in terms of the stream so that leads to this
check failing:

https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/adaptivedemux/gstadaptivedemux.c?id=304a628de777ed0a180e8a23dc0e689ca9b0e24c#n3725

And it just aborts. Trying to figure out what exactly does this check mean to
do.

A side question is whether we should really care to seek to 'now' time or if it
is simpler just to go to the last fragment and start with it. A downside here
is that if the fragment is very long it might make us be too far in the past
for a live stream. Maybe it is not a good idea.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #10 from Thiago Sousa Santos <***@gmail.com> ---
For the record, that check fails because it hasn't pushed any buffers because
it tries to seek beyond the last fragment timestamp so it hasn't updated its
segment.position and it is 0. So it is not in the live seeking range.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #11 from Edward Hervey <***@bilboed.com> ---
(In reply to Thiago Sousa Santos from comment #9)

Post by "GStreamer" (GNOME Bugzilla)
A side question is whether we should really care to seek to 'now' time or if
it is simpler just to go to the last fragment and start with it. A downside
here is that if the fragment is very long it might make us be too far in the
past for a live stream. Maybe it is not a good idea.

The problem is that you rarely want to use the absolute latest fragment. The
live seek range returned takes into account the "safety margin" that most
protocols recommend (i.e. a few fragments before the latest one).

(In reply to Thiago Sousa Santos from comment #10)

Post by "GStreamer" (GNOME Bugzilla)
For the record, that check fails because it hasn't pushed any buffers
because it tries to seek beyond the last fragment timestamp so it hasn't
updated its segment.position and it is 0. So it is not in the live seeking
range.

Ok, I'll look into it. Maybe we should take into account the fact that
nothing has been downloaded yet.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #12 from Edward Hervey <***@bilboed.com> ---
The reason why it ends up with a position of 0 is because ... we try to seek to
an invalid position.

The manifest doesn't contain a suggestedPresentationDelay, which should be used
to figure out how much back from "now" we should start playing.

Regarding that property, the specs state that : "When not specified, then no
value is provided and the client is expected to choose a suitable value."

We currently don't have a default suitable value.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #13 from Edward Hervey <***@bilboed.com> ---
(In reply to Seungha Yang from comment #5)

Post by "GStreamer" (GNOME Bugzilla)
I'm also investigating this bug. This special mpd has zero
minimumUpdatePeriod which is mentioned as special case by spec. Not only
handle this crash, we might add handle for this zero minimumUpdatePeriod
IMHO.

Do you have any updates on that special case ? Can you file a separate bug ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #14 from Seungha Yang <***@lge.com> ---
(In reply to Edward Hervey from comment #13)

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Seungha Yang from comment #5)

Post by "GStreamer" (GNOME Bugzilla)
I'm also investigating this bug. This special mpd has zero
minimumUpdatePeriod which is mentioned as special case by spec. Not only
handle this crash, we might add handle for this zero minimumUpdatePeriod
IMHO.

Do you have any updates on that special case ? Can you file a separate bug ?

I'll update about it in next week with new bug. Note that, when I saw dash.js
implementation, it limits minimum update period to 1 sec whatever
minimumUpdatePeriod is defined.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #15 from Edward Hervey <***@bilboed.com> ---
I pushed the two first commits, since those were safe. I'm not sure the last
patch is needed anymore.

commit 5a693fdd9dc1e7e227309424503e3e0e78f9e9c0
Author: Thiago Santos <***@gmail.com>
Date: Mon May 29 22:28:21 2017 -0700

adaptivedemux: do not erase data while updates-loop is running

Make sure the manifest update loop is stopped before proceeding with the
resetting of the manifest data. Otherwise, the updates loop will try to
use it and it leads to a segfault

https://bugzilla.gnome.org/show_bug.cgi?id=783028

commit 95a27867411826549119711a3c35b3493ab3070e
Author: Thiago Santos <***@gmail.com>
Date: Mon May 29 22:26:09 2017 -0700

mpdparser: remove duplicate free of client data

https://bugzilla.gnome.org/show_bug.cgi?id=783028

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352849|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352850|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #16 from Seungha Yang <***@lge.com> ---
Hello Edward Hervey
I'll update on bug #780589 for minimumUpdatePeriod handling

Anyway, I cannot access reported issue streams now... (maybe temporary server
close?)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Athanasios Oikonomou <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

--- Comment #17 from Athanasios Oikonomou <***@gmail.com> ---
I am getting crash using that live dash url on 1.12:
https://live-w.lwc.vrtcdn.be/groupc/live/d05012c2-6a5d-49ff-a711-79b32684615b/live.isml/.mpd

Is the same error or a new one?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Arun Raghavan <***@arunraghavan.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@arunraghavan.net

--- Comment #18 from Arun Raghavan <***@arunraghavan.net> ---
That URL doesn't crash on master, but doesn't play either. Does play with 1.10.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #19 from Debdeep <***@gmail.com> ---
Can still reproduce the crash with the patch using the below patches :
mpdparser: remove duplicate free of client data
adaptivedemux: do not erase data while updates-loop is running

Used the same URL as reported :
http://vm2.dashif.org/livesim/segtimeline_1/testpic_2s/Manifest.mpd

Crash still appears in the
gst_mpd_client_get_next_segment_availability_start_time() function.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

A Ashley <***@ashley-family.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@ashley-family.net

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.12.1

--- Comment #20 from Edward Hervey <***@bilboed.com> ---
Existing patches backported to 1.12. Will check remaining issue afterwards

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

--- Comment #21 from Debdeep <***@gmail.com> ---
Is there is any other patch/fix available for this, as the reported issue with
the URL http://vm2.dashif.org/livesim/segtimeline_1/testpic_2s/Manifest.mpd,
can still be reproduced with v1.12.1 ?

Also, another crash in mpdparser is observed when using the below live URL's :
http://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestBR.mpd"
http://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestARD.mpd"

Function in which crash is observed :
gst_mpdparser_get_rep_idx_with_max_bandwidth()

Line :
framerate = representation->RepresentationBase->frameRate;

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=783028

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
Target Milestone|1.12.1 |1.12.2

--- Comment #22 from Edward Hervey <***@bilboed.com> ---
Pushed to both master and 1.12

commit ead63a268647eff2bcf92d8dc55ca41ee44247c2
Author: Thiago Santos <***@gmail.com>
Date: Mon May 29 22:47:10 2017 -0700

dashdemux: update manifest streams correctly if pads aren't exposed

In some cases, it is possible that we need to update the manifest before
pads have been exposed at all. If there are no current pads, just expose
the next prepared streams. This doesn't handle the case where a manifest
update would happen while a live streams is changing periods, which is a
type of use case that we're unaware of real usages yet.

https://bugzilla.gnome.org/show_bug.cgi?id=783028