Bug 23625 – Skia platform doesn't render text to a canvas or support clipping to an image buffer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Scott Violet]

As shown by a number of layout tests.

[Scott Violet]

Created attachment 27155 [details]
Fix for 23625

This patch helps Chromium pass a number of canvas related test cases.

[Eric Seidel (no email)]

Comment on attachment 27155 [details]
Fix for 23625

WebKit style omits names for arguments in headers when the names do not provide additional clarity:

Like this:
+    void applyClipFromImage(const WebCore::FloatRect& rect, const SkBitmap& imageBuffer);

neither rect or imageBuffer should be in the definition.

Otherwise this looks fine.  One of us can fix those nits when landing.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/40383

[Dirk Schulze]

The end of clipToImageBuffer is not necessarily reached with the next restore. You can use combinations of save and restore between clipToImageBuffer and it's corresponding restore. SVG Masking uses different save/restores between clipToImageBuffer and the real restore.

At the moment we use clipToImageBuffer on three points. Filling/stroking texts with gradients or patterns on Canvas, clipping text on a background for CSS and SVG Masking. There is no implementation for SVG Masking on Skia, but there is a patch for making SVG Masking platform aware at https://bugs.webkit.org/show_bug.cgi?id=19243

And I don't know if it is possible to use clipToImageBuffer during a clipToImageBuffer. So it's maybe necessary to store more clipImages:

context->save();
context->clipToImageBuffer();

...
context->save();
context->clipToImageBuffer();
...
context->restore();
...

context->restore();

[Scott Violet]

Do you have a test case I can use?

I believe:

PlatformContextSkia::State::State(const State& other)

Needs to reset the m_imageBufferClip to an empty bitmap. Then the scenario you mention should work fine.

[Dirk Schulze]

Well, just use the patch mentined above (for the save/restore's during the clipToImageBuffer call) and visit sites like:
http://svg.tutorial.aptico.de/grafik_svg/kap14_2.svg
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-masking-mask-01-b.html

I'don't have an example for the use of clipToImageBuffer during a clipToImageBuffer call.
Don't know if masking an object with an masked object in SVG could cause that situation, but maybe not.

[Dirk Schulze]

Created attachment 27269 [details]
masked mask

This is an example of an masked object as a mask. Don't know if webkit can handle it. Haven't tested it on Safari. But it seems to call clipToImageBuffer during the clipToImageBuffer call.
The SVG Masking patch from above is needed to test this on skia.

[Scott Violet]

(In reply to comment #7)
> Created an attachment (id=27269) [review]
> masked mask

Can you attach a screenshot as to what this should look like?

[Dirk Schulze]

Created attachment 27281 [details]
masked mask as tif

... on safari 3.2

[Scott Violet]

(In reply to comment #9)
> Created an attachment (id=27281) [review]
> masked mask as tif
> 
> ... on safari 3.2
> 

If I fix the bug in PlatformContextSkia when I run your test case I hit a divide by zero error in SkPMColorToColor in SkiaUtils.cpp. This is likely happening because getImageData doesn't fix up the alpha before extracting a value. I'll attach a patch shortly that fixes PlatformContextSkia.

[Scott Violet]

Created attachment 27298 [details]
Temporary patch to PlatfromContextSkia to avoid crash

This patch fixes a crash in PlatformContextSkia if beginLayerClippedToImage is called without a subsequent restore. This patch is finicky though, and don't consider it a final patch. It's only useful to see the the divide by zero I just mentioned.

[Scott Violet]

Created attachment 27319 [details]
Fixes three bugs introduced in first patch

This fixes three bugs found in the first patch:

. When a new layer was started clipped to an image we used the
  assignment operator to copy the SkBitmap. If the SkBitmap owns it's
  pixels, this is not the right thing to do. Instead we need to create
  a copy of the image.
. State holds an SkBitmap by value. State's copy constructor does a
  memcpy. This is confusing and subtle, I've converted to use a member
  initializer list which I think is clearer and less error prone.
. When creating a new layer there is no need to copy the clip image.

[Eric Seidel (no email)]

Comment on attachment 27319 [details]
Fixes three bugs introduced in first patch

I don't think this is the patch you meant to attach.

[Scott Violet]

Created attachment 27320 [details]
Fixes three bugs introduced in first patch (the right patch this time)

Yes, you are absolutely right. Sorry about that.

[Dimitri Glazkov (Google)]

Eric, can you look at this again?

[Eric Seidel (no email)]

Comment on attachment 27320 [details]
Fixes three bugs introduced in first patch (the right patch this time)

Looks sane enough.  Sad we're not using smart pointers here and have to do our own "safeRef()" calls.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/40689.

[Darin Fisher (:fishd, Google)]

*** Bug 23555 has been marked as a duplicate of this bug. ***