1306945 - No video when replay at YouTube

Status: RESOLVED FIXED

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

Description

[atlanto]

STR:
1. Open YouTube music video in new tab and activate the tab
2. Ensure video starting, disable Autoplay
3. Switch to another tab and wait for the video to finish(for the audio indicator to disappear)
4. Switch to the tab and click Replay

ER:
The music video is replaying

AR:
Audio starts. However, video is not rendered unless tab switching once again.

mozregression:
29:47.74 INFO: Last good revision: 4c2062016341b01b05bcb421a8fb72f96d6ccf2c
29:47.74 INFO: First bad revision: 537e6ba21ea98583674247e61ccfdc54a62e5899
29:47.74 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4c2062016341b01b05bcb421a8fb72f96d6ccf2c&tochange=537e6ba21ea98583674247e61ccfdc54a62e5899

29:48.73 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1280297

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Break youtube web site 

Confirmed to Reproduce on Nightly52.0a1 Windows10.


FYI, 
Setting media.suspend-bkgnd-video.enabled = false(Bug 1275472 default value of Aurora51.0a2 and Beta50.0b3) seems to fix the problem.

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

[Alice0775 White]

The problem occurs with/without e10s.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Seems very strange, since bug 1280297 should not have had any functional effect. Still I'll look into it.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to atlanto from comment #0)
> mozregression:
> 29:47.74 INFO: Last good revision: 4c2062016341b01b05bcb421a8fb72f96d6ccf2c
> 29:47.74 INFO: First bad revision: 537e6ba21ea98583674247e61ccfdc54a62e5899

I used |mach mozregression --launch <cset> --repo mozilla-inbound| with these two changesets to verify the range. On both I get the same behaviour - when I hit the replay button it doesn't replay (even the time slider doesn't advance). This is different from the behaviour on Nightly, where I see what you describe - the time slider advances but no video is rendered.

For the record the video I used is https://www.youtube.com/watch?v=JAHDbZOigBl

I'll run mozregression and see if I get a different result.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Sorry, wrong URL in previous comment. The last character should be an uppercase i and not a lowercase L. Updating URL field.

I got a regression window from the behaviour I described in comment 5 (where even the time slider doesn't advance) to the current behaviour (where the time slider advances but the video doesn't show):

INFO: Last good revision: 516c82acb163c7f75048b46118d5414db5cec558
INFO: First bad revision: 103dc4eddacf4d128ae5bd6516485ee53d61c967
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=516c82acb163c7f75048b46118d5414db5cec558&tochange=103dc4eddacf4d128ae5bd6516485ee53d61c967
INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1282710

I'm working on getting the other regression window, where it goes from working normally to not advancing the time slider.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

This is the regression window where it went from working fine to the time slider not advancing at all:

INFO: Last good revision: 8374debdc9336ce898c41353bd4ae5be0189dbb1
INFO: First bad revision: 3206ef153b98d1200aab86f50a27d884509d39e1
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8374debdc9336ce898c41353bd4ae5be0189dbb1&tochange=3206ef153b98d1200aab86f50a27d884509d39e1
INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1275481

Comment 8

[Marcia Knous [:marcia]]

Tracking 52+ as this is a regression that relates to a popular site.

Comment 9

[Tzuhao Kuo [:kaku]]

This is a regression of suspend-video-decoder project.

So, the root cause is that we are not able to switch the video decoder of a video element which has reached the end because the already-ended-video is regarded as "has no video" so that our "switching decoder code" returns early here: http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/dom/media/MediaDecoderStateMachine.cpp#1816

Comment 10

[u480271]

:kaku, would the next check for the state != PLAY_STATE_PLAYING also hand the ended case?

Comment 11

[Tzuhao Kuo [:kaku]]

Yes, it does, the code also returns early there.

Comment 12

[Tzuhao Kuo [:kaku]]

@Dan, sorry that I was wrong at comment 9, the HasVideo() keeps true even the video has ended; the real root cause the the check of mPlayState. 

So, I think that we can easily fix this bug by removing the check of mPlayState entirely. Actually, we can switch the decoder no matter the element is playing or not. A patch is following.

Comment 13

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1306945 - remove the check of mPlayState in the MDSM::VisibilityChanged();

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

Comment 14

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

Comment on attachment 8797413 [details]
Bug 1306945 - remove the check of mPlayState in the MDSM::VisibilityChanged();

https://reviewboard.mozilla.org/r/83028/#review81678

Comment 15

[u480271]

Comment on attachment 8797413 [details]
Bug 1306945 - remove the check of mPlayState in the MDSM::VisibilityChanged();

https://reviewboard.mozilla.org/r/83028/#review82058

Comment 16

[Tzuhao Kuo [:kaku]]

Thanks for the review!

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77bb212d10c9

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4a0b0a51b1e
remove the check of mPlayState in the MDSM::VisibilityChanged(); r=jwwang,kamidphish

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4a0b0a51b1e

Comment 19

[Gerry Chang [:gchang]]

Hi :kaku,
Since this bug is a regression and also affects 50/51, do you consider to uplift this for 50/51 if this patch is not too risky?

Comment 20

[Tzuhao Kuo [:kaku]]

(In reply to Gerry Chang [:gchang] from comment #19)
> Hi :kaku,
> Since this bug is a regression and also affects 50/51, do you consider to
> uplift this for 50/51 if this patch is not too risky?
This bug is a regression of "suspend-video-decoder" feature (Bug 1276556) which now is enabled for nightly only. So, the aurora and beta channel should be ok now and we don't need to uplift this patch. Anyway, thanks for your reminding!

Comment 21

[Andrei Vaida [:avaida]]

qe-verify-, since Suspend Video Decoder is enabled on nightly-only, with plans to ride 54 (at least Phase 1, per 1293963).