902505 - Work - Zoom in content remains blurry and doesn't get repainted at high-res

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

Description

[Brian R. Bondy [:bbondy]]

With the patch on bug 901339 applied, the focal point of zooming appears to be fixed and I can test this bug again. I don't see the same jumpiness as I was seeing before, but I think there is still something wrong. Specifically, after I zoom in the content remains blurry, and doesn't get repainted at high-res. This is usually a result of the setResolution not getting called, or called on a window where it has no effect. I'm not sure if you want to track that in this bug or in a new bug but it's definitely something that needs to be corrected.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: bug902505_zoominresolution.diff

I don't actually know how to reproduce what you said, but I think this will fix it.  Before we used to hardcode id 1, and we changed it to pass in scroll id instead.  If you can test to see if it fixes the problem, that would be awesome.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 787033 [details] [diff] [review]
bug902505_zoominresolution.diff

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

This doesn't fix the issue, and in fact brings back the problem with the jittery scroll :(

Comment 3

[Brian R. Bondy [:bbondy]]

hrm I wasn't getting jittery scrolling, strange.
I'm not sure what ele would be causing this, do you have any other ideas other than what you mentioned in comment 0?

Comment 4

[Brian R. Bondy [:bbondy]]

By the way the reasoning in Comment 1 shows that setResolution isn't actually being called, since we pass in the scrollId now instead of a hardcoded 1.

But after your testing you seen that calling setResolution actually doesn't fix the issue and does make it worse.

I think we do want to call setResolution as this patch does, but probably we need a second fix on top of that somewhere. (Where?). 

Also maybe the blurry thing you see needs an additional fix as well.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

You're right that the scrollId being passed in is not 1, I verified that by printing it. That means that setResolution is currently not being called at all, which explains the blurriness. Enabling a call to setResolution on the right presshell should fix the bluriness with no other effects, but determining the "right" presshell is the tricky part. In theory it should just be the content presshell but I *think* that doesn't work because of bug 732971. Or maybe it does work and there's just other code that needs fixing up somewhere. I'm not really sure. Once I get my surface pro set up to build and run code I'll investigate this a bit more.

Comment 6

[Brian R. Bondy [:bbondy]]

Awesome, thanks kats!

Comment 7

[Kartikaya Gupta (email:[e-mail])]

So I tried putting a call to setResolution in apzc.js, where it sends the Content:SetCacheViewport message. I tried calling setResolution on both the Browser.windowUtils and the windowUtils variable already present in that code, and both of those had the effect of (a) making the text sharp when zooming in, and (b) scaling up the UI elements and generally mucking up the positioning of the content window.

If you try this yourself to see what I mean by "text sharpening", be very gentle with zooming in - zoom in a little and give it a second to repaint, you will see the text becomes much sharper. For some reason I was also seeing it get stretched horizontally more and I'm not sure what that's about.

So this confirms my theory that you are being affected by bug 732971 or some variant thereof. Fixing that properly is probably a good idea now.

Comment 8

[Brian R. Bondy [:bbondy]]

great, thanks for looking into it!

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> So I tried putting a call to setResolution in apzc.js, where it sends the
> Content:SetCacheViewport message. I tried calling setResolution on both the
> Browser.windowUtils and the windowUtils variable already present in that
> code, and both of those had the effect of (a) making the text sharp when
> zooming in, and (b) scaling up the UI elements and generally mucking up the
> positioning of the content window.

So this doesn't seem to be happening any more with the latest code. It's a little concerning because something else might be going wrong now and I'm not sure what.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Set the resolution correctly

With my patches for bug 732971 applied, this patch makes FxMetro re-render content sharply after zooming. There are other issues that need to be sorted out though; for one thing this reveals a breakage that was probably introduced in bug 916125. I'm looking into that now.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

The problem I mentioned in comment 10 happens because metro isn't setting a CSS viewport on the content window. As a result, we end up setting a clip on the content which corresponds to the size of scrollport at a 1.0 resolution. Changing the resolution then results in the clip being visible. To fix this we need to either ifdef out the code at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=c38d71ed0463#2381 or implement meta-viewport support, or something.

Comment 12

[Matt Brubeck (:mbrubeck)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> The problem I mentioned in comment 10 happens because metro isn't setting a
> CSS viewport on the content window.

As a stop-gap until full meta-viewport is supported, we could explicitly set the CSS viewport to equal the window dimensions, which matches our current behavior (basically act like the viewport is always "width=device-width").  I think we could just have each tab call SetCSSViewport when its browser is created and whenever the main window is resized.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> The problem I mentioned in comment 10 happens because metro isn't setting a
> CSS viewport on the content window. As a result, we end up setting a clip on
> the content which corresponds to the size of scrollport at a 1.0 resolution.
> Changing the resolution then results in the clip being visible. To fix this
> we need to either ifdef out the code at
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp?rev=c38d71ed0463#2381 or implement meta-viewport
> support, or something.

If we are getting the wrong clip on the root scroll frame of the root content document when a css viewport is not set then doesn't that imply our condition for applying that clip is wrong? ie shouldn't the clip be correct is there is no css viewport set?

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: Call SetCSSViewport

Implements my suggestion from comment 12.  Seems to fix the issue in comment 10.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #13)
> If we are getting the wrong clip on the root scroll frame of the root
> content document when a css viewport is not set then doesn't that imply our
> condition for applying that clip is wrong? ie shouldn't the clip be correct
> is there is no css viewport set?

I was wondering this as well, and for a while I was thinking that instead of checking GetIsViewportOverridden() as the condition, we should be checking GetXResolution() != 1.0 || GetYResolution() != 1.0. That is, the clip should only be set on that document if the resolution is 1.0. But I'm not sure that's right either - I feel like it's one step more removed than it should be.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> I was wondering this as well, and for a while I was thinking that instead of
> checking GetIsViewportOverridden() as the condition, we should be checking
> GetXResolution() != 1.0 || GetYResolution() != 1.0. That is, the clip should
> only be set on that document if the resolution is 1.0. But I'm not sure
> that's right either - I feel like it's one step more removed than it should
> be.

I would think the clip is adjusted based on the resolution, so if it works at resolution 1 I would expect it to work at resolution != 1.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 823499 [details] [diff] [review]
Set the resolution correctly

This sets the resolution on the content presShell when the scrollId refers to the root content element.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #16)
> 
> I would think the clip is adjusted based on the resolution, so if it works
> at resolution 1 I would expect it to work at resolution != 1.

I think if we were to use the scroll-position-clamping-scroll-port-size that would be true, because that is adjusted based on the resolution.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 823499 [details] [diff] [review]
Set the resolution correctly

This browser binding handler is going to go away soonish, I've got a wip that moves this all down to widget and calls UpdateRootFrame, which it appears also makes this resolution call. In the mean time, no harm in landing this front end fix.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

There's still one more issue I'm seeing with these patches - it seems like when zoomed in the page is moving faster than my finger. Trying to debug that now.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: Use the right transform from one layer to its parent

After spending all day chasing down that scrolling issue, I realized it wasn't a display problem at all but rather an input problem. The input events were getting transformed incorrectly before being handed to the APZC instance, which is why it was scrolling faster than my finger. With a lot of help from matt woodrow I have this patch that fixes the problem. I will test it on B2G to ensure it doesn't regress anything there and then flag it for review. The three patches on this bug combined with the stuff from bug 732971 (currently on inbound) fix all the zooming bluriness in Metro and don't seem to introduce any regressions.

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> (In reply to Timothy Nikkel (:tn) from comment #16)
> > 
> > I would think the clip is adjusted based on the resolution, so if it works
> > at resolution 1 I would expect it to work at resolution != 1.
> 
> I think if we were to use the scroll-position-clamping-scroll-port-size that
> would be true, because that is adjusted based on the resolution.

The clip should get scaled by the resolution automatically. The clip is in app units, and when it gets converted to pixels it should take into account the resolution.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #22)
> The clip should get scaled by the resolution automatically. The clip is in
> app units, and when it gets converted to pixels it should take into account
> the resolution.

Right, I keep forgetting that! Ok, so since the resolution is already accounted for, we shouldn't need the resolution anywhere in the condition. And since you can set the CSS viewport to be exactly whatever the default CSS viewport is (and there should be no changes in behaviour), the fact that the CSS viewport is overridden should not affect the condition. What does that leave?

I think maybe we do always want to be setting the clip, it's just that clip value is wrong when dealing with these zooming scenarios. The mScrollPort is approximately the CSS viewport, which is not really the right value to be using because it's not the same as the visible area on these mobile platforms.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 824407 [details] [diff] [review]
Use the right transform from one layer to its parent

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

Unfortunately this patch breaks behaviour on B2G. Sigh.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Combined with a tentative patch for bug 932525 I think I might have this working. Will spend some more time cleaning it up and testing.

Comment 26

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> I think maybe we do always want to be setting the clip, it's just that clip
> value is wrong when dealing with these zooming scenarios. The mScrollPort is
> approximately the CSS viewport, which is not really the right value to be
> using because it's not the same as the visible area on these mobile
> platforms.

If the CSS viewport is overridden then yes I agree that mScrollPort is not the right clip.

But are we still talking about the case where the CSS viewport takes the default value?

Comment 27

[Kartikaya Gupta (email:[e-mail])]

I'm forking this discussion about the clip to bug 933264. While trying to wrap my head around the pre- and post-scaling I found a test case where the clip is broken.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix hit testing and input transformation

Updated patch based on the week-and-a-half of teeth-gnashing and trying to figure out what the hell is going on. Pieces of the discussion can be found scattered on IRC logs, bugs, pieces of paper, etc. The most recent understanding is on bug 935219.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix hit testing and input transformation

Slightly updated to remove unnecessary changes, and added some documentation. Requesting feedback for now; will upgrade to a review request once I re-test on metro.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 828153 [details] [diff] [review]
Fix hit testing and input transformation

This works as expected.

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 828153 [details] [diff] [review]
Fix hit testing and input transformation

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

Looks good!

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +653,5 @@
> +  //   and RC.Inverse() * QC.Inverse() * PC.Inverse() * PA.Inverse()                at P.
> +  gfx3DMatrix childUntransform = ancestorUntransform
> +                               * aApzc->GetCSSTransform().Inverse()
> +                               * gfx3DMatrix(aApzc->GetCurrentAsyncTransform()).Inverse();
> +  gfxPoint hitTestPointForChildLayers = childUntransform.ProjectPoint(aHitTestPoint);

Would it be faster (and less repetitive) to just multiply hitTestPointForThisLayer by aApzc->GetTransientAsyncTransform().Inverse()? (Oh, just realized there is no GetTransientAsyncTransform(). We could add one; I don't feel strongly about it either way.)

@@ +694,5 @@
> +        R's nontransient async transform    (hereafter referred to as transform matrix RN)
> +        R's CSS transform                   (hereafter referred to as transform matrix RC)
> +
> +   Also, for any layer, the async transform is the combination of the transient and non-transient
> +   parts of the async transform. That is, for any layer L:

suggested wording: "the async transform is the combination of its transient and non-transient parts"

@@ +744,5 @@
>     combine it with aTransformToApzcOut to get the final transform required in this case.
>  
>     Note that for many of these layers, there will be no AsyncPanZoomController attached, and
>     so the async transform will be the identity transform. So, in the example above, if layers
>     L and P have APZC instances attached, MA, NA, OA, QA, and RA will be identity transforms.

You might want to say here instead that each of MT, MN, etc. are identity transforms, since technically MA could be the identity transform without MT and MN being identity transforms.

@@ +745,5 @@
>  
>     Note that for many of these layers, there will be no AsyncPanZoomController attached, and
>     so the async transform will be the identity transform. So, in the example above, if layers
>     L and P have APZC instances attached, MA, NA, OA, QA, and RA will be identity transforms.
>     Additionally, for space-saving purposes, each APZC instance stores its layers individual

layer's

Comment 32

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #31)
> Would it be faster (and less repetitive) to just multiply
> hitTestPointForThisLayer by aApzc->GetTransientAsyncTransform().Inverse()?
> (Oh, just realized there is no GetTransientAsyncTransform(). We could add
> one; I don't feel strongly about it either way.)

It would be marginally faster but other than having to add GetTransientAsyncTransform() my main concern is actually precision/accuracy. I feel like pushing the point through two transforms instead of one is likely to compound floating point errors, although that's more of a gut feeling than based on analysis.

> > +   Also, for any layer, the async transform is the combination of the transient and non-transient
> > +   parts of the async transform. That is, for any layer L:
> 
> suggested wording: "the async transform is the combination of its transient
> and non-transient parts"

Will fix.

> @@ +744,5 @@
> >     Note that for many of these layers, there will be no AsyncPanZoomController attached, and
> >     so the async transform will be the identity transform. So, in the example above, if layers
> >     L and P have APZC instances attached, MA, NA, OA, QA, and RA will be identity transforms.
> 
> You might want to say here instead that each of MT, MN, etc. are identity
> transforms, since technically MA could be the identity transform without MT
> and MN being identity transforms.

Good point, will fix.

> >     Additionally, for space-saving purposes, each APZC instance stores its layers individual
> 
> layer's

Whoops, will fix.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Made the above changes and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/14366dd910b6

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Also the CSS viewport patch:

https://hg.mozilla.org/integration/fx-team/rev/a562387a27bb

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-mc failures.
https://hg.mozilla.org/integration/fx-team/rev/23f887e36a37

https://tbpl.mozilla.org/php/getParsedLog.php?id=30343906&tree=Fx-Team

Comment 36

[Matt Brubeck (:mbrubeck)]

Most of the test failures are caused by a typo (Elements.browser should be Elements.browsers).  However, the failures in browser_snappedState.js are not related to that typo.  Working on this now.

Comment 37

[Matt Brubeck (:mbrubeck)]

Fixed the test failures, verified that tests are green locally, and re-landed:
https://hg.mozilla.org/integration/fx-team/rev/e49fc0c216b6

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14366dd910b6

Comment 39

[Matt Brubeck (:mbrubeck)]

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