Closed Bug 1336986 Opened 3 years ago Closed 3 years ago

Turn on layers.advanced.bullet-layers and fix tests fail.

Categories

(Core :: Graphics: WebRender, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(3 files)

In Bug 1328494, we had basic support for nsDisplayBullet. I'm going to turn on the pref by default and fix the tests fail.
We need bug 1332688 to get transform works.
Depends on: 1332688
Text layer is not fully supported yet. We need wait it as well.
Depends on: 1339683
Depends on: 1362324
Duplicate of this bug: 1362315
Current status.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f165384a0f4b981dab5671587cb5a4c342d2b91&group_state=expanded&selectedJob=98245768

Looks like try is good after the webrender blob image assertion fix. I'll wait for next webrender update and send a new try again.
Assignee: nobody → mtseng
Comment on attachment 8866653 [details]
Bug 1336986 - Enable layers.advanced.bullet-layers if webrender is enabled.

https://reviewboard.mozilla.org/r/138258/#review141646

::: commit-message-36102:1
(Diff revision 1)
> +Bug 1336986 - Enable layers.advanced.bullet-layers by default. r=kats

s/by default/if webrender is enabled/
Attachment #8866653 - Flags: review?(bugmail) → review+
Comment on attachment 8866653 [details]
Bug 1336986 - Enable layers.advanced.bullet-layers if webrender is enabled.

https://reviewboard.mozilla.org/r/138258/#review141648

Oh, please add pref("layers.advanced.bullet-layers", 2); to modules/libpref/init/all.js as well. There's a bunch of similar ones at the bottom of the file.
Comment on attachment 8866654 [details]
Bug 1336986 - Mark fails-if for some tests.

255 is a very dangerous fuzzy enough... :/. Do you know why this is happening? From the try push in comment 6, I only see fuzz needed for /layout/reftests/svg/outline.html. Why so much other fuzz here?
Flags: needinfo?(mtseng)
Attachment #8866654 - Flags: review?(mchang)
Note that the outline.html failure is from the skia m59 change. I had landed a patch on inbound to fuzz that as part of the skia bug, and then the whole thing got backed out twice and relanded. Anyway, ignore that one since it's got nothing to do with this bug. The try push in comment 6 includes the fuzzy-if statements already, which is why it doesn't show any other failures.
The try in comment 6 is already applied fuzzy-if. You can see the try in comment 5.
Flags: needinfo?(mtseng) → needinfo?(mchang)
Do you know why whole lines seem to be shifted versus their reference reftests? Can we document that somewhere?
Flags: needinfo?(mchang) → needinfo?(mtseng)
We shouldn't add fuzzy 255 annotations. Those tests should be marked as failing, e.g. fails-if(webrender).
It looks very much like the test images get antialiasing and the reference don't.
Yap, as kats said. One images get aa-ing and the others doesn't. I did some experiments.

1. Turn on text-layer by default. Thus, both test cases use aa for text. And the reftest is pass.
2. Turn off aa in webrender. The reftests are pass as well.

I agree fuzzy 255 isn't acceptable. Should we set fails-if as Markus suggested?
Flags: needinfo?(mtseng) → needinfo?(mchang)
Can we land this with AA turned off in WR then? We can turn it back on when we enable text layers. Until we turn those on it shouldn't really matter for anything other than bullet layers, right?
(In reply to Morris Tseng [:mtseng] [:Morris] from comment #17)
> Yap, as kats said. One images get aa-ing and the others doesn't. I did some
> experiments.
> 
> 1. Turn on text-layer by default. Thus, both test cases use aa for text. And
> the reftest is pass.
> 2. Turn off aa in webrender. The reftests are pass as well.
> 
> I agree fuzzy 255 isn't acceptable. Should we set fails-if as Markus
> suggested?

I'm ok with a fails-if right now and a follow up bug to re-enable the tests.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Can we land this with AA turned off in WR then? We can turn it back on when
> we enable text layers. Until we turn those on it shouldn't really matter for
> anything other than bullet layers, right?

I think that may be the case. None of the other enabled layers strike me as needing text. However, I'd prefer to just label these as fails is for now with a follow up bug + a comment in some of the reftests.list files on the follow up bug.
Flags: needinfo?(mchang)
The issue with fails-if is that if text rendering in WR regresses further for whatever reason we won't catch it. And it'll cause more work for whoever is working on text layers.
(I'm assuming it's possible to turn off AA in WR via our bindings.rs code - if it needs to be done upstream in WR then it's not really an option, so I'd be ok with fails-if)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> (I'm assuming it's possible to turn off AA in WR via our bindings.rs code -
> if it needs to be done upstream in WR then it's not really an option, so I'd
> be ok with fails-if)

We can turn it off at [1]. I guess we can disable it for now in the bindings.rs with this patch along with a comment to turn it back on with text layers.

[1] http://searchfox.org/mozilla-central/source/gfx/webrender_bindings/src/bindings.rs#883
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bef07e744f450252bf38660f087f3526f9bcab9a&selectedJob=99349140

I was wrong. Even if turn off subpixel AA, we still get the reftest fail. Though the max difference and count became lower. But it still had some 255 max diff (such as some test cases in R4 and R8). Mason, do you think we still need turn off subpixel AA and set fuzzy-if for those tests? Or just not skipping AA and skip-if tests?
Flags: needinfo?(mchang)
(In reply to Morris Tseng [:mtseng] [:Morris] from comment #23)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bef07e744f450252bf38660f087f3526f9bcab9a&selectedJob=9
> 9349140
> 
> I was wrong. Even if turn off subpixel AA, we still get the reftest fail.
> Though the max difference and count became lower. But it still had some 255
> max diff (such as some test cases in R4 and R8). Mason, do you think we
> still need turn off subpixel AA and set fuzzy-if for those tests? Or just
> not skipping AA and skip-if tests?

Let's not disable AA and just fails-if the tests. It'd also be good to know if they somehow work in the future for some reason rather than skipping.
Flags: needinfo?(mchang)
Comment on attachment 8868450 [details]
Bug 1336986 - Return false in ContainsOnlyColoredGlyphs if no glyphs are drawing.

https://reviewboard.mozilla.org/r/140054/#review143458

Do you have an example of how the caller would deal with this?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #31)
> Comment on attachment 8868450 [details]
> Bug 1336986 - Return false in ContainsOnlyColoredGlyphs if no glyphs are
> drawing.
> 
> https://reviewboard.mozilla.org/r/140054/#review143458
> 
> Do you have an example of how the caller would deal with this?

Yes, for bullet frame, if BulletRenderer::BuildGlyphForText [1] return true, we assume the mFont would be valid and later BulletRenderer::CreateWebRenderCommandsForText [2] will assert mFont and crashes. So, return false for ContainsOnlyColoredGlyphs when font is downloading fix the crash.

[1]: https://searchfox.org/mozilla-central/source/layout/generic/nsBulletFrame.cpp#416
[2]: https://searchfox.org/mozilla-central/source/layout/generic/nsBulletFrame.cpp#501
Comment on attachment 8868450 [details]
Bug 1336986 - Return false in ContainsOnlyColoredGlyphs if no glyphs are drawing.

https://reviewboard.mozilla.org/r/140054/#review143494
Attachment #8868450 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8866654 [details]
Bug 1336986 - Mark fails-if for some tests.

https://reviewboard.mozilla.org/r/138260/#review143536
Attachment #8866654 - Flags: review?(mchang) → review+
Comment on attachment 8866654 [details]
Bug 1336986 - Mark fails-if for some tests.

https://reviewboard.mozilla.org/r/138260/#review143564

Don't we want to fix this?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> Comment on attachment 8866654 [details]
> Bug 1336986 - Mark fails-if for some tests.
> 
> https://reviewboard.mozilla.org/r/138260/#review143564
> 
> Don't we want to fix this?

The fails are because font rendering result mismatch between skia and webrender. Once we turn on text layer, we should get the identical result and we can remove fails-if annotation.
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/48d70f0d29ff
Enable layers.advanced.bullet-layers if webrender is enabled. r=kats
https://hg.mozilla.org/projects/graphics/rev/0b49b72307be
Mark fails-if for some tests. r=mchang
https://hg.mozilla.org/projects/graphics/rev/afdc70d42052
Return false in ContainsOnlyColoredGlyphs if no glyphs are drawing. r=jrmuizel
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.