342366 - make cairo gfx faster (on Windows)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Stuart Parmenter]

i'm going to just put various patches in this bug for cairo speedups

Comment 1

[Stuart Parmenter]

Attachment: SetWorldTransform avoidance

If our transform is identity then we don't need to call SetWorldTransform whenever we select text in (assuming that there were no earlier calls to SetWorldTransform that we should be overwriting, which there aren't, at least in Mozilla).  This gives a reasonable Tp win -- more if we could wire up some places to avoid calling SaveDC/RestoreDC when this is the case.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The memcmp is a little ugly; I almost think we should be calling some sort of non-existent cairo_matrix_is_identity.  Is it faster to translate than it is to call SWT?  If so you can do _cairo_matrix_is_integer_translation and translate by the returned x,y (or do nothing if they're 0,0).

Comment 3

[Stuart Parmenter]

Attachment: SetWorldTransform avoidance 2

this is better, although doesn't do what you were suggesting.  I think i'd rather have a (hopefully) inline memcmp than calling out to some other cairo function.

Comment 4

[Stuart Parmenter]

Attachment: avoid save/restore in fast path (checked in)

This is a quick fix to avoid calling save/restoredc if we aren't modifying the worldtransform.  This can be taken a step or two further by taking seperate code paths entirely if the color is solid, no scale or rotation is set, etc by just using ExtTextOut directly but we currently have no way to get the color.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 228484 [details] [diff] [review]
avoid save/restore in fast path (checked in)

This looks great -- one question though, can't you still use this even if the matrix is not kIdentity, if it just has a translation?  (even just integer translation.)  It won't affect measuring, and the x/y offset will get handled anyway because you're still using cairo to draw.

So you might be able to use !HasNonTranslation() here instead of just == kIdentity.

Comment 6

[Stuart Parmenter]

Attachment: don't copy metrics for no reason (checked in)

Comment 7

[Stuart Parmenter]

Attachment: avoid save/restoredc in nativetheme code

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 228879 [details] [diff] [review]
avoid save/restoredc in nativetheme code

I think you should be checking whether the translation is an integer or not.  The old code didn't, because it assumed that SWT would do the right thing.  The coords you pass in are integers, and the matrix could well have a non-integer translation.

Comment 9

[Stuart Parmenter]

Attachment: speed up ComputeMetrics (checked in)

remove save/restore/cairo/etc from the ComputeMetrics function as none of it is actually needed.  Also includes a fix to the call to GetGlyphOutlineW to pass in a proper matrix (thanks to Steve Swanson for finding that one in bug 324857)

Comment 10

[Stuart Parmenter]

Attachment: return cached space width when string is only a space (checked in)

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 229208 [details] [diff] [review]
return cached space width when string is only a space (checked in)

r=me maybe with a comment saying that callers shouldn't measure " " but instead use the metrics?

Comment 12

[Stuart Parmenter]

Attachment: speed up text rendering (checked in)

this avoids going through cairo for basic text drawing when we can

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 232952 [details] [diff] [review]
speed up text rendering (checked in)


>+    /**
>+     * Get the source pattern (solid color, normal pattern, surface, etc)
>+     */
>+    already_AddRefed<gfxPattern> GetPattern();



>+already_AddRefed<gfxPattern>
>+gfxContext::GetPattern()
> {
>-    cairo_set_source_surface(mCairo, surface->CairoSurface(), offset.x, offset.y);
>+    cairo_pattern_t *pat = cairo_get_source(mCairo);
>+    NS_ASSERTION(pat, "I was told this couldn't be null");
>+
>+    gfxPattern *wrapper = nsnull;
>+    if (pat)
>+        wrapper = new gfxPattern(pat);
>+    else
>+        wrapper = new gfxPattern(gfxRGBA(0,0,0,0));
>+
>+    NS_ADDREF(wrapper);
>+    return wrapper;
> }


I would add a 
PRBool gfxContext::GetColor(gfxRGBA& currentColor)

that would return PR_FALSE if the current source is not a solid, and PR_TRUE and set currentColor appropriately otherwise.  This lets you avoid allocating the new gfxPattern instance just to fetch the current color, which I think would be the 99% common usage.

Comment 14

[Stuart Parmenter]

gonna mark this fixed for now