Closed Bug 947504 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: dmarcos, Assigned: diego)

References

Details

(Keywords: regression)

Attachments

(5 files, 4 obsolete files)

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
There's something really weird going on here. Can we find out if this reproduces on 1.1 HD or 1.2?
blocking-b2g: --- → 1.3?
Component: Gaia → Graphics
Keywords: qawanted, regression
Product: Firefox OS → Core
I don't see the issue in 1.1HD nor 1.2
QA Contact: mvaughan
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
CJ, does your team have access to this phone so that we can get a tighter regression range?
Flags: needinfo?(cku)
Sure.

Vincent, please figure out which patch cause this regression and the root cause
Flags: needinfo?(cku) → needinfo?(vlin)
(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.
Flags: needinfo?(vlin) → needinfo?(dmarcos)
We have a special path in the HWC to swap R and B and maybe the Helix binaries don't have that code?
(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.
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!
Summary: [Helix] Screen colors are off and display blinks → [v1.3] Screen colors are off and display blinks
(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!
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
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.
Component: Graphics → Vendcom
Product: Core → Firefox OS
Wayne - Can you contact our partner to get a 1.3 base image for Helix?
Flags: needinfo?(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.
Flags: needinfo?(dmarcos)
Whiteboard: [POVB]
removing nom as this is a vendor issue
blocking-b2g: 1.3? → ---
Flags: needinfo?(wchang)
(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.
Duplicate of this bug: 958460
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.
Blocks: 950800
blocking-b2g: --- → 1.3?
Component: Vendcom → General
Whiteboard: [POVB]
Blocks: 952916
Please comment from UX perspective
Flags: needinfo?(firefoxos-ux-bugzilla)
Assignee: nobody → dwilson
Attached patch Part 1 - gecko patch - m-c - v1 (obsolete) — Splinter Review
Attached patch Part 1 - gecko patch - 1.3 - v1 (obsolete) — Splinter Review
Attachment #8359905 - Attachment description: Part 2 - gaia patch - m-c - v1 → Part 2 - gaia pull request - m-c - v1
Attachment #8359901 - Flags: review?(kchen)
Attachment #8359905 - Flags: review?(kchen)
The patches above ensure HWC is only enabled by default on devices that fully support it.
Status: NEW → ASSIGNED
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 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.
Attachment #8359901 - Flags: review?(kchen) → review+
Comment on attachment 8359903 [details] [diff] [review]
Part 1 - gecko patch - 1.3 - v1

Please fix this up as above.
Attachment #8359903 - Flags: review+
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?
(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".
(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.
Attached patch Part 1 - gecko patch - m-c - v2 (obsolete) — Splinter Review
Attachment #8359901 - Attachment is obsolete: true
Attachment #8360061 - Flags: review+
Attached patch Part 1 - gecko patch - 1.3 - v2 (obsolete) — Splinter Review
Attachment #8359903 - Attachment is obsolete: true
Attachment #8360062 - Flags: review+
Attachment #8360061 - Attachment is obsolete: true
Attachment #8360092 - Flags: review+
Attachment #8360062 - Attachment is obsolete: true
Attachment #8360094 - Flags: review+
(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.
Attachment #8359905 - Flags: review?(kchen) → review?(yurenju.mozilla)
Attachment #8359905 - Flags: review?(yurenju.mozilla) → review+
Keywords: checkin-needed
Sounds like this should be in 1.3.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(firefoxos-ux-bugzilla)
https://hg.mozilla.org/integration/b2g-inbound/rev/6cc364f9d1b2
Master: 9e7f488adbc104c36341f2943d74a0e552e86f55
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6cc364f9d1b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Duplicate of this bug: 952916
Duplicate of this bug: 956265
Duplicate of this bug: 961059
You need to log in before you can comment on or make changes to this bug.