Closed Bug 907544 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect)

All
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jrmuizel, Assigned: BenWa)

References

Details

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

Attachments

(1 file, 7 obsolete files)

This will avoid having to do an allocation and two copies
Blocks: 907546
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
Status: ASSIGNED → NEW
Attached patch Buggy WIP (obsolete) — Splinter Review
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.
The above two comment should of been made under my account.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #797285 - Attachment is obsolete: true
Attachment #797996 - Flags: review?(jmuizelaar)
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.
These look great. I noticed a small problem causing the scrollbars not rendering. I'll see if I can find why.
(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.
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.
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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attached file M-Other failure (obsolete) —
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
Attached patch patch v3 (obsolete) — Splinter Review
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)
Attached patch patch (obsolete) — Splinter Review
Minor tweaks
Attachment #799573 - Attachment is obsolete: true
Attachment #799573 - Flags: review?(jmuizelaar)
Attachment #799593 - Flags: review?(jmuizelaar)
Attachment #799593 - Flags: review?(jmuizelaar) → review+
OS: Mac OS X → Windows XP
Hardware: x86 → All
Waiting on try and still need to fix indention and restore GetDC.
Attached patch patch v4Splinter Review
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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
Blocks: 914195
Depends on: 919479
Depends on: 953253
Depends on: 953334
You need to log in before you can comment on or make changes to this bug.