1025562 - Weird tiling behavior when scrolling on video thumbnails

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Saurabh Anand [:sawrubh]]

STR:
* Open http://mobile.theverge.com/entertainment/2014/6/13/5807924/listen-to-the-best-music-deadmau5-has-made-in-years
* Touch and scroll over the Video thumbnail under the More Stories section.

What happens:
Weird light gray tiles appear and disappear as you scroll

I'm getting this consistently without fail on a Nexus5 4.4.3. I tried getting people on #mobile to reproduce this but they weren't able to. A screenshot won't help, do we have some other ways through which I could help provide more debugging information to figure out the actual issue.

Comment 1

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

The thing also turns light gray if you long-press on the video. I think that's just the focus highlight styling on the element. It appearing one tile at a time is possibly a bug though. It could also be that we're just painting too slowly because of the gradient background in the video poster image.

Comment 2

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

I'm not entirely sure if this is a bug or not. I grabbed a profile: http://people.mozilla.org/~bgirard/cleopatra/#report=cbb2e8e6fa85b48b1bb512aad4e86e71e9ddf2f9 and if I look at the sample range [7219,7393] I can see the six tiles getting rasterized and uploaded one at a time. This matches the visible behaviour on this page (where the video poster image gets drawn as 6 tiles but maybe they're supposed to all get uploaded to the compositor together?

Comment 3

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

Actually this is a regression so it's definitely a bug. Probably the coherency region needs fixing in TiledContentClient.cpp to account for the recent changes in ClientTiledThebesLayer.cpp

Comment 4

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

Regression window:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e712b724d17&tochange=e6f113c83095

Guessing it's bug 1016558.

Comment 5

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

Inbound range confirms: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=388f59954ef6&tochange=f1f4b18bcbaa

Comment 6

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

Attachment: Part 1 - Add logging to TiledContentClient.cpp

Comment 7

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

Attachment: Part 2 - Remove busted code

This fixes the problem for me locally. No idea if it'll break other things, but even if it does I'd like to work through them one at a time rather than try to fix everything all in one shot while I don't have reliable ways to exercise all this code. Will do a try push to make sure it's good, do some local testing on B2G, and flag for review.

Comment 8

[Saurabh Anand [:sawrubh]]

https://www.dropbox.com/s/xdtr7d04s5nh04d/2.mp4 is how it looks. I haven't tried the patch though.

Comment 9

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

Try push https://tbpl.mozilla.org/?tree=Try&rev=77ab8c44a58b is green except for win xpcshell which is unrelated. Basic sanity checks on FxOS also seem ok.

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8440943 [details] [diff] [review]
Part 1 - Add logging to TiledContentClient.cpp

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

Looks sound, and well done on adding - I always end up having several bespoke logging patches in my queue and never think of doing it properly...

::: gfx/layers/client/TiledContentClient.cpp
@@ +1109,5 @@
>                                           void* aCallbackData)
>  {
> +  TILING_PRLOG_OBJ(("TILING 0x%p: Progressive update valid region %s\n", mThebesLayer, tmpstr.get()), aValidRegion);
> +  TILING_PRLOG_OBJ(("TILING 0x%p: Progressive update invalid region %s\n", mThebesLayer, tmpstr.get()), aInvalidRegion);
> +  TILING_PRLOG_OBJ(("TILING 0x%p: Progressive update old valid region %s\n", mThebesLayer, tmpstr.get()), aOldValidRegion);

I think it'd make sense to have the invalid region come after the old valid region, but this obviously doesn't really matter at all.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8440944 [details] [diff] [review]
Part 2 - Remove busted code

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

LGTM, and you've flagged it before so if there are issues, I'd rather work from a broken state without this code than with.

Comment 12

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

(In reply to Chris Lord [:cwiiis] from comment #10)
> Comment on attachment 8440943 [details] [diff] [review]
> I think it'd make sense to have the invalid region come after the old valid
> region, but this obviously doesn't really matter at all.

That was the order the parameters were in, so I decided to leave it as-is. As you said, it doesn't really matter :)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2893df3be9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7250e9b202

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/fc2893df3be9
https://hg.mozilla.org/mozilla-central/rev/fc7250e9b202

Comment 14

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

Comment on attachment 8440943 [details] [diff] [review]
Part 1 - Add logging to TiledContentClient.cpp

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1016558
User impact if declined: Visible tiling artifacts while repainting. That is, when content repaints on screen, you can see individual tiles repaint one at a time when you shouldn't be able to - it should repaint as a block.
Testing completed (on m-c, etc.): locally, on m-c
Risk to taking this patch (and alternatives if risky): there is a risk of introducing other regressions because this code is not well understood, but I am prepared to fix regressions quickly if any are found because this is also needed for B2G 2.0. (Code affects Fennec and B2G)
String or IDL/UUID changes made by this patch: none

Comment 15

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

Comment on attachment 8440944 [details] [diff] [review]
Part 2 - Remove busted code

Ditto

Comment 16

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

Comment on attachment 8440943 [details] [diff] [review]
Part 1 - Add logging to TiledContentClient.cpp

Aurora approval granted. 

There is no sheriff available to uplift until next week. Can you please land on Aurora so that we can start to shake out any follow-up bugs?

Comment 17

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

Turns out there is a code dependency on bug 1023882 that I forgot about. I could rebase this to apply without that, but I'd like to uplift that as well to make uplifting this and other bugs easier. I'll land as soon as all the pending approvals are resolved.

Comment 18

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f29fb8b5b4aa
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/6fd0c74f602a

Comment 19

[Cristina Madaras, QA [:CristinaM]]

Those light gray tiles does not appear anymore when scrolling over the video thumbnail. 
Verified as fixed in builds:
33.0a1 (2014-07-06)
32.0a2 (2014-07-06)

Device: LG Nexus 4 (Android 4.4.2)