1305636 - Opacity in nsSVGUtils::PaintFrameWithEffects is applied twice.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

Create this bug to trace fixing of bug 1304991 comment 10.

In nsSVGUtils::PaintFrameWithEffects, aOpacity is applied twice, once in nsSVGMaskFrame::GetMaskForMaskedFrame, and onece in gfxContext::PushGroupForBlendBack.

Fix this problem here and create some test case at the same time.

Comment 1

[u459114]

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

Comment 2

[u459114]

Attachment: Bug 1305636 - Part 1. In nsSVGUtils::PaintFrameWithEffects, do not pass opacity to GetMaskForMaskedFrame.

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

Comment 3

[u459114]

Attachment: Bug 1305636 - Part 2. Reftest.

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

Comment 4

[u459114]

Comment on attachment 8795262 [details]
Bug 1305636 - Part 2. Reftest.

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

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8795261 [details]
Bug 1305636 - Part 1. In nsSVGUtils::PaintFrameWithEffects, do not pass opacity to GetMaskForMaskedFrame.

https://reviewboard.mozilla.org/r/81374/#review80016

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8795262 [details]
Bug 1305636 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/81376/#review80020

Comment 7

[Markus Stange [:mstange]]

Hmm, I'm somewhat regretting my r+s here and in bug 1304991 now...
The reason we applied the opacity to the mask surface was that it made things faster. Since bug 1210560, we were applying opacity both the fast way and the slow way. And now we're removing the fast way. Shouldn't we instead try to remove the slow way instead? Or is the fast way not actually that much faster than the slow way?

Comment 8

[u459114]

(In reply to Markus Stange [:mstange] from comment #7)
> Hmm, I'm somewhat regretting my r+s here and in bug 1304991 now...
> The reason we applied the opacity to the mask surface was that it made
> things faster. Since bug 1210560, we were applying opacity both the fast way
> and the slow way. And now we're removing the fast way. Shouldn't we instead
> try to remove the slow way instead? Or is the fast way not actually that
> much faster than the slow way?
OK, change should be minor.
May I know why you think apply opacity in mask surface is faster? Is that because the size of mask surface is smaller?

Comment 9

[Markus Stange [:mstange]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #8)
> May I know why you think apply opacity in mask surface is faster? Is that
> because the size of mask surface is smaller?

It probably depends on the Moz2D backend. It would be nice to measure the difference. The ideas is that we have optimized paths for applying the opacity during the RGB->luminance conversion (so we only need to look at the data once), but we might not have optimized paths for applying the opacity at the same time as applying the mask (so we may need to loop over the pixels twice).

Comment 10

[u459114]

Attachment: Part 1. Only apply opacity once in nsSVGUtils::PaintFrameWithEffects and nsSVGIntegrationUtils::PaintMaskAndClipPath.

Comment 11

[u459114]

(In reply to Markus Stange [:mstange] from comment #9)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #8)
> > May I know why you think apply opacity in mask surface is faster? Is that
> > because the size of mask surface is smaller?
> 
> It probably depends on the Moz2D backend. It would be nice to measure the
> difference. The ideas is that we have optimized paths for applying the
> opacity during the RGB->luminance conversion (so we only need to look at the
Hmm.. such as ComputesRGBLuminanceMask_NEON I see.
> data once), but we might not have optimized paths for applying the opacity
> at the same time as applying the mask (so we may need to loop over the
> pixels twice).

Comment 12

[u459114]

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

Comment 13

[u459114]

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

Comment 14

[u459114]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b778574ce482489c9502e877bfff1bf5770ec3bd
Bug 1305636 - Part 1. Revert the first patch in bug 1304991. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/64a3ced3732f64d86a289e4d57f8ab5dbf045e5c
Bug 1305636 - Part 2. Only apply opacity once in nsSVGUtils::PaintFrameWithEffects and nsSVGIntegrationUtils::PaintMaskAndClipPath. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/59b346e0119faa8afad86bc31188cabffd68a2e0
Bug 1305636 - Part 3. Reftest. r=mstange

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64a3ced3732f
https://hg.mozilla.org/mozilla-central/rev/59b346e0119f