1196065 - Add sanity tests for image decoders

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

In bug 1193119, and probably more bugs following that one, there is some major refactoring happening in the GIF decoder code. I think it's important that we have some sanity tests for the image decoders, to help verify that this refactoring is correct.

We can gradually add to these tests, but for now I'm mostly interested in verifying that the decoders generally work correctly in these two cases:

1. When the data is delivered as a single chunk.
2. When the data is delivered as a series of single-byte chunks.

These two scenarios test that the 'hold' mechanism (which the decoders use to deal with the situation where not all the data is available) works in the most extreme scenarios. Hopefully this ensures that it works in general.

Comment 1

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

Attachment: Add sanity tests for image decoders

Here's the patch. This is almost entirely test-only changes, but there are two
minor non-test changes I made to facilitate these tests:

1. I got rid of a warning in SourceBuffer.cpp that these tests trigger. I'd
   rather not spam that warning thousands of times in everyone's logs.

2. I made it possible to pass an alternate IResumable to Decoder::Decode(), to
   make the multichunk tests possible. (Since there's no DecodePool running
   during those tests, the default implementation of IResumable that Decoder
   uses wouldn't work.)

Note that I just noticed that there's some commented out code in the test file.
I'll fix that before landing this, so please ignore that during your review.

Comment 2

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

Attachment: Add sanity tests for image decoders

In this version I removed the commented out code and added more verbose comments.

Comment 3

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

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

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8649659 [details] [diff] [review]
Add sanity tests for image decoders

FYI, you can also use .sjs files and mochitest to introduce arbitrary delays when sending images.

Comment 5

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

(In reply to Timothy Nikkel (:tn) from comment #4)
> Comment on attachment 8649659 [details] [diff] [review]
> Add sanity tests for image decoders
> 
> FYI, you can also use .sjs files and mochitest to introduce arbitrary delays
> when sending images.

Thanks for the review!

You're right; we could've introduced something like this much earlier.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cafd6b3967b

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/066ad55e75a9

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/066ad55e75a9 because of static build failure:


/builds/slave/m-in-m64-st-an-d-0000000000000/build/src/image/test/gtest/TestDecoders.cpp:120:3: error: 'AddRef' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
/builds/slave/m-in-m64-st-an-d-0000000000000/build/src/image/test/gtest/TestDecoders.cpp:120:3: error: 'Release' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
make[5]: *** [Unified_cpp_image_test_gtest0.o] Error 1
make[4]: *** [image/test/gtest/target] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [compile] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2
Return code: 2
'mach build' did not run successfully. Please check log for errors.
Running post_fatal callback...
Exiting -1

Comment 9

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

Attachment: Add sanity tests for image decoders

It looks like the NS_INLINE_DECL_REFCOUNTING for NoResume needed to become
NS_INLINE_DECL_REFCOUNTING(NoResume, override).

Comment 10

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

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

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8551bc98b4be

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8551bc98b4be