867774 - Make RasterImage::DoComposite and friends threadsafe

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

Going on the premise that we're going to leave animated GIF frames in their 8-bit paletted form in order to save memory, we're going to need to make RasterImage::DoComposite() (the function that blends frames together to make an output frame) threadsafe.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Hang with DoComposite, DrawFrameTo

I got a hang yesterday and after Firefox was killed, the OSX crash tool reported the attached stack. Is this likely to be this bug or should I file a new one and attach this report? I haven't seem it happening again

Comment 2

[Joe Drew (not getting mail)]

Felipe, you're almost certainly seeing either bug 876499 or bug 876332. Nothing has landed on this bug.

Comment 3

[Joe Drew (not getting mail)]

Plan of attack, with the intent to land all patches once they're available (i.e., be continuously green):

1. Make blending helper functions ClearFrame, CopyFrameImage, DrawFrameTo work on raw pointers instead of imgFrame*, and make sure they're threadsafe at the same time.
2. Build a FrameBlender class that tears these ClearFrame, CopyFrameImage, DrawFrameTo, as well as DoComposite, out of RasterImage. This FrameBlender will actually *own* the imgFrame, and RasterImage will own it in turn. (That way, RasterImage can ask for the most recent imgFrame, and never have to worry about the details of how it came to be.)
3. Use this FrameBlender from RasterImage when requested to by the refresh driver.
4. Write tests for image formats to ensure blending and frame disposal are handled correctly.

Comment 4

[Joe Drew (not getting mail)]

Attachment: make blending helper functions threadsafe

This patch changes ClearFrame, CopyFrameImage, and DrawFrameTo to work on raw pointers rather than using imgFrame* and Thebes. The only external code it uses is pixman to do [AX]RGB blending of 32-bit frames.

This doesn't change DoComposite() at all, but it sets the stage for us to do so in future.

https://tbpl.mozilla.org/?tree=Try&rev=7b424e9c4544

Comment 5

[Joe Drew (not getting mail)]

Attachment: Factor out all blending into a helper class FrameBlender

Comment 6

[Joe Drew (not getting mail)]

Arg, forgot to comment. From the patch:

Create a FrameBlender class that holds on to an image's frames and can blend frames together on demand (while leaving the decision as to which frames to external users).

FrameBlender steals RasterImage::mFrames, RasterImage::DoComposite, and the
RasterImage blending helper functions CopyFrameImage, DrawFrameTo, and
ClearFrame. Now RasterImage doesn't hold on to its frames directly, and defers
all blending to FrameBlender::DoComposite.

The basic idea here is that we'll eventually have two separate drivers of animated images, one driven off the main thread, the other driven from inside the main thread (i.e., RasterImage as it is after this patch). The latter will be very task-driven: it will only deal with animations that loop, because those don't involve image state changes (i.e. from animating -> not animating purely because of the image animating).

https://tbpl.mozilla.org/?tree=Try&rev=adefaba9048d

Comment 7

[Joe Drew (not getting mail)]

Note: this patch implements parts 2 and 3 from comment 3, simply because it was easy to do so.

Comment 8

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

Comment on attachment 763594 [details] [diff] [review]
Factor out all blending into a helper class FrameBlender

Review of attachment 763594 [details] [diff] [review]:
-----------------------------------------------------------------

Love it!

::: image/src/FrameBlender.cpp
@@ +15,5 @@
> +using namespace mozilla::image;
> +
> +// a mask for flags that will affect the decoding
> +#define DECODE_FLAGS_MASK (imgIContainer::FLAG_DECODE_NO_PREMULTIPLY_ALPHA | imgIContainer::FLAG_DECODE_NO_COLORSPACE_CONVERSION)
> +#define DECODE_FLAGS_DEFAULT 0

These don't appear to be actually used. May as well remove them.

Comment 9

[Joe Drew (not getting mail)]

Attachment: animated image blending tests

This is a set of reftests that should exercise all the different animated image blending and disposal types, both GIF and APNG.

This set of reftests should pass both before and after these patches. Thus, I've pushed it to try before:

https://tbpl.mozilla.org/?tree=Try&rev=b7c68f6acc83

and after:

https://tbpl.mozilla.org/?tree=Try&rev=ff8c54f25358

Comment 10

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

Comment on attachment 764357 [details] [diff] [review]
animated image blending tests

Review of attachment 764357 [details] [diff] [review]:
-----------------------------------------------------------------

Fine with me. Looks like you could combine delaytest.html and nodelaytest.html by adding another query parameter. Also, small nit: I kinda prefer "delay-test" and "no-delay-test", as with "nodelaytest" it's easy to assume the first word is "node".

Comment 11

[Joe Drew (not getting mail)]

Ahem, let's give that another try:

https://tbpl.mozilla.org/?tree=Try&rev=4dc092ba7fac

and


https://tbpl.mozilla.org/?tree=Try&rev=3cf98f90b771

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 762274 [details] [diff] [review]
make blending helper functions threadsafe

Review of attachment 762274 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +48,5 @@
>  
>  #include "GeckoProfiler.h"
>  #include <algorithm>
>  
> +#define PIXMAN_DONT_DEFINE_STDINT

Do you need this?

@@ +2329,5 @@
>  nsresult
> +RasterImage::DrawFrameTo(uint8_t *aSrcData, const nsIntRect& aSrcRect,
> +                         uint32_t aSrcPaletteLength, bool aSrcHasAlpha,
> +                         uint8_t *aDstPixels, const nsIntRect& aDstRect,
> +                         uint32_t aBlendMethod)

aDstPixels could be uint32_t. aBlendMethod should be an enumerated type.

@@ +2412,5 @@
> +                           aSrcRect.x, aSrcRect.y,
> +                           aDstRect.width, aDstRect.height);
> +
> +  pixman_image_unref(src);
> +  pixman_image_unref(dst);

Move this into an else.

Comment 13

[Joe Drew (not getting mail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0380ec657f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7158e859e28c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f8e2d6d991

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f0380ec657f8
https://hg.mozilla.org/mozilla-central/rev/7158e859e28c
https://hg.mozilla.org/mozilla-central/rev/f5f8e2d6d991

Comment 15

[Joe Drew (not getting mail)]

And, because I don't know how to qrefresh: https://hg.mozilla.org/integration/mozilla-inbound/rev/bca4f298207c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bca4f298207c

Comment 17

[Kartikaya Gupta (email:[e-mail])]

FYI these patches seem to have moved about 2044 KiB from "heap-unclassified" to "images" on http://areweslimyet.com/mobile (see the bottom graph, where the red line jumps circa June 18. Not sure if that's expected or not.

Comment 18

[Joe Drew (not getting mail)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> FYI these patches seem to have moved about 2044 KiB from "heap-unclassified"
> to "images" on http://areweslimyet.com/mobile (see the bottom graph, where
> the red line jumps circa June 18. Not sure if that's expected or not.

Yeah, we are now accounting for a couple of objects that we weren't before.