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)

initialize variables

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.

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.

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

:cajbir

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c1149744c29 all the intermittent oranges are now non-mse related

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).

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

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

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 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.

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

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

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

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

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.

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

:mattwoodrow

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.

(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 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.

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

add part#

:cajbir

:cajbir

:kinetik

:kinetik

add part#. Carrying r+

add part#

add part#

add part#

add part#

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 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 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 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.

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

Carrying r+, rebase on previous patch changes

Carrying r+

Carrying r+

rebase...

rebase

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.

missed a line in the previous rebase

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

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)

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 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

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.

Rebase.

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.

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 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 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 on attachment 8557764 [details] [diff] [review] Part1. Fix initialization of variables [Triage Comment]