992218 - Regression: Content glitches/flashes during periods of heavy repainting

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Mark Capella [:capella]]

Not sure if this is the right component for this report ...

Using last a current m-c repo / build (No patches in queue), I notice this oddness while moving text selection handles around the page while in reader mode.

(Starts around :05sec)

   https://www.dropbox.com/s/do5fxn065s2kuu1/readerModeFlicker.mp4

Comment 1

[Aaron Train [:aaronmt]]

Without a doubt a regression from recent graphics refactoring work. CC'ing some folks.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

Regression window wanted:
mozilla central:
good build: 08-03-2014 
bad build: 09-03-2014

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

Comment 3

[Aaron Train [:aaronmt]]

Teodora, can you use narrow this using mozilla-inbound please?

I suspect this is a regression bug 963073 (enable b2g tiling by default).

Comment 4

[Teodora Vermesan (:TeoVermesan)]

(In reply to Aaron Train [:aaronmt] from comment #3)
> I suspect this is a regression bug 963073 (enable b2g tiling by default).

Yes, the bug is Bug 963073.
inbound:
good build: 1394227648
bad build: 1394229086

pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c8355055899c&tochange=40b2ac413772

Comment 5

[Chris Lord [:cwiiis]]

This is interesting... I expect it's another case of things going wrong in the progressive update code (can you reproduce if you change layers.progressive-paint to false?)

I'll be on PTO for 2 weeks, so I'm not going to get to this soon. needinfoing myself in case this isn't picked up before then, and kats in case this is related to the progressive update code.

Comment 7

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

Added to my list of "possibly progressive update" bugs.

Comment 8

[Aaron Train [:aaronmt]]

I think what I see is this bug but am not 100% certain: https://www.youtube.com/watch?v=WRpKmnobUNg (720p it)

Comment 9

[Mark Capella [:capella]]

Yah, that UI "twitching" ... I do see it in other places, I just documented the first one I could repro.

Comment 10

[Bas Schouten (:bas.schouten)]

I know little to nothing about this code so for now I'm going to un-need-info myself.

Comment 11

[Mark Capella [:capella]]

Ah, just realized comment #5 might have been a question for me ... in any case after a little more tinkering, I find I can only repro this on my GS/3 4.3, and have been unable to so far on my N7 4.4.2.

On the GS3 I can repro regardless of the layers.progressive-paint setting.

Comment 12

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

I was also able to repro this on the Nexus 4. I'm looking into it.

Comment 13

[Aaron Train [:aaronmt]]

also reproducible just by viewing an animation e.g, a *.gif, http://i.imgur.com/wEGGj3w.gif

Comment 14

[Mark Capella [:capella]]

Ah, better STR! Does occur on the N7 ...

(Spotted that GIF recently btw! Cooler than Honda's Awesome-o http://www.telegraph.co.uk/technology/technology-video/8877806/Honda-show-off-new-faster-smarter-Asimo-robot.html)

Comment 15

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

That gif makes it much easier to reproduce, thanks! (Also pretty scary..)

I'm not yet sure of the cause of the glitching but if tiling is enabled it usually seems to affect one or two tiles at a time, and if I disable tiling the entire content area seems affected. So it's not specific to tiling per se. I also looked at the layer tree dumps on both the client and compositor sides and there is no difference between the glitchy composites and the normal ones. I suspect that this is just inadequate locking somewhere and some buffer that is in the middle of getting updated is copied, resulting in partial garbage. I'm going to try to audit the codepaths to see if I can find this by inspection.

Comment 16

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

CC'ing nical as well, since nical, Bas and Cwiiis are the ones who worked on the b2g-tiling code that caused this regression.

Comment 17

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

On a debug build where things are slower this is actually quite funny. Tiles are just being misplaced, making it look like a jigsaw puzzle:

https://www.youtube.com/watch?v=Us5sH6AY_4c

Bas, nical, any thoughts on where the problem might be?

Comment 18

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

Attachment: Workaround

After much debugging I seem to have narrowed it down to the texture pool (unsurprising in hindsight) and shrinking the size of the texture pool to 0 fixes the problem. I had tried some things that I thought were equivalent to this before (such as never returning textures to the pool) but they didn't work, so maybe there are some subtler differences between this and what I was trying. Anyway, stashing this patch here for now.

Comment 20

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

Comment on attachment 8410463 [details] [diff] [review]
Workaround

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

Would you rather distable the texture client pool some other way, if it comes to that? Tomorrow I'll take another look to see if I can figure out exactly what codepaths are triggering the improper texture reuse.

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8410463 [details] [diff] [review]
Workaround

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

I would be very apprehensive to disable this cache on b2g because it means multiple synchronous round trip to the compositor to allocate every single tile. I'm afraid this will come out as major performance/checkerboarding regressions. If it comes to it we should instead double down on this issue and bring in nical/cwiiis who know the new tiling code the best.

Comment 22

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

Attachment: Patch

I narrowed down the problem to recycling the backbuffer while it is still in use. Based on the code at [1] I think this is the correct fix but as we said before this texture pool is kinda funky so I'm not sure if I'm missing something.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp?rev=68c8916ca208#510

Comment 23

[Benoit Girard (:BenWa)]

Comment on attachment 8411058 [details] [diff] [review]
Patch

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

I'm not familiar with this code. -> nical

Comment 24

[Nicolas Silva [:nical]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Created attachment 8411058 [details] [diff] [review]
> Patch
> 
> I narrowed down the problem to recycling the backbuffer while it is still in
> use. Based on the code at [1] I think this is the correct fix but as we said
> before this texture pool is kinda funky so I'm not sure if I'm missing
> something.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TiledContentClient.cpp?rev=68c8916ca208#510

In theory, as long as a gralloc buffer is bound to a gl texture, it should remain locked. Except error on our side, if a gralloc buffer is the front buffer of something, it should be bound to a texture.
What should happen here is that the client side blocks on the gralloc lock when over-producing, which is the desired result. obviously something is wrong in what I said but that was the idea.

if we take this patch, recycling will be less efficient, but we might not have a choice in the end. I just want to understand what is breaking my assumptions before giving the r+

Comment 25

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

(In reply to Nicolas Silva [:nical] from comment #24)
> In theory, as long as a gralloc buffer is bound to a gl texture, it should
> remain locked. Except error on our side, if a gralloc buffer is the front
> buffer of something, it should be bound to a texture.
> What should happen here is that the client side blocks on the gralloc lock
> when over-producing, which is the desired result. obviously something is
> wrong in what I said but that was the idea.

On Fennec we don't use gralloc so the behaviour you're describing is probably not valid there.

> if we take this patch, recycling will be less efficient, but we might not
> have a choice in the end. I just want to understand what is breaking my
> assumptions before giving the r+

Honestly I'll be happy to try out any other suggestions you have - I don't really understand this code that well so I'm just grasping at straws.

Comment 26

[Nicolas Silva [:nical]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> (In reply to Nicolas Silva [:nical] from comment #24)
> > In theory, as long as a gralloc buffer is bound to a gl texture, it should
> > remain locked. Except error on our side, if a gralloc buffer is the front
> > buffer of something, it should be bound to a texture.
> > What should happen here is that the client side blocks on the gralloc lock
> > when over-producing, which is the desired result. obviously something is
> > wrong in what I said but that was the idea.
> 
> On Fennec we don't use gralloc so the behaviour you're describing is
> probably not valid there.

Oops, my bad. I am still actively thinking about this. I'll either have a counter-proposal or a r+ by the end of tomorrow.

Comment 27

[Nicolas Silva [:nical]]

Comment on attachment 8411058 [details] [diff] [review]
Patch

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

r=me with the modification below:

::: gfx/layers/client/TiledContentClient.cpp
@@ +484,5 @@
>  TileClient::DiscardBackBuffer()
>  {
>    if (mBackBuffer) {
>      MOZ_ASSERT(mBackLock);
> +    if (mBackLock->GetReadCount() > 1) {

Please change this condition to 

(!mBackBuffer->ImplementsLocking() && mBackLock->GetReadCount() > 1)

This will keep the current behavior where the texture's blocking lock will prevent over-production (on b2g and windows D3D11), and fallback to discarding tiles more aggressively if the the TextureClient doesn't have a blocking lock (like on fennec).

Comment 28

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

Makes sense. I verified the updated patch still fixes the problem on Fennec and landed:

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

Comment 29

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

Attachment: Patch v2

This is the updated patch. Requesting uplift to 30 as well (currently aurora, soon to be beta)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 963073
User impact if declined: while content is repainting (e.g. animated gifs, dragging around selection handles, and many other scenarios) often the repainted area will be of the wrong spot, because some other tile's content is painted instead of the right thing. This results in ugly visual glitching, pretty severe in some cases.
Testing completed (on m-c, etc.): locally so far
Risk to taking this patch (and alternatives if risky): this code is used on B2G, metro, and fennec, so it has the potential to negatively impact B2G or metro. However the additional condition nical had me add to the patch should restrict it to Fennec.
String or IDL/UUID changes made by this patch: none

Comment 30

[Aaron Train [:aaronmt]]

> Testing completed (on m-c, etc.): locally so far

For driver, tested the patch locally last evening against the steps outlined in this bug and it's duplicates and it worked for me.

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Prime example of naming actively confusing even us: this cset landed with this commit msg, " Don't flag texture clients for reuse [...]". A TextureClient is not "a client" but "a texture" (on the client side). When the idea that's floating around, to s/TextureClient/ClientTexture/.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

*Whence

Comment 33

[Botond Ballo [:botond]]

(In reply to Benoit Jacob [:bjacob] from comment #31)
> Prime example of naming actively confusing even us: this cset landed with
> this commit msg, " Don't flag texture clients for reuse [...]". A
> TextureClient is not "a client" but "a texture" (on the client side). When
> the idea that's floating around, to s/TextureClient/ClientTexture/.

+1. While we're at it, let's please fix the inconsistency where in "ClientLayer", "Layer" is at the end but in "LayerComposite", "Layer" is at the beginning.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

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

Uplifted with minor rebase:

https://hg.mozilla.org/releases/mozilla-aurora/rev/10a8ef2b96c7

Comment 36

[Chris Lord [:cwiiis]]

The patch for this looks sound to me, clearing needinfo.

Comment 37

[Teodora Vermesan (:TeoVermesan)]

Using the gif provided in the URL, there are no more glitches/flashes, so:
Verified fixed on:
Build: Firefox for Android 30 Beta 4
Device: Alcatel One Touch (Android 4.1.2)