Closed Bug 975121 Opened 6 years ago Closed 6 years ago

Linux - reversed colors with gfx.xrender.enabled = false

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: johns, Assigned: mtseng)

References

Details

Attachments

(1 file, 1 obsolete file)

With gfx.xrender.enabled set to false* and hardware acceleration enabled, I've noticed sporadicly reversed colors in linux. I bisected the culprit to:

http://hg.mozilla.org/mozilla-central/rev/c0e256be477547426276d2a9d078e7a4c6c1729b

This occured once before with linux OMTC: Bug 895954
Which :mattwoodrow tracked down in bug 910488 comment 5
A site I've found to reliably reproduce is:
https://github.com/mozilla/

The project links show up orange, instead of blue.
Attached patch check_GetPreferredARGB32Format (obsolete) — Splinter Review
Need to check GetPreferredARGB32Format in this condition for correct result.

try push: https://tbpl.mozilla.org/?tree=Try&rev=72a5076ed23e
Assignee: nobody → mtseng
Attachment #8379473 - Flags: review?(gal)
Comment on attachment 8379473 [details] [diff] [review]
check_GetPreferredARGB32Format

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

I don't think we can rely on this being correct.

TextureSource objects have a format (GetFormat()) that canonically defines what format (and thus byte ordering) the data is in.

What should use this information (possibly by storing it in the Effect) to determine what swizzling the shader needs to do.
mattwoodrow, you're right. We have information from TextureSource. re-upload patch to use this info. Thanks!

try push: https://tbpl.mozilla.org/?tree=Try&rev=96863754f11a
Attachment #8379565 - Flags: review?(gal)
Attachment #8379473 - Attachment is obsolete: true
Attachment #8379473 - Flags: review?(gal)
Comment on attachment 8379565 [details] [diff] [review]
check_rb_swap_for_component_alpha

Fun bug :)
Attachment #8379565 - Flags: review?(gal) → review+
Comment on attachment 8379565 [details] [diff] [review]
check_rb_swap_for_component_alpha

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +828,4 @@
>      config.SetComponentAlpha(true);
> +    EffectComponentAlpha* effectComponentAlpha =
> +      static_cast<EffectComponentAlpha*>(aEffect);
> +    gfx::SurfaceFormat format = effectComponentAlpha->mOnWhite->GetFormat();

I am not quite familiar with component_alpha effect.
Logic here is use the format of mOnWhite to decide whether apply swizzling.

it swaps both RB channel of mOnWhite and mOnBlack in pixel shader. We assume that the format of mOnWhite and mOnBlack is the same. If this assumption is wrong(for example, mOnWhite is B8G8R8A8 and mOnBlack is R8G8B8A8), thing goes wrong.

I suggest add a assertion here.
MOZ_ASSERT(effectComponentAlpha->mOnWhite->GetFormat() == effectComponentAlpha->mOnBlack->GetFormat());
https://hg.mozilla.org/mozilla-central/rev/5fa7f5df67b3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Duplicate of this bug: 976425
You need to log in before you can comment on or make changes to this bug.