Eventually we want to convert most instances of PushGroup/PopGroup to a PushGroup/PopGroup API in Moz2D which will not feature the ability to pop to a separate pattern or surface. Users which do not need other functionality can already be converted to a similar API on gfxContext.

This is more involved SVG changes, would like review from Jwatt as well.

Comment on attachment 8671548 [details] [diff] [review] Part 2: Convert some simple users to PushGroupForBlendBack. Review of attachment 8671548 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicLayerManager.cpp @@ +108,5 @@ > + // If the layer is opaque in its visible region we can push a gfxContentType::COLOR > + // group. We need to make sure that only pixels inside the layer's visible > + // region are copied back to the destination. Remember if we've already > + // clipped precisely to the visible region. > + *aNeedsClipToVisibleRegion = !didCompleteClip || aRegion.GetNumRects() > 1; Seems like this comment and the *aNeedsClipToVisibleRegion could be lifted out. ::: layout/svg/nsSVGPatternFrame.cpp @@ +416,5 @@ > > patternWithChildren->mSource = nullptr; > > if (aGraphicOpacity != 1.0f) { > + gfx->PopGroupAndBlend(); The opacity is ignored here.

Did you mean to r- that v2?

Fix a transform mistake and a use-after-free.

Comment on attachment 8671722 [details] [diff] [review] Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v2 Review of attachment 8671722 [details] [diff] [review]: ----------------------------------------------------------------- Why doesn't this code PushClip for clipping to the ClipPath?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8) > Comment on attachment 8671722 [details] [diff] [review] > Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and > temporary surfaces. v2 > > Review of attachment 8671722 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why doesn't this code PushClip for clipping to the ClipPath? Trivial clips already work through applying it to the context. For other cases I just tried to stick as close to what's currently happening as possible.

I think I sort of found out my answer. As I understand it, we need to be able to construct the clip path using an arbitrary combination of unions and intersections of paths. e.g. (A and B) or C

Comment on attachment 8671722 [details] [diff] [review] Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v2 Review of attachment 8671722 [details] [diff] [review]: ----------------------------------------------------------------- I've only quickly looked this over. jwatt should do a more thorough review. ::: layout/svg/nsSVGUtils.cpp @@ +711,5 @@ > } > > + if (aFrame->StyleDisplay()->mMixBlendMode != NS_STYLE_BLEND_NORMAL) { > + RefPtr<DrawTarget> targetDT = target->GetDrawTarget(); > + target = &aContext; This looks like a dead store.

Updated a mistake in transforms.

Comment on attachment 8674267 [details] [diff] [review] Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v3 Carrying r+ from jrmuizel.

Fixed some bugs with relation to context state management.

Comment on attachment 8677425 [details] [diff] [review] Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces. v3 Review of attachment 8677425 [details] [diff] [review]: ----------------------------------------------------------------- This adds a big hunk code to BasicLayerManager which isn't that appealing to me. Is it basically just implementing PushGroup inline?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #17) > Comment on attachment 8677425 [details] [diff] [review] > Convert BasicLayers usecases to PushGroupForBlendback and temporary > surfaces. v3 > > Review of attachment 8677425 [details] [diff] [review]: > ----------------------------------------------------------------- > > This adds a big hunk code to BasicLayerManager which isn't that appealing to > me. Is it basically just implementing PushGroup inline? Somewhat, it's a little more efficient as it avoids some dances around pattern management and state changes and such. I'm actually happy about that because it makes the cost paid by the intermediate surface creation explicitly visible.

This code doesn't ever get hit using our tests, nor do I see any way it ever would get hit. Let's just kill it rather than bother converting it.

Correct patch.

My qref skills are unimpressive.

Comment on attachment 8677425 [details] [diff] [review] Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces. v3 Review of attachment 8677425 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicLayerManager.cpp @@ +184,5 @@ > + dataSurf->Unmap(); > + dataSurf = src->GetDataSurface(); > + dataSurf->Map(DataSourceSurface::MapType::READ, &map); > + dataSurf->Unmap(); > + } What is the code above here for? @@ +188,5 @@ > + } > + > + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)), > + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform), > + DrawOptions(group.mOpacity, group.mOperator)); Mask is not supported with DrawTargetCG. Please use MaskSurface.

Comment on attachment 8678827 [details] [diff] [review] Part 4: Remove unused code to support non-operator OVER. r=roc Review of attachment 8678827 [details] [diff] [review]: ----------------------------------------------------------------- Can you explain why this is unused? Seems to me it's still necessary if some code (an addon maybe) does a canvas.drawWindow while the globalCompositeOperation is not OVER.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25) > Comment on attachment 8678827 [details] [diff] [review] > Part 4: Remove unused code to support non-operator OVER. r=roc > > Review of attachment 8678827 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you explain why this is unused? Seems to me it's still necessary if some > code (an addon maybe) does a canvas.drawWindow while the > globalCompositeOperation is not OVER. Well, if that addon exists that would be wonderful, and I'd love to hear about it, but since none of our tests hit it, and drawWindow is not in the Canvas spec and simply a facility we're providing to people, it seems like a better idea to break this, and only add code to deal with this if it turns out there's actually anyone using it.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #24) > Comment on attachment 8677425 [details] [diff] [review] > Convert BasicLayers usecases to PushGroupForBlendback and temporary > surfaces. v3 > > Review of attachment 8677425 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/basic/BasicLayerManager.cpp > @@ +184,5 @@ > > + dataSurf->Unmap(); > > + dataSurf = src->GetDataSurface(); > > + dataSurf->Map(DataSourceSurface::MapType::READ, &map); > > + dataSurf->Unmap(); > > + } > > What is the code above here for? Just debugging stuff that slipped in. It's gone. > @@ +188,5 @@ > > + } > > + > > + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)), > > + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform), > > + DrawOptions(group.mOpacity, group.mOperator)); > > Mask is not supported with DrawTargetCG. Please use MaskSurface. Will do, I'd have to check the mask transform is a translation only, but I don't think that's ever not the case.

(In reply to Bas Schouten (:bas.schouten) from comment #26) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25) > > Comment on attachment 8678827 [details] [diff] [review] > > Part 4: Remove unused code to support non-operator OVER. r=roc > > > > Review of attachment 8678827 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Can you explain why this is unused? Seems to me it's still necessary if some > > code (an addon maybe) does a canvas.drawWindow while the > > globalCompositeOperation is not OVER. > > Well, if that addon exists that would be wonderful, and I'd love to hear > about it, but since none of our tests hit it, and drawWindow is not in the > Canvas spec and simply a facility we're providing to people, it seems like a > better idea to break this, and only add code to deal with this if it turns > out there's actually anyone using it. Could you be persuaded to take this approach, and if not could you point me at some application of this so I can make sure I test it when converting this code?

I can't point to any application using drawWindow with non-OVER, but it doesn't seem right to me to just break it. I imagine it'd be pretty easy to keep non-OVER operators working with drawWindow. Is it not just a few lines of code, perhaps with a temporary surface?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #24) > > + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)), > > + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform), > > + DrawOptions(group.mOpacity, group.mOperator)); > > Mask is not supported with DrawTargetCG. Please use MaskSurface. Then shouldn't we either remove Mask from DrawTarget or implement it for DrawTargetCG? It seems bad to have DrawTarget methods that one should never call.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > I can't point to any application using drawWindow with non-OVER, but it > doesn't seem right to me to just break it. What I'm trying to get at is that it seems wrong to have combinations of features that just don't work because we don't think that particular combination is important. That makes our APIs weird and unpredictable. I understand that drawWindow isn't Web-exposed, but it has still been widely used in addons.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > > I can't point to any application using drawWindow with non-OVER, but it > > doesn't seem right to me to just break it. > > What I'm trying to get at is that it seems wrong to have combinations of > features that just don't work because we don't think that particular > combination is important. That makes our APIs weird and unpredictable. I > understand that drawWindow isn't Web-exposed, but it has still been widely > used in addons. If we want this combination to work. We need to have a reftest for it somehow, as it stands, we don't even know if it works as expected right now! (I mean, I assume it does, but the current code isn't being tested anywhere) And yes, adding it is just a couple of lines of code to create a temporary surface, and that's fine, but I feel having code specifically to support an untested, never been used functionality around, is unwise. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #24) > > > + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)), > > > + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform), > > > + DrawOptions(group.mOpacity, group.mOperator)); > > > > Mask is not supported with DrawTargetCG. Please use MaskSurface. > > Then shouldn't we either remove Mask from DrawTarget or implement it for > DrawTargetCG? It seems bad to have DrawTarget methods that one should never > call. I agree, I -really- don't want to kill mask though since I think it's a useful function (as this case proved) in general to have. But apparently according to Jeff it would take some effort to implement in DrawTargetCG.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > > I can't point to any application using drawWindow with non-OVER, but it > > doesn't seem right to me to just break it. > > What I'm trying to get at is that it seems wrong to have combinations of > features that just don't work because we don't think that particular > combination is important. That makes our APIs weird and unpredictable. I > understand that drawWindow isn't Web-exposed, but it has still been widely > used in addons. Also, I don't know.. do for example Canvas shadows work with DrawWindow right now? I somehow doubt it, or at least, maybe or maybe not as expected etc. So I'm not sure in the case of DrawWindow we can say this is a valid concern. Also, if we -do- decide to support this, I'd like to move the code to CanvasRenderingContext2D::DrawWindow so we make it explicit what this code is designed to support.

I also can't find any codepaths using this. I don't think there's any way Canvas uses this code either. Is it okay if I replace this with an ASSERT?

Comment on attachment 8679975 [details] [diff] [review] Part 6: Remove unused code to support non-operator OVER in nsCSSRendering. r=roc Never mind. I've found where this is used.

CanvasRenderingContext2D::DrawWindow already has code to draw to a temporary surface. We draw directly under the following condition: // Rendering directly is faster and can be done if mTarget supports Azure // and does not need alpha blending. if (gfxPlatform::GetPlatform()->SupportsAzureContentForDrawTarget(mTarget) && GlobalAlpha() == 1.0f) The temp-surface code always draws with OVER, which is already a bug when you have GlobalAlpha() < 1.0 and non-OVER composition op. But anyway, I suggest you go ahead and remove the non-OVER path from RenderDocument, asserting that the current operator is OVER. And just add an operator check to the above condition that assertion can't be hit. And if you're feeling really good, pass the correct op in CanvasRenderingContext2D::DrawWindow's call to DrawSurface. Shadows still wouldn't work but otherwise at least we're going forwards not backwards and it's a trivial patch. (In reply to Bas Schouten (:bas.schouten) from comment #33) > I agree, I -really- don't want to kill mask though since I think it's a > useful function (as this case proved) in general to have. But apparently > according to Jeff it would take some effort to implement in DrawTargetCG. We should just remove it until we can make it usable. The cost of someone accidentally using it outweighs the cost of maybe having to reland it.

qref to remove silly debugging hunk.

Comment on attachment 8680296 [details] [diff] [review] Part 6: Convert code to support non-operator OVER in nsCSSRendering to moz2d. v2 Review of attachment 8680296 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ -5053,5 @@ > return DrawResult::TEMPORARY_ERROR; > } > > - gfxContext* ctx = aRenderingContext.ThebesContext(); > - CompositionOp op = ctx->CurrentOp(); Assert that the CurrentOp is OVER here. ::: layout/base/nsLayoutUtils.cpp @@ +6254,5 @@ > + IntRect tmpDTRect; > + > + if (destCtx->CurrentOp() != CompositionOp::OP_OVER) { > + Rect imageRect = ToRect(params.imageSpaceToDeviceSpace.TransformBounds(params.region.Rect())); > + imageRect.ToIntRect(&tmpDTRect); Just casting here is not good. RoundOut before calling ToIntRect.

Comment on attachment 8680308 [details] [diff] [review] Part 4: Remove code to support non-operator OVER in nsRenderDocument and move to CanvasRenderingContext2D::DrawWindow. r=roc Review of attachment 8680308 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!!

Comment on attachment 8680296 [details] [diff] [review] Part 6: Convert code to support non-operator OVER in nsCSSRendering to moz2d. v2 Review of attachment 8680296 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ -5053,5 @@ > return DrawResult::TEMPORARY_ERROR; > } > > - gfxContext* ctx = aRenderingContext.ThebesContext(); > - CompositionOp op = ctx->CurrentOp(); We don't need it to be.. right? I made nsLayoutUtils::DrawImage deal with !OVER in the rest of the patch. ::: layout/base/nsLayoutUtils.cpp @@ +6254,5 @@ > + IntRect tmpDTRect; > + > + if (destCtx->CurrentOp() != CompositionOp::OP_OVER) { > + Rect imageRect = ToRect(params.imageSpaceToDeviceSpace.TransformBounds(params.region.Rect())); > + imageRect.ToIntRect(&tmpDTRect); You're absolutely right.

Comment on attachment 8674267 [details] [diff] [review] Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v3 Review of attachment 8674267 [details] [diff] [review]: ----------------------------------------------------------------- nsSVGClipPathFrame::ApplyClipOrPaintClipMask really needs to die, but rather than supply you with a bunch of detailed review comments on how I'd like that done I'll just write a follow-up patch. Thanks for getting things this far. ::: layout/svg/nsSVGClipPathFrame.cpp @@ +175,5 @@ > + gfxContextMatrixAutoSaveRestore autoRestoreMatrix(&aReferenceContext); > + > + aReferenceContext.SetMatrix(gfxMatrix()); > + gfxRect rect = aReferenceContext.GetClipExtents(); > + ToRect(rect).ToIntRect(&intRect); We should probably use |intRect = RoundedOut(ToRect(rect));| here.

Comment on attachment 8683678 [details] [diff] [review] Part 7: Convert GTK widget code to use Moz2D instead of PushGroup/PopGroup Review of attachment 8683678 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gtk/nsWindow.cpp @@ +2221,5 @@ > // call UpdateTranslucentWindowAlpha once. After we have dropped > // support for non-Thebes graphics, UpdateTranslucentWindowAlpha will be > // our private interface so we can rework things to avoid this. > boundsRect = region.GetBounds(); > + dt->PushClipRect(Rect(boundsRect.x, boundsRect.y, Pushing clips to the DrawDarget, wrapping the DrawTarget in a gfxContext and then passing the gfxContext off to some arbitrary code is currently prone to restoring the wrong clipping, no? The gfxContext ctor doesn't save the clipping stack off the DrawTarget, so if the code that is passed the gfxContext pushes clipping state then pops it, the DrawTarget will have all clipping cleared.

Ah, no I'm mis-remembering - clip handling is safe because gfxContext::Restore() only pops as many clips as were pushed. It's transforms on DrawTarget that aren't maintained when they're wrapped in a gfxContext (per query to Bas, because adjusting for device transforms would be iffy). So there's no problem here.

Comment on attachment 8683678 [details] [diff] [review] Part 7: Convert GTK widget code to use Moz2D instead of PushGroup/PopGroup Taking review as discussed with Karl and Bas.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc53a141139 https://hg.mozilla.org/integration/mozilla-inbound/rev/11ce065429a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/5515fd47eb5a https://hg.mozilla.org/integration/mozilla-inbound/rev/2ccdc96a5559 https://hg.mozilla.org/integration/mozilla-inbound/rev/2afe2414f3fc https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6e7bbaf2ad https://hg.mozilla.org/integration/mozilla-inbound/rev/291eaaa12013 https://hg.mozilla.org/integration/mozilla-inbound/rev/af5c9cf6898b

https://hg.mozilla.org/mozilla-central/rev/8fc53a141139 https://hg.mozilla.org/mozilla-central/rev/11ce065429a5 https://hg.mozilla.org/mozilla-central/rev/5515fd47eb5a https://hg.mozilla.org/mozilla-central/rev/2ccdc96a5559 https://hg.mozilla.org/mozilla-central/rev/2afe2414f3fc https://hg.mozilla.org/mozilla-central/rev/fc6e7bbaf2ad https://hg.mozilla.org/mozilla-central/rev/291eaaa12013 https://hg.mozilla.org/mozilla-central/rev/af5c9cf6898b https://hg.mozilla.org/mozilla-central/rev/265f46e1ee7b https://hg.mozilla.org/mozilla-central/rev/3f97905378fd https://hg.mozilla.org/mozilla-central/rev/1c7753515d42