Closed Bug 828240 Opened 7 years ago Closed 3 years ago

Create a single nsDisplayTransform for children-only transforms

Categories

(Core :: SVG, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

(Depends on 2 open bugs, Blocks 4 open bugs)

Details

(Keywords: perf, Whiteboard: [qf:p1])

Attachments

(2 files)

Right now we create a separate nsDisplayTransform item for each direct child frame of an SVG frame that has a children-only transform (as returned by the HasChildrenOnlyTransform implementations). This means that we can end up creating a lot more nsDisplayTransform items than we should which in itself can be a perf hit (see second paragraph of bug 801949 comment 11). It also means that we end up doing a lot more matrix operations than we should.
Blocks: 801949
The main issue that any patch will need to address is that nsDisplayTransform consumers assume that the item's transform applies to its mFrame. Probably all consumers will need to be taught the difference between nsDisplayTransforms that applies to its mFrame, and those that apply only to the children of mFrame.
One option for fixing this would be to have all children of nsSVGOuterSVGFrame/nsSVGInnerSVGFrame wrapped by an extra anonymous child (turning the actual children into grandchildren). That way only one nsDisplayTransform would be created for a viewBox on an <svg>, because there would only be one direct child of the <svg>'s frame.
Blocks: 869505
Assignee: nobody → jwatt
Blocks: 1204549
Blocks: 790861
Blocks: 786064
No longer blocks: 801949
I tried several different approaches to doing this. I initially thought I'd create a new nsDisplayTransformChildren and teach the bits of the code that need to handle transforms how to handle that too. Unfortunately that turned out to require a lot of (invasive) code and was really awkward to fit in. Having to change so many bits of the code just for this one element that can have children-only transforms seemed wrong.

Instead this patch takes the approach of applying the children-only transforms to the nsSVGOuterSVGFrame's anonymous child frame. That isn't ideal either since when nsDisplayTransform code applies the transform it will affect the position of the anonymous frame (the position being to account for the <svg>'s border and padding). Specifically it will scale that offset, and we need to avoid that. I tried various things that I thought should work such as:

* overriding nsIFrame::GetUsedMargin() (margins are not scaled by transforms)
* changing 'transform-box'/'transform-origin' in svg.css
* accounting for the offset in the children-only transform

All of these ended up interacting badly with other parts of the code.

The least hacky way I can find to do this is to allow nsDisplayTransform to scale the anonymous frame's position, but to incorporate an extra translation into the transform that we return in order to cancel out the scaling of the position.
Attachment #8854660 - Flags: review?(longsonr)
Comment on attachment 8854660 [details] [diff] [review]
Fix for nsSVGOuterSVGFrame

>+    if (ownMatrix.HasNonTranslation()) {
>+      // The nsDisplayTransform code will apply this transform to our frame,
>+      // including to our frame position.  We don't want our frame position to
>+      // be scaled though, so we need to correct for that in the transform.
>+      // XXX Yeah, this is a bit hacky.
>+      CSSPoint pos = CSSPixel::FromAppUnits(GetPosition());
>+      CSSPoint scaledPos = CSSPoint(ownMatrix._11 * pos.x, ownMatrix._22 * pos.y);

will this work if the svg element transform is a skew or in fact anything other than a scale + translate?
We don't need to do scaledPos = ownMatrix * pos here? I.e account for any _12 and _21 parts?

>+      CSSPoint deltaPos = scaledPos - pos;
>+      ownMatrix *= gfxMatrix::Translation(-deltaPos.x, -deltaPos.y);
>+    }
>+
>+    *aOwnTransform = gfx::ToMatrix(ownMatrix);
>+  }
>+
(In reply to Robert Longson from comment #4)
> Comment on attachment 8854660 [details] [diff] [review]
> Fix for nsSVGOuterSVGFrame
> 
> >+    if (ownMatrix.HasNonTranslation()) {
> >+      // The nsDisplayTransform code will apply this transform to our frame,
> >+      // including to our frame position.  We don't want our frame position to
> >+      // be scaled though, so we need to correct for that in the transform.
> >+      // XXX Yeah, this is a bit hacky.
> >+      CSSPoint pos = CSSPixel::FromAppUnits(GetPosition());
> >+      CSSPoint scaledPos = CSSPoint(ownMatrix._11 * pos.x, ownMatrix._22 * pos.y);
> 
> will this work if the svg element transform is a skew or in fact anything
> other than a scale + translate?
> We don't need to do scaledPos = ownMatrix * pos here? I.e account for any
> _12 and _21 parts?

The transform we're handling here is the children-only transform. So purely the viewBox, unless we're a root-<svg> in which case it could inclued currentScale/Translate. So there is only scale, and translation. And it's only the scale part that causes problems since border/padding offset and translation interact fine together.
Comment on attachment 8854660 [details] [diff] [review]
Fix for nsSVGOuterSVGFrame

>+
>+    if (ownMatrix.HasNonTranslation()) {
>+      // The nsDisplayTransform code will apply this transform to our frame,
>+      // including to our frame position.  We don't want our frame position to
>+      // be scaled though, so we need to correct for that in the transform.
>+      // XXX Yeah, this is a bit hacky.

Can we assert ownMatrix.IsRectilinear() somewhere in this method if this is the viewBox transform?

>+      CSSPoint pos = CSSPixel::FromAppUnits(GetPosition());
>+      CSSPoint scaledPos = CSSPoint(ownMatrix._11 * pos.x, ownMatrix._22 * pos.y);
>+      CSSPoint deltaPos = scaledPos - pos;
>+      ownMatrix *= gfxMatrix::Translation(-deltaPos.x, -deltaPos.y);
>+    }
>+
>+    *aOwnTransform = gfx::ToMatrix(ownMatrix);
>+  }
>+
>+  return true;
> }

r=longsonr with the assert added.
Attachment #8854660 - Flags: review?(longsonr) → review+
We should have this on gfxMatrix for parity with gfx::Matrix. I could probably just land this without review, but I guess I shouldn't.
Attachment #8855091 - Flags: review?(longsonr)
Attachment #8855091 - Flags: review?(longsonr) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4365ec0e81de
part 1 - Add an IsRectilinear method to gfxMatrix. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/0901f80a81db
part 2 - Create only one nsDisplayTransform for outer-<svg> children-only transforms. r=longsonr
https://hg.mozilla.org/mozilla-central/rev/4365ec0e81de
https://hg.mozilla.org/mozilla-central/rev/0901f80a81db
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
this improved our tsvg benchmark:
== Change summary for alert #5875 (as of April 06 2017 08:08 UTC) ==

Improvements:

  9%  tsvgx summary windows7-32 pgo e10s     513.98 -> 468.79
  8%  tsvgx summary linux64 opt e10s         495.59 -> 454.09
  8%  tsvgx summary windows7-32 pgo          551.46 -> 506.63
  8%  tsvgx summary osx-10-10 opt e10s       498.6 -> 459.73
  8%  tsvgx summary windows7-32 opt e10s     556.46 -> 513.42
  8%  tsvgx summary osx-10-10 opt            554.6 -> 512.54
  7%  tsvgx summary linux64 pgo e10s         344.85 -> 319.2
  7%  tsvgx summary linux64 opt              546.02 -> 508.98
  6%  tsvgx summary windows7-32 opt          605.41 -> 568.06
  6%  tsvgx summary linux64 pgo              377.97 -> 354.89
  5%  tsvgx summary windows8-64 opt e10s     258.81 -> 245.46
  5%  tsvgx summary windows8-64 pgo e10s     232.05 -> 220.25
  4%  tsvgx summary windows8-64 opt          304.87 -> 292.48
  4%  tsvgx summary windows8-64 pgo          269.56 -> 259.76

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5875
\o/
Adding [qf] so this gets traced back from Quantum Flow perf measurements.
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Depends on: 1428091
Depends on: 1448257
Depends on: 1486952
Depends on: 1523343
No longer depends on: 1523343
Regressions: 1523343
You need to log in before you can comment on or make changes to this bug.