Closed Bug 1306945 Opened 3 years ago Closed 3 years ago

No video when replay at YouTube

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox50 --- disabled
firefox51 --- disabled
firefox52 + fixed

People

(Reporter: euthanasia_waltz, Assigned: kaku)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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
Blocks: 1280297
Keywords: regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bugmail)
Version: unspecified → 50 Branch
[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.
[Tracking Requested - why for this release]:
The problem occurs with/without e10s.
Summary: [e10s] No video when replay at YouTube → No video when replay at YouTube
Seems very strange, since bug 1280297 should not have had any functional effect. Still I'll look into it.
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
(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.
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.
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
Assignee: bugmail → nobody
Blocks: 1282710, 1275481
No longer blocks: 1280297
Flags: needinfo?(kaku)
Flags: needinfo?(dglastonbury)
Tracking 52+ as this is a regression that relates to a popular site.
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
Flags: needinfo?(kaku)
:kaku, would the next check for the state != PLAY_STATE_PLAYING also hand the ended case?
Flags: needinfo?(dglastonbury) → needinfo?(kaku)
Yes, it does, the code also returns early there.
Flags: needinfo?(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.
Assignee: nobody → kaku
Comment on attachment 8797413 [details]
Bug 1306945 - remove the check of mPlayState in the MDSM::VisibilityChanged();

https://reviewboard.mozilla.org/r/83028/#review81678
Attachment #8797413 - Flags: review?(jwwang) → review+
Comment on attachment 8797413 [details]
Bug 1306945 - remove the check of mPlayState in the MDSM::VisibilityChanged();

https://reviewboard.mozilla.org/r/83028/#review82058
Attachment #8797413 - Flags: review?(dglastonbury) → review+
Thanks for the review!

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77bb212d10c9
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4a0b0a51b1e
remove the check of mPlayState in the MDSM::VisibilityChanged(); r=jwwang,kamidphish
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e4a0b0a51b1e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
Flags: needinfo?(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!
Flags: needinfo?(kaku)
qe-verify-, since Suspend Video Decoder is enabled on nightly-only, with plans to ride 54 (at least Phase 1, per 1293963).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.