The default bug view has changed. See this FAQ.

Transfer cairo glyph optimizations from trunk to branch

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

({fixed1.8})

1.8 Branch
fixed1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
As part of <textPath> implemention I did some performance and specification
conformance improvements to the svg cairo backend glue.  Profiling a slow svg
with a moderate amount of text, the areas that I improved are showing at the top
of the hits.  Merging in just those changes from the trunk gives about a 3x
improvement in this case (empire).

The first major change is hit detection based. the character boxes, rather than
the stroked boundaries.  The other is calculation region coverage based on the
text extents, rather than always getting the extent of the stroked boundaries
(which is still used if someone strokes text).

This code has been baking on the trunk since August 26th.
(Assignee)

Comment 1

12 years ago
Created attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk
Attachment #198635 - Flags: review?(scootermorris)

Updated

12 years ago
Attachment #198635 - Flags: review?(scootermorris) → review+
(Assignee)

Comment 2

12 years ago
Comment on attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk

Very large performance boost for SVG when dealing with large amounts of text
and/or dynamic text (3x speed increase seen in real svg use with moderate
amount of dynamic text mixed with other graphical elements).  This code has
been baking in the trunk for six weeks, should be safe.
Attachment #198635 - Flags: approval1.8rc1?

Comment 3

12 years ago
Comment on attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk

Might have taken something like this in Beta2, but we're approaching RC1 which
could be our final release so we don't really have room to take such large
changes.
Attachment #198635 - Flags: approval1.8rc1? → approval1.8rc1-
Comment on attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk

A minimal patch would help get this into 1.8.  The cast cleanups were optional,
right?

>-  cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
>+  if (hasCoveredStroke) {
>+    cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
>+  } else {
>+    cairo_text_extents_t extent;
>+    cairo_text_extents(ctx,
>+                       NS_ConvertUCS2toUTF8(text).get(),
>+                       &extent);
>+    cairo_rectangle(ctx, x + extent.x_bearing, y + extent.y_bearing,
>+                    extent.width, extent.height);
>+  }
> 
>   double xmin, ymin, xmax, ymax;
> 
>   if (hasCoveredStroke) {

Shaver points out that you have two hasCoveredStroke tests that (unless the
else clause could cause that flag to be set somehow) should be unified here.

>-  float sX, sY, eX, eY, eWidth, eHeight;
>-  mSource->GetX(&sX);
>-  mSource->GetY(&sY);
>-  box->GetX(&eX);
>-  box->GetY(&eY);
>-  box->GetWidth(&eWidth);
>-  box->GetHeight(&eHeight);
>-  cairo_rectangle(ctx, sX + eX, sY + eY, eWidth, eHeight);
>+  metrics->SelectFont(ctx);
>+
>+  nsAutoString text;
>+  mSource->GetCharacterData(text);
>+
>+  float xx, yy;
>+  mSource->GetX(&xx);
>+  mSource->GetY(&yy);
>+
>+  cairo_matrix_t matrix;
>+
>+  for (PRUint32 i=0; i<text.Length(); i++) {

Comment what's going on here?

/be
Attachment #198635 - Flags: superreview?(vladimir)
(Assignee)

Comment 5

12 years ago
(In reply to comment #4)
> (From update of attachment 198635 [details] [diff] [review] [edit])
> A minimal patch would help get this into 1.8.  The cast cleanups were optional,
> right?

Yes, I was trying to keep the new branch code as close as possible to the code
that had been baking on the trunk.  I'll attach a more minimal patch.

> >-  cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
> >+  if (hasCoveredStroke) {
> >+    cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
> >+  } else {
> >+    cairo_text_extents_t extent;
> >+    cairo_text_extents(ctx,
> >+                       NS_ConvertUCS2toUTF8(text).get(),
> >+                       &extent);
> >+    cairo_rectangle(ctx, x + extent.x_bearing, y + extent.y_bearing,
> >+                    extent.width, extent.height);
> >+  }
> > 
> >   double xmin, ymin, xmax, ymax;
> > 
> >   if (hasCoveredStroke) {
> 
> Shaver points out that you have two hasCoveredStroke tests that (unless the
> else clause could cause that flag to be set somehow) should be unified here.

This is the result of starting from the trunk code and removing the textPath
code.  New patch will combine the blocks.

> >-  float sX, sY, eX, eY, eWidth, eHeight;
> >-  mSource->GetX(&sX);
> >-  mSource->GetY(&sY);
> >-  box->GetX(&eX);
> >-  box->GetY(&eY);
> >-  box->GetWidth(&eWidth);
> >-  box->GetHeight(&eHeight);
> >-  cairo_rectangle(ctx, sX + eX, sY + eY, eWidth, eHeight);
> >+  metrics->SelectFont(ctx);
> >+
> >+  nsAutoString text;
> >+  mSource->GetCharacterData(text);
> >+
> >+  float xx, yy;
> >+  mSource->GetX(&xx);
> >+  mSource->GetY(&yy);
> >+
> >+  cairo_matrix_t matrix;
> >+
> >+  for (PRUint32 i=0; i<text.Length(); i++) {
> 
> Comment what's going on here?

Comment added.

(Assignee)

Comment 6

12 years ago
Created attachment 199324 [details] [diff] [review]
more minimal patch, comments from brendan addressed
Attachment #198635 - Attachment is obsolete: true
Attachment #199324 - Flags: superreview?(vladimir)
Attachment #199324 - Flags: review?(scootermorris)
(Assignee)

Comment 7

12 years ago
*** Bug 312191 has been marked as a duplicate of this bug. ***
Comment on attachment 199324 [details] [diff] [review]
more minimal patch, comments from brendan addressed

r=me; this looks pretty safe and lightweight.  Only SVG can really be affected
by this.
Attachment #199324 - Flags: superreview?(vladimir) → superreview+

Updated

12 years ago
Attachment #199324 - Flags: review?(scootermorris) → review+
(Assignee)

Updated

12 years ago
Attachment #199324 - Flags: approval1.8rc1?

Updated

12 years ago
Attachment #199324 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 9

12 years ago
Checked in on branch.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Attachment #198635 - Flags: review+

Updated

10 years ago
Attachment #198635 - Flags: superreview?(vladimir)
You need to log in before you can comment on or make changes to this bug.