Enable 'layers.advanced.background-color' by default

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Depends on: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

8 months ago
I suppose some failures will be similar with bug 1354463 ('layers.advanced.canvas-background-color'). But we still should have a bug for background-color.
The current try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b21095b276c3cb9a2b06af2dbe038c2cd669c3&selectedJob=92249781
Depends on: 1358066
Depends on: 1358961
(Assignee)

Updated

8 months ago
Depends on: 1359314
(Assignee)

Comment 1

8 months ago
Created attachment 8862297 [details] [diff] [review]
fuzzy_wip
(Assignee)

Comment 2

8 months ago
I add fuzz to ignore some failures which is caused by layer tree change. The current try result is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=94701423.

The remaining failures aren't too many. Looks like there are about 4~5 issues. I can create bugs for these issues first.
Comment on attachment 8862297 [details] [diff] [review]
fuzzy_wip

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

::: layout/reftests/reftest-sanity/reftest.list
@@ +155,5 @@
>  # When using 565 fuzzy.html and fuzzy-ref.html will compare as equal
>  fails fuzzy-if(false,2,1) random-if(Android) == fuzzy.html fuzzy-ref.html
>  
>  # Test that reftest-no-paint fails correctly
> +fails random-if(webrender) == reftest-no-paint.html reftest-no-paint-ref.html

FYI the "reftest-sanity" suite is mostly intended to make sure that the reftest harness itself is working properly. Marking a test here as random-if is undesirable because it means that we don't know if the harness itself is working properly or not. We should look at this test and see if it makes sense to even run with webrender. If it does, we should make sure it passes; if it doesn't, then we can just skip-if(webrender) it with an explanation.
(Assignee)

Updated

8 months ago
Depends on: 1361662
(Assignee)

Updated

8 months ago
Depends on: 1361668
(Assignee)

Updated

7 months ago
Depends on: 1151016
(Assignee)

Updated

7 months ago
Depends on: 1365511
mask-gradient-translucent-end-color-1.html failure would be fixed by bug 1350663.
Depends on: 1350663
(Assignee)

Updated

7 months ago
Depends on: 1366692
(Assignee)

Comment 5

7 months ago
Created attachment 8869959 [details] [diff] [review]
Part1. Add reftest flags for background color layer to make try passed

Almost all failures are fixed or identified. I think we can enable the background color layer by default. I added flags based on the try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=94701423.
Attachment #8869959 - Flags: review?(bugmail)
(Assignee)

Comment 6

7 months ago
Created attachment 8869960 [details] [diff] [review]
Part2. Enable "layers.advanced.background-color" by default
Attachment #8862297 - Attachment is obsolete: true
Attachment #8869960 - Flags: review?(bugmail)
Attachment #8869959 - Flags: review?(bugmail) → review+
Comment on attachment 8869960 [details] [diff] [review]
Part2. Enable "layers.advanced.background-color" by default

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

Please clean up commit messages to not have the file delta info
Attachment #8869960 - Flags: review?(bugmail) → review+
(In reply to Ethan Lin[:ethlin] from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=9
> 4701423.

Just noticed, this try push is a month old. Please verify with a recent try push that nothing has changed.
Comment on attachment 8869959 [details] [diff] [review]
Part1. Add reftest flags for background color layer to make try passed

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

::: layout/reftests/reftest-sanity/reftest.list
@@ +155,5 @@
>  # When using 565 fuzzy.html and fuzzy-ref.html will compare as equal
>  fails-if(!stylo) fuzzy-if(false,2,1) random-if(Android) == fuzzy.html fuzzy-ref.html
>  
> +# Test that reftest-no-paint fails correctly. We always create color layers with WR enabled and color layers will paint after the color changed.
> +fails skip-if(webrender) == reftest-no-paint.html reftest-no-paint-ref.html

The comment you added here seems a bit wrong. The reftest is expected to fail, because the test does a repaint. Presumably with webrender enabled we don't do a repaint because we generate the color layer instead and just update the color on that. So saying "color layers will paint after the color changed" is misleading.

::: layout/reftests/writing-mode/reftest.list
@@ +146,5 @@
>  # tests involving sideways-lr mode
>  == 1193519-sideways-lr-1.html 1193519-sideways-lr-1-ref.html
>  == 1193519-sideways-lr-2.html 1193519-sideways-lr-2-ref.html
>  fuzzy-if(winWidget,3,84) == 1193519-sideways-lr-3.html 1193519-sideways-lr-3-ref.html
> +fails-if(webrender) fuzzy-if(winWidget,3,112) == 1193519-sideways-lr-4.html 1193519-sideways-lr-4-ref.html # see bug 1366692. Rounding error with WR enabled.

Also here - I think you need the fails-if(webrender) to be after the fuzzy-if(winWidget). Otherwise on windows with webrender enabled it will expect a fuzzy pass instead of a failure. (The last matched condition overrides previous ones)
(Assignee)

Comment 10

7 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> (In reply to Ethan Lin[:ethlin] from comment #5)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=9
> > 4701423.
> 
> Just noticed, this try push is a month old. Please verify with a recent try
> push that nothing has changed.

Okay, I sent another try to check.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ba9958a495d481211ddd6fb524a005bec8aaea&selectedJob=101427063
(Assignee)

Comment 11

7 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Comment on attachment 8869959 [details] [diff] [review]
> Part1. Add reftest flags for background color layer to make try passed
> 
> Review of attachment 8869959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/reftest-sanity/reftest.list
> @@ +155,5 @@
> >  # When using 565 fuzzy.html and fuzzy-ref.html will compare as equal
> >  fails-if(!stylo) fuzzy-if(false,2,1) random-if(Android) == fuzzy.html fuzzy-ref.html
> >  
> > +# Test that reftest-no-paint fails correctly. We always create color layers with WR enabled and color layers will paint after the color changed.
> > +fails skip-if(webrender) == reftest-no-paint.html reftest-no-paint-ref.html
> 
> The comment you added here seems a bit wrong. The reftest is expected to
> fail, because the test does a repaint. Presumably with webrender enabled we
> don't do a repaint because we generate the color layer instead and just
> update the color on that. So saying "color layers will paint after the color
> changed" is misleading.
> 
To my understanding, the test changes the background to black to match the reference test file. However, the test expects that the color change will not cause invalidation region change since the painted layer doesn't check the color value. So originally it will not repaint and it will fail. For WR, we use color layer so it will repaint after the color changed.

> ::: layout/reftests/writing-mode/reftest.list
> @@ +146,5 @@
> >  # tests involving sideways-lr mode
> >  == 1193519-sideways-lr-1.html 1193519-sideways-lr-1-ref.html
> >  == 1193519-sideways-lr-2.html 1193519-sideways-lr-2-ref.html
> >  fuzzy-if(winWidget,3,84) == 1193519-sideways-lr-3.html 1193519-sideways-lr-3-ref.html
> > +fails-if(webrender) fuzzy-if(winWidget,3,112) == 1193519-sideways-lr-4.html 1193519-sideways-lr-4-ref.html # see bug 1366692. Rounding error with WR enabled.
> 
> Also here - I think you need the fails-if(webrender) to be after the
> fuzzy-if(winWidget). Otherwise on windows with webrender enabled it will
> expect a fuzzy pass instead of a failure. (The last matched condition
> overrides previous ones)

Okay, I will change the flag order.
(Assignee)

Comment 12

7 months ago
According to the try result[1] with the patches, there are some new failures[2] about mask. I'll investigate these failures.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=be7d359a395a02bf7ee3274220cdf6fa843a342f&selectedJob=101485157
[2] layout/reftests/invalidation/mask-invalidation-2a.html, layout/reftests/invalidation/mask-invalidation-2b.html, layout/reftests/w3c-css/submitted/masking/mask-image-1a.html
(In reply to Ethan Lin[:ethlin] from comment #11)
> To my understanding, the test changes the background to black to match the
> reference test file. However, the test expects that the color change will
> not cause invalidation region change since the painted layer doesn't check
> the color value.

But the test expects that the color change *will* cause invalidation. That's why the reftest.list has a "fails" annotation. That is, the test is expected to fail the "no-paint" check, which means it's expected to do a repaint. If with WR the test is producing an UNEXPECTED-PASS, that must be because with WR we're not doing a repaint, and so the "no-paint" check is passing. Or am I missing something?
(Assignee)

Comment 14

7 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> (In reply to Ethan Lin[:ethlin] from comment #11)
> > To my understanding, the test changes the background to black to match the
> > reference test file. However, the test expects that the color change will
> > not cause invalidation region change since the painted layer doesn't check
> > the color value.
> 
> But the test expects that the color change *will* cause invalidation. That's
> why the reftest.list has a "fails" annotation. That is, the test is expected
> to fail the "no-paint" check, which means it's expected to do a repaint. If
> with WR the test is producing an UNEXPECTED-PASS, that must be because with
> WR we're not doing a repaint, and so the "no-paint" check is passing. Or am
> I missing something?

Oh..I think I misunderstood the meaning of the test. I just studied the flag "reftest-no-paint" again. You are right. I should change the comment for the test.
(Assignee)

Comment 15

7 months ago
(In reply to Ethan Lin[:ethlin] from comment #12)
> According to the try result[1] with the patches, there are some new
> failures[2] about mask. I'll investigate these failures.
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=be7d359a395a02bf7ee3274220cdf6fa843a342f&selectedJob=1
> 01485157
> [2] layout/reftests/invalidation/mask-invalidation-2a.html,
> layout/reftests/invalidation/mask-invalidation-2b.html,
> layout/reftests/w3c-css/submitted/masking/mask-image-1a.html

These new failures are due to the patch in bug 1350663. The patch will fix "mask-gradient-translucent-end-color-1.html". I would like to mark the reftest as fails-if before it's landed.
(Assignee)

Updated

7 months ago
Depends on: 1367991
(Assignee)

Updated

7 months ago
Blocks: 1367994
(Assignee)

Updated

7 months ago
No longer blocks: 1367994
Depends on: 1367994
(Assignee)

Comment 16

7 months ago
I got two more reftest failures based on the try result[1]. One is "dom/animation/test/crashtests/1272475-1.html" and other one is 
"layout/reftests/w3c-css/submitted/masking/mask-image-1-ref.html".
The 1272475-1.html is about NAN handling problem in rust and Morris is working on it. I created a bug 1367994 to track this issue and mark it as fails-if for now. The mask-image-1-ref.html is about mask layer invalidation and I will fix it in bug 1367991.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9402f210d1a1fc3f227669dca27c69cdf7cbe0f9&selectedJob=102209235
(Assignee)

Comment 17

7 months ago
Created attachment 8871657 [details] [diff] [review]
Part1. Add reftest flags for background color layer to make try passed

I adjusted some flags based on the new try result. Kats, could you have a review again? Thanks.
[1] is the try with the patches, including the fix in bug 1367991.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=160dbda62cafe7c98b50f06a2851fdbda7e4a876
Attachment #8869959 - Attachment is obsolete: true
Attachment #8871657 - Flags: review?(bugmail)
Comment on attachment 8871657 [details] [diff] [review]
Part1. Add reftest flags for background color layer to make try passed

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

::: dom/animation/test/crashtests/crashtests.list
@@ +6,5 @@
>  pref(dom.animations-api.core.enabled,true) load 1216842-4.html
>  pref(dom.animations-api.core.enabled,true) load 1216842-5.html
>  pref(dom.animations-api.core.enabled,true) load 1216842-6.html
> +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-1.html # see bug 1367994
> +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-2.html # see bug 1367994

According to your try push in comment 16 only 1272475-1 is failing, the second one seems to run ok. Can we leave 1272475-2 without any annotation?
Attachment #8871657 - Flags: review?(bugmail) → review+
(Assignee)

Comment 19

7 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Comment on attachment 8871657 [details] [diff] [review]
> Part1. Add reftest flags for background color layer to make try passed
> 
> Review of attachment 8871657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/test/crashtests/crashtests.list
> @@ +6,5 @@
> >  pref(dom.animations-api.core.enabled,true) load 1216842-4.html
> >  pref(dom.animations-api.core.enabled,true) load 1216842-5.html
> >  pref(dom.animations-api.core.enabled,true) load 1216842-6.html
> > +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-1.html # see bug 1367994
> > +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-2.html # see bug 1367994
> 
> According to your try push in comment 16 only 1272475-1 is failing, the
> second one seems to run ok. Can we leave 1272475-2 without any annotation?

Okay, I will remove it.
(Assignee)

Comment 20

7 months ago
There is two new reftest failures[1] object-position-webm-001.html and object-position-webm-002.html in R8. It looks like we should set 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm tests. But there is a 'fails' for these two tests. Per discussion with Jerry, the annotation for webm tests will become fails-if(webrender) with his patches. So for now I would like skip these two tests.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=92aae8ae3c741ee29ec090157cfa0570f957fc71&selectedJob=103952160
[2] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/webm-video/reftest.list#44
(Assignee)

Comment 21

7 months ago
Created attachment 8873780 [details] [diff] [review]
skip object-position-webm reftests
Attachment #8873780 - Flags: review?(howareyou322)

Updated

7 months ago
Attachment #8873780 - Flags: review?(howareyou322) → review+
(Assignee)

Updated

7 months ago
See Also: → bug 1369678
(Assignee)

Comment 22

7 months ago
try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec92a6b7d00799f2b4ccd77154ede404d54a0f9c

Comment 23

7 months ago
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb164b643bdb
Add reftest flags for background color layer to make try passed. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c514102ec3
Enable background color layer by default for webrender. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f37512b62a
Add webm reftest flags for webrender. r=pchang
(In reply to Ethan Lin[:ethlin] from comment #20)
> There is two new reftest failures[1] object-position-webm-001.html and
> object-position-webm-002.html in R8. It looks like we should set
> 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm
> tests. But there is a 'fails' for these two tests. Per discussion with
> Jerry, the annotation for webm tests will become fails-if(webrender) with
> his patches. So for now I would like skip these two tests.

In general I'm not a fan of skip-if when fails or fails-if will do. With skip-if the test is skipped entirely, so if the test exposes a crash regression we will miss it. If it fails randomly, then even random-if is better than skip-if.

Comment 25

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb164b643bdb
https://hg.mozilla.org/mozilla-central/rev/57c514102ec3
https://hg.mozilla.org/mozilla-central/rev/21f37512b62a
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 26

6 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> (In reply to Ethan Lin[:ethlin] from comment #20)
> > There is two new reftest failures[1] object-position-webm-001.html and
> > object-position-webm-002.html in R8. It looks like we should set
> > 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm
> > tests. But there is a 'fails' for these two tests. Per discussion with
> > Jerry, the annotation for webm tests will become fails-if(webrender) with
> > his patches. So for now I would like skip these two tests.
> 
> In general I'm not a fan of skip-if when fails or fails-if will do. With
> skip-if the test is skipped entirely, so if the test exposes a crash
> regression we will miss it. If it fails randomly, then even random-if is
> better than skip-if.

The reftest expects always failed and my patches make it unexpected-passed on e10s, so I have to skip it for now. I created another bug 1369678 to tracking this issue. According to the try push[1] in bug 1366502, we can remove the skip-if after the patches in bug 1366502 are landed.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a699c3a79b6f0695df8202ed111f023afa4280
(Assignee)

Comment 27

6 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> (In reply to Ethan Lin[:ethlin] from comment #20)
> > There is two new reftest failures[1] object-position-webm-001.html and
> > object-position-webm-002.html in R8. It looks like we should set
> > 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm
> > tests. But there is a 'fails' for these two tests. Per discussion with
> > Jerry, the annotation for webm tests will become fails-if(webrender) with
> > his patches. So for now I would like skip these two tests.
> 
> In general I'm not a fan of skip-if when fails or fails-if will do. With
> skip-if the test is skipped entirely, so if the test exposes a crash
> regression we will miss it. If it fails randomly, then even random-if is
> better than skip-if.

Right, random-if should be better. I will use it next time. Thanks.
(Assignee)

Updated

6 months ago
Blocks: 1373530
You need to log in before you can comment on or make changes to this bug.