Bug 233609 - Store image resolution in layout test snapshots, and have ImageDiff read it
Summary: Store image resolution in layout test snapshots, and have ImageDiff read it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks: 232525
  Show dependency treegraph
 
Reported: 2021-11-29 17:43 PST by Simon Fraser (smfr)
Modified: 2021-11-30 10:05 PST (History)
4 users (show)

See Also:


Attachments
Patch (15.16 KB, patch)
2021-11-29 17:50 PST, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
Patch (15.14 KB, patch)
2021-11-29 19:47 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-11-29 17:43:13 PST
Store image resolution in layout test snapshots, and have ImageDiff read it
Comment 1 Simon Fraser (smfr) 2021-11-29 17:50:14 PST
Created attachment 445369 [details]
Patch
Comment 2 Darin Adler 2021-11-29 19:01:26 PST
Comment on attachment 445369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445369&action=review

> Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:67
> +    float resolutionWidth = 72.0 * scaleFactor;
> +    float resolutionHeight = 72.0 * scaleFactor;

Why convert double to float here instead of just using kCFNumberDoubleType?

> Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:69
> +    auto resolutionWidthNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionWidth));
> +    auto resolutionHeightNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionHeight));

Seems incorrect, but likely harmless, to use kCFNumberFloat32Type here instead of kCFNumberFloatType.

Better to just use doubles anyway, though.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:80
> +    auto properties = CGImageSourceCopyPropertiesAtIndex(imageSource, 0, nullptr);
> +    if (properties) {

Could scope the variable to the if.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:85
> +            if (CFNumberGetValue(resolutionXProperty, kCFNumberFloat32Type, &resolutionX) && CFNumberGetValue(resolutionYProperty, kCFNumberFloat32Type, &resolutionY)) {

kCFNumberFloatType is correct here, but also, could use double

> Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:120
> +    float resolutionWidth = 72.0 * imageSize.width / windowSize.width;
> +    float resolutionHeight = 72.0 * imageSize.height / windowSize.height;
> +    auto resolutionWidthNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionWidth));
> +    auto resolutionHeightNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionHeight));

Same comments as about about double and about kCFNumberFloatType.
Comment 3 Simon Fraser (smfr) 2021-11-29 19:42:39 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 445369 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445369&action=review
> 
> > Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:67
> > +    float resolutionWidth = 72.0 * scaleFactor;
> > +    float resolutionHeight = 72.0 * scaleFactor;
> 
> Why convert double to float here instead of just using kCFNumberDoubleType?
> 
> > Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:69
> > +    auto resolutionWidthNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionWidth));
> > +    auto resolutionHeightNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionHeight));
> 
> Seems incorrect, but likely harmless, to use kCFNumberFloat32Type here
> instead of kCFNumberFloatType.

Looking at the CF source, it seems that kCFNumberFloatType and kCFNumberFloat32Type are synonymous, as are kCFNumberFloat64Type and kCFNumberFloatDoubleType.
Comment 4 Simon Fraser (smfr) 2021-11-29 19:47:03 PST
Created attachment 445372 [details]
Patch
Comment 5 EWS 2021-11-29 23:28:36 PST
Committed r286285 (244644@main): <https://commits.webkit.org/244644@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445372 [details].
Comment 6 Radar WebKit Bug Importer 2021-11-29 23:29:19 PST
<rdar://problem/85852852>
Comment 7 Darin Adler 2021-11-30 09:19:03 PST
(In reply to Simon Fraser (smfr) from comment #3)
> Looking at the CF source, it seems that kCFNumberFloatType and
> kCFNumberFloat32Type are synonymous, as are kCFNumberFloat64Type and
> kCFNumberFloatDoubleType.

Yes, understood. That’s what I meant when I said the mistake was "harmless".

But my comment is: why use the wrong one? One is intended for the "C float type", and that is kCFNumberFloatType.
Comment 8 Darin Adler 2021-11-30 09:19:54 PST
Very happy what you landed, of course. Glad you liked my suggestion of just using double.
Comment 9 Simon Fraser (smfr) 2021-11-30 10:05:53 PST
I originally followed the code in mayHaveDensityCorrectedSize().