1322087 - When finish playback, Spinner is spinning for a while. And finally "The media could not be played." display

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

[Tracking Requested - why for this release]: unable to playback twitter video due to "The media could not be played."


Build identifier:
https://hg.mozilla.org/mozilla-central/rev/6bdef7ba8b4108a996b9f61ef9f81c5ea6c93017
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161204030210

The problem occur with/without e10s.
It occurs when you explicitly start playing by clicking the video. (i.e., playback with audio). 

Reproducible : always

Steps to reproduce:
1. Open twitter
     (e.g, https://twitter.com/shlml/status/805428754864427008 )
2. Start to playback the video by click on the video
3. Wait to finish the video

3. Playback again if any

Actual Results:
When finish playback, Spinner is spinning fo a while. And  finally "The media could not be played." display

Expected Results:
Playback button[>] should display

Comment 1

[Alice0775 White]

Expected Results:
Repeat button in video controls should display

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5ffa818f1cb34646e58543bc0443f9da3067c246&tochange=e39a5f086e68cf61c54069fbc42a97b89ed97a27

Regressed by: Bug 1314219

Comment 3

[Alice0775 White]

Attachment: about:suppot

Comment 4

[Alice0775 White]

Attachment: screenshot

Comment 5

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

It is not really a regression but it reveals a bug that is less likely to happen before bug 1314219.

Comment 6

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

Attachment: Bug 1322087. Part 1 - move |if (!mFirstFrameLoaded)| up to save some duplicate checks.

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

Comment 7

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

Attachment: Bug 1322087. Part 2 - move readyState to HAVE_ENOUGH_DATA when we have no more data to fetch.

http://searchfox.org/mozilla-central/rev/dc8cf05768b83a6ef0b4039edd6efddd56ee4109/dom/media/MediaDecoderStateMachine.cpp#1065
Changing nextFrameStatus to UNAVAILABLE might change readyState to HAVE_CURRENT_DATA and cause 'waiting' to fire.
It doesn't make sense to fire 'waiting' at the end of playback.

http://searchfox.org/mozilla-central/rev/dc8cf05768b83a6ef0b4039edd6efddd56ee4109/dom/html/HTMLMediaElement.cpp#5423
Note the check for Ended() doesn't work as expected to prevent 'waiting' from firing at the end of playback
because of the way how TailDispatcher schedules stateChange tasks.

This patch keeps readyState in HAVE_ENOUGH_DATA when playback is near the end for wanting no more data.
readyState will change to HAVE_CURRENT_DATA when the playing state of MediaDecoder is changed to PLAY_STATE_ENDED.

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

Comment 8

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

Comment on attachment 8817113 [details]
Bug 1322087. Part 2 - move readyState to HAVE_ENOUGH_DATA when we have no more data to fetch.

https://reviewboard.mozilla.org/r/97546/#review97908

Comment 9

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

Comment on attachment 8817113 [details]
Bug 1322087. Part 2 - move readyState to HAVE_ENOUGH_DATA when we have no more data to fetch.

https://reviewboard.mozilla.org/r/97546/#review97910

Cancel review.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8817112 [details]
Bug 1322087. Part 1 - move |if (!mFirstFrameLoaded)| up to save some duplicate checks.

https://reviewboard.mozilla.org/r/97544/#review97912

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8817113 [details]
Bug 1322087. Part 2 - move readyState to HAVE_ENOUGH_DATA when we have no more data to fetch.

https://reviewboard.mozilla.org/r/97546/#review97916

Comment 12

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

Attachment: Bug 1322087. Part 3 - allow MediaResoure::Tell() to be called on the main thread.

It is safe to call Tell() on the main thread because it won't block the main thread like Read() does.

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

Comment 13

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

Attachment: Bug 1322087. Part 4 - enable some test in mediasource-endofstream.html.

P2 fixes the test because it forces HAVE_ENOUGH_DATA when endOfStream() is called.

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

Comment 14

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

Comment on attachment 8817113 [details]
Bug 1322087. Part 2 - move readyState to HAVE_ENOUGH_DATA when we have no more data to fetch.

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

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8817419 [details]
Bug 1322087. Part 3 - allow MediaResoure::Tell() to be called on the main thread.

https://reviewboard.mozilla.org/r/97696/#review98108

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8817420 [details]
Bug 1322087. Part 4 - enable some test in  mediasource-endofstream.html.

https://reviewboard.mozilla.org/r/97698/#review98110

Comment 17

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

Thanks!

Comment 18

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

Comment on attachment 8817112 [details]
Bug 1322087. Part 1 - move |if (!mFirstFrameLoaded)| up to save some duplicate checks.

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

Comment 19

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

Comment on attachment 8817113 [details]
Bug 1322087. Part 2 - move readyState to HAVE_ENOUGH_DATA when we have no more data to fetch.

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

Comment 20

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

Comment on attachment 8817419 [details]
Bug 1322087. Part 3 - allow MediaResoure::Tell() to be called on the main thread.

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

Comment 21

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

Comment on attachment 8817420 [details]
Bug 1322087. Part 4 - enable some test in  mediasource-endofstream.html.

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa8864946562
Part 1 - move |if (!mFirstFrameLoaded)| up to save some duplicate checks. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/f988f0ed610f
Part 2 - move readyState to HAVE_ENOUGH_DATA when we have no more data to fetch. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8ded70fe7212
Part 3 - allow MediaResoure::Tell() to be called on the main thread. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/fa4c79383dbe
Part 4 - enable some test in  mediasource-endofstream.html. r=cpearce

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa8864946562
https://hg.mozilla.org/mozilla-central/rev/f988f0ed610f
https://hg.mozilla.org/mozilla-central/rev/8ded70fe7212
https://hg.mozilla.org/mozilla-central/rev/fa4c79383dbe

Comment 24

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

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #5)
> It is not really a regression but it reveals a bug that is less likely to
> happen before bug 1314219.

Since bug 1314219 makes this issue more likely to happen, we will need uplift the changes to Aurora(52).

Comment 25

[Julien Cristau [:jcristau]]

tracking for 52 per comment 24

Comment 26

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

Attachment: 1322087_fix_aurora52.patch

Approval Request Comment
[Feature/Bug causing the regression]:1322087
[User impact if declined]:Some Twitter videos can't be played again when playback reaches the end.
[Is this code covered by automated tests?]:Yes.
[Has the fix been verified in Nightly?]:Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Described in comment 0.
[List of other uplifts needed for the feature/fix]:None.
[Is the change risky?]:Low risk.
[Why is the change risky/not risky?]:The change is simple and easy to test/verify.
[String changes made/needed]:None.

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8818807 [details] [diff] [review]
1322087_fix_aurora52.patch

fix video playback issue in aurora52

Comment 28

[Julien Cristau [:jcristau]]

It would be nice to get verification on this once it lands in aurora, with STR in comment 0.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/481d62db5b32

Comment 30

[ovidiu boca[:Ovidiu]]

Tested on Windows 10, Mac 10.10 and Ubuntu 16.04 with FF Nighlty 53.0a1(2016-12-29) and I can confirme the fix, the issue is no longer reproducible.

Comment 31

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  53.0a1 (2016.12.04) on Win 10 x86.
I can confirm this issue is fixed, I verified using Firefox 52.0b4, on Win 10 x86 and Windows 8.1 x64.