Bug 23625

Summary: Skia platform doesn't render text to a canvas or support clipping to an image buffer
Product: WebKit Reporter: Scott Violet <sky>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Severity: Normal CC: fishd, krit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Description Flags
Fix for 23625
eric: review+
masked mask
masked mask as tif
Temporary patch to PlatfromContextSkia to avoid crash
Fixes three bugs introduced in first patch
eric: review-
Fixes three bugs introduced in first patch (the right patch this time) eric: review+

Description Scott Violet 2009-01-29 12:00:31 PST
As shown by a number of layout tests.
Comment 1 Scott Violet 2009-01-29 12:50:22 PST
Created attachment 27155 [details]
Fix for 23625

This patch helps Chromium pass a number of canvas related test cases.
Comment 2 Eric Seidel (no email) 2009-01-29 13:08:49 PST
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.
Comment 3 Darin Fisher (:fishd, Google) 2009-01-29 16:24:08 PST
Comment 4 Dirk Schulze 2009-02-02 15:49:43 PST
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:



Comment 5 Scott Violet 2009-02-02 16:50:58 PST
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.
Comment 6 Dirk Schulze 2009-02-02 17:05:23 PST
Well, just use the patch mentined above (for the save/restore's during the clipToImageBuffer call) and visit sites like:

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.
Comment 7 Dirk Schulze 2009-02-02 17:51:05 PST
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.
Comment 8 Scott Violet 2009-02-03 09:26:37 PST
(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?
Comment 9 Dirk Schulze 2009-02-03 11:21:43 PST
Created attachment 27281 [details]
masked mask as tif

... on safari 3.2
Comment 10 Scott Violet 2009-02-03 16:52:06 PST
(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.
Comment 11 Scott Violet 2009-02-03 16:59:02 PST
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.
Comment 12 Scott Violet 2009-02-04 10:40:54 PST
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.
Comment 13 Eric Seidel (no email) 2009-02-04 11:20:34 PST
Comment on attachment 27319 [details]
Fixes three bugs introduced in first patch

I don't think this is the patch you meant to attach.
Comment 14 Scott Violet 2009-02-04 11:23:12 PST
Created attachment 27320 [details]
Fixes three bugs introduced in first patch (the right patch this time)

Yes, you are absolutely right. Sorry about that.
Comment 15 Dimitri Glazkov (Google) 2009-02-05 08:37:23 PST
Eric, can you look at this again?
Comment 16 Eric Seidel (no email) 2009-02-05 13:03:49 PST
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.
Comment 17 Dimitri Glazkov (Google) 2009-02-05 15:29:10 PST
Landed as http://trac.webkit.org/changeset/40689.
Comment 18 Darin Fisher (:fishd, Google) 2009-02-12 22:55:10 PST
*** Bug 23555 has been marked as a duplicate of this bug. ***