Last Comment Bug 626694 - [OGL] Inverse website colours
: [OGL] Inverse website colours
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla10
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on:
Blocks: opengl-mobile
  Show dependency treegraph
 
Reported: 2011-01-18 10:22 PST by Aaron Train [:aaronmt]
Modified: 2011-10-12 03:18 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Possible Fix (5.08 KB, patch)
2011-02-05 20:27 PST, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Remove IsRGB (203 bytes, patch)
2011-10-11 07:01 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Remove IsRGB (1.45 KB, patch)
2011-10-11 13:22 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Remove IsRGB (5.54 KB, patch)
2011-10-11 13:31 PDT, Benoit Girard (:BenWa)
matt.woodrow: review+
Details | Diff | Review

Description Aaron Train [:aaronmt] 2011-01-18 10:22:28 PST
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre Fennec/4.0b4pre ID:20110118042354

Seen this happen on:

http://m.digg.com
http://pastebin.com
http://news.ycombinator.com
http://m.cnn.com

Device: HTC Google Nexus One

Prefs:
layers.acceleration.disabled, false
layers.accelerate-all, true
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-01-19 12:48:46 PST
I can also see this.
Comment 2 Jeff Muizelaar [:jrmuizel] 2011-01-19 12:59:03 PST
It looks like R and B are swapped for some reason.
Comment 3 Jeff Muizelaar [:jrmuizel] 2011-01-19 14:48:03 PST
This happens on my Galaxy S too. It must have regressed in the last little bit.
Comment 4 Jeff Muizelaar [:jrmuizel] 2011-01-19 14:49:07 PST
Romaxa, do you think the EGL changes broke this?
Comment 5 Oleg Romashin (:romaxa) 2011-01-20 14:05:49 PST
hmmm.. I see it on my Galaxy Tab too.. I'll check it tomorrow.
Comment 6 Oleg Romashin (:romaxa) 2011-01-21 01:29:32 PST
with backing surface path it render fine, but without backing surface it swap colors also on maemo6.

Here we are expecting BGRALayerProgramType always
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/GLContextProviderEGL.cpp#1029

and color swap happening because we are changing mShaderType here: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/GLContext.cpp#1358
and the rest of code seems does not care about that and some messup happening around
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-01-21 15:53:38 PST
Looks like this was caused by the component alpha changes:

      // Note BGR: Cairo's image surfaces are always in what
      // OpenGL and our shaders consider BGR format.
      ColorTextureLayerProgram *basicProgram =
        aManager->GetBasicLayerProgram(mLayer->CanUseOpaqueSurface(),
                                       !mTexImage->IsRGB());

The comment is wrong, and my guess is that mTexImage doesn't have the right IsRGB().
Comment 8 Oleg Romashin (:romaxa) 2011-01-22 10:37:01 PST
sure, you right, IsRGB I guess already dead after recent changes related to mShaderType...
Comment 9 Oleg Romashin (:romaxa) 2011-01-22 10:39:26 PST
And reason why it works with backing surface is CreateBackingSurface mIsRGBFormat = PR_TRUE.
Comment 10 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-02-05 20:27:57 PST
Created attachment 510086 [details] [diff] [review]
Possible Fix

I haven't actually tested this other than confirm that it compiles and doesn't horribly break anything on mac.

Romaxa: Can you please check if this fixes the issue on maemo and that I haven't accidentally broken anything else in the EGL provider.
Comment 11 Ali Juma [:ajuma] 2011-07-19 13:28:24 PDT
This does not seem to be happening anymore. (Testing on a Nexus S, it was fixed sometime between the nightly builds of April 8 and April 10.) Any objections to marking this Resolved?
Comment 12 Benoit Girard (:BenWa) 2011-07-25 07:24:46 PDT
Matt the issue doesn't seem to reproduce. Are the changes still needed?
Comment 13 Benoit Girard (:BenWa) 2011-07-25 13:22:46 PDT
Taking the bug so it has an owner.
Comment 14 Benoit Girard (:BenWa) 2011-07-25 13:28:46 PDT
This may be related to bug 661002.
Comment 15 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-07-25 15:20:31 PDT
Removing the IsRGB() code paths still seem worthwhile as it's been replaced by GetShaderProgramType().

I'm not sure about the other hunks, probably not if the bug no longer happens.
Comment 16 Benoit Girard (:BenWa) 2011-10-11 07:01:58 PDT
Created attachment 566205 [details] [diff] [review]
Remove IsRGB
Comment 17 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-10-11 13:05:01 PDT
I think you may forgotten to qref this patch BenWa. You can definitely have an r+ in it's current form though.
Comment 18 Benoit Girard (:BenWa) 2011-10-11 13:22:23 PDT
Created attachment 566316 [details] [diff] [review]
Remove IsRGB

Opps!
Comment 19 Benoit Girard (:BenWa) 2011-10-11 13:31:14 PDT
Created attachment 566320 [details] [diff] [review]
Remove IsRGB

Sorry, trying to work on too many patches at once :(
Comment 20 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-10-11 16:30:33 PDT
Comment on attachment 566320 [details] [diff] [review]
Remove IsRGB

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

Is that assertion really the only code reading IsRGB()? r+++++
Comment 21 Marco Bonardo [::mak] 2011-10-12 03:18:11 PDT
https://hg.mozilla.org/mozilla-central/rev/38789564d25b
Comment 22 Marco Bonardo [::mak] 2011-10-12 03:18:46 PDT
https://hg.mozilla.org/mozilla-central/rev/6b0b58d5da86

Note You need to log in before you can comment on or make changes to this bug.