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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

Here is try result of basic mask composition reftest.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=577792c6d8e7

These test failed only on Mac.

Comment 1

[u459114]

Using cario back-end can be a solution:   
nsSVGMaskFrame::GetMaskForMaskedFrame
  RefPtr<DrawTarget> destMaskDT =
      Factory::CreateDrawTarget(BackendType::CAIRO, maskSurfaceSize,
                                SurfaceFormat::A8);

Comment 2

[u459114]

Attachment: MozReview Request: Bug 1231643 - Part 1. Create skia-A8-surface for mask composition when backendtype of the source DrawTarget is CG; r=mstange

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

Comment 3

[u459114]

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 4

[u459114]

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/

Comment 5

[u459114]

Attachment: MozReview Request: Bug 1231643 - Part 2. Enable mask-composite reftest; r=dbaron

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

Comment 6

[Markus Stange [:mstange]]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[u459114]

Attachment: 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.

Comment 9

[Markus Stange [:mstange]]

I bet Lee would be interested in finding out why this fails with Skia.

Comment 10

[Lee Salzman [:lsalzman]]

(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

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 12

[u459114]

(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.

Comment 15

[u459114]

Changed the patches according to suggestion in comment 10

Comment 16

[Markus Stange [:mstange]]

(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?

Comment 17

[u459114]

(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

[Markus Stange [:mstange]]

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.

Comment 19

[Markus Stange [:mstange]]

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

Comment 20

[u459114]

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

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25682407&repo=mozilla-inbound

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f300b2f6a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b49f02205330

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04219460c57d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec382036764c

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04219460c57d
https://hg.mozilla.org/mozilla-central/rev/ec382036764c