1169339 - Flickering during scrolling on flame-kk

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

Since Bug 1150549 land, scroll by fling causes the flickering. And occasionally, time seems to be rendered incorrect position for a short time.

I confirmed that the symptom is fixed by locally backed out Bug 1150549. Confirmed on locally build latest master flame-kk.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

I think it's worth considering backing 1150549 out unless there's some value in keeping it on trunk.

Comment 2

[Sotaro Ikeda [:sotaro]]

I reproduced the following STR  

[STR]
-[1] Open browser app and move to http://www.yahoo.co.jp/
-[2] zoon the page a little bit.
-[3] flick the page. diagonal direction might be easier to see the problem.

continue [2] [3] until the problem happen. During [3], the flickering was seen.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: video of the symptom on flame-kk

Comment 4

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

Let's back this out if we can't fix it within a day.  No-Jun, can you reproduce this?

Comment 5

[Sotaro Ikeda [:sotaro]]

on nexus-5, it was very easy to reproduce the problem on the following STR.

[STR]
[1] show homescreen
[2] repeat vertical scroll.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: video of the symptom on nexus-5-l

Comment 7

[No-Jun Park [:njpark]]

Talked to sotaro, it is easier to see this issue on nexus 5-l, but I do see some sort of flickering when flinged diagonally on a non mobile website.

Comment 8

[Nicolas Silva [:nical]]

I think that the issue comes from that progressive tiling is accounted for as over-production by the code that manages copy-on-write locks, and that is causing locks to be released too early.

Comment 9

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

This is a difficult patch to back out, and we'd like to produce a correct solution for the problem.  That may take a couple of days; No-Jun, could you keep us in touch with B2G QA and if this regression ends up being a blocker for any testing or workflows, let us know and we'll look for plan B.

Comment 10

[Nicolas Silva [:nical]]

Attachment: store tiles column by colmun instead of row by row

I tinkered with the locks but the problem persisted. In fact the real issue was that our tiling code stored tiles column after column instead of row after row. part of the code in the TiledContentHost simplification was using the more conventional row after row order (when reordering the tile buffer after a change of origin), hence some flickering.

This patch is the quickest way to fix the issue (and also reorganizes UseTileTexture in order to fix an issue on linux).

In the process of debugging this I found a LOT of things that are way more complex than they need to be so I will follow up with more simplifications (including reorganizing tiles row after row because what we are doing now is unconventional and silly). I'll look into the locking issue. It may not be a big problem because it only affects b2g and D3D11 and existed before the TiledContentHost simplification. At worse will cause the content thread to wait for the back buffer for a short time instead of picking a new texture from the pool. Still worth figuring out.

Comment 11

[Markus Stange [:mstange]]

(In reply to Nicolas Silva [:nical] from comment #10)
> hence some flickering.

Why would this cause flickering?

Comment 12

[Sotaro Ikeda [:sotaro]]

I confirmed the fix by applying the path on master nexus-5-l.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8613294 [details] [diff] [review]
store tiles column by colmun instead of row by row

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

::: gfx/layers/composite/TiledContentHost.cpp
@@ +110,5 @@
>                 CompositableTextureSourceRef& aTextureSource,
>                 const IntRect& aUpdateRect,
>                 TextureHost* aNewTexture,
>                 Compositor* aCompositor)
>  {

Is this change intentional?

Comment 14

[Nicolas Silva [:nical]]

Attachment: Fix without the UseTileTexture part.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0bc0aa63816

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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