1181863 - Add a function similar to imgTools::DecodeImage which works off-main-thread

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

It'd be much better if imgTools::DecodeImage worked off-main-thread, especially now that we have DOM interfaces like ImageBitmap that can trigger decoding in workers.

I think the best way to do this is to eliminate DecodeImage's dependency on RasterImage and just implement it directly against Decoder/DecodePool. That will require reducing the coupling between Decoder/DecodePool and RasterImage, but I don't think that will actually be a huge amount of work.

Comment 1

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

Attachment: (Part 1) - Add imgITools::DecodeSurface() to allow image decoding without involving any main-thread-only objects

Finally, all the prerequisites are in place to implement this. Here's the patch.
Mostly pretty straightforward, although the code in imgTools::DecodeSurface() is
unfortunately a bit verbose because of the interaction with nsIInputStream.

Part 2 will follow with tests.

Comment 2

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

Attachment: (Part 1) - Add support for reading from nsIInputStreams directly to SourceBuffer

In the interests of cleaner code, I've added a new part 1 which moves the code
that reads from an nsIInputStream into a SourceBuffer directly into the
SourceBuffer class. This allows RasterImage and the new method we're adding in
part 2 to share the same code.

Comment 3

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

Attachment: (Part 2) - Add ImageOps::DecodeToSurface() to allow image decoding without involving any main-thread-only objects

This version of part 2 is the same except that:

- It uses the new code added in part 1 to append from an nsIInputStream to a
  SourceBuffer.

- I moved the new method from imgITools to ImageOps. Placing the new method on
  imgITools seemed pointless because it's a C++-only method, and putting it
  there requires an ugly boilerplate do_CreateInstance() call at every callsite.

Comment 4

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

Attachment: (Part 3) - Add tests for DecodeToSurface()

This part adds reasonably comprehensive tests for DecodeToSurface(). The patch
is a bit bigger than it otherwise might be because these are the first gtests in
ImageLib. I'd like to add many more in the long term, though, so I've tried to
keep things as reusable as possible.

Everything should be pretty straightforward - there are shared helper functions
and testcases in Common.h, with the testcases being implemented as data URIs.
Each of the tests loads one of these data URIs, packages up the resulting
nsIInputStream in a runnable, and runs DecodeToSurface() off-main-thread. We
then verify some basic properties of the surface, and walk over its data using
the IsSolidColor() function to verify that DecodeToSurface() produced the
results we expect. In this case, every test but the corrupt one is intended to
produce an all-green surface.

There's a slightly ugly thing happening here - I needed to force XPCOM to
initialize the image module at the beginning to ensure the right initialization
order - that's done in the ImageModuleAvailable test, as documented in the
comment there. Otherwise I got failures in the Necko code (!!). I don't like
this, but it doesn't seem worth resolving as part of this bug.

Comment 5

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

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

Comment 6

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

Attachment: (Part 1) - Add support for reading from nsIInputStreams directly to SourceBuffer

Minor tweak here to make sure we use the right calling convention for the callback.

Comment 7

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

Attachment: (Part 3) - Add tests for DecodeToSurface()

I reworked the tests to load the testcase images from a file instead of using
data URIs, because MSVC has a 16k limit on string literals. =\

Really, this may be for the best, though. Having the testcase images in separate
files is definitely convenient.

Comment 8

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

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

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e8dd82cbf50
https://hg.mozilla.org/integration/mozilla-inbound/rev/44dc3228c00b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7cd400d799

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca56c20306df

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e8dd82cbf50
https://hg.mozilla.org/mozilla-central/rev/44dc3228c00b
https://hg.mozilla.org/mozilla-central/rev/4d7cd400d799
https://hg.mozilla.org/mozilla-central/rev/ca56c20306df