1062022 - mozRequestFullScreen shrinks video

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Russ Nicoletti [:russn]]

Attachment: Portrait, before calling mozRequestFullScreen

While testing the patch for bug 1055198, I noticed the video was being shrunk by mozRequestFullScreen. Also, while in this shrunk size, the screen doesn't respond to taps on the video itself, it only responds to taps on the header at the top of the screen.

Device: flame

Build info...

Build Id: 20140901040203
Gecko: 34.0a1
Gaia: e7d31f0e9b6b19d9b484eeec8fb980718bc40d79

Comment 1

[Russ Nicoletti [:russn]]

Attachment: Portrait, after calling mozRequestFullScreen

Comment 2

[Russ Nicoletti [:russn]]

Attachment: Landscap before calling mozRequestFullScreen

Comment 3

[Russ Nicoletti [:russn]]

Attachment: Landscape, after calling mozRequestFullScreen

Comment 4

[Russ Nicoletti [:russn]]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1055198#c7, https://bugzilla.mozilla.org/show_bug.cgi?id=1055198#c11, and https://bugzilla.mozilla.org/show_bug.cgi?id=1055198#c13.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

Anthony, could you help us investigate this?

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

It is most likely a layout or layers issue. That is either fullscreen of apzc. Jet - do you have any suggestions on who would be best to look at this?

Comment 7

[Jet Villegas (inactive)]

Attachment: testfulscreen.html

Adding a simple test to help isolate this.

Comment 8

[Jet Villegas (inactive)]

I just tried this on a Nexus 4 running today's trunk Gaia/Gecko and could not reproduce. Can you try the test file on the browser app on your Flame build?

Comment 9

[Russ Nicoletti [:russn]]

I opened the test html on my flame and did not see what I'm seeing in the video app. Here is the video code invoking mozRequestFullScreen: https://github.com/mozilla-b2g/gaia/pull/23500/files#diff-892af42bd5e9304faab35deaeabaaf1bR786. 'dom.player' is set to 'document.getElementById('player'); html is: '<video src="about:blank" id="player"></video>'. I don't see any difference between how video app is using the API and how the test html is using it.

From IRC with jet:

jet russn: the test file I added doesn't do the other stuff your app does though (e.g.. VideoUtils.fitContainer(...)) after the mozRequestFullScreen call

russn> jet: I will experiment with removing the calls after calling mozRequestFullScreen

Comment 10

[Russ Nicoletti [:russn]]

I could find no change in behavior regarding changing the flow after calling mozRequestFullScreen. However, when I add an 'alert' before the mozRequestFullScreen call, the issue does not reproduce. I suppose it could mean there is some kind of timing issue but I don't have any other insight into that at the moment.

Comment 11

[Jet Villegas (inactive)]

I see the usage of mozRequestFullScreen is both immediate in setControlsVisibility() and on a timeout in scheduleVideoControlsAutoHiding(). It's possible that mozRequestFullScreen and mozCancelFullScreen are racing in certain cases here.

Comment 12

[Russ Nicoletti [:russn]]

I had that same thought but I found even when the call to mozCancelFullScreen is commented out, the behavior is the same. Needs more investigation.

Comment 13

[Russ Nicoletti [:russn]]

Ok, the change in behavior based in an 'alert' being called before mozRequestFullScreen() is a red herring -- somehow invoking the alert is causing problems for mozRequestFullScreen(): After adding mozfullscreenchange and mozfullscreenerror event handlers, I tested with the 'alert' before mozRequestFullScreen() and received a mozfullscreenerror event; I then tested without the 'alert' before the mozRequestFullScreen() call and received a mozfullscreenchange event and the video distorted in the way I described above and, crucially, the software home button was hidden; so it's clear mozRequestFullScreen() is working in that respect.

Comment 14

[Russ Nicoletti [:russn]]

There are two flows in which mozRequestFullScreen is called:

1) When first selecting a video to play, 'showPlayer' is called which auto-hides the video controls by invoking scheduleVideoControlsAutoHiding, which calls setControlsVisibility(false)
2) When tapping the screen while a video is playing, toggleVideoControls is the event handler for this event and calls setControlsVisibility() with true or false depending on if the controls or currently showing.

To keep the diagnosis simple, I commented out scheduleVideoControlsAutoHiding from showPlayer so I could focus on the simpler 'toggleVideoControls' flow. In this flow, the video app makes no other calls other than setControlsVisibility(). Even in this flow the screen gets distorted by mozRequestFullScreen. 

So I can state conclusively that I don't see how the distortion can be caused by something the video app is doing either before or after the mozRequestFullScreen call. With that said, I have no explanation for the difference in behavior of the video app compared to the attached 'testfullscreen' test.

Comment 15

[Russ Nicoletti [:russn]]

John, can you provide your thoughts of this bug (bug 1055198 has the patch and more context of video app support of software home button)? Thanks.

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hey Russ,

It is caused by the transform css setting which is created by VideoUtils.fitContainer. Once you entered fullscreen mode, you need to remove that setting.

The patch of bug 1055198 calls fullscreen at video element. But I don't feel that's the correct. It looks like a workaround of "software home button" overlapping. In our case, we should call fullscreen mode at `player-view` and use our normal way to show/hide controllers.

Another one, there is a dummy software home button after applying bug 1055198 even if we don't enable software home button. I found it is created by the patch of bug 1055198 with ScreenLayout.getCurrentLayout('hardwareHomeButton'). This css query only returns if this device "has" hardware home button instead of it enables software home button. That seems so strange.

BTW, I had retested the "software home button" overlapping issue with the html provided by Jet. It does work correctly with "video element". I had modified the html to support fullscreen mode with div element. Please find it at [1]. The software home button had overlapped at browser app when we tap the "toggle Page Container" button.


[1] http://huchengtw-moz.github.io/test-fullscreen.html

ps. I tested in flame with pvt build:
Gaia      8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7
Gecko     https://hg.mozilla.org/mozilla-central/rev/6b3948d3413a
BuildID   20140909160207
Version   35.0a1
ro.build.date   Fri Jun 27 15:57:58 CST 2014
ro.bootloader   L1TC00011230
ro.build.version.incremental   110

Comment 17

[Russ Nicoletti [:russn]]

I didn't have any luck with removing the transform css setting; I wasn't able to solve the problem that way. However, using 'player-view' to invoke full screen mode did solve the problem. Thanks for that suggestion.

Also, I made a change to position the video controls based on whether the software home button is present or not.

The PR for bug 1055198 is updated with these changes.

I will add the "ScreenLayout.getCurrentLayout('hardwareHomeButton')" issue of displaying the software home button to bug 1055198.

I consider this bug resolved now that the fix is known and incorporated into the PR for the video app supporting the software home button.

Comment 18

[Russ Nicoletti [:russn]]

FYI, the issue of the software home button appearing when it is not enabled is tracked here: bug 1065654

Comment 19

[David Flanagan [:djf]]

Russ: for what its worth, mozRequestFullScreen() never worked well in the Gallery and Video apps, especially when switching orientation and going into and out of fullscreen mode. So I gave up long ago on it and we do permanent fullscreen in those apps through the manifest.

If you're able to make it work smoothly, we might want to consider using it again for Gallery and Video.

Comment 20

[Russ Nicoletti [:russn]]

David, see https://bugzilla.mozilla.org/show_bug.cgi?id=1037255#c5. As it is, the approach is for video app to use mozRequestFullScreen in order to hide swb. I will NI some folks in bug 1055198 regarding the issues we're having with this API and how to hide the swb some other way if this API is buggy.

Comment 21

[Russ Nicoletti [:russn]]

I'm reopening this because even though the video app can use mozRequestFullScreen on the player-view element, there are still multiple issues with the API in the video app.

1) When invoking mozRequestFullScreen on the video player element, the width and height of the video player element are changed to that of the player-view element [1] which for some reason results in the video player element being "shrunk" -- [2] and [3]. mozRequestFullScreen is only being called in response to toggling the video controls on; there is no logic to fit the video within the container in this flow. 

[1]:
"B4 fullscreen -- dom.player width/height: 352/288" video.js:304
"B4 fullscreen -- dom.playerView width/height: 320/569" video.js:306
"mozfullscreenchange" video.js:292
"dom.player width/height: 320/569" video.js:293

[2] https://bugzilla.mozilla.org/attachment.cgi?id=8483161
[3] https://bugzilla.mozilla.org/attachment.cgi?id=8483162

2) The other issue is when invoking mozRequestFullScreen on the player-view element. In this case, the video is not distorted. However, when changing the orientation of the device, the width and height of the video container does not change the first time the orientation is changed. On subsequent orientation changes, it changes but the values are what they should have been the last time [4]. This results in the video being improperly position within the screen by the 'fitContainer' function. See [5] for the context of the player-view, video player, and video-container.

[4]:
(orientation was changed from portrait to landscape -- width/height do not change)
"handleScreenLayoutChange -- dom.playerView width/height: 320/569" video.js:33
(orientation was changed from landscape to portrait -- width/height change to 'landscape')
"handleScreenLayoutChange -- dom.playerView width/height: 569/320" video.js:336

[5] https://github.com/mozilla-b2g/gaia/blob/master/apps/video/index.html#L76-L113

I've updated the PR to include the additional logging: https://bugzilla.mozilla.org/attachment.cgi?id=8481591

John, I would appreciate your input on this. Please let me know if I need to add or clarify anything. Thanks.

Comment 22

[Russ Nicoletti [:russn]]

More information regarding 1) above: there appears to be an issue with the transform after requesting fullscreen. The dimensions of the video container are the same before and after mozRequestFullScreen and the dimensions of the video do not change. These are the variables used to "fit" the video within the container. So it seems to me, when VideoUtils.fitContainer is called before having requested fullscreen and when it is called after requesting fullscreen, the result should be the same; the video should be positioned within the container identically before and after. But that is not the case as illustrated by [1] (video before requesting full screen), and [2] (video after requesting full screen). My speculation is a gecko issue is responsible.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8483161
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8483162

Note that I introduced a call to VideoUtils.fitContainer here to test this scenario: https://github.com/mozilla-b2g/gaia/pull/23500/files#diff-892af42bd5e9304faab35deaeabaaf1bR297

Comment 23

[Russ Nicoletti [:russn]]

Note #2 regarding 1) in comment 21. To test the scenario of invoking fullscreen on the player element, change 'playerView' to 'player' on this line -- https://github.com/mozilla-b2g/gaia/pull/23500/files#diff-892af42bd5e9304faab35deaeabaaf1bR312

Comment 24

[Russ Nicoletti [:russn]]

David and/or Punam, can one of you apply the patch from bug 1055198 [1] and give me your comments regarding the issue(s) in comment 21 and comment 22? Thanks.

[1] https://github.com/mozilla-b2g/gaia/pull/23500

Comment 25

[John Hu [:johnhu][:johu][:醬糊小弟]]

For the method 1)
    We should not try to calculate the position of full-sized video element by ourself. It is handled by gecko. In my comment 16, I found it is caused by the CSS transform created by fitContainer. I didn't retest it again in my device. But I feel we should remove all css settings relating to position, like the transform, and {1}.

For the method 2)
    I feel the your ref [4] is an issue of screenlayout which is based on CSS media query, the event of MediaQueryList{2}. In this case, I would suggest you to change to listen "onresize" event instead of screenlayoutchange. I had found similar issue few month ago and file a issue about the timing of dispatching from MediaQueryList. The side effect of changing screenlayoutchange event to resize event is that the Firefox Nightly dispatches multiple resize events while we hit the rotate button in nightly responsive design view.


{1} https://github.com/mozilla-b2g/gaia/blob/master/apps/video/style/video.css#L221-L229
{2} https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList

Comment 26

[David Flanagan [:djf]]

(In reply to Russ Nicoletti [:russn] from comment #24)
> David and/or Punam, can one of you apply the patch from bug 1055198 [1] and
> give me your comments regarding the issue(s) in comment 21 and comment 22?
> Thanks.
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/23500

I don't have time to look at this today, but will leave the needinfo set. I'm responding here to ask whether you are testing this with a .3gp video from the camera or with some kind of external video. Remember that .3gp videos are generally rotated and there is a CSS transform involved to make them play right-side up.  I wonder if that could be related to the weird numbers you're seeing when you change the orientation of the phone.

Comment 27

[Russ Nicoletti [:russn]]

Attachment: Rotated, 3gp video

Comment 28

[Russ Nicoletti [:russn]]

(In reply to David Flanagan [:djf] from comment #26)
> (In reply to Russ Nicoletti [:russn] from comment #24)
> > David and/or Punam, can one of you apply the patch from bug 1055198 [1] and
> > give me your comments regarding the issue(s) in comment 21 and comment 22?
> > Thanks.
> > 
> > [1] https://github.com/mozilla-b2g/gaia/pull/23500
> 
> I don't have time to look at this today, but will leave the needinfo set.
> I'm responding here to ask whether you are testing this with a .3gp video
> from the camera or with some kind of external video. Remember that .3gp
> videos are generally rotated and there is a CSS transform involved to make
> them play right-side up.  I wonder if that could be related to the weird
> numbers you're seeing when you change the orientation of the phone.

I have tested with rotated, 3gp videos as well as webm videos. They exhibit the same behavior. I have attached one of the 3gp videos I've used to test. I also have updated the fullscreen test [1] to use this video. Interestingly, the fullscreen test works properly, even with a transform property that is similar to what the video app is using. I will continue to explore the transform the video app is using because when I remove the transform property entirely, the video is not deformed. Although I don't see why the transform property should be different after calling mozRequestFullScreen since the size of the video and video container is unchanged after the API call. In any event, I will continue down that road of exploration.

[1] http://russnicoletti.github.io/requestFullScreen.html

Comment 29

[Punam Dahiya [:pdahiya]]

(In reply to Russ Nicoletti [:russn] from comment #24)
> David and/or Punam, can one of you apply the patch from bug 1055198 [1] and
> give me your comments regarding the issue(s) in comment 21 and comment 22?
> Thanks.
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/23500


Hi Russ
I am seeing a bug in positioning a video while  switching orientation on Friday's build id: 20140919040205 (Flame and Nexus4) even before your patch was installed. Not sure if it's related,  as I am not familiar with how VideoUtils fitContainer and mozRequestFullScreen works.

Comment 30

[Russ Nicoletti [:russn]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #25)
> For the method 1)
>     We should not try to calculate the position of full-sized video element
> by ourself. It is handled by gecko. In my comment 16, I found it is caused
> by the CSS transform created by fitContainer. I didn't retest it again in my
> device. But I feel we should remove all css settings relating to position,
> like the transform, and {1}.
> 
> For the method 2)
>     I feel the your ref [4] is an issue of screenlayout which is based on
> CSS media query, the event of MediaQueryList{2}. In this case, I would
> suggest you to change to listen "onresize" event instead of
> screenlayoutchange. I had found similar issue few month ago and file a issue
> about the timing of dispatching from MediaQueryList. The side effect of
> changing screenlayoutchange event to resize event is that the Firefox
> Nightly dispatches multiple resize events while we hit the rotate button in
> nightly responsive design view.
> 
> 
> {1}
> https://github.com/mozilla-b2g/gaia/blob/master/apps/video/style/video.
> css#L221-L229
> {2} https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList

I can confirm that using the 'resize' event instead of 'screenlayoutchange' solves the layout issue when changing screen orientation in fullscreen mode.

I also have noticed a significant difference in the behavior of mozRequestFullScreen using the latest base image (v180) -- with the v180 base image, the system is automatically adjusting the window to account for the presence of the swb; therefore, the video app does not have to make layout changes for the swb. I am in the process of updating the 1055198 PR to use the 'resize' event instead of the 'screenlayoutchange' event and to no longer adjust the layout when the swb is displayed and hidden.

David, I am clearing your NI as I consider the issue resolved. Feel free to comment nonetheless.

Comment 31

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hey Russ,

I didn't get the meaning of swb. What's swb?

From my knowledge, the base image doesn't bring lots differences. It contains latest gonk code, linux kernel driver, and other native codes. Most of them are affecting the timing of message pumping to gecko. But the rotation handling is handled by gecko. And gecko asks gonk to rotate the whole frame buffer. So, it should not give us different result. When and how to rotate is decided by gecko.

Comment 32

[Russ Nicoletti [:russn]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #31)
> Hey Russ,
> 
> I didn't get the meaning of swb. What's swb?
> 
> From my knowledge, the base image doesn't bring lots differences. It
> contains latest gonk code, linux kernel driver, and other native codes. Most
> of them are affecting the timing of message pumping to gecko. But the
> rotation handling is handled by gecko. And gecko asks gonk to rotate the
> whole frame buffer. So, it should not give us different result. When and how
> to rotate is decided by gecko.

swb --> 'software button' aka 'software home button' (apologies for the cryptic abbreviation -- I try to spell everything out to avoid confusion but I guess I got lazy this time).

Comment 33

[Russ Nicoletti [:russn]]

Regarding the difference in behavior of the system with respect to the software home button mentioned in comment 30, there was a regression in the system app that was responsible for this change in behavior.

Oldest build where behavior is correct:

Gaia      c71fd5d8c9c7cb021c97e5e9fbb29f92b50a084d
Gecko     https://hg.mozilla.org/mozilla-central/rev/892768985915
BuildID   20140908040204
Version   35.0a1

Most recent build where behavior is incorrect (system app appears to ignore 'fullscreen_layout' property):

Gaia      72262d054ffa5d0d2b5a0033f713149281511aea
Gecko     https://hg.mozilla.org/mozilla-central/rev/4f2cac8d72da
BuildID   20140917160215
Version   35.0a1

I will continue to narrow down the regression window tomorrow.

Comment 34

[Russ Nicoletti [:russn]]

Bug 1073611 has been created to track the regression in the software home button behavior discussed above. I consider this bug resolved based on comment 30. I will update the 1055198 PR to reflect the resolution in comment 30.