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 711849 - smoothstreaming: Better handling of multi audio tracks
smoothstreaming: Better handling of multi audio tracks
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 719357 719575
Blocks:
 
 
Reported: 2013-11-11 14:59 UTC by Guillaume Desmottes
Modified: 2013-12-20 04:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mssdemux: avoid downloading not-linked streams (6.68 KB, patch)
2013-11-18 14:22 UTC, Thiago Sousa Santos
none Details | Review
mssdemux: avoid downloading not-linked streams (6.67 KB, patch)
2013-11-18 14:35 UTC, Thiago Sousa Santos
none Details | Review
mssdemux: avoid downloading not-linked streams (9.71 KB, patch)
2013-11-29 21:17 UTC, Thiago Sousa Santos
none Details | Review
mssdemux: prevent deadlock by pushing buffers from different threads (10.19 KB, patch)
2013-11-29 21:17 UTC, Thiago Sousa Santos
none Details | Review
mssdemux: properly combine flows (3.14 KB, patch)
2013-11-29 21:17 UTC, Thiago Sousa Santos
none Details | Review
mssdemux: fix deadlock on stream switching (1.70 KB, patch)
2013-11-29 21:17 UTC, Thiago Sousa Santos
none Details | Review

Description Guillaume Desmottes 2013-11-11 14:59:54 UTC
At the moment, gstmssdemux exposes one source pad per audio track (one per <StreamIndex> in the manifest file). Each of this track is downloaded even if most of the time only once will actually be used.

One way to fix this could be to only download the stream if the pad is actually linked. But this is not going to help when using input-selector as all pads would be linked anyway in that case.
Comment 1 Sebastian Dröge (slomo) 2013-11-11 15:01:42 UTC
input-selector will return not-linked for the unselected pads, and send a reconfigure event once it becomes selected. That could be used.

But better would be an interface that playbin could use to proxy the current-audio (etc) properties to demuxers. This is also needed for HLS and DASH and DVDs. I'm sure there was a bug about that somewhere but can't find it currently :)
Comment 3 Thiago Sousa Santos 2013-11-18 14:22:05 UTC
Created attachment 260122 [details] [review]
mssdemux: avoid downloading not-linked streams

When a stream gets a not-linked return, it will be marked as so and
won't download any more new fragments until a reconfigure event
is received. This will make mssdemux expose all pads, but only download
fragments for the streams that are actually being used.

Relying on the pads being linked/unlinked isn't enough in this scenario
as there might be an input-selector downstream that is actually discarding
buffers for a given linked pad.
Comment 4 Guillaume Desmottes 2013-11-18 14:32:18 UTC
Review of attachment 260122 [details] [review]:

::: ext/smoothstreaming/gstmssdemux.c
@@ +1400,3 @@
+        gst_pad_push (stream->pad, GST_BUFFER_CAST (object));
+    GST_DEBUG_OBJECT (mssdemux, "Pushed on pad %s:%s result: %d (%s)", stream,
+        GST_DEBUG_PAD_NAME (stream->pad), ret, gst_flow_get_name (ret));

gstmssdemux.c:1401:5: error: format '%s' expects argument of type 'char *', but argument 8 has type 'struct GstMssDemuxStream *' [-Werror=format=]
     GST_DEBUG_OBJECT (mssdemux, "Pushed on pad %s:%s result: %d (%s)", stream,
     ^
gstmssdemux.c:1401:5: error: format '%d' expects argument of type 'int', but argument 10 has type 'const char *' [-Werror=format=]
gstmssdemux.c:1401:5: error: format '%s' expects argument of type 'char *', but argument 11 has type 'int' [-Werror=format=]
gstmssdemux.c:1401:5: error: too many arguments for format [-Werror=format-extra-args]
Comment 5 Thiago Sousa Santos 2013-11-18 14:35:29 UTC
Created attachment 260124 [details] [review]
mssdemux: avoid downloading not-linked streams

When a stream gets a not-linked return, it will be marked as so and
won't download any more new fragments until a reconfigure event
is received. This will make mssdemux expose all pads, but only download
fragments for the streams that are actually being used.

Relying on the pads being linked/unlinked isn't enough in this scenario
as there might be an input-selector downstream that is actually discarding
buffers for a given linked pad.
Comment 6 Olivier Crête 2013-11-18 16:23:01 UTC
This still downloads one of each at startup, right? I'm sure we can do something better. Maybe do a non-serialized downstream query and only start streaming if that returns TRUE?
Comment 7 Thiago Sousa Santos 2013-11-25 21:13:58 UTC
(In reply to comment #6)
> This still downloads one of each at startup, right? I'm sure we can do
> something better. Maybe do a non-serialized downstream query and only start
> streaming if that returns TRUE?

Yes. I want to do that on a future improvement. For now I just want them to switch.

I have made it work with a few patches to mssdemux and core/base elements. The main issue now is that after a switch the new stream starting might push buffers a little bit behind the downstream position and because of input-selector and discont flag handling, basesink/audiosink will actually play those buffers and the stream's audio will go back and forth for a few seconds. I'd assume some element in playbin would drop those buffers because they are 'late' (behind current playback position) and only play from the correct position to make sure streams are aligned after switching.

Any ideas on how to handle this?
Comment 8 Sebastian Dröge (slomo) 2013-11-26 13:19:47 UTC
(In reply to comment #5)
> Created an attachment (id=260124) [details] [review]
> mssdemux: avoid downloading not-linked streams
> 
> When a stream gets a not-linked return, it will be marked as so and
> won't download any more new fragments until a reconfigure event
> is received. This will make mssdemux expose all pads, but only download
> fragments for the streams that are actually being used.

Didn't read the patch but make sure to start again if you receive a RECONFIGURE event on a stream. input-selector sends that when it switches pads. Also input-selector usually (unless configured otherwise, playbin does that for external subtitles) returns not-linked for unselected pads.
Comment 9 Thiago Sousa Santos 2013-11-29 21:17:05 UTC
Created attachment 263165 [details] [review]
mssdemux: avoid downloading not-linked streams

When a stream gets a not-linked return, it will be marked as so and
won't download any more new fragments until a reconfigure event
is received. This will make mssdemux expose all pads, but only download
fragments for the streams that are actually being used.

Relying on the pads being linked/unlinked isn't enough in this scenario
as there might be an input-selector downstream that is actually discarding
buffers for a given linked pad.
Comment 10 Thiago Sousa Santos 2013-11-29 21:17:09 UTC
Created attachment 263167 [details] [review]
mssdemux: prevent deadlock by pushing buffers from different threads

When streams are switching, the old active stream can be blocked because
input-selector will block not-linked streams. In case the mssdemux's
stream loop is blocked pushing a buffer to a full queue downstream it will
never unblock as the queue will not drain (input-selector is blocking).

In this scenario, stream switching will deadlock as input-selector is
waiting for the newly active stream data and the stream_loop that would
push this data is blocked waiting for input-selector.

To solve this issue, whenever an stream is reactivated on a reconfigure
it will enter into the 'catch up mode', in this mode it can push buffers
from its download thread until a certain timestamp. This works because
this timestamp will always be behind or equal to the maximum timestamp
pushed for all streams, after pushing data for this timestamp, the stream
will go back to default and be pushed sequentially from the main
streaming thread. By this time, the input-selector should have already
released the thread.
Comment 11 Thiago Sousa Santos 2013-11-29 21:17:15 UTC
Created attachment 263168 [details] [review]
mssdemux: properly combine flows

Use a proper flow combination function to maintain correct return to
make sure a not-linked isn't returned while a stream switch is happening
Comment 12 Thiago Sousa Santos 2013-11-29 21:17:20 UTC
Created attachment 263169 [details] [review]
mssdemux: fix deadlock on stream switching

Make sure the dataqueue is set to flushing to unblock a possible
_peek on the queue from a stream that is being put to sleep.
Comment 13 Thiago Sousa Santos 2013-12-18 22:56:35 UTC
This is partially fixed, the mssdemux part is done. Fixes are still needed on baseaudiosink and pipeline buffering.

commit 67f3190301bfdd6673cbfd4ceb20d9791187df17
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon Dec 16 16:14:24 2013 -0300

    mssdemux: remove the stream loop task
    
    Download and push from the same task, makes code a lot simpler
    to maintain. Also pushing from separate threads avoids deadlocking
    when gst_pad_push blocks due to downstream queues being full

commit 4e6e1315da7b12a31ae535d889652101611d9f6b
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Fri Dec 13 17:31:11 2013 -0300

    mssdemux: Improve logging
    
    Show the stream's pad on log messages to make easier to debug
    issues in the multiple threads

commit 056420940e62bf41ce0451cc4b8417acba500454
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Tue Dec 10 18:08:40 2013 -0300

    mssdemux: improve flow return handling
    
    Handle different flow returns both from the streaming and the
    downloading loops

commit e847bea1e1e2d7d68ee2830861201f5957ede7ee
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Tue Dec 10 15:41:00 2013 -0300

    mssdemux: remove stream locks
    
    Simplify the locking by using a single lock instead of having one
    lock per stream. This still works and is simpler to maintain.

commit 2ce4f6a8e4c8aa33d08d9f5fe28a83779fadf03f
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Tue Nov 12 09:58:31 2013 -0300

    mssdemux: avoid downloading not-linked streams
    
    When a stream gets a not-linked return, it will be marked as so and
    won't download any more new fragments until a reconfigure event
    is received. This will make mssdemux expose all pads, but only download
    fragments for the streams that are actually being used.
    
    Relying on the pads being linked/unlinked isn't enough in this scenario
    as there might be an input-selector downstream that is actually discarding
    buffers for a given linked pad.
    
    When streams are switching, the old active stream can be blocked because
    input-selector will block not-linked streams. In case the mssdemux's
    stream loop is blocked pushing a buffer to a full queue downstream it will
    never unblock as the queue will not drain (input-selector is blocking).
    
    In this scenario, stream switching will deadlock as input-selector is
    waiting for the newly active stream data and the stream_loop that would
    push this data is blocked waiting for input-selector.
    
    To solve this issue, whenever an stream is reactivated on a reconfigure
    it will enter into the 'catch up mode', in this mode it can push buffers
    from its download thread until it reaches the currrent GstSegment's position.
    This works because this timestamp will always be behind or equal to the maximum
    timestamp pushed for all streams, after pushing data for this timestamp,
    the stream will go back to default and be pushed sequentially from the main
    streaming thread. By this time, the input-selector should have already
    released the thread.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=711849
Comment 14 Thiago Sousa Santos 2013-12-20 04:26:04 UTC
This is now working. Marking as fixed.