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

RESOLVED FIXED in Firefox 37

Status

()

P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla38
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 unaffected, firefox37 fixed, firefox38 fixed)

Details

Attachments

(12 attachments, 26 obsolete attachments)

1.26 KB, patch
Details | Diff | Splinter Review
3.44 KB, patch
Details | Diff | Splinter Review
1.30 KB, patch
Details | Diff | Splinter Review
1.32 KB, patch
Details | Diff | Splinter Review
21.58 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.92 KB, patch
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
2.04 KB, patch
Details | Diff | Splinter Review
15.01 KB, patch
Details | Diff | Splinter Review
10.55 KB, patch
Details | Diff | Splinter Review
23.44 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.09 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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)
(Assignee)

Updated

4 years ago
Blocks: 1106776

Updated

4 years ago
Priority: -- → P2
(Assignee)

Updated

4 years ago
Blocks: 1127414
(Assignee)

Comment 1

4 years ago
Created attachment 8556763 [details] [diff] [review]
Fix initialization of variables

initialize variables
Attachment #8556763 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Blocks: 1127674

Updated

4 years ago
Attachment #8556763 - Flags: review?(cajbir.bugzilla) → review+
(Assignee)

Updated

4 years ago
Depends on: 1127775
(Assignee)

Updated

4 years ago
Blocks: 1092276
(Assignee)

Updated

4 years ago
Blocks: 1085247
(Assignee)

Comment 2

4 years ago
Created attachment 8557420 [details] [diff] [review]
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.
Attachment #8557420 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 3

4 years ago
Created attachment 8557423 [details] [diff] [review]
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.
Attachment #8557423 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 4

4 years ago
Created attachment 8557427 [details] [diff] [review]
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
Attachment #8557427 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Blocks: 1096089
(Assignee)

Comment 5

4 years ago
Created attachment 8557459 [details] [diff] [review]
Update webref tests

:cajbir
(Assignee)

Updated

4 years ago
Attachment #8557427 - Attachment is obsolete: true
Attachment #8557427 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8557459 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 6

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

all the intermittent oranges are now non-mse related
(Assignee)

Comment 8

4 years ago
Created attachment 8557478 [details] [diff] [review]
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).
Attachment #8557478 - Flags: review?(kinetik)
(Assignee)

Comment 9

4 years ago
Created attachment 8557491 [details] [diff] [review]
Add support for partial init segment

Simplify handling of pending decoders as there really can only be one: mCurrentDecoder
Attachment #8557491 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 10

4 years ago
Created attachment 8557492 [details] [diff] [review]
Limit the data parsed to the size of the init segment

limit webm demuxer on the init segment size when reading the metadata.
Attachment #8557492 - Flags: review?(kinetik)
(Assignee)

Comment 11

4 years ago
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)
(Assignee)

Comment 12

4 years ago
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.
Attachment #8557492 - Attachment is obsolete: true
Attachment #8557492 - Flags: review?(kinetik)
(Assignee)

Comment 13

4 years ago
Created attachment 8557546 [details] [diff] [review]
Limit metadata parsing to init segment size if known

Use init segment size if known and limit parser to that size.
Attachment #8557546 - Flags: review?(kinetik)
(Assignee)

Comment 14

4 years ago
Created attachment 8557603 [details] [diff] [review]
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.
Attachment #8557603 - Flags: review?(matt.woodrow)
(Assignee)

Comment 15

4 years ago
Created attachment 8557604 [details] [diff] [review]
Only notify reader we have data, when we actually do

Don't unecessarily tell the MediaSourceReader we have data when we don't.
Attachment #8557604 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Blocks: 1128332
Attachment #8557603 - Flags: review?(matt.woodrow) → review+
Attachment #8557604 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8557686 [details] [diff] [review]
Add support for partial WebM init segment

Combine the two patches together as the nestegg approach just doesn't work under all circumstances.
Attachment #8557686 - Flags: review?(kinetik)
(Assignee)

Updated

4 years ago
Attachment #8557478 - Attachment is obsolete: true
Attachment #8557478 - Flags: review?(kinetik)

Comment 17

4 years ago
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.
Attachment #8557459 - Flags: review?(cajbir.bugzilla) → review?(karlt)
(Assignee)

Updated

4 years ago
Summary: mediasource.duration isn't updated before updateend event fired → updateend is fired before decoder is being initialized and errors aren't reported to the parent source buffer.
(Assignee)

Comment 18

4 years ago
Created attachment 8557707 [details] [diff] [review]
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
Attachment #8557707 - Flags: review?(karlt)
(Assignee)

Comment 19

4 years ago
Created attachment 8557721 [details] [diff] [review]
Always attempt to create a new decoder if we don't have one

:mattwoodrow
(Assignee)

Updated

4 years ago
Attachment #8557721 - Flags: review?(matt.woodrow)
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.
Attachment #8557459 - Flags: review?(karlt) → review+
(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.
Attachment #8557707 - Flags: review?(karlt) → review+
(Assignee)

Updated

4 years ago
Attachment #8557423 - Attachment is obsolete: true
Attachment #8557423 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8557491 - Attachment is obsolete: true
Attachment #8557491 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 24

4 years ago
Created attachment 8557762 [details] [diff] [review]
Add support for partial init segment

somehow we ended up with two, and I don't know which is which. So re-uploading
Attachment #8557762 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 25

4 years ago
Created attachment 8557764 [details] [diff] [review]
Part1. Fix initialization of variables

add part#
(Assignee)

Updated

4 years ago
Attachment #8556763 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Created attachment 8557767 [details] [diff] [review]
Part2. appendBuffer scanning the data before firing updateend

:cajbir
(Assignee)

Updated

4 years ago
Attachment #8557420 - Attachment is obsolete: true
Attachment #8557420 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 27

4 years ago
Created attachment 8557770 [details] [diff] [review]
Part3. Add support for partial init segment

:cajbir
(Assignee)

Updated

4 years ago
Attachment #8557762 - Attachment is obsolete: true
Attachment #8557762 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 28

4 years ago
Created attachment 8557772 [details] [diff] [review]
Part4. Add support for partial WebM init segment

:kinetik
(Assignee)

Updated

4 years ago
Attachment #8557686 - Attachment is obsolete: true
Attachment #8557686 - Flags: review?(kinetik)
(Assignee)

Comment 29

4 years ago
Created attachment 8557774 [details] [diff] [review]
Part5. Limit metadata parsing to init segment size if known

:kinetik
(Assignee)

Updated

4 years ago
Attachment #8557546 - Attachment is obsolete: true
Attachment #8557546 - Flags: review?(kinetik)
(Assignee)

Updated

4 years ago
Attachment #8557767 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8557770 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8557772 - Flags: review?(kinetik)
(Assignee)

Updated

4 years ago
Attachment #8557774 - Flags: review?(kinetik)
(Assignee)

Comment 30

4 years ago
Created attachment 8557779 [details] [diff] [review]
Part6. Update webref tests

add part#. Carrying r+
(Assignee)

Updated

4 years ago
Attachment #8557459 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
Created attachment 8557784 [details] [diff] [review]
Part7. Only notify reader we have data, when we actually do

add part#
(Assignee)

Updated

4 years ago
Attachment #8557604 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Created attachment 8557785 [details] [diff] [review]
Part8. We don't always need an init segment to be ready

add part#
(Assignee)

Updated

4 years ago
Attachment #8557603 - Attachment is obsolete: true
(Assignee)

Comment 33

4 years ago
Created attachment 8557786 [details] [diff] [review]
Part8. Disable test timing out on Windows {8,7}

add part#
(Assignee)

Updated

4 years ago
Attachment #8557707 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
Created attachment 8557788 [details] [diff] [review]
Part9. Always attempt to create a new decoder if we don't have one

add part#
Attachment #8557788 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #8557721 - Attachment is obsolete: true
Attachment #8557721 - Flags: review?(matt.woodrow)
Attachment #8557788 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8557770 - Flags: review?(cajbir.bugzilla) → review+
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.
Attachment #8557772 - Flags: review?(kinetik) → review+
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.
Attachment #8557774 - Flags: review?(kinetik) → review+
(Assignee)

Comment 39

4 years ago
Created attachment 8557838 [details] [diff] [review]
Part2. appendBuffer scanning the data before firing updateend

Carrying r+ comments. Always return a promise. Much more elegant.
Attachment #8557838 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #8557767 - Attachment is obsolete: true
Attachment #8557767 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 40

4 years ago
Created attachment 8557839 [details] [diff] [review]
Part3. Add support for partial init segment

Carrying r+, rebase on previous patch changes
(Assignee)

Updated

4 years ago
Attachment #8557770 - Attachment is obsolete: true
Attachment #8557838 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 41

4 years ago
Created attachment 8557840 [details] [diff] [review]
Part4. Add support for partial WebM init segment

Carrying r+
(Assignee)

Updated

4 years ago
Attachment #8557772 - Attachment is obsolete: true
(Assignee)

Comment 42

4 years ago
Created attachment 8557841 [details] [diff] [review]
Part5. Limit metadata parsing to init segment size if known

Carrying r+
(Assignee)

Updated

4 years ago
Attachment #8557774 - Attachment is obsolete: true
(Assignee)

Comment 43

4 years ago
Created attachment 8557844 [details] [diff] [review]
Part7. Only notify reader we have data, when we actually do

rebase...
(Assignee)

Updated

4 years ago
Attachment #8557784 - Attachment is obsolete: true
(Assignee)

Comment 44

4 years ago
Created attachment 8557846 [details] [diff] [review]
Part9. Always attempt to create a new decoder if we don't have one

rebase
(Assignee)

Updated

4 years ago
Attachment #8557788 - Attachment is obsolete: true
(Assignee)

Comment 45

4 years ago
Created attachment 8558297 [details] [diff] [review]
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.
Attachment #8558297 - Flags: review?(matt.woodrow)
(Assignee)

Comment 46

4 years ago
Created attachment 8558397 [details] [diff] [review]
Part3. Add support for partial init segment

missed a line in the previous rebase
(Assignee)

Updated

4 years ago
Attachment #8557839 - Attachment is obsolete: true
(Assignee)

Comment 47

4 years ago
Created attachment 8558398 [details] [diff] [review]
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
(Assignee)

Updated

4 years ago
Attachment #8557840 - Attachment is obsolete: true
(Assignee)

Comment 48

4 years ago
Created attachment 8558437 [details] [diff] [review]
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)
Attachment #8558437 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #8558297 - Attachment is obsolete: true
Attachment #8558297 - Flags: review?(matt.woodrow)
(Assignee)

Comment 49

4 years ago
Created attachment 8558438 [details] [diff] [review]
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.
Attachment #8558438 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Depends on: 1129224
(Assignee)

Comment 50

4 years ago
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
Attachment #8558437 - Attachment is obsolete: true
Attachment #8558437 - Flags: review?(matt.woodrow)
(Assignee)

Comment 51

4 years ago
Created attachment 8558897 [details] [diff] [review]
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.
Attachment #8558897 - Flags: review?(matt.woodrow)
(Assignee)

Comment 52

4 years ago
Created attachment 8558898 [details] [diff] [review]
Part11. Fix potential crash

Rebase.
Attachment #8558898 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #8558438 - Attachment is obsolete: true
Attachment #8558438 - Flags: review?(matt.woodrow)
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.
Attachment #8558897 - Flags: review?(matt.woodrow) → review+
Attachment #8558898 - Flags: review?(matt.woodrow) → review+
Blocks: 1127845
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1097252
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.
Attachment #8557838 - Flags: approval-mozilla-aurora?
status-firefox36: --- → unaffected
status-firefox37: --- → affected
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+
Attachment #8557838 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8557764 [details] [diff] [review]
Part1. Fix initialization of variables

[Triage Comment]
Attachment #8557764 - Flags: approval-mozilla-aurora+
Attachment #8557779 - Flags: approval-mozilla-aurora+
Attachment #8557785 - Flags: approval-mozilla-aurora+
Attachment #8557786 - Flags: approval-mozilla-aurora+
Attachment #8557841 - Flags: approval-mozilla-aurora+
Attachment #8557844 - Flags: approval-mozilla-aurora+
Attachment #8557846 - Flags: approval-mozilla-aurora+
Attachment #8558397 - Flags: approval-mozilla-aurora+
Attachment #8558398 - Flags: approval-mozilla-aurora+
Attachment #8558897 - Flags: approval-mozilla-aurora+
Attachment #8558898 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.