Closed Bug 1073554 Opened 5 years ago Closed 5 years ago

Visible seams in webpages while panning

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 37
Tracking Status
firefox34 --- unaffected
firefox35 + verified
firefox36 + verified
firefox37 --- verified
fennec 35+ ---

People

(Reporter: gcp, Assigned: mattwoodrow)

References

Details

(Keywords: regression, reproducible, Whiteboard: layer-tiles)

Attachments

(1 file)

Attached image SC20140926-170240.jpg
Galaxy Tab 10.1, Android 3.2, current m-c, regression since about a month (sorry for not reporting earlier, but there was so much gfx breakage on Android that it was impossible to tell that this is a separate problem).

1) Visit http://www.pianoworld.com/forum/ubbthreads.php/forum_summary.html
2) Pan up and down the entire page a few times
3) Observe visible blue or yellow horizontal seams in the page (see screenshot)

I bisected this to:

The first bad revision is:
changeset:   202105:abcdfdab902b
user:        Matt Woodrow <mwoodrow@mozilla.com>
date:        Thu Aug 28 15:36:07 2014 +1200
summary:     Bug 1051134 - Revert changes to padding out textures that will be resampled for mobile. r=Bas

Which, when looking what it patches, does indeed look like a very likely regressor!
Keywords: regression
Blocks: 1051134
tracking-fennec: --- → ?
That change just backed out a change from a week or so earlier, it shouldn't have created anything new.
(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> That change just backed out a change from a week or so earlier, it shouldn't
> have created anything new.

What's the original bug it reverted? I notice in the bug comments "I couldn't reproduce the original problem, and Jeff fixed it another way", so it seems your fix *did* work and Jeff's didn't :-)
Okay, I see the related bugs are bug 1048110 and bug 1023473.

Whatever "fix for mobile" was made there was not complete.
[Tracking Requested - why for this release]:
tracking-fennec: ? → 35+
Assignee: nobody → matt.woodrow
Snorp said he thought this regressed in 34. So NI to him to test and report back
Flags: needinfo?(snorp)
I can't reproduce it there, but I can't reliably reproduce it on 35/36 either.
Flags: needinfo?(snorp)
Whiteboard: tiles → layer-tiles
Still reproduces. Verified with a try build from blassey that backing out bug 1023473 does not fix this. 

Backing out bug 1051134 which is itself a backout did fix this when I originally reported it, but apparently regresses performance, which means we can have our blue seams faster now.
Keywords: reproducible
I suspect the original fix for this issue was this or in the changesets right after:
changeset:   198480:a287654390b1
user:        Matt Woodrow <mwoodrow@mozilla.com>
date:        Mon Aug 04 15:29:55 2014 +1200
summary:     Bug 1048110 - Expand complex visible regions if we're going to be resampling when using tiling. r=Bas

Unfortunately those were also builds that were broken (Java exception on launch) on my hardware where this reproduces. Sigh.
So for the case where backing out the backout isn't possible because of performance issues, I went to the place where the code that was backed out was added, which is this:

changeset:   198480:a287654390b1
user:        Matt Woodrow <mwoodrow@mozilla.com>
date:        Mon Aug 04 15:29:55 2014 +1200
summary:     Bug 1048110 - Expand complex visible regions if we're going to be resampling when using tiling. r=Bas

Now at this place, we are in a broken (with seams) state. I went back further then, trying to find the place where this once-upon-a-time worked. I was expecting to find something like "enable tiled rendering", but the actual bisection pointed to something more interesting:

The first bad revision is:
changeset:   195415:7a5b3439faa7
user:        Jeff Muizelaar <jmuizelaar@mozilla.com>
date:        Fri Jul 18 14:25:34 2014 -0400
summary:     Bug 1023473. Pad out tile contents by one pixel. r=BenWa

But that directly contradicts the conclusion in comment 7!
Blocks: 1023473
After investigation, we found that the backout-bug-1023473 try build didn't actually back out the entire patch. So that explains all the above.

Conclusion: this is caused by bug 1023473.
Does backing out the patch on bug 1023473 cause bug 1023473 to regress or is it handled by bug 1048110 now?
Is this still reproducible?
As far as I know, yes, I don't think anything landed since comment 10 that changes this.

Needinfo myself to test https://tbpl.mozilla.org/?tree=Try&rev=4893d9de121b (which backs out 1023473) when I get back from Portland.
Flags: needinfo?(gpascutto)
Confirming that the try build with bug 1023473 backed out works correctly.
Flags: needinfo?(gpascutto)
What is the step forward here?
Flags: needinfo?(gpascutto)
Verifying that the backout doesn't regress bug 1023473 itself. (See comment 11)
Flags: needinfo?(gpascutto)
Next week is the last beta for 35.0 so if a clean backout of bug 1023473 is possible (and doesn't regress the issue further) we should get an uplift of the backout asap for the Jan 5th beta.
Flags: needinfo?(matt.woodrow)
Someone else will need to do it, he is away till the 5th.
We talked about this in IRC and in order to not ship this new regression, we should get the backout on 35 asap and take care of checking for regressions to FxOS separately - since they have different shipping dates where one (Fennec) is next week and the other has more time for repair if needed.  I believe Blassey can do the backout so we're ready for this afternoon's final mobile beta gtb.
Flags: needinfo?(matt.woodrow) → needinfo?(blassey.bugs)
The patch from the Try run doesn't apply cleanly to beta anymore. I'm going to punt on this for someone who can better ensure no accidental bustage on something this close to release.
Wonderful, the backout push has Android reftest failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=7033680d2fc4&filter-searchStr=Android%20reftest
Flags: needinfo?(snorp)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(blassey.bugs)
> REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/transform-3d/perspective-origin-1b.html | image comparison (==), max difference: 130, number of differing pixels: 70 

random-if(Android&&AndroidVersion==17) == perspective-origin-1b.html perspective-origin-1a.html

I guess we made it "random" on more than just version 17 now.

> REFTEST TEST-UNEXPECTED-FAIL | http://10.26.133.19:30555/tests/layout/reftests/transform-3d/rotatex-transformorigin-1a.html | image comparison (==), max difference: 80, number of differing pixels: 6 

reftest.list says we shouldn't have any Android issues with that test.

I'm not going to back this out as it's past bedtime for me, but I can't say I'm thrilled with the idea of this shipping as-is either.
All this means is that test started being random on the emulator (in addition to also being random on the panda). We should just drop the android version conditional on the random if
Flags: needinfo?(blassey.bugs)
Who's going to take care of changing the test expectations if that's all that's intended to be done here? Leaving things broken indefinitely on release branches isn't OK.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #25)
> Who's going to take care of changing the test expectations if that's all
> that's intended to be done here? Leaving things broken indefinitely on
> release branches isn't OK.

I'm going to handle it as well as backing out on aurora/central
Flags: needinfo?(snorp)
blassey and I both tested beta with the backout applied and couldn't reproduce this anymore. Does anyone else have a different experience?
Just to be clear - you backed out bug 1023473 on Beta and the problem is gone?

Since this is a backout of a backout of a backout bug, please confirm what's being backed out when "backing out" is in the comment :)
(In reply to Milan Sreckovic [:milan] from comment #28)
> Just to be clear - you backed out bug 1023473 on Beta and the problem is
> gone?
> 
> Since this is a backout of a backout of a backout bug, please confirm what's
> being backed out when "backing out" is in the comment :)

This is the backout that was pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/7033680d2fc4

This does appear more or less the patch that is in bug 1023473.
Actually I can reproduce this still even with the backout. STR:

1) Go to planet.mozilla.org
2) Zoom in all the way
3) pan to the right about half way
4) quickly scroll down
5) Observe horizontal and vertical black (dark gray?) lines
Clarified with snorp that his issue is while scrolling, mine is static corruption that stays.

I *cannot* reproduce it:
a) With the try build from comment 22.
b) With Nightly.
c) With current Beta from the Google Play store.

snorp believes it's possible his Visit* crash fixes have also fixed this bug.
I've filed a new issue about the seams around low res stuff when scrolling at bug 1118858.

I expect we should backout the backout of bug 1023473 as it doesn't seem to make a difference anymore and as it's more likely that snorps fix to the Visit* crash fixed this bug.
RyanVM is going to backout the backout on beta and release.
Marking this fixed, per comments #32 and #33
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Err, that would be comments #31 and #32
Backed out on beta. Will merge to release in a bit.
https://hg.mozilla.org/releases/mozilla-beta/rev/400119b1311a
Flags: needinfo?(matt.woodrow)
Target Milestone: --- → Firefox 37
See also bug 1110229 for a similar (but likely with a different root cause) seeming bug described in Comment 30.
See Also: → 1110229
Verified as fixed on Firefox 35 RC and Firefox 35 Beta 11 on Acer Iconia A500 (Android 3.2.1) with the STR from comment 0.
Mihai, you changed the status flag for 36. I guess that is still fixed but did you verify it?
Flags: needinfo?(mihai.g.pop)
Verified as fixed on Firefox 36 Beta 1 and Aurora 37.0a2(2015-01-14) on Samsung Galaxy Tab 10.1(Android 4.0.4)
Status: RESOLVED → VERIFIED
Flags: needinfo?(mihai.g.pop)
You need to log in before you can comment on or make changes to this bug.