Pass the D3DSurface9 down into Cairo so that it can release the DC and LockRect to get at the bits

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: BenWa)

Tracking

unspecified
mozilla26
All
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P1][Australis:M?])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

5 years ago
This will avoid having to do an allocation and two copies
(Reporter)

Updated

5 years ago
Blocks: 907546
(Reporter)

Comment 1

5 years ago
Created attachment 793985 [details] [diff] [review]
A rough sketch of what this would look like

The basic idea is here it just needs polished up into something that builds.
Whiteboard: [Australis:P?][Australis:M?]
Jeff, can you take this? If not, can you reassign it to someone else on your team who may be able to help us out here?

Marking as P1 since this will be instrumental to fixing a major TART regression (bug 907546).
Assignee: nobody → jmuizelaar
Status: NEW → ASSIGNED
Whiteboard: [Australis:P?][Australis:M?] → [Australis:P1][Australis:M?]
No short term help from the graphics team that we can commit to.  Unassigning to not give false hope.
Assignee: jmuizelaar → nobody
Created attachment 797285 [details] [diff] [review]
Buggy WIP

Updating the progress we made.
Attachment #793985 - Attachment is obsolete: true
Comment on attachment 797285 [details] [diff] [review]
Buggy WIP

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

::: gfx/thebes/gfxWindowsSurface.cpp
@@ +37,5 @@
> +gfxWindowsSurface::gfxWindowsSurface(IDirect3DSurface9 *surface, uint32_t flags) :
> +    mOwnsDC(false), mForPrinting(false), mWnd(nullptr)
> +{
> +    mDC = 0; // GetDC will fail
> +    Init(cairo_win32_surface_create_with_d3dsurface9(surface));

This patch disables most of the use of the Direct3DSurface9 and just gets the DC. This should be nearly equivilant to the old code but this fails. This needs to be debug before re-enabling the D3DSurface9 codepaths.
(Assignee)

Comment 6

5 years ago
The above two comment should of been made under my account.
(Assignee)

Comment 7

5 years ago
Created attachment 797996 [details] [diff] [review]
patch v1
Attachment #797285 - Attachment is obsolete: true
Attachment #797996 - Flags: review?(jmuizelaar)
(Assignee)

Comment 8

5 years ago
mconley can you get us some performance numbers for this patch see if it helps you?
Flags: needinfo?(mconley)
I'll do some local measurements as well, but for thoroughness, I've done some try pushes:

m-c baseline: https://tbpl.mozilla.org/?tree=Try&rev=aa679ad1ed19
m-c with patch: https://tbpl.mozilla.org/?tree=Try&rev=71b0f1d88072

UX baseline: https://tbpl.mozilla.org/?tree=Try&rev=f2824c8ebafe
UX with patch: https://tbpl.mozilla.org/?tree=Try&rev=6ef776e4bdf8
Flags: needinfo?(mconley)
Results are in - here's the data:

For m-c, this is a comparison of the TART measurements from baseline to m-c with the patch (green is indicates a performance improvement from the patch, red represents a regression):

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010917,29011681,29011687,29011709,29011733,29011745,29011759&newTestIds=29010861,29011545,29011577,29011583,29011595,29011607,29011629&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

Here's the same thing for UX - green = good, red = bad:

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010947,29011657,29011693,29011703,29011715,29011721,29011727&newTestIds=29010931,29011551,29011565,29011571,29011601,29011613,29011635&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

And here's a comparison between m-c with patch, and UX with patch (where m-c with patch is "Original" and UX with patch is "New"):

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010861,29011545,29011577,29011583,29011595,29011607,29011629&newTestIds=29010931,29011551,29011565,29011571,29011601,29011613,29011635&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

Which seems to be an improvement over m-c without the patch, and UX without the patch:

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29010917,29011681,29011687,29011709,29011733,29011745,29011759&newTestIds=29010947,29011657,29011693,29011703,29011715,29011721,29011727&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

The data is a little noisy, but here are some thoughts:

1) The "error" measurements have always been a little volatile, so it's not surprising that we're seeing them jump around a bit here. For those who don't know, "error" is an absolute measurement of how many milliseconds the actual transition time differs from the duration specified in the CSS.

2) This patch appears to be a win for both m-c and UX, but these measurements suggest that UX gets a little bit of an advantage on some of the measurements. Notably, UX does better on the "fade" tests, which exercises / measures the animation only, without adding or removing tabs or doing any of the other stuff that tabbrowser.xml does when opening and closing tabs.

TL;DR:  We should see a general TART improvement overall, but we'll likely see a small win for Windows XP in the fade tests for UX.

This doesn't kill the UX branch TART regression, but it's definitely a step in the right direction. So I think this is a big thumbs up from me.

Feel free to contradict my reading - I'm totally open to hearing other interpretations.
And by "small win" for the fade tests, I should have been more clear - I expect this to essentially neutralize the regression on the fade measurements.
(Assignee)

Comment 12

5 years ago
These look great. I noticed a small problem causing the scrollbars not rendering. I'll see if I can find why.
(Reporter)

Comment 13

5 years ago
(In reply to Benoit Girard (:BenWa) from comment #12)
> These look great. I noticed a small problem causing the scrollbars not
> rendering. I'll see if I can find why.

I suspect this could be caused by a broken GetDC implementation.
(Assignee)

Comment 14

5 years ago
On the gfxWindowSurface? That would explain it. I'll take a look.
This looks great!

It appears that the patch solidly improves performance both on m-c and on ux, and ux gains slightly more from it.

This means that when we check ux TART regressions compared to m-c, it would only very slightly improve, regardless of the fact that this appears to be a great all-around win.

As for the .error values, while I wouldn't call them "volatile" (as they're pretty stable), they do indeed represent a different thing (as Mike described) than the average intervals which are .half/.all .

So I wouldn't worry too much about them as long as they (or the regression compared to m-c) don't get into absolute ranges of ~10-20ms (which means the animation took another frame to complete).

The specific absolute values we're seeing in the details from comment 10 are generally sub 2ms, and the difference from m-c is typically less than 1ms. This is totally within noise level of these values and means ux is pretty much identical to m-c on these .error values.
(Assignee)

Comment 16

5 years ago
I ran with glass and found more wasn't rendering. I implemented GetDC but it didn't work and its hard to debug while traveling. I'll have to debug it Tuesday. Until then the number may not be trustworthy.
(In reply to Benoit Girard (:BenWa) from comment #16)
> I ran with glass and found more wasn't rendering. I implemented GetDC but it
> didn't work and its hard to debug while traveling. I'll have to debug it
> Tuesday. Until then the number may not be trustworthy.

Sure - happy to run the numbers again when the next patch comes in.
(Assignee)

Comment 18

5 years ago
Created attachment 799071 [details] [diff] [review]
patch v2

Fixed the problem
Assignee: nobody → bgirard
Attachment #797996 - Attachment is obsolete: true
Attachment #797996 - Flags: review?(jmuizelaar)
Attachment #799071 - Flags: review?(jmuizelaar)
Thanks - patch pushed to Try based on m-c:

https://tbpl.mozilla.org/?tree=Try&rev=dff890def885

and based on UX:

https://tbpl.mozilla.org/?tree=Try&rev=a780ad91b47c

I'll toss up the breakdown tonight.
(Assignee)

Comment 21

5 years ago
Created attachment 799152 [details]
M-Other failure
(Reporter)

Comment 22

5 years ago
Comment on attachment 799071 [details] [diff] [review]
patch v2

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

::: gfx/cairo/cairo/src/cairo-win32-surface.c
@@ +522,5 @@
> +    if (hr) {
> +        IDirect3DSurface9_GetDC (surface->d3d9surface, &surface->dc);
> +        return CAIRO_INT_STATUS_UNSUPPORTED;
> +    }
> +    local = cairo_image_surface_create_for_data (rectout.pBits, surface->format, width, height, rectout.Pitch);

Probably worth wrapping this line

@@ +737,5 @@
>  						interest_rect->x,
>  						interest_rect->y,
>  						interest_rect->width,
> +						interest_rect->height, &local);
> +	

trailing whitespace here and other places.

::: gfx/layers/d3d9/ThebesLayerD3D9.cpp
@@ +563,5 @@
>        break;
>      }
>    }
>    NS_ASSERTION(srcTextures.Length() == destTextures.Length(), "Mismatched lengths");
> +  destinationSurface = nullptr;

this line could use a comment too.

::: gfx/thebes/gfxWindowsSurface.cpp
@@ +36,5 @@
>  
> +gfxWindowsSurface::gfxWindowsSurface(IDirect3DSurface9 *surface, uint32_t flags) :
> +    mOwnsDC(false), mForPrinting(false), mWnd(nullptr)
> +{
> +    mDC = 0; // GetDC will fail

This comment needs to be removed.
Attachment #799071 - Flags: review?(jmuizelaar) → review+
Hrm. The results of this last patch are less encouraging than the previous patch.

While we're definitely still seeing wins for m-c and UX both, when comparing m-c+patch and UX+patch, UX seems to fare worse than m-c vs UX. :/

A few possibilities:

1) Something is different about this patch that is causing the change
2) We're seeing noise in the measurements
3) We're being affected by the fact that my original measurements in comment 10 were on PGO builds, and these latest measurements are all on non-PGO builds.


m-c (06dad82dbb36) vs m-c+patch (dff890def885):

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29104563&newTestIds=29083787,29106793,29106803,29106815&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

UX (21244604bef8) vs UX+patch (a780ad91b47c)

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29060413,29061533,29061539,29061551,29061557&newTestIds=29083485,29106787,29106809,29106821&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

m-c+patch (dff890def885) vs UX+patch (a780ad91b47c)

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29083787,29106793,29106803,29106815&newTestIds=29083485,29106787,29106809,29106821&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

m-c (06dad82dbb36) vs UX (21244604bef8)

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29104563&newTestIds=29060413,29061533,29061539,29061551,29061557&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org
(Assignee)

Comment 24

5 years ago
Created attachment 799573 [details] [diff] [review]
patch v3

Fixed failure, review comment and changed GetDC function.
Attachment #799071 - Attachment is obsolete: true
Attachment #799152 - Attachment is obsolete: true
Attachment #799573 - Flags: review?(jmuizelaar)
(Assignee)

Comment 25

5 years ago
Created attachment 799593 [details] [diff] [review]
patch

Minor tweaks
Attachment #799573 - Attachment is obsolete: true
Attachment #799573 - Flags: review?(jmuizelaar)
Attachment #799593 - Flags: review?(jmuizelaar)
(Reporter)

Updated

5 years ago
Attachment #799593 - Flags: review?(jmuizelaar) → review+
OS: Mac OS X → Windows XP
Hardware: x86 → All
(Assignee)

Comment 27

5 years ago
Waiting on try and still need to fix indention and restore GetDC.
(Assignee)

Comment 28

5 years ago
Created attachment 799756 [details] [diff] [review]
patch v4

fixed DC and indentation
Attachment #799593 - Attachment is obsolete: true
Attachment #799756 - Flags: review+
This seems to give a meaningful 6% TART improvement on windows xp: http://graphs.mozilla.org/graph.html#tests=[[293,131,37],[281,131,37]]&sel=1378246162000,1378418962000&displayrange=7&datatype=running

And also 16% tsvgx improvements, also on xp: http://graphs.mozilla.org/graph.html#tests=[[293,131,37],[281,131,37]]&sel=1378246162000,1378418962000&displayrange=7&datatype=running

But other windows platforms are less affected by this (because they don't use D3D9 by default on our try slaves?).

It also has unexpected (to me) 30% improvement on tsvgr_opacity on ubuntu32, but 100% regression on ubuntu 64.

Talos compare of revisions 365e150efda0 (old) to b5d62f5c733c: http://compare-talos.mattn.ca/?oldRevs=365e150efda0&newRev=b5d62f5c733c&server=graphs.mozilla.org&submit=true

Are these kind of changes expected from this?
https://hg.mozilla.org/mozilla-central/rev/b5d62f5c733c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Comment 32

5 years ago
(In reply to Avi Halachmi (:avih) from comment #30)
> It also has unexpected (to me) 30% improvement on tsvgr_opacity on ubuntu32,
> but 100% regression on ubuntu 64.

It looks like these changes are within noise levels. If we can find non d3d9 platforms that are effected by this then we're doing something wrong.
(In reply to Benoit Girard (:BenWa) from comment #32)
> It looks like these changes are within noise levels. If we can find non d3d9
> platforms that are effected by this then we're doing something wrong.

If we look beyond the bi/tri modal behavior, it actually looks to me as if both regressed in in 20-100%. I don't think it's noise, but it might also be due to another patch (which I don't think I'm aware of).

http://graphs.mozilla.org/graph.html#tests=[[225,131,33],[225,131,35]]&sel=1377815674109,1378420474109&displayrange=7&datatype=running
(Assignee)

Updated

5 years ago
Blocks: 914195

Updated

5 years ago
Depends on: 919479

Updated

5 years ago
Depends on: 953253
(Reporter)

Updated

5 years ago
Depends on: 953334
You need to log in before you can comment on or make changes to this bug.