Bug 11867 – Bring back masking & implement patterns/mask properly in ksvg2/svg/

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Masks are currently disabled, as the concept was flawed. Patterns still worked but the implementation is very hack, relying on NSGraphicsContext graphicsPort trickery (in platform/mac/GraphicsContextMac.mm).

Fix that properly by adding off-screen rendering functionality to GraphicsContext, and rewrite SVGPatternElement / SVGMaskElement to use the new static GraphicsContext::createImageContext function.

Attaching patch soon.

[Nikolas Zimmermann]

Created attachment 11903 [details]
Initial patch

Fixes all masking regressions, and speeds up gradients-on-stroke of text a lot.

[Oliver Hunt]

Comment on attachment 11903 [details]
Initial patch

Looks good.  Tell me just before you land :D

[Nikolas Zimmermann]

Created attachment 11913 [details]
Updated patch

Updated patch fixing all known problems. Uses optimal sized images for gradient-on-stroke of text now.

[Eric Seidel (no email)]

Comment on attachment 11913 [details]
Updated patch

Ok, I looked through this briefly.  A few thoughts.

1.  I'm not sure that GraphicsContext should carry a size.  By doing so, you're sorta making it both an image and a context.  Contexts (at least for CG) are infinite planes which know how to receive drawing commands.  A context made form an image just has an implicit clip.  Better would probably be a make a separate ImageBuffer object, which you could in turn make a GraphicsContext from.  You could then make the CGImage directly from the ImageBuffer object (the ImageBuffer could enve know how to cache a CGImage pointer, so you don't have to recreate one every time during the pattern callback -- which is likely slow, since it's at least one malloc, and possibly a buffer copy.)

2.  It feels weird to pass around GraphicsContext* as images.  Again, I think an ImageBuffer is in order here.

"drawMaskerContext" logic should just be generic logic on RenderObject.  Should be able to say "drawSubTree()" or something, possibly passing it a cliprect.  And it would just return you an ImageBuffer* or something.  Maybe you have to pass it the ImageBuffer, that might make more sense.

You should probably chat with Hyatt or Maciej (someone who is more current with the WebKit project than I).  All in all this is a *great* start.  I think it could be done a bit cleaner though, and doing it in a really clean way will be a huge benifit for the rest of the project, as this code can be re-used all over the place (mask, pattern, filter, <img> with SVG are just a few uses which pop to mind).

[Oliver Hunt]

Comment on attachment 11913 [details]
Updated patch

Looks sane to me, anyone else have any comments?

[Nikolas Zimmermann]

(In reply to comment #5)
> (From update of attachment 11913 [details] [edit])
> Looks sane to me, anyone else have any comments?

I'm preparing a new patch, adressing Eric's comments.

[Nikolas Zimmermann]

(In reply to comment #4)
> (From update of attachment 11913 [details] [edit])
> Ok, I looked through this briefly.  A few thoughts.
> 
> 1.  I'm not sure that GraphicsContext should carry a size.  By doing so, you're
> sorta making it both an image and a context.  Contexts (at least for CG) are
> infinite planes which know how to receive drawing commands.  A context made
> form an image just has an implicit clip.  Better would probably be a make a
> separate ImageBuffer object, which you could in turn make a GraphicsContext
> from.  You could then make the CGImage directly from the ImageBuffer object
> (the ImageBuffer could enve know how to cache a CGImage pointer, so you don't
> have to recreate one every time during the pattern callback -- which is likely
> slow, since it's at least one malloc, and possibly a buffer copy.)

Oh good catch Eric, regarding the patternCallback() thing!
This imageContextSize() stuff was really unclean, it's gone now
and a new "static ImageBuffer* createImageBuffer(const IntSize&, bool grayScale)"
has been added to GraphicsContext, this time w/O SVG_SUPPORT blocks, as it's
really generic enough now.

> 2.  It feels weird to pass around GraphicsContext* as images.  Again, I think
> an ImageBuffer is in order here.
Definately. All fixed by passing around ImageBuffer pointers now.
 
> "drawMaskerContext" logic should just be generic logic on RenderObject.  Should
> be able to say "drawSubTree()" or something, possibly passing it a cliprect. 
> And it would just return you an ImageBuffer* or something.  Maybe you have to
> pass it the ImageBuffer, that might make more sense.
Sounds nice, thining about that now.
 
> You should probably chat with Hyatt or Maciej (someone who is more current with
> the WebKit project than I).  All in all this is a *great* start.  I think it
> could be done a bit cleaner though, and doing it in a really clean way will be
> a huge benifit for the rest of the project, as this code can be re-used all
> over the place (mask, pattern, filter, <img> with SVG are just a few uses which
> pop to mind).
Absolutely agreeing, 4am was a bad time to think about design :-)

Keep tuned... patch coming soon

[Nikolas Zimmermann]

Created attachment 11915 [details]
Updated patch II

Incorporated all comments. Still no new regressions, all masking related stuff fixed. Heavy speedup for gradient-on-stroke.

[Nikolas Zimmermann]

Created attachment 11916 [details]
Final patch

This fixes all comments, from IRC review with Sam.

[Nikolas Zimmermann]

Landed in r18315.