1239702 - [Lockscreen][Flame] The unlock slider is very laggy

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
1. Lock your phone.
2. Wake up the phone.
3. Use the slider (do not push it quickly to the right so that it stays locked)

=> Observe this is very laggy.

This is observed on the Flame only, Aries is very smooth.

Comment 1

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I'll perform branch checks first to verify this is a regression.

Comment 2

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue DOES occur on the latest Flame 2.6 build.

Actual Results: The slider is laggy when being dragged.

Environmental Variables:
Device: Flame 2.6
BuildID: 20160114030239
Gaia: 2d6c11a1e82bd9f9b7491863c8d1ea4a14aea69f
Gecko: 6fa2ab99f52feb1b6ead5581b8f5d398546a55a5
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 46.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:46.0) Gecko/46.0 Firefox/46.0


This issue does NOT occur on the latest Flame 2.5 build.

Actual Results: The slider moves smoothly.

Environmental Variables:
Device: Flame 2.5
BuildID: 20160114100142
Gaia: 1dec9c2fba119aabcc82c95225dbef24de148323
Gecko: 30109dab01950921009775a05d309f224615ad2e
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 44.0 (2.5) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Mozilla-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.6
BuildID: 20151218104833
Gaia: d069027f9af6f835ef869f1f01b52339e5a3f423
Gecko: d8e260e1535f0ff3877b21d6846380dedf449453
Version: 46.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:46.0) Gecko/46.0 Firefox/46.0

First Broken 
Environmental Variables:
Device: Flame 2.6
BuildID: 20151218105736
Gaia: d069027f9af6f835ef869f1f01b52339e5a3f423
Gecko: 6630a176477ef903dcf3057db755abc43ba5018d
Version: 46.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:46.0) Gecko/46.0 Firefox/46.0

Last Working gaia / First Broken gecko - Issue DOES occur
Gaia: d069027f9af6f835ef869f1f01b52339e5a3f423
Gecko: 6630a176477ef903dcf3057db755abc43ba5018d

First Broken gaia / Last Working gecko - Issue does NOT occur
Gaia: d069027f9af6f835ef869f1f01b52339e5a3f423
Gecko: d8e260e1535f0ff3877b21d6846380dedf449453

Gecko Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d8e260e1535f0ff3877b21d6846380dedf449453&tochange=6630a176477ef903dcf3057db755abc43ba5018d

Comment 4

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Lee can you please take a look at this issue?  Seems to have been caused by the changes for bug 1082598.

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to Jayme Mercado [:JMercado] from comment #4)
> Lee can you please take a look at this issue?  Seems to have been caused by
> the changes for bug 1082598.

I'm not really set up to debug FF-OS stuff at the moment. Milan, do you know someone who might be better to look into this?

Comment 6

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

It will be good to understand where the performance drop came from, as it may apply to Android as well.

Jayme or Julien - if you set gfx.canvas.azure.accelerated to false, and repeat the comparisons, is there a difference between the two versions?

Comment 7

[No-Jun Park [:njpark]]

Adding qawanted for comment 6

Comment 8

[KTucker [:KTucker][Inactive 3/4/2016]]

Isn't this a dupe of bug 1220812?

Comment 9

[No-Jun Park [:njpark]]

In 2.5, whether gfx.canvas.azure.accelerated was set to true or false, both cases the switch UI was smooth.
In 2.6, in both cases, the UI was laggy, and it was slightly worse when gfx.canvas.azure.accelerated was set to false.

Comment 10

[No-Jun Park [:njpark]]

(In reply to KTucker [:KTucker] from comment #8)
> Isn't this a dupe of bug 1220812?

It's slightly different, since in this case it's the responsiveness of the unlock switch that's lagging.

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to No-Jun Park [:njpark] from comment #10)
> (In reply to KTucker [:KTucker] from comment #8)
> > Isn't this a dupe of bug 1220812?
> 
> It's slightly different, since in this case it's the responsiveness of the
> unlock switch that's lagging.

Yes, here it's clearly a graphics issue while bug 1220812 is more a gaia thing.

Comment 12

[Sotaro Ikeda [:sotaro]]

When gfx.canvas.azure.backends was cairo, UI was smooth.

Comment 13

[Sotaro Ikeda [:sotaro]]

Took profile on master flame-kk with the following
- gfx.canvas.azure.backends skia
- gfx.canvas.azure.accelerated false

http://people.mozilla.org/~bgirard/cleopatra/#report=6975ccedd40fa7cd5c79aa0ba8a28d3e8b683e95&selection=0,1,2,3,4,5,6,45,46,47,8,9,10,11,12,13,14,15,16,17,18,36,24,25,26,27,28

Comment 14

[Sotaro Ikeda [:sotaro]]

On b2g v2.5, Color32_arm_neon() is used in SkARGB32_Blitter::blitAntiH().
  > https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/file/tip/gfx/skia/skia/src/opts/SkBlitRow_opts_arm_neon.cpp#l1539
But on master, SK_OPTS_NS::blit_row_color32() is used instead.
  > https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/opts/SkBlitRow_opts.h#20

Comment 15

[Sotaro Ikeda [:sotaro]]

Somehow none version of Sk4px::Wide::addNarrowHi() was used instead of neon version
 https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/opts/Sk4px_none.h#57

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix SK_ARM_HAS_NEON build config

The patch addressed the problem on my flame-kk.

Comment 17

[Sotaro Ikeda [:sotaro]]

Master skis still seems to have the problem.
 https://skia.googlesource.com/skia/+/master/include/core/SkPreConfig.h#196

Comment 18

[Lee Salzman [:lsalzman]]

Comment on attachment 8711997 [details] [diff] [review]
patch - Fix SK_ARM_HAS_NEON build config

Currently we're using SK_ARM_HAS_OPTIONAL_NEON, meaning that it should be dynamically checking for neon.

So long as mozilla::supports_neon() returns true, and thus sk_cpu_arm_has_neon() returns true, it should have the same effect.

Finding the bug with the dynamic checking would be preferable to modifying a Skia header file.

Alternatively if it needs to be static it can be forced with SK_ARM_HAS_NEON in the moz.build

Comment 19

[Sotaro Ikeda [:sotaro]]

The way of choosing of Sk4px is static. It seems better to force SK_ARM_HAS_NEON in the moz.build, instread of making it dynamic in gecko.
  https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/Sk4px.h#237

Comment 20

[Lee Salzman [:lsalzman]]

(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> The way of choosing of Sk4px is static. It seems better to force
> SK_ARM_HAS_NEON in the moz.build, instread of making it dynamic in gecko.
>  
> https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/Sk4px.
> h#237

Googling around, it appears that it's not so much as it is that depending on compiler version you can end up getting __ARM_NEON or __ARM_NEON__, with __ARM_NEON allegedly being newer, and __ARM_NEON__ being legacy.

So, okay, I think I understand what's going on now. The problem is most likely that the dynamic detection IS working, just that we compile the relevant SkOpts_neon.cpp with -fcpu=neon in the moz.build. For whatever reason, our compiler defines __ARM_NEON__ but not __ARM_NEON? So even though SkOpts_neon.cpp is getting used, it is not being compiled properly...

So maybe the better patch would be to check for both __ARM_NEON and __ARM_NEON__, so both cases are handled, rather than just checking for one or the other.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix SK_ARM_HAS_NEON build config

Updated the patch based on the comment on irc.

Comment 22

[Lee Salzman [:lsalzman]]

Comment on attachment 8712009 [details] [diff] [review]
patch - Fix SK_ARM_HAS_NEON build config

Submitted upstream bug report here with patch: https://codereview.chromium.org/1630903004/

Comment 23

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f509224136ed

Comment 24

[Lee Salzman [:lsalzman]]

Skia accepted this upstream, and Android builds still look green, so we're good now: https://skia.googlesource.com/skia/+/05332d9c8aefe00c361c893410deca33f4438695

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17427005e936

Comment 26

[Julien Wajsberg [:julienw]]

Thanks a lot for caring about Flame :)

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17427005e936

Comment 28

[Martin Shuman [:Marty]]

This issue is verified fixed on the latest Flame 2.6 build.
The unlock slider moves slowly, and doesn't appear to lag.

Environmental Variables:
Device: Flame 2.6
BuildID: 20160225030411
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: c1e0d1890cfee9d86c8d566b0490053f21e0afc6
Gonk: 8a066f7fa7410e32b58def35f322aa33f03db283
Version: 47.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:47.0) Gecko/47.0 Firefox/47.0