Distinguish between CSSPixel and "device pixel" for Fennec

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Depends on 1 bug)

23 Branch
mozilla24
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:hd+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 wontfix, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(12 attachments, 5 obsolete attachments)

6.35 KB, patch
ajones
: review+
Details | Diff | Splinter Review
6.45 KB, patch
ajones
: review+
Details | Diff | Splinter Review
4.36 KB, patch
ajones
: review+
Details | Diff | Splinter Review
4.03 KB, patch
ajones
: review+
Details | Diff | Splinter Review
64.29 KB, patch
ajones
: review+
Details | Diff | Splinter Review
4.86 KB, patch
ajones
: review+
BenWa
: review+
Details | Diff | Splinter Review
1.47 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
82.40 KB, image/png
Details
1.60 KB, patch
kats
: review+
Details | Diff | Splinter Review
25.25 KB, patch
Details | Diff | Splinter Review
91.74 KB, image/png
Details
141.72 KB, image/png
Details
+++ This bug was initially created as a clone of Bug #880676 +++

After implementing bug 880676 and poking around the code, I have come to the conclusion that on Fennec, we have been conflating CSS pixels with "device pixels" (as layout calls them). On Fennec they have always been equivalent because we our nsIWidget::GetDefaultScale() always returns 1.0, and we never apply a "full zoom" pixel scale. So we've been able to get away with this so far. However, my attempts to further propagate the new templated gfx classes show that we'll need to fix this to make things work properly. Fixing this should also allow us to fix bug 803207 and various other related bugs.

My plan for fixing this is involves introducing a new pixel type in layout/base/Units.h. I'm considering calling this "DevicePixel" since that's what layout refers to it as, but that is very ambiguous so I'm open to suggestions on the name. I'm also considering "LayoutDevicePixel" but that's a little verbose. This new pixel type and associated scaling factors will need to be propagated to various parts of the code, in some cases replacing what I had already typed as CSSPixel.
LayoutDevicePixel sounds OK to me.
WIP of the changes; this patch needs to be broken up a bit into multiple independent patches. Putting it up anyway for early feedback. I tested this on a B2G device and it seems to work ok after I added the changes to APZC::OnScale and TabChild::HandlePossibleViewportChange.
I cleaned up the patches somewhat and added more stuff. Try build with the updated queue at https://tbpl.mozilla.org/?tree=Try&rev=88c1e5fddadc. There's some more things I want to test before putting them here and requesting review.
This patch has some functional changes for devices where mDevPixelsPerCSSPixel is not 1.0. These corrections fell out of part 4 in this patch queue, but I'm doing this first so that this corrective patch can be more easily uplifted to other branches if necessary.
Attachment #764237 - Attachment is obsolete: true
Attachment #764866 - Flags: review?(ajones)
Personally I find storing mZoom pretty asinine because it always has to be multiplied by the intrinsic scale to get anything useful. I plan to change this in a future bug but for now it's simpler to leave it as-is.

Note that code I'm killing in TabChild.cpp here was originally copied from Fennec, circa [1]. The hysteresis thing is not actually hysteresis, it accounts for when the device is rotated and the screen width changes. With the APZC it is not needed because the intrinsic scale automatically changes when the device rotates, and since the zoom is based relative to that it doesn't need updating. I tested the new code on an unagi device and it behaves correctly.

[1] http://hg.mozilla.org/mozilla-central/file/c12150cfdfef/mobile/android/chrome/content/browser.js#l3756
Attachment #764870 - Flags: review?(ajones)
I don't know what this was doing before but I can't believe it ever worked reasonably. Also I think the front-end code that picks out elements to zoom to doesn't work, because I never got anything but an empty rect coming in to this function.
Attachment #764872 - Flags: review?(ajones)
I abbreviated this to "Layout" in the ScaleFactor typedefs to reduce verbosity but I can change that if you prefer something else.
Attachment #764235 - Attachment is obsolete: true
Attachment #764873 - Flags: review?(ajones)
Conceptually a ViewTransform is actually from LayoutDevicePixels to ScreenPixels, so I had to update this accordingly.
Attachment #764877 - Flags: review?(bgirard)
Attachment #764877 - Flags: review?(ajones)
There might be other fixes that need to go in for fixed position layers and such, but I found this one while trying to fix bug 803207.
Attachment #764945 - Flags: review?(chrislord.net)
Comment on attachment 764866 [details] [diff] [review]
Part 1 - Apply the mDevPixelsPerCSSPixel scaling factor to a few missing spots

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +395,5 @@
>      // This is derived from the code in
>      // gfx/layers/ipc/CompositorParent.cpp::TransformShadowTree.
>      const gfx3DMatrix& rootTransform = GetRoot()->GetTransform();
> +    CSSToLayerScale paintScale(metrics.mDevPixelsPerCSSPixel / rootTransform.GetXScale(),
> +                               metrics.mDevPixelsPerCSSPixel / rootTransform.GetYScale());

Suggestion: It would be nice to factor out the float maths here. mDevPixelsPerCSSPixel could be of type CssToDevScale and we could have a method to create DevToLayerScale from a gfx3DMatrix.
Attachment #764866 - Flags: review?(ajones) → review+
Comment on attachment 764870 [details] [diff] [review]
Part 2 - Fix zoom calculations in APZC

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

The code that didn't make any sense won't be missed. Ding dong the witch is dead.
Attachment #764870 - Flags: review?(ajones) → review+
Comment on attachment 764872 [details] [diff] [review]
Part 3 - Fix ZoomToRect calculations in APZC

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1318,2 @@
>      localMinZoom = std::max(localMinZoom, mMinZoom);
> +    localMinZoom /= intrinsicScale;

I don't like reusing the variable here because each statement changes the meaning of the variable.

@@ +1321,1 @@
>  

Observation: mFrameMetrics.mZoom is on a different scale to mMinZoom and mMaxZoom in that mZoom has the intrinsic scale applied. This patch doesn't make it worse but it also doesn't make it any better.
Attachment #764872 - Flags: review?(ajones) → review+
Comment on attachment 764873 [details] [diff] [review]
Part 4 - Add the LayoutDevicePixel types

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

I favour consistency over brevity. I'd suggest choosing either Layout or LayoutDevice rather than using both but I don't feel strongly about it.
Attachment #764873 - Flags: review?(ajones) → review+
Comment on attachment 764874 [details] [diff] [review]
Part 5 - Convert various things to use the new type

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

::: layout/base/Units.h
@@ +56,5 @@
>  typedef gfx::ScaleFactor<LayoutDevicePixel, LayerPixel> LayoutToLayerScale;
>  typedef gfx::ScaleFactor<LayerPixel, LayoutDevicePixel> LayerToLayoutScale;
>  typedef gfx::ScaleFactor<LayerPixel, ScreenPixel> LayerToScreenScale;
>  typedef gfx::ScaleFactor<ScreenPixel, LayerPixel> ScreenToLayerScale;
> +typedef gfx::ScaleFactor<ScreenPixel, ScreenPixel> ScreenToScreenScale;

Doesn't this indicate that we've got yet another co-ordinate space we're not accounting for properly?
Attachment #764874 - Flags: review?(ajones) → review+
Attachment #764877 - Flags: review?(ajones) → review+
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #13)
> Suggestion: It would be nice to factor out the float maths here.
> mDevPixelsPerCSSPixel could be of type CssToDevScale and we could have a
> method to create DevToLayerScale from a gfx3DMatrix.

Agreed. The first part of this suggestion is done in part 4; I can do the second part as a follow-up somewhere.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #15)
> >      localMinZoom = std::max(localMinZoom, mMinZoom);
> > +    localMinZoom /= intrinsicScale;
> 
> I don't like reusing the variable here because each statement changes the
> meaning of the variable.

Ok, I'll just squash those two lines into one. It's not that complicated anyway.

> Observation: mFrameMetrics.mZoom is on a different scale to mMinZoom and
> mMaxZoom in that mZoom has the intrinsic scale applied. This patch doesn't
> make it worse but it also doesn't make it any better.

Yes, this is true. I plan to fix this up in bug 885023.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #16)
> I favour consistency over brevity. I'd suggest choosing either Layout or
> LayoutDevice rather than using both but I don't feel strongly about it.

Ok, I'll use LayoutDevice everywhere then.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #17)
> >  typedef gfx::ScaleFactor<ScreenPixel, LayerPixel> ScreenToLayerScale;
> > +typedef gfx::ScaleFactor<ScreenPixel, ScreenPixel> ScreenToScreenScale;
> 
> Doesn't this indicate that we've got yet another co-ordinate space we're not
> accounting for properly?

Sort of. It's an artificial coordinate space that's created by the way mZoom is defined in FrameMetrics. It's silly and I will fix this up as part of bug 885023.
Sweet. I figured that you were on to it. Keep up the good work :-)
On my Peak, this patch set crams everything into the upper-left-hand corner.

Let me know if you need more information/testing.
Comment on attachment 764945 [details] [diff] [review]
Part 7 - Fix the critical display port so it works on hidpi Fennec

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

LGTM.
Attachment #764945 - Flags: review?(chrislord.net) → review+
Attachment #764877 - Flags: review?(bgirard) → review+
Using mwu's suggestion of forcing the widget-scale to 1.5 I was able to reproduce a similar problem as comment 22 on my unagi. The attached patch fixes the problem for me, I think (it's a bit hard to know what's expected behaviour when running with a 1.5 scale on an unagi). :ekr, if you have some time, can you try applying this patch on top of everything else to see if it fixes the problem for you?
Attachment #765521 - Flags: review?(ajones)
Attachment #765521 - Flags: feedback?(ekr)
Comment on attachment 765521 [details] [diff] [review]
Follow-up for peak/B2G-hidpi devices

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

This appears to substantially improve the situation. The  image is no longer crammed into the corner and links now work bett
though not perfectly.
Attachment #765521 - Flags: feedback?(ekr) → feedback+
Attachment #765521 - Flags: review?(ajones) → review+
(In reply to Ryan VanderMeulen [:RyanVM][UTC-4] from comment #26)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> > Updated try run at https://tbpl.mozilla.org/?tree=Try&rev=b4240dd9d259
> 
> Funny story, this caused Linux reftest failures. Backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3a96c5cf458c
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=24397393&tree=Mozilla-
> Inbound

Dangit. At least this is better than Android intermittents. I suspect this might actually be fixed by the follow-up patch I put together for the peak, since from the looks of the test it might exercise the same code path and the reftest analyzer shows similar behaviour. Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=3056b44876ab
Depends on: 885709
The follow-up patch fixed those two reftest failures but caused another one, test-displayport-2.html. The reftest appears to be an inequality test, so the test page is supposed to only paint the displayport size (200x200) but instead the whole page (800x1000) is green. I think. (The reftest analyzer isn't working properly on this failure).
I was able to reproduce the reftest failure locally and believe it is caused by the mZoom variable being set improperly in some cases. Specifically, at [1] mZoom is assigned from TabChild::GetZoom, which returns the zoom in the mLastMetrics that the TabChild holds. However that mLastMetrics is only initialized properly "if AsyncPanZoomEnabled()" [2] which appears to not be the case in the reftest-ipc suite. So this scenario should never actually get hit in a non-test environment.

That being said, I was able to work around the failure by modifying my patch in comment 24 to initialize userZoom a little differently. The attached patch has the fix; the calculation is almost equivalent to the previous one except that it doesn't use the mZoom variable from the frame metrics. Carrying r+ from previous version.

Try build (a full one this time) at https://tbpl.mozilla.org/?tree=Try&rev=6f9f150fd986 shows greenness.

[1] https://hg.mozilla.org/integration/mozilla-inbound/file/2272804a8e71/layout/base/nsDisplayList.cpp#l670
[2] https://hg.mozilla.org/integration/mozilla-inbound/file/2272804a8e71/dom/ipc/TabChild.cpp#l330
Attachment #765521 - Attachment is obsolete: true
Attachment #766040 - Flags: review+
kats: I just pulled m-c and it's better but still pretty iffy.... Like sometimes I get clicks and sometimes I don't. Anything I can do to help debug?
kats: I just pulled m-c and it's better but still pretty iffy.... Like sometimes I get clicks and sometimes I don't. Anything I can do to help debug?
(Forgot to remove the [leave open] when I re-landed everything, so closing this bug out now).

:ekr - If you can find reproducible scenarios where the clicks don't work please file a bug with STR and CC me. Alternatively if you're willing to do a bit of debugging it would be helpful to stick a printf_stderr at say [1] that prints out the tap point's x and y coordinates. Then, which clicks don't work, see if that printf is getting hit by looking at the logcat. If it is getting hit then the clicks are being delivered by APZC as intended and the problem is in in the front-end; if it is not getting hit then the problem is in the gesture detector and/or APZC. That will help narrow it down, since there are lots of places where clicks could get dropped.

[1] http://hg.mozilla.org/mozilla-central/file/50332b66c7a1/gfx/layers/ipc/AsyncPanZoomController.cpp#l672
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla24
Nominating for hd? since we need this to fix the browser on 1.1hd.
blocking-b2g: --- → hd?
Just to note, there are still similar issues with link highlighting (i.e. the scale isn't taken into account and you end up highlighting things 1/3 towards the top-left corner from where you pressed) - I'll file a separate bug for this.
blocking-b2g: hd? → hd+
Waiting for mwu to add the peak manifest to v1-train so I can build and test this patch.
Flags: needinfo?(mwu)
Working on building this now
Flags: needinfo?(mwu)
This one compiles, still testing
Attachment #778616 - Attachment is obsolete: true
Ok, so the above backport compiles and runs fine but frankly I'm not seeing anything that was broken without the patch that is fixed with the patch. There's some stuff that is broken both with and without the patch, and for me at least clicking on links in the browser works both with and without the patch. So I'm not sure exactly what is fixed by these patches that needs backporting.

:mwu, can you elaborate on comment 35?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #40)
> Ok, so the above backport compiles and runs fine but frankly I'm not seeing
> anything that was broken without the patch that is fixed with the patch.
> There's some stuff that is broken both with and without the patch, and for
> me at least clicking on links in the browser works both with and without the
> patch. So I'm not sure exactly what is fixed by these patches that needs
> backporting.
> 
> :mwu, can you elaborate on comment 35?

If we have bug parity without backporting this, that's fine. I was under the impression that this fixed many issues on the peak.
An update - kats and I determined that some other parts of the patch series also need to be backported.

Updated

6 years ago
Duplicate of this bug: 897688

Updated

6 years ago
Duplicate of this bug: 895896
Hi! 

Just for your information.
I applied the "backport" patch on Helix device, but doesn't work.

--
Keven

Comment 46

6 years ago
Shouldn’t this be reopened then?
(In reply to Matěj Cepl from comment #46)
> Shouldn’t this be reopened then?

No. The backport patch is incomplete, but the fix on trunk is fine.
Ok, so here's a somewhat more comprehensive backport of these patches. This builds and runs and seems to behave sanely for me, but I wasn't really reproducing the bustedness that everybody else is seeing so I can't say that it actually fixes stuff. Can somebody please try with this patch and see if it improves any brokenness?
Attachment #778638 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> Created attachment 784553 [details] [diff] [review]
> Backported "device pixel" fixes (v2)
> 
> Ok, so here's a somewhat more comprehensive backport of these patches. This
> builds and runs and seems to behave sanely for me, but I wasn't really
> reproducing the bustedness that everybody else is seeing so I can't say that
> it actually fixes stuff. Can somebody please try with this patch and see if
> it improves any brokenness?

Thanks !

Just tested on Nexus S with b2g18hd, and it's a bit better than in the previous cases. However, there is still two issues:
 - full width is not used
 - height seems to be bogus

I'll upload screenshots right away.
Please find attached a first picture of the website http://mobile.lemonde.fr/ that demonstrates the issue. This one is after loading, screen not yet touched.
Please find attached a second screenshot of the same website, after panning a bit.
Flags: needinfo?(bugmail.mozilla)
I can try the backport patch on Helix device.

--
Keven
Hi!

I got the same issue as Alexandre reported on Helix device.

--
Keven
(In reply to Alexandre LISSY :gerard-majax from comment #50)
> Created attachment 784622 [details]
> LeMonde.fr on Nexus S
> 
> Please find attached a first picture of the website
> http://mobile.lemonde.fr/ that demonstrates the issue. This one is after
> loading, screen not yet touched.

This I think is related to the fact that lemonde.fr has a meta-viewport tag. Pages that don't have a meta-viewport seem to render fine, and that's what i was testing on. I'll see if I can find the relevant code on the HD branch and see what it's doing.

(In reply to Alexandre LISSY :gerard-majax from comment #51)
> Created attachment 784624 [details]
> LeMonde.fr on Nexus S after panning
> 
> Please find attached a second screenshot of the same website, after panning
> a bit.

That's just weird. I don't know what would cause this except maybe a typo in my code somewhere. I'll keep an eye out for it.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54)
> This I think is related to the fact that lemonde.fr has a meta-viewport tag.
> Pages that don't have a meta-viewport seem to render fine, and that's what i
> was testing on. I'll see if I can find the relevant code on the HD branch
> and see what it's doing.

I think I fixed this on m-c in bug 879004 (patch part 3, the change to nsDisplayList.cpp where I change it mViewport from being a device pixel calculation to a CSS pixel calculation). I'm backporting that change too to see if that fixes this. I'm also backporting some of the other fixes in that bug.
Thanks, I'll test as soon as I can :)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #55)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54)
> > This I think is related to the fact that lemonde.fr has a meta-viewport tag.
> > Pages that don't have a meta-viewport seem to render fine, and that's what i
> > was testing on. I'll see if I can find the relevant code on the HD branch
> > and see what it's doing.
> 
> I think I fixed this on m-c in bug 879004 (patch part 3, the change to
> nsDisplayList.cpp where I change it mViewport from being a device pixel
> calculation to a CSS pixel calculation). I'm backporting that change too to
> see if that fixes this. I'm also backporting some of the other fixes in that
> bug.

After applying this backport and the one for bug 879004, I no longer have the behavior described in comment #50. However, comment #51 still applies.

Please note that if I rotate my device and that I scroll, the issue described in comment #51 is « fixed ».
Let me be clear, it's fixed as long as you stay in landscape orientation. If you revert to portrait, it's back.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54)
> (In reply to Alexandre LISSY :gerard-majax from comment #51)
> > Created attachment 784624 [details]
> > LeMonde.fr on Nexus S after panning
> > 
> > Please find attached a second screenshot of the same website, after panning
> > a bit.
> 
> That's just weird. I don't know what would cause this except maybe a typo in
> my code somewhere. I'll keep an eye out for it.

So with the backport patches from this bug and from bug 879004, the browser works fine for me on the v1.1.0hd branch on a Helix device. Clicking on links, panning, and zooming all works well, and I don't see the stretching problem at all.

Keven, since you were able to reproduce the problem on a Helix device, can you tell me which (a) system image, (b) gecko changeset and (c) gaia changeset you used?

I'm using the 20130805 image from the Helix intranet page, with Gecko changeset bfb440d5c23e7c7a (plus my two backport patches) and Gaia changeset 64f48c2ceb.
Flags: needinfo?(kkuo)
Hi! Kats,

After patched backport patches, 883646 and 879004, the browser works.
The issue I reported at comment 53 is with patch 883646 only. No 879004 applied.

--
Keven
Flags: needinfo?(kkuo)

Comment 62

6 years ago
Build 20130808003842 on Geeksphone Peak:

+ zoom in/out: works fine
+ URLs: works fine
+ scrolling to the end of the page: works fine

- some pages won't use the whole width after loading but if you click on an URL and go back they will fit perfectly
- while scrolling the screen sometimes turns black for the fraction of a second

On the whole: A great progress. Thanks a lot!
Alright then, I don't think there's anything more I can do here. The backport patches on bug 879004 and this bug (bug 883646) can be uplifted to the HD branch. Do they need to be r+'d or can they just be landed?
Flags: needinfo?(ryanvm)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63)
> Do they need to be r+'d or can they just be landed?

That's really a call for you and the module owners. I'm happy to land whenever.
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.