GNOME Bugzilla – Bug 684790
isomp4/qtdemux: Whenever a moov atom is found, restart the demuxer
Last modified: 2018-10-04 02:29:55 UTC
The attached patch provides a way for the qtdemux element to continue working whenever a MOOV atom is received without going through a state change. Basically, on MPEG DASH, there are files that are not self-initialized, that is, files that in order to be demuxed at any desired segment they first need to send the initialization segment (the segment containing the MOOV atom). With this patch it is now possible for qtdemux to continue working correctly without the need to go the READY state and then back to the PAUSED/PLAYING.
Hi Jorge, no patch is actually attached ;)
Created attachment 234599 [details] [review] Patch
Is this still needed, dash works nowadays. If it is, could you please rebase the patch against latest git?
(In reply to Sebastian Dröge (slomo) from comment #3) > Is this still needed, dash works nowadays. If it is, could you please rebase > the patch against latest git? We really need this kind of approach for DASH. One of examples, qtdemux does not support track_id change in case of MPEGDASH Representation switching. There is no restriction about track_id change on DASH spec., except for the case that bitstreamSwitching attribute was set true. For that reason, we cannot support this mpd http://vm2.dashif.org/livesim-dev/testpic2_2s/Manifest.mpd Not only that case, other boxes such as trex should be parsed again per moov. Since this report is quite old, can I report new patchset on top of the latest code?
Sure
Created attachment 340289 [details] [review] qtdemux: Re-expose streams per moov box, if needed New moov box implies incoming streams might have something differences from that of a previous moov. So, it makes sense that exposing new srcpads or removing oldpads, if there are something differences per moov.
Thiago, what do you think? This should probably also come with (conditional) GstStreams usage, like in tsdemux whenever a new program is found.
(In reply to Sebastian Dröge (slomo) from comment #7) > Thiago, what do you think? > > This should probably also come with (conditional) GstStreams usage, like in > tsdemux whenever a new program is found. That's what I really want to do (GstStream usage in qtdemux and demuxer's own streamcollection object). As we know, playbin3 case, demuxer will be reused unlike playbin2 (decodebin2 makes new demuxer whenever period change). So, qtdemux may need it's own StreamCollection object and updating it based on new stream-start event. However, because the use of GstStream may cause lots of structure change, the want to listen maintainer's opinions on that.
Review of attachment 340289 [details] [review]: Took a quick look, overall code looks fine. Will try to run it later today. Besides DASH track_Id changes, what other cases would be covered here? What should we do with pull-mode? Should we also test that it works by sending sequentially 2 independent moov files? Just checking what should we add together with this to validate media tests. This change looks simple enough that we could do it without GstStreams API (as it should work anyway) and do the porting on a separate patch to make reviewing and spotting regressions easier. ::: gst/isomp4/qtdemux.h @@ +235,3 @@ + + /* Whether demux needs to re-expose or remove srcpads */ + gboolean need_reconfigure; Reconfigure has a certain meaning in gstreamer related to the RECONFIGURE event, maybe we want something like "has_new_headers" or "has_new_moov"?
(In reply to Thiago Sousa Santos from comment #9) > This change looks simple enough that we could do it without GstStreams API > (as it should work anyway) Yes but with GstStreams we could do like tsdemux and only add/remove streams that have actually changed for example. And properly advertise the available streams without relying on decodebin3 to do that.
(In reply to Thiago Sousa Santos from comment #9) > Review of attachment 340289 [details] [review] [review]: > Besides DASH track_Id changes, what other cases would be covered here? What > should we do with pull-mode? I expect this will cover all push-mode fragmented format cases. And pull-mode does not considered. Because ISOBMFF spec is saying that only one moov box is allowed, does it makes sense that multi-moov on pull-mode? > Should we also test that it works by sending sequentially 2 independent moov > files? Just checking what should we add together with this to validate media > tests. In my opinion, any DASH mpd (multi-Representation, Bitrate switching cases) with playbin3 can be a test case for this, because playbin3 reuses demuxer. > This change looks simple enough that we could do it without GstStreams API > (as it should work anyway) and do the porting on a separate patch to make > reviewing and spotting regressions easier. I'll split patch :) > Reconfigure has a certain meaning in gstreamer related to the RECONFIGURE > event, maybe we want something like "has_new_headers" or "has_new_moov"? Because my intention for the boolean variable was to indicate "exposing new srcpad or removing any oldpads is needed", but not "new moov", I will consider new naming excluding "reconfigure"
Created attachment 340348 [details] [review] qtdemux: Unref oldpad using g_slist_free_full() From only demux point of view, increased pad refcount by this patch does not seems to make sense. But if we do not it, it causes crash somewhere in decodebin.
Created attachment 340349 [details] [review] qtdemux: Expose srcpad whenever new stream is found for fragmented stream Allow stream addition in the new moov. A new stream can be found in streaming case such as MPEG-DASH, and there is no reason to ignore the new stream.
Created attachment 340350 [details] [review] qtdemux: Expose only activated streams which belong to current moov With updated moov, any streams which belong to previous moov (but not current) shouldn't be exposed.
Created attachment 340351 [details] [review] qtdemux: Parsing trex again with new moov Trex box has default values for sample fragments, and the defaults have no dependency on previous moov. So, demux needs to parse it per moov.
Hello Thiago and slomo I think my previous solution need to be more generalized. That is, not only for new moov box, but also a stream which following new stream-start event should be handled. Note that when playbin3 is used, there is no specific processing for stream-start event until now, but streams of before/after stream-start must be differentiated.
After stream-start there should be a new moov though, no? But basically after stream-start, the demuxer should consider it as if a completely new stream started now. Why do you think your previous patches are wrong though?
(In reply to Sebastian Dröge (slomo) from comment #17) > Why do you think your previous patches are wrong though? Not wrong, but seems cannot cover both stream-start/new moov case efficiently...
Created attachment 342412 [details] [review] qtdemux: Use GList to manage QtDemuxStream
Created attachment 342413 [details] [review] qtdemux: Switch id for managing streams to stream-id To differentiate streams of after/before new stream-start event, use stream-id with track-id
Created attachment 342414 [details] [review] qtdemux: Create stream whenever got new moov Whenever demux got moov, create new stream. Check only whether there is duplicated track-id in current moov. This patch is pre-work for rework of moov handling.
Created attachment 342415 [details] [review] qtdemux: Try to expose whenever got new moov or new stream-start Whenever got new moov or new stream-start, demux will try to expose new pad by following rule. Demux compare stream-id in the current moov with previous one, then * If matched stream-id found, reuse existing pad (most common case) * Although no matching stream-id, if there is a pad can be reused, no need to expose pad, but send new stream-start (mostly upstream sent new stream-start) * Otherwise, expose new pad with new stream-start
Basic idea is, duplication/existence/new stream check will not be done in _parse_trak() function. Then, whenever got new moov, call _expose_streams() and decides reuse/expose/remove stream in that function.
Created attachment 342685 [details] [review] qtdemux: Switch id for managing streams to stream-id Fix conflict when applying patch
Created attachment 342686 [details] [review] qtdemux: Try to expose whenever got new moov or new Expose pad whenever new stream-id was found. Previous patch was trying to reuse srcpad, if downstream can accept the caps although stream-id was not patched. But in some cases, parser element does not forward caps event to downstream when caps was not changed. To ensure stream-start -> caps -> segment sequence, I changed for demux to make new pad when new steram-id was found in this patch.
Created attachment 349904 [details] [review] qtdemux: Use GList to manage QtDemuxStream
Created attachment 349905 [details] [review] qtdemux: Use track-id and upstream-id to manage streams
Created attachment 349906 [details] [review] qtdemux: Create stream whenever got new moov
Created attachment 349907 [details] [review] qtdemux: Try to expose whenever got new moov or new stream-start
Created attachment 349908 [details] [review] tests: qtdemux: Add test for stream change Add test case to verify track-id change and stream change
Hello slomo, Thiago and Edward Could you review this patches after GST1.12 code freeze? (it's quite a big change...) Main concept is "trying to make new stream per moov OR per stream-start" and compare the new stream with that of previous moov, in order to decide whether re-exposing is required or not. There seems to no regression on playbin/playbin3 usecase with gst-validate and manually run some multi-period DASH-IF streams. Note that, in my environment, following TCs are failed only in playbin3 regardless of my patch [4 / 270] validate.dash.playback.reverse_playback.dash_exMPD_BIP_TC1: Timeout (Application timed out: 240.0 secs) [8 / 270] validate.dash.playback.seek_with_stop.dash_exMPD_BIP_TC1: Timeout (Application timed out: 240.0 secs) However, although my patches are applied, stream change does not work in playbin3 because a bug in decodebin3. It was reported in bug #772230 (nobody review it yet) and the patch in bug #772230 could fix stream change issue in playbin3. I hope this could fix the bug #781270 also.
Review of attachment 349904 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +97,3 @@ #define QTDEMUX_TREE_NODE_FOURCC(n) (QT_FOURCC(((guint8 *) (n)->data) + 4)) +#define STREAM_IS_EOS(s) ((s)->time_position == GST_CLOCK_TIME_NONE) Put as a separate patch. @@ +102,3 @@ +#define QTDEMUX_NTH_STREAM(demux,i) ((QtDemuxStream *)g_list_nth_data(demux->active_streams,i)) +#define QTDEMUX_N_STREAMS(demux) (g_list_length(demux->active_streams)) This summarizes my main "issue" with this patch. Using a linked list instead of an array doesn't bring much overhead for qtdemux but: 1) We need to avoid using g_list_nth_data() as much as possible 2) We need to avoid using g_list_length() as much as possible They are both functions that iterate over all/most of the list. n_streams => Just use/update the n_streams variable, which you update whenever a stream is added/removed nth_stream => In virtually all scenarios we are either: * Using the first entry (just use it directly) * Using an index which we calculated in the same function while iterating (just remember the stream entry) @@ +2093,2 @@ if (hard) { + for (iter = qtdemux->active_streams; iter; iter = next) { Use g_list_free_full() @@ +2557,2 @@ static void gst_qtdemux_remove_stream (GstQTDemux * qtdemux, int i) This could just be modified to be (GstQTDemux *demux, GstQTDemuxStream *stream); And then you can do active_streams = g_list_remove(active_streams, stream); @@ +2654,3 @@ return; + stream = QTDEMUX_NTH_STREAM (qtdemux, 0); stream = (GstQTDemuxStream*) qtdemux->active_streams->data; Or make a macro QTDEMUX_FIRST_STREAM() if you want @@ +3377,3 @@ } /* try to get it fast and simple */ This "optimization" needs to go away if we use a linked list, because we can no longer do direct access. @@ +5732,2 @@ GST_DEBUG_OBJECT (qtdemux, "we reached the end of our segment."); + QTDEMUX_NTH_STREAM (qtdemux, index)->time_position = GST_CLOCK_TIME_NONE; Remember the stream in the previous iteration instead of using NTH_STREAM @@ +6125,3 @@ return -1; + stream = QTDEMUX_NTH_STREAM (demux, smallidx); Store the stream object in the previous iteration. @@ +11616,2 @@ /* now we are ready to add the stream */ + if (QTDEMUX_N_STREAMS (qtdemux) >= GST_QTDEMUX_MAX_STREAMS) We no longer have this limitation by switching to a linked list :D
Review of attachment 349905 [details] [review]: Definitely brings in some much needed improvements
Review of attachment 349906 [details] [review]: ok
Review of attachment 349907 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +2145,3 @@ g_list_delete_link (qtdemux->active_streams, iter); } + for (iter = qtdemux->old_streams; iter; iter = next) { g_list_free_full() @@ +11929,3 @@ GST_DEBUG_OBJECT (qtdemux, "exposing streams"); + /* At below, figure out which stream in active_streams has identical stream-id You can only re-use streams dynamically within elements that support it (parsebin, decodebin3,...). You need to check for that to do so. Checkout tsdemux for reference.
(In reply to Edward Hervey from comment #35) > Review of attachment 349907 [details] [review] [review]: > You can only re-use streams dynamically within elements that support it > (parsebin, decodebin3,...). > > You need to check for that to do so. Checkout tsdemux for reference. Thanks for detailed review Can I ask some stupid question? :) Roughly, my understanding about your suggestion is - check GST_BIN_FLAG_STREAMS_AWARE to know whether parent bin can support dynamic stream change. Then, if it's supported (playbin3 case), re-use reusable streams and expose/remove only necessary streams. otherwise (playbin case), expose all new streams and remove all the previous streams with no-more-pads signal Is it correct understanding?
(In reply to Seungha Yang from comment #36) > Can I ask some stupid question? :) > Roughly, my understanding about your suggestion is > - check GST_BIN_FLAG_STREAMS_AWARE to know whether parent bin can support > dynamic stream change. > > Then, > if it's supported (playbin3 case), re-use reusable streams and expose/remove > only necessary streams. > otherwise (playbin case), expose all new streams and remove all the previous > streams with no-more-pads signal > > Is it correct understanding? It's not a stupid question, and yes, this is what I meant :)
Created attachment 350051 [details] [review] qtdemux: Add parentheses in macro
Created attachment 350052 [details] [review] qtdemux: Adjust the number of args of some functions To be used with g_list_free_full in the next patch
Created attachment 350053 [details] [review] qtdemux: Use GList to manage QtDemuxStream Move to GList from static array
Created attachment 350054 [details] [review] qtdemux: Store stream-id to manage streams In order to figure out stream change such as track-id change or stream-id change, demux will store stream-id per QtDemuxStream structure.
Created attachment 350055 [details] [review] qtdemux: Create stream whenever got new moov Whenever demux got moov, demux will create new stream. Only exception is duplicated track-id in a moov box. In that case the first stream will be accepted. This patch is pre-work for rework of moov handling.
Created attachment 350056 [details] [review] qtdemux: Remove duplication of initializing member variables Most initialization of variables in gst_qtdemux_init() are duplicated in gst_qtdemux_reset() function.
Created attachment 350057 [details] [review] qtdemux: Try to expose whenever got new moov or new stream-start
Review of attachment 350055 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +9601,3 @@ + stream2 = QTDEMUX_STREAM (id2); + + if (stream1->track_id > stream2->track_id) { Or you could just make the function _func(QtDemuxStream *stream1, QtDemuxStream *stream2) { return stream1->track_id - stream2->track_id; } :)
Review of attachment 350057 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +11900,3 @@ + +static gboolean +qtdemux_reuse_and_configure_steram (GstQTDemux * qtdemux, s/steram/stream/
Created attachment 350068 [details] [review] qtdemux: Create stream whenever got new moov
Created attachment 350069 [details] [review] qtdemux: Try to expose whenever got new moov or new stream-start
(In reply to Edward Hervey from comment #46) > Review of attachment 350057 [details] [review] [review]: I'm always thankful for your review :D
Overall the patches are really good at this point. Let's see if we can get them merged after 1.12 is released. Thanks !
(In reply to Edward Hervey from comment #50) > Overall the patches are really good at this point. Let's see if we can get > them merged after 1.12 is released. > > Thanks ! Wow!! Thanks for review
*** Bug 782519 has been marked as a duplicate of this bug. ***
Hi Seungha, could you rebase the patches against current master ? Thanks
Created attachment 351800 [details] [review] [1/7] qtdemux: Add parentheses in macro
Created attachment 351801 [details] [review] [2/7] qtdemux: Adjust the number of args of some functions
Created attachment 351802 [details] [review] [3/7] qtdemux: Use GList to manage QtDemuxStream
Created attachment 351803 [details] [review] [4/7] qtdemux: Store stream-id to manage streams
Created attachment 351804 [details] [review] [5/7] qtdemux: Create stream whenever got new moov
Created attachment 351805 [details] [review] [6/7] qtdemux: Remove duplication of initializing member variables
Created attachment 351806 [details] [review] [7/7] qtdemux: Try to expose whenever got new moov or new stream-start
Created attachment 354819 [details] [review] [8/8] qtdemux: Protect _expose_streams() from flush event Flush during stream change can break autoplugging or the flush event could be dropped.
Edward, you merge them?
They don't apply cleanly, so the patches need to be rebased (again)
(In reply to Edward Hervey from comment #63) > They don't apply cleanly, so the patches need to be rebased (again) I'll update rebased patches after regression test (using gst-validate)
Created attachment 371793 [details] [review] [1/9] qtdemux: Add parentheses in macro
Created attachment 371794 [details] [review] [2/9] qtdemux: Adjust the number of args of some functions
Created attachment 371795 [details] [review] [3/9] qtdemux: Use GList to manage QtDemuxStream
Created attachment 371796 [details] [review] [4/9] qtdemux: Store stream-id to manage streams
Created attachment 371797 [details] [review] [5/9] qtdemux: Create stream whenever got new moov
Created attachment 371798 [details] [review] [6/9] qtdemux: Remove duplication of initializing member variables
Created attachment 371799 [details] [review] [7/9] qtdemux: Try to expose whenever got new moov or new stream-start
Created attachment 371800 [details] [review] [8/9] qtdemux: Protect _expose_streams() from flush event
Created attachment 371801 [details] [review] [9/9] tests: qtdemux: Add test for stream change
I could not find any regression on qtdemux with my patch set (using gst-validate-launch for playbin and playbin3)
Thanks for that ! Can you also check why the qtmux unit test is failing ? Seems to be caused by those patches. Once that is cleared, I'll push them
Created attachment 371834 [details] [review] [10/10] qtdemux: Update pending segment in _expose_streams() only for push mode New segment will be made in loop function in case of pull mode. So, if it was set in _expose_streams(), duplicated segment events can be pushed which cause qtmux unit test failure
(In reply to Edward Hervey from comment #75) > Thanks for that ! Can you also check why the qtmux unit test is failing ? > Seems to be caused by those patches. Once that is cleared, I'll push them I updated new patch to fix qtmux unit test failure.
Review of attachment 371794 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +2573,3 @@ gst_tag_list_unref (stream->stream_tags); if (stream->pad) { + GstElement *qtdemux = gst_pad_get_parent_element (stream->pad); This is inefficient. Just store the qtdemux object in a stream field. @@ +2575,3 @@ + GstElement *qtdemux = gst_pad_get_parent_element (stream->pad); + + if (G_UNLIKELY (!qtdemux)) { This should never happen (and will go away once you store it in the stream)
Review of attachment 371795 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +2145,3 @@ gst_flow_combiner_reset (qtdemux->flowcombiner); + for (iter = qtdemux->active_streams; iter; iter = g_list_next (iter)) { + gst_qtdemux_stream_clear (QTDEMUX_STREAM (iter->data)); or just g_list_foreach() ? @@ +2692,3 @@ return; + stream = QTDEMUX_FIRST_STREAM (qtdemux); It would be cleaner if you moved the check before (the n_streams one) to just under with "if (!stream) return;" @@ +5897,2 @@ guint size; gint index; Shouldn't stream->track_id be used instead of index in this function and others ? That seems more consistent than a list index where the list/order might potentially change. @@ +6292,2 @@ int i; int smallidx = -1; Same comment as before
Review of attachment 371796 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +1935,3 @@ } +/* Copied from mpegtsbase code */ This should no longer be needed now that we have the gst_pad_create_stream_id functions in core.
Apart from the comments above, this looks good. Do you think there are any urls from the dashif forum we could add to the "adaptive" gst-validate testsuite ?
(In reply to Edward Hervey from comment #81) > Apart from the comments above, this looks good. I'll update patch again. Thanks for your review :) > Do you think there are any urls from the dashif forum we could add to the > "adaptive" gst-validate testsuite ? This url is one of issue streams and this patch set could solve it. (different track-id per representation). I hope the stream will be added to test case. http://vm2.dashif.org/livesim-dev/testpic2_2s/Manifest.mpd Another suggestion is a "multi audio track" dash stream. http://dash.akamaized.net/dash264/TestCases/10a/1/iis_forest_short_poem_multi_lang_480p_single_adapt_aaclc_sidx.mpd
(In reply to Edward Hervey from comment #80) > Review of attachment 371796 [details] [review] [review]: > > ::: gst/isomp4/qtdemux.c > @@ +1935,3 @@ > } > > +/* Copied from mpegtsbase code */ > > This should no longer be needed now that we have the > gst_pad_create_stream_id functions in core. gst_pad_create_stream_id() function seems to be usable only if we have src pad. But I'd like to create stream_id before srcpad is created. (same as mpegtsbase usecase) Is there new util function for that? Note that mpegtsbase's _get_upstream_id() still exist.
(In reply to Seungha Yang from comment #83) > gst_pad_create_stream_id() function seems to be usable only if we have src > pad. But I'd like to create stream_id before srcpad is created. (same as > mpegtsbase usecase) > > Is there new util function for that? Note that mpegtsbase's > _get_upstream_id() still exist. Ah, that explains why you weren't using that function (not having the pad). But it's a bit stupid because the only thing the function uses the pad for is ... for the debugging message :) We should maybe have a function that allows creating such stream_id at the element level have the pad use that.
> Ah, that explains why you weren't using that function (not having the pad). > But it's a bit stupid because the only thing the function uses the pad for > is ... for the debugging message :) We should maybe have a function that > allows creating such stream_id at the element level have the pad use that. Create bug #795976 to track that issue.
(In reply to Edward Hervey from comment #84) > (In reply to Seungha Yang from comment #83) Fully agree :)
Created attachment 371848 [details] [review] [2/9] qtdemux: Adjust the number of args of some functions
Created attachment 371849 [details] [review] [3/9] qtdemux: Use GList to manage QtDemuxStream * Move to GList from static array * Logging track-id instead of array index. It's more meaningful.
Created attachment 371851 [details] [review] [4/9] qtdemux: Store stream-id to manage streams Just rebase commit with FIXME comment.
Created attachment 371852 [details] [review] [7/9] qtdemux: Try to expose whenever got new moov or new stream-start the patch for qtmux unit test is merged into this
And finally in \o/ Thanks !
(In reply to Seungha Yang from comment #88) > Created attachment 371849 [details] [review] [review] > [3/9] qtdemux: Use GList to manage QtDemuxStream > > * Move to GList from static array > * Logging track-id instead of array index. It's more meaningful. Why a GList instead of a GArray or GPtrArray? Unless you need to remove/insert elements in the middle/beginning, the latter is usually more efficient in all regards.
(In reply to Sebastian Dröge (slomo) from comment #92) > (In reply to Seungha Yang from comment #88) > > Created attachment 371849 [details] [review] [review] [review] > > [3/9] qtdemux: Use GList to manage QtDemuxStream > > > > * Move to GList from static array > > * Logging track-id instead of array index. It's more meaningful. > > Why a GList instead of a GArray or GPtrArray? Unless you need to > remove/insert elements in the middle/beginning, the latter is usually more > efficient in all regards. Actually qtdemux might need remove any streams in the middle or beginning. https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/isomp4/qtdemux.c#n12197 I thought that streams in a mp4 file is not bulky and therefore GList will not bring much overhead over GPtrArray. If we'd like to change to GPtrArray, I think it's not difficult though.
Can someone who worked on this can look at this crash -> https://bugzilla.gnome.org/show_bug.cgi?id=797239 We have a strong impression this is caused by work done for this issue.
(In reply to Nicolas Dufresne (ndufresne) from comment #94) > Can someone who worked on this can look at this crash -> > https://bugzilla.gnome.org/show_bug.cgi?id=797239 > > We have a strong impression this is caused by work done for this issue. I uploaded a patch in bug #797239. When I was working on this bug, MSS usecase was not fully tested, sorry :(