Bug 5862

Summary: feDisplacementMap filter is not implemented
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: SVGAssignee: Oliver Hunt <oliver>
Status: CLOSED FIXED    
Severity: Normal    
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on:    
Bug Blocks: 5857    
Attachments:
Description Flags
First patch for displacement mapping
none
First patch for displacement mapping
none
Formatting changes
eric: review-
A few more formatting changes
oliver: review-
Fixed filter kernel
none
Updated to patch against ToT
eric: review+
Testcase
none
displacement map for testcase
none
Source image for testcase none

Description Oliver Hunt 2005-11-28 17:21:33 PST
 
Comment 1 Oliver Hunt 2006-02-08 18:10:45 PST
Created attachment 6352 [details]
First patch for displacement mapping

Okay this is the first version of the feDisplacementMap implementation.  Filter should be correct (per logic + quartz composer) however it's difficult to validate given test case (http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-filters-displace-01-f.html) fails to render the inputs correctly, let alone the output.  That said sphere map appears to be doing correct thing on page.
Comment 2 Oliver Hunt 2006-02-08 18:10:46 PST
Created attachment 6353 [details]
First patch for displacement mapping

Okay this is the first version of the feDisplacementMap implementation.  Filter should be correct (per logic + quartz composer) however it's difficult to validate given test case (http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-filters-displace-01-f.html) fails to render the inputs correctly, let alone the output.  That said sphere map appears to be doing correct thing on page.
Comment 3 Oliver Hunt 2006-02-08 18:29:22 PST
Created attachment 6354 [details]
Formatting changes

Damn formatting changes
Comment 4 Eric Seidel (no email) 2006-02-08 18:49:49 PST
Comment on attachment 6354 [details]
Formatting changes

This patch looks fine.

Ideally, we would want to remove the data duplication in the kcanvas objects, by making the KCanvasFEDisplacementMap object just fetch its needed data off of it's element() instead of storing a second copy locally.

Also, there are a couple tabs in the cikernel file which need to be fixed before we land.

also XChannelSelector() and m_XChannelSelector should be xChannelSelector() and m_xChannelSelector per our style guidelines.  (Or just completely removed per my above comment)

It also doesn't look like KCanvasFEDisplacementMap has a default constructor which sets things to sane default values (also unecessary if its data members are removed)

getVectorForChannel could be done using an array of floats of 0's, followed by setting one at the correct offset to 1 and returning.  Your method is also totally fine.

Since you don't have commit bit, I'm going to mark this as r-.  If you did, it would be find to land, making those tweaks as you landed.  This does not need another review, but a final patch should be posted.
Comment 5 Oliver Hunt 2006-02-09 18:40:43 PST
Created attachment 6375 [details]
A few more formatting changes
Comment 6 Eric Seidel (no email) 2006-02-09 21:07:47 PST
Comment on attachment 6375 [details]
A few more formatting changes

Fabulous, as always. r=me.
Comment 7 Oliver Hunt 2006-02-11 17:46:47 PST
Created attachment 6417 [details]
Fixed filter kernel

oops... correct the kernel so it works when the dispalcement map isn't completely symmetrical :)
Comment 8 Eric Seidel (no email) 2006-02-11 17:55:52 PST
Comment on attachment 6417 [details]
Fixed filter kernel

great!
Comment 9 Eric Seidel (no email) 2006-02-11 18:00:02 PST
Comment on attachment 6417 [details]
Fixed filter kernel

Hum... doens't apply cleanly though.  Let's get one more patch.  And I'll land your test case at that time too.
Comment 10 Oliver Hunt 2006-02-11 19:00:47 PST
Created attachment 6418 [details]
Updated to patch against ToT
Comment 11 Oliver Hunt 2006-02-11 19:01:43 PST
Created attachment 6419 [details]
Testcase
Comment 12 Oliver Hunt 2006-02-11 19:02:27 PST
Created attachment 6420 [details]
displacement map for testcase
Comment 13 Oliver Hunt 2006-02-11 19:03:24 PST
Created attachment 6421 [details]
Source image for testcase
Comment 14 Eric Seidel (no email) 2006-02-12 04:57:17 PST
Comment on attachment 6418 [details]
Updated to patch against ToT

Looks good.  Lets land this.