Last Comment Bug 778367 - [Azure] Minimize intermediate surface size used by push/popgroup
: [Azure] Minimize intermediate surface size used by push/popgroup
Status: RESOLVED FIXED
[qa?]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on: 797314
Blocks: 795692 769993
  Show dependency treegraph
 
Reported: 2012-07-27 17:07 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-11-16 06:05 PST (History)
16 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
fixed


Attachments
Minimize intermediate surface size (8.25 KB, patch)
2012-07-27 17:07 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Minimize intermediate surface size v2 (20.93 KB, patch)
2012-09-28 17:55 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Mark a test fuzzy where DWrite renders differently to a smaller surface (1.60 KB, patch)
2012-09-30 13:53 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Part 1: Minimize intermediate surface size v3 (21.09 KB, patch)
2012-10-01 07:01 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
painting regressions on an IP board forum page (74.97 KB, image/png)
2012-10-02 17:19 PDT, timbugzilla
no flags Details

Description Bas Schouten (:bas.schouten) 2012-07-27 17:07:22 PDT
Created attachment 646768 [details] [diff] [review]
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) 2012-08-27 19:47:06 PDT
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] 2012-09-04 23:36:39 PDT
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) 2012-09-28 17:55:35 PDT
Created attachment 666130 [details] [diff] [review]
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) 2012-09-30 13:53:37 PDT
Created attachment 666374 [details] [diff] [review]
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) 2012-10-01 07:01:08 PDT
Created attachment 666529 [details] [diff] [review]
Part 1: Minimize intermediate surface size v3

This corrects a small problem that would occur with PushGroupAndCopyBackground.
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-10-01 12:15:35 PDT
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, back in August) 2012-10-01 17:34:41 PDT
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 2012-10-02 17:18:45 PDT
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 2012-10-02 17:19:30 PDT
Created attachment 667254 [details]
painting regressions on an IP board forum page
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-10-02 18:48:43 PDT
https://hg.mozilla.org/mozilla-central/rev/bca6d709dba2
Comment 11 timbugzilla 2012-10-03 03:17:48 PDT
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) 2012-10-12 23:20:54 PDT
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) 2012-10-12 23:23:20 PDT
...Ignore the "non-Azure" part. Didn't realize that I was already using Azure here.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-10-12 23:25:06 PDT
...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) 2012-10-13 00:04:21 PDT
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 2012-10-15 09:32:46 PDT
We need a Beta approval nom & uplift here - can someone please nominate?
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-10-15 10:07:05 PDT
FWIW, what regressed the canvas 2d tests was the patch in bug 795135.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:44:52 PDT
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) 2012-10-21 09:56:29 PDT
(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 2012-10-21 10:10:34 PDT
(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) 2012-10-26 18:55:54 PDT
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) 2012-10-26 19:00:32 PDT
(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 2012-10-29 10:04:19 PDT
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) 2012-10-29 16:25:52 PDT
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) 2012-10-29 17:46:59 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/fd4cc20c6a2e
Comment 26 Mihaela Velimiroviciu (:mihaelav) 2012-11-16 06:05:12 PST
Is there anything QA can do to verify this fix? Or are there automated tests for it?

Note You need to log in before you can comment on or make changes to this bug.