1125776 - updateend is fired before decoder is being initialized and errors aren't reported to the parent source buffer.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

MediaSource's duration attribute isn't always updated before the update/updateend event is fired.

Normally, when you call SourceBuffer.appendBuffer() with an init segment, the metadata should be read and the mediasource duration updated, and only then update/updatend event be fired.

However, currently the metadata is being read by the MediaDecoderStateMachine in a concurrent thread. This happens in parallel, and mediasource's duration will be initialized in its own time.
The behaviour is as such racy.

When mediasource duration hasn't yet been initialized (it's NaN when MediaSource element is created), and an init segment has been loaded, appendBuffer should wait for the metadata to be read by the MDSM and only then fire update/updateend event.

This is the last task at this stage that cause Youtube test suite 23 and 24 to fail (bug 1116645)

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Fix initialization of variables

initialize variables

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: appendBuffer scanning the data before firing updateend

Make our appendBuffer more in-line with W3C segment parser loop algorithm. We ensure we do not fire updateend until TrackBuffer::AppendData has finished its job and initialize the decoder. Metadata are read within appendBuffer and set the mediasource duration only if it's NaN (we used to set as max(audio.duration,video.duration) and only the first init segment parsed should ever set the mediasource duration. Duration is then updated as per buffered data is being added.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Add support for partial init segment

Add partial initialization segment support. As appendBuffer initialize the newly created decoders before firing updateend, we don't want to call readmetadata on an incomplete init segment as it would otherwise error. We need to detect if an init segment is incomplete and queue it for later initialization.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Update webref tests

Update web-ref tests. We only fail those tests due to small mismatch in the media duration calculation (bug 1065207). The only one I can't explain is why the H264 Windows duration test timeout. I should get a fail. At least its now always consistent

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Update webref tests

:cajbir

Comment 6

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c1149744c29

all the intermittent oranges are now non-mse related

Comment 7

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7cad14f0af

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Add minimal init segment detection to WebMBufferedParser

the libnestegg solution turned out to not be appropriate under all circumstances. It will choke if any of the data provided is invalid. So if you add an init segment plus half a media segment, nestegg_init will return an error.
So I modified WebMBufferedParser to find the Tracks element and record its end offset ; we assume this is always the end of the init segment.
We will fail further down the track as WebMReader::ReadMetadata will also fail. We should limit the size of the data provided to nestegg_init to the size of the init segment just found. I can wrap this patch and the previous one together. if you want (probbly better that way too).

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Add support for partial init segment

Simplify handling of pending decoders as there really can only be one: mCurrentDecoder

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Limit the data parsed to the size of the init segment

limit webm demuxer on the init segment size when reading the metadata.

Comment 11

[Jean-Yves Avenard [:jya]]

Page to test:
http://people.mozilla.org/~jyavenard/tests/mse_webm/init-only.html?chunks=1&eos_chunk=10&eos_delay=-1&delay_chunk=0&start=-1&size=237

The init segment is 236 bytes here. Feeding 237 bytes would cause readmetadata to fail, and playback wouldn't start and decoder initialization would fail.

we now handle this condition just fine (so long as the rest of the media segments are complete)

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8557492 [details] [diff] [review]
Limit the data parsed to the size of the init segment

After sleeping on it, I don't like this approach. It breaks the abstraction of the MediaSource component.

The WebMReader has access to that information already as it has parsed the data using WebMBufferedParser already.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Limit metadata parsing to init segment size if known

Use init segment size if known and limit parser to that size.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: We don't always need an init segment to be ready

The MediaSourceReader doesn't need to have an init segment available anymore, as readmetadata is done within the trackbuffer.

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: Only notify reader we have data, when we actually do

Don't unecessarily tell the MediaSourceReader we have data when we don't.

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: Add support for partial WebM init segment

Combine the two patches together as the nestegg approach just doesn't work under all circumstances.

Comment 17

[cajbir (:cajbir)]

Comment on attachment 8557459 [details] [diff] [review]
Update webref tests

Review of attachment 8557459 [details] [diff] [review]:
-----------------------------------------------------------------

I defer to Karl for web platform test changes.

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: Disable test timing out on Windows {8,7}

Disable a test that timesout on Windows {7,8}. The loadeddata event is never fired. I'll keep tracking that one

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: Always attempt to create a new decoder if we don't have one

:mattwoodrow

Comment 20

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d674f8df98b

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8557459 [details] [diff] [review]
Update webref tests

>   [Test setting same duration multiple times does not fire duplicate durationchange]
>-    disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1116007
>+    expected: FAIL
> 
>diff --git a/testing/web-platform/tests/media-source/mediasource-duration.html b/testing/web-platform/tests/media-source/mediasource-duration.html
>--- a/testing/web-platform/tests/media-source/mediasource-duration.html
>+++ b/testing/web-platform/tests/media-source/mediasource-duration.html
>@@ -206,12 +206,12 @@
>                   test.expectEvent(mediaElement, 'ended', 'Playback ended');
>                   test.waitForExpectedEvents(function()
>                   {
>                       mediaElement.removeEventListener('durationchange', durationchangeEventHandler);
>                       assert_equals(durationchangeEventCounter, expectedDurationChangeEventCount, 'durationchanges');
>                       test.done();
>                   });
>               });
>-          }, 'Test setting same duration multiple times does not fire duplicate durationchange', {timeout: 2500});
>+          }, 'Test setting same duration multiple times does not fire duplicate durationchange', {timeout: 10000});

We're not meant to change files in the tests directory as these are imported from upstream and so will be overridden.  I suggest marking this subtest and the parent TIMEOUT.  If it is intermittent, then we'll need to disable the whole test file unfortunately.  r+ on the all the rest.

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (back 2 Feb:karlt) from comment #21)
> We're not meant to change files in the tests directory as these are imported
> from upstream and so will be [...]

overwritten.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8557707 [details] [diff] [review]
Disable test timing out on Windows {8,7}

I don't know what happened to loadeddata, but the annotations are appropriate for what you describe.

Perhaps the missing loaddata is responsible for the timeout in mediasource-duration.html - I haven't looked closely.

Comment 24

[Jean-Yves Avenard [:jya]]

Attachment: Add support for partial init segment

somehow we ended up with two, and I don't know which is which. So re-uploading

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Fix initialization of variables

add part#

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: Part2. appendBuffer scanning the data before firing updateend

:cajbir

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add support for partial init segment

:cajbir

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Add support for partial WebM init segment

:kinetik

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Limit metadata parsing to init segment size if known

:kinetik

Comment 30

[Jean-Yves Avenard [:jya]]

Attachment: Part6. Update webref tests

add part#. Carrying r+

Comment 31

[Jean-Yves Avenard [:jya]]

Attachment: Part7. Only notify reader we have data, when we actually do

add part#

Comment 32

[Jean-Yves Avenard [:jya]]

Attachment: Part8. We don't always need an init segment to be ready

add part#

Comment 33

[Jean-Yves Avenard [:jya]]

Attachment: Part8. Disable test timing out on Windows {8,7}

add part#

Comment 34

[Jean-Yves Avenard [:jya]]

Attachment: Part9. Always attempt to create a new decoder if we don't have one

add part#

Comment 35

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8557767 [details] [diff] [review]
Part2. appendBuffer scanning the data before firing updateend

Review of attachment 8557767 [details] [diff] [review]:
-----------------------------------------------------------------

Looks awesome so far, mainly just want to see the promise bit changed.

::: dom/media/mediasource/TrackBuffer.cpp
@@ +550,5 @@
>    mParentDecoder->GetReader()->MaybeNotifyHaveData();
>  
> +  MSE_DEBUG("TrackBuffer(%p): Reader %p activated",
> +            this, aDecoder->GetReader());
> +  mInitializationPromise.ResolveIfExists(aDecoder->GetRealMediaDuration(), __func__);

This promise expects a bool, not a duration.

Use aDecoder->GetRealMediaDuration() != 0 to make the conversion explicit if that's what you meant :)

::: dom/media/mediasource/TrackBuffer.h
@@ +43,5 @@
>    // NotifyDataArrived on the decoder to keep buffered range computation up
>    // to date.  Returns false if the append failed.
> +  bool AppendData(LargeDataBuffer* aData,
> +                  int64_t aTimestampOffset /* microseconds */,
> +                  bool& aGotMedia, nsRefPtr<InitializationPromise>& aPromise);

Lets switch to unconditionally returning the promise, and drop aGotMedia and the bool retval (as discussed on irc).

Comment 36

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8557770 [details] [diff] [review]
Part3. Add support for partial init segment

Review of attachment 8557770 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/TrackBuffer.cpp
@@ +180,5 @@
> +
> +  if (newInitData) {
> +    if (!gotInit) {
> +      // We need a new decoder, but we can't initialize it yet.
> +      nsRefPtr<SourceBufferDecoder> decoder = NewDecoder(aTimestampOffset);

Comment that the new decoder is stored in mDecoders/mCurrentDecoder, so we don't need to do anything with 'decoder'.

@@ +420,2 @@
>      nsRefPtr<dom::TimeRanges> r = new dom::TimeRanges();
> +    mInitializedDecoders[i]->GetBuffered(r);

This change probably should be part of the previous patch :)

@@ +509,5 @@
> +  // As this decoder hasn't been initialized yet, the resource isn't yet in use.
> +  bool wasEnded = aDecoder->GetResource()->IsEnded();
> +  if (!wasEnded) {
> +    aDecoder->GetResource()->Ended();
> +  }

This is a horrible hack. It would be nice if we could just pass a parameter to ReadMetadata to ask it to be non-blocking, but I guess that's quite a hassle to hook up.

Comment 37

[Matthew Gregan [:kinetik]]

Comment on attachment 8557772 [details] [diff] [review]
Part4. Add support for partial WebM init segment

Review of attachment 8557772 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webm/WebMBufferedParser.h
@@ +94,5 @@
>    // Current offset with the stream.  Updated in chunks as Append() consumes
>    // data.
>    int64_t mCurrentOffset;
>  
> +  // Tracks elelement's end offset. This indicates the end of the init segment.

Typo in element

@@ +96,5 @@
>    int64_t mCurrentOffset;
>  
> +  // Tracks elelement's end offset. This indicates the end of the init segment.
> +  // Will only be set if a Segment Information has been found.
> +  uint32_t mInitEndOffset;

Make this int64_t since it's an offset.  Then, if you init it to -1, GetInitEndOffset can drop the !mInitEndOffset test and return the value when mRangeParsers isn't empty.

Comment 38

[Matthew Gregan [:kinetik]]

Comment on attachment 8557774 [details] [diff] [review]
Part5. Limit metadata parsing to init segment size if known

Review of attachment 8557774 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webm/WebMBufferedParser.h
@@ +241,5 @@
>    // dependencies to arrive at aTime.
>    bool GetOffsetForTime(uint64_t aTime, int64_t* aOffset);
>  
> +  // Returns end offset of init segment or -1 if none found.
> +  int32_t GetInitEndOffset();

Being an offset, make it int64_t.

Comment 39

[Jean-Yves Avenard [:jya]]

Attachment: Part2. appendBuffer scanning the data before firing updateend

Carrying r+ comments. Always return a promise. Much more elegant.

Comment 40

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add support for partial init segment

Carrying r+, rebase on previous patch changes

Comment 41

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Add support for partial WebM init segment

Carrying r+

Comment 42

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Limit metadata parsing to init segment size if known

Carrying r+

Comment 43

[Jean-Yves Avenard [:jya]]

Attachment: Part7. Only notify reader we have data, when we actually do

rebase...

Comment 44

[Jean-Yves Avenard [:jya]]

Attachment: Part9. Always attempt to create a new decoder if we don't have one

rebase

Comment 45

[Jean-Yves Avenard [:jya]]

Attachment: Part10. Handle concurrent aborts

Handle successive appendBuffer that could have been interrupted half-way through.
A test case would have been something like:
sb.appendBuffer()
sb.OnEvent(updatestarting) {
  sb.abort();
  sb.appendBuffer()
}

The first appendBuffer would have queued the task and set sb.updating to true
on abort(), sb.updating is set to false. When the second sb.appendBuffer() runs : it would set sb.updating to true.
Now that the first queued task runs, sees that sb.updating is true and happily continue. Then the second append buffer queued would run and queue a second initialization task. And boom.

This situation could happen with YouTube when you seek, it starts buffering and immediately change resolution (which calls abort).

We add a small update ID that we record at the time we queue the buffer append task. And if upon execution the two update IDs do not match, we abort.

Comment 46

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add support for partial init segment

missed a line in the previous rebase

Comment 47

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Add support for partial WebM init segment

made a mistake when I carried the r+ comments, and now that mInitEndOffset has a default value of -1, we need to test for positive, not non-zero

Comment 48

[Jean-Yves Avenard [:jya]]

Attachment: Part10. Handle concurrent aborts

Can't reject the promise in shutdown. It is possible that the SourceBuffer got deleted while shutdown is running ; that would cause the promise to still hold a reference to the SourceBuffer and it would be deleted outside the main thread (causing a crash as SourceBuffer not being thread-safe)

Comment 49

[Jean-Yves Avenard [:jya]]

Attachment: Fix potential crash

Got that race condition once. Shutdown completed right before the initialization task got to run. mParentDecoder was set to null, and attempting to get the lock cause a null dereference: [39007] ###!!! ASSERTION: You can't dereference a NULL nsRefPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/nsRefPtr.h, line 229. This is not the perfect solution, as getting the lock isn't atomic here. Ideally, we should use our own monitor when it comes to shutdown/init thread rather than using the parent decoder one. The likelihood of still happening however is super low.

Comment 50

[Jean-Yves Avenard [:jya]]

Comment on attachment 8558437 [details] [diff] [review]
Part10. Handle concurrent aborts

One of the issue this patch tried to get around has been fixed in bug 1129224. So writing a new version

Comment 51

[Jean-Yves Avenard [:jya]]

Attachment: Part10. Handle concurrent aborts

Reworked following resolution of bug 1129224. We promise must be resolved, even if disconnected. Also rename some methods to make them more explicit in what they actually do.

Comment 52

[Jean-Yves Avenard [:jya]]

Attachment: Part11. Fix potential crash

Rebase.

Comment 53

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8558897 [details] [diff] [review]
Part10. Handle concurrent aborts

Review of attachment 8558897 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/SourceBuffer.h
@@ +32,5 @@
>  class ErrorResult;
>  class LargeDataBuffer;
>  class TrackBuffer;
>  template <typename T> class AsyncEventRunner;
> +typedef MediaPromise<bool, nsresult, /* IsExclusive = */ true> TrackBufferInitializationPromise;

TrackBufferAppendPromise might be more accurate, now that we always return one.

Comment 54

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=15a0aaae5dd7

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e16b5300cd42
https://hg.mozilla.org/mozilla-central/rev/52dc02feb987
https://hg.mozilla.org/mozilla-central/rev/71dd88a5ad5f
https://hg.mozilla.org/mozilla-central/rev/f51f8fcad8e6
https://hg.mozilla.org/mozilla-central/rev/77dec541fd06
https://hg.mozilla.org/mozilla-central/rev/d2c9755aaee2
https://hg.mozilla.org/mozilla-central/rev/a9f556f08487
https://hg.mozilla.org/mozilla-central/rev/e05a56284a00
https://hg.mozilla.org/mozilla-central/rev/3c4dc1b74b7f
https://hg.mozilla.org/mozilla-central/rev/e1128d8612ff
https://hg.mozilla.org/mozilla-central/rev/15a0aaae5dd7

Comment 57

[Ralph Giles (:rillian)]

Comment on attachment 8557838 [details] [diff] [review]
Part2. appendBuffer scanning the data before firing updateend

Requesting 37 uplift of all patches on this bug.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Dependency of later critical fixes. Possible video playback stalls with YouTube. Less consistent testing.
[Describe test coverage new/current, TreeHerder]: Landed on m-c two weeks ago.
[Risks and why]: This is a significant rewrite code specific to the MSE feature. it's been working fine on nightly.
[String/UUID change made/needed]: None.

Comment 58

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8557838 [details] [diff] [review]
Part2. appendBuffer scanning the data before firing updateend

Dependency for other MSE work. Been on m-c for ~2 weeks already without issue. Aurora+

Comment 59

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8557764 [details] [diff] [review]
Part1. Fix initialization of variables

[Triage Comment]

Comment 60

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=d2675abc9228