Closed Bug 874721 Opened 6 years ago Closed 6 years ago

Implement CompositorD3D9

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: nrc, Assigned: nrc)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 18 obsolete files)

9.03 KB, patch
bas.schouten
: review+
nrc
: checkin+
Details | Diff | Splinter Review
1.27 KB, patch
mattwoodrow
: review+
nrc
: checkin+
Details | Diff | Splinter Review
107.24 KB, patch
nrc
: review+
Details | Diff | Splinter Review
31.74 KB, patch
nrc
: review+
Details | Diff | Splinter Review
8.51 KB, patch
nrc
: review+
Details | Diff | Splinter Review
1.24 KB, patch
nrc
: review+
Details | Diff | Splinter Review
10.65 KB, patch
nrc
: review+
Details | Diff | Splinter Review
7.59 KB, patch
nrc
: review+
Details | Diff | Splinter Review
3.09 KB, patch
nrc
: review+
Details | Diff | Splinter Review
7.63 KB, patch
nrc
: review+
Details | Diff | Splinter Review
No description provided.
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).
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.
Bas, maybe you could look at Nick's code and see if you can see anything?

Nick, can you attach a patch?
Attached patch WIP patch (obsolete) — Splinter Review
Bas: do you see anything obviously wrong with the solid colour rendering? (Just ignore all the wrong stuff elsewhere for now :-p )
Flags: needinfo?(bas)
Attached patch preliminary (obsolete) — Splinter Review
Attached patch WIP patch (obsolete) — Splinter Review
rebased
Attachment #763291 - Attachment is obsolete: true
Diagnosed problem and sent solution on IRC, caused by uninitialized memory in renderTargetOffset.
Flags: needinfo?(bas)
Attached patch unify mask typesSplinter Review
Attachment #763905 - Attachment is obsolete: true
Attachment #769635 - Flags: review?(bas)
Attachment #763906 - Attachment is obsolete: true
Attachment #769636 - Flags: review?(bas)
Attachment #769635 - Flags: review?(bas) → review+
Attachment #769636 - Attachment is obsolete: true
Attachment #769636 - Flags: review?(bas)
Attachment #772920 - Flags: review?(bas)
Corrected a few minor errors
Attachment #772925 - Flags: review?(bas)
Attachment #772926 - Flags: review?(bas)
Attached patch patch 7: fuzz some tests (obsolete) — Splinter Review
Attachment #772930 - Flags: review?(bas)
Attached patch patch 8: Re-fix WebGL (obsolete) — Splinter Review
Attachment #772931 - Flags: review?(bas)
Attachment #772928 - Attachment is obsolete: true
Attachment #772928 - Flags: review?(bas)
Attachment #772999 - Flags: review?(bas)
Attached patch patch 7: fuzz some tests (obsolete) — Splinter Review
Attachment #772930 - Attachment is obsolete: true
Attachment #772930 - Flags: review?(bas)
Attachment #773000 - Flags: review?(bas)
Attached patch patch 8: Re-fix WebGL (obsolete) — Splinter Review
Attachment #772931 - Attachment is obsolete: true
Attachment #772931 - Flags: review?(bas)
Attachment #773028 - Flags: review?(bas)
Attached patch patch 7: fuzz some tests (obsolete) — Splinter Review
Attachment #773000 - Attachment is obsolete: true
Attachment #773000 - Flags: review?(bas)
Attachment #773083 - Flags: review?(bas)
Attached patch patch 7: fuzz some tests (obsolete) — Splinter Review
Attachment #773083 - Attachment is obsolete: true
Attachment #773083 - Flags: review?(bas)
Attachment #773194 - Flags: review?
Attachment #773194 - Flags: review? → review?(bas)
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.
Attachment #772920 - Flags: review?(bas) → review+
Attachment #772925 - Flags: review?(bas) → review+
Attachment #772926 - Flags: review?(bas) → review+
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?
(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.
(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.
Blocks: 886892
Attachment #772999 - Flags: review?(bas) → review+
Status: NEW → ASSIGNED
Attachment #769635 - Flags: checkin+
Needed for OMTC d3d11 and d3d9 (and probably OMTC basic on Windows too).
Attachment #778113 - Flags: review?(matt.woodrow)
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?
Attachment #778113 - Flags: review?(matt.woodrow) → review+
Attachment #778158 - Flags: review? → review?(bas)
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)
Attachment #778158 - Flags: review?(bas) → review+
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).
Attachment #778257 - Flags: review?(bas)
Attachment #778113 - Flags: checkin+
Depends on: 896896
Attachment #772923 - Flags: review?(bas) → review?(matt.woodrow)
Attachment #773028 - Flags: review?(bas) → review?(matt.woodrow)
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!
Flags: needinfo?(bas)
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.
Attachment #772923 - Flags: review?(matt.woodrow) → review+
(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!
Flags: needinfo?(bas)
Attachment #773194 - Flags: review?(bas) → review+
(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!
Flags: needinfo?(bas)
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 :).
Attachment #778257 - Flags: review?(bas) → review+
Flags: needinfo?(bas)
Depends on: 899435
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.
Attachment #773028 - Flags: review?(matt.woodrow) → review+
(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.
Blocks: 899787
Blocks: 899435
No longer depends on: 899435
Version: 21 Branch → Trunk
(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?
Flags: needinfo?(bas)
(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 :).
Flags: needinfo?(bas)
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.
Flags: needinfo?(bas)
(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?
Flags: needinfo?(bas)
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.
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
Blocks: 900244
Blocks: 900245
Blocks: 900248
Blocks: 900249
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.
Whiteboard: [leave open]
Attachment #772920 - Attachment is obsolete: true
Attachment #784196 - Flags: review+
Attachment #772925 - Attachment is obsolete: true
Attachment #784200 - Flags: review+
Attachment #772926 - Attachment is obsolete: true
Attachment #784201 - Flags: review+
Attachment #772999 - Attachment is obsolete: true
Attachment #778257 - Attachment is obsolete: true
Attachment #784202 - Flags: review+
Attachment #773194 - Attachment is obsolete: true
Attachment #784203 - Flags: review+
Attachment #773028 - Attachment is obsolete: true
Attachment #784204 - Flags: review+
Patches are rebased and adjusted for reviewers comments. Carrying r=Bas or mattwoodrow
(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.
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
backed out an incorrect change from that patch (warning -> assertion): https://hg.mozilla.org/integration/mozilla-inbound/rev/92259ea31bb4
Depends on: 902929
Depends on: 902339
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?
(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.
(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
This should have been resolved as fixed a while back. The implementation is finished. Actually turning it on is bug 899787.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.