1207830 - Add GTests for downscale-during-decode

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

We need GTests for downscale-during-decode. To make these tests useful, we need to add an image comparison function to Common.h - single-color images aren't good enough to test downscaling.

I'll add regressions that we weren't able to add tests for yet as "See Also" bugs on this bug.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Make it possible to create an anonymous decoder that downscales.

OK, let's get these tests in; they're long overdue.

This patch is pretty trivial; it just lets us create an anonymous decoder (i.e.
a decoder without an associated RasterImage, the kind we use in GTests) that
does downscaling during decode. The additional parameter to
CreateAnonymousDecoder() and the code to implement it are identical to the
similar code for CreateDecoder(); simple enough.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add downscale-during-decode GTests for all image formats.

This patch adds the bulk of the tests. There's some refactoring to allow sharing
code between the regular decoder tests and the downscale-during-decode tests.
The main difference between the two cases is that we pass an output size (called
a 'target size' in older code; I need to go through and clean that up) to the
Decoder in the DDD case, and the expected output is different between the two
cases. The DDD testcases use the same alternating green-and-red images that we
use in TestDownscalingFilter.

There are tests for every image format. I've verified that bug 1261964 would've
been caught by these tests, as well.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add a test that downscaling ICOs with AND mask transparency doesn't cause disaster.

It'd be really nice to have better test coverage of ICOs with AND mask
transparency. This patch adds a DDD test for that situation. The testcase comes
from bug 1206836. Unfortunately it's a bit difficult to work with such images (I
can't figure out how to make ImageMagick produce them) so for now I'm forced to
use an existing image which isn't easily checked by the existing output testing
functions. As a result, this new test only verifies that the decoder produced a
surface of the correct size and so forth and doesn't crash (or, implicitly,
trigger an ASAN violation).

I've filed a followup bug for adding an image comparison function which would
allow us to verify the output, too. I think it's worth having this test in the
tree even without that, though.

Comment 4

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=918654548d5d

Comment 5

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e8a94b23f99

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8756728 [details] [diff] [review]
(Part 2) - Add downscale-during-decode GTests for all image formats.

Review of attachment 8756728 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for tests! Especially tests that provide coverage to places where real bugs were found.

Why does the log message have all those "New Binary File:" lines?

::: image/test/gtest/TestDecoders.cpp
@@ +202,5 @@
> +  WithSingleChunkDecode(aTestCase, Some(outputSize), [&](Decoder* aDecoder) {
> +    RefPtr<SourceSurface> surface = CheckDecoderState(aTestCase, aDecoder);
> +    if (!surface) {
> +      return;
> +    }

This surprised me. I think CheckDecoderState will only return null if the TEST_CASE_HAS_ERROR flag is set, right? Do these tests have that flag? If not, using EXPECT_TRUE (or similar) would seem to be more appropriate.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8756729 [details] [diff] [review]
(Part 3) - Add a test that downscaling ICOs with AND mask transparency doesn't cause disaster.

Review of attachment 8756729 [details] [diff] [review]:
-----------------------------------------------------------------

Again, why the "New binary File" line in the commit message?

Comment 8

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> This surprised me. I think CheckDecoderState will only return null if the
> TEST_CASE_HAS_ERROR flag is set, right? Do these tests have that flag? If
> not, using EXPECT_TRUE (or similar) would seem to be more appropriate.

Yep, I concur. I'll make that change (and add a comment as to why EXPECT_TRUE is preferable).

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add downscale-during-decode GTests for all image formats.

In this updated version of part 2 I implement the change requested above and
increase the fuzz by 1 on the first row.

Comment 10

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc7102718a99bc960054cbad9f178df932234c8
Bug 1207830 (Part 1) - Make it possible to create an anonymous decoder that downscales. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/acbbb2038fdd02faf8228587b5d3d6cf7a9fb0d6
Bug 1207830 (Part 2) - Add downscale-during-decode GTests for all image formats. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/f10f4b5ce594dd6004b5de023b4e53782ed9c0d5
Bug 1207830 (Part 3) - Add a test that downscaling ICOs with AND mask transparency doesn't cause disaster. r=njn

Comment 11

[Carsten Book [:Tomcat]]

this made gtests unhappy and had to back this out (sorry seth) for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=28884712&repo=mozilla-inbound

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/100c964f5e75
https://hg.mozilla.org/integration/mozilla-inbound/rev/40d54fc24c41
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b521813510a

Comment 13

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b4f32b9abdb

Comment 14

[Seth Fowler [:seth] [:s2h]]

(In reply to Carsten Book [:Tomcat] from comment #11)
> this made gtests unhappy and had to back this out (sorry seth) for test
> failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=28884712&repo=mozilla-
> inbound

It looks like somewhat more fuzz is necessary for the JPEG test on Windows. I noticed already that the JPEG test behaves differently between OS X and Linux. This is a bit bizarre to me; is libjpeg-turbo doing something platform-specific? It's also possible that it's a color management thing; if so, that should become clear pretty quickly when I add tests for color management, which is on my todo list.

Comment 15

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b02a890b94e4b1879f0bd8e2361444e17de5dc5
Bug 1207830 (Part 1) - Make it possible to create an anonymous decoder that downscales. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a61f536f0e6b992ddc587a2d4620e58da3a2fe6
Bug 1207830 (Part 2) - Add downscale-during-decode GTests for all image formats. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/7856fc2284735fbca3b33f76eb045c9ad3e8b8c9
Bug 1207830 (Part 3) - Add a test that downscaling ICOs with AND mask transparency doesn't cause disaster. r=njn

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add downscale-during-decode GTests for all image formats.

Increased the fuzz by one more notch. Looks like this unbusts Windows, per the
try job above.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b02a890b94e
https://hg.mozilla.org/mozilla-central/rev/3a61f536f0e6
https://hg.mozilla.org/mozilla-central/rev/7856fc228473