Direct3D 10 Layers Backend

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b7
x86
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

7 years ago
A Direct3D layers backend is used on Windows Vista+ where Direct3D 9+ hardware is available.
(Assignee)

Updated

7 years ago
Blocks: 546508
No longer blocks: 534425

Updated

7 years ago
Blocks: 534425
No longer blocks: 546508
(Assignee)

Updated

7 years ago
Blocks: 546508
No longer blocks: 534425
(Assignee)

Updated

7 years ago
Depends on: 550846
(Assignee)

Comment 1

7 years ago
Created attachment 431267 [details] [diff] [review]
First Layers D3D10 Backend

This is a very first D3D10 Layers backend. It needs some love still, reftests for one, aren't working.
Some comments:

1. RenderLayersVS should use the position naming sets instead of the color ones. It might be good to use xyz for the YUV samples in RenderYCbCrLayers. Also the use of YUV and YCbCr is inconsistent.

2. RenderYCbCrLayersPS should use LayerOpacity?

3. You should be able to use the mapped texture data directly in CopyToTarget instead of making a copy

4. When redefining an inherited virtual function, explicitly declare it virtual in the declaration of the derived class.

5. I don't like allocating a texture in ContainerLayerD3D10::RenderLayer. Can you convince me that we want to be doing this?
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Some comments:
> 
> 1. RenderLayersVS should use the position naming sets instead of the color
> ones. It might be good to use xyz for the YUV samples in RenderYCbCrLayers.
> Also the use of YUV and YCbCr is inconsistent.

Sure, yeah, xyz is a better idea :). I didn't change all the naming yet since roc changed it. Especially since I'm now focusing on OGL I won't change it around just yet.

> 
> 2. RenderYCbCrLayersPS should use LayerOpacity?
Yeah, I noticed that this morning when writing the OGL shaders and looking at the D3D ones.

> 
> 3. You should be able to use the mapped texture data directly in CopyToTarget
> instead of making a copy
CopyToTarget is not working yet, this is one of my attempts to fix it, but I might add it didn't help (as expected). I probably did something silly, haven't looked at it yet because of OGL and all.

> 
> 4. When redefining an inherited virtual function, explicitly declare it virtual
> in the declaration of the derived class.
> 
> 5. I don't like allocating a texture in ContainerLayerD3D10::RenderLayer. Can
> you convince me that we want to be doing this?

For group opacity everything needs to be rendered to a temporary surface, we could use complex cache systems for surfaces and such for that, but several graphics people have told me that an allocation like this does not require sync and should be cheap. I was planning to measure that further along the way though. I discussed it with roc and some others and we decided this was the most straight-forward way, and we can address it if it becomes a performance bottleneck.
Does this work with direct2d? I seem to recall you saying it did not.
(Assignee)

Comment 5

7 years ago
(In reply to comment #4)
> Does this work with direct2d? I seem to recall you saying it did not.

No, but making it work would simply involve ThebesSurfaces handing out D2D surfaces rather than ImageSurfaces and then avoiding the copy by grabbing their textures directly when drawing.
6. Unnecessary memset(&quadTransform, 0, sizeof(quadTransform));

7. HRESULT hr;
   hr = effectTechnique->GetPassByIndex(0)->Apply(0);
   Doesn't do anything with hr.

8. It would be nice if we could name the numbers that are parameters to CD3D10_TEXTURE2D_DESC. Perhaps define's like:
#define TEXTURE_MIPMAP_MULTISAMPLE 1
#define TEXTURE_ARRAY_SINGLE 1
(In reply to comment #3)
> (In reply to comment #2)
> > 5. I don't like allocating a texture in ContainerLayerD3D10::RenderLayer. Can
> > you convince me that we want to be doing this?
> 
> For group opacity everything needs to be rendered to a temporary surface, we
> could use complex cache systems for surfaces and such for that, but several
> graphics people have told me that an allocation like this does not require sync
> and should be cheap. I was planning to measure that further along the way
> though. I discussed it with roc and some others and we decided this was the
> most straight-forward way, and we can address it if it becomes a performance
> bottleneck.

I did some measurement of this.

CreateTexture2D/Release() seems to take about 0.10ms on my fast d3d10 machine regardless of the texture size.

For reference malloc/memset/free(720*480*4) takes about 0.46ms on the same machine.

0.1ms is approx 260000 instructions or .6% of our per frame budget assuming 60fps.

So, while I don't think this will kill us, I think that the any added complexity may well be worth the cost. We should probably try to design with this in mind.
(Assignee)

Comment 8

7 years ago
(In reply to comment #7)
> (In reply to comment #3)
> > (In reply to comment #2)
> 
> So, while I don't think this will kill us, I think that the any added
> complexity may well be worth the cost. We should probably try to design with
> this in mind.

We can probably work with a surface pool that hands out textures to containers. And keeps them around when containers need one again. But I still think we can wait with that until everything works first :-). I do agree we need to design with it in mind.
Looks cool!

> GetNextSibbling

Typo

+  if (mIsOpaqueContent) {

The first child of an opaque container can be treated as opaque, even if it's not really opaque itself. (This can happen when there's opaque content in other layers covering the transparent bits of this layer, for example.) You should probably use the opaqueness test from BasicLayers.

+    mSoftwareSurface = new gfxImageSurface(gfxIntSize(mInvalidatedRect.width,
+                                                      mInvalidatedRect.height),
+                                           gfxASurface::ImageFormatRGB24);
+  } else {
+    mSoftwareSurface = new gfxImageSurface(gfxIntSize(mInvalidatedRect.width,
+                                                      mInvalidatedRect.height),
+                                           gfxASurface::ImageFormatARGB32);
+  }

Why not GDI surfaces? Wouldn't that be a lot faster for text and themes?

You'll need to update this to trunk. You should just stub out CopyFrom for now, since it's going away.

+   * \param aClippingRegion Region to clip to. Setting an empty region

We use @param, @return etc.

It might be useful to add a comment or two explaining the threading issues here.
BTW, seems like CairoImageD3D10::SetData could be improved. If the source image is already a gfxImageSurface you don't need to copy. Or maybe you can wrap a D2D surface around your new D3D texture and use cairo to paint directly into it? Seems like that would be optimal.
(In reply to comment #9)
> Why not GDI surfaces? Wouldn't that be a lot faster for text and themes?

Or D2D when available, I guess.
9. UpdateSubresource is apparently a slow path, so we should try to avoid it and use STAGING buffers instead. (we don't necessarily need this to land but we should at least comment about it the code and have it mind)
(Assignee)

Comment 13

7 years ago
(In reply to comment #12)
> 9. UpdateSubresource is apparently a slow path, so we should try to avoid it
> and use STAGING buffers instead. (we don't necessarily need this to land but we
> should at least comment about it the code and have it mind)

Sure, using a staging buffer around which we wrap an imageSurface, which we then CopySubresourceRegion and release when we're done would in theory prevent some copy of memory, so this would probablt be a good idea!
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> (In reply to comment #12)
> Sure, using a staging buffer around which we wrap an imageSurface, which we
> then CopySubresourceRegion and release when we're done would in theory prevent
> some copy of memory, so this would probablt be a good idea!
Do note this means we'd have to use gfxImageSurfaces.. I don't think it's possible at the moment to wrap a windowsSurface around existing bits.. Otherwise we'd still be stuck with a copy from the gfxImageSurface to the staging buffer.
Can't we have an gfxWindowsSurface around a DIB?
(Assignee)

Comment 16

7 years ago
(In reply to comment #15)
> Can't we have an gfxWindowsSurface around a DIB?

I guess we could. For theme that would probably be useful.
We can have a gfxWindowsSurface around a DIBSection but not around arbitrary bits (like those from a staging buffer). Drawing text to image surfaces is probably not going to be a great time. Some one should check what chrome does here, but my guess is that they use DIBSections.
(Assignee)

Comment 18

7 years ago
Created attachment 478336 [details] [diff] [review]
Part 1: Detect presence of D3D10 headers
Attachment #431267 - Attachment is obsolete: true
Attachment #478336 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 19

7 years ago
Created attachment 478338 [details] [diff] [review]
Part 2: Set #defines for D3D10 layers
Attachment #478338 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 20

7 years ago
Created attachment 478341 [details] [diff] [review]
Part 3: Add D3D10 layers code

This is the bulk of the work. This code passes all reftests and mochitests.
(Assignee)

Comment 21

7 years ago
Created attachment 478342 [details] [diff] [review]
Part 4: Build D3D10 layers code
(Assignee)

Comment 22

7 years ago
Created attachment 478343 [details] [diff] [review]
Part 5: Try using D3D10 layers by default
Attachment #478343 - Flags: review?(roc)
It was my understanding that Direct3D 9 would be handling Layers Compositing across all Windows systems.

Is there a performance gain by using DX10 for Vista and Seven instead?
Comment on attachment 478343 [details] [diff] [review]
Part 5: Try using D3D10 layers by default

How much does this add to startup time?
Attachment #478343 - Flags: review?(roc) → review+
(In reply to comment #23)
> It was my understanding that Direct3D 9 would be handling Layers Compositing
> across all Windows systems.
> 
> Is there a performance gain by using DX10 for Vista and Seven instead?

Yes, a very significant one, because we no longer have to busywait for DirectX 9 and DirectX 10 (D2D) to synchronize.
(Assignee)

Comment 26

7 years ago
(In reply to comment #24)
> Comment on attachment 478343 [details] [diff] [review]
> Part 5: Try using D3D10 layers by default
> 
> How much does this add to startup time?

It does not add, it also prevents us from having to load D3D9. Cutting startup time by about 40ms.
Ah, of course. Nice.
(Assignee)

Updated

7 years ago
Depends on: 597950
(Assignee)

Comment 28

7 years ago
Some changed test results from Win 7 opt Talos:

Ts: +/- 475 to +/- 440
Tp4: +/- 325 to +/- 305
Tp4_pbytes: +/- 155MB to +/- 130 MB
Tp4_memset: +/- 175MB to +/- 155 MB

Sadly there was no Txul run on my build but I expect that's improved as well. In any case these are promising results.
(Assignee)

Updated

7 years ago
Attachment #478341 - Flags: review?(jmuizelaar)
(Assignee)

Updated

7 years ago
Attachment #478342 - Flags: review?(jmuizelaar)
Comment on attachment 478342 [details] [diff] [review]
Part 4: Build D3D10 layers code

As expected, this duplicates a lot of the d3d9 backend. I'm ok with this for now, but we should at least have some infrastructure for sharing code between d3d9 and d3d10. So please try and share at least one piece of code between them.
(Assignee)

Comment 30

7 years ago
Created attachment 479911 [details] [diff] [review]
Part 3: Add D3D10 layers code v2

Fix a minor bug where we were not properly checking our opacity forwarding conditions.
Attachment #478341 - Attachment is obsolete: true
Attachment #479911 - Flags: review?(jmuizelaar)
Attachment #478341 - Flags: review?(jmuizelaar)
(Assignee)

Comment 31

7 years ago
Created attachment 479917 [details] [diff] [review]
Part 5: Allow turning on D3D10 layers through a pref.
Attachment #478343 - Attachment is obsolete: true
Attachment #479917 - Flags: review?(vladimir)
(Assignee)

Comment 32

7 years ago
Created attachment 479918 [details] [diff] [review]
Part 4: Build D3D10 layers code v2

Move LayersBackend enumeration update to correct patch.
Attachment #479918 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

7 years ago
Attachment #478342 - Attachment is obsolete: true
Attachment #478342 - Flags: review?(jmuizelaar)
(Assignee)

Updated

7 years ago
Attachment #479918 - Flags: review?(ted.mielczarek) → review?(jmuizelaar)
(Assignee)

Comment 33

7 years ago
Created attachment 479919 [details] [diff] [review]
Part 5: Allow turning on D3D10 layers through a pref. v2

Move LayersBackend enumeration update to the correct patch.
Attachment #479919 - Flags: review?(vladimir)
Attachment #479919 - Flags: review?(vladimir) → review+
Attachment #479917 - Attachment is obsolete: true
Attachment #479917 - Flags: review?(vladimir)
Comment on attachment 478336 [details] [diff] [review]
Part 1: Detect presence of D3D10 headers

stealing review
Attachment #478336 - Flags: review?(ted.mielczarek) → review+
Attachment #478338 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 479911 [details] [diff] [review]
Part 3: Add D3D10 layers code v2

Let's get this in, given that it's pref'd off
Attachment #479911 - Flags: review+
Attachment #479918 - Flags: review?(jmuizelaar) → review+
a=me for b7 (pref'd off)
(Assignee)

Comment 37

7 years ago
http://hg.mozilla.org/mozilla-central/rev/53ef76ec6c53
http://hg.mozilla.org/mozilla-central/rev/64d63f171108
http://hg.mozilla.org/mozilla-central/rev/c7d04e4989c2
http://hg.mozilla.org/mozilla-central/rev/2f24116f41a3
http://hg.mozilla.org/mozilla-central/rev/8dcc5e960d5c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
I had experienced a weird problem about D3D10 layers.

In biolab game, the whole game area seems to be blurred and washed out with D3D10 layers. http://www.phoboslab.org/biolab/

http://img704.imageshack.us/img704/2184/biolabwattafak.png

Anyone can confirm?

Comment 39

7 years ago
Yes, I can confirm.
Depends on: 601273
I got an error building. It might be that D3D10 layer is activated whenever d3d10.h is found, despite Direct2D not being available.

Solved it temporarily by commenting the line "AC_CHECK_HEADER(d3d10.h, MOZ_ENABLE_D3D10_LAYER=1)" in configure.in

Compiling on Windows server 2003, MS platform SDK for Windows Server 2003 and VS 2008 express. Forced target Windows version to 502.

Tell me if I should open a new bug with the build error log attached.
(In reply to comment #38)
> I had experienced a weird problem about D3D10 layers.
> 
> In biolab game, the whole game area seems to be blurred and washed out with
> D3D10 layers. http://www.phoboslab.org/biolab/
> 
> http://img704.imageshack.us/img704/2184/biolabwattafak.png
> 
> Anyone can confirm?

(In reply to comment #39)
> Yes, I can confirm.


Bas, any feedback on this? Should i file a separate bug? It seems the problem applies to all canvas content.
(Assignee)

Comment 42

7 years ago
> Bas, any feedback on this? Should i file a separate bug? It seems the problem
> applies to all canvas content.

Sorry! I somehow missed these comment, yes please file, I'll work on a fix.

(In reply to comment #40)
> I got an error building. It might be that D3D10 layer is activated whenever
> d3d10.h is found, despite Direct2D not being available.
> 
> Solved it temporarily by commenting the line "AC_CHECK_HEADER(d3d10.h,
> MOZ_ENABLE_D3D10_LAYER=1)" in configure.in
> 
> Compiling on Windows server 2003, MS platform SDK for Windows Server 2003 and
> VS 2008 express. Forced target Windows version to 502.
> 
> Tell me if I should open a new bug with the build error log attached.

This is a known problem, there's a patch for it that's approved for landing post beta 7. This is caused by the outdated Platform SDK I believe. You should use the newer Platform SDKs I think.
Thanks! Filed Bug 602200
Blocks: 603532
(In reply to comment #42)

> > Forced target Windows version to 502.
> > 
> > Tell me if I should open a new bug with the build error log attached.

I opened bug 603532, which may be what you got too, is it not?

> This is a known problem, there's a patch for it that's approved for landing
> post beta 7. This is caused by the outdated Platform SDK I believe. You should
> use the newer Platform SDKs I think.

I don't know where that patch is.
Fwiw, SDKs should be up-to-date on Try Server.
Target Milestone: --- → mozilla2.0b7
Version: unspecified → Trunk
(In reply to comment #44)
> I opened bug 603532, which may be what you got too, is it not?

Right, the same error. CC'ing myself, thanks.

> I don't know where that patch is.
> Fwiw, SDKs should be up-to-date on Try Server.

I'm using this one: Microsoft Platform SDK for Windows Server 2003 R2.

Not sure if it's outdated, last time I checked it was supported in MDN build instructions.
Blocks: 601358
No longer blocks: 603532
Comment on attachment 479911 [details] [diff] [review]
Part 3: Add D3D10 layers code v2

Please add a script for generating the shader headers like d3d9 backend has.
Comment on attachment 479911 [details] [diff] [review]
Part 3: Add D3D10 layers code v2

Some comments:

1. When redefining an inherited virtual function, explicitly declare it virtual in the declaration of the derived class. (ContainerLayerD3D10)

2. Why does PaintToTarget() use OPERATOR_SOURCE?

3. Perhaps PaintToTarget() should Unmap before destroying?

4. The d3d9 backend calls Present for each rect in the clipping region
   The d3d10 backend doesn't. Why?
Comment on attachment 479911 [details] [diff] [review]
Part 3: Add D3D10 layers code v2

1. Why do the d3d10 and d3d9 containerlayers have different conditions for using intermediate surfaces? (ShouldUseIntermediate())

2. What's the story for device loss going to be?

Nothing else really pops out. We can always improve it iteratively.
Attachment #479911 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 49

7 years ago
(In reply to comment #47)
> Comment on attachment 479911 [details] [diff] [review]
> Part 3: Add D3D10 layers code v2
> 
> Some comments:
> 
> 1. When redefining an inherited virtual function, explicitly declare it virtual
> in the declaration of the derived class. (ContainerLayerD3D10)

Whoops, sorry.

> 
> 2. Why does PaintToTarget() use OPERATOR_SOURCE?

I assumed (as a lot of our code seems to) SOURCE is fastest for the destination surface type in most cases.

> 3. Perhaps PaintToTarget() should Unmap before destroying?

Technically it probably should. I don't think it matters, but I will fix.

> 
> 4. The d3d9 backend calls Present for each rect in the clipping region
>    The d3d10 backend doesn't. Why?

It's more expensive to do. The ability was not implemented for D3D10 so we simply can't unless we manually create a texture and copy rects internally to the backbuffer and then present. This is better since we've got the whole layer tree anyway since retained layers landed. It seems to work well.

(In reply to comment #48)
> Comment on attachment 479911 [details] [diff] [review]
> Part 3: Add D3D10 layers code v2
> 
> 1. Why do the d3d10 and d3d9 containerlayers have different conditions for
> using intermediate surfaces? (ShouldUseIntermediate())

D3D10 has the code to propagate transforms/transparency to lower layers in the tree. Allowing it to prevent the intermediate in a lot of cases. There's a patch to do something similar in bug 584494.

> 2. What's the story for device loss going to be?

See Bug 604271. We handle it outside the actual layer manager for now.

> 
> Nothing else really pops out. We can always improve it iteratively.
(Assignee)

Comment 50

7 years ago
(In reply to comment #46)
> Comment on attachment 479911 [details] [diff] [review]
> Part 3: Add D3D10 layers code v2
> 
> Please add a script for generating the shader headers like d3d9 backend has.

For effects it's a single command (the entire effect file compiles to a single header), I can add a script for that but it would more or less be an alias to the command?
(In reply to comment #50)
> (In reply to comment #46)
> > Comment on attachment 479911 [details] [diff] [review] [details]
> > Part 3: Add D3D10 layers code v2
> > 
> > Please add a script for generating the shader headers like d3d9 backend has.
> 
> For effects it's a single command (the entire effect file compiles to a single
> header), I can add a script for that but it would more or less be an alias to
> the command?

I think that's fine. It can serve as documentation.
(In reply to comment #49)
> (In reply to comment #47)

> > 
> > 2. Why does PaintToTarget() use OPERATOR_SOURCE?
> 
> I assumed (as a lot of our code seems to) SOURCE is fastest for the destination
> surface type in most cases.

The other backends all do OPERATOR_OVER. I'd switch so that it's the same everywhere and you can add a comment about switching to SOURCE if necessary.
(Assignee)

Comment 53

7 years ago
http://hg.mozilla.org/mozilla-central/rev/9be28f94a4a0

Comment 54

7 years ago
The IE9 fish demo is running slower now.  Actually I noticed the drop in speed with an hourly earlier in the day.  However the current hourly, which includes your stuff Bas, drops the speed even more.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101019 Firefox/4.0b8pre - Build ID: 20101019132546
(Assignee)

Comment 55

7 years ago
(In reply to comment #54)
> The IE9 fish demo is running slower now.  Actually I noticed the drop in speed
> with an hourly earlier in the day.  However the current hourly, which includes
> your stuff Bas, drops the speed even more.
> 
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101019
> Firefox/4.0b8pre - Build ID: 20101019132546

Are you talking it was faster before with D3D10? Or when you were using D3D9.

Comment 56

7 years ago
Bas:

Got my facts a little wrong.  I noticed sometime in the past couple of days that the Fish demo ran a little slower for me.  I'm on a high end machine (i7 Core Quad @ 4.21Ghz, 12GB PC3 RAM, ATI HD3870).  Up to 1,000 fish ran at 60fps.  Now I am getting about ~40fps @ 1000 and ~55fps @ 500. I'm using DX10 (I turned this on awhile ago) and it is definitely faster then DX9. 

So at this point all I can say is 'something' lowered my Fish fps. As a side note there was a fix recently that drastically sped up the large IE flying logo demo.  Even though it is still as fast I'm wondering if this could have affected the Fish demo.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101019 Firefox/4.0b8pre - Build ID: 20101019142759
(Assignee)

Comment 57

7 years ago
(In reply to comment #56)
> Bas:
> 
> Got my facts a little wrong.  I noticed sometime in the past couple of days
> that the Fish demo ran a little slower for me.  I'm on a high end machine (i7
> Core Quad @ 4.21Ghz, 12GB PC3 RAM, ATI HD3870).  Up to 1,000 fish ran at 60fps.
>  Now I am getting about ~40fps @ 1000 and ~55fps @ 500. I'm using DX10 (I
> turned this on awhile ago) and it is definitely faster then DX9. 
> 
> So at this point all I can say is 'something' lowered my Fish fps. As a side
> note there was a fix recently that drastically sped up the large IE flying logo
> demo.  Even though it is still as fast I'm wondering if this could have
> affected the Fish demo.
> 
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101019
> Firefox/4.0b8pre - Build ID: 20101019142759

Okay, please open a separate bug on this so we can try and find a regression window. Thanks a lot for the info!

Updated

7 years ago
Blocks: 605808
You need to log in before you can comment on or make changes to this bug.