1280707 - Create more reftest test cases with large mask size

Status: NEW

(Core :: Layout, defect, P3)

Description

[Astley Chen (inactive)]

Found these 2 test failures on linux platform only whenever we enable css masking in bug 1251161. Need to address the root cause and solve them before we ship masking feature.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c54d173eb87d

Comment 1

[Ethan Lin[:ethlin]]

Attachment: change image size to 20x20

Reftest will test the document displays once it has loaded[1]. For images, we use another thread to decode, so images may not display correctly in the first transaction. It should be the reason to cause this intermittent problem. 
We can use 'MozReftestInvalidate' and 'reftest-wait' to solve the problem but that's for Firefox only, not suitable for w3c reftest. 
After referencing other reftests[2], I think many reftests use small images to ensure the decode completion when snapshot. So I simply downscale the image size from 100x100 to 20x20. The try server result[3] looks good after downscaling.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Creating_reftest-based_unit_tests
[2] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/background-tiling-zoom-1.html?from=background-tiling-zoom-1.html
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3e27f80ebee&selectedJob=22645546

Comment 2

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

(In reply to Ethan Lin[:ethlin] from comment #1)
> Reftest will test the document displays once it has loaded[1]. For images,
> we use another thread to decode, so images may not display correctly in the
> first transaction. It should be the reason to cause this intermittent
> problem. 
> We can use 'MozReftestInvalidate' and 'reftest-wait' to solve the problem
> but that's for Firefox only, not suitable for w3c reftest. 
> After referencing other reftests[2], I think many reftests use small images
> to ensure the decode completion when snapshot. So I simply downscale the
> image size from 100x100 to 20x20. The try server result[3] looks good after
> downscaling.

The reftest harness should be waiting for images to be decoded before taking a snapshot.  Did you investigate why this isn't happening?  Might there be an actual code bug?

The failures on Linux appear to have happened only on the tests mask-mode-b.html and mask-image-1a.html.

(The failures on Mac OS X appear to be mask-composite-{2a,2b}.html and mask-image-{3c,3d}.html.  Were those covered by a different bug?)

Comment 3

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

(Is it possible, for example, that the equal-except-ref code in TryToStartImageLoadOnValue is triggering when it shouldn't, or that there's some interaction with the StyleImageLoader that's missing?)

Comment 4

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

Another question, maybe more relevant:  does AddAndRemoveImageAssociations not work in some cases, such that we actually depend on the AssociateImage calls in nsCSSRendering that are present for background-image and border-image, but not for mask-image?

Comment 5

[u459114]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #2)
> (The failures on Mac OS X appear to be mask-composite-{2a,2b}.html and
> mask-image-{3c,3d}.html.  Were those covered by a different bug?)
fuzzy-if is needed for these test cases. Please see bug 1231643 comment 10. In short, Skia blending results are generally off by 1, and we use skia A8 surface as mask composition target.

I saw Astley already did it(add fuzzy-if) in bug 1276834's patch.

Comment 6

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #4)
> Another question, maybe more relevant:  does AddAndRemoveImageAssociations
> not work in some cases, such that we actually depend on the AssociateImage
> calls in nsCSSRendering that are present for background-image and
> border-image, but not for mask-image?

After more tests, I found the reftest of background image waits for images to be decoded before taking a snapshot, but the mask-image reftest doesn't on try server. It may be related to AddAndRemoveImageAssociations though I didn't find anything wrong in the function. 
Another problem is I can't reproduce the problem locally. My local reftest of mask-image always waits for decoding even I marked AddAndRemoveImageAssociations or used large png. I will keep studying this issue.

Comment 10

[u459114]

I think it's because of not handling the returning value of nsCSSRendering::PaintBackgroundWithSC at [1].
I am going to fix bug 1258510 first

[1] https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#578

Comment 11

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

(In reply to Ethan Lin[:ethlin] from comment #6)
> After more tests, I found the reftest of background image waits for images
> to be decoded before taking a snapshot, but the mask-image reftest doesn't
> on try server. It may be related to AddAndRemoveImageAssociations though I
> didn't find anything wrong in the function. 
> Another problem is I can't reproduce the problem locally. My local reftest
> of mask-image always waits for decoding even I marked
> AddAndRemoveImageAssociations or used large png. I will keep studying this
> issue.

There are a number of places in nsDisplayList.cpp and nsDisplayListInvalidation.h where we use the result of ShouldSyncDecodeImages or ShouldInvalidateToSyncDecodeImages.  But I don't think any of that code deals with mask-image.  Should there be equivalent code for mask-image?

Comment 12

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

Maybe also worth looking at http://searchfox.org/mozilla-central/search?q=SYNC_DECODE_IMAGES&path=

Comment 13

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #11)
> (In reply to Ethan Lin[:ethlin] from comment #6)
> > After more tests, I found the reftest of background image waits for images
> > to be decoded before taking a snapshot, but the mask-image reftest doesn't
> > on try server. It may be related to AddAndRemoveImageAssociations though I
> > didn't find anything wrong in the function. 
> > Another problem is I can't reproduce the problem locally. My local reftest
> > of mask-image always waits for decoding even I marked
> > AddAndRemoveImageAssociations or used large png. I will keep studying this
> > issue.
> 
> There are a number of places in nsDisplayList.cpp and
> nsDisplayListInvalidation.h where we use the result of
> ShouldSyncDecodeImages or ShouldInvalidateToSyncDecodeImages.  But I don't
> think any of that code deals with mask-image.  Should there be equivalent
> code for mask-image?

Right, after discussion with CJ, we should handle DrawResult in nsDisplaySVGEffects::PaintAsLayer and update InvalidRegion in nsDisplaySVGEffects::GetLayerState. CJ will fix in bug 1258510.

Comment 14

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

Comment on attachment 8765312 [details] [diff] [review]
change image size to 20x20

I think this is obscuring a real bug.  We don't have this problem with background-image.  So I think we should fix the real bug instead of doing this.

Comment 15

[u459114]

The patches in bug 1258510 can fix random failure here. I would suggest create a test case with big mask here.

Comment 16

[Astley Chen (inactive)]

Per comment 15, rename bug title and set as follow-up of meta bug.