1151071 - [Grey Picker] Scrolling on a grey selection or picker page will display black bars on the sides of the screen

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Scrolling issue

Description:
When the user scrolls on a large grey picker page, such as the date and time city selection, or language selection pickers, there will be large black bars that appear on both sides of the screen


Repro Steps:
1) Update a Flame to 20150403010203
2) Open Settings app
3) Select Language> Language Picker
4) quickly scroll up or down and view the edges of the screen


Actual:
A black bar is seen appearing on the sides

Expected:
There are no graphical errors while scrolling

Environmental Variables:
Device: Flame Master (319mb)(Kitkat)(Full Flash)
Build ID: 20150403010203
Gaia: 7969b367a7da62877c3a24a26d3cb5fda89d766c
Gecko: 70a113676b21
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0


Repro frequency: 10/10
See attached: Logcat, Video - https://youtu.be/v1BASkyVrEM

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This does NOT occur on Flame 2.2

There are no graphical errors while scrolling

Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat)(Full Flash)
Build ID: 20150402002500
Gaia: 1ceca464053dee4a8bf10ea5abeef724d68c2ff2
Gecko: 427b4da96714
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Regression of a core feature.

Window already requested

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20150402114235
Gaia: f37be8b44cb7c3a147b9615ab76743b760f08eeb
Gecko: 8f7ab41e6cc8
Version: 40.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20150402115556
Gaia: f37be8b44cb7c3a147b9615ab76743b760f08eeb
Gecko: d0fc7202b4cb
Version: 40.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8f7ab41e6cc8&tochange=d0fc7202b4cb

Caused by either Bug 1148022 or Bug 1148855.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Markus, can you please take a look at this? Thanks!

Comment 5

[Markus Stange [:mstange]]

I can reproduce this. My languages list only has three items, but the bug also shows up in the "City" list in the Date & Time preferences.

Comment 6

[Markus Stange [:mstange]]

I think what happens here is that the scrolled layer has become opaque, because bug 1148855 has made it possible for it to pull up a background color, and now we're resampling opaque black from outside the layer when doing low resolution painting.
Jeff tells me we should clamp (i.e. not sample from outside the visible region), so I need to find out why we're not doing that.

Comment 7

[Markus Stange [:mstange]]

Attachment: "screenshot"

Comment 8

[Markus Stange [:mstange]]

Jeff pointed me to https://hg.mozilla.org/mozilla-central/annotate/4fe763cbe196/gfx/layers/client/TiledContentClient.cpp#l1267 which disables the mechanism that prevents us from sampling outside the visible region for low-resolution tiles.

Comment 9

[Markus Stange [:mstange]]

Attachment: patch

Matt, what do you think about this?

Comment 10

[Markus Stange [:mstange]]

Hmm, this patch will actually turn all potentially resampled layers transparent. That's not good.

Comment 11

[Markus Stange [:mstange]]

So, there are two solutions here which I'd deem acceptable in the long run:
 (1) The texture stays opaque but we don't resample from outside the visible region. or
 (2) The layer is extended into all directions by a certain amount, so that when we sample from beyond the original visible region, we only sample valid pixels. If the extended layer can still be opaque, it should be opaque.

(1) seems impossible because apparently we can only clamp to tiles, not to regions inside them. (We could theoretically supply the fragment shader with a clamping rect, but if we have a region with touching rects then we still want to sample from adjacent rects of the region, so this is not a good solution.)
(2) sounds possible, but it would have to be done in FrameLayerBuilder, before we attempt to pull up a background color into the extended layer. And we'd need to extend by 4 layer pixels always, because any parts of the layer could be drawn with low resolution later on.

Comment 12

[Markus Stange [:mstange]]

Attachment: MozReview Request: bz://1151071/mstange

/r/6667 - Bug 1151071 - Make sure low-resolution tiles are transparent. r=mattwoodrow

Pull down this commit:

hg pull -r 937aafea272624ff0a29406c98d5bf9e4c1e804d https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8588777 [details]
MozReview Request: bz://1151071/mstange

/r/6667 - Bug 1151071 - Make sure low-resolution tiles are transparent. r=mattwoodrow

Pull down this commit:

hg pull -r 937aafea272624ff0a29406c98d5bf9e4c1e804d https://reviewboard-hg.mozilla.org/gecko/

Comment 14

[Markus Stange [:mstange]]

I didn't implement any of the solutions from comment 11 because that would have been more work, and I want to fix the regressions from bug 1148855 quickly.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8588777 [details]
MozReview Request: bz://1151071/mstange

https://reviewboard.mozilla.org/r/6665/#review5557

Ship It!

::: gfx/layers/client/TiledContentClient.cpp
(Diff revision 1)
> +    if (mResolution != 1) {

Please add a comment explaining why we need to do this.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1151071 - Make sure low-resolution tiles are transparent. r=mattwoodrow