1332268 - Update EffectComponentAlpha texture coordinates when rendering split layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Miko Mynttinen]

When layers with alpha component are transformed and split, the texture coordinates might be incorrect.
This patch will correct the texture coordinates similarly to the other TexturedEffect based types.

Comment 1

[Miko Mynttinen]

Attachment: Bug 1332268 - Update EffectComponentAlpha texture coordinates when rendering split layers

Review commit: https://reviewboard.mozilla.org/r/105784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105784/

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8828329 [details]
Bug 1332268 - Update EffectComponentAlpha texture coordinates when rendering split layers

https://reviewboard.mozilla.org/r/105784/#review106826

::: gfx/layers/Compositor.cpp:341
(Diff revision 1)
>      // update the texture coordinates to account for the split.
>      const EffectTypes type = aEffectChain.mPrimaryEffect->mType;
>  
>      if (type == EffectTypes::RGB || type == EffectTypes::YCBCR ||
> -        type == EffectTypes::NV12 || type == EffectTypes::RENDER_TARGET) {
> +        type == EffectTypes::NV12 || type == EffectTypes::RENDER_TARGET ||
> +        type == EffectTypes::COMPONENT_ALPHA) {

Can you please instead add 'virtual TexturedEffect* AsTexturedEffect()' on Effect?

That way we can do an effective dynamic cast rather than checking a big list of types and static_casting.

You could also assert that type == EffectTypes::SOLID_COLOR if we *don't* take this branch, so that it'll start failing if anyone adds a new EffectType and doesn't fix this code.

Comment 3

[Miko Mynttinen]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Comment on attachment 8828329 [details]
> Bug 1332268 - Update EffectComponentAlpha texture coordinates when rendering
> split layers
> 
> https://reviewboard.mozilla.org/r/105784/#review106826
> 
> ::: gfx/layers/Compositor.cpp:341
> (Diff revision 1)
> >      // update the texture coordinates to account for the split.
> >      const EffectTypes type = aEffectChain.mPrimaryEffect->mType;
> >  
> >      if (type == EffectTypes::RGB || type == EffectTypes::YCBCR ||
> > -        type == EffectTypes::NV12 || type == EffectTypes::RENDER_TARGET) {
> > +        type == EffectTypes::NV12 || type == EffectTypes::RENDER_TARGET ||
> > +        type == EffectTypes::COMPONENT_ALPHA) {
> 
> Can you please instead add 'virtual TexturedEffect* AsTexturedEffect()' on
> Effect?
> 
> That way we can do an effective dynamic cast rather than checking a big list
> of types and static_casting.

Thank you for the review. I've made these changes.

Comment 4

[Miko Mynttinen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b45c2f7790af5eefae25b44f3e77dd7c0472ec6

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d3e3c0a23c6
Update EffectComponentAlpha texture coordinates when rendering split layers r=mattwoodrow

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing own test on Windows 7 VM:

https://hg.mozilla.org/integration/autoland/rev/ec7143bb25f0a4fab2b783a833da6b763ffb3e10

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7d3e3c0a23c64491856e905c5a43550895f0dfe7
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=70686907&repo=autoland

REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/transform-3d/component-alpha-1.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/transform-3d/component-alpha-1-ref.html | image comparison, max difference: 120, number of differing pixels: 111

Comment 7

[Miko Mynttinen]

Comment on attachment 8828329 [details]
Bug 1332268 - Update EffectComponentAlpha texture coordinates when rendering split layers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105784/diff/1-2/

Comment 8

[Miko Mynttinen]

Comment on attachment 8828329 [details]
Bug 1332268 - Update EffectComponentAlpha texture coordinates when rendering split layers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105784/diff/2-3/

Comment 9

[Miko Mynttinen]

The failing reftest was supposed to be fuzzy. This is now fixed along with the EffectType check.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5617bca9fd981f64b5f105cca52fac08a2dfadb

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6389cb311c1
Update EffectComponentAlpha texture coordinates when rendering split layers r=mattwoodrow

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d6389cb311c1