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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: video of the bug (mp4 format)

Here's a video screencast of the bug. (in mp4 format, because that's what "adb shell screenrecord" spits out)

Comment 2

[Daniel Holbert [:dholbert]]

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
}

Comment 3

[Daniel Holbert [:dholbert]]

I'm using a OnePlus One phone, with CyanogenMod 12.1 (equivalent to Android 5.1.1).

Comment 4

[Daniel Holbert [:dholbert]]

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")

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

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

:snorp, can you take a look at this, Jamie is a bit swamped.

Comment 7

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

[Tracking Requested - why for this release]:
This is from a SkiaGL canvas fix that was uplifted to 42.

Comment 8

[Daniel Holbert [:dholbert]]

(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.

Comment 9

[Daniel Holbert [:dholbert]]

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.]

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking.

Comment 11

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

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?

Comment 12

[Bas Schouten (:bas.schouten)]

(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.

Comment 13

[Jamie Nicol [: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.

Comment 14

[Jamie Nicol [:jnicol]]

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.

Comment 15

[Jamie Nicol [:jnicol]]

Attachment: Patch v1

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?

Comment 16

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

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

Comment 17

[Jamie Nicol [:jnicol]]

> 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.

Comment 18

[Jamie Nicol [:jnicol]]

Do we want this for 42 and 43? Or were we just disabling skiagl on those?

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/302c448ac938

Comment 20

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

(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.

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/302c448ac938

Comment 22

[Jamie Nicol [:jnicol]]

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

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b7656e9f3b1

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e7061d4d10fd