Closed Bug 1231643 Opened 8 years ago Closed 8 years ago

The composition result of these three mask-modes(subtract/ intersect/ exclude) is not correct on MacOS

Categories

(Core :: Layout, defect)

defect
Not set
normal

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);
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/
Attachment #8722495 - Flags: review?(mstange)
Attachment #8722567 - Flags: review?(dbaron)
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 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+
Attached file 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.
Blocks: mask-ship
I bet Lee would be interested in finding out why this fails with Skia.
(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?
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.
(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.
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.
Changed the patches according to suggestion in comment 10
Status: NEW → ASSIGNED
(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?
(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.
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)
Attachment #8722495 - Flags: review?(mstange) → review+
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
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
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)
https://hg.mozilla.org/mozilla-central/rev/04219460c57d
https://hg.mozilla.org/mozilla-central/rev/ec382036764c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.