Closed Bug 957560 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(4 files)

New D3D textures have landed preffed off. Let's make them default instead of the deprecated ones.
try push with the prefs to get an idea of what's left to fix https://tbpl.mozilla.org/?tree=Try&rev=3c4182977a70
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
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+
(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
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)
(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 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+
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 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.
(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.
(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.
try push with the pref and talos: https://tbpl.mozilla.org/?tree=Try&rev=4f13d344c6b0
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.
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.
Depends on: 965250
try+talos push try: -b o -p win32 -u all -t all
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+
Blocks: 982413
Depends on: 973892
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+
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
Depends on: 990027
Fixed as part of bug 982413.
Status: NEW → RESOLVED
Closed: 6 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)
(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.