We need to implement OMTC for Direct3D 10. This bug will track that implementation.

Do we have a sense of how this work is progressing? Also, is there anything metro team members can do to help? My understanding is that we need to use the async scroller in the layers system for pan and zoom, and that this requires OMTC to work. So we have a lot of bugs blocked on omtc for Windows.

Just for giggles, I tried flicking this on and doing a Try push: https://tbpl.mozilla.org/?tree=Try&rev=20a80d5f4143 Looks like we fail to initialise the compositor everywhere except Win8. Is that because we are missing libraries on the test machines? We're not particularly green even on Win8.

Looks like the Win8 debug orange is mostly due to a bad assertion that can be easily fixed - should assert that texture is shmem || memory image, but just checks for shmem.

Comment on attachment 776937 [details] [diff] [review] bug fix for resizing windows ugliness Review of attachment 776937 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if we can create a test for this. Very silly of me, nice catch.

Comment on attachment 776960 [details] [diff] [review] formatting - getting my ocd on Review of attachment 776960 [details] [diff] [review]: ----------------------------------------------------------------- Can we only break lines which are over 100 characters? I find a lot of these changes disastrously ugly. ::: gfx/layers/d3d11/CompositorD3D11.cpp @@ +56,5 @@ > RefPtr<ID3D11BlendState> mPremulBlendState; > RefPtr<ID3D11BlendState> mNonPremulBlendState; > }; > > +CompositorD3D11::CompositorD3D11(nsIWidget* aWidget) I actually prefer the other style. I don't think our style guide offers any hints here so I guess we'll have to fight to the death :-). ::: gfx/layers/d3d11/CompositorD3D11.h @@ +135,5 @@ > > virtual nsIWidget* GetWidget() const MOZ_OVERRIDE { return mWidget; } > virtual const nsIntSize& GetWidgetSize() MOZ_OVERRIDE; > > + ID3D11Device* GetDevice() { return mDevice; } Similar style disagreements here :) @@ +159,5 @@ > > + nsIWidget* mWidget; > + // GetWidgetSize requires us to return a reference to an nsIntSize. Since we > + // don't otherwise keep this value around, we need mSize to avoid a dangling > + // reference problem. How about we -not- make it return a reference? :P

(In reply to Bas Schouten (:bas.schouten) from comment #11) > Comment on attachment 776960 [details] [diff] [review] > formatting - getting my ocd on > > Review of attachment 776960 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can we only break lines which are over 100 characters? I find a lot of these > changes disastrously ugly. > > ::: gfx/layers/d3d11/CompositorD3D11.cpp > @@ +56,5 @@ > > RefPtr<ID3D11BlendState> mPremulBlendState; > > RefPtr<ID3D11BlendState> mNonPremulBlendState; > > }; > > > > +CompositorD3D11::CompositorD3D11(nsIWidget* aWidget) > > I actually prefer the other style. I don't think our style guide offers any > hints here so I guess we'll have to fight to the death :-). Wait, somebody with no sense of style whatsoever seems to have edited the style guide an said pointer *'s need to snuggle with the types :P. I'm going to try and figure out where that decision happened and whether I can fight them to the death instead :-).

Comment on attachment 776960 [details] [diff] [review] formatting - getting my ocd on Review of attachment 776960 [details] [diff] [review]: ----------------------------------------------------------------- I talked about this a little on IRC and I'd prefer we did the line breaking a little more with human judgement. When the length is longer than a 100 characters I'd say it's a pretty strong limit so readability needs to be significantly improved for it to stay on one line. Between 80 and 100 I think in general it depends on what reads better. ::: gfx/layers/d3d11/CompositorD3D11.cpp @@ +722,5 @@ > > + hr = mDevice->CreateVertexShader(LayerQuadMaskVS, > + sizeof(LayerQuadMaskVS), > + nullptr, > + byRef(mAttachments->mVSQuadShader[Mask2d])); A line like this is a great idea to break up! And I consider it a failure I didn't do so myself :-) @@ +803,5 @@ > { > nsRefPtr<ID3D11Texture2D> backBuf; > > + mSwapChain->GetBuffer(0, __uuidof(ID3D11Texture2D), > + (void**)backBuf.StartAssignment()); A line like this is not, in my opinion.

I should note I realize our style guide says 80 characters for most code (I think js/ uses 100). I think that's something we should discuss, I realize that strictly speaking your changes are correct.

Comment on attachment 777347 [details] [diff] [review] Implement CreateRenderTargetFromSource Review of attachment 777347 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/d3d11/CompositorD3D11.cpp @@ +387,5 @@ > > RefPtr<ID3D11Texture2D> texture; > + mDevice->CreateTexture2D(&desc, nullptr, byRef(texture)); > + > + if (aSource) { We should add some bounds checking here, out of bounds CopySubresourceRegion's can cause driver crashes on some buggy drivers. ::: gfx/layers/d3d11/TextureD3D11.cpp @@ +45,5 @@ > > mTextures[0] = aTexture; > > RefPtr<ID3D11Device> device; > mTextures[0]->GetDevice(byRef(device)); Meh, we waste a GetDevice here now, but then again, who cares.

Is there a bug for D3D10 too?

(In reply to Marco Castelluccio [:marco] from comment #17) > Is there a bug for D3D10 too? No, we are only using d3d11 for OMTC.

(In reply to Nick Cameron [:nrc] from comment #18) > No, we are only using d3d11 for OMTC. There's bug 756608 for d3d9. Is that still planned?

(In reply to Marco Castelluccio [:marco] from comment #19) > (In reply to Nick Cameron [:nrc] from comment #18) > > No, we are only using d3d11 for OMTC. > > There's bug 756608 for d3d9. Is that still planned? Yes, it's almost done actually, just ironing out a bug or two.

(In reply to Bas Schouten (:bas.schouten) from comment #14) > I should note I realize our style guide says 80 characters for most code (I > think js/ uses 100). I think that's something we should discuss, I realize > that strictly speaking your changes are correct. I think there is some scope for having 'local style', although I think that is intended for old stuff rather than new stuff. We have not been too strict about the 80 char limit in gfx, but some modules (e.g., layout are). I kind of like the 80 char limit because it makes it easy to have two files side by side, but I agree we should not be too strict about it. Anyway, I prefer to have a soft 80 char limit than to have a hard 100 char limit. I'll adjust the formatting to be have more overruns.

(In reply to Bas Schouten (:bas.schouten) from comment #12) > (In reply to Bas Schouten (:bas.schouten) from comment #11) > > Comment on attachment 776960 [details] [diff] [review] > > formatting - getting my ocd on > > > > Review of attachment 776960 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Can we only break lines which are over 100 characters? I find a lot of these > > changes disastrously ugly. > > > > ::: gfx/layers/d3d11/CompositorD3D11.cpp > > @@ +56,5 @@ > > > RefPtr<ID3D11BlendState> mPremulBlendState; > > > RefPtr<ID3D11BlendState> mNonPremulBlendState; > > > }; > > > > > > +CompositorD3D11::CompositorD3D11(nsIWidget* aWidget) > > > > I actually prefer the other style. I don't think our style guide offers any > > hints here so I guess we'll have to fight to the death :-). > > Wait, somebody with no sense of style whatsoever seems to have edited the > style guide an said pointer *'s need to snuggle with the types :P. I'm going > to try and figure out where that decision happened and whether I can fight > them to the death instead :-). Heh, I prefer the * being part of the type, but don't have a strong preference. The style was kind of inconsistent before though - sometimes with type sometimes with the var.

(In reply to Bas Schouten (:bas.schouten) from comment #10) > Comment on attachment 776937 [details] [diff] [review] > bug fix for resizing windows ugliness > > Review of attachment 776937 [details] [diff] [review]: > ----------------------------------------------------------------- > > I wonder if we can create a test for this. Very silly of me, nice catch. Hmm, this doesn't seem to fix the Window resizing thing. But I think the fix is correct. I've not actually noticed any visual change though, so I'm not sure how to test it.

(In reply to Bas Schouten (:bas.schouten) from comment #11) > Comment on attachment 776960 [details] [diff] [review] > @@ +159,5 @@ > > > > + nsIWidget* mWidget; > > + // GetWidgetSize requires us to return a reference to an nsIntSize. Since we > > + // don't otherwise keep this value around, we need mSize to avoid a dangling > > + // reference problem. > > How about we -not- make it return a reference? :P I'll look into it, but I'll do it as a separate patch since it is more than just formatting.

now with bounds checks, carrying r=Bas

We choke on this further down in Moz2D (call to CreateSharedBitmap) but it was hard to detect and fallback there so added the check here.

Comment on attachment 778129 [details] [diff] [review] LockDrawTarget for shmem texture clients Review of attachment 778129 [details] [diff] [review]: ----------------------------------------------------------------- This is in general a great move! :) ::: gfx/layers/client/TextureClient.cpp @@ +128,5 @@ > + return mDrawTarget; > + } > + > + gfxASurface* surface = GetSurface(); > + mDrawTarget = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surface, mSize); Could we remove the dependency on Thebes and create a draw target for data directly? This'll make things a lot easier when we want to get rid of thebes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f69d2dc0960 https://hg.mozilla.org/integration/mozilla-inbound/rev/181a43481b18 https://hg.mozilla.org/integration/mozilla-inbound/rev/719257a27dbb

https://hg.mozilla.org/mozilla-central/rev/7f69d2dc0960 https://hg.mozilla.org/mozilla-central/rev/181a43481b18 https://hg.mozilla.org/mozilla-central/rev/719257a27dbb

Comment on attachment 778129 [details] [diff] [review] LockDrawTarget for shmem texture clients (In reply to Bas Schouten (:bas.schouten) from comment #29) > Comment on attachment 778129 [details] [diff] [review] > LockDrawTarget for shmem texture clients > > Review of attachment 778129 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is in general a great move! :) > > ::: gfx/layers/client/TextureClient.cpp > @@ +128,5 @@ > > + return mDrawTarget; > > + } > > + > > + gfxASurface* surface = GetSurface(); > > + mDrawTarget = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surface, mSize); > > Could we remove the dependency on Thebes and create a draw target for data > directly? This'll make things a lot easier when we want to get rid of thebes. That's actually pretty tricky because it means being able to open shmems as Azure DrawTargets rather than Thebes surfaces (and probably add methods for opening SurfaceDescriptors as DrawTargets). Which is not impossible, but more than I really want to get involved in for this. Just opening SurfaceDescriptors as DrawTargets but leaving the shmem stuff essentially means just wrapping the Thebes surface, but at a lower level.

Comment on attachment 778129 [details] [diff] [review] LockDrawTarget for shmem texture clients Review of attachment 778129 [details] [diff] [review]: ----------------------------------------------------------------- Hrm, we'll want to do that eventually I suppose, but maybe now is not the time!

https://hg.mozilla.org/integration/mozilla-inbound/rev/51d91b3220d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc535ec4206

https://hg.mozilla.org/mozilla-central/rev/51d91b3220d9 https://hg.mozilla.org/mozilla-central/rev/bdc535ec4206

How do I force Direct3D 11 OMTC for testing purposes in Nightly? It seems to be ignoring the force Direct2D option in about:config. The same option works fine with OMTC disabled. In Firefox 24, I have the opposite problem, in that D3D11 OMTC works even if I force disable Direct2D. So, while something's changed between now and then, forcing on or off D2D seems to be being ignored. BTW, the content renderer seems to have improved so that the black box problem (on some older graphics cards) is no longer present there--although it's still present in Direct2D layers and canvas--hence why I wish to test the D3D11 OMTC. Note that about:support still shows the Direct2D content renderer as working even though it says I'm in D3D9 OMTC.

(In reply to Terrell Kelley from comment #36) > How do I force Direct3D 11 OMTC for testing purposes in Nightly? It seems to > be ignoring the force Direct2D option in about:config. The same option works > fine with OMTC disabled. I set the following to true in about:config layers.acceleration.force-enabled layers.offmainthreadcomposition.enabled layers.offmainthreadcomposition.animate-opacity layers.offmainthreadcomposition.animate-transform layers.async-video.enabled about:support now shows 1/1 Direct3D 11 (OMTC).

(In reply to Brian Carpenter [:geeknik] from comment #37) > I set the following to true in about:config > > layers.acceleration.force-enabled > layers.offmainthreadcomposition.enabled > layers.offmainthreadcomposition.animate-opacity > layers.offmainthreadcomposition.animate-transform > layers.async-video.enabled > > about:support now shows 1/1 Direct3D 11 (OMTC). Adding those I hadn't already tried got Direct2D in content and canvas, but layers still says "Direct3D 9 (OTMC)." My guess is that gfx.direct2d.force-enabled is being ignored by Layers when OTMC is on. But I didn't want to file a bug until I knew I wasn't doing something stupid.

I've now filed bug 927405 about this.

I verified that 1/1 Direct3D 11 (OMTC) is activated on Windows 8.1, Windows 7 and Windows Vista. And did some exploratory on some websites: https://etherpad.mozilla.org/firefox-Direct3D11-OMTC Dropping verifyme since Direct3D 11 is activated. On a particular website I encountered a black rendering issue. This only happens if 'layers.offmainthreadcomposition.enabled' is enabled. Reproduced on all OS`s from above using Firefox 28beta, Aurora and Nightly. Screencast: http://www.screencast.com/t/VqP9UsR7 Should I log a new issue on that?

Yes please, and make it block bug 899785 which is to turn OMTC DX11 on by default.

Thanks Milan, I logged bug 977520 for that.

Calling this verified based on comment 40. Given the scope, should this get the feature keyword and/or relnoted?

Putting ni? on myself to come back and check on this - but can someone summarize what the note should tell the user about this change?

This is a trick bug - OMTC D3D11 is implemented, but off by default. On by default shows up in bug 899785. If we want to release note this, it should be "if you enable this option, then... and you may notice some performance degradation in...". Let me know if that's the case and I can get you the text for the two ... places.

(In reply to Milan Sreckovic [:milan] from comment #45) > This is a trick bug - OMTC D3D11 is implemented, but off by default. On by > default shows up in bug 899785. If we want to release note this, it should > be "if you enable this option, then... and you may notice some performance > degradation in...". Let me know if that's the case and I can get you the > text for the two ... places. Ah. In that case, no note needed - we will not note something the user would have to manually enable.