Closed Bug 1320258 Opened 3 years ago Closed 3 years ago

MediaDecoderReader::UpdateBufferedWithPromise() caused events out of order

Categories

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

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

(Keywords: regression)

Attachments

(2 files)

No description provided.
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.
Depends on: 1309516
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.
Depends on: 1313635
Assignee: nobody → jwwang
Priority: -- → P3
Regression from bug 1251460.
Blocks: 1251460
Keywords: regression
Blocks: 1319706
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 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.
Attachment #8814797 - Flags: review?(jyavenard) → review-
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.
Attachment #8814797 - Flags: review- → review+
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.
Attachment #8814308 - Flags: review?(jyavenard) → review+
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.
Version: unspecified → 48 Branch
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/08264c30352d
https://hg.mozilla.org/mozilla-central/rev/cd9a0b0df8a3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Does this need uplift nominations?
Flags: needinfo?(jwwang)
No. For the bugs (which are non-trivial) it depends on, I will just let this bug ride the train.
Flags: needinfo?(jwwang)
You need to log in before you can comment on or make changes to this bug.