1343459 - Label runnables under dom/media/mediasource

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1343459. Part 1 - Label runnables under dom/media/mediasource.

Review commit: https://reviewboard.mozilla.org/r/116482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116482/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1343459. Part 2 - remove the 'updateend' handler which might fire before we register it.

Note the race is uncovered by P1 which kinda change the order of events.

Review commit: https://reviewboard.mozilla.org/r/116484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116484/

Comment 3

[Jean-Yves Avenard [:jya]]

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.

Comment 4

[Jean-Yves Avenard [:jya]]

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

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 9

[Jean-Yves Avenard [:jya]]

hmm, I thought I had responded to that one...
i see, I didn't validate my earlier comment on reviewboard.

will do so now.

Comment 10

[Jean-Yves Avenard [:jya]]

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.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 12

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

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.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

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

[Jean-Yves Avenard [:jya]]

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.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 17

[Jean-Yves Avenard [:jya]]

The JS one.

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116482/diff/1-2/

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116484/diff/1-2/

Comment 20

[Jean-Yves Avenard [:jya]]

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

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116482/diff/2-3/

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116484/diff/2-3/

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the reviews!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c162d29766f8
https://hg.mozilla.org/mozilla-central/rev/1a9984c2800d