Closed Bug 1317636 Opened 8 years ago Closed 8 years ago

Split nsSVGClipPathFrame::GetClipMask into nsSVGClipPathFrame::CreateClipMask & nsSVGClipPathFrame::PaintClipMask

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

Bug 1317636 - Part 2. Extract PaintFrameIntoMask from nsSVGClipPathFrame::PaintClipMask.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1317636 - Part 1. Implement nsSVGClipPathFrame::CreateClipMask and PaintClipMask.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1317636 - Part 3. Implement ComposeExtraMask.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1317636 - Part 4. Use nsSVGUtils::DetermineMaskUsage.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1317636 - Part 5. Fix identation.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1317636 - Part 6. Use PaintClipMask in nsSVGIntegrationUtils::PaintMask.
58 bytes, text/x-review-board-request
mstange
: review+
Details 
      No description provided.
nsSVGClipPathFrame::GetClipMask create a new A8 surface and paint the content of mask onto that new created surface. It's not always we want, in some cases, we actually only need mask-painting only.
Comment on attachment 8811180 [details]
Bug 1317636 - Part 1. Implement nsSVGClipPathFrame::CreateClipMask and PaintClipMask.

https://reviewboard.mozilla.org/r/93392/#review93880

::: layout/svg/nsSVGClipPathFrame.cpp:90
(Diff revision 3)
> +nsSVGClipPathFrame::CreateClipMask(gfxContext& aReferenceContext,
> +                                   IntPoint& aOffset)
> +{
> +  gfxContextMatrixAutoSaveRestore autoRestoreMatrix(&aReferenceContext);
> +
> +  IntRect bonuds;

typo: bonuds -> bounds

and you can move the declaration down to the line where you assign to it

::: layout/svg/nsSVGClipPathFrame.cpp:234
(Diff revision 3)
> +      }
> +      aMaskContext.Restore();
> +    }
> +  }
> +
> +  // Moz2D transforms in the opposite direction to Thebes

Really? Or is this about the DT transform acting as the inverse of the pattern transform?
Comment on attachment 8811690 [details]
Bug 1317636 - Part 1. Implement nsSVGClipPathFrame::CreateClipMask and PaintClipMask.

https://reviewboard.mozilla.org/r/93712/#review93886

Could you fold this patch into the previous one? That will make it easier for me to see what is getting added vs what's just getting moved around.
Comment on attachment 8811180 [details]
Bug 1317636 - Part 1. Implement nsSVGClipPathFrame::CreateClipMask and PaintClipMask.

https://reviewboard.mozilla.org/r/93392/#review93888

::: layout/svg/nsSVGClipPathFrame.cpp:234
(Diff revision 3)
> +      }
> +      aMaskContext.Restore();
> +    }
> +  }
> +
> +  // Moz2D transforms in the opposite direction to Thebes

Oh, I see that this is already in the code. Please ignore my comment.
Comment on attachment 8811181 [details]
Bug 1317636 - Part 2. Extract PaintFrameIntoMask from nsSVGClipPathFrame::PaintClipMask.

https://reviewboard.mozilla.org/r/93394/#review93892
Attachment #8811181 - Flags: review?(mstange) → review+
Comment on attachment 8811691 [details]
Bug 1317636 - Part 3. Implement ComposeExtraMask.

https://reviewboard.mozilla.org/r/93714/#review93894
Attachment #8811691 - Flags: review?(mstange) → review+
Comment on attachment 8811692 [details]
Bug 1317636 - Part 4. Use nsSVGUtils::DetermineMaskUsage.

https://reviewboard.mozilla.org/r/93716/#review93896
Attachment #8811692 - Flags: review?(mstange) → review+
Comment on attachment 8811692 [details]
Bug 1317636 - Part 4. Use nsSVGUtils::DetermineMaskUsage.

https://reviewboard.mozilla.org/r/93716/#review93898

typo in commit message: Ues -> Use
Comment on attachment 8811693 [details]
Bug 1317636 - Part 5. Fix identation.

https://reviewboard.mozilla.org/r/93718/#review93904
Attachment #8811693 - Flags: review?(mstange) → review+
Comment on attachment 8811694 [details]
Bug 1317636 - Part 6. Use PaintClipMask in nsSVGIntegrationUtils::PaintMask.

https://reviewboard.mozilla.org/r/93720/#review93908

Looks good!

There is one more optimization you can make: If both maskUsage.shouldGenerateMaskLayer and maskUsage.shouldGenerateClipMaskLayer are true, we now make a copy of "target" when you combine the masks. This copy can be eliminated if you draw the first mask into a different DrawTarget. We have to pay the allocation cost either way.

Here's how the copy is happening:
RefPtr<DrawTarget> dt = Factory::Create...(...)
dt->DrawSomeStuff();
RefPtr<SourceSurface> snapshot = dt->Snapshot(); // This does not make a copy yet
dt->MaskSurface(ColorPattern(Color(...)), snapshot, Point(0, 0); // Here, the draw target calls MarkDirty() which tells its snapshot to make a copy of the data. This copy does not happen if you draw the snapshot into a different draw target.
Attachment #8811694 - Flags: review?(mstange) → review+
Attachment #8811180 - Attachment is obsolete: true
Attachment #8811180 - Flags: review?(mstange)
Blocks: 1318418
Comment on attachment 8811690 [details]
Bug 1317636 - Part 1. Implement nsSVGClipPathFrame::CreateClipMask and PaintClipMask.

https://reviewboard.mozilla.org/r/93712/#review93936

Thanks, this patch was much easier to review.

::: layout/svg/nsSVGClipPathFrame.cpp:90
(Diff revision 3)
> -nsSVGClipPathFrame::GetClipMask(gfxContext& aReferenceContext,
> +nsSVGClipPathFrame::CreateClipMask(gfxContext& aReferenceContext,
> +                                   IntPoint& aOffset)
> +{
> +  gfxContextMatrixAutoSaveRestore autoRestoreMatrix(&aReferenceContext);
> +
> +  IntRect bounds;

Please remove this line and do
IntRect bounds = RoundedOut(ToRect(rect));
below.

::: layout/svg/nsSVGClipPathFrame.cpp:94
(Diff revision 3)
> +
> +  IntRect bounds;
> +  aReferenceContext.SetMatrix(gfxMatrix());
> +  gfxRect rect = aReferenceContext.GetClipExtents();
> +  bounds = RoundedOut(ToRect(rect));
> +  if (bonuds.IsEmpty()) {

this line still needs fixing

::: layout/svg/nsSVGClipPathFrame.cpp:99
(Diff revision 3)
> +  if (bonuds.IsEmpty()) {
> +    // We don't need to create a mask surface, all drawing is clipped anyway.
> +    return nullptr;
> +  }
> +
> +  DrawTarget* sourceDT = aReferenceContext.GetDrawTarget();

maybe call this referenceDT

::: layout/svg/nsSVGClipPathFrame.cpp:270
(Diff revision 3)
> +                                DrawResult* aResult)
> +{
> +  MOZ_ASSERT(!IsTrivial(), "Caller needs to use ApplyClipPath");
> +
> +  IntPoint offset;
> +  RefPtr<DrawTarget> maskDT= CreateClipMask(aReferenceContext, offset);

missing space in front of =
Attachment #8811690 - Flags: review?(mstange) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8bf6e2a8de2
Part 1. Implement nsSVGClipPathFrame::CreateClipMask and PaintClipMask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d2096c38e4ef
Part 2. Extract PaintFrameIntoMask from nsSVGClipPathFrame::PaintClipMask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/81d5d07d925a
Part 3. Implement ComposeExtraMask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/279946785abc
Part 4. Use nsSVGUtils::DetermineMaskUsage. r=mstange
https://hg.mozilla.org/integration/autoland/rev/0541cb3d94d9
Part 5. Fix identation. r=mstange
https://hg.mozilla.org/integration/autoland/rev/59fe3f5f5bd2
Part 6. Use PaintClipMask in nsSVGIntegrationUtils::PaintMask. r=mstange
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: