Closed Bug 626694 Opened 9 years ago Closed 8 years ago

[OGL] Inverse website colours

Categories

(Core :: Graphics, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
fennec + ---

People

(Reporter: aaronmt, Assigned: BenWa)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Component: General → Graphics
Product: Fennec → Core
QA Contact: general → thebes
I can also see this.
It looks like R and B are swapped for some reason.
tracking-fennec: --- → ?
This happens on my Galaxy S too. It must have regressed in the last little bit.
Romaxa, do you think the EGL changes broke this?
hmmm.. I see it on my Galaxy Tab too.. I'll check it tomorrow.
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
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().
sure, you right, IsRGB I guess already dead after recent changes related to mShaderType...
And reason why it works with backing surface is CreateBackingSurface mIsRGBFormat = PR_TRUE.
tracking-fennec: ? → 2.0next+
Attached patch Possible Fix (obsolete) — Splinter Review
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.
Attachment #510086 - Flags: feedback?
Attachment #510086 - Flags: feedback? → feedback?(romaxa)
tracking-fennec: 2.0next+ → 7+
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?
tracking-fennec: 7+ → +
Matt the issue doesn't seem to reproduce. Are the changes still needed?
Taking the bug so it has an owner.
Assignee: nobody → bgirard
This may be related to bug 661002.
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.
Attached patch Remove IsRGB (obsolete) — Splinter Review
Attachment #566205 - Flags: review?(matt.woodrow)
I think you may forgotten to qref this patch BenWa. You can definitely have an r+ in it's current form though.
Attached patch Remove IsRGB (obsolete) — Splinter Review
Opps!
Attachment #566205 - Attachment is obsolete: true
Attachment #566316 - Flags: review?(matt.woodrow)
Attachment #566205 - Flags: review?(matt.woodrow)
Attachment #566316 - Attachment is patch: true
Attachment #566316 - Attachment is obsolete: true
Attachment #566316 - Flags: review?(matt.woodrow)
Attached patch Remove IsRGBSplinter Review
Sorry, trying to work on too many patches at once :(
Attachment #566320 - Flags: review?(matt.woodrow)
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+++++
Attachment #566320 - Flags: review?(matt.woodrow) → review+
Attachment #510086 - Attachment is obsolete: true
Attachment #510086 - Flags: feedback?(romaxa)
https://hg.mozilla.org/mozilla-central/rev/38789564d25b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.