ImageDiff no longer needs a --tolerance argument, and fix sometimes-black diff images
Created attachment 442897 [details] Patch
*** Bug 232467 has been marked as a duplicate of this bug. ***
Comment on attachment 442897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442897&action=review Looks good. Just a few suggestions before landing. > Tools/ImageDiff/ImageDiff.cpp:111 > + "usage: ImageDiff [-h] [-v] [-d] [-e] [-t TOLERANCE] ([actualImage baselineImage] | <stdin>)\n" \ I think you should remove `[-t TOLERANCE]` from this line now as well. > Tools/ImageDiff/ImageDiff.cpp:123 > " -t, --tolerance TOLERANCE\n" \ > " compare the images with the given tolerance\n" These two lines should be removed as well. > Tools/ImageDiff/PlatformImage.cpp:101 > + float pixel = static_cast<float>(diffPixel[p]); Is it possible that this cast is unnecessary? > Tools/ImageDiff/PlatformImage.cpp:105 > + diffPixel[p] = static_cast<unsigned char>(pixel); Ditto.
Ah, there is also the style error which seems pretty simple to fix as well.
(In reply to Martin Robinson from comment #3) > Comment on attachment 442897 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=442897&action=review > > Looks good. Just a few suggestions before landing. > > > Tools/ImageDiff/ImageDiff.cpp:111 > > + "usage: ImageDiff [-h] [-v] [-d] [-e] [-t TOLERANCE] ([actualImage baselineImage] | <stdin>)\n" \ > > I think you should remove `[-t TOLERANCE]` from this line now as well. > > > Tools/ImageDiff/ImageDiff.cpp:123 > > " -t, --tolerance TOLERANCE\n" \ > > " compare the images with the given tolerance\n" > > These two lines should be removed as well. Thanks, I know I did that in some other incarnation of this patch! > > > Tools/ImageDiff/PlatformImage.cpp:101 > > + float pixel = static_cast<float>(diffPixel[p]); > > Is it possible that this cast is unnecessary? > > > Tools/ImageDiff/PlatformImage.cpp:105 > > + diffPixel[p] = static_cast<unsigned char>(pixel); Both casts are not necessary but I like having them because it makes them clear to the reader.
Style warning is unavoidable; we don't link ImageDiff with WTF so can only use std:: types.
https://trac.webkit.org/changeset/285109/webkit
<rdar://problem/84884082>