Enable new D3D textures by default when OMTC is on on Windows

RESOLVED FIXED in mozilla31

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla31
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

4 years ago
New D3D textures have landed preffed off. Let's make them default instead of the deprecated ones.
(Assignee)

Comment 1

4 years ago
try push with the prefs to get an idea of what's left to fix https://tbpl.mozilla.org/?tree=Try&rev=3c4182977a70
(Assignee)

Comment 2

4 years ago
oops, that last try push had talos enabled instead of the tests.
Here is a new one https://tbpl.mozilla.org/?tree=Try&rev=11419e6a63a4
(Assignee)

Comment 3

4 years ago
Created attachment 8357831 [details] [diff] [review]
Use the new textures as the default on windows.
Attachment #8357831 - Flags: review?(bas)

Comment 4

4 years ago
Does this mean we have fixed the locking issues with content? And d3d9 textures are all done?

(Looks like you've got a failing reftest, btw)
Attachment #8357831 - Flags: review?(bas) → review+
(Assignee)

Comment 5

4 years ago
(In reply to Nick Cameron [:nrc] from comment #4)
> Does this mean we have fixed the locking issues with content? And d3d9
> textures are all done?

New ContentClient is disabled on windows at the moment, we still have the locking issues there. d3d9 textures are all done.

> (Looks like you've got a failing reftest, btw)

Yeah, I'll fix that before preffing it on
(Assignee)

Comment 6

4 years ago
Created attachment 8359818 [details] [diff] [review]
clear the allocated D3D0//11 TextureClient if the flag says so

I am still unclear about the reftest: it doesn't always fail (3 out of 7). My best guess right now is that it's related to the fact that I forgot to clear the D3D clients when AllocateForSurface is called with the flag ALLOC_CLEAR_SURFACE.

This patch fixes it, we'll see in the next try push if it makes a difference on the reftest.
Attachment #8359818 - Flags: review?(ncameron)
(Assignee)

Comment 7

4 years ago
try push https://tbpl.mozilla.org/?tree=Try&rev=96092f1c6cee
(Assignee)

Comment 8

4 years ago
(In reply to Nicolas Silva [:nical] from comment #7)
> try push https://tbpl.mozilla.org/?tree=Try&rev=96092f1c6cee

the last try push had the wrong set of prefs: https://tbpl.mozilla.org/?tree=Try&rev=0a3591ad037f

Comment 9

4 years ago
Comment on attachment 8359818 [details] [diff] [review]
clear the allocated D3D0//11 TextureClient if the flag says so

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

::: gfx/layers/client/ContentClient.cpp
@@ -656,5 @@
> -+    // XXX - We should do something!
> -+    // This happened while resizing the window.
> -+    NS_WARNING("No DrawTarget for quadrant update!");
> -+    return;
> -+  }

Is this fixing a mistake from the previous patch or should it not be here?
Attachment #8359818 - Flags: review?(ncameron) → review+

Comment 10

4 years ago
Could you do a try push with talos too, just to see what happens to perf? (Not saying you should block landing if we do regress, but we do need to know).

Comment 11

4 years ago
(In reply to Nick Cameron [:nrc] from comment #10)
> Could you do a try push with talos too, just to see what happens to perf?
> (Not saying you should block landing if we do regress, but we do need to
> know).

In particular, I worry that we will be calling Flush (which is super-expensive) via Unlock (on d3d11) more than previously and that will cause regressions.
(Assignee)

Comment 12

4 years ago
(In reply to Nick Cameron [:nrc] from comment #9)
> ::: gfx/layers/client/ContentClient.cpp
> @@ -656,5 @@
> > -+    // XXX - We should do something!
> > -+    // This happened while resizing the window.
> > -+    NS_WARNING("No DrawTarget for quadrant update!");
> > -+    return;
> > -+  }
> 
> Is this fixing a mistake from the previous patch or should it not be here?

It should probably be it's own bug, I forgot to take it out of the patch. I ran into it after resizing the window continuously during several seconds. The BorrwoDrawTarget method does have paths where it returns nullptr so I don't think it could be caused to the changes in this patch (nor the previous one), It's just a bit hard to reproduce.
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a346b879068
Whiteboard: [leave open]
(Assignee)

Comment 14

4 years ago
(In reply to Nicolas Silva [:nical] from comment #12)
> > Is this fixing a mistake from the previous patch or should it not be here?
> 
> It should probably be it's own bug, I forgot to take it out of the patch. I
> ran into it after resizing the window continuously during several seconds.
> The BorrwoDrawTarget method does have paths where it returns nullptr so I
> don't think it could be caused to the changes in this patch (nor the
> previous one), It's just a bit hard to reproduce.

Ignore my previous comment, this is just a rebase that got messed up. It's not part of the final patch.
(Assignee)

Comment 15

4 years ago
try push with the pref and talos: https://tbpl.mozilla.org/?tree=Try&rev=4f13d344c6b0
(Assignee)

Comment 16

4 years ago
To compare, a talos push without the pref https://tbpl.mozilla.org/?tree=Try&rev=ef85560567ed

Since in both cases we don't use the new textures for thebes I don't expect much of a difference.
https://hg.mozilla.org/mozilla-central/rev/3a346b879068
Blocks: 904890
(Assignee)

Comment 18

4 years ago
The comparison between the two try pushes is here: http://compare-talos.mattn.ca/?oldRevs=ef85560567ed&newRev=4f13d344c6b0&server=graphs.mozilla.org&submit=true

I launched the jobs several times and saw some really crazy numbers at the beginning but it tends to even out as I run more jobs. I have no idea what to make of the numbers though (especially after the crazy variations I have seen).
I think we should just land this ASAP.

It's not currently enabled, and we don't want anyone spending time investigating performance issues with old-textures if it's about to be disabled and then deleted.
(Assignee)

Updated

4 years ago
Depends on: 965250
(Assignee)

Comment 20

4 years ago
try+talos push try: -b o -p win32 -u all -t all
(Assignee)

Comment 21

4 years ago
rrr: i meant https://tbpl.mozilla.org/?tree=Try&rev=ae83bf31da71
(Assignee)

Comment 22

4 years ago
talos. again. https://tbpl.mozilla.org/?tree=Try&rev=3a21cf277f98
(Assignee)

Comment 23

4 years ago
Created attachment 8373586 [details] [diff] [review]
Fix CaiorTextureClientD3D9 and DataTextureSource

This patch fixes the black rectangle problem with CairoTextureClientD3D9, which was caused by me forgetting to reset mNeedsClear, resulting on the texture being cleared twice, making the region that was not invalidated black.

Also fixes a problem with DataTextureSourceD3D9/11 where we would forget to pass the TextureFlags or pass the incorrectly when creating the texture.
Attachment #8373586 - Flags: review?(bas)
Attachment #8373586 - Flags: review?(bas) → review+
(Assignee)

Comment 24

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ffc31314d2d
https://hg.mozilla.org/mozilla-central/rev/1ffc31314d2d
Blocks: 982413
(Assignee)

Updated

4 years ago
Depends on: 973892
(Assignee)

Comment 26

4 years ago
Created attachment 8399181 [details] [diff] [review]
Fix the remaining new textures regressions on Windows

This makes new textures on par with the deprecated ones on windows (that is, they have the same failing tests...)
Attachment #8399181 - Flags: review?(matt.woodrow)
Attachment #8399181 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 27

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6969e4fb6f
(Assignee)

Comment 28

4 years ago
There was an unused variable on some platforms because of #ifdefed code causing build failurs (WError).
Fixed: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b596070fe54
https://hg.mozilla.org/mozilla-central/rev/ed6969e4fb6f
https://hg.mozilla.org/mozilla-central/rev/0b596070fe54

Updated

4 years ago
Depends on: 990027
(In reply to Carsten Book [:Tomcat] from comment #29)
> https://hg.mozilla.org/mozilla-central/rev/ed6969e4fb6f
> https://hg.mozilla.org/mozilla-central/rev/0b596070fe54

Backed out for causing bug 990027.
https://hg.mozilla.org/integration/mozilla-inbound/rev/864067766916
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/864067766916
(Assignee)

Comment 32

4 years ago
Fixed as part of bug 982413.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla31
Nical, is there anything QA needs to look out for here?
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 34

4 years ago
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #33)
> Nical, is there anything QA needs to look out for here?

No, thanks. This switched from an architecture to another on a configuration that wasn't shippable in either case (OMTC on Windows) so both had their bugs when we made the switch. The work to make it shippable is happening elsewhere.
Flags: needinfo?(nical.bugzilla)
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.