778367 - [Azure] Minimize intermediate surface size used by push/popgroup

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Minimize intermediate surface size

In order do reduce fill-rate requirements for push/popgroups with small clip bounds, we should minimize the intermediate surface's size to the bounds of the clipped region.

Comment 1

[Bas Schouten (:bas.schouten)]

Jeff, we really need this! We should get it on beta quickly if we still want Azure-Thebes to be on in the next cycle.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 646768 [details] [diff] [review]
Minimize intermediate surface size

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

This makes me somewhat sad, as I'd like to not pay this cost on backends that already do this. That being said, it does seem like a reasonable short term solution.

Apologies for this review taking so long.

::: gfx/thebes/gfxContext.h
@@ +731,5 @@
>      mozilla::RefPtr<DrawTarget> parentTarget;
>      mozilla::gfx::AntialiasMode aaMode;
>      bool patternTransformChanged;
>      Matrix patternTransform;
> +    mozilla::gfx::Point deviceOffset;

Add a comment about how this is for minimizing intermediate surface size.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Minimize intermediate surface size v2

This is another attempt, the previous caused some test failures.

I've now used mTransform (which was an 'unused' member of gfxContext) as the place to store our UserToDevice (not DrawTarget!) transform. Based on that we do conversions and translations, etc.

Only when we actually set the transform on our drawtarget we now adjust for the draw target offset from device origin. This is easier to read and eliminates most of the GetTransform costs.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Mark a test fuzzy where DWrite renders differently to a smaller surface

This test appears to have tiny differences because DWrite appears to render differently.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Minimize intermediate surface size v3

This corrects a small problem that would occur with PushGroupAndCopyBackground.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 666374 [details] [diff] [review]
Mark a test fuzzy where DWrite renders differently to a smaller surface

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

Add a comment about why it's fuzzy

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/377cda004bd7 - I know I saw it on Try, but maybe the world changed out from under you since then, because you were failing 650228-1.html, and opacity-mixed-scrolling-2.html by 91/1197, needs moar fuzz, on Windows.

Comment 8

[timbugzilla]

I tried a moz-inbound win32 PGO build [1] with these patches in, but there were severe painting regressions. Screenshot from the following page attached.

http://www.daimenhutchison.com/rugby/index.php/topic/37224-t20-world-cup/page__st__450

http://hg.mozilla.org/integration/mozilla-inbound/rev/a6991bc0778d

Comment 9

[timbugzilla]

Attachment: painting regressions on an IP board forum page

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bca6d709dba2

Comment 11

[timbugzilla]

I am still seeing painting problems on the latest moz-central win32 PGO build that includes these patches. 

https://hg.mozilla.org/mozilla-central/rev/635fcc11d2b1

I also observed them on the try build with just these patches.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

I have a suspiscion (from partial bisection) that this is the cause of Bug 800658 --- a regression in a few canvas 2d tests on non-Azure that slipped in during the 3-week window during which I had accidentally disabled canvas tests.

Bisection continues.... (made tricky by this having landed thrice).

Btw, it's nice to have hg links for each landing on a bug.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

...Ignore the "non-Azure" part. Didn't realize that I was already using Azure here.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

...ah, on Linux, the regression is only with the default Azure/Cairo. No regression with Azure/Skia.

Testcase:
http://philip.html5.org/tests/canvas/suite/tests/2d.path.isPointInPath.transform.2.html

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Sorry for the false alarm! If I'm not mistaken, my current regression window is
last good: 024f0c7ca3fc
first bad: aa73f5544e07
This no longer includes any Bas.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

We need a Beta approval nom & uplift here - can someone please nominate?

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

FWIW, what regressed the canvas 2d tests was the patch in bug 795135.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Bas - is this ready to nominate now?  We'd want to see this fix go into Beta 3 and thus be landed by EOD PT tomorrow (Monday).

Comment 19

[Bas Schouten (:bas.schouten)]

(In reply to Lukas Blakk [:lsblakk] from comment #18)
> Bas - is this ready to nominate now?  We'd want to see this fix go into Beta
> 3 and thus be landed by EOD PT tomorrow (Monday).

No, it's not in my opinion, there's a top crasher still where this is high on the 'suspect' list. I've already got one fix ready to try, and I'm working on another. I'm going to do what I can to get those in -today-. But then we'll still have to await tomorrow's crash stats to be 100% sure it's fixed. If it is we could land the whole batch tomorrow. But if it's not or if there's insufficient data by the time of tomorrow the risk is probably too high.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Bas Schouten (:bas) from comment #19)
> (In reply to Lukas Blakk [:lsblakk] from comment #18)
> > Bas - is this ready to nominate now?  We'd want to see this fix go into Beta
> > 3 and thus be landed by EOD PT tomorrow (Monday).
> 
> No, it's not in my opinion, there's a top crasher still where this is high
> on the 'suspect' list. I've already got one fix ready to try, and I'm
> working on another. I'm going to do what I can to get those in -today-. But
> then we'll still have to await tomorrow's crash stats to be 100% sure it's
> fixed. If it is we could land the whole batch tomorrow. But if it's not or
> if there's insufficient data by the time of tomorrow the risk is probably
> too high.

OK, thanks for the speedy response.  If we have to - we can take this for Beta 4 so we have time for more data.

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 666529 [details] [diff] [review]
Part 1: Minimize intermediate surface size v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 715768
User impact if declined: Reduced performance
Testing completed (on m-c, etc.): Extensive nightly coverage and significant Aurora. This also needs bugfixes in bug 797314, bug 797231, bug 797797, bug 758531 and bug 803949.
Risk to taking this patch (and alternatives if risky): Need to switch of Azure content, reducing performance.
String or UUID changes made by this patch: None

Comment 22

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas.schouten) from comment #21)
> Comment on attachment 666529 [details] [diff] [review]
> Part 1: Minimize intermediate surface size v3
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 715768
> User impact if declined: Reduced performance
> Testing completed (on m-c, etc.): Extensive nightly coverage and significant
> Aurora. This also needs bugfixes in bug 797314, bug 797231, bug 797797, bug
> 758531 and bug 803949.
> Risk to taking this patch (and alternatives if risky): Need to switch of
> Azure content, reducing performance.
> String or UUID changes made by this patch: None

The former comment had a mistake, bug 797231 is -not- a regression from this bug. It's good to have but not needed to fix anything introduced by this fix.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 666529 [details] [diff] [review]
Part 1: Minimize intermediate surface size v3

Please land asap so this is in tomorrow's Beta.

Comment 24

[Bas Schouten (:bas.schouten)]

Comment on attachment 666374 [details] [diff] [review]
Mark a test fuzzy where DWrite renders differently to a smaller surface

We need this too so tests succeed!

Comment 25

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/releases/mozilla-beta/rev/fd4cc20c6a2e

Comment 26

[Mihaela Velimiroviciu (:mihaelav)]

Is there anything QA can do to verify this fix? Or are there automated tests for it?