Closed Bug 778367 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa?])

Attachments

(3 files, 2 obsolete files)

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.
Attachment #646768 - Flags: review?(jmuizelaar)
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.
Blocks: 769993
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.
Attachment #646768 - Flags: review?(jmuizelaar) → review+
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.
Assignee: nobody → bas.schouten
Attachment #646768 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #666130 - Flags: review?(jmuizelaar)
This test appears to have tiny differences because DWrite appears to render differently.
Attachment #666374 - Flags: review?(jmuizelaar)
Blocks: 795692
This corrects a small problem that would occur with PushGroupAndCopyBackground.
Attachment #666130 - Attachment is obsolete: true
Attachment #666130 - Flags: review?(jmuizelaar)
Attachment #666529 - Flags: review?(jmuizelaar)
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
Attachment #666374 - Flags: review?(jmuizelaar) → review+
Attachment #666529 - Flags: review?(jmuizelaar) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/bca6d709dba2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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.
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.
...Ignore the "non-Azure" part. Didn't realize that I was already using Azure here.
...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
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.
We need a Beta approval nom & uplift here - can someone please nominate?
FWIW, what regressed the canvas 2d tests was the patch in bug 795135.
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).
(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.
(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 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
Attachment #666529 - Flags: approval-mozilla-beta?
(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 on attachment 666529 [details] [diff] [review]
Part 1: Minimize intermediate surface size v3

Please land asap so this is in tomorrow's Beta.
Attachment #666529 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
Attachment #666374 - Flags: approval-mozilla-beta?
Attachment #666374 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is there anything QA can do to verify this fix? Or are there automated tests for it?
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.