1578985 - Picture-in-Picture does not remember location and size of the popout windows

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, enhancement)

Description

[heindrich.paul]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

Open a Picture-in-Picture video and close it. Reopen it or open a new video and the size of the window and its location is reset to the bottom right corner.

Actual results:

The size and location is reset to the default and original values.

Expected results:

It should remember a user's preference to size and location based on an earlier window.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Mike Hommey [:glandium]]

I was about to file a bug for the same request, but while looking at the dependencies of the videopip bug, I'm not sure it's actually desirable in every situation. For example, say you have a dual monitor setup and you have two browser windows, one on each monitor. You open PiP from one of them. Would you want a PiP window open from the other to open at the location of the first one?
On the other hand, if you place a PiP window somewhere, close it, and reopen it from the same video in the same tab in the same window, it's more likely that you do want it where you may have moved it last (that's actually why I came here in the first place).

Comment 3

[Thomas (:thomschnch)]

Hello
I would try to fix this. I implemented a prototype who seems to work for me on my Fedora Linux.
I will attach the file to show you the actual state.

Please can you assignee this bug to me

Comment 4

[Thomas (:thomschnch)]

Attachment: PictureInPicture_20200406.patch

This is a Prototype of how it could work.
I hope the code quality is not to bad

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hi Thomas,

Thanks for taking this on. Instead of using the preferences system to store these values, I think it might make more sense to use XULStore, which is meant to hold things like window position and UI state between restarts.

Here's an example of XULStore being used to set a value: https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/browser/components/distribution.js#558-563

and to get a value: https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/browser/components/places/content/treeView.js#375-379

Here's the documentation for the XULStore interface: https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/toolkit/components/xulstore/nsIXULStore.idl

Where you're doing this work, PictureInPicture.jsm, makes sense - so for XULStore, I suggest using PLAYER_URI as the first argument for setting and getting the values.

Throughout this patch, I'll note that certain things were re-ordered - that generally makes patches harder to review. I highly recommend reducing the changes in the patch to encompass only the things that are required, and not doing any re-ordering unless necessary. If you're ttempting to make the code more consistent with those re-orderings, that might make a good follow-up patch in a separate bug.

Comment 6

[Thomas (:thomschnch)]

Attachment: Save Positon & Size from PictureinPicture

Hello Mike

Thanks for your help. I try to do what you sad. I go back with all re-ordered code and use the XULStore to save the Position. I stored the position as single values I hope this is OK.

I tested this patch on Windows and there the position work good I do not see a problems

On Linux are two minor bugs
If the window is in top left the window can moves 2 pixel down when a secound video is loaded then it stay stable. I think the problem coming from a round somewhere handling the parameter layout.css.devPixelsPerPx=1.2 if it is -1.0 it is ok

The second problem can occur in the right button if the aspectRation of the new video is changed it can move some pixel to another position. This I could also see in an officiel firefox version 75 when I move the video to top left and a new video is loaded. I could locate the problem in the function moveTo() if there are sent the same coordinates like if the window open it move the window to wrong position. I see some BUGs for this the best example is 1530495. I could reduce the problem be only use this function if needed.

Can you take a second look to this patch.
What should I do next?

Comment 7

[Thomas (:thomschnch)]

Hello Mike
How can I help to get this a step further?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hi Thomas,

I'm afraid I'm currently heads down on another project and don't have cycles to give feedback on this for a little while. I hope to have some time in the next few weeks. Apologies

Comment 9

[Thomas (:thomschnch)]

Attachment: PictureInPicture_export_20200723.patch

Hello Mike
Last Weekend something broke I had to make a small fix that on linux the window stay stable on top left.
I see that the pip window now has a border and the problem with the shift of two pixel in the left top seems to gone.

How looks with your time can you take a look into this patch?

Thanks

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Hi Thomas,

I apologize for the long delay. I finally have some time to get back to this. I'll have some review feedback for you today.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9165899 [details] [diff] [review]
PictureInPicture_export_20200723.patch

Review of attachment 9165899 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing this patch, Thomas. A few things:

It'd be best if this was posted to Phabricator so I can review it there, rather than directly to Bugzilla. Here are instructions: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html - let me know if you need guidance.

I worry that this patch is trying to do too much right out of the gate. I highly recommend we try to start with something much simpler and build on top of it.

For now, let's focus on trying to remember the position of the window, and not worry about the size of the window.

In that case, if I'm understanding your approach correctly, I think what we'll do is identify the nearest corner, and save the X-Y distances relative to that corner to XULStore, to account for display resolution changes. What about if there are multiple displays? Bug 890125, bug 440895 and bug 372650 added support for getting "workspace IDs" on Windows, macOS and Linux. I wonder if we should persist that ID to XULStore too for users that use multiple displays. This is what SessionStore does, for example.

Can you simplify the patch to only store the position of the window, relative to the nearest corner, and also persist/restore the workspace ID? Then we can start thinking about how we want to handle persisting / restoring window dimensions.

Comment 13

[Niklas Baumgardner]

Attachment: Bug 1578985 - Saving the last location and size of Picture in Picture window and opening there the next time. r?mconley,Gijs

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15a1bbaf80e4
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley

Comment 15

[Dorel Luca [:dluca]]

Backed out changeset 15a1bbaf80e4 (bug 1578985) for Browser-chrome failures pictureinpicture/tests/browser_resizeVideo.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319800704&repo=autoland&lineNumber=3116

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=15a1bbaf80e4be5b94a55197b0e207a57dd42ad4

Backout:
https://hg.mozilla.org/integration/autoland/rev/28ac6e3ab989fc6625273fd2962f9df2c624cc9c

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc07ab3cd238
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley

Comment 17

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_resizeVideo.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/5865ac26a9e2f6e1df09f5afbf8302667795a764
Log link: https://treeherder.mozilla.org/logviewer?job_id=320303079&repo=autoland&lineNumber=2913

Comment 18

[Narcis Beleuzu [:NarcisB]]

Please also check failures on browser_saveLastPiPLoc.js -> https://treeherder.mozilla.org/logviewer?job_id=320306854&repo=autoland&lineNumber=2181

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6272305d35d0
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley

Comment 20

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Nylks9cXQ-mtsCwGMbx5VQ.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=e52abe08f115bb2a941c529ae89bb3eb0f5794f4&searchStr=bc4

Logs:
https://treeherder.mozilla.org/logviewer?job_id=320656207&repo=autoland&lineNumber=4733
https://treeherder.mozilla.org/logviewer?job_id=320656328&repo=autoland&lineNumber=63975

Backout: https://hg.mozilla.org/integration/autoland/rev/e9fac2a6c4e604bee05cb26eee7eab3b0b2fcbf3

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e444f22bea93
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley

Comment 22

[Andreea Pavel [:apavel]]

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

Comment 23

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 71.a1(20190904163258) on Windows 10x64.
The issue is verified fixed with Firefox 84.0b3 (20201119195818) on Windows 10x64, macOS 10.12 and Ubuntu 18.04. The PiP player size and place are remembered when reopened.

Comment 24

[u681555]

Bug happened again from Firefox 86.