Closed Bug 750898 Opened 8 years ago Closed 7 years ago

Review and land DirectX 10 support for Windows 8 Metro builds

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jimm, Assigned: bbondy)

References

Details

(Whiteboard: completed-elm)

Attachments

(1 file, 5 obsolete files)

No description provided.
Not sure if we added entry points to cairo, but if we did, it's moving to a new library - bug 751273.
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.
Attached patch Patch v1. (obsolete) — Splinter Review
We'd like to land this patch on m-c in preparation for the Metro merge.
Assignee: nobody → netzen
Attachment #622847 - Flags: review?(bas.schouten)
Attached patch Patch v2. (obsolete) — Splinter Review
Added in layout/media/symbols.def.in
Attachment #622847 - Attachment is obsolete: true
Attachment #622847 - Flags: review?(bas.schouten)
Attachment #622866 - Flags: review?(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!
Attachment #622866 - Flags: review?(bas.schouten) → review-
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.
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.
If you're using D2D, you're using the D3D10 layer manager. One implies the other these days.
OK thanks, so I'll remove the cairo code I added that we were using before.
Attached patch Patch v3. (obsolete) — Splinter Review
Using layer manager, removed cairo code
Attachment #622866 - Attachment is obsolete: true
Attachment #625191 - Flags: review?(bas.schouten)
I'll be doing a follow up bug for DX9 support.
Summary: Review and land gfx changes for Fx metro on mc → Review and land DirectX 10 support for Windows 8 Metro builds
Blocks: 756557
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?
Attached patch Patch v4. (obsolete) — Splinter Review
Implemented review comments.
Attachment #625191 - Attachment is obsolete: true
Attachment #625191 - Flags: review?(bas.schouten)
Attachment #625781 - Flags: review?(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.
> 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.
Attached patch Patch v5 (obsolete) — Splinter Review
Attachment #625781 - Attachment is obsolete: true
Attachment #625781 - Flags: review?(bas.schouten)
Attachment #625832 - Flags: review?(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?
> Doesn't look like this part got fixed?

Please see Comment 15.
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.
Attachment #625832 - Flags: review?(bas.schouten)
OS: Windows 7 → Windows 8 Metro
Duplicate of this bug: 737837
Whiteboard: completed-elm
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.
Attachment #637536 - Flags: review?(bas.schouten)
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 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.
Attachment #637536 - Flags: review?(bas.schouten) → review+
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.
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 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.
Attachment #637536 - Flags: review+ → review?(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 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-.
Attachment #637536 - Flags: review?(bas.schouten) → review+
> 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.
Depends on: 774285
Attachment #625832 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/aa572892dca1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
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.
ok sounds good. FWIW I cancelled my mc merge due to issues with bug 626472.
Depends on: 770694
Component: Graphics → Graphics: Layers
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.