Closed Bug 1196065 Opened 4 years ago Closed 4 years ago

Add sanity tests for image decoders

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1196066
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.
Attachment #8649595 - Flags: review?(tnikkel)
In this version I removed the commented out code and added more verbose comments.
Attachment #8649659 - Flags: review?(tnikkel)
Attachment #8649595 - Attachment is obsolete: true
Attachment #8649595 - Flags: review?(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.
Attachment #8649659 - Flags: review?(tnikkel) → review+
(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.
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
It looks like the NS_INLINE_DECL_REFCOUNTING for NoResume needed to become
NS_INLINE_DECL_REFCOUNTING(NoResume, override).
Attachment #8649659 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8551bc98b4be
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.