Label runnables under dom/media/mediasource

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [QDL][TDC-MVP][MEDIA])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1341539
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review118128

::: dom/media/mediasource/test/test_AudioChange_mp4.html:62
(Diff revision 1)
>      })
>      .then(fetchAndLoad.bind(null, audiosb, 'aac51-48000-128000-', ['2'], '.m4s'))
>      .then(function() {
> -      var promises = [];
>        ms.endOfStream();
> -      promises.push(once(el, 'ended'));
> +      return once(el, 'ended');

That can't be the reason.

endOfStream will *queue* the event updateend. The event will be fired during the next event loop, that is when the current JS flow finishes.

All of the instructions following endOfSteam are done in the same event loop; the updateend event can't be fired during that time.

So the idea that the listener may not be set prior the event being fired is incorrect.
Attachment #8842711 - Flags: review?(jyavenard) → review-

Comment 4

2 years ago
mozreview-review
Comment on attachment 8842710 [details]
Bug 1343459. Part 1 - Label runnables under dom/media/mediasource.

https://reviewboard.mozilla.org/r/116482/#review118130

::: dom/media/mediasource/MediaSource.cpp:58
(Diff revision 1)
>  }
>  
>  #define MSE_DEBUG(arg, ...) MOZ_LOG(GetMediaSourceLog(), mozilla::LogLevel::Debug, ("MediaSource(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))
>  #define MSE_API(arg, ...) MOZ_LOG(GetMediaSourceAPILog(), mozilla::LogLevel::Debug, ("MediaSource(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))
>  
> +#define NS_DispatchToMainThread(...) CompileError_UseAbstractMainThreadInstead

this is very ugly ...

And NS_DispatchToMainThread is no longer used...

::: dom/media/mediasource/SourceBuffer.cpp:42
(Diff revision 1)
>  
>  #define MSE_DEBUG(arg, ...) MOZ_LOG(GetMediaSourceLog(), mozilla::LogLevel::Debug, ("SourceBuffer(%p:%s)::%s: " arg, this, mType.OriginalString().Data(), __func__, ##__VA_ARGS__))
>  #define MSE_DEBUGV(arg, ...) MOZ_LOG(GetMediaSourceLog(), mozilla::LogLevel::Verbose, ("SourceBuffer(%p:%s)::%s: " arg, this, mType.OriginalString().Data(), __func__, ##__VA_ARGS__))
>  #define MSE_API(arg, ...) MOZ_LOG(GetMediaSourceAPILog(), mozilla::LogLevel::Debug, ("SourceBuffer(%p:%s)::%s: " arg, this, mType.OriginalString().Data(), __func__, ##__VA_ARGS__))
>  
> +#define NS_DispatchToMainThread(...) CompileError_UseAbstractMainThreadInstead

same here
Attachment #8842710 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 5

2 years ago
endOfStream will not queue 'updateend' according to my test. I got 4 'updateend' events no matter |ms.endOfStream()| is called or not.
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review118128

> That can't be the reason.
> 
> endOfStream will *queue* the event updateend. The event will be fired during the next event loop, that is when the current JS flow finishes.
> 
> All of the instructions following endOfSteam are done in the same event loop; the updateend event can't be fired during that time.
> 
> So the idea that the listener may not be set prior the event being fired is incorrect.

endOfStream will not queue 'updateend' according to my test. I got 4 'updateend' events no matter |ms.endOfStream()| is called or not.
(Assignee)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8842710 [details]
Bug 1343459. Part 1 - Label runnables under dom/media/mediasource.

https://reviewboard.mozilla.org/r/116482/#review118140

::: dom/media/mediasource/MediaSource.cpp:58
(Diff revision 1)
>  }
>  
>  #define MSE_DEBUG(arg, ...) MOZ_LOG(GetMediaSourceLog(), mozilla::LogLevel::Debug, ("MediaSource(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))
>  #define MSE_API(arg, ...) MOZ_LOG(GetMediaSourceAPILog(), mozilla::LogLevel::Debug, ("MediaSource(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))
>  
> +#define NS_DispatchToMainThread(...) CompileError_UseAbstractMainThreadInstead

http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/dom/media/MediaDecoderStateMachine.cpp#65

It is devised by bholley to prevent future use of NS_DispatchToMainThread by accident.
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review118128

> endOfStream will not queue 'updateend' according to my test. I got 4 'updateend' events no matter |ms.endOfStream()| is called or not.

http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/dom/media/mediasource/SourceBuffer.cpp#410
'updateend' is fired by the following call path:
SourceBuffer::AppendData -> SourceBuffer::AppendDataCompletedWithSuccess -> SourceBuffer::StopUpdating() -> QueueAsyncSimpleEvent("updateend")
(Assignee)

Updated

2 years ago
Flags: needinfo?(jyavenard)
hmm, I thought I had responded to that one...
i see, I didn't validate my earlier comment on reviewboard.

will do so now.
Flags: needinfo?(jyavenard)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review118160

::: dom/media/mediasource/test/test_AudioChange_mp4.html:62
(Diff revision 1)
>      })
>      .then(fetchAndLoad.bind(null, audiosb, 'aac51-48000-128000-', ['2'], '.m4s'))
>      .then(function() {
> -      var promises = [];
>        ms.endOfStream();
> -      promises.push(once(el, 'ended'));
> +      return once(el, 'ended');

My mistake...

you are right, ms.endOfStream doesn't send an updateend event.

My other comments were valid however...

It's certainly not an issue of and event listener not being registered on time for the reasons I mentioned.
So your changes won't solve anything.

the test load two segments.
one stereo, followed by one in 5.1. They are continuous.

When you called play() earlier, playback would have started and reached the end, at which point as the mediasource isn't ended, it will fire "waiting".

Once endOfStream is called, if playback has already reached the end of the data -> ended will be fired

if playback hasn't reached the end yet, then it will play to the end and fire ended.
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review118160

> My mistake...
> 
> you are right, ms.endOfStream doesn't send an updateend event.
> 
> My other comments were valid however...
> 
> It's certainly not an issue of and event listener not being registered on time for the reasons I mentioned.
> So your changes won't solve anything.
> 
> the test load two segments.
> one stereo, followed by one in 5.1. They are continuous.
> 
> When you called play() earlier, playback would have started and reached the end, at which point as the mediasource isn't ended, it will fire "waiting".
> 
> Once endOfStream is called, if playback has already reached the end of the data -> ended will be fired
> 
> if playback hasn't reached the end yet, then it will play to the end and fire ended.

No. The problem is not about the 'ended' event, but about the 'updateedn' event. Since P1 changes the event order, it is possible the event is missed because the event handler is not registered on time. Therefore P2 registers the 'updateend' handler at the beginning of the test to ensure it never misses the event.
(Assignee)

Updated

a year ago
Flags: needinfo?(jyavenard)

Comment 12

a year ago
mozreview-review
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review121892

::: dom/media/mediasource/test/test_AudioChange_mp4.html:62
(Diff revision 1)
>      })
>      .then(fetchAndLoad.bind(null, audiosb, 'aac51-48000-128000-', ['2'], '.m4s'))
>      .then(function() {
> -      var promises = [];
>        ms.endOfStream();
> -      promises.push(once(el, 'ended'));
> +      return once(el, 'ended');

Which event handler would miss updateend, which function at which line?

I don't see how that's possible as, as mentioned earlier updateend is *queued* in the current event loop. updateend won't be fired then.

This fix makes no sense to me.

Comment 13

a year ago
mozreview-review
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review121894

::: dom/media/mediasource/test/test_AudioChange_mp4.html
(Diff revision 1)
>      .then(fetchAndLoad.bind(null, audiosb, 'aac51-48000-128000-', ['2'], '.m4s'))
>      .then(function() {
> -      var promises = [];
>        ms.endOfStream();
> -      promises.push(once(el, 'ended'));
> +      return once(el, 'ended');
> -      promises.push(once(audiosb, 'updateend'));

the only thing valid here is the removal of promises.push(once(audiosb, 'updateend'));

Everything else is superflous and uncessary.
(Assignee)

Comment 14

a year ago
mozreview-review-reply
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review121892

> Which event handler would miss updateend, which function at which line?
> 
> I don't see how that's possible as, as mentioned earlier updateend is *queued* in the current event loop. updateend won't be fired then.
> 
> This fix makes no sense to me.

#59 promises.push(once(audiosb, 'updateend'));
By the time #59 is run, the 4th 'updateend' is already fired and it is too late to register the handler. |Promise.all(promises)| will never be resolved.

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review121892

> #59 promises.push(once(audiosb, 'updateend'));
> By the time #59 is run, the 4th 'updateend' is already fired and it is too late to register the handler. |Promise.all(promises)| will never be resolved.

sure, but you've removed that line (as it should be).
so there's now no reason on why the number of updateend should be lodged and measured.
(Assignee)

Comment 16

a year ago
(In reply to Jean-Yves Avenard [:jya] from comment #12)
> I don't see how that's possible as, as mentioned earlier updateend is
> *queued* in the current event loop. updateend won't be fired then.

Which "current event loop" do you mean?
The JS one.
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a year ago
mozreview-review
Comment on attachment 8842711 [details]
Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

https://reviewboard.mozilla.org/r/116484/#review121928
Attachment #8842711 - Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

a year ago
Thanks for the reviews!

Comment 24

a year ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c162d29766f8
Part 1 - Label runnables under dom/media/mediasource. r=jya
https://hg.mozilla.org/integration/autoland/rev/1a9984c2800d
Part 2 - remove the 'updateend' handler which might fire before we register it. r=jya

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c162d29766f8
https://hg.mozilla.org/mozilla-central/rev/1a9984c2800d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a year ago
Whiteboard: [QDL][TDC-MVP][MEDIA]
You need to log in before you can comment on or make changes to this bug.