1297031 - TEST-UNEXPECTED-FAIL in mask-composite-2c.html while CSS masking is enabled

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Astley Chen (inactive)]

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a730d5de4528&selectedJob=26088958

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/masking/mask-composite-2c.html == file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/masking/mask-composite-2-ref.html | image comparison, max difference: 255, number of differing pixels: 10000

Regressed by:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91667ea2a01e

Comment 1

[Ethan Lin[:ethlin]]

Attachment: force use group for Alpha type

Skia's drawBitmap() uses different way(drawBitmapAsMask) to handle kAlpha_8_SkColorType and that causes wrong blending result. So I force the 'AutoPaintSetup' use group method while the color type is kAlpha_8_SkColorType.

Comment 2

[Lee Salzman [:lsalzman]]

Comment on attachment 8784307 [details] [diff] [review]
force use group for Alpha type

Review of attachment 8784307 [details] [diff] [review]:
-----------------------------------------------------------------

The basic idea of this patch is fine given the constraints that xfermodes with A8 bitmaps just don't work at all in Skia.
Mainly I just have some nitpicking that can be easily fixed before we land this:

This check: bitmap.colorType() == kAlpha_8_SkColorType
That currently applies to all xfermodes, but the default interpretation of mask blitting in Skia is OP_OVER. So in the very common case of OP_OVER, we don't need to force a group, as Skia will do the right thing.
So this check would be better: bitmap.colorType() == kAlpha_8_SkColorType && aOptions.mCompositionOp != CompositionOp::OP_OVER
That way we don't pay a speed hit for the common case unnecessarily.

Other nitpick: forceNeedGroup/aForceNeedGroup is awkward wording. I would prefer the simpler forceGroup/aForceGroup.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: force use group for Alpha

Thanks for the review. I addressed the above comments to this patch.

Comment 4

[Ethan Lin[:ethlin]]

Attachment: force use group for Alpha (carry r+: lsalzman)

Change to r=lsalzman.

Comment 5

[Ethan Lin[:ethlin]]

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee91a2a2892&selectedJob=26329392

Comment 6

[Ethan Lin[:ethlin]]

Attachment: force use group for Alpha (carry r+: lsalzman)

Rebase to inbound.

Comment 7

[u459114]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e78454e242072bbcada626874f2900bbe1c3e647
Bug 1297031 - Force to use group in DrawTargetSkia::DrawSurface when colorType is kAlpha_8_SkColorType. r=lsalzman

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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