Bug 232201

Summary: Allow ImageDiff to read from files
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, gsnedders, jbedard, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 232212    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch darin: review+

Description Simon Fraser (smfr) 2021-10-23 11:25:11 PDT
Allow ImageDiff to read from files
Comment 1 Simon Fraser (smfr) 2021-10-23 11:26:48 PDT
Created attachment 442267 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-10-23 11:28:17 PDT
I also wonder whether we should compile ImageDiff with optimization enabled in debug builds (since that will improve its performance).
Comment 3 Simon Fraser (smfr) 2021-10-23 11:30:13 PDT
Created attachment 442269 [details]
Patch
Comment 4 Alexey Proskuryakov 2021-10-23 14:57:22 PDT
Ideally, I'd love to have ImageDiff rewritten in a scripting language, so that it doesn't need to be compiled. This would avoid the need to have macOS SDK when doing iOS testing, for example.

Performance doesn't matter too much, as ImageDiff is only invoked when hashes don't match.

What happens with all this stderr printing when running from run-webkit-tests?
Comment 5 Alexey Proskuryakov 2021-10-23 15:01:09 PDT
Very sad to see naive tolerance find its way into WebKit.
Comment 6 Simon Fraser (smfr) 2021-10-23 15:34:01 PDT
Comment on attachment 442269 [details]
Patch

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

> Tools/ImageDiff/ImageDiff.cpp:92
> +    bool verbose = true;

This should be false.
Comment 7 Simon Fraser (smfr) 2021-10-23 15:37:11 PDT
My plan is to (In reply to Alexey Proskuryakov from comment #4)
> Ideally, I'd love to have ImageDiff rewritten in a scripting language, so
> that it doesn't need to be compiled. This would avoid the need to have macOS
> SDK when doing iOS testing, for example.
> 
> Performance doesn't matter too much, as ImageDiff is only invoked when
> hashes don't match.

We'd need to invoke some library to decode a PNG into pixel data if doing it in script.

Maybe we should just commit an ImageDiff binary?

> What happens with all this stderr printing when running from
> run-webkit-tests?

It shows up with --debut-rwt-logging. The patch has it true by default by mistake.

> Very sad to see naive tolerance find its way into WebKit

I've discovered that the built-in tolerance is enough to fail to distinguish between rgb(0, 127, 0) and rgb(0, 128, 0), which seems really bad when testing color component rounding. I plan to experiment with removing the built-in tolerance, but implementing WPT-style tolerance and using that when necessary.
Comment 8 Simon Fraser (smfr) 2021-10-23 21:06:37 PDT
(In reply to Alexey Proskuryakov from comment #4)
> Ideally, I'd love to have ImageDiff rewritten in a scripting language, so
> that it doesn't need to be compiled. This would avoid the need to have macOS
> SDK when doing iOS testing, for example.

Turns out wpt has this already:

    def get_differences(self, screenshots, urls, page_idx=None):
        from PIL import Image, ImageChops, ImageStat

        lhs = Image.open(io.BytesIO(base64.b64decode(screenshots[0]))).convert("RGB")
        rhs = Image.open(io.BytesIO(base64.b64decode(screenshots[1]))).convert("RGB")
        self.check_if_solid_color(lhs, urls[0])
        self.check_if_solid_color(rhs, urls[1])
        diff = ImageChops.difference(lhs, rhs)
        minimal_diff = diff.crop(diff.getbbox())
        mask = minimal_diff.convert("L", dither=None)
        stat = ImageStat.Stat(minimal_diff, mask)
        per_channel = max(item[1] for item in stat.extrema)
        count = stat.count[0]

We'd have to teach it about webkit-style diffing, or migrate entirely to WPT-style diffs.
Comment 9 Darin Adler 2021-10-23 22:24:39 PDT
Comment on attachment 442269 [details]
Patch

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

> Tools/ImageDiff/ImageDiff.cpp:66
> +    std::unique_ptr<PlatformImage> diffImage = actualImage->difference(*baselineImage, difference);

auto?

> Tools/ImageDiff/ImageDiff.cpp:102
> +                fprintf(stderr, "Failed to read numberic tolerance value from %s\n", argv[i + 1]);

Typo in “numeric” here

> Tools/ImageDiff/ImageDiff.cpp:135
> +            if (!strncmp(file1Path, "-", 1)) {

Can check he first character just by dereferencing. Don’t need strncmp:

    If (file1Path[0] == ‘-‘)

Since it’s a null-terminated string.

> Tools/ImageDiff/ImageDiff.cpp:140
> +            if (!strncmp(file2Path, "-", 1)) {

Ditto.

> Tools/ImageDiff/ImageDiff.cpp:161
> +            auto result = processImages(std::exchange(actualImage, { }), std::exchange(baselineImage, { }), tolerance);

I’d use std::move or WTFMove here. No reason we need to clear out the local variable since it’s unused. Unlike the different case below where it’s part of a loop.

> Tools/ImageDiff/ImageDiff.cpp:165
> +            return EXIT_SUCCESS;

Why not just return the result of processImages?

> Tools/ImageDiff/ImageDiff.cpp:175
> +            fprintf(stderr, "ImageDiff: read %lu bytes from stdin %s", strlen(buffer), buffer);

strlen is size_t so this ought to be %zu, or something else even more portable that doesnot rely on size_t being same size as long.

> Tools/ImageDiff/ImageDiff.cpp:214
> +            if (result == EXIT_FAILURE)

Or if result != sucess, return result

> Tools/ImageDiff/cg/PlatformImageCG.cpp:76
> +    CGDataProviderRef dataProvider = CGDataProviderCreateWithFilename(filePath);

I’d use auto more and adoptCF
Comment 10 Simon Fraser (smfr) 2021-10-23 23:35:56 PDT
(In reply to Darin Adler from comment #9)
> Comment on attachment 442269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442269&action=review
>
> I’d use auto more and adoptCF

This code doesn't pull in any of WTF (because it gets built in weird configs like building for macOS on iOS builders), hence the manual retain/release.
Comment 11 Sam Sneddon [:gsnedders] 2021-10-24 02:26:41 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> (In reply to Alexey Proskuryakov from comment #4)
> > Ideally, I'd love to have ImageDiff rewritten in a scripting language, so
> > that it doesn't need to be compiled. This would avoid the need to have macOS
> > SDK when doing iOS testing, for example.
> 
> Turns out wpt has this already:
> 
>     def get_differences(self, screenshots, urls, page_idx=None):
>         from PIL import Image, ImageChops, ImageStat
> 
>         lhs =
> Image.open(io.BytesIO(base64.b64decode(screenshots[0]))).convert("RGB")
>         rhs =
> Image.open(io.BytesIO(base64.b64decode(screenshots[1]))).convert("RGB")
>         self.check_if_solid_color(lhs, urls[0])
>         self.check_if_solid_color(rhs, urls[1])
>         diff = ImageChops.difference(lhs, rhs)
>         minimal_diff = diff.crop(diff.getbbox())
>         mask = minimal_diff.convert("L", dither=None)
>         stat = ImageStat.Stat(minimal_diff, mask)
>         per_channel = max(item[1] for item in stat.extrema)
>         count = stat.count[0]
> 
> We'd have to teach it about webkit-style diffing, or migrate entirely to
> WPT-style diffs.

Will follow up more on Monday, but there was some intention to move over to using Pillow (i.e., the PIL fork that WPT uses), though there were some (Apple-internal) blockers that I can't remember the status of.

The WebKit-style diffing is only used for pixel tests, though, right? For reftests it is always run with zero tolerance currently.
Comment 12 Simon Fraser (smfr) 2021-10-24 11:13:35 PDT
https://trac.webkit.org/changeset/284762/webkit
Comment 13 Radar WebKit Bug Importer 2021-10-24 11:14:19 PDT
<rdar://problem/84595422>
Comment 14 Alexey Proskuryakov 2021-10-24 15:35:22 PDT
> We'd need to invoke some library to decode a PNG into pixel data if doing it in script.

Right. It's possible that python has what we need, but we haven't been able to find a nice enough path forward with that yet.

> Maybe we should just commit an ImageDiff binary?

I think that on the balance, it's better do build ImageDiff on demand as we do now, but we'll need to reevaluate if we ever do run tests on machines without a functional macOS SDK.