Closed Bug 1210444 Opened 4 years ago Closed 4 years ago

Animated cloud/sun flickers horribly on forecast.io, on Firefox Android, with gfx.canvas.azure.accelerated = true

Categories

(Core :: Graphics, defect)

Unspecified
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 + fixed
firefox43 + fixed
firefox44 --- fixed
fennec 42+ ---

People

(Reporter: dholbert, Assigned: jnicol)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(2 files)

STR:
 1. Visit forecast.io in Firefox Nightly on an Android phone.
 2. Wait for it to load its main screen, with the temperature and a little cloud/sun animation.

ACTUAL RESULTS: The cloud/sun animation flickers really horribly.
EXPECTED RESULTS: No flickering.

This is a recent regression, and I just tracked down the nightly regression range.

Good: http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2015-09-22-03-02-04-mozilla-central-android-api-11/
Bad: http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2015-09-23-03-02-30-mozilla-central-android-api-11/

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2235e56c94cf61614902fd3a4ac7b837f7154b97&tochange=19b4265d0d568d232fb3a34705f947b6db7496dc
Here's a video screencast of the bug. (in mp4 format, because that's what "adb shell screenrecord" spits out)
Sorry, that pushlog in comment 0 was incorrect -- I should've used mozilla-central in the URL, not mozilla-inbound.

Corrected pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2235e56c94cf61614902fd3a4ac7b837f7154b97&tochange=19b4265d0d568d232fb3a34705f947b6db7496dc

My tentative suspects in that range (searching for "paint", "anim", and "layer") are:
{
c05eccce5a92	Daniel Holbert — Bug 1198894: Use nsChangeHint_RepaintFrame instead of NS_STYLE_HINT_VISUAL to trigger simple repaints in nsStyleStruct.cpp CalcDifference methods. r=heycam

0a34ebc90b12	Markus Stange — Bug 1195400 - Check ancestor geometry roots when determining scrollability of a layer. r=mattwoodrow

17a4283a8b6f	Markus Stange — Bug 1195400 - Don't use SingleTiledContentClient for layers that are larger than the maximum texture size. r=mattwoodrow
}
I'm using a OnePlus One phone, with CyanogenMod 12.1 (equivalent to Android 5.1.1).
Last good inbound build:
 http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android-api-11/1442837076/
First bad inbound build:
 http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android-api-11/1442837616/

--> regression from https://hg.mozilla.org/integration/mozilla-inbound/rev/499fd45447a2 , bug 1206076

I verified that I can "fix" this bug by toggling that changeset's pref to false, too. ("gfx.canvas.azure.accelerated")
Blocks: 1206076
Flags: needinfo?(bas)
Summary: Animated cloud/sun flickers horribly on forecast.io, on Firefox Android → Animated cloud/sun flickers horribly on forecast.io, on Firefox Android, with gfx.canvas.azure.accelerated = true
Here's a longer video showing more of the flickering, & showing that it becomes fixed if I toggle the azure.accelerated pref to false:  https://www.youtube.com/watch?v=94pGSOJ_pPw
:snorp, can you take a look at this, Jamie is a bit swamped.
Flags: needinfo?(bas) → needinfo?(snorp)
[Tracking Requested - why for this release]:
This is from a SkiaGL canvas fix that was uplifted to 42.
OS: Unspecified → Android
Whiteboard: [gfx-noted]
(In reply to Milan Sreckovic [:milan] from comment #7)
> [Tracking Requested - why for this release]:
> This is from a SkiaGL canvas fix that was uplifted to 42.

...though hopefully Bas will soon be reverting the offending part of that uplift (the pref-flip) on all branches, per bug 1206076 comment 23.  So, soon, this should hopefully not be reproducible by default anywhere, unless you actively set gfx.canvas.azure.accelerated to true.
Also worth mentioning:
 - I tried to reproduce this on my Nexus 7 tablet, and was unable to do so. (Plus, I got served the desktop UI there, probably because of my tablet's larger viewport).  So, this may require a phone.
 - This is reproducible with a fresh Firefox Nightly profile. I know this because I accidentally cleared my Nightly app data (by uninstalling a Nightly version) when tracking down the regression range.  [I also reproduced it in "Guest Session" which is basically like a fresh profile.]
Bas, to confirm, Skia GL is off by default on Android, so this bug only shows up with the preference forcing it?

Also, Bas, who should look at Sotaro's suggestion in https://bugzilla.mozilla.org/show_bug.cgi?id=1206076#c26 as to where the problem may lie?  Doesn't sound like it's specific to Android?
Flags: needinfo?(bas)
(In reply to Milan Sreckovic [:milan] from comment #11)
> Bas, to confirm, Skia GL is off by default on Android, so this bug only
> shows up with the preference forcing it?
> 
> Also, Bas, who should look at Sotaro's suggestion in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1206076#c26 as to where the
> problem may lie?  Doesn't sound like it's specific to Android?

No, on -android- it's still -on- by default, I've suggested switching it off to fix beta issues.
Flags: needinfo?(bas)
Assignee: nobody → jnicol
The flicker occurs when the call to eglClientWaitSync() in EGLImageTextureHost::Lock() fails. (The assertion gets hit in debug builds, though this is much less likely to occur in debug builds.) Need to work out why that call is failing.
The call fails because the sync has already been destroyed when the SharedSurface was destroyed from the client side. I think we're missing some synchronisation to ensure it remains alive long enough for the host.
Attached patch Patch v1Splinter Review
Nical, are you okay to review this? From our chat on irc it seems like this is a necessary evil, at least for the time being?
Attachment #8675760 - Flags: review?(nical.bugzilla)
Attachment #8675760 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8675760 [details] [diff] [review]
Patch v1

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

::: gfx/layers/client/TextureClientSharedSurface.cpp
@@ +26,5 @@
>                                                         gl::SurfaceFactory* factory)
>    : TextureClient(aAllocator, aFlags | TextureFlags::RECYCLE)
>    , mSurf(Move(surf))
> +{
> +  AddFlags(mSurf->GetTextureFlags());

Are we going to fail this assertion when calling AddFlags? https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#225
> Are we going to fail this assertion when calling AddFlags?
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TextureClient.cpp#225

I am unsure at what point the TextureClient gets shared with the compositor, but in any case the RECYCLE flag is set and the texture will not have been added to the CompositableClient yet, so we're fine.

Here is a good try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a11554285480
(With some unrelated intermittent failures). Requesting checkin please.
Keywords: checkin-needed
Do we want this for 42 and 43? Or were we just disabling skiagl on those?
Flags: needinfo?(milan)
(In reply to Jamie Nicol [:jnicol] from comment #18)
> Do we want this for 42 and 43? Or were we just disabling skiagl on those?

Let's request the uplift for 42 and 43.
Flags: needinfo?(milan)
https://hg.mozilla.org/mozilla-central/rev/302c448ac938
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8675760 [details] [diff] [review]
Patch v1

Approval Request Comment
[Feature/regressing bug #]: bug 1206076
[User impact if declined]: Flickering animations on websites using canvas
[Describe test coverage new/current, TreeHerder]: Try tested on nightly, manually verified to build and fix regression on beta. Relevant code has not changed.
[Risks and why]: Low risk. Only changes a specific type of texture client. The same mechanism is already used elsewhere without problems.
[String/UUID change made/needed]: N/A
Attachment #8675760 - Flags: approval-mozilla-beta?
Attachment #8675760 - Flags: approval-mozilla-aurora?
Comment on attachment 8675760 [details] [diff] [review]
Patch v1

We are doing a 42 beta 9 for android, taking it because it might affect a bunch of website.
Attachment #8675760 - Flags: approval-mozilla-beta?
Attachment #8675760 - Flags: approval-mozilla-beta+
Attachment #8675760 - Flags: approval-mozilla-aurora?
Attachment #8675760 - Flags: approval-mozilla-aurora+
tracking-fennec: --- → ?
tracking-fennec: ? → 42+
You need to log in before you can comment on or make changes to this bug.