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 795424 - qtdemux: Add MSE-style flush
qtdemux: Add MSE-style flush
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-04-20 22:27 UTC by Alicia Boya García
Modified: 2018-11-03 15:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: Add MSE-style flush (3.27 KB, patch)
2018-04-20 22:28 UTC, Alicia Boya García
none Details | Review
qtdemux tests: Add tests for MSE-style flush and fragments (23.63 KB, patch)
2018-04-20 22:40 UTC, Alicia Boya García
none Details | Review
qtdemux: Add MSE-style flush (6.05 KB, patch)
2018-04-24 18:53 UTC, Alicia Boya García
none Details | Review
qtdemux: Add MSE-style flush (6.60 KB, patch)
2018-05-21 18:00 UTC, Alicia Boya García
none Details | Review
qtdemux: Add MSE-style flush (3.83 KB, patch)
2018-09-21 23:33 UTC, Alicia Boya García
none Details | Review
qtdemux: Add MSE-style flush (2.76 KB, patch)
2018-10-28 10:25 UTC, Alicia Boya García
none Details | Review
qtdemux: Add MSE-style flush (2.82 KB, patch)
2018-10-28 10:34 UTC, Alicia Boya García
reviewed Details | Review

Description Alicia Boya García 2018-04-20 22:27:56 UTC
The abort() method of SourceBuffer in Media Source Extensions is
expected to flush the demuxer and discard the current fragment,
if any. The configuration of tracks, if any, should be preserved.

qtdemux already has an implementation of flush events geared towards
seeks which is not compatible with this use case.

For this reason, a new type of flush is added, identified by the
presence of a `media_source_extensions_flush` field in its FLUSH_STOP
structure that implements the behavior required by the MSE spec.
Comment 1 Alicia Boya García 2018-04-20 22:28:01 UTC
Created attachment 371183 [details] [review]
qtdemux: Add MSE-style flush
Comment 2 Alicia Boya García 2018-04-20 22:38:56 UTC
I've created some tests too to check it works properly. They are in a separate patch, which builds upon some of the tooling introduced in https://bugzilla.gnome.org/show_bug.cgi?id=794902.
Comment 3 Alicia Boya García 2018-04-20 22:40:04 UTC
Created attachment 371186 [details] [review]
qtdemux tests: Add tests for MSE-style flush and fragments
Comment 4 Sebastian Dröge (slomo) 2018-04-21 10:37:32 UTC
Why is the normal flushing not sufficient? You should flush on the sinkpad, not the srcpads. And qtdemux should then forget all data it has but not forget the headers it parsed (i.e. remember what tracks there are, the caps, etc.).
Comment 5 Alicia Boya García 2018-04-23 10:37:35 UTC
(In reply to Sebastian Dröge (slomo) from comment #4)
> Why is the normal flushing not sufficient?

Flushing as is currently implemented (let's call it "jump-style flush") in qtdemux is not designed to work by itself, but only in response to a seek. qtdemux receives a seek to a time position, finds the frame in the sample table, rewinds its internal pointers to its offset and emits a byte seek upstream to the offset of the first frame to be decoded after the seek. When upstream flushes, qtdemux knows it is reading a frame. Note even though this kind of flush technically comes from upstream, it's always a direct consequence of qtdemux itself performing a byte seek.

MSE-style flushing is completely different. Here upstream (actually, the application), by itself, decides to send the flush (there are no seek events involved in the process). qtdemux in response forgets the sample table (in the jump-style flush the table had to be preserved) and the demuxer sets up to read a new atom from the start, usually a moof box (in the jump-style flush qtdemux set up to read a frame somewhere *inside* an mdat).

> You should flush on the sinkpad, not the srcpads.

I know. Have I done otherwise anywhere?

> And qtdemux should then forget all data it has but not forget the headers it
> parsed (i.e. remember what tracks there are, the caps, etc.).

This was always my intention and it's totally the case with my patch.

There are even tests checking that. See test_qtdemux_fragments_flush_between_frags for instance. You can see how the initialization segment (bytes containing the headers -- the moov box) is only fed once:

+  qtdemux_fragment_tester_feed_and_wait (tester,
+      new_initialization_segment (dash_movie));

Then a fragment (moof+mdat) is fed, then flush is sent, then a new fragment. No need to send the headers again.

+  qtdemux_fragment_tester_feed_and_wait (tester,
+      new_fragment_buffer (dash_movie, 0));
+  [...]
+
+  qtdemux_fragment_tester_mse_flush (tester);
+
+  qtdemux_fragment_tester_feed_and_wait (tester,
+      new_fragment_buffer (dash_movie, 1));
Comment 6 Thibault Saunier 2018-04-23 12:52:51 UTC
(In reply to Alicia Boya García from comment #5)
> (In reply to Sebastian Dröge (slomo) from comment #4)
> > Why is the normal flushing not sufficient?
> 
> Flushing as is currently implemented (let's call it "jump-style flush") in
> qtdemux is not designed to work by itself, but only in response to a seek.
> qtdemux receives a seek to a time position, finds the frame in the sample
> table, rewinds its internal pointers to its offset and emits a byte seek
> upstream to the offset of the first frame to be decoded after the seek. When
> upstream flushes, qtdemux knows it is reading a frame. Note even though this
> kind of flush technically comes from upstream, it's always a direct
> consequence of qtdemux itself performing a byte seek.

To me this is the bug, flushing is not necessarily the consequence of a seek and elements can't react as if it was always the case. The fix is probably to check the seqnum of the flush-stop event, if it is not the same as the previous seek, you flush the way you do in that patch, otherwise you keep current behavior.
Comment 7 Sebastian Dröge (slomo) 2018-04-23 13:37:21 UTC
Yes I agree with Thibault here. The tricky part will be about keeping the sample tables or not. After a flush, the demuxer should probably check if the first buffer contains a moov/moof somehow for your case, and for the seek case (and also adaptivedemux keyframe-only mode) check if we're at a known sample boundary.

The demuxer should not depend on the flush coming from a seek or not, and it should also properly handle disconts in the stream (as signaled by the corresponding buffer flag).

Not sure how to implement this in a reliable way though.
Comment 8 Alicia Boya García 2018-04-24 18:53:41 UTC
Created attachment 371339 [details] [review]
qtdemux: Add MSE-style flush

The abort() method of SourceBuffer in Media Source Extensions is
expected to flush the demuxer and discard the current fragment,
if any. The configuration of tracks, if any, should be preserved.

Before this patch, qtdemux was only to able to handle correctly a flush
when it was a direct consequence of a byte seek that qtdemux itself sent
upstream in response to a time seek.

This patch modifies the handling of flush events in such a way that
qtdemux can differentiate between these flushes that are a consequence
of a seek and flushes coming from the application on its own (like in
MSE).

In the latter case, all unparsed buffered data is discarded, the sample
table is resetted and the demuxer prepares to read a new atom from the
beginning.
Comment 9 Alicia Boya García 2018-04-24 19:05:10 UTC
I've modified the patch to not need any special flags. The event seqnum is used to differentiate between both kinds of flushes.

It was necessary to make some minor additional changes because before offset_seek_seqnum was only set in qtdemux_seek_offset() -- when a moov atom needs to be jumped over, but not on response to time seeks.

Also, moov jumps don't require propagating the flush downstream whilst time seeks do. This difference is tracked with a new `propagate_flush` variable that should be set along with `offset_seek_seqnum`.
Comment 10 Thibault Saunier 2018-04-24 19:22:33 UTC
Review of attachment 371339 [details] [review]:

::: gst/isomp4/qtdemux.h
@@ -196,2 +196,5 @@
    * detect incoming FLUSH events corresponding to that */
   guint32 offset_seek_seqnum;
+  /* Whether the FLUSH events received in consequence of a seek
+   * sent upstream should be propagated downstream. */
+  gboolean propagate_flush;

Why don't you propagate other flushes?
Comment 11 Alicia Boya García 2018-04-24 20:34:25 UTC
The only flush not propagated is the one resulting from skipping the mdat at the beginning of the file (one after seeking after, then another one seeking before once we parsed the headers).

This is the behavior qtdemux has before, so I'm playing safe by keeping it.

A possible motivation for it is to avoid downstream discard events resulting from parsing the headers. Right out of my mind comes SEGMENT events, which are emitted when the headers are parsed; but I'm not sure if elements actually discard such data or if they are right to do so.
Comment 12 Alicia Boya García 2018-05-17 17:19:09 UTC
Please, could someone review this?
Comment 13 Thibault Saunier 2018-05-17 22:02:58 UTC
Review of attachment 371339 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +1526,3 @@
   gst_event_set_seqnum (event, seqnum);
+  qtdemux->offset_seek_seqnum = seqnum;
+  qtdemux->propagate_flush = TRUE;

This is the only place where it is set to TRUE, in the user flushing case, this might very well still be set to FALSE.

I think it should be set to TRUE in _init, and then maybe be reset in the  code path in FLUSH_STOP where it was set to FALSE.
Comment 14 Thibault Saunier 2018-05-17 22:02:59 UTC
Review of attachment 371339 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +1526,3 @@
   gst_event_set_seqnum (event, seqnum);
+  qtdemux->offset_seek_seqnum = seqnum;
+  qtdemux->propagate_flush = TRUE;

This is the only place where it is set to TRUE, in the user flushing case, this might very well still be set to FALSE.

I think it should be set to TRUE in _init, and then maybe be reset in the  code path in FLUSH_STOP where it was set to FALSE.
Comment 15 Thibault Saunier 2018-05-17 22:12:07 UTC
(In reply to Sebastian Dröge (slomo) from comment #7)
> Yes I agree with Thibault here. The tricky part will be about keeping the
> sample tables or not. After a flush, the demuxer should probably check if
> the first buffer contains a moov/moof somehow for your case, and for the
> seek case (and also adaptivedemux keyframe-only mode) check if we're at a
> known sample boundary.

This patch takes a simple approach where the sample table is flushed whenever we get a flush from the "app/user" it might not be always correct indeed, but I guess it won't break (people should not flush without knowing what they do :)), do you think it would?

> The demuxer should not depend on the flush coming from a seek or not, and it
> should also properly handle disconts in the stream (as signaled by the
> corresponding buffer flag).

Well, if you get a flushing seek, and transmit a flushing seek upstream, you can rely on the fact that you will get flushes. Or what you meant was related to making a distinction between a flush coming from a seek or coming from something else? I think it is not so unusual in practice.
Comment 16 Sebastian Dröge (slomo) 2018-05-18 07:24:40 UTC
(In reply to Thibault Saunier from comment #15)
> (In reply to Sebastian Dröge (slomo) from comment #7)
> > Yes I agree with Thibault here. The tricky part will be about keeping the
> > sample tables or not. After a flush, the demuxer should probably check if
> > the first buffer contains a moov/moof somehow for your case, and for the
> > seek case (and also adaptivedemux keyframe-only mode) check if we're at a
> > known sample boundary.
> 
> This patch takes a simple approach where the sample table is flushed
> whenever we get a flush from the "app/user" it might not be always correct
> indeed, but I guess it won't break (people should not flush without knowing
> what they do :)), do you think it would?

I could imagine apps/users wanting to flush while keeping the sample table too. IMHO you should re-use the existing logic that is already there for resyncing the adaptivedemux keyframe-only mode.

Make sure to look at buffer offsets, check for a new moov/moof, etc after the flush.

While you might not need that right now, let's better fix this properly from the beginning.

> > The demuxer should not depend on the flush coming from a seek or not, and it
> > should also properly handle disconts in the stream (as signaled by the
> > corresponding buffer flag).
> 
> Well, if you get a flushing seek, and transmit a flushing seek upstream, you
> can rely on the fact that you will get flushes. Or what you meant was
> related to making a distinction between a flush coming from a seek or coming
> from something else? I think it is not so unusual in practice.

It requires additional code for mapping seek and flushes together though. This seems unnecessary and potentially fragile (what if a flush arrives just between the seek and upstream flushing us?)
Comment 17 Thibault Saunier 2018-05-18 12:45:35 UTC
Review of attachment 371186 [details] [review]:

I stopped reviewing at that point, will come back to it after we discuss possible ways to simplify it as a whole.

::: tests/check/elements/qtdemux.c
@@ +30,3 @@
 #include <fcntl.h>
+#include <gst/app/app.h>
+#include <pthread.h>

Not required if you use GLib types

@@ +1297,3 @@
+  GstMemory *mem;
+  GstBuffer *buf;
+  mem = read_file_to_memory ("ibpibp-dash.mp4");

This seems to be leaked. Also that function doesn't exist anywhere yet, I know it comes from some other commit.

@@ +1305,3 @@
+typedef struct _Barrier
+{
+  pthread_cond_t cond;

You should use GLib mutexes and conds here.

@@ +1316,3 @@
+  int ret;
+  ret = pthread_mutex_init (&barrier->mutex, NULL);
+  g_assert (ret == 0);

Use libcheck assertion functions (fail_unless and friends) instead of GLib ones.

Reporting only here, but there are many occurrences of that.

@@ +1340,3 @@
+    pthread_cond_wait (&barrier->cond, &barrier->mutex);
+  }
+  if (barrier->errored) {

I would rather "assert" in the hanlder, and do nothin in that code path, maybe you could even remove that field all together then.

@@ +1346,3 @@
+  }
+
+  /* Reset the barrier so that it can be reused. */

Then reset `->error` too?

@@ +1371,3 @@
+static GstPadProbeReturn
+lift_barrier_on_empty_buffer (GstPad * pad, GstPadProbeInfo * info,
+    void *userdata)

I do not get where the empty buffer comes from.

@@ +1380,3 @@
+
+  buffer = info->data;
+  if (gst_buffer_n_memory (buffer) == 0) {

Would be more obvious to check `gst_buffer_get_size()` here I think.

@@ +1475,3 @@
+  g_printerr ("ERROR from element %s: %s\n",
+      GST_OBJECT_NAME (msg->src), err->message);
+  g_printerr ("Debugging info: %s\n", (dbg_info) ? dbg_info : "none");

You could used `g_error` instead.

@@ +1489,3 @@
+
+  appsrc_pad = gst_element_get_static_pad (GST_ELEMENT (tester->appsrc), "src");
+  g_assert_nonnull (appsrc_pad);

Unnecessary check.

@@ +1508,3 @@
+
+static void
+qtdemux_fragment_tester_mse_flush (QtDemuxFragmentTester * tester)

Those are just flush, please update using normal flushes in that function.
Comment 18 Alicia Boya García 2018-05-18 14:43:29 UTC
Review of attachment 371339 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +1526,3 @@
   gst_event_set_seqnum (event, seqnum);
+  qtdemux->offset_seek_seqnum = seqnum;
+  qtdemux->propagate_flush = TRUE;

> This is the only place where it is set to TRUE, in the user flushing case

Actually that's not the case. That line of code belongs to gst_qtdemux_do_push_seek(), which is for handling seeks.

As the comment in qtdemux.h explains, qtdemux->propagate_flush only applies to flushes that are a consequence of a seek. User caused flushes are always propagated downstream.

Let's review the three cases where qtdemux can receive a flush.

1) The demuxer receives a TIME SEEK event from downstream. The TIME SEEK event is consumed and gst_qtdemux_do_push_seek() is called. qtdemux finds what byte position in the file corresponds to the requested time position using the sample table and sends a BYTE SEEK upstream. Upstream rewinds the file and sends a flush downstream, followed by a BYTE segment and then the new data. qtdemux takes advantage of this to propagate the flush downstream so that GStreamer stops decoding the old frames.

2) The demuxer attempts to read a file where the mdat precedes the moov. At the beginning, the demuxer finds out about this and calls qtdemux_seek_offset() to skip the mdat (whose size is known). This sends a BYTE SEEK upstream too, which again causes a flush, segment and new data to arrive at qtdemux. In this case qtdemux does not have any need to propagate the flush downstream, so it doesn't. This is the case where propagate_flush = FALSE.

3) The user sends a flush on their own to the demuxer, not caused by any seek. This is the new use case introduced by this patch. Because of how the MP4 format is designed, this use case is only useful for fragmented media. In it, the fragment currently being parsed is discarded (this includes its sample table) and a new one is expected. [In lower level terms, the flush instructs the demuxer to start reading a MP4 element from the beginning (QTDEMUX_STATE_INITIAL), clears all buffered data and discards the sample table of each track; but header data obtained from the moov -- if any, remains.] In this case the flush is always propagated, regardless of the propagate_flush property.

gst_qtdemux_handle_sink_event is able to tell apart cases 1 and 2 from case 3 by looking at the seqnum of the flush event and comparing it to demux->offset_seek_seqnum.

Regarding case 2, I have some questions:

1. Would anything bad happen if I just propagated the flush downstream?
2. Couldn't we just remove the GST_SEEK_FLAG_FLUSH flag and just skip the flush completely? I don't think we are doing anything useful on response to it and we're not propagating it either.

I tried to play safe by keeping the current behavior, but I would not be against simplifying it out.

::: gst/isomp4/qtdemux.h
@@ -196,2 +196,5 @@
    * detect incoming FLUSH events corresponding to that */
   guint32 offset_seek_seqnum;
+  /* Whether the FLUSH events received in consequence of a seek
+   * sent upstream should be propagated downstream. */
+  gboolean propagate_flush;

Other flushes are always propagated. It could be better clarified.
Comment 19 Alicia Boya García 2018-05-18 15:34:06 UTC
(In reply to Sebastian Dröge (slomo) from comment #16)
> I could imagine apps/users wanting to flush while keeping the sample table
> too. IMHO you should re-use the existing logic that is already there for
> resyncing the adaptivedemux keyframe-only mode.
> 
> Make sure to look at buffer offsets, check for a new moov/moof, etc after
> the flush.
> 
> While you might not need that right now, let's better fix this properly from
> the beginning.

Can you provide an example of a test case where you would want to keep the sample table after a user-initiated flush? I can't think of any case. What would such a flush be supposed to reset?

> > > The demuxer should not depend on the flush coming from a seek or not, and it
> > > should also properly handle disconts in the stream (as signaled by the
> > > corresponding buffer flag).
> > 
> > Well, if you get a flushing seek, and transmit a flushing seek upstream, you
> > can rely on the fact that you will get flushes. Or what you meant was
> > related to making a distinction between a flush coming from a seek or coming
> > from something else? I think it is not so unusual in practice.
> 
> It requires additional code for mapping seek and flushes together though.
> This seems unnecessary and potentially fragile (what if a flush arrives just
> between the seek and upstream flushing us?)

That won't happen because the seek is synchronous. Indeed, this fact is used to reset demux->offset_seek_seqnum after the seek. (See qtdemux_seek_offset() and gst_qtdemux_do_push_seek())
Comment 20 Sebastian Dröge (slomo) 2018-05-18 15:42:35 UTC
(In reply to Alicia Boya García from comment #19)
> (In reply to Sebastian Dröge (slomo) from comment #16)
> > I could imagine apps/users wanting to flush while keeping the sample table
> > too. IMHO you should re-use the existing logic that is already there for
> > resyncing the adaptivedemux keyframe-only mode.
> > 
> > Make sure to look at buffer offsets, check for a new moov/moof, etc after
> > the flush.
> > 
> > While you might not need that right now, let's better fix this properly from
> > the beginning.
> 
> Can you provide an example of a test case where you would want to keep the
> sample table after a user-initiated flush? I can't think of any case. What
> would such a flush be supposed to reset?

It would flush everything downstream to directly start playback again from whatever next data comes. Similarly to how the adaptivedemux keyframe-only mode is currently implemented in qtdemux.

> > > > The demuxer should not depend on the flush coming from a seek or not, and it
> > > > should also properly handle disconts in the stream (as signaled by the
> > > > corresponding buffer flag).
> > > 
> > > Well, if you get a flushing seek, and transmit a flushing seek upstream, you
> > > can rely on the fact that you will get flushes. Or what you meant was
> > > related to making a distinction between a flush coming from a seek or coming
> > > from something else? I think it is not so unusual in practice.
> > 
> > It requires additional code for mapping seek and flushes together though.
> > This seems unnecessary and potentially fragile (what if a flush arrives just
> > between the seek and upstream flushing us?)
> 
> That won't happen because the seek is synchronous. Indeed, this fact is used
> to reset demux->offset_seek_seqnum after the seek. (See
> qtdemux_seek_offset() and gst_qtdemux_do_push_seek())

There's nothing preventing another flush-start from another thread, it's an OOB event.
Comment 21 Thibault Saunier 2018-05-18 16:03:30 UTC
> There's nothing preventing another flush-start from another thread, it's an OOB event.

It can't (should not at the very least) have the same seqnum.
Comment 22 Alicia Boya García 2018-05-18 16:10:33 UTC
(In reply to Sebastian Dröge (slomo) from comment #20)
> > > It requires additional code for mapping seek and flushes together though.
> > > This seems unnecessary and potentially fragile (what if a flush arrives just
> > > between the seek and upstream flushing us?)
> > 
> > That won't happen because the seek is synchronous. Indeed, this fact is used
> > to reset demux->offset_seek_seqnum after the seek. (See
> > qtdemux_seek_offset() and gst_qtdemux_do_push_seek())
> 
> There's nothing preventing another flush-start from another thread, it's an
> OOB event.

In that case gst_pad_push_event() would return FALSE before the FLUSH event is received by qtdemux.
Comment 23 Alicia Boya García 2018-05-18 20:05:16 UTC
(In reply to Sebastian Dröge (slomo) from comment #20)
> It would flush everything downstream to directly start playback again from
> whatever next data comes. Similarly to how the adaptivedemux keyframe-only
> mode is currently implemented in qtdemux.

If I understood correctly, this potential adaptivedemux-style flush would propagate downstream and then accept a byte SEGMENT event from upstream with a potentially different offset, e.g. pointing to a different frame. The flush itself would probably do nothing except being propagated, and most of the logic would be in the SEGMENT event handling.

How would you want to tell apart these two potential types of flushes? When in my first patch I tagged the flush event structure naming it "media_source_extensions_flush", I was doing it in case in the future the need for a different kind of flush arose. Should I bring that again? And in that case, what should an untagged flush event do?
Comment 24 Alicia Boya García 2018-05-18 22:00:45 UTC
Review of attachment 371186 [details] [review]:

::: tests/check/elements/qtdemux.c
@@ +1493,3 @@
+      lift_barrier_on_empty_buffer, &tester->barrier, NULL);
+
+  if (gst_buffer_n_memory (buffer) == 0) {

> I do not get where the empty buffer comes from.

Here it comes. It serves to notice when the previous buffer has been demuxed.
Comment 25 Edward Hervey 2018-05-19 08:49:37 UTC
Are this patches using the new refactoring that landed recently in master qtdemux ? They should make sure use-cases easier to handle.
Comment 26 Alicia Boya García 2018-05-19 15:37:13 UTC
(In reply to Edward Hervey from comment #25)
> Are this patches using the new refactoring that landed recently in master
> qtdemux ? They should make sure use-cases easier to handle.

What refactoring is that (e.g. commit hash) and how would it make these use-cases easier to handle?
Comment 27 Edward Hervey 2018-05-20 05:21:12 UTC
It's the commits from https://bugzilla.gnome.org/show_bug.cgi?id=684790 , which went in 10 days ago.
Comment 28 Alicia Boya García 2018-05-21 11:05:24 UTC
(In reply to Edward Hervey from comment #25)
> Are this patches using the new refactoring that landed recently in master
> qtdemux ? They should make sure use-cases easier to handle.

It's not, but it can be rebased easily, the only conflict is the move to GList for qtdemux->streams.

It shouldn't make this use-case any easier or harder, because when you do one of these flushes you are usually interrupting the parsing of a fragment and starting to read a new one; but both fragments are usually from the same movie and a moov box does not need to appear (it can, but it's redundant).
Comment 29 Alicia Boya García 2018-05-21 18:00:00 UTC
Created attachment 372315 [details] [review]
qtdemux: Add MSE-style flush

The abort() method of SourceBuffer in Media Source Extensions is
expected to flush the demuxer and discard the current fragment,
if any. The configuration of tracks, if any, should be preserved.

Before this patch, qtdemux was only to able to handle correctly a flush
when it was a direct consequence of a byte seek that qtdemux itself sent
upstream in response to a time seek.

This patch modifies the handling of flush events in such a way that
qtdemux can differentiate between these flushes that are a consequence
of a seek and flushes coming from the application on its own (like in
MSE).

In the latter case, all unparsed buffered data is discarded, the sample
table is resetted and the demuxer prepares to read a new atom from the
beginning.
Comment 30 Alicia Boya García 2018-05-21 18:05:00 UTC
I have rebased the patch for the latest master.

I've also renamed offset_seek_seqnum to seek_caused_flush_seqnum and should_propagate to seek_caused_flush_should_propagate, which despite being longer names may alleviate confusion over their purpose.

Sebastian, what should we do regarding the sample table reset concern?

I need to reset the sample table, so if you foreshadow the need for a different type of user caused flush that does not reset the sample table, we should use some kind of tagging to accomodate that (like I did in the first version of my patch with GstStructure). Either way, there has to be a default behavior in absence of tagging, and I think this kind of flush makes sense for that.
Comment 31 Alicia Boya García 2018-09-21 23:16:55 UTC
It has been a while without any activity in this bug, so first I'll try to do a recap on the current behavior of qtdemux.

Currently qtdemux may receive a flush in the following scenarios:

* Case 1: qtdemux receives a seek or a flush from user: when it arrives it soft-resets. The flush propagates.

* Case 2: qtdemux makes a seek by itself to skip an mdat that appears before moov: qtdemux expects a flush; when it arrives it soft-resets. The flush does not propagate. (Not that it could, as at this point there are still no streams.)

In BYTES mode, the soft-reset consists on clearing the GstAdapter (e.g. if qtdemux was in the middle of parsing a frame, the half-frame is discarded), clearing the seek segment (i.e. if the user did a seek to set start/stop times or a playback rate, it's reset back to default values) and resetting the flow combiner (i.e. if a stream was in EOS, it no longer is).

TIME mode is a bit different in that seeks from the user are forwarded directly to the source element and not handled by qtdemux. qtdemux will still receive a flush from the source, and like before it will soft-reset and propagate the flush.

In TIME mode, however soft-resets actually reset more stuff: pretty much everything related to the overall movie is resetted except for the streams. The sample tables of the streams are preserved, except in mss_mode, where they are cleared.

----

After careful consideration, I've decided that it's best not to alter the default behavior of flushes. Sebastian mentioned "adaptivedemux keyframe-only mode" as an example of a case where an intelligent source may want to feed qtdemux with specific chunks of the file to emit very specific frames. They may still want to flush downstream at some point without losing the sample tables.

Therefore, I'm modifying the patch back so that it only reacts if the flush contains a specific GstStructure field. In absence of that field, the code path remains virtually identical.
Comment 32 Alicia Boya García 2018-09-21 23:33:44 UTC
Created attachment 373736 [details] [review]
qtdemux: Add MSE-style flush

The abort() method of SourceBuffer in Media Source Extensions is
expected to flush the demuxer and discard the current fragment,
if any. The configuration of tracks, if any, should be preserved.

qtdemux already has an implementation of flush events geared towards
seeks which is not compatible with this use case.

For this reason, a new type of flush is added, identified by the
presence of a `media_source_extensions_flush` field in its FLUSH_STOP
structure that implements the behavior required by the MSE spec.
Comment 33 Sebastian Dröge (slomo) 2018-09-25 10:06:25 UTC
So in relation to the implemented flushing behaviours in comment 31, what do you need to happen exactly and at what points does it differ/conflict with what is implemented already? You are in TIME mode but want to only flush out the mdat related pieces, keeping headers/etc all parsed and afterwards expecting data from a specific offset into the mdat?


The MSE case does not seem that special to me conceptually, so I'd rather think about it again another time how to add this behaviour in a general way.
Comment 34 Sebastian Dröge (slomo) 2018-09-25 10:27:21 UTC
(In reply to Sebastian Dröge (slomo) from comment #33)
> You are in TIME mode but want to only flush out
> the mdat related pieces, keeping headers/etc all parsed and afterwards
> expecting data from a specific offset into the mdat?

Or rather, flush out mdat and moof related parts but not moov? I.e. sample tables, actual samples we currently might have, etc... and then afterwards you expect to start with a moof?
Comment 35 Alicia Boya García 2018-09-26 18:29:08 UTC
(In reply to Sebastian Dröge (slomo) from comment #33)
> So in relation to the implemented flushing behaviours in comment 31, what do
> you need to happen exactly and at what points does it differ/conflict with
> what is implemented already? You are in TIME mode but want to only flush out
> the mdat related pieces, keeping headers/etc all parsed and afterwards
> expecting data from a specific offset into the mdat?

Unlike adaptivedemux, MSE uses BYTES segment, not TIME.

Also unlike adaptivedemux, MSE can not know beforehand the start time of the segments it feeds to the demuxer. This makes TIME mode inappropriate for MSE. For starters, using TIME in MSE would stop using tfdt values.
Comment 36 Alicia Boya García 2018-09-26 18:37:14 UTC
(In reply to Sebastian Dröge (slomo) from comment #34)
> Or rather, flush out mdat and moof related parts but not moov? I.e. sample
> tables, actual samples we currently might have, etc... and then afterwards
> you expect to start with a moof?

Exactly.

A typical use case is when an application is feeding pieces of a certain fragment to qtdemux. At some point, the user seeks to a far away unbuffered area; then the application does not want to bother downloading and feeding the rest of the old fragment to qtdemux: as soon as it has downloaded a piece of the new fragment it will just .abort() (i.e. tell the demuxer "I don't care about that old fragment anymore") and feed the new fragment.
Comment 37 Alicia Boya García 2018-10-28 10:25:07 UTC
Created attachment 374073 [details] [review]
qtdemux: Add MSE-style flush

The abort() method of SourceBuffer in Media Source Extensions is
expected to flush the demuxer and discard the current fragment,
if any. The configuration of tracks, if any, should be preserved.

qtdemux has different bahavior for flush events depending on the
context.

This patch introduces a `variant: mse-bytestream` caps setting that
allows the user to declare the data being feed to the demuxer is a MSE
ISO BMFF Bytestream, which makes use of a similar flush behavior than
adaptivedemux sources.
Comment 38 Alicia Boya García 2018-10-28 10:34:19 UTC
Created attachment 374074 [details] [review]
qtdemux: Add MSE-style flush

The abort() method of SourceBuffer in Media Source Extensions is
expected to flush the demuxer and discard the current fragment,
if any. The configuration of tracks, if any, should be preserved.

qtdemux has different bahavior for flush events depending on the
context.

This patch introduces a `variant: mse-bytestream` caps setting that
allows the user to declare the data being feed to the demuxer is a MSE
ISO BMFF Bytestream [1], which makes use of a similar flush behavior than
adaptivedemux sources.

[1] https://www.w3.org/TR/mse-byte-stream-format-isobmff/
Comment 39 Sebastian Dröge (slomo) 2018-10-28 10:36:00 UTC
Comment on attachment 374074 [details] [review]
qtdemux: Add MSE-style flush

Looks fine to me, please ping me once you tested this in WebKit and then let's get it in :)
Comment 40 GStreamer system administrator 2018-11-03 15:29:26 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/467.