Opacity in nsSVGUtils::PaintFrameWithEffects is applied twice.

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: cjku, Assigned: cjku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1305637
(Assignee)

Comment 1

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4875cb15d637
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8795261 - Flags: review?(mstange)
Attachment #8795262 - Flags: review?(mstange)

Comment 5

a year ago
mozreview-review
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
Attachment #8795261 - Flags: review?(mstange) → review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8795262 [details]
Bug 1305636 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/81376/#review80020
Attachment #8795262 - Flags: review?(mstange) → review+
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?
(Assignee)

Comment 8

a year ago
(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?
Flags: needinfo?(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).
Flags: needinfo?(mstange)
(Assignee)

Comment 10

a year ago
Created attachment 8795422 [details] [diff] [review]
Part 1. Only apply opacity once in nsSVGUtils::PaintFrameWithEffects and nsSVGIntegrationUtils::PaintMaskAndClipPath.
(Assignee)

Comment 11

a year ago
(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).
(Assignee)

Updated

a year ago
Attachment #8795422 - Flags: review?(mstange)
Attachment #8795422 - Flags: review?(mstange) → review+
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05cfc196ddb0
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c82bc7a452bb
(Assignee)

Comment 14

a year ago
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
(Assignee)

Updated

a year ago
Blocks: 1251161, 1224422
No longer blocks: 1305637

Comment 15

a year ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/b778574ce482
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64a3ced3732f
https://hg.mozilla.org/mozilla-central/rev/59b346e0119f
You need to log in before you can comment on or make changes to this bug.