Closed Bug 1576605 Opened 2 months ago Closed Last month

Video would not switch to picture in picture mode

Categories

(Toolkit :: Video/Audio Controls, defect, P1)

70 Branch
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- verified
firefox71 --- verified

People

(Reporter: alice0775, Assigned: mconley)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(2 files)

Attached file about:support

Reproducible: reproducible with new profile, often

Steps to reproduce:
1. Create new profile and start browser
2. Open any video (e.g, https://videos.cdn.mozilla.net/uploads/mozillaorg/Mozilla_2014_i_am.webm )
3. Start playback if necessary
4. Left click on blue toggle button
   --- if not reproduced, restart browser and repeat from step 2.
   Or
   Right click on the video and choose "Picture in Picture"
   --- if not reproduced, restart browser and repeat from step 2.

Actual Results:
Nothing happens

Expected Results:
Should switch to PiP mode

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f246623260ee3b80c2021c48f5d56f54073f8060&tochange=b134d9db39a9da2cdb1a4b7c19e6023063fde7f4

Regressed by: Bug 1568320

Hi Alice0775 White, thanks for testing Picture-in-Picture. Are there any errors in the Browser Console when you're able to reproduce this issue?

Flags: needinfo?(alice0775)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #1)

Hi Alice0775 White, thanks for testing Picture-in-Picture. Are there any errors in the Browser Console when you're able to reproduce this issue?

No error in browser console.

Only error on loaded when the video, but I think this is not related.
[Exception... "Favicon at "https://videos.cdn.mozilla.net/favicon.ico" failed to load: Forbidden." nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource:///modules/FaviconLoader.jsm :: onStopRequest :: line 236" data: no] FaviconLoader.jsm:236:22

Flags: needinfo?(alice0775)

Mike is looking into this more, but some stuff I noticed...

I struggled to repro, but eventually managed to do so on Windows. It doesn't seem to repro on mac per talking to Mike, unsure why. Also unsure why it's not consistent...

In any case, when things break, I noticed that the child successfully calls togglePictureInPicture at https://searchfox.org/mozilla-central/rev/8ea946dcf51f0d6400362cc1d49c8d4808eeacf1/toolkit/actors/PictureInPictureChild.jsm#753, but at that point seems to believe we're already in picture in picture. That is, gWeakVideo points to the page's video. Unfortunately, I only got the debugger out once I could reproduce, so I'm not sure how we got into that state without an actual picture in picture window. In any case, at that point we never end up opening a picture in picture window because we just hang on to the weak video ref (or overwrite it with the same video) no matter how many times you click the button. I don't see any code that nulls out the weak video ref, except the page unload...

When you observe the screen carefully,
when the problem first occurs, it is observed that a small window opens for a moment and closes immediately. And after that, clicking the toggle button won't respond to anything.

I can also see this often, on Windows 10.

I think I've figured this out. Bug 1568320 made it so that we're accidentally sending two MozTogglePictureInPicture events rather than 1 when we're dealing with a video that has the built-in controls visible, which is being interpreted as "open and then close the video in Picture-in-Picture". The events come so quickly though and go through messaging in such a way that it's non-deterministic.

The correct fix here is to fire only a single MozTogglePictureInPicture event

Assignee: nobody → mconley
Blocks: 1527926

In bug 1568320, we made it so that the PictureInPictureToggleChild attaches handlers to
the toggle, even on videos that have controls by default, so we don't need the
videocontrols UAWidget to call the method for firing the event anymore.

Priority: -- → P1
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63f962127985
Don't fire extra MozTogglePictureInPicture when clicking toggle on videos with built-in controls. r=JSON_voorhees
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Can you request uplift to beta? That is, if you think is would not be risky in some way. Thanks.

Flags: needinfo?(mconley)

Comment on attachment 9089540 [details]
Bug 1576605 - Don't fire extra MozTogglePictureInPicture when clicking toggle on videos with built-in controls. r?JSON_Voorhees

Beta/Release Uplift Approval Request

  • User impact if declined: Users testing Picture-in-Picture on Beta will find that videos that use the built-in browser controls may not enter Picture-in-Picture mode sometimes.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a small, well-contained and well-understood patch.
  • String changes made/needed: None.
Flags: needinfo?(mconley)
Attachment #9089540 - Flags: approval-mozilla-beta?

Comment on attachment 9089540 [details]
Bug 1576605 - Don't fire extra MozTogglePictureInPicture when clicking toggle on videos with built-in controls. r?JSON_Voorhees

Improvement for an upcoming feature (though it won't ship in 70)
OK for uplift for beta 5.

Attachment #9089540 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Verified as fixed on Firefox Nightly 71.0a1 (2019-09-09) and on Firefox 70.0b5 on Windows 10 x64, Windows 7 x32, Ubuntu 18.04 and Mac OS X 10.15.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.