1009306 - Visible seams on the page (maybe at tile borders?)

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect, P5)

Description

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Screenshot showing the drawing issue

I am seeing one pixel drawing errors that look to happen in vertical columns. See attached screenshot.

Comment 1

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

Is there a specific device/version/URL you're seeing this on?

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

I have been using a Nexus 5. I suspect that this is a rounding error at some specific zoom level as I have only seen it on sites that allow pinch zooming. Reddit in the screenshot http://reddit.com/r/android or http://news.ycombinator.comu

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Screenshot from Galaxy Nexus

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

I spent some time looking at this and can see it on 30 and newer. I might have a more consistent test case.

Comment 5

[Aaron Train [:aaronmt]]

Reproducible. I see both bug 1014333 and this bug.

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d01bf8596d3b&tochange=21f293fc8d34

b2g-tiling work

Test-URL: http://www.reddit.com/r/askscience/comments/28vajr/if_i_had_a_sealed_room_filled_with_butane_gas_and/

Milan, like bug 1014333 we need action here.

Comment 6

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

Negotiating for Chris' time.

Comment 7

[Chris Lord [:cwiiis]]

Like in bug 1014333, which I think is a dupe of this, I'm not sure this is easily fixable, or indeed where I'd even start if I wanted to fix this.

I'll spend some more time with it today, but I'd suggest that someone more familiar with the rasterisation part of Gecko would be a better candidate to fix this.

Comment 9

[Chris Lord [:cwiiis]]

Trying to be more methodical about this.

My hypothesis is that this is the result of a rounding error in Gecko during progressive painting somewhere. The alternative hypothesis would be that Cairo has rounding errors or accuracy issues, but this would not be fixable so I'm going with the former until I've exhausted the possibilities.

I don't think this error is introduced when copying buffer contents into tiles - these errors would be more visible I believe, but also when the zoom is stable, mResolution is 1.0 and so there is no place to introduce rounding errors (I've checked this).

Another place rounding error could be introduced is in ClientTiledLayerBuffer::PaintThebes, which calls Scale, then translates by the bounds origin of the paint region. This theory should be easily testable by always over-allocating this region and removing the translate. Attempting this now.

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #9)
> Trying to be more methodical about this.
> 
> My hypothesis is that this is the result of a rounding error in Gecko during
> progressive painting somewhere. The alternative hypothesis would be that
> Cairo has rounding errors or accuracy issues, but this would not be fixable
> so I'm going with the former until I've exhausted the possibilities.
> 
> I don't think this error is introduced when copying buffer contents into
> tiles - these errors would be more visible I believe, but also when the zoom
> is stable, mResolution is 1.0 and so there is no place to introduce rounding
> errors (I've checked this).
> 
> Another place rounding error could be introduced is in
> ClientTiledLayerBuffer::PaintThebes, which calls Scale, then translates by
> the bounds origin of the paint region. This theory should be easily testable
> by always over-allocating this region and removing the translate. Attempting
> this now.

Actually, the comment about mResolution applies in this case too - and all the same, always over-allocating does not fix the issue, so the error doesn't appear to be here.

It just seems that combining old rendering with new rendering causes sub-pixel offsets that introduce these rendering errors.

It feels like this is a layout issue either in the ScrollBy functions, which should be scrolling to pixel-aligned value, or in the invalidation code that should be invalidating layers when their sub-pixel scroll offset changes - or both (I guess if the former worked, the latter wouldn't be necessary(?))

What makes this kind of hard to test is that without progressive rendering, these regions don't have irregular overlaps, so you would see fewer 'seams'. Next, I'll see if disabling render cancelling causes the incidence of these rendering issues to decrease - if so, it would point to this being a layout issue, otherwise it would point to this being an issue either still in the tiling code somewhere, or in cairo.

Comment 11

[Chris Lord [:cwiiis]]

Always over-allocating the paint buffer and disabling cancelling, I can still easily reproduce this - I'm fairly certain this is a layout issue at this point. I'm going to see if I can reproduce with progressive painting off entirely now (I've figured out some better, more consistent STR).

Comment 12

[Chris Lord [:cwiiis]]

I can easily reproduce this with both progressive paint and the low precision buffer turned off.

STR:

1- Go to a long Reddit comment thread, like http://www.reddit.com/r/askscience/comments/25mmxp/in_a_loaded_bike_wheel_which_spokes_are_under_the/ in the b2g browser, or in fennec (the bug reproduces in either - ftr, I'm testing in b2g)

2- Scroll down and zoom far into a comment box so that the border of the comment box is in the center of the screen and the scroll position is at the left edge of the page. You should zoom in far enough that the border takes a pixel or two on the screen (ftr, I zoom in so that about 6 lines of text fit vertically on the screen)

3- Scroll about two screens to the right.

4- Scroll down a little so that the border is still easily visible but the vertical scroll position has changed

5- Scroll back to the left again

At this point, you should see that the border thickness changes along the line. If you don't, just repeat steps 3-5 until it does (but this happens pretty much 100% of the time for me).

There are two errors happening here, as commented on in paragraph 3 of comment #10.

n?mattwoodrow to provide some layout/invalidation insight.

Comment 13

[Chris Lord [:cwiiis]]

mstange on IRC points to this #ifdef, which I mistakenly thought was removed: http://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1565

Also, that ClampAndAlignWithPixels knows about layer resolution, so something may be going wrong there.

These are two good pointers that I think I can continue fruitfully investigating with - I'll post anything I find.

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #13)
> mstange on IRC points to this #ifdef, which I mistakenly thought was
> removed:
> http://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.
> cpp#1565

If I remove this #ifndef and the related #ifndef's above it, I can no longer reproduce this error.

It seems whole-layer invalidations happen any time your scrolling takes you to the maximum scroll extent of the frame, which makes sense (if you want to be able to scroll to the absolute edge, you're probably going to lose your sub-pixel alignment - I think we may want to revisit this though, I think it would be ok to have a sub-pixel area you can't scroll to).

I think it would be reasonable to just remove these #ifndef's, but we may also want to change the above behaviour, at least for mobile. Layout comment on this would be valuable.

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Allow invalidation when a layer's sub-pixel scroll position changes

This fixes the issue, but leaves us with the issue that we have whole-layer invalidations when you hit scroll extents.

Comment 16

[Chris Lord [:cwiiis]]

Perhaps an alternative fix for the remaining issue, rather than changing the flexible scroll functions, is to make sure that our scroll-clamping-scroll-port-size takes this into account?

Comment 17

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #16)
> Perhaps an alternative fix for the remaining issue, rather than changing the
> flexible scroll functions, is to make sure that our
> scroll-clamping-scroll-port-size takes this into account?

Looking at where this actually gets set, I'm not sure how it could take it into account - it already starts in ParentLayer coordinates, so I'm not sure where the subpixel difference ends up coming from... Rounding error? Oversizing the scroll clamping scroll port size by an arbitrary amount has odd effects, so that isn't an option it seems.

Comment 18

[Chris Lord [:cwiiis]]

I've had no luck so far in trying to force Gecko to always align to layer pixels. Changing ClampAndAlignWithPixels to ignore the bounds and always return the aligned value still results in whole-layer invalidations when hitting the top or bottom of the scroll extent. I don't know why this is yet.

Comment 19

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #18)
> I've had no luck so far in trying to force Gecko to always align to layer
> pixels. Changing ClampAndAlignWithPixels to ignore the bounds and always
> return the aligned value still results in whole-layer invalidations when
> hitting the top or bottom of the scroll extent. I don't know why this is yet.

Actually that does work :) For some reason the define isn't working in the file I'm using it in, so patch incoming once I figure that out...

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Always align scroll position with pixels on mobile

I think this is a reasonable replacement for the previous behaviour of not invalidating in this situation.

Comment 21

[Chris Lord [:cwiiis]]

Kevin, do these patches fix things for you? I can no longer reproduce, but perhaps I've changed some other things locally and it'd be good to get confirmation one way or the other.

Comment 22

[Markus Stange [:mstange]]

(In reply to Chris Lord [:cwiiis] from comment #16)
> Perhaps an alternative fix for the remaining issue, rather than changing the
> flexible scroll functions, is to make sure that our
> scroll-clamping-scroll-port-size takes this into account?

See bug 1012752 for more discussion on this topic. I didn't realize that this bug only occurs when scrolling to the end, otherwise I would have pointed to bug 1012752 sooner.

Comment 23

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #13)
> Also, that ClampAndAlignWithPixels knows about layer resolution, so
> something may be going wrong there.

The way that it gets the resolution is broken, see bug 1030370. That might or might not affect the case in this bug.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8446007 [details] [diff] [review]
Always align scroll position with pixels on mobile

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

The bug this patch is trying to fix is basically bug 1012752, and the issues there apply. I think we need to just figure out what we're going to do in bug 1012752.

Comment 25

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Comment on attachment 8446007 [details] [diff] [review]
> Always align scroll position with pixels on mobile
> 
> Review of attachment 8446007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The bug this patch is trying to fix is basically bug 1012752, and the issues
> there apply. I think we need to just figure out what we're going to do in
> bug 1012752.

Fair enough, in that case, I've added a dependency. I'm not sure how good an idea it is to apply the first patch without fixing this, as it's quite easy to hit this path if there's a small amount of horizontal scroll, for example.

Comment 26

[Chris Lord [:cwiiis]]

I've been talking with mstange on IRC, and this is how I understand things.

We have these situations:

1. Unpatched (how it is now):
On mobile, we live with 'seams' that commonly occur at validation edges (greatly exacerbated by progressive tiled rendering), where text position shifts and borders shift (e.g. bug 1014333).

2. Patched (attachment 8445931 [details] [diff] [review]):
These seams stop occurring, but we still get the behaviour where if you scroll to an extent, you get a whole-layer invalidation (bad for perf) and then all text and borders shift slightly (which looks bad, but not as bad as (1)).

3. Patched (attachment 8445931 [details] [diff] [review] + attachment 8446007 [details] [diff] [review] or attachment 8424948 [details] [diff] [review]):
No seams, no shift in text position or border thickness at scroll extents, but possible exposure of a pixel-wide area on the bottom and/or right edge of scrollable containers when scrolled to their extents.

4. Proper fix:
Scroll range takes into account layer pixel-grid snapping so that the extents of the scroll range don't cause the layer's pixel alignment to change (rendering attachment 8446007 [details] [diff] [review] obsolete).


(4) is obviously desirable, but hard. Personally, at least for mobile, I think (3) is a more desirable situation than (2), and certainly less bad than (1).

Given this, would you reconsider living with (3) until we get (4), or can we not settle for any less than (4) even on mobile?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't object to trying #3 on mobile.

Comment 28

[Chris Lord [:cwiiis]]

Comment on attachment 8446007 [details] [diff] [review]
Always align scroll position with pixels on mobile

Re-requesting review based on comment #27 (this patch only applies to mobile)

Comment 29

[Chris Lord [:cwiiis]]

On further thought, I think the second patch here won't result in those failures mentioned in bug 1012752 as as soon as you scroll something in b2g at least, it gets layerised - so snapping to layer pixels shouldn't need to take into account the origin (as it will effectively be zero and the snapping will match up).

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Lord [:cwiiis] from comment #29)
> On further thought, I think the second patch here won't result in those
> failures mentioned in bug 1012752 as as soon as you scroll something in b2g
> at least, it gets layerised - so snapping to layer pixels shouldn't need to
> take into account the origin (as it will effectively be zero and the
> snapping will match up).

No. ContainerState::CreateOrRecycleThebesLayer computes a subpixel offset for the layer contents so that mere layerization does not change the subpixel offset of content and cause a visible rendering change.

Comment 31

[Chris Lord [:cwiiis]]

Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8179a07a4181
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf1863c97a5

Comment 32

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42752306&tree=Mozilla-Inbound

Comment 33

[Chris Lord [:cwiiis]]

Attachment: Always align scroll position with pixels on mobile (mstange's patch)

This does the same thing as my patch, but better - mine has some issues that cause a couple of test failures, but I think this one will be ok. I've confirmed that it fixes the issue.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=192816210dac

Comment 34

[Chris Lord [:cwiiis]]

Comment on attachment 8449322 [details] [diff] [review]
Always align scroll position with pixels on mobile (mstange's patch)

Cancelling review, this patch doesn't yet work when the layer is zoomed.

Comment 35

[Kevin Brosnan [Ex-Mozilla]]

Chris is it worth me building the most recent patch? If so I should have some time tomorrow to deal with this.

Comment 36

[Chris Lord [:cwiiis]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #35)
> Chris is it worth me building the most recent patch? If so I should have
> some time tomorrow to deal with this.

If you could, yes please - it would be good to confirm this fixes the issue (it does as far as I can tell, but I'd rather have someone else confirm it). The remaining issue is that we end up with more invalidation than is necessary, but that doesn't have significant visual impact.

Comment 37

[Kevin Brosnan [Ex-Mozilla]]

This patch does not fix the issues in bug 1014333 I still see thicker than expected. I think the text alignment issues are resolved.

Comment 38

[Chris Lord [:cwiiis]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #37)
> This patch does not fix the issues in bug 1014333 I still see thicker than
> expected. I think the text alignment issues are resolved.

When you say you see thicker than expected, do you see the same line change thickness along the page? Could you post a screenshot, perhaps?

Comment 39

[Kevin Brosnan [Ex-Mozilla]]

Looks the same as https://bug1014333.bugzilla.mozilla.org/attachment.cgi?id=8428571

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Tracking (already was tracked in bug 1014333) + updating the tracking flags.

Comment 42

[Lawrence Mandel [:lmandel] (use needinfo)]

Chris - This bug looks like it's stalled. Assuming this is still reproducible, can you pick this back up again?

Comment 43

[Chris Lord [:cwiiis]]

(In reply to Lawrence Mandel [:lmandel] from comment #42)
> Chris - This bug looks like it's stalled. Assuming this is still
> reproducible, can you pick this back up again?

I think we need someone else to take a look at it - if it isn't caused by what I provided patches for (i.e. if it isn't fixed by attachment #8445931 [details] [diff] [review]), I'm really at a loss.

Comment 44

[Brad Lassey [:blassey] (use needinfo?)]

Milan, this is a regression in 32, and we've got 2 more weeks to take speculative fixes on beta. Need an assignee

Comment 45

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

I don't see us fixing this for 32.

Comment 46

[Chris Lord [:cwiiis]]

Also, I still think I was on the right track and that strictly speaking, this is a layout bug.

Comment 47

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

NI Jet, in case it is layout issue, and he has somebody that can take a look.

Comment 48

[Jet Villegas (inactive)]

(In reply to Milan Sreckovic [:milan] (PTO 8/11 - 8/15) from comment #47)
> NI Jet, in case it is layout issue, and he has somebody that can take a look.

Has someone tried to flip tiling off? 
https://bugzilla.mozilla.org/attachment.cgi?id=8387597&action=diff

I get that we're not going to turn tiling off to fix this bug, but the "(maybe at tile borders?)" line in the bug title suggests that this needs a bit more triage.

Comment 49

[Lawrence Mandel [:lmandel] (use needinfo)]

I think Kevin may be on PTO. ni aaronmt to help out.

Comment 50

[Aaron Train [:aaronmt]]

I'm having trouble trying to reproduce this again on my N5.

Comment 51

[Lawrence Mandel [:lmandel] (use needinfo)]

With no recent progress, I'm marking this as won't fix for 32.

Jet/Milan - We have already shipped 2 releases and are about to ship a third with this bug. As such, I'm not going to make the case that this needs to be fixed in 33. I will leave it as tracked until we get you to weigh in on when this can be prioritized for a fix. Can we figure out who should own this and get it on one of your prioritized lists?

Comment 52

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

I think we need somebody with a heavy layout experience here; I'm also not clear if bug 1012752 getting fixed helps or completely resolves this situation.

Comment 53

[Jet Villegas (inactive)]

Markus will have a look at this one next week, and Fix ETA is likely the following week.

Comment 54

[Sylvestre Ledru [:Sylvestre]]

Markus, Jet, any news on this bug? Thanks

Comment 55

[Kevin Brosnan [Ex-Mozilla]]

I noticed that this is not reproducible in aurora 34 but is on beta 33. I found a fixed range which points to Bug 1042423

Last good revision: 326c91338df0
First bad revision: 06b540d3667a
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=326c91338df0&tochange=06b540d3667a

Comment 56

[Chris Lord [:cwiiis]]

I'm super dubious of this being fixed, someone should check that the fix isn't actually just the whole page being constantly redrawn, or that progressive tiled updates haven't broken again.

n?kats, but I'll take a look at this too on Monday.

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Given the size of the patches in bug 1042423, I don't think we are going to uplift these patches in beta. It is too late in the cycle for this.
Except if Chris is able to do something this week, the 33 release is going to have this bug.

Comment 58

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

Bug 1042423 has known regressions anyway, so it's not safe to uplift until those have been addressed.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

OK. Wontfix for 33 then

Comment 60

[Markus Stange [:mstange]]

I can confirm that this issue is fixed on B2G. The #ifdefs that avoid invalidations on layer pixel alignment changes are "#ifndef MOZ_WIDGET_ANDROID"s, and MOZ_WIDGET_ANDROID is not set on B2G, only on Fennec. So B2G does invalidate on layer pixel alignment changes, which usually only happen once you hit the edge of the scrollable area (bug 1012752).

Removing the #ifdefs completely would fix the seams bug in Fennec, obviously. But, unlike B2G, this would cause Fennec to invalidate during regular panning, too, and regress bug 724786.

The cause of bug 724786 is the following:
Updating the main thread scroll position during panning happens with a window.scrollTo call from JS: http://hg.mozilla.org/mozilla-central/annotate/ae27ae77e32f/mobile/android/chrome/content/browser.js#l3661
The scrollTo DOM API currently only accepts integer values, so this scroll position is rounded to an integer CSS pixel value, and the resulting value in many cases can't be achieved without changing the layer pixel alignment.
Bug 724786 does not occur on B2G because B2G doesn't update the scroll position through a rounding JS API and thus can work with precise fractional values throughout the whole code path.

Fennec is going to switch to the C++ APZ code in the future, so I won't worry about the Fennec bug here too much. Once Fennec is switched, we can look into removing the #ifdefs that cause the seams, and hopefully bug 724786 won't regress at that point.

Comment 61

[Kevin Brosnan [Ex-Mozilla]]

I strongly disagree with punting on this issue for Fennec. If the solution is to convert the Java APZ to C++ then we need a commitment to actually finish this work instead the 'future'. Firefox for Android has a similar number of users as OS X Firefox. I would not accept this behavior on release of desktop Firefox.

Comment 62

[James Willcox (:snorp) ([e-mail]) (he/him)]

Alright, what if we stop using scrollTo and use something else which accepts a float? It's dumb to be restricted by the JS API here.

Comment 63

[James Willcox (:snorp) ([e-mail]) (he/him)]

Wait this bug is marked as FIXED. Is that not the case? :mstange's comments suggest that we are fundamentally hosed...

Comment 64

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Markus Stange [:mstange] from comment #60)

> Fennec is going to switch to the C++ APZ code in the future, so I won't
> worry about the Fennec bug here too much. Once Fennec is switched, we can
> look into removing the #ifdefs that cause the seams, and hopefully bug
> 724786 won't regress at that point.

You have to worry about the Fennec bug. As Kevin points out, Android is almost the second largest Firefox platform. These kinds of rendering issues make Firefox (and Mozilla) look really bad.

We can't wait for the switch to C++ APZ unless it's landing in Fx36, which it is not.

Comment 65

[Markus Stange [:mstange]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #62)
> Alright, what if we stop using scrollTo and use something else which accepts
> a float? It's dumb to be restricted by the JS API here.

I'm currently playing with that (I added an nsIDOMWindowUtils setScrollXYFloat method) and will report back when I know how well it works.

Comment 66

[James Willcox (:snorp) ([e-mail]) (he/him)]

Markus, any update?

Comment 67

[Markus Stange [:mstange]]

Yes, this is working well locally, still need to work out a few more kinks before I can start assembling reviewable patches.

Comment 68

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

Markus, let us know if Jamie can help.

Comment 69

[u279076]

Markus, what's the current status of this bug?

Comment 70

[Kevin Brosnan [Ex-Mozilla]]

I still see this from time to time on Firefox for Android. Kats' APZ Android work may change the behavior. bug 1206872

Comment 71

[u279076]

Kevin, does this still reproduce for you?

Comment 72

[Kevin Brosnan [Ex-Mozilla]]

Yes.

Comment 73

[u279076]

Resetting status flags since this wasn't really fixed in 34/35.

Comment 74

[Markus Stange [:mstange]]

This is probably fixed now that we're using WebRender on Android.