Similar to bug 593604. A bit more tricky because we need to hack cairo-d2d-surface to let us composite subpixel-AA text onto an RGBA surface. But also a bit less tricky because bug 593604 and friends creates all the Gecko infrastructure that's needed.

Comment on attachment 493606 [details] [diff] [review] Make cairo_tee_surface support flush() You need to flush 'master' as well

I suggest getting some Talos results from tryserver before landing, this could potentially regress performance.

Rebased against 736fd3b7b3fe

Fixed review comment.

I still have one try server failure with this patch, bugs/602200-4.html. This fails because the call to IDirect3D10Texture::Map inside LayerManagerD3D10::PaintToTarget returns DXGI_ERROR_DEVICE_REMOVED. The value of ID3D10Device::GetDeviceRemovedReason is also DXGI_ERROR_DEVICE_REMOVED. I assume this is a driver crash, not sure how we can handle it though. Seems to be fine on my local hardware (ATI Radeon HD 4850).

(In reply to comment #11) > Created attachment 502182 [details] [diff] [review] > Part 4: Make ThebesLayerD3D10 support component alpha > > I still have one try server failure with this patch, bugs/602200-4.html. > > This fails because the call to IDirect3D10Texture::Map inside > LayerManagerD3D10::PaintToTarget returns DXGI_ERROR_DEVICE_REMOVED. The value > of ID3D10Device::GetDeviceRemovedReason is also DXGI_ERROR_DEVICE_REMOVED. > > I assume this is a driver crash, not sure how we can handle it though. Seems to > be fine on my local hardware (ATI Radeon HD 4850). The right thing to do if this is consistently crashing on this test, is figure out what it might be doing that no other test is doing. If it's just this test it's actually an excellent opportunity to localize a driver bug.

Filed bug 626285 to help debug this driver crash. Could we consider disabling this test temporarily until the issue is resolved?

(In reply to comment #14) > Filed bug 626285 to help debug this driver crash. > > Could we consider disabling this test temporarily until the issue is resolved? I think we should just get you this machine quickly (it shouldn't have to take long) and nail the root cause. We've no idea what other kind of issues this may cause on NVidia hardware in the wild.

We need to subtract the previous offset, not add it. Looks like this fixed the crash and only reftest failure.

Comment on attachment 502180 [details] [diff] [review] Part 3: Add Component Alpha shaders to D3D10 ># HG changeset patch ># Parent dadce2ff9465a502a64253004d0e0d19d1e3530a ># User Matt Woodrow <mwoodrow@mozilla.com> > >diff --git a/gfx/layers/d3d10/LayerManagerD3D10.fx b/gfx/layers/d3d10/LayerManagerD3D10.fx >--- a/gfx/layers/d3d10/LayerManagerD3D10.fx >+++ b/gfx/layers/d3d10/LayerManagerD3D10.fx >@@ -36,26 +36,40 @@ BlendState NonPremul > DestBlend = Inv_Src_Alpha; > BlendOp = Add; > SrcBlendAlpha = One; > DestBlendAlpha = Inv_Src_Alpha; > BlendOpAlpha = Add; > RenderTargetWriteMask[0] = 0x0F; // All > }; > >+BlendState ComponentAlphaBlend >+{ >+ AlphaToCoverageEnable = FALSE; >+ BlendEnable[0] = TRUE; >+ SrcBlend = One; >+ DestBlend = Inv_Src1_Color; >+ BlendOp = Add; >+ SrcBlendAlpha = One; >+ DestBlendAlpha = Inv_Src_Alpha; >+ BlendOpAlpha = Add; >+ RenderTargetWriteMask[0] = 0x0F; // All >+}; >+ > RasterizerState LayerRast > { > ScissorEnable = True; > CullMode = None; > }; > > Texture2D tRGB; > Texture2D tY; > Texture2D tCb; > Texture2D tCr; >+Texture2D tRGBWhite; > > SamplerState LayerTextureSamplerLinear > { > Filter = MIN_MAG_MIP_LINEAR; > AddressU = Wrap; > AddressV = Wrap; > }; > >@@ -70,16 +84,21 @@ struct VS_INPUT { > float2 vPosition : POSITION; > }; > > struct VS_OUTPUT { > float4 vPosition : SV_Position; > float2 vTexCoords : TEXCOORD0; > }; > >+struct PS_OUTPUT { >+ float4 vSrc; >+ float4 vAlpha; >+}; >+ > VS_OUTPUT LayerQuadVS(const VS_INPUT aVertex) > { > VS_OUTPUT outp; > outp.vPosition.z = 0; > outp.vPosition.w = 1; > > // We use 4 component floats to uniquely describe a rectangle, by the structure > // of x, y, width, height. This allows us to easily generate the 4 corners >@@ -142,16 +161,31 @@ float4 YCbCrShader(const VS_OUTPUT aVert > color.r = yuv.g * 1.164 + yuv.r * 1.596; > color.g = yuv.g * 1.164 - 0.813 * yuv.r - 0.391 * yuv.b; > color.b = yuv.g * 1.164 + yuv.b * 2.018; > color.a = 1.0f; > > return color * fLayerOpacity; > } > >+PS_OUTPUT ComponentAlphaShader(const VS_OUTPUT aVertex) : SV_Target >+{ >+ PS_OUTPUT result; >+ float4 src; >+ float4 alphas; >+ >+ src = tRGB.Sample(LayerTextureSamplerLinear, aVertex.vTexCoords); >+ alphas = 1.0 - tRGBWhite.Sample(LayerTextureSamplerLinear, aVertex.vTexCoords) + src; >+ src.a = alphas.g; >+ alphas.a = alphas.g; You're not using SRC1_ALPHA so this line could go. What you could do is: >+ alphas = 1.0 - tRGBWhite.Sample(LayerTextureSamplerLinear, aVertex.vTexCoords).rgbg + src.rgbg; and then change the DestBlendAlpha to Inv_Src1_Alpha to make it real easy for the compiler. In any case using Inv_Src1_Alpha for the DestAlphaBlend and removing the src.a = alphas.g is probably the best idea since that will at least allow the compiler to make this optimization. >+ result.vSrc = src * fLayerOpacity; >+ result.vAlpha = alphas * fLayerOpacity; The HLSL compiler should be able to figure it out if you assign to result.vSrc and result.vAlpha directly and do a *=.

Comment on attachment 502182 [details] [diff] [review] Part 4: Make ThebesLayerD3D10 support component alpha In general this looks fine. I'd like some minor changes though and another quick review pass after those are made. >diff --git a/gfx/layers/d3d10/ThebesLayerD3D10.cpp b/gfx/layers/d3d10/ThebesLayerD3D10.cpp >--- a/gfx/layers/d3d10/ThebesLayerD3D10.cpp >+++ b/gfx/layers/d3d10/ThebesLayerD3D10.cpp >+ThebesLayerD3D10::VerifyContentType(SurfaceMode aMode) > { > if (mD2DSurface) { >- gfxASurface::gfxContentType type = CanUseOpaqueSurface() ? >+ gfxASurface::gfxContentType type = aMode != SURFACE_SINGLE_CHANNEL_ALPHA ? > gfxASurface::CONTENT_COLOR : gfxASurface::CONTENT_COLOR_ALPHA; > >- if (type != mD2DSurface->GetContentType()) { >- mD2DSurface = new gfxD2DSurface(mTexture, type); >- >- if (!mD2DSurface || mD2DSurface->CairoStatus()) { >- NS_WARNING("Failed to create surface for ThebesLayerD3D10."); >- mD2DSurface = nsnull; >- return; >- } >+ if (type != mD2DSurface->GetContentType() || >+ (aMode == SURFACE_COMPONENT_ALPHA && !mTextureOnWhite) || >+ (aMode != SURFACE_COMPONENT_ALPHA && mTextureOnWhite)) { >+ // Incorrect formats, dump everything and let it be recreated >+ mD2DSurface = nsnull; >+ mD2DSurfaceOnWhite = nsnull; >+ mSRView = nsnull; >+ mSRViewOnWhite = nsnull; >+ mTexture = nsnull; >+ mTextureOnWhite = nsnull; In the passed if we switched from opaque to transparent we re-used the mTexture. This is faster. We should continue doing that, i.e. never toss out mTexture unless our visible region changes sizes. This texture is identical in all layers other than size. >+static void >+FillSurface(gfxASurface* aSurface, const nsIntRegion& aRegion, >+ const nsIntPoint& aOffset, const gfxRGBA& aColor) >+{ >+ nsRefPtr<gfxContext> ctx = new gfxContext(aSurface); >+ ctx->Translate(-gfxPoint(aOffset.x, aOffset.y)); >+ gfxUtils::ClipToRegion(ctx, aRegion); >+ ctx->SetColor(aColor); >+ ctx->Paint(); This is slow on D2D, please draw the region and then use a Fill(); call, avoid clipping.

Comment on attachment 504569 [details] [diff] [review] Part 5: Make ContainerLayerD3D10 support component alpha v2 >@@ -183,38 +194,78 @@ ContainerLayerD3D10::RenderLayer() >+ // If we have an opaque ancestor layer, then we can be sure that >+ // all the pixels we draw into are either opaque already or will be >+ // covered by something opaque. Otherwise copying up the background is >+ // not safe. >+ if (HasOpaqueAncestorLayer(this) && >+ transform3D.Is2D(&transform) && !transform.HasNonIntegerTranslation()) { >+ // Copy background up from below >+ D3D10_BOX srcBox; >+ srcBox.left = visibleRect.x + PRInt32(transform.x0) - PRInt32(previousRenderTargetOffset[0]); >+ srcBox.top = visibleRect.y + PRInt32(transform.y0) - PRInt32(previousRenderTargetOffset[1]); >+ srcBox.right = srcBox.left + visibleRect.width; >+ srcBox.bottom = srcBox.top + visibleRect.height; >+ srcBox.back = 1; >+ srcBox.front = 0; >+ >+ nsRefPtr<ID3D10Resource> destResource; >+ nsRefPtr<ID3D10Resource> srcResource; You don't need destResource here, you can just use renderTexture, it inherits from ID3D10Resource. >+ >+ rtView->GetResource(getter_AddRefs(destResource)); >+ previousRTView->GetResource(getter_AddRefs(srcResource)); >+ >+ device()->CopySubresourceRegion(destResource, 0, >+ 0, 0, 0, >+ srcResource, 0, >+ &srcBox); We should be careful here, in theory the parent layer mVisibleRegion is allowed to be smaller than mVisibleRegion for this layer (mVisibleRegion is allowed to be an overestimate of the true visible region). We should probably check for that or risk touching a non existing source area.

As discussed on IRC, we believe this should block.

Fixed review suggestions, and added optimization discussed in irc.

Fixed review comments.

Fixed review suggestions. Carrying forward r=Bas

Only recreate mTexture when the size has changed.

http://hg.mozilla.org/mozilla-central/rev/df6f10906476 http://hg.mozilla.org/mozilla-central/rev/6c285cfcedf4 http://hg.mozilla.org/mozilla-central/rev/69e4f4ecbdc2 http://hg.mozilla.org/mozilla-central/rev/aeb9104f1534 http://hg.mozilla.org/mozilla-central/rev/89c2c85429fb

This was backed out in <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=9b6a73bdb237>. I'm not sure why the bug was not reopened.

Fixed bug exposed by empty transactions. Carrying forward r=Bas

Only call flush if the backend supports it. Carrying forward r=roc.

I still have one failure on tryserver (4 crashtests and 1 reftest) where VerifyContentType is clearing our mValidRegion. http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mwoodrow@mozilla.com-0fe73c7e1223/try-w32-dbg/tryserver_win7-debug_test-crashtest-build291.txt.gz http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mwoodrow@mozilla.com-0fe73c7e1223/try-w32-dbg/tryserver_win7-debug_test-reftest-build301.txt.gz The content type is changing from CONTENT_COLOR_ALPHA to CONTENT_COLOR. I haven't been able to find time to debug this and I'm going away for a week, I would really appreciate if someone could try fix this and get this landed. The more time in nightlies the better for a large change like this.

As far as I can tell GetSurfacdMode() changes from returning SURFACE_OPAQUE on the previous paint to SURFACE_COMPONENT_ALPHA when the assertion hits.

Sorry to ask this here, but what kind of benefits is had by these patches? and what makes those benefits qualify as a blocker/hardblocker?

Bugs like 594325 will be fixed.

So, the problem here was that mSupportsComponentAlphaChildren was set at the time of RenderLayer. Validate, however, in D3D10, is called for the tree -before- then. So whereas mSupportsComponentAlphaChildren would always be PR_FALSE on the -first- transaction. Any subsequent transactions with an identical layer tree would have it set as expected. I've changed the patch to determine mSupportsComponentAlphaChildren at validation time.

Comment on attachment 507508 [details] [diff] [review] Part 5: Make ContainerLayerD3D10 support component alpha v4 >- float black[] = { 0, 0, 0, 0}; >- device()->ClearRenderTargetView(rtView, black); >+ effect()->GetVariableByName("vRenderTargetOffset")-> >+ GetRawValue(previousRenderTargetOffset, 0, 8); >+ >+ if (mVisibleRegion.GetNumRects() == 1 && (GetContentFlags() & CONTENT_OPAQUE)) { >+ // don't need a background, we're going to paint all opaque stuff >+ mSupportsComponentAlphaChildren = PR_TRUE; >+ } else { >+ const gfx3DMatrix& transform3D = GetEffectiveTransform(); >+ gfxMatrix transform; >+ // If we have an opaque ancestor layer, then we can be sure that >+ // all the pixels we draw into are either opaque already or will be >+ // covered by something opaque. Otherwise copying up the background is >+ // not safe. >+ if (mSupportsComponentAlphaChildren) { >+#ifdef DEBUG >+ bool is2d = >+#endif >+ transform3D.Is2D(&transform); >+ NS_ASSERTION(is2d, "Transform should be 2d when mSupportsComponentAlphaChildren."); This is a very weird idiom. Splitting the declaration outside an #ifdef is really ugly. Also, why does Is2D do two things? Should it be split into two functions? >+ >+ // Copy background up from below >+ D3D10_BOX srcBox; >+ srcBox.left = visibleRect.x + PRInt32(transform.x0) - PRInt32(previousRenderTargetOffset[0]); >+ srcBox.top = visibleRect.y + PRInt32(transform.y0) - PRInt32(previousRenderTargetOffset[1]); >+ srcBox.right = srcBox.left + visibleRect.width; >+ srcBox.bottom = srcBox.top + visibleRect.height; >+ srcBox.back = 1; >+ srcBox.front = 0; Please add a comment about what srcBox is and where this calculation comes from. > previousViewportSize = mD3DManager->GetViewport(); > mD3DManager->SetViewport(nsIntSize(visibleRect.Size())); > } else { > #ifdef DEBUG > PRBool is2d = > #endif > GetEffectiveTransform().Is2D(&contTransform); > NS_ASSERTION(is2d, "Transform must be 2D"); > } > >+ extra white space > void > ContainerLayerD3D10::Validate() > { >+ nsIntRect visibleRect = mVisibleRegion.GetBounds(); >+ >+ mSupportsComponentAlphaChildren = PR_FALSE; >+ PRBool useIntermediate = UseIntermediateSurface(); >+ >+ if (useIntermediate) { if (UseIntermediateSurface()) {

You don't want to separate "get 2D matrix" from "is 2D matrix", because you should never do the first without the second (and you almost never want to do the second without the first).

Parsed review comments and removed some redundant code. Reftests had issues because of bug 629416. Pulled and pushed another set to try.

Comment on attachment 507629 [details] [diff] [review] Part 5: Make ContainerLayerD3D10 support component alpha v5 >+ if (mSupportsComponentAlphaChildren) { >+ bool is2d = transform3D.Is2D(&transform); You use PRBool in one place and bool in the other. I like bool better, but consistency is nice too. Make a call

http://hg.mozilla.org/mozilla-central/rev/bbee61de7173 http://hg.mozilla.org/mozilla-central/rev/99c20fdff1c2 http://hg.mozilla.org/mozilla-central/rev/4b4ea122f423 http://hg.mozilla.org/mozilla-central/rev/b064de51203d http://hg.mozilla.org/mozilla-central/rev/8bb432a920fe

Comment on attachment 506075 [details] [diff] [review] Part 2: Make cairo_tee_surface support flush() >diff --git a/gfx/cairo/cairo/src/cairo-surface-wrapper.c b/gfx/cairo/cairo/src/cairo-surface-wrapper.c >--- a/gfx/cairo/cairo/src/cairo-surface-wrapper.c >+++ b/gfx/cairo/cairo/src/cairo-surface-wrapper.c >@@ -519,8 +519,16 @@ _cairo_surface_wrapper_init (cairo_surfa > wrapper->target = cairo_surface_reference (target); > } > > void > _cairo_surface_wrapper_fini (cairo_surface_wrapper_t *wrapper) > { > cairo_surface_destroy (wrapper->target); > } >+ >+cairo_status_t >+_cairo_surface_wrapper_flush (cairo_surface_wrapper_t *wrapper) >+{ >+ if (wrapper->target->backend->flush) { >+ return wrapper->target->backend->flush(wrapper->target); >+ } >+} This looks broken. Compilers are rightfully complaining about it since return value may be undefined. Should I open a new bugreport about it?

(In reply to comment #39) > Comment on attachment 506075 [details] [diff] [review] > Part 2: Make cairo_tee_surface support flush() > > >diff --git a/gfx/cairo/cairo/src/cairo-surface-wrapper.c b/gfx/cairo/cairo/src/cairo-surface-wrapper.c > >--- a/gfx/cairo/cairo/src/cairo-surface-wrapper.c > >+++ b/gfx/cairo/cairo/src/cairo-surface-wrapper.c > >@@ -519,8 +519,16 @@ _cairo_surface_wrapper_init (cairo_surfa > > wrapper->target = cairo_surface_reference (target); > > } > > > > void > > _cairo_surface_wrapper_fini (cairo_surface_wrapper_t *wrapper) > > { > > cairo_surface_destroy (wrapper->target); > > } > >+ > >+cairo_status_t > >+_cairo_surface_wrapper_flush (cairo_surface_wrapper_t *wrapper) > >+{ > >+ if (wrapper->target->backend->flush) { > >+ return wrapper->target->backend->flush(wrapper->target); > >+ } > >+} > > > This looks broken. Compilers are rightfully complaining about it since return > value may be undefined. > Should I open a new bugreport about it? Looks like a problem to me! Please do.