1274624 - Poor performance dragging element

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Gordon Cowie]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160520030251

Steps to reproduce:

http://pebblecode.com/404/

Click and drag the windows


Actual results:

The windows move very slowly, less than 1FPS

I did a performance trace and the log shows 3+ second "Paint" events


Expected results:

Windows should move smoothly, 60FPS - as in Chrome and Internet Explorer

Comment 1

[Abe - QA (:Abe_LV)]

Reproducible. The windows move very slowly in Firefox as compared to other browsers. 

---
Version 	49.0a1
Build ID 	20160524030227
Update Channel 	nightly
User Agent 	Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

The slowness here is due to these CSS rules, used to draw the background:

> background-image: linear-gradient(0deg, transparent 50%, #8592e1 50%),linear-gradient(90deg, #8592e1 50%, #000 50%);
> background-size: 2px 2px, 2px 2px;

The page is drawing the background using 2x2 pixel tiles, and we appear to be rendering the gradient for each one of these tiles [1]. This takes some time.


[1] https://dxr.mozilla.org/mozilla-central/rev/3c5025f98e561a20e24d97c91a9e4e0ec28015ea/layout/base/nsCSSRendering.cpp#2836-2912

Comment 3

[[github robot]]

Commits pushed to master at https://github.com/taskcluster/taskcluster-tools

https://github.com/taskcluster/taskcluster-tools/commit/3f6301bf11a9c930630c29a9dad9ccd3e81cc4ef
Bug 1274624 - Fixes floating zero in taskview UI

https://github.com/taskcluster/taskcluster-tools/commit/2fbd7391422bb93bf165298c5094cb5388b7b290
Merge pull request #117 from eliperelman/bug-1284624

Bug 1274624 - Fixes floating zero in taskview UI

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1274624.patch

This patch adds the ability for PaintGradient to paint to a temporary surface, and use that as a pattern to fill the provided rendering context.

Since we snap each tile and aRepeatSize is rational in device space, we can't just tile by aRepeatSize. But we can determine a 'real' repeat size that takes snapping into account. We then paint to a surface of that size, and then paint from that surface to the original context.

Not sure I like this patch, so just asking f? for now.


A different approach, if we don't mind where we're rounding, would be to paint four tiles of size {floor,ceil}(aRepeatSize.width) x {floor,ceil}(aRepeatSize.height) and group all the roundings together. i.e. all tiles of floor(width) would be grouped to the left, and all tiles of ceil(width) would be grouped to the right; and similar vertically. We would still need our current code, though, in case we get a ridiculously large aRepeatSize.

Comment 5

[Markus Stange [:mstange] [on PTO until Sept 30]]

Is aRepeatSize fractional on this page? In other words, if you bail out for non-device pixel aligned repeat sizes, does it fix the problem on this page?

I thought we had code that basically turned linear-gradient + background-repeat into a repeating-linear-gradient. Then we'd fill the whole area using one gradient fill. Is that code not kicking in here? Or am I misremembering?

The GCD idea is brilliant. I like it.
And the tilePixels.value() < dirtyPixels.value() check is spot on.

A few months ago Mason sped up repeated scaled raster image drawing by scaling up one instance of the image and then repeating the scaled up surface. We only did that when the repeat size was aligned with device pixels, and just fell back to the old code when it was not. It fixed most of our image-scaling related problems on macOS, probably because most images are CSS pixel aligned and scale factors on macOS are usually 1.0 or 2.0 because not many people zoom.

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Markus Stange [:mstange] from comment #5)
> Is aRepeatSize fractional on this page? In other words, if you bail out for
> non-device pixel aligned repeat sizes, does it fix the problem on this page?

Yes, my first version of this patch did that, but I got a bit carried away trying to generalise it.

> I thought we had code that basically turned linear-gradient +
> background-repeat into a repeating-linear-gradient. Then we'd fill the whole
> area using one gradient fill. Is that code not kicking in here? Or am I
> misremembering?

That does seem to be the case. If the gradient is axis-aligned and the length of the destination rect along that axis, we should be doing that. I'll investigate.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Just got the chance to look at this again today.

The fast path won't be hit currently for any gradient whose first stop isn't at position 0[1]. This is because we don't use the original gradient line: we normalise the stops we have to the range [0, 1] so the start point isn't where we think it is, and we end up failing the check at [2].

In this particular case, the it *also* fails the |stopDelta != 0| check above that because all of the stops are in the same place.


[1] Technically not quite correct -- any gradient whose first stop is on the destination rect boundary and meets all the other conditions should hit this path.
[2] https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/layout/base/nsCSSRendering.cpp#2752

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1274624-linear.patch

This patch makes sure we hit the fast path in this case, and now in most cases involving axis-aligned linear gradients.

Comment 9

[Markus Stange [:mstange] [on PTO until Sept 30]]

Comment on attachment 8773850 [details] [diff] [review]
1274624-linear.patch

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

Looks good! Can you add a reftest for at least one of the hard InterpolateColorStop cases?

::: layout/base/nsCSSRendering.cpp
@@ +2713,5 @@
> +    // right way up.
> +    if (lineStart.x > lineEnd.x || lineStart.y > lineEnd.y) {
> +      gfxPoint tmp = lineStart;
> +      lineStart = lineEnd;
> +      lineEnd = tmp;

std::swap please.

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1274624-test.patch

The only non-trivial (to me) part to InterpolateColorStop was getting the alpha blending correct. This test pokes at that in a way that would fail quite clearly.

Comment 11

[Markus Stange [:mstange] [on PTO until Sept 30]]

Comment on attachment 8774820 [details] [diff] [review]
1274624-test.patch

Thanks. I expect you'll probably need some fuzzy() annotations though, because gradient rendering is always a little noisy, even when you don't expect it to be.

Could you also add a test that compares a repeated linear-gradient to a repeating-linear-gradient? Maybe one that hits the swap + scale(-1) case.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1274624-test.patch

(In reply to Markus Stange [:mstange] from comment #11)
> Thanks. I expect you'll probably need some fuzzy() annotations though,
> because gradient rendering is always a little noisy, even when you don't
> expect it to be.

Yep, will add that after a try run since it's green locally.

> Could you also add a test that compares a repeated linear-gradient to a
> repeating-linear-gradient? Maybe one that hits the swap + scale(-1) case.

Done.

Comment 13

[Markus Stange [:mstange] [on PTO until Sept 30]]

Comment on attachment 8774919 [details] [diff] [review]
1274624-test.patch

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

Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72ae67aeb498
Speed up tiled axis-aligned linear gradient painting - r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d58d0626631
Test that colour interpolation is correct for clamped linear gradients - r=mstange

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ef7be15608
Fuzz linear-size-1a.html on Windows - r=bustage

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72ae67aeb498
https://hg.mozilla.org/mozilla-central/rev/0d58d0626631
https://hg.mozilla.org/mozilla-central/rev/f7ef7be15608

Comment 17

[Carmen]

I was able to reproduce the issue on Nightly 49.0a1 (2016-05-10) on Win 10 Pro x64.

I have verified the fix on Beta 50.0b3 Build 1 using Win 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Yosemite 10.10.4

Comment 18

[Cristian Comorasu, QA [:ccomorasu], Release Desktop QA]

Thank you, Carmen!