Closed
Bug 1231643
Opened 9 years ago
Closed 9 years ago
The composition result of these three mask-modes(subtract/ intersect/ exclude) is not correct on MacOS
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: u459114, Assigned: u459114)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
Here is try result of basic mask composition reftest.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=577792c6d8e7
These test failed only on Mac.
Using cario back-end can be a solution:
nsSVGMaskFrame::GetMaskForMaskedFrame
RefPtr<DrawTarget> destMaskDT =
Factory::CreateDrawTarget(BackendType::CAIRO, maskSurfaceSize,
SurfaceFormat::A8);
Review commit: https://reviewboard.mozilla.org/r/36051/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36051/
https://reviewboard.mozilla.org/r/36051/#review32677
::: layout/svg/nsSVGIntegrationUtils.cpp:558
(Diff revision 1)
> + SurfaceFormat::A8);
CG surface has a bug while using A8 format. I think that is why nsSVGMaskFrame::GetMaskForMaskedFrame do not using CG backend.
Reference:
https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGMaskFrame.cpp#295
Comment on attachment 8722495 [details]
MozReview Request: Bug 1231643 - Part 1. Create skia-A8-surface for mask composition when backendtype of the source DrawTarget is CG; r=mstange
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36051/diff/1-2/
Review commit: https://reviewboard.mozilla.org/r/36095/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36095/
Attachment #8722495 -
Flags: review?(mstange)
Attachment #8722567 -
Flags: review?(dbaron)
Comment 6•9 years ago
|
||
What happens if you use the Skia backend instead?
Usually I'd say we should fix the backend to be correct, but in the case of CG we want to get rid of it soon and replace it with Skia, so it's not worth fixing the CG backend.
Comment 7•9 years ago
|
||
Comment on attachment 8722567 [details]
MozReview Request: Bug 1231643 - Part 2. Enable mask-composite reftest; r=dbaron
https://reviewboard.mozilla.org/r/36095/#review33057
I think the same reviewer as the main patch could have reviewed this, but sure.
Attachment #8722567 -
Flags: review?(dbaron) → review+
Switching to skia make test case fail.
The correct result should be RGB(0, 0, 255), using skia as backend, the result become RBG(1, 1, 255). I am looking into it.
Comment 9•9 years ago
|
||
I bet Lee would be interested in finding out why this fails with Skia.
Comment 10•9 years ago
|
||
(In reply to C.J. Ku[:cjku] from comment #8)
> Created attachment 8723382 [details]
> reftest log
>
> Switching to skia make test case fail.
> The correct result should be RGB(0, 0, 255), using skia as backend, the
> result become RBG(1, 1, 255). I am looking into it.
Skia blending results are generally off by 1, as Skia adds 1 to the alpha value when doing /256 math (since the alpha is on a scale of 0..255 initially).
This is a known "feature" of Skia. It's okay to add fuzz in the reftest manifests to compensate for this.
However, in so much as we ideally want to minimize dependencies on Cairo... If you are going to explicitly create a Cairo DT, or even a Skia DT, please put at least a comment here indicating that the problem is with the CG backend specifically, or at least check if the DT type is CG, and if not, still use the CreateSimilarDrawTarget path.
Or if we could easily just fix the CG backend to workaround this, that would really be better if it is not that much trouble?
Comment 11•9 years ago
|
||
Can you give some more detail in what the bug is in the CG backend? We use masking with A8 images elsewhere so it's not clear why this would fail.
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Can you give some more detail in what the bug is in the CG backend? We use
> masking with A8 images elsewhere so it's not clear why this would fail.
There is no problem for CG::A8 with source_over composition op, which is the default composition mode.
https://www.w3.org/TR/css-masking/#propdef-mask-composite
In mask-composite, we need three more composition modes. So I bring them in in bug 686281, and found the composition result on CG::A8 with CompositionOp::OP_OUT/ CompositionOp::OP_IN/ CompositionOp::OP_XOR is wrong.
Updated•9 years ago
|
Flags: needinfo?(jmuizelaar)
Attachment #8722495 -
Attachment description: MozReview Request: Bug 1231643 - Create Cairo A8 surface for mask composition. → MozReview Request: Bug 1231643 - Part 1. Create Skia A8 surface for mask composition when backendtype of the source DrawTarget is COREGRAPHIC.
Assignee | ||
Comment 15•9 years ago
|
||
Changed the patches according to suggestion in comment 10
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 16•9 years ago
|
||
(In reply to C.J. Ku[:cjku] from comment #12)
> and found the composition result on CG::A8 with
> CompositionOp::OP_OUT/ CompositionOp::OP_IN/ CompositionOp::OP_XOR is wrong.
Do you have an example with pixel values? E.g. could you give us an example source pixel and an example destination pixel and say what the correct result should be and what CG outputs?
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #16)
> (In reply to C.J. Ku[:cjku] from comment #12)
> > and found the composition result on CG::A8 with
> > CompositionOp::OP_OUT/ CompositionOp::OP_IN/ CompositionOp::OP_XOR is wrong.
>
> Do you have an example with pixel values? E.g. could you give us an example
> source pixel and an example destination pixel and say what the correct
> result should be and what CG outputs?
Since we are using A8 surface, RGB value is regardless. I list only alpha value here.
1. mask-composite: add = SOURCE_OVER
CG result is correct.
2. mask-composite: substract = SOURCE_OUT
formula: αo = αs x (1 – αb)
when source-alpha = 0, destination-alpha = 1,
Correct value: αo = 0 x (1 - 1) = 0
On CG: αo = 0 (correct)
When source-alpha = 1, destination-alpha = 0:
Correct value: αo = 1 x (1 - 0) = 1
On CG: αo = 0 (wrong)
3. mask-composite: intersect = SOURCE_IN
formula: αo = αs x αb
When source-alpha = 0, destination-alpha = 1,
Correct value: αo = 0 x 1 = 0
On CG: αo = 0 (correct)
When source-alpha = 1, destination-alpha = 0:
Correct value: αo = 1 x 0 = 0
On CG: αo = 1 (wrong)
4. mask-composite: exclude; #(XOR)
formula: αo = αs x (1 - αb) + αb x (1 – αs)
When source-alpha = 0, destination-alpha = 1:
Correct value: αo = 0 X (1 - 1) + 1 x (1 - 0) = 1
On CG: αo = 1 (correct)
When source-alpha = 1, destination-alpha = 0,
Correct value: αo = 1 x (1 - 0) + 0 x (1 - 1)= 1
On CG: αo = 0 (wrong)
Some finding
When source-alpha = 0, destination-alpha = 1, the result is always correct.
When source-alpha = 1, destination-alpha = 0, except add, the result is always wrong.
Comment 18•9 years ago
|
||
Sorry about the delay, I was confused about the status of this bug.
Jeff and I talked about how to fix DrawTargetCG today, and we didn't come a conclusion. It requires more thought. So we won't block this bug on solving that problem.
By the way, the reftest should already be passing on OS X these days because bug 1207332 has landed to make us use Skia. But we want this workaround anyway.
Flags: needinfo?(jmuizelaar)
Updated•9 years ago
|
Attachment #8722495 -
Flags: review?(mstange) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8722495 [details]
MozReview Request: Bug 1231643 - Part 1. Create skia-A8-surface for mask composition when backendtype of the source DrawTarget is CG; r=mstange
https://reviewboard.mozilla.org/r/36051/#review39885
Assignee | ||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd58bce52916ce3439fa3c35795a789d67e6eaeb
Bug 1231643 - Part 1. Create skia-A8-surface for mask composition when backendtype of the source DrawTarget is CG; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8227ba76e0477af7a9f42f9351ed65ef36ea3b
Bug 1231643 - Part 2. Enable mask-composite reftest; r=dbaron
Comment 21•9 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25682407&repo=mozilla-inbound
Flags: needinfo?(cku)
Comment 22•9 years ago
|
||
Comment 26•9 years ago
|
||
Comment 27•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/04219460c57d
https://hg.mozilla.org/mozilla-central/rev/ec382036764c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•