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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[John Schoenick [:johns]]

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

Comment 1

[John Schoenick [:johns]]

A site I've found to reliably reproduce is:
https://github.com/mozilla/

The project links show up orange, instead of blue.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: check_GetPreferredARGB32Format

Need to check GetPreferredARGB32Format in this condition for correct result.

try push: https://tbpl.mozilla.org/?tree=Try&rev=72a5076ed23e

Comment 3

[Matt Woodrow (:mattwoodrow)]

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.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: check_rb_swap_for_component_alpha

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

Comment 5

[Andreas Gal :gal]

Comment on attachment 8379565 [details] [diff] [review]
check_rb_swap_for_component_alpha

Fun bug :)

Comment 6

[u459114]

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());

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa7f5df67b3

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5fa7f5df67b3