Bug 4193 – Add pixel-based regression tests for svg

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Add pixel-based regression tests for svg

Now that svg2png has made it into CVS, it's time to add a set of pixel based regression tests for SVG.

KDE already has their own version of such:
http://websvn.kde.org/trunk/tests/ksvgtests/

as well as:
http://websvn.kde.org/trunk/kdenonbeta/ksvg2/scripts/

The OpenClipart library might make a great starting point (or some of the files in such, rather):
http://www.openclipart.org/

I'm hoping those with QA tendencies will go to town on this one.  The script could be modeled after 
kde's regressiontest.sh, or webkit-tests, or a combination thereof.

I'm also happy to patch svg2png to behave like DumpRenderTree does and take file names on stdin as 
necessary.

[Ben La Monica]

Created attachment 3213 [details]
Added diff support to svg2png

Here is the initial diff support for svg2png. This we, we can use Core Image,
and we don't have to have ImageMagick installed.

Please look over the memory usage. I'm not sure if I deallocate everything
properly (still new to Obj-C)

[Eric Seidel (no email)]

Comment on attachment 3213 [details]
Added diff support to svg2png

Some thoughts: 
pojo: a positive integer is an integer > 0... your log message is sorta
redundant

you still have tabs, choose "re-indent" from the format menu

and I'm still not comfortable with --diff not workign for multiple files...
although that could be a "first step" if necessary

[Eric Seidel (no email)]

The memory looked fine (from my cursory overview).  I would suggest that you make --diff work like --
output, have it be an "explicitDiffPath", and have --diff-suffix set a special suffix which is appended to the 
file name (like "pngPath") works currently.  If --diff-suffix is set, then you can use assume you are to 
dump diffs and do so with the appropriate suffix, one per file passed.  The reason why we want to design 
for multiple files, is that the eventual plan is to work like DumpRenderTree and read multiple files from 
stdin.

[Ben La Monica]

Created attachment 3217 [details]
multiple svgs can be tested now

Can now test multple svgs at once using the --test method.

ie:

svg2png --test -reference hisc-appl-konqueror.svg

Will create hisc-appl-konqueror.png in the same directory, and will compare it
against hisc-appl-konqueror-reference.png.

[Ben La Monica]

Created attachment 3219 [details]
Fixed several bugs, removed tabs, cleaned up code, fixed help

[Ben La Monica]

Created attachment 3236 [details]
Tentative final patch for svg2png

Here is the tentative final patch for svg2png.

* Separated the functions out into a separate file for inclusion into other
apps.
* Added an Animated GIF showing the two images side by side (still needs a
little work to get it to loop)
* Finally got the diff to read out of the image in memory instead of loading it
from file again.

[Eric Seidel (no email)]

Comment on attachment 3236 [details]
Tentative final patch for svg2png

We've discussed several issues over IRC (including spacing, function breakout,
copyright) which will need to be fixe before review:+

[Ben La Monica]

Created attachment 3238 [details]
Fixed the issues that were discussed in IRC

* Save the file inside of main now instead of in the individual functions
* Clarified some of the logic
* Improved some of the variable names to make code more readable
* removed the NSLog functions, replaced with printf

[Darin Adler]

Comment on attachment 3238 [details]
Fixed the issues that were discussed in IRC

This is great. We'll eventually want to use this for WebKit outside SVG to.

But the framework search path that uses ../../../../WebKitBuild is incorrect.
That hardcodes the particular directory structure you are using and won't work
with people who have a different build results path.

[Eric Seidel (no email)]

I plan to prepare a patch to DumpRenderTree to add this same functionality once this lands.

[Eric Seidel (no email)]

A few more corrections ben, since this is going through one more round of fixes:

#1 there are still some areas where spacing is not in line with the webkit guidlines.  There is always a 
space after a comma.  As an example:
saveAnimatedGIFToFile(svgBitmap,referenceBitmap,animatedGIFPath);
should have spaces.

#2 I think you didn't quite understand mjs' comment about inline-data.  The arguments:
int maxWidth, int maxHeight
are still carrying inline data, even if you "parse it out" into bools in the lines:
if (maxWidth > 0)
    isMaxWidthSpecified = YES;

I still don't understand why you don't just make maxWidth=500 by default (in main())?  There are only 
two behaviors.  1.  specify a maxWidth,height.  2.  dont' specify one, and it defaults to 500,500.  There 
is no way to have the png grow without bounds, or?  By specifying 0?  You could consider making all 4 
values isMaxWidthSpecified, isMaxHeightSpecified, maxWidth, maxHeight globals... then you wouldnt' 
have to pass them into your function call.  Again, I'm not sure you need isMaxHeightSpecified, etc. 
unless your desire is to support unbounded growth.

#3 why?
-                printf("Unrecognized option %@\n", argString);
+                printf("Unrecognized option %s\n", [argString cString]);
does %@ not work?  either way, you should be using UTF8String instead of cString (deprecated).  Or you 
can just use "arg" directly, since that's already the char * version of the string.

#4 personally I think that
+            NSString *animatedGIFPath = [[svgPNGPath stringByDeletingPathExtension]
+                stringByAppendingPathExtension:@"gif"];
should use the -diff filename instead.

#5 to answer darin's concern, just open up the target inspector, and remove the "frameworks search 
path" variable there.  Xcode added that for you when you added WebCore+SVG.  Removing it will cause 
things to work correctly on all systems.

#6 is this still valid?
+ * TODO: Get the images to loop forever with a 1 sec delay.
I know that's possible with CG.

[Ben La Monica]

Created attachment 3260 [details]
Fixed changes, added output-dir and baseline-dir

Fixed the items we discussed in the above comment.
Added an output-dir option to allow the output images to be put in a separate
directory
Added a baseline-dir option to allow the baseline images to be in a separate
directory
Fixed the animated gif.

[Eric Seidel (no email)]

Comment on attachment 3260 [details]
Fixed changes, added output-dir and baseline-dir

Ben messaged me on IRC to say there was "something wrong with this patch and to
not approve until he had fixed it in the morning"

[Darin Adler]

Comment on attachment 3260 [details]
Fixed changes, added output-dir and baseline-dir

Looks fine, r=me.

[Ben La Monica]

Created attachment 3264 [details]
Fixed small issues

Reordered some CFReleases, fixed some comments, and fixed the xcode project
file.

[Ben La Monica]

Created attachment 3265 [details]
cvs diff patch

This patch is from the cvs diff. I can't add the two files that are necessary
because they aren't in cvs.

[Ben La Monica]

Created attachment 3266 [details]
ImageDiff.h

[Ben La Monica]

Created attachment 3267 [details]
ImageDiff.m

[Eric Seidel (no email)]

So the Tool is in.... but this bug really isn't complete w/o a way to run the tool over the svg test suite.