750898 - Review and land DirectX 10 support for Windows 8 Metro builds

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Jim Mathies [:jimm]]

Not sure if we added entry points to cairo, but if we did, it's moving to a new library - bug 751273.

Comment 2

[Brian R. Bondy [:bbondy]]

We added a bunch of code to cairo but it doesn't look like it'll conflict.  I'll do a pull soon on elm to be sure.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

We'd like to land this patch on m-c in preparation for the Metro merge.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Added in layout/media/symbols.def.in

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 622866 [details] [diff] [review]
Patch v2.

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

It seems to me that this is all in the wrong location. This will essentially put us on BasicLayers which has a lot of drawbacks. What we want to do is rather than connect anything to cairo, is completely bypass BasicLayers here and solely support D3D10 layers. LayerManagerD3D10 should get methods to render to an existing SwapChain or surface, working from the code already in there that shouldn't be hard, please let me know if there are any questions!

Comment 6

[Brian R. Bondy [:bbondy]]

Thanks for the feedback, we were mostly concerned with getting a working prototype up initially.  I'll take another pass at this after a couple of other tasks I have to do first.

Comment 7

[Brian R. Bondy [:bbondy]]

I think we can assume that one of MOZ_ENABLE_D3D10_LAYER or MOZ_ENABLE_D3D9_LAYER is defined for the Metro browser right?

I'm trying to determine if I should be removing the cairo code for the equivalent LayerManagerD3D10::Initialize code, or if I should keep it as a fallback with Basic Layers.

Comment 8

[Joe Drew (not getting mail)]

If you're using D2D, you're using the D3D10 layer manager. One implies the other these days.

Comment 9

[Brian R. Bondy [:bbondy]]

OK thanks, so I'll remove the cairo code I added that we were using before.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Using layer manager, removed cairo code

Comment 11

[Brian R. Bondy [:bbondy]]

I'll be doing a follow up bug for DX9 support.

Comment 12

[Bas Schouten (:bas.schouten)]

Comment on attachment 625191 [details] [diff] [review]
Patch v3.

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

::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +281,5 @@
> +                                                 &swapChain1);
> +    if (FAILED(hr)) {
> +        return false;
> +    }
> +    mSwapChain = reinterpret_cast<IDXGISwapChain*>(swapChain1);

No reinterpret cast is needed, IDXGISwapChain1 inherits from IDXGISwapChain.

You're also leaking the SwapChain here as the assignment will addref, but nothing will release the initial ref. What probably is the right thing to do here is just:

nsRefPtr<IDXGISwapChain1> swapChain1;

hr = dxgiFactory->CreateSwapChainForComposition(dxgiDevice, &swapDesk, nullptr, getter_AddRefs(swapChain1));

if (FAILED(hr)) {
  return false;
}

mSwapChain = swapChain1;

::: widget/windows/winrt/FrameworkViewGfx.cpp
@@ +56,5 @@
> +
> +  if (IsRenderMode(gfxWindowsPlatform::RENDER_GDI) ||
> +      IsRenderMode(gfxWindowsPlatform::RENDER_IMAGE_STRETCH32) ||
> +      IsRenderMode(gfxWindowsPlatform::RENDER_IMAGE_STRETCH24)) {
> +    NS_WARNING("Unsupported render mode, can't draw. Needs to be D2D.");

D3D9 layers actually requires GDI so if we want to support D3D9 at all we'll need GDI.

@@ +80,5 @@
> +  UpdateForWindowSizeChange();
> +
> +  bool result = true;
> +  switch (mWidget->GetLayerManager()->GetBackendType()) {
> +#ifdef MOZ_ENABLE_D3D9_LAYER

The code to initialize this is commented out. Do we actually want this here?

@@ +131,5 @@
> +    new mozilla::layers::LayerManagerD3D10(mWidget);
> +  if (layerManager->Initialize(true)) {
> +    mWidget->mLayerManager = layerManager;
> +    IDXGISwapChain1 *swapChain =
> +      reinterpret_cast<IDXGISwapChain1*>(layerManager->SwapChain());

We should QI here like we're supposed to do in my opinion.

@@ +137,5 @@
> +    Microsoft::WRL::ComPtr<ISwapChainBackgroundPanelNative> dxRootPanelAsNative;
> +    // set the swap chain on the SwapChainBackgroundPanel
> +    reinterpret_cast<IUnknown*>(mMainPage->GetRootElement())->
> +      QueryInterface(__uuidof(ISwapChainBackgroundPanelNative),
> +      (void**)&dxRootPanelAsNative);

Ew, there really isn't some more typesafe way of doing this?

@@ +141,5 @@
> +      (void**)&dxRootPanelAsNative);
> +    dxRootPanelAsNative->SetSwapChain(swapChain);
> +  }
> +
> +  // Let's try DX9 if DX10 didn't work

Hrm, we may want to remove this?

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

Implemented review comments.

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 625781 [details] [diff] [review]
Patch v4.

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

::: widget/windows/winrt/FrameworkViewGfx.cpp
@@ +102,5 @@
> +    new mozilla::layers::LayerManagerD3D10(mWidget);
> +  if (layerManager->Initialize(true)) {
> +    mWidget->mLayerManager = layerManager;
> +    IDXGISwapChain1 *swapChain1;
> +    layerManager->SwapChain()->QueryInterface(IID_PPV_ARGS(&swapChain1));

You're leaking swapChain1 here. QI will add a reference. I believe code similar to your DXGIFactory code elsewhere (using nsRefPtr) will work.

@@ +107,5 @@
> +
> +    // set the swap chain on the SwapChainBackgroundPanel
> +    Microsoft::WRL::ComPtr<ISwapChainBackgroundPanelNative> swapChainBackgroundPanelNative;
> +    IInspectable* panelInspectable =
> +      reinterpret_cast<IInspectable*>(mMainPage->GetRootElement());

static_cast<IUnknown*>

@@ +108,5 @@
> +    // set the swap chain on the SwapChainBackgroundPanel
> +    Microsoft::WRL::ComPtr<ISwapChainBackgroundPanelNative> swapChainBackgroundPanelNative;
> +    IInspectable* panelInspectable =
> +      reinterpret_cast<IInspectable*>(mMainPage->GetRootElement());
> +    panelInspectable->QueryInterface(IID_PPV_ARGS(&swapChainBackgroundPanelNative));

Then QI here.

Comment 15

[Brian R. Bondy [:bbondy]]

> You're leaking swapChain1 here

Oops thanks, will fix.

> @@ +107,5 @@
> > +
> > +    // set the swap chain on the SwapChainBackgroundPanel
> > +    Microsoft::WRL::ComPtr<ISwapChainBackgroundPanelNative> swapChainBackgroundPanelNative;
> > +    IInspectable* panelInspectable =
> > +      reinterpret_cast<IInspectable*>(mMainPage->GetRootElement());
> 
> static_cast<IUnknown*>

The reason this is ugly is because we're converting from a C++/CX type to a WRL type.
  
All WRL objects inherit not only from IUnknown, but also IInspectable, so that should be sufficient.
static_cast will not work in that case.

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5

Comment 17

[Bas Schouten (:bas.schouten)]

Comment on attachment 625832 [details] [diff] [review]
Patch v5

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

::: widget/windows/winrt/FrameworkViewGfx.cpp
@@ +107,5 @@
> +
> +    // set the swap chain on the SwapChainBackgroundPanel
> +    Microsoft::WRL::ComPtr<ISwapChainBackgroundPanelNative> swapChainBackgroundPanelNative;
> +    IInspectable* panelInspectable =
> +      reinterpret_cast<IInspectable*>(mMainPage->GetRootElement());

Doesn't look like this part got fixed?

Comment 18

[Brian R. Bondy [:bbondy]]

> Doesn't look like this part got fixed?

Please see Comment 15.

Comment 19

[Brian R. Bondy [:bbondy]]

Comment on attachment 625832 [details] [diff] [review]
Patch v5

This is going to change to code that uses a winrt core window instead because MS is no longer supporting XAML interop.  Cancelling review until I update this code.

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: GFX for CoreWindow Patch v1

Finished this a while ago when DirectX / XAML interop was complete, but wanted to get it reviewed before we get on m-c.

Instead of setting the swap chain directly on the XAML element we had to remove XAML interop completely.  The new way just uses CoreWindow.

Comment 22

[Brian R. Bondy [:bbondy]]

Once this is r+'ed it won't actually land on m-c until the rest of the metro browser stuff does by the way.

Comment 23

[Bas Schouten (:bas.schouten)]

Comment on attachment 637536 [details] [diff] [review]
GFX for CoreWindow Patch v1

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

::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +290,5 @@
> +    }
> +
> +    // We need this because we don't want DXGI to respond to Alt+Enter.
> +    dxgiFactory->MakeWindowAssociation(swapDesc.OutputWindow, DXGI_MWA_NO_WINDOW_CHANGES);
> +#ifdef MOZ_METRO

nit: Since we don't need anything from the local scope later on, I prefer if you just scope this block unconditionally and avoid this #ifdef. i.e.

#ifdef foo
if (true) {
} else
#endif
{
  bla();
}

::: widget/windows/winrt/FrameworkViewGfx.cpp
@@ +79,5 @@
> +  paintEvent.willSendDidPaint = true;
> +  paintEvent.didSendWillPaint = true;
> +
> +  UpdateForWindowSizeChange();
> +  gfxWindowsPlatform::GetPlatform()->UpdateRenderMode();

It's nice that we update the render mode, but what do we do when D2D has gone away after this?

@@ +110,5 @@
> +    mWidget->mLayerManager = layerManager;
> +  }
> +}
> +
> +} } }

nit: All other files here add new lines. Let's do that here too.

Comment 24

[Brian R. Bondy [:bbondy]]

Sorry for the delay.

Updated the first nit on elm.  

> > +  UpdateForWindowSizeChange();
> > +  gfxWindowsPlatform::GetPlatform()->UpdateRenderMode();

> It's nice that we update the render mode, but what do we do when D2D 
> has gone away after this?

What are you suggesting I add/change here?

> > +} } }
> nit: All other files here add new lines. Let's do that here too.

This is the current convention in the winrt component across the other files so I'm going to leave that one as is.

Comment 25

[Brian R. Bondy [:bbondy]]

By the way I mentioned originally this was for elm only, but I'd like to land the stuff under /gfx onto m-c directly since I see no reason not to have it there yet. 

I'll wait until I have more clarification on Comment 24 and an OK from you to land it on m-c though.

Comment 26

[Brian R. Bondy [:bbondy]]

Comment on attachment 637536 [details] [diff] [review]
GFX for CoreWindow Patch v1

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

Re-requesting r? even know it was r+ based on Comment 24 needed feedback.

Comment 27

[Bas Schouten (:bas.schouten)]

> > > +} } }
> > nit: All other files here add new lines. Let's do that here too.
> 
> This is the current convention in the winrt component across the other files
> so I'm going to leave that one as is.

How did that happen? :) It seems like a bad idea to randomly use different code conventions in other components. Not that it's not happened elsewhere but in my mind it's a huge pain in our codebase already.

Comment 28

[Bas Schouten (:bas.schouten)]

Comment on attachment 637536 [details] [diff] [review]
GFX for CoreWindow Patch v1

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

::: widget/windows/winrt/FrameworkViewGfx.cpp
@@ +79,5 @@
> +  paintEvent.willSendDidPaint = true;
> +  paintEvent.didSendWillPaint = true;
> +
> +  UpdateForWindowSizeChange();
> +  gfxWindowsPlatform::GetPlatform()->UpdateRenderMode();

So, I'm not 100% sure what to do here, but basically if we detect here that Direct2D has gone away we need to deal with it -somehow-.

Comment 29

[Brian R. Bondy [:bbondy]]

> How did that happen? :)

Someone else started the convention and I just followed.  I'm not opposed to changing all files in widget/windows/winrt there though but I don't think it's a big deal.  By the way there are some other different conventions in winrt too like line length and using C++/CX.

> Re detect if Direct2D has gone away. 

I'll post a followup bug to do that since that's inside the widget/winrt code, and I can land the gfx stuff on m-c which doesn't include it.

Comment 30

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/aa572892dca1

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa572892dca1

Comment 32

[Jim Mathies [:jimm]]

While doing a merge today, there waa some addition code in LayerManagerD3D10::CreateDrawTarget:

  CD3D10_TEXTURE2D_DESC desc(DXGI_FORMAT_B8G8R8A8_UNORM, aSize.width, aSize.height, 1, 1);
  desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE;
  desc.MiscFlags = D3D10_RESOURCE_MISC_GDI_COMPATIBLE; <--

I left this on elm. Is it needed on mc? If so we need to land it.

Comment 33

[Brian R. Bondy [:bbondy]]

That's not part of this patch so it probably was changed unintentionally at some other time.  The line should be removed from elm which would sync it to m-c.  

I removed it and pushed it to elm.
By the way there's another patch that hasn't landed on m-c yet with a small gfx fix, which fixes snap state. We'll have to land that in that patch.  I'll mark it later.

Comment 34

[Jim Mathies [:jimm]]

ok sounds good. FWIW I cancelled my mc merge due to issues with bug 626472.