874721 - Implement CompositorD3D9

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Nick Cameron [:nrc]]

I plan to do this using shaders at first so that we can go as quickly possible to OMTC for D3D9 and to find any problems with textures etc. We will go for a fixed function pipeline in the long run (bug 874722).

Comment 2

[Nick Cameron [:nrc]]

OK, a status update - I've been working on this on and off due to a slew of other bugs. I now have OMTC d3d9 working using a programmable pipeline compositor. I had a few interesting hold-ups with intermediate surfaces, mask layers, buffer rotation, and component alpha. Currently it works for everything I could think to throw at it (component alpha and non- thebes layers, single and double buffering, buffer rotation and non, video, canvas, webgl, mask layers, 3D tranforms, regular surfing for a while), the only thing that is busted is colour layers (more below). Currently I only have shmem texture client/hosts working (using shmem or memory images). Next step is a SYSTEMMEM texture client/host, then I'll get review and try to land.

Color Layers are an issue. I have spent more time on this then I would like to admit. I am sure the code is right. I have used PIX to grab the d3d commands and they are identical for the compositor and the layer manager (old version, which def. works). There are no textures involved, so I don't see why being OMTC should be an issue. I have tried using Clear() (without shaders) and that works (but of course mask layers don't). I've modified the shaders to paint a constant colour and that isn't happening - so I think the problem is we are not calling the shaders, but I'm not sure how because PIX is showing the correct commands getting called. I've tried disabling all clipping, scissor rects etc., and just painting a 100x100 quad and even that doesn't work.

I kind of suspect a compiler bug. I have managed to make colour layers work by copying the old code and executing that in DrawQuad outside of the effect-type switch statement. That worked. So I incrementally changed the code until it was more and more like the new code. What made the difference between rendering and not-rendering turned out to be removing code that was never executed due to an early return(!). A few other changes which should not change the semantics also gave success or failure. So, I looked at the disassembly. I verified that the code (at the instruction level) that shouldn't be executed wasn't using the debugger. Looking at the generated code the only difference in code that was actually executed was the register allocation. (This makes sense, code in the same function, even if not executed, could affect the register allocation). But, I wasn't able to identify exactly what was causing the difference, the different registers looked like they would be overwritten in both cases before the d3d calls. But there were some pretty exotic instructions generated which I'm not really familiar with, so I probably missed something.

Its also probably the case that I'm doing something obviously stupid like putting a * in the wrong place and I'm just not noticing it, because that is what usually happens when I spend days trying to debug something tricky.

Sadly, I don't have a disassembly dump of working code (other changes seem to have broken the setup I had before). I'll try and get it back at some point.

I also need to try using the debug/software d3d library and if doing optimised builds makes a difference. After that I'm probably just going to give up and write the fixed function pipeline version, because I could probably have done it already in the time I have spent trying to debug stupid colour layers.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bas, maybe you could look at Nick's code and see if you can see anything?

Nick, can you attach a patch?

Comment 4

[Nick Cameron [:nrc]]

Attachment: WIP patch

Bas: do you see anything obviously wrong with the solid colour rendering? (Just ignore all the wrong stuff elsewhere for now :-p )

Comment 5

[Nick Cameron [:nrc]]

Attachment: preliminary

Comment 6

[Nick Cameron [:nrc]]

Attachment: WIP patch

rebased

Comment 7

[Bas Schouten (:bas.schouten)]

Diagnosed problem and sent solution on IRC, caused by uninitialized memory in renderTargetOffset.

Comment 8

[Nick Cameron [:nrc]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=63c095365dbf

Comment 9

[Nick Cameron [:nrc]]

Attachment: unify mask types

Comment 10

[Nick Cameron [:nrc]]

Attachment: compositor and texture client/hosts

Comment 11

[Nick Cameron [:nrc]]

try again: https://tbpl.mozilla.org/?tree=Try&rev=7fc21114aa4f

Comment 12

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74a5d55fd3b8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74a5d55fd3b8

Comment 14

[Nick Cameron [:nrc]]

Attachment: patch 2: compositor and texture client/hosts

Comment 15

[Nick Cameron [:nrc]]

Corrected a few minor errors

Comment 16

[Nick Cameron [:nrc]]

Attachment: patch 3: Fallback for EnsureAllocated and compositor, and some more sanity checks

Comment 17

[Nick Cameron [:nrc]]

Attachment: patch 4: identify multi-process OMTC and use shmem

Comment 18

[Nick Cameron [:nrc]]

Attachment: patch 5: don't OOM with large canvases

Comment 19

[Nick Cameron [:nrc]]

Attachment: patch 6: notify the layer when its compositable is about to get detached from it

Comment 20

[Nick Cameron [:nrc]]

Attachment: patch 7: fuzz some tests

Comment 21

[Nick Cameron [:nrc]]

Attachment: patch 8: Re-fix WebGL

Comment 22

[Nick Cameron [:nrc]]

Attachment: patch 6: notify the layer when its compositable is about to get detached from it

Comment 23

[Nick Cameron [:nrc]]

Attachment: patch 7: fuzz some tests

Comment 24

[Nick Cameron [:nrc]]

Attachment: patch 8: Re-fix WebGL

Comment 25

[Nick Cameron [:nrc]]

Attachment: patch 7: fuzz some tests

Comment 26

[Nick Cameron [:nrc]]

Attachment: patch 7: fuzz some tests

Comment 27

[Bas Schouten (:bas.schouten)]

Comment on attachment 772920 [details] [diff] [review]
patch 2: compositor and texture client/hosts

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

I didn't review this in too much detail, but I believe that's okay since we want to switch this mostly to Fixed function short term anyway. And this won't be switched on by default until then, in the meanwhile we have some time to find any bugs.

::: gfx/layers/d3d9/ColorLayerD3D9.cpp
@@ +45,5 @@
>    color[1] = (float)(layerColor.g * opacity);
>    color[2] = (float)(layerColor.b * opacity);
>    color[3] = (float)(opacity);
>  
> +  aManager->device()->SetPixelShaderConstantF(CBvColor, color, 1);

Nice drive-by fix ;)

::: gfx/layers/d3d9/CompositorD3D9.cpp
@@ +101,5 @@
> +
> +  return rt;
> +}
> +
> +// TODO this method doesn't actually use aSource - do we need it to? (d3d11 doesn't)

Yes, this is a bug in D3D11 then I suppose :).

@@ +130,5 @@
> +  MOZ_ASSERT(aRenderTarget);
> +  RefPtr<CompositingRenderTargetD3D9> oldRT = mCurrentRT;
> +  mCurrentRT = static_cast<CompositingRenderTargetD3D9*>(aRenderTarget);
> +  mCurrentRT->BindRenderTarget(device());
> +  PrepareViewport(mCurrentRT->GetSize(), gfxMatrix());

This is an externally exposed function, should we be calling it ourselves here?

@@ +263,5 @@
> +                                           textureCoords.y,
> +                                           textureCoords.width,
> +                                           textureCoords.height),
> +                                         1);
> +                                    

nit: whitespace

@@ +497,5 @@
> +}
> +
> +void
> +CompositorD3D9::SetSamplerForFilter(Filter aFilter)
> +{

You probably want to store the current filter and not call this if it's not needed, state changes like this are not completely negligible. D3D9 might optimize this internally already with the flag combination we sent when creating the device though I guess.

@@ +539,5 @@
> +                        rect.Pitch,
> +                        gfxASurface::ImageFormatARGB32);
> +
> +  mTarget->SetSource(imageSurface);
> +  mTarget->SetOperator(gfxContext::OPERATOR_OVER);

Shouldn't this be SOURCE if you're not clearing the target?

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +256,5 @@
> +  }
> +}
> +
> +IntRect
> +TextureHostD3D9::GetTileRect(uint32_t aID) const

I wonder if we should share this code between Compositors.

Comment 28

[Bas Schouten (:bas.schouten)]

Comment on attachment 772999 [details] [diff] [review]
patch 6: notify the layer when its compositable is about to get detached from it

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

Is this all just to facilitate D3D9 cleaning up resources?

Comment 29

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas.schouten) from comment #28)
> Comment on attachment 772999 [details] [diff] [review]
> patch 6: notify the layer when its compositable is about to get detached
> from it
> 
> Review of attachment 772999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this all just to facilitate D3D9 cleaning up resources?

Bas and I discussed this on IRC. In summary, 'no'. We agreed we don't like the spaghetti nature of the calls here. One alternative is to check compositables are valid before use, but this is a more fragile approach because we might forget to add the check in the future. Bas will think about this and leave a comment later.

Comment 30

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas.schouten) from comment #27)
> Comment on attachment 772920 [details] [diff] [review]
> patch 2: compositor and texture client/hosts
> 
> Review of attachment 772920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't review this in too much detail, but I believe that's okay since we
> want to switch this mostly to Fixed function short term anyway. And this
> won't be switched on by default until then, in the meanwhile we have some
> time to find any bugs.

We do want fixed function, but that will share a lot of code with this. And 'short' might not be so short that we want this sitting around without a decent review. I think we should turn this on by default because that is how we will find bugs, both for fixed d3d9 when that happens and for d3d11. It would be really good to have some windows users testing OMTC. Also, this should just be an improved experience, so I don't see why we wouldn't want to turn it on.

There are a few comments with review questions marked 'REVIEW' in this patch. Could you check for those please?

> > +// TODO this method doesn't actually use aSource - do we need it to? (d3d11 doesn't)
> 
> Yes, this is a bug in D3D11 then I suppose :).
> 

Fixing this now. I thought we had a conversation when the d3d11 backend landed that we didn't need this, but perhaps that was only for Metro, or maybe I forgot some other caveat.

> @@ +130,5 @@
> > +  MOZ_ASSERT(aRenderTarget);
> > +  RefPtr<CompositingRenderTargetD3D9> oldRT = mCurrentRT;
> > +  mCurrentRT = static_cast<CompositingRenderTargetD3D9*>(aRenderTarget);
> > +  mCurrentRT->BindRenderTarget(device());
> > +  PrepareViewport(mCurrentRT->GetSize(), gfxMatrix());
> 
> This is an externally exposed function, should we be calling it ourselves
> here?
> 

I don't see why not. We do the same thing in both places, so it is that or code dup or a pointless internal version of the function.

> ::: gfx/layers/d3d9/TextureD3D9.cpp
> @@ +256,5 @@
> > +  }
> > +}
> > +
> > +IntRect
> > +TextureHostD3D9::GetTileRect(uint32_t aID) const
> 
> I wonder if we should share this code between Compositors.

Yeah, there is kind of a lot of code shared between d3d9 and d3d11. I wasn't really sure of the best way to share it, but we probably should.

Comment 31

[Nick Cameron [:nrc]]

Attachment: patch 9: repaint window on resize

Needed for OMTC d3d11 and d3d9 (and probably OMTC basic on Windows too).

Comment 32

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 778113 [details] [diff] [review]
patch 9: repaint window on resize

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

::: widget/windows/nsWindowGfx.cpp
@@ +37,5 @@
>  #include "nsIWidgetListener.h"
>  #include "mozilla/unused.h"
>  
>  #include "LayerManagerOGL.h"
> +#include "ClientLayerManager.h"

Do you need this?

Comment 33

[Nick Cameron [:nrc]]

Attachment: patch 10: implement CreateRenderTargetFromSource and clear surfaces before use

Comment 34

[Bas Schouten (:bas.schouten)]

Comment on attachment 778158 [details] [diff] [review]
patch 10: implement CreateRenderTargetFromSource and clear surfaces before use

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +47,5 @@
>    MOZ_ASSERT(aTexture);
>  
>    mTextures[0] = aTexture;
> +  HRESULT hr = mTextures[0]->GetSurfaceLevel(0, getter_AddRefs(mSurface));
> +  NS_ASSERTION(mSurface, "Couldn't create surface for texture");

Please change the message to 'get surface' instead of create, as this method does not 'create' the surface technically.

Is your intention to also assert SUCCEEDED(hr) here? right now this is going to result in an unused variable. (Might want to make it DebugOnly<> if it's just for the assert)

Comment 35

[Nick Cameron [:nrc]]

Attachment: patch 6 alternative: check that a layer's compositable is attached before use

An alternative for patch 6, checking that the compositable is valid, rather than notifying the layer when it becomes invalid. I'm not 100% sure this will work. It assumes that IPDL is not going to do anything nasty to the compositable when it is detached. It shouldn't delete because it is ref-counted, but I'm not sure if IPDL will delete it anyway (I think it should only delete the CompositableParent).

Comment 36

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dfc488f4a4c

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2dfc488f4a4c

Comment 38

[Nick Cameron [:nrc]]

Bas: is it OK to keep around a IDirect3DSurface9 as long as we keep around the texture from which we GetSurfaceLevel it? Or should we release it (null it out) when we release the DC we got from it with IDirect3DSurface9::GetDC?

Also still need review for test changes (trivial patch) and an opinion on which version of patch 6 you prefer.

Cheers!

Comment 39

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 772923 [details] [diff] [review]
patch 3: Fallback for EnsureAllocated and compositor, and some more sanity checks

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

::: gfx/layers/client/TextureClient.h
@@ +159,5 @@
>    ~TextureClientShmem() { ReleaseResources(); }
>  
>    virtual bool SupportsType(TextureClientType aType) MOZ_OVERRIDE
>    {
> +    return aType == TEXTURE_SHMEM || aType == TEXTURE_CONTENT || TEXTURE_FALLBACK;

aType == TEXTURE_FALLBACK?

This will always return true currently.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +860,4 @@
>  {
>    MOZ_ASSERT(aId != 0);
>  
>    nsRefPtr<LayerManager> lm = sCurrentCompositor->GetLayerManager();

This will need to be rebased, since sCurrentCompositor no longer exists.

Should be easy though.

Comment 40

[Bas Schouten (:bas.schouten)]

(In reply to Nick Cameron [:nrc] from comment #38)
> Bas: is it OK to keep around a IDirect3DSurface9 as long as we keep around
> the texture from which we GetSurfaceLevel it? Or should we release it (null
> it out) when we release the DC we got from it with IDirect3DSurface9::GetDC?

It's always okay, it holds a reference to its texture. Direct3D is sane (well, 9 is relatively sane :P)

> 
> Also still need review for test changes (trivial patch) and an opinion on
> which version of patch 6 you prefer.

I prefer the second iteration we did :-).

> 
> Cheers!

Comment 41

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas.schouten) from comment #40)
> (In reply to Nick Cameron [:nrc] from comment #38)
> > 
> > Also still need review for test changes (trivial patch) and an opinion on
> > which version of patch 6 you prefer.
> 
> I prefer the second iteration we did :-).
> 

Could you review that patch please? :-) It is 'patch 6 alternate:', the only one still needing a review from you. Thanks!

Comment 42

[Bas Schouten (:bas.schouten)]

Comment on attachment 778257 [details] [diff] [review]
patch 6 alternative: check that a layer's compositable is attached before use

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

::: gfx/layers/composite/CanvasLayerComposite.cpp
@@ +32,5 @@
>  
>    CleanupResources();
>  }
>  
> +void CanvasLayerComposite::SetCompositableHost(CompositableHost* aHost)

nit: If you're fixing this hunk might as well add the missing newline after void as well :).

Comment 43

[Nick Cameron [:nrc]]

GREEN ON TRY!!!!!!!!!!!

https://tbpl.mozilla.org/?tree=Try&rev=13fe586fd6f0

Comment 44

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 773028 [details] [diff] [review]
patch 8: Re-fix WebGL

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

Is this still needed with bug 897409?

I guess it might perform better than doing a copy, but the naming should reflect that it's not required.

::: gfx/layers/CopyableCanvasLayer.h
@@ +39,5 @@
>    }
>  
>    virtual void Initialize(const Data& aData);
>    
> +  virtual bool RequiresImageSurface() MOZ_OVERRIDE

PrefersImageSurface?

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +485,5 @@
>    }
>  
> +  if (aFlags & RequiresImageSurface) {
> +    mIsOpaque = false;
> +  }

This is a non-obvious way to ensure that we take the gfxImageSurface path, better naming or some asserts would help.

Comment 45

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #44)
> Comment on attachment 773028 [details] [diff] [review]
> patch 8: Re-fix WebGL
> 
> Review of attachment 773028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this still needed with bug 897409?
> 

I believe so, this patch was applied when I found 897409, but I will double check.

Comment 46

[Nick Cameron [:nrc]]

(In reply to Nick Cameron [:nrc] from comment #45)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #44)
> > Comment on attachment 773028 [details] [diff] [review]
> > patch 8: Re-fix WebGL
> > 
> > Review of attachment 773028 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Is this still needed with bug 897409?
> > 
> 
> I believe so, this patch was applied when I found 897409, but I will double
> check.

This is unnecessary after the new version of the patch in bug 897409. I wonder if it is worth doing all this work to avoid the copies? It probably is, although I guess d3d must be doing something similar somewhere under the hood.

Bas: what do you reckon - is it worth a bit of effort to do a LockRect instead of creating a temp surface and copying back to the texture?

Comment 47

[Bas Schouten (:bas.schouten)]

(In reply to Nick Cameron [:nrc] from comment #46)
> (In reply to Nick Cameron [:nrc] from comment #45)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #44)
> > > Comment on attachment 773028 [details] [diff] [review]
> > > patch 8: Re-fix WebGL
> > > 
> > > Review of attachment 773028 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Is this still needed with bug 897409?
> > > 
> > 
> > I believe so, this patch was applied when I found 897409, but I will double
> > check.
> 
> This is unnecessary after the new version of the patch in bug 897409. I
> wonder if it is worth doing all this work to avoid the copies? It probably
> is, although I guess d3d must be doing something similar somewhere under the
> hood.
> 
> Bas: what do you reckon - is it worth a bit of effort to do a LockRect
> instead of creating a temp surface and copying back to the texture?

That depends on what you mean here, I'm unsure about exactly what you mean by the two approaches you're describing :).

Comment 48

[Matt Woodrow (:mattwoodrow)]

Reading back from webgl requires a surface that has direct access to the bits.

Our current code will allocate a temporary image surface, do GL readback into that, and then use cairo/thebes to paint the temporary to the windows device context for the texture.

The alternative is to do a LockRect on the d3d9 texture to get the pixels, and do readback directly into that.

Comment 49

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #48)
> Reading back from webgl requires a surface that has direct access to the
> bits.
> 
> Our current code will allocate a temporary image surface, do GL readback
> into that, and then use cairo/thebes to paint the temporary to the windows
> device context for the texture.
> 
> The alternative is to do a LockRect on the d3d9 texture to get the pixels,
> and do readback directly into that.

Which D3D9 texture are we talking about here? A D3DUSAGE_DYNAMIC texture that we're actually doing the drawing to? Or a D3DPOOL_SYSTEMMEM texture we've already read back to?

Comment 50

[Matt Woodrow (:mattwoodrow)]

It's D3DPOOL_SYSTEMMEM, but I'm not sure what the second part of your question meant.

We create the texture, then we want to update it with new content from WebGL each frame.

We can call LockRect on that texture, tell GL to read pixels into that memory and then unlock.

Or we can readback into a temporary gfxImageSurface, get the DC for the texture, and paint the temporary into the DC using thebes.

Comment 51

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #50)
> It's D3DPOOL_SYSTEMMEM, but I'm not sure what the second part of your
> question meant.
> 
> We create the texture, then we want to update it with new content from WebGL
> each frame.
> 
> We can call LockRect on that texture, tell GL to read pixels into that
> memory and then unlock.

So what mechanism do you use to tell GL to read back into that now?

Since I assume none of this is actual GL but rather all ANGLE on D3D9 it all depends on how it's being implemented under the hood.

> Or we can readback into a temporary gfxImageSurface, get the DC for the
> texture, and paint the temporary into the DC using thebes.

How do you 'readback into' do you mean a glReadPixels that runs on ANGLE on D3D9?

In reality on D3D9 the right way to do what you're trying to do is to use http://msdn.microsoft.com/en-us/library/windows/desktop/bb174405%28v=vs.85%29.aspx (unless we're doing multisampling). You use that to get your data into a SYSTEMMEM texture, after that you ideally just use a LockRect on that SYSTEMMEM texture.

Comment 52

[Nick Cameron [:nrc]]

We discussed the WebGL canvas on irc and did some experiments. Currently for d3d9 we get readback in any case due to getting a basic source surface. A long with the improvements from Bug 897409, there is no improvement in having image surfaces from the texture client. So patch 8 is much reduced. Carrying r=mattwoodrow.

Comment 53

[Nick Cameron [:nrc]]

Attachment: patch 2: compositor and texture client/hosts

Comment 54

[Nick Cameron [:nrc]]

Attachment: patch 3: Fallback for EnsureAllocated and compositor. More sanity checks.

Comment 55

[Nick Cameron [:nrc]]

Attachment: patch 4: identify multiprocess OMTC and use shmem

Comment 56

[Nick Cameron [:nrc]]

Attachment: patch 5: large canvases

Comment 57

[Nick Cameron [:nrc]]

Attachment: patch 6: check the a layer's compositable is attached before use

Comment 58

[Nick Cameron [:nrc]]

Attachment: patch 7: fuzz tests

Comment 59

[Nick Cameron [:nrc]]

Attachment: patch 8: fix for webgl

Comment 60

[Nick Cameron [:nrc]]

Attachment: patch 10: implement CreateRenderTargetFromSource and clear surfaces before use

Comment 61

[Nick Cameron [:nrc]]

Patches are rebased and adjusted for reviewers comments. Carrying r=Bas or mattwoodrow

Comment 62

[Nick Cameron [:nrc]]

(In reply to Nick Cameron [:nrc] from comment #43)
> GREEN ON TRY!!!!!!!!!!!
> 
> https://tbpl.mozilla.org/?tree=Try&rev=13fe586fd6f0

Just leaving this here, although it is a few days old.

Comment 63

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff6b2c79b2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/800c42ff8a39
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc2fe96748c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1894d7ef6805
https://hg.mozilla.org/integration/mozilla-inbound/rev/db732bedb72a
https://hg.mozilla.org/integration/mozilla-inbound/rev/030dde7c4d86
https://hg.mozilla.org/integration/mozilla-inbound/rev/575d8f92093c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed14faa7107

Comment 64

[Nick Cameron [:nrc]]

Backed out for Talos orange, possibly needs a clobber, possibly conflicts with some of the new texture host stuff. In any case, backed out for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbcffb6957ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/76eced21cb7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c27f63e03ad7

Comment 65

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=168406c98cd8

Comment 66

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6962b015a6a

Comment 67

[Nick Cameron [:nrc]]

backed out an incorrect change from that patch (warning -> assertion): https://hg.mozilla.org/integration/mozilla-inbound/rev/92259ea31bb4

Comment 68

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 784196 [details] [diff] [review]
patch 2: compositor and texture client/hosts

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +527,5 @@
> +    }
> +    NS_ASSERTION(mDC, "We need a DC here");
> +    mSurface = new gfxWindowsSurface(mDC);
> +  } else {
> +    // d3d9 SYSTEMMEM surfaces do not support GDI with an alpha channel

Are you sure this is true and how do you know?

@@ +537,5 @@
> +
> +      mSurface = new gfxImageSurface(reinterpret_cast<unsigned char*>(lockedRect.pBits),
> +                                     gfxIntSize(mSize.width, mSize.height),
> +                                     lockedRect.Pitch,
> +                                     gfxASurface::ImageFormatARGB32);

Perhaps this should be creating a DIBSection backed gfxWindowSurface() and not a gfxImageSurface()? What do we currently do?

Comment 69

[Nick Cameron [:nrc]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #68)
> Comment on attachment 784196 [details] [diff] [review]
> patch 2: compositor and texture client/hosts
> 
> Review of attachment 784196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d9/TextureD3D9.cpp
> @@ +527,5 @@
> > +    }
> > +    NS_ASSERTION(mDC, "We need a DC here");
> > +    mSurface = new gfxWindowsSurface(mDC);
> > +  } else {
> > +    // d3d9 SYSTEMMEM surfaces do not support GDI with an alpha channel
> 
> Are you sure this is true and how do you know?
> 

The comment is a bit vague, sorry. Technically, we can't get a DC from the d3d9 surface if it has alpha, see http://msdn.microsoft.com/en-us/library/windows/desktop/bb205894%28v=vs.85%29.aspx

> @@ +537,5 @@
> > +
> > +      mSurface = new gfxImageSurface(reinterpret_cast<unsigned char*>(lockedRect.pBits),
> > +                                     gfxIntSize(mSize.width, mSize.height),
> > +                                     lockedRect.Pitch,
> > +                                     gfxASurface::ImageFormatARGB32);
> 
> Perhaps this should be creating a DIBSection backed gfxWindowSurface() and
> not a gfxImageSurface()? What do we currently do?

We currently create a gfxWindowSurface and then copy that to the texture using an intermediate image surface. I believe (now) that that is a better approach and it is on my list to change to doing things the old way, but it is kind of low priority for now.

Comment 70

[Nick Cameron [:nrc]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #68)
> Comment on attachment 784196 [details] [diff] [review]
> patch 2: compositor and texture client/hosts
> 
> Review of attachment 784196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d9/TextureD3D9.cpp
> @@ +527,5 @@
> > +    }
> > +    NS_ASSERTION(mDC, "We need a DC here");
> > +    mSurface = new gfxWindowsSurface(mDC);
> > +  } else {
> > +    // d3d9 SYSTEMMEM surfaces do not support GDI with an alpha channel
> 
> Are you sure this is true and how do you know?
> 
> @@ +537,5 @@
> > +
> > +      mSurface = new gfxImageSurface(reinterpret_cast<unsigned char*>(lockedRect.pBits),
> > +                                     gfxIntSize(mSize.width, mSize.height),
> > +                                     lockedRect.Pitch,
> > +                                     gfxASurface::ImageFormatARGB32);
> 
> Perhaps this should be creating a DIBSection backed gfxWindowSurface() and
> not a gfxImageSurface()? What do we currently do?

Filed bug 912766

Comment 71

[Nick Cameron [:nrc]]

This should have been resolved as fixed a while back. The implementation is finished. Actually turning it on is bug 899787.