Summary: | Bring back masking & implement patterns/mask properly in ksvg2/svg/ | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||
Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | ||||||||||||
Priority: | P2 | ||||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Nikolas Zimmermann
2006-12-18 12:22:15 PST
Created attachment 11903 [details]
Initial patch
Fixes all masking regressions, and speeds up gradients-on-stroke of text a lot.
Comment on attachment 11903 [details]
Initial patch
Looks good. Tell me just before you land :D
Created attachment 11913 [details]
Updated patch
Updated patch fixing all known problems. Uses optimal sized images for gradient-on-stroke of text now.
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).
Comment on attachment 11913 [details]
Updated patch
Looks sane to me, anyone else have any comments?
(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. (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 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.
Created attachment 11916 [details]
Final patch
This fixes all comments, from IRC review with Sam.
Landed in r18315. |