The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

(Blocks: 1 bug)

unspecified
mozilla18
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox16 unaffected, firefox17+ fixed, firefox18+ fixed)

Details

(Whiteboard: [qa?])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #646768 - Flags: review?(jmuizelaar)
(Assignee)

Comment 1

5 years ago
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+
(Assignee)

Comment 3

5 years ago
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.
Assignee: nobody → bas.schouten
Attachment #646768 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #666130 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

5 years ago
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.
Attachment #666374 - Flags: review?(jmuizelaar)
(Assignee)

Updated

5 years ago
Blocks: 795692
(Assignee)

Comment 5

5 years ago
Created attachment 666529 [details] [diff] [review]
Part 1: Minimize intermediate surface size v3

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.

Comment 8

5 years ago
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

5 years ago
Created attachment 667254 [details]
painting regressions on an IP board forum page
https://hg.mozilla.org/mozilla-central/rev/bca6d709dba2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 11

5 years ago
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.
Depends on: 797314
Blocks: 770524
No longer blocks: 770524

Updated

5 years ago
tracking-firefox17: --- → +
tracking-firefox18: --- → +

Updated

5 years ago
status-firefox16: --- → unaffected
status-firefox17: --- → affected
status-firefox18: --- → fixed
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).
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
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?
(Assignee)

Comment 22

5 years ago
(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+
(Assignee)

Comment 24

5 years ago
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+
(Assignee)

Comment 25

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

Updated

5 years ago
status-firefox17: affected → fixed
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.