The default bug view has changed. See this FAQ.

Review and land DirectX 10 support for Windows 8 Metro builds

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jimm, Assigned: bbondy)

Tracking

(Depends on: 1 bug)

Trunk
mozilla17
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: completed-elm)

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Not sure if we added entry points to cairo, but if we did, it's moving to a new library - bug 751273.
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 622847 [details] [diff] [review]
Patch v1.

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)
(Assignee)

Comment 4

5 years ago
Created attachment 622866 [details] [diff] [review]
Patch v2.

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-
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 9

5 years ago
OK thanks, so I'll remove the cairo code I added that we were using before.
(Assignee)

Comment 10

5 years ago
Created attachment 625191 [details] [diff] [review]
Patch v3.

Using layer manager, removed cairo code
Attachment #622866 - Attachment is obsolete: true
Attachment #625191 - Flags: review?(bas.schouten)
(Assignee)

Comment 11

5 years ago
I'll be doing a follow up bug for DX9 support.
(Assignee)

Updated

5 years ago
Summary: Review and land gfx changes for Fx metro on mc → Review and land DirectX 10 support for Windows 8 Metro builds
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 13

5 years ago
Created attachment 625781 [details] [diff] [review]
Patch v4.

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.
(Assignee)

Comment 15

5 years ago
> 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.
(Assignee)

Comment 16

5 years ago
Created attachment 625832 [details] [diff] [review]
Patch v5
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?
(Assignee)

Comment 18

5 years ago
> Doesn't look like this part got fixed?

Please see Comment 15.
(Assignee)

Comment 19

5 years ago
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)
(Assignee)

Updated

5 years ago
OS: Windows 7 → Windows 8 Metro
(Assignee)

Updated

5 years ago
Duplicate of this bug: 737837
(Assignee)

Updated

5 years ago
Whiteboard: completed-elm
(Assignee)

Comment 21

5 years ago
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.
Attachment #637536 - Flags: review?(bas.schouten)
(Assignee)

Comment 22

5 years ago
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+
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 25

5 years ago
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.
(Assignee)

Comment 26

5 years ago
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+
(Assignee)

Comment 29

5 years ago
> 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.
(Assignee)

Updated

5 years ago
Depends on: 774285
(Assignee)

Updated

5 years ago
Attachment #625832 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/aa572892dca1
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/aa572892dca1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 32

5 years ago
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.
(Assignee)

Comment 33

5 years ago
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.
(Reporter)

Comment 34

5 years ago
ok sounds good. FWIW I cancelled my mc merge due to issues with bug 626472.
(Assignee)

Updated

5 years ago
Depends on: 770694
(Assignee)

Updated

5 years ago
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.