Bug 114329 – [Cairo] Take current matrix into pattern matrix to end up at identity scale when cairo_fill runs

• Status: UNCONFIRMED
• Component: WebKitGTK

[Kyungjin Kim]

When drawPatternToCairoContext if the current scale is not 1, pixman_scaled_bilinear_scanline_8888_8888_OVER_asm_neon is called and takes too much time.
This patch puts 1 / scale in the pattern matrix so it gets identity scale when cairo_fill runs.

[Kyungjin Kim]

Created attachment 197197 [details]
Patch

[Kyungjin Kim]

Created attachment 197202 [details]
Patch

[Viatcheslav Ostapenko]

Comment on attachment 197202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197202&action=review

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:210
> +    if (scaleX != 1 && scaleY != 1 && totalMatrix.xx != 1 && totalMatrix.yy != 1 && destBitmapWidth && destBitmapHeight)

I think this condition should be 
if ((scaleX != 1 || scaleY != 1) && (totalMatrix.xx != 1 || totalMatrix.yy != 1) && destBitmapWidth && destBitmapHeight)

Otherwise this patch looks great.

[Kyungjin Kim]

(In reply to comment #3)
> (From update of attachment 197202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197202&action=review
> 
> > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:210
> > +    if (scaleX != 1 && scaleY != 1 && totalMatrix.xx != 1 && totalMatrix.yy != 1 && destBitmapWidth && destBitmapHeight)
> 
> I think this condition should be 
> if ((scaleX != 1 || scaleY != 1) && (totalMatrix.xx != 1 || totalMatrix.yy != 1) && destBitmapWidth && destBitmapHeight)
> 
> Otherwise this patch looks great.

done, Thanks

[Kyungjin Kim]

Created attachment 198018 [details]
Patch

[Martin Robinson]

So it seems what you are doing here is that instead of creating a scaled pattern matrix, for every render you are creating a temporary image surface with the scaled source? Is this also slow on non-ARM CPUs? I think a change like this needs a little justification as well. Can it be fixed or optimized in Pixman? The Skia port used to cache scaled images -- if we *must* scale the image it would probably be better to cache the scaled version.

[Kyungjin Kim]

(In reply to comment #6)
> So it seems what you are doing here is that instead of creating a scaled pattern matrix, for every render you are creating a temporary image surface with the scaled source? 

Yes

> Is this also slow on non-ARM CPUs? I think a change like this needs a little justification as well. 

I don't think so. But on ARM CPUs this patch helps the performance in some benchmarks. i.e) Browsermark2 2d transform scores 1200 to 2000

> Can it be fixed or optimized in Pixman? 
I doubt it, the function already been neon-optimized as i heard.

> The Skia port used to cache scaled images -- if we *must* scale the image it would probably be better to cache the scaled version.

I can't feel you for caching scaled images part. This patch makes drawPatternToCairoContext look alike Image::drawPattern in skia.

[Martin Robinson]

(In reply to comment #7)

> I can't feel you for caching scaled images part. This patch makes drawPatternToCairoContext look alike Image::drawPattern in skia.

As I recall, the Skia port did cache scaled images when drawing them.

[Kyungjin Kim]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > I can't feel you for caching scaled images part. This patch makes drawPatternToCairoContext look alike Image::drawPattern in skia.
> 
> As I recall, the Skia port did cache scaled images when drawing them.

Thanks for the suggestion but I'm not sure this patch can cover the caching model. If the target is a gl surface, it would be good to cache source image to avoid uploading texture every time, for image surface I don't think so.

[Kyungjin Kim]

Any more comment?

[Michael Catanzaro]

Reassigning some EFL bugs that are likely shared with GTK/WPE to the GTK component.