830347 - (D3D11-OMTC) Implement D3D11 Compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Bas Schouten (:bas.schouten)]

We need an implementation of the new Compositor API in D3D11.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add D3D11 device to gfxWindowsPlatform.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add D3D11 Compositor to Layers.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Integrate CompositorD3D11 into windows Widget.

Comment 4

[Jim Mathies [:jimm]]

part 2 is failing to apply on mc tip due to a problem in CompositorParent. It looks like AllocPLayer has changed to AllocPLayerTransaction, and returns a LayerTransactionParent.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 741821 [details] [diff] [review]
Part 2: Add D3D11 Compositor to Layers.

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

::: gfx/layers/Makefile.in
@@ +93,5 @@
>          ReadbackManagerD3D10.cpp \
>          ShadowLayerUtilsD3D10.cpp \
>          ThebesLayerD3D10.cpp \
> +        CompositorD3D11.cpp \
> +        TextureD3D11.cpp \

looks like the files should be in alphabetical order here.

::: gfx/layers/client/CompositableClient.cpp
@@ +112,2 @@
>    case TEXTURE_SHMEM:
> +    MOZ_ASSERT(parentBackend == LAYERS_OPENGL || parentBackend == LAYERS_D3D11);

I wouldn't go for a fatal assertion here. Not even sure that we actually want to ensure this. I expect software backend would use TextureClientShmem in this situation as well. Let's drop the assertion.

::: gfx/layers/client/ImageClient.cpp
@@ +28,5 @@
>    RefPtr<ImageClient> result = nullptr;
>    switch (aCompositableHostType) {
>    case BUFFER_IMAGE_SINGLE:
> +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> +        aForwarder->GetCompositorBackendType() == LAYERS_D3D11) {

We should remove these 3 if statements instead.

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +350,5 @@
> +
> +  return rt;
> +}
> +
> +// TODO[Bas] this method doesn't actually use aSource

Maybe this should be fixed before landing?

@@ +593,5 @@
> +  memcpy(&mVSConstants.projection, &projection, 64);
> +}
> +
> +const nsIntSize&
> +CompositorD3D11::GetWidgetSize()

This is not exactly the same behavior as Compositor OGL which returns the size of the last beginning of a frame (see BeginFrame). I haven't gone into the details of looking whether this could lead to race conditions but it doesn't seem unreasonable that the size stays the same during a composition. Can you justify than one way is better than the other?

::: gfx/layers/d3d11/CompositorD3D11.fx
@@ +98,5 @@
> +float4 TransformedPostion(float2 aInPosition)
> +{
> +  // the current vertex's position on the quad
> +  float4 position = float4(0, 0, 0, 1);
> +  

nit: trailing space

@@ +121,5 @@
> +  result.w = aTransformedPosition.w;
> +  result.xyz = aTransformedPosition.xyz / aTransformedPosition.w;
> +  result -= vRenderTargetOffset;
> +  result.xyz *= result.w;
> +  

nit: trailing space

::: gfx/layers/d3d11/CompositorD3D11.h
@@ +34,5 @@
> +};
> +
> +struct DeviceAttachmentsD3D11;
> +
> +class CompositorD3D11 : public Compositor

Please use MOZ_OVERRIDE whenever applicable in this class

@@ +144,5 @@
> +  nsRefPtr<gfxContext> mTarget;
> +
> +  nsIWidget *mWidget;
> +  // XXX - Bas - wth?
> +  nsIntSize mSize;

See my other comment about resize behavior in GL vs D3D compositors, This looks like a left-over from a copy-pasting of the CompositorOGL stuff.

::: gfx/layers/d3d11/CompositorD3D11Shaders.h
@@ +3,5 @@
> +// Generated by Microsoft (R) HLSL Shader Compiler 9.30.9200.16384
> +//
> +//
> +///
> +// Buffer Definitions: 

nit: trailing space

@@ +32,5 @@
> +// Input signature:
> +//
> +// Name                 Index   Mask Register SysValue  Format   Used
> +// -------------------- ----- ------ -------- -------- ------- ------
> +// POSITION                 0   xy          0     NONE   float   xy  

nit: trailing space

@@ +40,5 @@
> +//
> +// Name                 Index   Mask Register SysValue  Format   Used
> +// -------------------- ----- ------ -------- -------- ------- ------
> +// SV_Position              0   xyzw        0      POS   float   xyzw
> +// TEXCOORD                 0   xy          1     NONE   float   xy  

nit: trailing space

@@ +97,5 @@
> +mad r1.xyzw, cb0[4].xyzw, r0.xxxx, r1.xyzw
> +mad r1.xyzw, cb0[6].xyzw, r0.zzzz, r1.xyzw
> +mad o0.xyzw, cb0[7].xyzw, r0.wwww, r1.xyzw
> +mad o1.xy, v0.xyxx, cb0[9].zwzz, cb0[9].xyxx
> +ret 

trailing space

@@ +103,5 @@
> +#endif
> +
> +const BYTE LayerQuadVS[] =
> +{
> +     68,  88,  66,  67,  94, 179, 

trailing spaces on every line in this array

@@ +426,5 @@
> +//
> +// Name                 Index   Mask Register SysValue  Format   Used
> +// -------------------- ----- ------ -------- -------- ------- ------
> +// SV_Position              0   xyzw        0      POS   float       
> +// TEXCOORD                 0   xy          1     NONE   float       

trailing spaces

@@ +453,5 @@
> +ps_4_0
> +dcl_constantbuffer cb0[1], immediateIndexed
> +dcl_output o0.xyzw
> +mov o0.xyzw, cb0[0].xyzw
> +ret 

trailing spaces a little bit of everywhere in this file (I'll not point them all out)

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +279,5 @@
> +  IntRect rect = GetTileRect(mCurrentTile);
> +  return nsIntRect(rect.x, rect.y, rect.width, rect.height);
> +}
> +
> +uint32_t GetRequiredTiles(uint32_t aSize, uint32_t aMaxSize)

should be static

::: gfx/layers/d3d11/TextureD3D11.h
@@ +48,5 @@
> +public:
> +  // Use aTexture == nullptr for rendering to the window
> +  CompositingRenderTargetD3D11(ID3D11Texture2D *aTexture);
> +
> +  virtual TextureSourceD3D11* AsSourceD3D11() { return this; }

Please use MOZ_OVERRIDE whenever applicable

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 741819 [details] [diff] [review]
Part 1: Add D3D11 device to gfxWindowsPlatform.

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +572,5 @@
>      if (!IsRunningInWindowsMetro()) {
>        supportedFeatureLevelsCount--;
>      }
>  
> +    nsRefPtr<IDXGIAdapter1> adapter1 = GetDXGIAdapter();

It looks like you're playing it a bit loose with nsRefPtr vs RefPtr. Try to be consistent:)

@@ +1451,5 @@
>      }
>  #endif
>  }
>  
> +ID3D11Device*

This should probably return an already_addrefed.

@@ +1500,5 @@
>  {
>    return GetModuleHandleA("nvumdshim.dll");
>  }
> +
> +IDXGIAdapter1*

This should probably return an already_addrefed.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 741826 [details] [diff] [review]
Part 3: Integrate CompositorD3D11 into windows Widget.

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

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +886,5 @@
>  }
>  
>  void nsBaseWidget::CreateCompositor(int aWidth, int aHeight)
>  {
> +  // Recreating this is tricky as when we 

nit - unfinished comment?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 741821 [details] [diff] [review]
Part 2: Add D3D11 Compositor to Layers.

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +536,5 @@
> +
> +  ID3D11Buffer *buffer = mAttachments->mVertexBuffer;
> +  UINT size = sizeof(Vertex);
> +  UINT offset = 0;
> +  mContext->IASetVertexBuffers(0, 1, &buffer, &size, &offset);

Do we need to do this every frame?

@@ +589,5 @@
> +
> +  gfx3DMatrix projection = gfx3DMatrix::From2D(viewMatrix);
> +  projection._33 = 0.0f;
> +
> +  memcpy(&mVSConstants.projection, &projection, 64);

sizeof(mVSConstants.projection) instead of 64

@@ +629,5 @@
> +                              0);
> +  } else {
> +    mSwapChain->ResizeBuffers(1, rect.width, rect.height,
> +                              DXGI_FORMAT_B8G8R8A8_UNORM,
> +                              DXGI_SWAP_CHAIN_FLAG_GDI_COMPATIBLE);

Do we need GDI_COMPATBILE?

@@ +707,5 @@
> +  *(VertexShaderConstants*)resource.pData = mVSConstants;
> +  mContext->Unmap(mAttachments->mVSConstantBuffer, 0);
> +  mContext->Map(mAttachments->mPSConstantBuffer, 0, D3D11_MAP_WRITE_DISCARD, 0, &resource);
> +  *(PixelShaderConstants*)resource.pData = mPSConstants;
> +  mContext->Unmap(mAttachments->mPSConstantBuffer, 0);

Does it make sense to put pixel and vertex shaders into a single buffer so that we don't have to do multiple Maps per frame?

@@ +729,5 @@
> +  case FILTER_POINT:
> +    sampler = mAttachments->mPointSamplerState;
> +    break;
> +  default:
> +    sampler = mAttachments->mLinearSamplerState;

Drop the 'default' case so that we get compiler warnings when we don't handle the enum.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +92,5 @@
> +    }
> +
> +    if (mTexture) {
> +      return;
> +    }

Joe and I both had a bit of a problem reading this code:
how about:

if (desc.Width == aSize.width && desc.Height == aSize.height) {
   // we can reuse our texture
   return;
}

mTexture = nullptr
mSurface = nullptr
CreateDT();

@@ +332,5 @@
> +
> +  CD3D11_TEXTURE2D_DESC desc(dxgiFormat, size.width, size.height,
> +                            1, 1, D3D11_BIND_SHADER_RESOURCE, D3D11_USAGE_IMMUTABLE);
> +
> +  uint32_t maxSize = GetMaxTextureSizeForFeatureLevel(mDevice->GetFeatureLevel());

Shouldn't this be using the actual max texture size?

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Nicolas Silva [:nical] from comment #5)
> Comment on attachment 741821 [details] [diff] [review]
> Part 2: Add D3D11 Compositor to Layers.
> 
> Review of attachment 741821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Makefile.in
> @@ +93,5 @@
> >          ReadbackManagerD3D10.cpp \
> >          ShadowLayerUtilsD3D10.cpp \
> >          ThebesLayerD3D10.cpp \
> > +        CompositorD3D11.cpp \
> > +        TextureD3D11.cpp \
> 
> looks like the files should be in alphabetical order here.

If that makes you happy I'm all for it! :)

> ::: gfx/layers/client/CompositableClient.cpp
> @@ +112,2 @@
> >    case TEXTURE_SHMEM:
> > +    MOZ_ASSERT(parentBackend == LAYERS_OPENGL || parentBackend == LAYERS_D3D11);
> 
> I wouldn't go for a fatal assertion here. Not even sure that we actually
> want to ensure this. I expect software backend would use TextureClientShmem
> in this situation as well. Let's drop the assertion.

Agreed!

> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ +28,5 @@
> >    RefPtr<ImageClient> result = nullptr;
> >    switch (aCompositableHostType) {
> >    case BUFFER_IMAGE_SINGLE:
> > +    if (aForwarder->GetCompositorBackendType() == LAYERS_OPENGL ||
> > +        aForwarder->GetCompositorBackendType() == LAYERS_D3D11) {
> 
> We should remove these 3 if statements instead.

Even better!

> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +350,5 @@
> > +
> > +  return rt;
> > +}
> > +
> > +// TODO[Bas] this method doesn't actually use aSource
> 
> Maybe this should be fixed before landing?

No, this is only important for subpixel AA and we don't really need to worry about that for initial landing. I was told we might not even care about subpixel AA for metro release.

> @@ +593,5 @@
> > +  memcpy(&mVSConstants.projection, &projection, 64);
> > +}
> > +
> > +const nsIntSize&
> > +CompositorD3D11::GetWidgetSize()
> 
> This is not exactly the same behavior as Compositor OGL which returns the
> size of the last beginning of a frame (see BeginFrame). I haven't gone into
> the details of looking whether this could lead to race conditions but it
> doesn't seem unreasonable that the size stays the same during a composition.
> Can you justify than one way is better than the other?

No, except that -this- returns what the function is asking for, if we want to change what it returns, let's rename it :)

> ::: gfx/layers/d3d11/CompositorD3D11Shaders.h
> @@ +3,5 @@
> > +// Generated by Microsoft (R) HLSL Shader Compiler 9.30.9200.16384
> > +//
> > +//
> > +///
> > +// Buffer Definitions: 
> 
> nit: trailing space

This file is autogenerated so you'll have to live with them ;).

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 741821 [details] [diff] [review]
> Part 2: Add D3D11 Compositor to Layers.
> 
> Review of attachment 741821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +536,5 @@
> > +
> > +  ID3D11Buffer *buffer = mAttachments->mVertexBuffer;
> > +  UINT size = sizeof(Vertex);
> > +  UINT offset = 0;
> > +  mContext->IASetVertexBuffers(0, 1, &buffer, &size, &offset);
> 
> Do we need to do this every frame?

It seems like the safest thing, once per frame is not that high overhead and otherwise we'd have to guarantee no external party changes the state of the D3D11 device. Right now that's easy but it might be different in the future.

> @@ +629,5 @@
> > +                              0);
> > +  } else {
> > +    mSwapChain->ResizeBuffers(1, rect.width, rect.height,
> > +                              DXGI_FORMAT_B8G8R8A8_UNORM,
> > +                              DXGI_SWAP_CHAIN_FLAG_GDI_COMPATIBLE);
> 
> Do we need GDI_COMPATBILE?

I believe we do to avoid bugs on some hardware on desktop :s.

> @@ +707,5 @@
> > +  *(VertexShaderConstants*)resource.pData = mVSConstants;
> > +  mContext->Unmap(mAttachments->mVSConstantBuffer, 0);
> > +  mContext->Map(mAttachments->mPSConstantBuffer, 0, D3D11_MAP_WRITE_DISCARD, 0, &resource);
> > +  *(PixelShaderConstants*)resource.pData = mPSConstants;
> > +  mContext->Unmap(mAttachments->mPSConstantBuffer, 0);
> 
> Does it make sense to put pixel and vertex shaders into a single buffer so
> that we don't have to do multiple Maps per frame?

I don't think that's possible? But I'll have another look.

> @@ +332,5 @@
> > +
> > +  CD3D11_TEXTURE2D_DESC desc(dxgiFormat, size.width, size.height,
> > +                            1, 1, D3D11_BIND_SHADER_RESOURCE, D3D11_USAGE_IMMUTABLE);
> > +
> > +  uint32_t maxSize = GetMaxTextureSizeForFeatureLevel(mDevice->GetFeatureLevel());
> 
> Shouldn't this be using the actual max texture size?

I was unable to find such a thing in D3D11.

Comment 11

[Nicolas Silva [:nical]]

(In reply to Bas Schouten (:bas.schouten) from comment #9)
> (In reply to Nicolas Silva [:nical] from comment #5)
> > Comment on attachment 741821 [details] [diff] [review]
> > @@ +593,5 @@
> > > +  memcpy(&mVSConstants.projection, &projection, 64);
> > > +}
> > > +
> > > +const nsIntSize&
> > > +CompositorD3D11::GetWidgetSize()
> > 
> > This is not exactly the same behavior as Compositor OGL which returns the
> > size of the last beginning of a frame (see BeginFrame). I haven't gone into
> > the details of looking whether this could lead to race conditions but it
> > doesn't seem unreasonable that the size stays the same during a composition.
> > Can you justify than one way is better than the other?
> 
> No, except that -this- returns what the function is asking for, if we want
> to change what it returns, let's rename it :)

I'll file a followup bug to make all backends do the same thing.
This method is called by backend independent logic so all backends should have the same semantics.
Since querying the widget size to a Widget that belongs to another thread without any form of synchronization I don't know that the way it is done in the D3D compositor is correct. By looking at our code it doesn't appear that realy bad things would happen (at most some things could be a bit glitchy while the window is resized I suppose).
If you keep it like this, you should remove the mWidgetSize member that is unused.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 741821 [details] [diff] [review]
Part 2: Add D3D11 Compositor to Layers.

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

::: gfx/layers/d3d11/TextureD3D11.h
@@ +228,5 @@
> +  int32_t maxTextureSize;
> +  switch (aFeatureLevel) {
> +  case D3D_FEATURE_LEVEL_11_1:
> +  case D3D_FEATURE_LEVEL_11_0:
> +    maxTextureSize = 16384;

D3D11_REQ_TEXTURE2D_U_OR_V_DIMENSION

@@ +232,5 @@
> +    maxTextureSize = 16384;
> +    break;
> +  case D3D_FEATURE_LEVEL_10_1:
> +  case D3D_FEATURE_LEVEL_10_0:
> +    maxTextureSize = 8192;

D3D10_REQ_TEXTURE2D_U_OR_V_DIMENSION

@@ +235,5 @@
> +  case D3D_FEATURE_LEVEL_10_0:
> +    maxTextureSize = 8192;
> +    break;
> +  case D3D_FEATURE_LEVEL_9_3:
> +    maxTextureSize = 4096;

D3D_FL9_3_REQ_TEXTURE2D_U_OR_V_DIMENSION

@@ +238,5 @@
> +  case D3D_FEATURE_LEVEL_9_3:
> +    maxTextureSize = 4096;
> +    break;
> +  default:
> +    maxTextureSize = 2048;

D3D_FL9_1_REQ_TEXTURE2D_U_OR_V_DIMENSION

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> > @@ +629,5 @@
> > > +                              0);
> > > +  } else {
> > > +    mSwapChain->ResizeBuffers(1, rect.width, rect.height,
> > > +                              DXGI_FORMAT_B8G8R8A8_UNORM,
> > > +                              DXGI_SWAP_CHAIN_FLAG_GDI_COMPATIBLE);
> > 
> > Do we need GDI_COMPATBILE?
> 
> I believe we do to avoid bugs on some hardware on desktop :s.

Can we put a bug number in a comment.


> > > +  uint32_t maxSize = GetMaxTextureSizeForFeatureLevel(mDevice->GetFeatureLevel());
> > 
> > Shouldn't this be using the actual max texture size?
> 
> I was unable to find such a thing in D3D11.

Neither was I.

Comment 14

[Jim Mathies [:jimm]]

Is this blocked on something or is it ready to land?

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Last we talked, today looked like a day for it.

Comment 16

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05762bbaf268
https://hg.mozilla.org/integration/mozilla-inbound/rev/478c8b6a9736
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f52629c9d08

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05762bbaf268
https://hg.mozilla.org/mozilla-central/rev/478c8b6a9736
https://hg.mozilla.org/mozilla-central/rev/6f52629c9d08

Comment 18

[Philip Chee]

Is there a missing include somewhere?

c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error C2065: 'D3D_FEATURE_LEVEL_11_1' : undeclared identifier
c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error C2051: case expression not constant
c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(235) : error C2065: 'D3D_FL9_3_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier
c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(238) : error C2065: 'D3D_FL9_1_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier

Comment 19

[Jacek Caban]

Pushed fix for mingw compilation:

https://hg.mozilla.org/integration/mozilla-inbound/rev/182f3878a17b

The error was:

TextureD3D11.h:115:89: error: operands to ?: have different types ‘__gnu_cxx::__alloc_traits<std::allocator<mozilla::RefPtr<ID3D11Texture2D> > >::value_type {aka mozilla::RefPtr<ID3D11Texture2D>}’ and ‘ID3D11Texture2D*’

Comment 20

[Jim Mathies [:jimm]]

(In reply to Philip Chee from comment #18)
> Is there a missing include somewhere?
> 
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error
> C2065: 'D3D_FEATURE_LEVEL_11_1' : undeclared identifier
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(226) : error
> C2051: case expression not constant
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(235) : error
> C2065: 'D3D_FL9_3_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier
> c:\t1\hg\comm-central\mozilla\gfx\layers\d3d11\TextureD3D11.h(238) : error
> C2065: 'D3D_FL9_1_REQ_TEXTURE2D_U_OR_V_DIMENSION' : undeclared identifier

I wonder if this landing is going to force the requirement of the Win 8 sdk, looks like it might.

Comment 21

[Bill Gianopoulos [:WG9s]]

Does requiring windows 8 SDK mean we also need to drop support for older Visual C compilers?  See bug 866425.

Comment 22

[Bill Gianopoulos [:WG9s]]

Can we do this with ifdefs to make it not a requirement?

Comment 23

[Jim Mathies [:jimm]]

This is being discussed in bug 869868.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/24d361942d24

Comment 25

[Philip Chee]

https://hg.mozilla.org/mozilla-central/rev/24d361942d24

Comment 26

[Jim Mathies [:jimm]]

Attachment: 601 sdk fix

Comment 27

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e223c0cdc8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a8d467615f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a2a23786f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1d7931af7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5daf23a9a20

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Jim Mathies [:jimm] from comment #26)
> Created attachment 747398 [details] [diff] [review]
> 601 sdk fix

This "fix" actually breaks the build if you're using the Windows 8 SDK with regular Firefox (without --enable-metro).  Then the target version is still 601 and so these shims are included and conflict with the real values in the SDK headers.

I worked around this with --with-windows-version=602, but we should fix this.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e223c0cdc8d
https://hg.mozilla.org/mozilla-central/rev/b1a8d467615f
https://hg.mozilla.org/mozilla-central/rev/d5a2a23786f5
https://hg.mozilla.org/mozilla-central/rev/5a1d7931af7b
https://hg.mozilla.org/mozilla-central/rev/c5daf23a9a20

Comment 30

[Jim Mathies [:jimm]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #28)
> (In reply to Jim Mathies [:jimm] from comment #26)
> > Created attachment 747398 [details] [diff] [review]
> > 601 sdk fix
> 
> This "fix" actually breaks the build if you're using the Windows 8 SDK with
> regular Firefox (without --enable-metro).  Then the target version is still
> 601 and so these shims are included and conflict with the real values in the
> SDK headers.
> 
> I worked around this with --with-windows-version=602, but we should fix this.

Sorry about that, I'll touch this up today.