Closed Bug 1332268 Opened 3 years ago Closed 3 years ago

Update EffectComponentAlpha texture coordinates when rendering split layers

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: miko, Assigned: miko)

References

Details

Attachments

(1 file)

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 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.
Attachment #8828329 - Flags: review?(matt.woodrow) → review+
(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.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d3e3c0a23c6
Update EffectComponentAlpha texture coordinates when rendering split layers r=mattwoodrow
Keywords: checkin-needed
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
Flags: needinfo?(mikokm)
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
Flags: needinfo?(mikokm)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d6389cb311c1
Update EffectComponentAlpha texture coordinates when rendering split layers r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d6389cb311c1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.