Add GTests for downscale-during-decode

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
See Also: → bug 1213744
Whiteboard: [gfx-noted]
(Assignee)

Updated

2 years ago
Blocks: 1275824
(Assignee)

Updated

2 years ago
Depends on: 1261964
(Assignee)

Comment 1

2 years ago
Created attachment 8756727 [details] [diff] [review]
(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.
Attachment #8756727 - Flags: review?(n.nethercote)
(Assignee)

Comment 2

2 years ago
Created attachment 8756728 [details] [diff] [review]
(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.
Attachment #8756728 - Flags: review?(n.nethercote)
(Assignee)

Comment 3

2 years ago
Created attachment 8756729 [details] [diff] [review]
(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.
Attachment #8756729 - Flags: review?(n.nethercote)
(Assignee)

Updated

2 years ago
Blocks: 1276061
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.
Attachment #8756728 - Flags: review?(n.nethercote) → review+
Attachment #8756727 - Flags: review?(n.nethercote) → review+
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?
Attachment #8756729 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 8

2 years ago
(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).
(Assignee)

Comment 9

2 years ago
Created attachment 8757154 [details] [diff] [review]
(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.
(Assignee)

Updated

2 years ago
Attachment #8756728 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
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
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
Flags: needinfo?(seth)
(Assignee)

Comment 14

2 years ago
(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.
Flags: needinfo?(seth)
(Assignee)

Comment 15

2 years ago
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
(Assignee)

Comment 16

2 years ago
Created attachment 8757490 [details] [diff] [review]
(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.
(Assignee)

Updated

2 years ago
Attachment #8757154 - Attachment is obsolete: true

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b02a890b94e
https://hg.mozilla.org/mozilla-central/rev/3a61f536f0e6
https://hg.mozilla.org/mozilla-central/rev/7856fc228473
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.