Bug 232522

Summary: ImageDiff no longer needs a --tolerance argument, and fix sometimes-black diff images
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glenn, gsnedders, jbedard, mrobinson, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149828    
Attachments:
Description Flags
Patch mrobinson: review+

Simon Fraser (smfr)
Reported 2021-10-29 21:32:35 PDT
ImageDiff no longer needs a --tolerance argument, and fix sometimes-black diff images
Attachments
Patch (15.12 KB, patch)
2021-10-29 21:36 PDT, Simon Fraser (smfr) 		mrobinson: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2021-10-29 21:36:45 PDT
Created attachment 442897 [details] Patch
Simon Fraser (smfr)
Comment 2 2021-10-29 22:59:28 PDT
*** Bug 232467 has been marked as a duplicate of this bug. ***
Martin Robinson
Comment 3 2021-11-01 07:22:03 PDT
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.
Martin Robinson
Comment 4 2021-11-01 07:24:32 PDT
Ah, there is also the style error which seems pretty simple to fix as well.
Simon Fraser (smfr)
Comment 5 2021-11-01 08:21:35 PDT
(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.
Simon Fraser (smfr)
Comment 6 2021-11-01 08:22:02 PDT
Style warning is unavoidable; we don't link ImageDiff with WTF so can only use std:: types.
Simon Fraser (smfr)
Comment 7 2021-11-01 09:24:06 PDT
https://trac.webkit.org/changeset/285109/webkit
Radar WebKit Bug Importer
Comment 8 2021-11-01 09:25:20 PDT
<rdar://problem/84884082>
Note You need to log in before you can comment on or make changes to this bug.