Closed Bug 1025562 Opened 5 years ago Closed 5 years ago

Weird tiling behavior when scrolling on video thumbnails

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox31 --- unaffected
firefox32 + verified
firefox33 + verified
fennec 32+ ---

People

(Reporter: sawrubh, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
Component: General → Graphics, Panning and Zooming
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.
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?
Flags: needinfo?(chrislord.net)
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
Flags: needinfo?(chrislord.net)
Keywords: regression
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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.
Assignee: nobody → bugmail.mozilla
https://www.dropbox.com/s/xdtr7d04s5nh04d/2.mp4 is how it looks. I haven't tried the patch though.
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 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.
Attachment #8440943 - Flags: review?(chrislord.net) → review+
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.
Attachment #8440944 - Flags: review?(chrislord.net) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/fc2893df3be9
https://hg.mozilla.org/mozilla-central/rev/fc7250e9b202
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
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
Attachment #8440943 - Flags: approval-mozilla-aurora?
Comment on attachment 8440944 [details] [diff] [review]
Part 2 - Remove busted code

Ditto
Attachment #8440944 - Flags: approval-mozilla-aurora?
tracking-fennec: ? → 32+
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?
Attachment #8440943 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8440944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.