Closed Bug 1181863 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.
Depends on: 1184996
Depends on: 1187386
Blocks: 1187118
Depends on: 1187546
Summary: Make imgTools::DecodeImage work off-main-thread → Add a function similar to imgTools::DecodeImage which works off-main-thread
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.
Attachment #8638865 - Flags: review?(tnikkel)
Attachment #8638865 - Attachment is obsolete: true
Attachment #8638865 - Flags: review?(tnikkel)
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.
Attachment #8640092 - Flags: review?(tnikkel)
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.
Attachment #8640097 - Flags: review?(tnikkel)
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.
Attachment #8640102 - Flags: review?(tnikkel)
Minor tweak here to make sure we use the right calling convention for the callback.
Attachment #8640849 - Flags: review?(tnikkel)
Attachment #8640092 - Attachment is obsolete: true
Attachment #8640092 - Flags: review?(tnikkel)
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.
Attachment #8640850 - Flags: review?(tnikkel)
Attachment #8640102 - Attachment is obsolete: true
Attachment #8640102 - Flags: review?(tnikkel)
Blocks: 1189632
Attachment #8640849 - Flags: review?(tnikkel) → review+
Attachment #8640097 - Flags: review?(tnikkel) → review+
Attachment #8640850 - Flags: review?(tnikkel) → review+
Depends on: 1191347
You need to log in before you can comment on or make changes to this bug.