Convert PushGroup/PopGroup calls to specialized versions as much as possible

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

(Depends on: 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(8 attachments, 12 obsolete attachments)

4.54 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
9.37 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
20.62 KB, patch
jwatt
: review+
bas.schouten
: review+
Details | Diff | Splinter Review
6.04 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
16.80 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
5.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.79 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.18 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8668638 [details] [diff] [review]
Part 1: Add an API specifically intended for users that just Push and Pop-Mask/Pop-Paint
Attachment #8668638 - Flags: review?(jmuizelaar)
Attachment #8668638 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 2

3 years ago
Created attachment 8671548 [details] [diff] [review]
Part 2: Convert some simple users to PushGroupForBlendBack.
Attachment #8671548 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

3 years ago
Created attachment 8671549 [details] [diff] [review]
Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces.

This is more involved SVG changes, would like review from Jwatt as well.
Attachment #8671549 - Flags: review?(jwatt)
Attachment #8671549 - Flags: review?(jmuizelaar)
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.
Attachment #8671548 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 5

3 years ago
Created attachment 8671565 [details] [diff] [review]
Part 2: Convert some simple users to PushGroupForBlendBack. v2
Attachment #8671548 - Attachment is obsolete: true
Attachment #8671565 - Flags: review?(jmuizelaar)
Attachment #8671565 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 6

3 years ago
Did you mean to r- that v2?
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 7

3 years ago
Created attachment 8671722 [details] [diff] [review]
Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v2

Fix a transform mistake and a use-after-free.
Attachment #8671549 - Attachment is obsolete: true
Attachment #8671549 - Flags: review?(jwatt)
Attachment #8671549 - Flags: review?(jmuizelaar)
Attachment #8671722 - Flags: review?(jwatt)
Attachment #8671722 - Flags: review?(jmuizelaar)
Attachment #8671565 - Flags: review- → review+
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?
(Assignee)

Comment 9

3 years ago
(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
Flags: needinfo?(jmuizelaar)
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.
Attachment #8671722 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8674267 [details] [diff] [review]
Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v3

Updated a mistake in transforms.
Attachment #8671722 - Attachment is obsolete: true
Attachment #8671722 - Flags: review?(jwatt)
Attachment #8674267 - Flags: review?(jwatt)
(Assignee)

Comment 13

3 years ago
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.
Attachment #8674267 - Flags: review+
(Assignee)

Comment 14

3 years ago
Created attachment 8675435 [details] [diff] [review]
Part 4: Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces.
Attachment #8675435 - Flags: review?(jmuizelaar)
(Assignee)

Comment 15

3 years ago
Created attachment 8676302 [details] [diff] [review]
Part 4: Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces. v2

Fixed some bugs with relation to context state management.
Attachment #8675435 - Attachment is obsolete: true
Attachment #8675435 - Flags: review?(jmuizelaar)
Attachment #8676302 - Flags: review?(jmuizelaar)
(Assignee)

Comment 16

3 years ago
Created attachment 8677425 [details] [diff] [review]
Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces. v3
Attachment #8676302 - Attachment is obsolete: true
Attachment #8676302 - Flags: review?(jmuizelaar)
Attachment #8677425 - Flags: review?(jmuizelaar)
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?
(Assignee)

Comment 18

3 years ago
(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.
(Assignee)

Comment 19

3 years ago
Created attachment 8678827 [details] [diff] [review]
Part 4: Remove unused code to support non-operator OVER. r=roc

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.
Attachment #8678827 - Flags: review?(roc)
(Assignee)

Comment 20

3 years ago
Created attachment 8678832 [details] [diff] [review]
Part 7: Remove unused PushGroup/PopGroup/PopGroupToSource functions
Attachment #8678832 - Flags: review?(jmuizelaar)
(Assignee)

Comment 21

3 years ago
Created attachment 8678851 [details] [diff] [review]
Part 7: Remove unused PushGroup/PopGroup/PopGroupToSource functions v2

Correct patch.
Attachment #8678851 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8678832 - Attachment is obsolete: true
Attachment #8678832 - Flags: review?(jmuizelaar)
(Assignee)

Comment 22

3 years ago
Created attachment 8678853 [details] [diff] [review]
Part 7: Remove unused PushGroup/PopGroup/PopGroupToSource functions v3

My qref skills are unimpressive.
Attachment #8678851 - Attachment is obsolete: true
Attachment #8678851 - Flags: review?(jmuizelaar)
(Assignee)

Comment 23

3 years ago
Created attachment 8678856 [details] [diff] [review]
Part 8: Remove unused PushGroup/PopGroup/PopGroupToSource functions v4
Attachment #8678853 - Attachment is obsolete: true
Attachment #8678856 - Flags: review?(jmuizelaar)
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.
Attachment #8677425 - Flags: review?(jmuizelaar) → review-
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.
Attachment #8678827 - Flags: review?(roc)
(Assignee)

Comment 26

3 years ago
(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.
(Assignee)

Comment 27

3 years ago
(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.
(Assignee)

Comment 28

3 years ago
Created attachment 8679957 [details] [diff] [review]
Part 5: Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces. v4
Attachment #8677425 - Attachment is obsolete: true
Attachment #8679957 - Flags: review?(jmuizelaar)
(Assignee)

Comment 29

3 years ago
(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?
Flags: needinfo?(roc)
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?
Flags: needinfo?(roc)
(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.
Flags: needinfo?(jmuizelaar)
(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.
(Assignee)

Comment 33

3 years ago
(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.
(Assignee)

Comment 34

3 years ago
(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.
Flags: needinfo?(roc)
(Assignee)

Comment 35

3 years ago
Created attachment 8679975 [details] [diff] [review]
Part 6: Remove unused code to support non-operator OVER in nsCSSRendering. r=roc

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?
Attachment #8679975 - Flags: review?(roc)
(Assignee)

Comment 36

3 years ago
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.
Attachment #8679975 - Flags: review?(roc)
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.
Flags: needinfo?(roc)
(Assignee)

Comment 38

3 years ago
Created attachment 8680294 [details] [diff] [review]
Part 6: Convert code to support non-operator OVER in nsCSSRendering to moz2d.
Attachment #8679975 - Attachment is obsolete: true
Attachment #8680294 - Flags: review?(roc)
(Assignee)

Comment 39

3 years ago
Created attachment 8680296 [details] [diff] [review]
Part 6: Convert code to support non-operator OVER in nsCSSRendering to moz2d. v2

qref to remove silly debugging hunk.
Attachment #8680296 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8680294 - Attachment is obsolete: true
Attachment #8680294 - Flags: review?(roc)
(Assignee)

Comment 40

3 years ago
Created attachment 8680308 [details] [diff] [review]
Part 4: Remove code to support non-operator OVER in nsRenderDocument and move to CanvasRenderingContext2D::DrawWindow. r=roc
Attachment #8678827 - Attachment is obsolete: true
Attachment #8680308 - Flags: review?(roc)
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.
Attachment #8680296 - Flags: review?(roc) → review+
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!!
Attachment #8680308 - Flags: review?(roc) → review+
(Assignee)

Comment 43

3 years ago
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.
Attachment #8678856 - Flags: review?(jmuizelaar) → review+
Attachment #8679957 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

3 years ago
Attachment #8678856 - Attachment description: Part 7: Remove unused PushGroup/PopGroup/PopGroupToSource functions v4 → Part 8: Remove unused PushGroup/PopGroup/PopGroupToSource functions v4
(Assignee)

Comment 44

3 years ago
Created attachment 8683678 [details] [diff] [review]
Part 7: Convert GTK widget code to use Moz2D instead of PushGroup/PopGroup
Attachment #8683678 - Flags: review?(karlt)
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.
Attachment #8674267 - Flags: review?(jwatt) → review+
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.
Attachment #8683678 - Flags: review?(karlt) → review+
Depends on: 1224798

Updated

3 years ago
Depends on: 1224824
Depends on: 1225119
Depends on: 1224974
Depends on: 1225719

Updated

3 years ago
Depends on: 1226255

Updated

3 years ago
Depends on: 1227615

Updated

3 years ago
Depends on: 1228916

Updated

3 years ago
Depends on: 1229095

Updated

3 years ago
Depends on: 1251431
Flags: needinfo?(jmuizelaar)
Depends on: 1258650

Updated

2 months ago
Depends on: 1467552
You need to log in before you can comment on or make changes to this bug.