1459760 - Bookmarks star animation flickers (regression from servo/webrender#2644)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Markus Stange [:mstange]]

Steps to reproduce:
 1. Enable gfx.webrender.all
 2. Go to a website that you don't have bookmarked.
 3. Click the star icon at the end of the URL button.

Expected results:
The animation should look good.

Actual results:
The star flickers during the animation.

Comment 1

[Markus Stange [:mstange]]

I must have accidentally clicked the security sensitive checkbox. This bug is not security sensitive.

Comment 2

[Darkspirit]

Are you on MacOS? I couldn't reproduce on Debian Testing (Radeon RX480) and Win10 (Nvidia GTX 1060) so far.

Comment 3

[Markus Stange [:mstange]]

Yes I am.

Comment 4

[Darkspirit]

Regression range of bug 1460389:
last good:
mozregression --repo autoland --launch 3b84be9ee9e6a19d8e49aa86b71c9d60b5eaec8b --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://github.com/carllerche/h2/pull/273'

first bad:
mozregression --repo autoland --launch 1a51d479e4404608fea41fc4a8e3d5b21b1850a7 --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://github.com/carllerche/h2/pull/273'

Does this have the same range? (If yes, why am I not able to reproduce it?^^)

Comment 5

[Darkspirit]

(Your report is older. That makes no sense.)

Comment 6

[Markus Stange [:mstange]]

> 32:17.19 INFO: No more inbound revisions, bisection finished.
> 32:17.19 INFO: Last good revision: cfc5d19e856fc43766669069b7e2695dd519e5c3
> 32:17.19 INFO: First bad revision: 81f389c2524e58b4b95844667719cc1642573019
> 32:17.19 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cfc5d19e856fc43766669069b7e2695dd519e5c3&tochange=81f389c2524e58b4b95844667719cc1642573019

This was regressed by the WebRender update in bug 1440664, two months ago.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

I did local builds to bisect that WR update but for me all those builds were good. I ran mozregression for the STR and got a different result:

15:26.76 INFO: Last good revision: 16a647445e5cd89ffbefb3f3a490acaaa4f8868e
15:26.76 INFO: First bad revision: 25c5ae00eb5388349d20231d360c6b7710576afd
15:26.76 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=16a647445e5cd89ffbefb3f3a490acaaa4f8868e&tochange=25c5ae00eb5388349d20231d360c6b7710576afd

which points to bug 1457241.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

.. and within that WR update [1] is good and [2] is bad, which points to servo/webrender#2644.

Markus, can you check these two builds to confirm good/bad?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6f3d29802caa54931079ac30ae7accc6ecc338
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d988eae089d461267498b7923523657d52ee68

Comment 9

[Markus Stange [:mstange]]

https://hg.mozilla.org/try/rev/6b6f3d29802caa54931079ac30ae7accc6ecc338 is good
https://hg.mozilla.org/try/rev/f4d988eae089d461267498b7923523657d52ee68 is bad

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Thanks.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Mac only, dropping to P2

Comment 12

[Aria Beingessner [:Gankra]]

I'm seeing this on windows still.

Comment 13

[Aria Beingessner [:Gankra]]

result from some investigation: the bookmark button uses the same technique as the refresh button to animate (using a css animated transform on an SVG spritesheet to run the animation off-main-thread) **but** it also uses -moz-context-properties, which is a dark magic tool to inject values into an svg image from the DOM -- in this case, setting the svg's "stroke" property.

It appears that internally this is implemented by just inlining the contents of the svg into the display list, which changes this from "animating an external image" to "animating a blob image".

It's possible then that either we are invalidating the blob incorrectly (which we know definitely happens in at least two cases), or simply too slowly.

Comment 14

[Aria Beingessner [:Gankra]]

strangely i can't seem to reproduce this on linux? Not much platform specific about blobs...

Comment 15

[Kartikaya Gupta (email:[e-mail])]

I'll try and reduce this to a simpler testcase.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

I made a standalone page that reproduces the animation as closely as I can manage (you need to set svg.context-properties.content.enabled=true to make it work in Firefox) but it doesn't reproduce the missing frame problem. I'll try debugging the problem directly a little bit, maybe that will shed some light on what I missed in my reduced test case.

https://staktrace.github.io/moz-pages/bug/1459760/index.html

Comment 17

[Kartikaya Gupta (email:[e-mail])]

So the bookmark star animation is an SVG image that's 660px by 40px, containing 20 sprites that are 33px wide each. There's a CSS transform animation on it that translates it leftwards 19 times by 33px each, and that's what produces the visual animation effect. This transform is intended to run on the compositor, and in my local debugging it mostly did, and in those cases the animation frames were not visible. Randomly I'd get new display list builds during the animation, and those frames would be visible, producing a flickering effect. (Also I slowed down the animation to take 5+ seconds so that it was easier to see).

In my test page, the SVG is a Background display item which we can successfully render via CreateWebRenderCommands. However, in the chrome, the SVG is a xul image item:

XULImage p=0x135b10760 f=0x123697180(ImageBox(image)(0) id:star-button-animatable-image) key=76 bounds(0,0,39600,2400) layerBounds(0,0,39600,2400) visible(0,0,39600,2400) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x123697180 agr=0x123697180

and when we call CreateWebRenderCommands on it, it fails at [1], causing us to fall back to blob rendering for it. That being said, we do seem to use the entire 660x40 size when rendering it as a blob, so from that point of view it should still work fine.

Given that blobs are currently rendered during the rendering phase, it's might be that the blob rendering code isn't taking into account the OMTA transform that gets updated, or maybe there's a WR bug in that it doesn't adjust the clip on the blob correctly. I'll inspect that next.

But at any rate, the XULImage vs Background display item difference seems to explain why I can reproduce this problem in the UI but not in a content page. Also the intermittency of the behaviour seems explainable by when new scenes are built.

[1] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/layout/xul/nsImageBoxFrame.cpp#555

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Correction: tnikkel pointed out that the XULImage display item is not the important one here, because the element in question has the image set via the background-image CSS property, which goes in the Background item:

Background p=0x12fd70520 f=0x12c026388(ImageBox(image)(0) id:star-button-animatable-image) key=2 bounds(0,0,39600,2400) layerBounds(0,0,39600,2400) visible(0,0,39600,2400) componentAlpha(0,0,0,0) clip(0,0,39600,2400) asr() clipChain(0x12fd70720 <0,0,39600,2400> [root asr]) ref=0x12c026388 agr=0x12c026388

I'll redo some of my debugging with this in mind.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: WR capture of bad frame

Stashing a capture of the bad frame. The display item with the image_key: ((1), 40) in scene-1-0.ron is the SVG image that is transformed to produce the animation effect. It's an external image, rather than a blob image. And there's a PushReferenceFrame a few display items up with an animated transform binding which is the binding that we update at render time.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

mstange/Gankro: can you still repro this problem on the latest nightly? I can sort of see it in a local build but it looks different than it used to. In a nightly from CI it seems to work fine.

Comment 21

[Markus Stange [:mstange]]

It's no longer happening for me. I don't know whether that's due to WebRender changes or UI styling changes.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

I bisected, and it was fixed by https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bbbbc8a982fea935173e1b13499cb366c74863b5&tochange=f7659b60b7b02ccdcf902f34273f7c386620cafd - i.e. bug 1474300. I didn't bother isolating which PR did it.

Comment 23

[Ciprian Georgiu, Desktop QA]

I've tried to reproduce this bug, but unfortunately I cannot see any flickers on the star icon with an affected Nightly build (2018-05-07). I've used Windows 10 x64, 7 x64, macOS 10.13 and STR from comment 0.

Markus, could you please help us verify this bug on latest Beta [1], since I wasn't able to reproduce the initial issue?

[1] https://www.mozilla.org/en-US/firefox/channel/desktop/

Comment 24

[Darkspirit]

Beta doesn't matter yet. Have you restarted Nightly after flipping the pref?

Comment 25

[Ciprian Georgiu, Desktop QA]

Yes, I have restarted the browser after changing the pref.

If Beta doesn't matter yet (I assume because the pref is not created in Beta at the moment), I think we can try to verify this only on latest Nightly.

Did you manage to reproduce the issue Jan, if so could you please attach a screencast; maybe I'm missing something here.

Comment 26

[Markus Stange [:mstange]]

WebRender is not enabled on Beta 63, so this does not need to be verified on Beta.