Closed Bug 1274624 Opened 3 years ago Closed 3 years ago

Poor performance dragging element

Categories

(Core :: Graphics, defect)

49 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- verified

People

(Reporter: gcowie, Assigned: eflores)

References

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(2 files, 2 obsolete files)

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
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
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Product: Firefox → Core
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
Assignee: nobody → edwin
Keywords: perf
Whiteboard: [gfx-noted]
Attached patch 1274624.patch (obsolete) — Splinter Review
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.
Attachment #8770991 - Flags: feedback?(mstange)
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.
(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.
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
This patch makes sure we hit the fast path in this case, and now in most cases involving axis-aligned linear gradients.
Attachment #8770991 - Attachment is obsolete: true
Attachment #8770991 - Flags: feedback?(mstange)
Attachment #8773850 - Flags: review?(mstange)
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.
Attachment #8773850 - Flags: review?(mstange) → review+
Attached patch 1274624-test.patch (obsolete) — Splinter Review
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.
Attachment #8774820 - Flags: review?(mstange)
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.
Attachment #8774820 - Flags: review?(mstange) → review+
(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.
Attachment #8774820 - Attachment is obsolete: true
Attachment #8774919 - Flags: review?(mstange)
Comment on attachment 8774919 [details] [diff] [review]
1274624-test.patch

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

Thanks!
Attachment #8774919 - Flags: review?(mstange) → review+
Pushed by eflores@mozilla.com:
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
Pushed by eflores@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ef7be15608
Fuzz linear-size-1a.html on Windows - r=bustage
See Also: → 1291528
Depends on: 1304704
Flags: qe-verify+
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
Thank you, Carmen!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1315113
You need to log in before you can comment on or make changes to this bug.