1269337 - zoomed out reveal.js slides stretch rather than scroll; regression from FF 45 to FF 46

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Slideshow presentations generated via `reveal.js` are not correctly rendering when in the "zoomed out" mode (accessed via the ESC key in reveal.js) and when the browser window is on my MacBook Pro's built in screen.

Steps to Reproduce (I'm using FF 46):  

 1. Go to a reveal.js presentation, for example: http://lab.hakim.se/reveal-js/#/ 

 2. Hit the Escape Key (which goes to a zoomed out mode, showing some number of columns of slides).

 3. Hit the right arrow key.

    * In FF 45, this used to scroll over to the right, revealing a new column of slides.

    * In FF 46, hitting the right arrow key causes the set of columns to remain the same, but stretches the content horizontally. Repeatedly hitting the key causes more stretching until the window is filled with stretched slides; only *then* do the slides start to scroll in response to the right arrow key.

----

NOTE: If I move the browser window to a secondary monitor attached to my MacBook Pro, the interaction works in the manner I expect. So to reproduce the issue, you need to be looking at the builtin screen on your laptop (and it may be Mac OS X specific).

Comment 1

[Timothy Nikkel (:tnikkel)]

I couldn't reproduce this, on the builtin screen of a macbook pro on FF46, or nightly.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

I did some more experimentation, since I hadn't realized the problem might be hard to reproduce.

When my browser window is tall and narrow, the problem does not seem to occur.

If I make the window short and wide, the problem occurs. (Specifically, the scaling issue seems more pronounced the further I adjust the aspect ratio in that direction.)

I have made a youtube video showing the problem: https://youtu.be/ko7cTooGKGQ

In the video I use Firefox 45 and Firefox 46 at the same time, with Firefox 45 on the top and Firefox 46 on the bottom.

An even more troubling thing that may be due to the same root cause: after scrolling as far as possible across the columns to the right, the zoomed out view in Firefox 45 gets to "The End" of the presentation, while Firefox 46 is not rending the last few slides at the end. This is also demonstrated in the above video.

Comment 3

[Markus Stange [:mstange]]

I can reproduce this on my retina MBP screen in Nightly with e10s+APZ on.

Comment 4

[Markus Stange [:mstange]]

This is a regression from bug 1097464.

Comment 5

[Liz Henry (:lizzard)]

Regression from 46 but we are unlikely to fix this in 46, at this point. We could still take a fix in 47.

Comment 6

[Markus Stange [:mstange]]

Thinker, can you take a look?

Comment 7

[Thinker Li [:sinker]]

I don't see it on Linux with nightly.  I would look into it later.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This is not a new regression and now too late to fix in Fx47.

Comment 9

[Astley Chen (inactive)]

Tested with FF50 x64 on Window 10 and Ubuntu 16.04, no stretch issue while scrolling the slides.
However, the symptom still exists on MacOS 10.11 platform.

Thinker, could you help to look into it ?

Comment 10

[Thinker Li [:sinker]]

I can not reproduce it, now.  But, I will talk to you offline later.

Comment 11

[Thinker Li [:sinker]]

I have done some studies.  It seems weird that the content of layers are good, but one of layers is stretched on screen after compositing.  The matrix seems good too.  I need more time to understand it.

Comment 12

[Thinker Li [:sinker]]

I just find out the root cause.  Gecko tries to create an intermediate surface for the container of layers of slices.  It is just too big exceeding our max size of buffers.  So, the compositor create a smaller buffer, but layers don't know it.  When ContainerLayerComposite calls |DrawQuad|, it gives the compositor a size bigger than surface's real size, so that the layer is stretched by the compositor.

I try to pass the size of the intermediate surface to the compositor instead of the size expected by the layer, and it works correctly.

Comment 13

[Thinker Li [:sinker]]

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp?q=LayerManagerComposite.cpp&redirect_type=direct#281

The problem is here, clip is not propagated to the subtree for perspective or non-invertible.  It forces to create a big intermediate surface that system can not handle it or over our limits.

Comment 14

[Thinker Li [:sinker]]

Attachment: wip.diff

This patch leverages the fact of fixed z-value (0) of a plane to untransform clips with any transform matrix.  With this change, a smaller visible-regions is available.  But, untransfrom might scale up the bounds of a clip, a pre-scale should be applied to the intermediate buffer to gain better size fitting and less memory usage.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Bug 1253042 is probably a duplicate.

Comment 17

[Dan Callahan [:callahad]]

Definitely a dupe. Closed Bug 1253042. For reference, :CosminMCG narrowed the regression window to https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=37c7812ce0e6d10c7e7182f12e752832835e1d67&tochange=eb30e5ee32f0a11e3df594b5b3e69d0a5566bb63 in that bug.

Bumping severity and flagging as DevRel:P1 since this bug makes it significantly more difficult for Developer Relations to do our job. Specifically, we have to dance around our preferred presentation software to avoid triggering the bug while on stage.

Comment 18

[Thinker Li [:sinker]]

Attachment: CompositingRenderTarget-with-factor-WIP.diff

Applying this patch on the top of the previous patch would fix the problem for firefox on Linux.

This patch adds a scale-factor to CompositingRenderTarget to scale down content of an intermediate surface.  It allows intermediate surfaces to use smaller buffers.

This patch scales down content of an intermediate surface only if the transform of the container layer scale down the sub-tree.  It means the intermediate surface will be pre-scaled.

Comment 19

[Thinker Li [:sinker]]

Attachment: CompositingRenderTarget-with-factor-WIP-v2.diff

Add changes to scale clip and |mRenderOffset| for |CompositorOGL|.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8770364 [details] [diff] [review]
CompositingRenderTarget-with-factor-WIP-v2.diff

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

Can you explain more about why we need to do this?

ChooseScaleAndSetTransform is already meant to be pushing scaling down onto the leaves to do this, why isn't that sufficient here?

Comment 21

[Thinker Li [:sinker]]

Matt, Thanks!  It did not occur to me.  I think I would move all the code to ChooseScaleAndSetTransform.

Comment 22

[Thinker Li [:sinker]]

Matt, with a perspective transform, animations on the sub-tree may scaled to an unexpected size that is not easy to estimate it during layer building.  Do you have any idea about this?

Comment 23

[Thinker Li [:sinker]]

Forget the last question, I misunderstood something.

Comment 24

[Thinker Li [:sinker]]

Hi Matt,

If there is an animation on a layer with one or more perspective ancestor layers, the descendant layers with intermediate surfaces would be scaled up/down, and their visible rect might be bigger than expected exceeding the max size allowed to allocate from system.  Is it reasonable for you?

Comment 25

[Jet Villegas (inactive)]

Thinker: I'm marking this 'wontfix' for 48 but will review again after you get a patch with a r+.

Comment 26

[Thinker Li [:sinker]]

Thanks!  And, I will move back to this bug in next few days.

Comment 27

[Liz Henry (:lizzard)]

Too late for 49, heading into beta 8.  We could still take a patch for 50.

Comment 28

[Thinker Li [:sinker]]

Attachment: CompositingRenderTarget-with-factor-v3.diff

Comment 29

[Thinker Li [:sinker]]

Attachment: CompositingRenderTarget-with-factor-v4.diff

Comment 30

[Matt Woodrow (:mattwoodrow)]

(In reply to Thinker Li [:sinker] PTO during Sep 22 ~ 30 from comment #24)
> Hi Matt,
> 
> If there is an animation on a layer with one or more perspective ancestor
> layers, the descendant layers with intermediate surfaces would be scaled
> up/down, and their visible rect might be bigger than expected exceeding the
> max size allowed to allocate from system.  Is it reasonable for you?

Can you test what happens in this case?

I think we try allocate a texture of the maximum size, and downscale content to fit, but it's worth making sure.

Comment 31

[Dan Callahan [:callahad]]

Attachment: screenshot of offset rendering

Something seems to have regressed further in Nightly recently; now reveal.js slides are rendering way off-center (see screenshot) - unsure if it's related to this bug.

Comment 32

[Dan Callahan [:callahad]]

Attachment: screenshot of distorted rendering in slide overview

(Adding a screenshot of the existing bug for good measure)

Comment 33

[Thinker Li [:sinker]]

(In reply to Dan Callahan [:callahad] from comment #31)
> Created attachment 8795365 [details]
> screenshot of offset rendering
See bug 1302955.

Comment 34

[Milan Sreckovic [:milan] (needinfo for best results)]

Do we have a continuation of that patch?

Comment 35

[Ryan VanderMeulen (PTO, back July 27)]

Attachment: win10 screenshot showing missing content after hitting escape

On Win10, I can't reproduce the stretching effect. I can, however, see some intermittent drawing issues that go away as soon as I hit the arrow or switch focus between windows. See missing content on the lower-right side of the attached screenshot.

I'm also unable to reproduce the stretching effect with a 2013 Retina MBP with 10.12 installed on it, which is unfortunate :(

Comment 36

[Thinker Li [:sinker]]

I had told with Matt about this bug last week.  I will work on a new direction of solution, so it takes time for awhile.

Comment 37

[Thinker Li [:sinker]]

This bug occurs on Linux and MacOS.  Do any one check if it also occurs on Windows?

Comment 38

[Thinker Li [:sinker]]

Attachment: Implement physical and logical size for GL render target

Comment 39

[Ryan VanderMeulen (PTO, back July 27)]

(In reply to Thinker Li [:sinker] from comment #37)
> This bug occurs on Linux and MacOS.  Do any one check if it also occurs on
> Windows?

I couldn't reproduce on Win10 in comment 35. Hardware acceleration & e10s settings no effect.

Comment 40

[Thinker Li [:sinker]]

Comment on attachment 8813601 [details] [diff] [review]
Implement physical and logical size for GL render target

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

Hi Matt, this is the first part.

Comment 41

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8813601 [details] [diff] [review]
Implement physical and logical size for GL render target

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

Nice!

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +712,5 @@
>    mGLContext->fGenFramebuffers(1, aFBO);
>  }
>  
>  GLuint
> +CompositorOGL::CreateTexture(IntRect& aRect, bool aCopyFromSource, GLuint aSourceFrameBuffer)

I'd prefer an explicit extra outparam for the clamped 'physical' rect, rather than just modifying aRect in place. It's quite hard to tell at the caller that it's going to be modified.

Comment 42

[Matt Woodrow (:mattwoodrow)]

Does D3D11 need a similar fix?

Comment 43

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #42)
> Does D3D11 need a similar fix?

We don't clamp size for D3D11.  I am not sure if there is a fixed limitation for D3D11.

Comment 44

[Thinker Li [:sinker]]

Attachment: Implement physical and logical size for GL render target, v2

r=mattwoodrow

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0418e43a56a
Implement physical and logical size for GL render target. r=mattwoodrow

Comment 46

[Carsten Book [:Tomcat]]

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

Comment 47

[Ryan VanderMeulen (PTO, back July 27)]

I'm thinking this is probably too risky for uplift to 51 at this point, but maybe good for 52?

Comment 48

[Thinker Li [:sinker]]

Ryan, I think so.

Comment 49

[Thinker Li [:sinker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9a07013698c3e7fb5e0564407145ad4e4594d08 for 52.

Comment 50

[Thinker Li [:sinker]]

Comment on attachment 8814839 [details] [diff] [review]
Implement physical and logical size for GL render target, v2

Approval Request Comment
[Feature/Bug causing the regression]: https://bugzilla.mozilla.org/show_bug.cgi?id=1269337
[User impact if declined]: Users may get a stretched result for elements with perserve-3d.
[Is this code covered by automated tests?]: no, but confirmed with manual test.
[Has the fix been verified in Nightly?]: yes (for more than one month)
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no much risk
[Why is the change risky/not risky?]: The patch have been in m-c for over one month.  There seems good.  And, the changes affects no much code.
[String changes made/needed]:

Comment 51

[Julien Cristau [:jcristau]]

Comment on attachment 8814839 [details] [diff] [review]
Implement physical and logical size for GL render target, v2

fix for large gl render targets, aurora52+

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/62c5bb01119f