Make RasterImage::DoComposite and friends threadsafe

RESOLVED FIXED in mozilla24

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla24
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Created attachment 757492 [details]
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
(Assignee)

Comment 2

5 years ago
Felipe, you're almost certainly seeing either bug 876499 or bug 876332. Nothing has landed on this bug.
(Assignee)

Comment 3

5 years ago
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.
Whiteboard: [leave open]
(Assignee)

Updated

5 years ago
Depends on: 882173
(Assignee)

Updated

5 years ago
Attachment #757492 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 762274 [details] [diff] [review]
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
Attachment #762274 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

5 years ago
Created attachment 763594 [details] [diff] [review]
Factor out all blending into a helper class FrameBlender
Attachment #763594 - Flags: review?(seth)
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Comment 7

5 years ago
Note: this patch implements parts 2 and 3 from comment 3, simply because it was easy to do so.
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.
Attachment #763594 - Flags: review?(seth) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 764357 [details] [diff] [review]
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
Attachment #764357 - Flags: review?(seth)
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
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".
Attachment #764357 - Flags: review?(seth) → review+
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.
Attachment #762274 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 15

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

Updated

5 years ago
Depends on: 888607
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.
(Assignee)

Comment 18

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

Updated

5 years ago
Depends on: 900200
You need to log in before you can comment on or make changes to this bug.