947504 - [v1.3] Screen colors are off and display blinks

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Diego Marcos [:dmarcos]]

Attachment: Video illustrating the screen issue

Running the following:

Gaia      64be4c387d839361142975cbac1642c77220669b
Gecko     http://hg.mozilla.org/mozilla-central/rev/725c36b5de1a
BuildID   20131205040201
Version   28.0a1
ro.build.version.incremental=eng.wanglei.20131106.235553

Steps to reproduce:

Just use your phone and enjoy the trip

Comment 1

[Jason Smith [:jsmith]]

There's something really weird going on here. Can we find out if this reproduces on 1.1 HD or 1.2?

Comment 2

[Diego Marcos [:dmarcos]]

I don't see the issue in 1.1HD nor 1.2

Comment 3

[Matthew Vaughan]

This issue looks to have started reproducing on the 12/01 1.3 HD build. Builds before 12/01 seemed to have an issue with colors being distorted at rare occasions, but the 12/01 and later builds reproduce the issue with distorted colors 100% of the time.

- Works -
Environmental Variables:
Device: Helix v1.3 MOZ RIL
BuildID: 20131130040205
Gaia: f23aebebcd28c4d19347def77c4836c8baebc2ce
Gecko: e9337081c744
Version: 28.0a1
Firmware Version: Y300-F1V100R001C00B004SP01

- Broken -
Environmental Variables:
Device: Buri v1.3 MOZ RIL
BuildID: 20131201040201
Gaia: f23aebebcd28c4d19347def77c4836c8baebc2ce
Gecko: 84a5a5800bd3
Version: 28.0a1
Firmware Version: Y300-F1V100R001C00B004SP01

Comment 4

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

CJ, does your team have access to this phone so that we can get a tighter regression range?

Comment 5

[u459114]

Sure.

Vincent, please figure out which patch cause this regression and the root cause

Comment 6

[Vincent Lin[:vilin]]

(In reply to Diego Marcos from comment #0)
> Created attachment 8344064 [details]
> Video illustrating the screen issue
> 
> Running the following:
> 
> Gaia      64be4c387d839361142975cbac1642c77220669b
> Gecko     http://hg.mozilla.org/mozilla-central/rev/725c36b5de1a
> BuildID   20131205040201
> Version   28.0a1
> ro.build.version.incremental=eng.wanglei.20131106.235553
> 
> Steps to reproduce:
> 
> Just use your phone and enjoy the trip

Well, it seems caused by HWComposer.

Please disable it for verification.
(rename or remove hwcomposer.msm7627a.so in system/lib/hw)
Meanwhile, I will also find the phone, code base/binary and then try to verify it.

Comment 7

[Andreas Gal :gal]

We have a special path in the HWC to swap R and B and maybe the Helix binaries don't have that code?

Comment 8

[William Hsu [:whsu]]

(In reply to Matthew Vaughan from comment #3)
> This issue looks to have started reproducing on the 12/01 1.3 HD build.
> Builds before 12/01 seemed to have an issue with colors being distorted at
> rare occasions, but the 12/01 and later builds reproduce the issue with
> distorted colors 100% of the time.
> 
> - Works -
> Environmental Variables:
> Device: Helix v1.3 MOZ RIL
> BuildID: 20131130040205
> Gaia: f23aebebcd28c4d19347def77c4836c8baebc2ce
> Gecko: e9337081c744
> Version: 28.0a1
> Firmware Version: Y300-F1V100R001C00B004SP01
> 
> - Broken -
> Environmental Variables:
> Device: Buri v1.3 MOZ RIL
> BuildID: 20131201040201
> Gaia: f23aebebcd28c4d19347def77c4836c8baebc2ce
> Gecko: 84a5a5800bd3
> Version: 28.0a1
> Firmware Version: Y300-F1V100R001C00B004SP01

Hi, Matthew,

Thanks for your help.
Could I have a request?
Could you please use the same device to find the regression window next time?
If you use the different devices to a reproduce bug, it is difficult to narrow down the root cause.

Also, please provide the correct information.
I think the firmware Version you provided is incorrect.
Thanks.

Comment 9

[William Hsu [:whsu]]

HI, all,

After I disabled the HWC (hwcomposer.msm7627a.so), the device works as normal.
This bug also happened on Buri device. We need our partner to help on this problem.
Thanks.

* Test Build:
 - Gaia:     1dd0e5c644b4c677a4e8fa02e50d52136db489d9
 - Gecko:    http://hg.mozilla.org/mozilla-central/rev/725c36b5de1a
 - BuildID   20131205040201
 - Version   28.0a1

* Actual Result: Can reproduce this bug.
                 But I cannot reproduce this bug after I removed the hwcomposer.msm7627a.so




Thanks!

Comment 10

[Matthew Vaughan]

(In reply to William Hsu [:whsu] from comment #8)
> (In reply to Matthew Vaughan from comment #3)
> > This issue looks to have started reproducing on the 12/01 1.3 HD build.
> > Builds before 12/01 seemed to have an issue with colors being distorted at
> > rare occasions, but the 12/01 and later builds reproduce the issue with
> > distorted colors 100% of the time.
> > 
> > - Works -
> > Environmental Variables:
> > Device: Helix v1.3 MOZ RIL
> > BuildID: 20131130040205
> > Gaia: f23aebebcd28c4d19347def77c4836c8baebc2ce
> > Gecko: e9337081c744
> > Version: 28.0a1
> > Firmware Version: Y300-F1V100R001C00B004SP01
> > 
> > - Broken -
> > Environmental Variables:
> > Device: Buri v1.3 MOZ RIL
> > BuildID: 20131201040201
> > Gaia: f23aebebcd28c4d19347def77c4836c8baebc2ce
> > Gecko: 84a5a5800bd3
> > Version: 28.0a1
> > Firmware Version: Y300-F1V100R001C00B004SP01
> 
> Hi, Matthew,
> 
> Thanks for your help.
> Could I have a request?
> Could you please use the same device to find the regression window next time?
> If you use the different devices to a reproduce bug, it is difficult to
> narrow down the root cause.
> 
> Also, please provide the correct information.
> I think the firmware Version you provided is incorrect.
> Thanks.

Hello William,

Your welcome for the help :). 
I do apologize for the mistakes with the regression window though. I meant to write "Helix" and not "Buri" in the Broken build's environmental variables. So the regression window was found using only the Helix device. As for the firmware version, it seems the only one available to us right now, and when the Helix was imaged, is the 10252013 image. I just wanted to make sure this info was in the bug. Sorry again!

Comment 11

[Diego Wilson [:diego]]

Definitely looks like the RB swap issue. Can you please make sure you have the following patch?

https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/commit/?h=b2g_ics_1.2&id=c8afdc78fc7ca376c43f43d5126fd110c54967cd

You should probably be using the CAF b2g_ics_1.2 branch for hardware/qcom/display on this device:

https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/log/?h=b2g_ics_1.2

Comment 12

[Andreas Gal :gal]

Yes, and the blinking is probably the other bug you had in CAF where the HWC failed to report back that it wasn't able to compose something and we didn't resort to GPU composition. This device needs an updated base system.

Comment 13

[Jason Smith [:jsmith]]

Wayne - Can you contact our partner to get a 1.3 base image for Helix?

Comment 14

[Wayne Chang [:wchang]]

(In reply to Jason Smith [:jsmith] from comment #13)
> Wayne - Can you contact our partner to get a 1.3 base image for Helix?

Will do. Keeping NI for myself to revisit.

Comment 15

[Jason Smith [:jsmith]]

removing nom as this is a vendor issue

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

Is bug 958460 a dupe?

Comment 17

[Jason Smith [:jsmith]]

(In reply to Kan-Ru Chen [:kanru] from comment #16)
> Is bug 958460 a dupe?

Yes. We actually confirmed that this works fine with hardware composer disabled.

Comment 19

[Jason Smith [:jsmith]]

Turns out per the dupe that this actually is not a vendor bug - it's actually because we're turning the hardware composer on for unsupported devices.

Comment 20

[Preeti Raghunath(:Preeti)]

Please comment from UX perspective

Comment 21

[Diego Wilson [:diego]]

Attachment: Part 1 - gecko patch - m-c - v1

Comment 22

[Diego Wilson [:diego]]

Attachment: Part 1 - gecko patch - 1.3 - v1

Comment 23

[Diego Wilson [:diego]]

Attachment: Part 2 - gaia pull request - m-c - v1

Comment 24

[Diego Wilson [:diego]]

Attachment: Part 2 - gaia pull request - 1.3 - v1

Comment 25

[Diego Wilson [:diego]]

The patches above ensure HWC is only enabled by default on devices that fully support it.

Comment 26

[Andreas Gal :gal]

Comment on attachment 8359901 [details] [diff] [review]
Part 1 - gecko patch - m-c - v1

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

::: b2g/app/b2g.js
@@ -279,5 @@
>  pref("layers.offmainthreadcomposition.async-animations", true);
>  pref("layers.async-video.enabled", true);
>  pref("layers.async-pan-zoom.enabled", true);
>  pref("gfx.content.azure.backends", "cairo");
> -pref("layers.composer2d.enabled", true);

You probably want to leave this in here and let it default to false.

::: b2g/chrome/content/settings.js
@@ +598,5 @@
>    Services.prefs.setBoolPref("layers.draw-borders", value);
>  });
> +
> +(function Composer2DSettingToPref() {
> +  //layers.composer.enabled can be toggled in three ways

Reformat this a little nicer please:

// layers.composer.enabled can be enabled in three ways. In order of precedence they are:
//
// 1. mozSettings "layers.composer.enabled"
// 2. a gecko pref "layers.composer.enabled"
// 3. presence of ro.display.colorfill prop at the Gonk level

@@ +606,5 @@
> +  //Third: ro.display.colorfill gonk prop
> +
> +  var req = navigator.mozSettings.createLock().get('layers.composer2d.enabled');
> +  req.onsuccess = function() {
> +    if(typeof(req.result['layers.composer2d.enabled']) == 'undefined') {

if_( and === instead of ==

@@ +610,5 @@
> +    if(typeof(req.result['layers.composer2d.enabled']) == 'undefined') {
> +      var enabled = false;
> +      if (Services.prefs.getPrefType('layers.composer2d.enabled') == Ci.nsIPrefBranch.PREF_BOOL) {
> +        enabled = Services.prefs.getBoolPref('layers.composer2d.enabled');
> +      }

} else {, however I think what you really want here is a fresh

if (!enabled) so this only runs if the pref is present, boolean, and false (or not present)

@@ +613,5 @@
> +        enabled = Services.prefs.getBoolPref('layers.composer2d.enabled');
> +      }
> +      else {
> +#ifdef MOZ_WIDGET_GONK
> +        if (libcutils.property_get('ro.display.colorfill') == 1) {

=== (you want to use === in JS because == is expensive, it does toString on objects with side effects and what not, which tends to cause hard to debug problems when things go wrong)

@@ +617,5 @@
> +        if (libcutils.property_get('ro.display.colorfill') == 1) {
> +          enabled = true;
> +        }
> +        else {
> +          enabled = false;

isn't this just enabled = (property_get() === 1)? instead of the if

@@ +622,5 @@
> +        }
> +#endif
> +      }
> +      navigator.mozSettings.createLock().set(
> +        {'layers.composer2d.enabled': enabled});

No new line before the { and a space after enabled

Comment 27

[Andreas Gal :gal]

Comment on attachment 8359901 [details] [diff] [review]
Part 1 - gecko patch - m-c - v1

Its all nits so I don't think there is a need to go over this again. Stealing from kchen.

Comment 28

[Andreas Gal :gal]

Comment on attachment 8359903 [details] [diff] [review]
Part 1 - gecko patch - 1.3 - v1

Please fix this up as above.

Comment 29

[Andreas Gal :gal]

Comment on attachment 8359905 [details] [review]
Part 2 - gaia pull request - m-c - v1

Don't you want to set that to false instead of removing it?

Comment 30

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

(In reply to Andreas Gal :gal from comment #29)
> Comment on attachment 8359905 [details] [review]
> Part 2 - gaia pull request - m-c - v1
> 
> Don't you want to set that to false instead of removing it?

I think the design is that it should be removed.  The idea is that no preference means use the automatic "does the hardware support it", preference to true means "force on, regardless of what the hardware support is" and false means "force off, regardless of what the hardware support is".

Comment 31

[Diego Wilson [:diego]]

(In reply to Milan Sreckovic [:milan] from comment #30)
> (In reply to Andreas Gal :gal from comment #29)
> > Comment on attachment 8359905 [details] [review]
> > Part 2 - gaia pull request - m-c - v1
> > 
> > Don't you want to set that to false instead of removing it?
> 
> I think the design is that it should be removed.  The idea is that no
> preference means use the automatic "does the hardware support it",
> preference to true means "force on, regardless of what the hardware support
> is" and false means "force off, regardless of what the hardware support is".

Milan is correct.

Comment 32

[Diego Wilson [:diego]]

Attachment: Part 1 - gecko patch - m-c - v2

Comment 33

[Diego Wilson [:diego]]

Attachment: Part 1 - gecko patch - 1.3 - v2

Comment 34

[Diego Wilson [:diego]]

Attachment: Part 1 - gecko patch - m-c - v3

Comment 35

[Diego Wilson [:diego]]

Attachment: Part 1 - gecko patch - 1.3 - v3

Comment 36

[Diego Wilson [:diego]]

(In reply to Andreas Gal :gal from comment #26)
> Comment on attachment 8359901 [details] [diff] [review]
> } else {, however I think what you really want here is a fresh
> 
> if (!enabled) so this only runs if the pref is present, boolean, and false
> (or not present)
>
> @@ +613,5 @@
> > +        enabled = Services.prefs.getBoolPref('layers.composer2d.enabled');
> > +      }
> > +      else {
> > +#ifdef MOZ_WIDGET_GONK
> > +        if (libcutils.property_get('ro.display.colorfill') == 1) {
> 

The gonk prop should only be queried if the gecko pref does not exist. The gecko pref is has precedence for enabling and disabling HWC, so it shall be honored even if it's false.

All other comments have been addressed.

Comment 37

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

Sounds like this should be in 1.3.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6cc364f9d1b2
Master: 9e7f488adbc104c36341f2943d74a0e552e86f55

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cc364f9d1b2

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4bc0d1cae6f
v1.3: 0b364cd4d653493ef25d0189674a412ff95582e3