Bug 232201 – Allow ImageDiff to read from files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Allow ImageDiff to read from files

[Simon Fraser (smfr)]

Created attachment 442267 [details]
Patch

[Simon Fraser (smfr)]

I also wonder whether we should compile ImageDiff with optimization enabled in debug builds (since that will improve its performance).

[Simon Fraser (smfr)]

Created attachment 442269 [details]
Patch

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

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

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

(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.

[Darin Adler]

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

[Simon Fraser (smfr)]

(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.

[Sam Sneddon [:gsnedders]]

(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.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/284762/webkit

[Radar WebKit Bug Importer]

<rdar://problem/84595422>

[Alexey Proskuryakov]

> 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.