Last Comment Bug 750898 - Review and land DirectX 10 support for Windows 8 Metro builds
: Review and land DirectX 10 support for Windows 8 Metro builds
Status: RESOLVED FIXED
completed-elm
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: Trunk
: x86_64 Windows 8.1
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
: 737837 (view as bug list)
Depends on: 774285 770694
Blocks: elm-merge 756557
  Show dependency treegraph
 
Reported: 2012-05-01 14:52 PDT by Jim Mathies [:jimm]
Modified: 2014-07-24 11:06 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1. (18.15 KB, patch)
2012-05-10 12:25 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v2. (19.04 KB, patch)
2012-05-10 12:54 PDT, Brian R. Bondy [:bbondy]
bas: review-
Details | Diff | Splinter Review
Patch v3. (12.72 KB, patch)
2012-05-18 12:22 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v4. (11.10 KB, patch)
2012-05-21 15:12 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v5 (11.14 KB, patch)
2012-05-21 17:39 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
GFX for CoreWindow Patch v1 (10.73 KB, patch)
2012-06-28 09:01 PDT, Brian R. Bondy [:bbondy]
bas: review+
Details | Diff | Splinter Review

Description Jim Mathies [:jimm] 2012-05-01 14:52:14 PDT

    
Comment 1 Jim Mathies [:jimm] 2012-05-02 15:29:13 PDT
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] 2012-05-02 19:28:01 PDT
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] 2012-05-10 12:25:38 PDT
Created attachment 622847 [details] [diff] [review]
Patch v1.

We'd like to land this patch on m-c in preparation for the Metro merge.
Comment 4 Brian R. Bondy [:bbondy] 2012-05-10 12:54:22 PDT
Created attachment 622866 [details] [diff] [review]
Patch v2.

Added in layout/media/symbols.def.in
Comment 5 Bas Schouten (:bas.schouten) 2012-05-10 16:56:25 PDT
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] 2012-05-11 06:13:41 PDT
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] 2012-05-16 19:23:07 PDT
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) 2012-05-16 19:25:43 PDT
If you're using D2D, you're using the D3D10 layer manager. One implies the other these days.
Comment 9 Brian R. Bondy [:bbondy] 2012-05-16 19:33:15 PDT
OK thanks, so I'll remove the cairo code I added that we were using before.
Comment 10 Brian R. Bondy [:bbondy] 2012-05-18 12:22:38 PDT
Created attachment 625191 [details] [diff] [review]
Patch v3.

Using layer manager, removed cairo code
Comment 11 Brian R. Bondy [:bbondy] 2012-05-18 12:23:27 PDT
I'll be doing a follow up bug for DX9 support.
Comment 12 Bas Schouten (:bas.schouten) 2012-05-18 12:55:20 PDT
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] 2012-05-21 15:12:53 PDT
Created attachment 625781 [details] [diff] [review]
Patch v4.

Implemented review comments.
Comment 14 Bas Schouten (:bas.schouten) 2012-05-21 16:20:35 PDT
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] 2012-05-21 17:23:48 PDT
> 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] 2012-05-21 17:39:44 PDT
Created attachment 625832 [details] [diff] [review]
Patch v5
Comment 17 Bas Schouten (:bas.schouten) 2012-05-21 20:30:18 PDT
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] 2012-05-22 04:36:48 PDT
> Doesn't look like this part got fixed?

Please see Comment 15.
Comment 19 Brian R. Bondy [:bbondy] 2012-05-24 07:19:10 PDT
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 20 Brian R. Bondy [:bbondy] 2012-06-28 08:41:08 PDT
*** Bug 737837 has been marked as a duplicate of this bug. ***
Comment 21 Brian R. Bondy [:bbondy] 2012-06-28 09:01:02 PDT
Created attachment 637536 [details] [diff] [review]
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] 2012-06-28 09:01:28 PDT
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) 2012-06-28 09:14:50 PDT
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] 2012-07-04 16:54:26 PDT
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] 2012-07-09 07:28:55 PDT
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] 2012-07-16 06:19:53 PDT
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) 2012-07-16 07:41:17 PDT
> > > +} } }
> > 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) 2012-07-16 07:47:06 PDT
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] 2012-07-16 07:59:38 PDT
> 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 31 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:06:14 PDT
https://hg.mozilla.org/mozilla-central/rev/aa572892dca1
Comment 32 Jim Mathies [:jimm] 2012-07-24 03:27:02 PDT
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] 2012-07-24 04:49:03 PDT
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] 2012-07-24 05:05:00 PDT
ok sounds good. FWIW I cancelled my mc merge due to issues with bug 626472.

Note You need to log in before you can comment on or make changes to this bug.