1320258 - MediaDecoderReader::UpdateBufferedWithPromise() caused events out of order

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1320258. Part 1 - add a test case to test 'ended' is fired when seeking to the end of media.

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

Comment 2

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

The test case fails because

http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/dom/media/MediaDecoderStateMachine.cpp#2720

'FirstFrameLoadedEvent' is delayed by the buffer update promise.

Comment 3

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

We should be able to update buffer ranges (as long as start time is known) before resolving the metadata promise since bug 1309516 is fixed.

Comment 4

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

Regression from bug 1251460.

Comment 5

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

Comment on attachment 8814308 [details]
Bug 1320258. Part 1 - add a test case to test 'ended' is fired when seeking to the end of media.

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

Comment 6

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

Updated the test case. The correct event order should be:
"seeked", "loadeddata", "ended".

'ended' is not fired because PlaybackEnded() happens before FirstFrameLoaded(). PlaybackEnded() returns early for |mPlayState == PLAY_STATE_LOADING|.

http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/dom/media/MediaDecoder.cpp#983

Comment 7

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

Attachment: Bug 1320258. Part 2 - remove MediaDecoderReader::UpdateBufferedWithPromise().

Bug 1313635 ensures buffer ranges are updated before resolving metadata.
So we can revert the changes in bug 1251460.

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

Comment 8

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

Comment on attachment 8814308 [details]
Bug 1320258. Part 1 - add a test case to test 'ended' is fired when seeking to the end of media.

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

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8814797 [details]
Bug 1320258. Part 2 - remove  MediaDecoderReader::UpdateBufferedWithPromise().

https://reviewboard.mozilla.org/r/95892/#review95880

Sorry, but I need this to handle bug 1319992.

As GetBuffered will become asynchronous, it is necessary to be able to use promises.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8814797 [details]
Bug 1320258. Part 2 - remove  MediaDecoderReader::UpdateBufferedWithPromise().

https://reviewboard.mozilla.org/r/95892/#review95884

after discussion on IRC. I will do differently.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8814308 [details]
Bug 1320258. Part 1 - add a test case to test 'ended' is fired when seeking to the end of media.

https://reviewboard.mozilla.org/r/95572/#review95886

::: dom/media/mediasource/test/mochitest.ini:64
(Diff revision 3)
>  [test_DurationChange.html]
>  [test_DurationUpdated.html]
>  [test_DurationUpdated_mp4.html]
>  skip-if = ((os == "win" && os_version == "5.1") || (toolkit == 'android')) # Not supported on xp and android 2.3
> +[test_EndedEvent.html]
> +skip-if = ((os == "win" && os_version == "5.1") || (toolkit == 'android')) # Not supported on xp and android 2.3

why is that not supported on xp or android?

you're using webm, this is supported.

Comment 12

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

Comment on attachment 8814308 [details]
Bug 1320258. Part 1 - add a test case to test 'ended' is fired when seeking to the end of media.

https://reviewboard.mozilla.org/r/95572/#review95886

> why is that not supported on xp or android?
> 
> you're using webm, this is supported.

Thanks for the notice.

Comment 13

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

Comment on attachment 8814308 [details]
Bug 1320258. Part 1 - add a test case to test 'ended' is fired when seeking to the end of media.

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

Comment 14

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

Comment on attachment 8814797 [details]
Bug 1320258. Part 2 - remove  MediaDecoderReader::UpdateBufferedWithPromise().

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

Comment 15

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

Comment on attachment 8814308 [details]
Bug 1320258. Part 1 - add a test case to test 'ended' is fired when seeking to the end of media.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95572/diff/4-5/

Comment 16

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

Comment on attachment 8814797 [details]
Bug 1320258. Part 2 - remove  MediaDecoderReader::UpdateBufferedWithPromise().

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08264c30352d
Part 1 - add a test case to test 'ended' is fired when seeking to the end of media. r=jya
https://hg.mozilla.org/integration/autoland/rev/cd9a0b0df8a3
Part 2 - remove  MediaDecoderReader::UpdateBufferedWithPromise(). r=jya

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08264c30352d
https://hg.mozilla.org/mozilla-central/rev/cd9a0b0df8a3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Does this need uplift nominations?

Comment 20

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

No. For the bugs (which are non-trivial) it depends on, I will just let this bug ride the train.