716140 - Multithreaded image decoding

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I couldn't find an existing bug for this, so here we are.

Comment 1

[Joe Drew (not getting mail)]

Thumbnail sketch of how this should probably work, based on discussion that Bobby, Jeff and I had (as I remember it, anyways):

All cache management, imgIDecoderObserver notification, etc remains on the main thread. We have a pool of worker threads that take as input raw image data (eg JPEG) and decompress it. As data comes in from the network, it is added to a buffer of "data to read", which is probably just the source data buffer that currently exists in RasterImage. The decoder has a pointer to its last read position. (Classic producer/consumer.)

There is a synchronization point before the first frame, at which time the decoder knows the size it needs, and it gets an imgFrame that's shared between it and the RasterImage. It then decodes the first frame into that imgFrame, updating the dirty rect and/or sending dirty notifications as necessary. This allows progressive display of the first frame of images.

Second and subsequent frames (eg animated images) *may* have a synchronization point with the main thread to retrieve an imgFrame, but don't have to, as we don't progressively display those frames. Therefore it's totally fine for a decoder to create imgFrames, decode into them, and then pass them to the main thread once done with them.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #0)
> I couldn't find an existing bug for this, so here we are.

The original bug was 78300. But it's so out-of-date that I thought it was better to just resolve it as a dupe of this one.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I promised bholley I'd do this if he did the prerequisite refactorings of the imagelib interfaces.  We'll see how badly I end up regretting that.

Comment 5

[Justin Lebar (not reading bugmail)]

As much as I want this, I'm not convinced it's Snappy:P1 now that bug 715308 has landed.  Do you have a testcase in hand which performs particularly badly on recent nightlies?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We're still awful at images coming out of the cache, right?

Comment 7

[(dormant account)]

(In reply to Justin Lebar [:jlebar] from comment #5)
> As much as I want this, I'm not convinced it's Snappy:P1 now that bug 715308
> has landed.  Do you have a testcase in hand which performs particularly
> badly on recent nightlies?

If you know more than I do, feel free to adjust priority. I don't have any evidence for this. I was swayed by the image-suck thread.

Comment 8

[Justin Lebar (not reading bugmail)]

I think this is at best a snappy P2.  But see also:

<jlebar> khuey, What's wrong with images coming out of cache?
<khuey> jlebar: if necko doesn't chunk things for us (which it doesn't when they come out of the cache) we sync decode anything under 150K compressed
<jlebar> khuey, We'll sync-decode anything under 150KB when decoding previously-discarded images, too, I think.
<jlebar> khuey, *that* I'd nom for snappy p1.  Much simpler than multithreaded decoding.  :)
<khuey> jlebar: sure
<khuey> I don't think we need multithreaded decoding if our main thread decoding isn't insane
<khuey> though we certainly still want it
<khuey> and our main thread decoding is insane
<khuey> but making it sane may be easier than exiling it
<jlebar> khuey, When we start putting not-huge images in the async decode pool, we might want to order them smallest-to-largest..

Comment 9

[Bobby Holley (:bholley)]

FWIW I think this won't be so hard once I fix all of the other stuff, which I'm currently in the process of doing (though I haven't had time to work on it for a few weeks).

Comment 10

[Andreas Gal :gal]

Attachment: patch

INCOMPLETE SKETCH, DON'T REVIEW, DON'T TEST. I just got bored on the flight to Taipei.

This patch add support to Decode to run off the main thread. All interactions with mObserver (imgIDecoderObserver) and mImage (RasterImage) are proxied back to the main thread. EnsureFrame is run on the main thread and we block the decoder thread until the operation is complete. All callbacks from the decoder are posted to the main thread, except when the decoder is running on the main thread, which is the async case. The caller expects the results to be available immediately, so we call the callback directly instead of posting it (RunOnMainThread).

What's missing:
- in RasterImage, setup a thread pool (single thread initially) and change the worker to run there
- make sure ->Write is only called from the decoder thread
- ShutdownDecoder has to be sent as runnable to the decoder thread

Comment 11

[Andreas Gal :gal]

async -> sync in comment #10

Comment 12

[Andreas Gal :gal]

Attachment: patch, part 2

This wires up the first patch with a decoder thread. Instead of posting an event to that thread's queue when shutting down a decoder, I made DecodeWorker->StopDecoding() force the worker into a safe point instead. The worker will immediately stop processing and it will not re-post itself and wake up the sleeping main thread, which then re-posts the worker and goes on to terminate the decoder. This is a bit cheesy and makes the MT wait, but its simple and this case should be rare enough to get away with this at least for v1 of the patch. The general machinery seems to work, minus a race condition crash due to lock/unlock of images. I will dig around a bit, I probably forgot to serialize some path in RasterImage.

Comment 13

[Justin Lebar (not reading bugmail)]

> blocking-kilimanjaro: --- → ?

IMO this should not block k9o.  All of the devices we're targeting that I'm aware of have one CPU core, so OMT decoding is not even a particularly big win.  But even if we were targeting dual-core devices, I'd still argue that this isn't a blocker: Insufficient /raw speed/ decoding images (such as you'd get from decoding on two cores in parallel, or decoding off the main core) isn't a problem, so much as our incredibly poor handling of decoded image data.

The big known problems we have with images are bug 689623 and bug 683290.  khuey is working on the latter.  To my knowledge, nobody is currently working on the former.  I'll go nom both of those bugs.

Comment 14

[Andreas Gal :gal]

This would help with using hardware decoders which are blocking.

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to Andreas Gal :gal from comment #14)
> This would help with using hardware decoders which are blocking.

Do you have a bug number?  All I see is bug 714408, which isn't quite right.

I wasn't aware that we had devices with hardware image decoders at all, and I'll wait until I read the bug before I ask why we care.  :)

Comment 16

[Marco Castelluccio [:marco]]

Maybe with bug 685516 and bug 783748, this is once again Snappy:P1?

Comment 17

[Joe Drew (not getting mail)]

I've looked into this, and I've outlined a thumbnail sketch of it below. Jeff, Justin, Kyle, Josh, I'd love to hear your thoughts.

(Starting with everything on the main thread, as is currently the case)

* Make the decoders accumulate their status into a standalone imgStatusTracker that is passed to the decoder
** This will require us to make it possible to have a record-only observer; the current observer records in the status tracker, and also notifies
* When a given chunk of decoding is done, the decoder passes its imgStatusTracker back to the image, which then "replays" the difference between its status and the decoder's to all of its listeners
* Make it so decoders only ever decode up to one full frame during a decode call, and return whether they've finished the frame along with their status tracker
* Whenever decoders need a new frame (i.e., we've just started or the decoder signals us that it's done the frame, and the channel still has more data), the image will send the decoder a new frame along with its data and status tracker.

Once we've reached this state, we can pretty much decouple the decoder event loop from the main event loop, since the data sets the main thread and the decoder thread are operating on should be disjoint.

The only wrinkle I can think of will be in progressively displaying images; normally we want to make it so only one frame reads and writes to a piece of data at a time, but we might have to bite the bullet and just read from the first frame while it's being written to.

Comment 18

[Jet Villegas (inactive)]

+cc: roc & milan, they'll want to check this out :)

Comment 19

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

Joe, you had mentioned something along the lines that you did a test with all image decoding disabled and it only produced a 1% improvement in Talos? If so, is there any more information about this? I'd like to understand what the actual bottlenecks are.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Seth Fowler [:seth] from comment #19)
> Joe, you had mentioned something along the lines that you did a test with
> all image decoding disabled and it only produced a 1% improvement in Talos?
> If so, is there any more information about this? I'd like to understand what
> the actual bottlenecks are.

I think it was Eric Butler (a long-ago intern) that did this in 2008. He didn't know ImageLib well at all. So unless joe has since repeated the experiment, I'd be skeptical of that number. I would guess it to be more in the realm of 15%, which is what we were (accidentally) measure as the perf win for decode-on-draw.

Comment 21

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

Yeah, that sounds a bit more like what I would've hoped for based on previous experience. The win here also likely depends on how efficient we are at discovering needed resources in the page (and hence feeding the decoders as quickly as possible); not sure where we stand there.

Comment 22

[Joe Drew (not getting mail)]

Going to attach and land patches as I go along in order to avoid bitrot.

Comment 23

[Joe Drew (not getting mail)]

Attachment: part 1: move imgStatusTrackerObserver from the .h file to the .cpp file

There's no point in exposing imgStatusTrackerObserver to status tracker consumers, since none of them care, and the status tracker and observer are pretty intimately related.

Comment 24

[Joe Drew (not getting mail)]

Attachment: part 2: don't use .gets() on RasterImage from decoders

If we're off the main thread, we obviously can't just interrogate our RasterImage, so use local data instead.

Comment 25

[Joe Drew (not getting mail)]

Attachment: part 3: merge imgIContainerObserver and imgIDecoderObserver

Thanks to Josh Matthews' heroics, imgIContainerObserver and imgIDecoderObserver are only implemented by one class, imgStatusTrackerObserver. There's no point in us having two of these interfaces, since they're not used or implemented in different places.

Comment 26

[Joe Drew (not getting mail)]

Attachment: part 4: make imgIDecoderObserver a C++ interface

It fills me with great joy to deCOMtaminate imgIDecoderObserver, though all of the heavy lifting was done by Josh Matthews. The new C++ interface is called imgDecoderObserver.

Comment 27

[Joe Drew (not getting mail)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c2c9ef4ef946
(win32 build run for kicks:  https://tbpl.mozilla.org/?tree=Try&rev=3fca7af4f594)

Comment 28

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 692046 [details] [diff] [review]
part 1: move imgStatusTrackerObserver from the .h file to the .cpp file

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

::: image/src/imgStatusTracker.h
@@ +194,5 @@
>    // List of proxies attached to the image. Each proxy represents a consumer
>    // using the image.
>    nsTObserverArray<imgRequestProxy*> mConsumers;
>  
> +  nsRefPtr<imgIDecoderObserver> mTrackerObserver;

Why the type change? Couldn't you just declare they imgStatusTrackerObserver type?

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 692048 [details] [diff] [review]
part 2: don't use .gets() on RasterImage from decoders

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

::: image/decoders/nsGIFDecoder2.cpp
@@ +218,5 @@
>      // Send a onetime invalidation for the first frame if it has a y-axis offset. 
>      // Otherwise, the area may never be refreshed and the placeholder will remain
>      // on the screen. (Bug 37589)
>      if (mGIFStruct.y_offset > 0) {
> +      nsIntRect r(0, 0, mGIFStruct.screen_width, mGIFStruct.y_offset);

I assume screen_width is the same value?

Comment 30

[Joe Drew (not getting mail)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #28)
> Comment on attachment 692046 [details] [diff] [review]
> part 1: move imgStatusTrackerObserver from the .h file to the .cpp file
> 
> Review of attachment 692046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgStatusTracker.h
> @@ +194,5 @@
> >    // List of proxies attached to the image. Each proxy represents a consumer
> >    // using the image.
> >    nsTObserverArray<imgRequestProxy*> mConsumers;
> >  
> > +  nsRefPtr<imgIDecoderObserver> mTrackerObserver;
> 
> Why the type change? Couldn't you just declare they imgStatusTrackerObserver
> type?

No, I may need multiple types to be held in that object depending on how development goes. Making it hold the base type means I can do that easily.

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 692052 [details] [diff] [review]
part 4: make imgIDecoderObserver a C++ interface

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

::: image/src/RasterImage.cpp
@@ +2888,5 @@
>  
>    if (status == SCALE_DONE) {
>      MOZ_ASSERT(request->done);
>  
> +    if (mObserver) {

This should still have a RefPtr unless you're sure otherwise.

::: image/src/VectorImage.cpp
@@ +727,5 @@
>    if (!mObserver)
>      return;
>  
> +  mObserver->FrameChanged(&nsIntRect::GetMaxSizedIntRect());
> +  mObserver->OnStopFrame();

This should still use a RefPtr

::: image/src/imgDecoderObserver.cpp
@@ +5,5 @@
> +
> +#include "imgDecoderObserver.h"
> +
> +imgDecoderObserver::~imgDecoderObserver()
> +{}

This can go away and just make the destructor abstract.

Comment 32

[Justin Lebar (not reading bugmail)]

> tracking-b2g18: --- → ?

What does this flag mean?

My general feeling is that we shouldn't be taking imagelib fixes on b2g at this point.  Nearly every time we touch imagelib we regress some edge case.

Additionally, the benefit of this patch on B2G would be relatively low aiui because B2G devices are single-core, and we already yield while decoding -- that is, the current code is a reasonable simulation of multi-threadedness on a single-core device.

Comment 33

[Milan Sreckovic [:milan] (needinfo for best results)]

This was requested for "post 1.0", so I'm pretty sure we want to "-" tracking-b2g18.  Perhaps it's useful to have an explicit "-", rather than "---" just to be clear what the expectations are?  JP, if I misunderstood, please change it back.

Comment 34

[Justin Lebar (not reading bugmail)]

Okay, thanks.  Again per comment 32, I don't think this will have a large effect on B2G unless we get more powerful hardware, so I'd minus even for k9o.  But maybe Joe or Jeff knows something I don't.

Comment 35

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b682f3b118b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecf95d9819f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcb93a716be
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecf6d05c4e2

Comment 36

[Joe Drew (not getting mail)]

Attachment: part 5: stop doing .setFrameFoo directly from decoders

We shouldn't be touching RasterImage directly from decoders. This brings us towards that goal by moving almost all the direct mImage touching to the base class, Decoder, with the end desire of being able to set these attributes on the imgStatusTracker instead of on the Image.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 693592 [details] [diff] [review]
part 5: stop doing .setFrameFoo directly from decoders

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

::: image/decoders/nsGIFDecoder2.cpp
@@ +208,3 @@
>    // Tell the superclass we're starting a frame
> +  PostFrameStart(RasterImage::FrameDisposalMethod(mGIFStruct.disposal_method),
> +                 mGIFStruct.delay_time);

Are you sure that delay_time is correct at this point? Fortunately, moving these to PostFrameStop will prevent the worry.

::: image/decoders/nsPNGDecoder.cpp
@@ +129,5 @@
>  #ifdef PNG_APNG_SUPPORTED
> +// get the timeout and frame disposal method for the current frame
> +void nsPNGDecoder::GetAnimFrameInfo(RasterImage::FrameDisposalMethod& aDispose,
> +                                    RasterImage::FrameBlendMethod& aBlend,
> +                                    int32_t aTimeout)

Use std::tuple or struct and make this a static member. That will avoid the missing reference mistake you've made here.

::: image/decoders/nsPNGDecoder.h
@@ +117,5 @@
> +#ifdef PNG_APNG_SUPPORTED
> +  // Get the frame info if we're animated.
> +  void GetAnimFrameInfo(RasterImage::FrameDisposalMethod& aDispose,
> +                        RasterImage::FrameBlendMethod& aBlend,
> +                        int32_t aTimeout);

ReadAnimFrameInfo

::: image/src/Decoder.h
@@ +156,5 @@
> +  // this frame.
> +  void PostFrameStart(RasterImage::FrameDisposalMethod aDisposalMethod = RasterImage::kDisposeKeep,
> +                      int32_t aTimeout = 0,
> +                      RasterImage::FrameBlendMethod aBlendMethod = RasterImage::kBlendOver);
> +

I think it would be better for these parameters to go to PostFrameStop(). I don't see us needing them any sooner. That way we can limit mImage modifications to PostFrameStop()

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b682f3b118b1
https://hg.mozilla.org/mozilla-central/rev/ecf95d9819f0
https://hg.mozilla.org/mozilla-central/rev/0bcb93a716be
https://hg.mozilla.org/mozilla-central/rev/3ecf6d05c4e2

Comment 39

[Joe Drew (not getting mail)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #37)
> Comment on attachment 693592 [details] [diff] [review]
> part 5: stop doing .setFrameFoo directly from decoders
> 
> Review of attachment 693592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/decoders/nsGIFDecoder2.cpp
> @@ +208,3 @@
> >    // Tell the superclass we're starting a frame
> > +  PostFrameStart(RasterImage::FrameDisposalMethod(mGIFStruct.disposal_method),
> > +                 mGIFStruct.delay_time);
> 
> Are you sure that delay_time is correct at this point?

Yes.

Comment 40

[Joe Drew (not getting mail)]

Update as to schedule: 

I've had to do a bunch of work I didn't expect (parts 1-5, plus an upcoming part 6) because of individual frame attributes (animated images). This wasn't included in what I was talking about in comment 17.

* Make the imgStatusTracker also track animated frame attributes and have arbitrary properties that get set on the image (1.5 day)
* Pass "standalone" otherwise-unconnected imgStatusTrackers to decoders so they set their data on that status tracker (2 days)
** Also requires that we support such imgStatusTrackers, and can "replay" the difference between two imgStatusTrackers to an observer
* Make decoders stop after decoding a full frame (2 days)
* Instead of requesting frames from an image, *send* decoders frames, and have them pass back those frames when they're done (2 days)

-- At this point we should be threadsafe --

* Audit for thread safety (1 day)
* Hook up thread pool, start dispatching to threads (2 days)

Comment 41

[Justin Lebar (not reading bugmail)]

Joe, if you agree that this won't have a meaningful effect on b2g, can we remove the b2g-gfx whiteboard tag?

Comment 42

[Joe Drew (not getting mail)]

Attachment: part 5: stop doing .setFrameFoo directly from decoders v2

I believe this implements all requested review changes.

Comment 43

[Joe Drew (not getting mail)]

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

Comment 44

[Joe Drew (not getting mail)]

Attachment: part 6: track animated frame attributes in imgStatusTracker

This patch adds tracking of animated frame attributes to imgStatusTracker (which currently does nothing with them). It also moves the frame attribute enumerations to a place where everyone can use them, imgDecoderObserver.h, from their previous location in RasterImage.

Comment 45

[Joe Drew (not getting mail)]

Attachment: part 7: track image properties in imgStatusTracker

This removes the last of the raw users of mImage.set in decoders. It also adds the ability for imgStatusTracker to track the arbitrary properties that we expose as part of the API. Eventually, instead of setting these properties from the Decoder base class, we'll set them from the imgStatusTracker that we pass back from the decoder after it's done its work.

For now, though, imgStatusTracker properties do nothing.

Comment 46

[Joe Drew (not getting mail)]

Attachment: part 8: remove explicit mBlockingOnload

Kyle, you added this to imgRequest explicitly in bug 697230, and it made its way to imgStatusTracker in bug 505385. I don't think there's any reason for it to exist separately from the state in imgStatusTracker - do you think the same way?

Are there any tests for this in content or layout?

Comment 47

[Joe Drew (not getting mail)]

On the presumption that there are tests for this, a try: https://tbpl.mozilla.org/?tree=Try&rev=fb69257e96e7

Comment 48

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Justin Lebar [:jlebar] from comment #41)
> Joe, if you agree that this won't have a meaningful effect on b2g, can we
> remove the b2g-gfx whiteboard tag?

It does have an effect on B2G, but not in basecamp time frame; it's requested for 1+.  Also, I didn't realize that anybody other than me and JP were looking at that whiteboard tag.  It doesn't necessarily mean what it appears to mean :)

Comment 49

[Joe Drew (not getting mail)]

And another push, this time much more likely to build: https://tbpl.mozilla.org/?tree=Try&rev=7ea8d761e391

Comment 50

[Justin Lebar (not reading bugmail)]

Do you intend to change how we do sync decodes in this bug?  That is, after this bug, will we still do a few ms of main-thread sync decoding under some circumstances?

I've run into situations where that hurts (e.g. [1], which scrolls jankily the first time I load it), and I imagine such situations are more common on mobile.

Anyway, I don't mean to increase the scope of your bug, just curious to know what you're doing.

[1] http://en.wikipedia.org/wiki/Emoji

Comment 51

[Joe Drew (not getting mail)]

I do not intend to change our sync decode strategy. It'll need some fun to implement it, but it should be the same before and after.

Comment 52

[Joe Drew (not getting mail)]

Attachment: Part 9: Remove imgDecoderObserver::OnDataAvailable

We have imgDecoderObserver::FrameUpdated and imgDecoderObserver::OnDataAvailable, and the latter is pretty much useless given that it does *exactly* what the former does, so let's remove it.

Comment 53

[Joe Drew (not getting mail)]

Attachment: Part 10: Record invalidations to the latest frame

imgStatusTracker needs to keep track of the invalidations to the latest frame. This does simply that and nothing else.

Comment 54

[Joe Drew (not getting mail)]

Attachment: Part 11: Explicitly record whether an image is animated

This is a simple patch to explicitly record whether an image is currently animated. It also, incidentally, moves two of the functions for nicer reading of the code.

Comment 55

[Joe Drew (not getting mail)]

Attachment: Part 12: Make a not-notifying implementation of imgDecoderObserver

This is the addition of an imgDecoderObserver that doesn't notify the imgStatusTracker when it's notified by the decoder. This new class is called imgStatusTrackerObserver, and the old version renamed imgStatusTrackerNotifyingObserver, because the new one is going to be the only one used in the future. (For now, it's unused.)

Comment 56

[Joe Drew (not getting mail)]

Attachment: Part 14: Notify a particular state

We need to be able to notify a particular piece of state. This patch factors that out of SyncNotify into a separate function.

Comment 57

[Joe Drew (not getting mail)]

Comment on attachment 697691 [details] [diff] [review]
Part 14: Notify a particular state

(Uploaded the wrong patch)

Comment 58

[Joe Drew (not getting mail)]

Attachment: Part 13: Notify a particular state

Comment 59

[Joe Drew (not getting mail)]

Attachment: Part 14: Calculate the difference between two imgStatusTrackers, notify for that difference, and apply it to the source

This patch adds two semi-unrelated functions, currently unused:
 - SyncAndSyncNotifyDifference, which synchronizes the current imgStatusTracker to have the state of the argument imgStatusTracker, and notifies our observers about the new state;
 - CloneForRecording, which will let us create a new imgStatusTracker based on the current state for use with SyncAndSyncNotifyDifference.

Comment 60

[Joe Drew (not getting mail)]

Attachment: Part 15: Add SetObserver method to Decoder instead of initializing it in the constructor

In order to have "throw-away" imgStatusTrackers that we'll use to track an individual decode chunk's status, we need to be able to change the observer on a decoder. This patch makes that possible.

Comment 61

[Joe Drew (not getting mail)]

Attachment: Part 9.5: Add imgDecoderObserver::OnStartFrame

In order to know when precisely to invalidate an image, we need to know when a new frame has begun. Thus this patch, on which I'll rebuild Part 10.

Comment 62

[Joe Drew (not getting mail)]

Attachment: Part 10 v2: Record invalidations to the latest frame

Comment 63

[Joe Drew (not getting mail)]

Attachment: Part 12 v2: Make a not-notifying implementation of imgDecoderObserver

Rebased to include part 9.5 and part 10 v2.

Comment 64

[Joe Drew (not getting mail)]

Attachment: part 16: Make a new imgStatusTracker per decode event, and replay its changes to observers after decoders are done

This is the first patch that starts using all the foundations established in the previous 15 to make multithreaded decoding possible. Every time we dispatch a chunk of decoding work (still on the main thread), we clone a new imgStatusTracker from the image's current status. After decoding, we then replay that status back to the image.

This is also the first patch that changes how we decode in mozilla-central, and starts to get us harder to back out. So only asking for feedback for now.

Going through try:  https://tbpl.mozilla.org/?tree=Try&rev=fcee9a59e241

Comment 65

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 693996 [details] [diff] [review]
part 5: stop doing .setFrameFoo directly from decoders v2

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

::: image/decoders/nsPNGDecoder.cpp
@@ +79,5 @@
> +  delay_den = png_get_next_frame_delay_den(aPNG, aInfo);
> +  dispose_op = png_get_next_frame_dispose_op(aPNG, aInfo);
> +  blend_op = png_get_next_frame_blend_op(aPNG, aInfo);
> +
> +  AnimFrameInfo animinfo;

animInfo please

@@ +184,5 @@
> +    alpha = RasterImage::kFrameOpaque;
> +  else
> +    alpha = RasterImage::kFrameHasAlpha;
> +
> +  AnimFrameInfo animinfo;

animInfo

Comment 66

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 694015 [details] [diff] [review]
part 6: track animated frame attributes in imgStatusTracker

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

::: image/src/VectorImage.cpp
@@ +718,5 @@
>  
>      observer->FrameChanged(&nsIntRect::GetMaxSizedIntRect());
> +    observer->OnStopFrame(mozilla::image::kFrameHasAlpha,
> +                          mozilla::image::kDisposeNotSpecified, 0,
> +                          mozilla::image::kBlendOver);

I would be nice to have a comment about how it is unfortunate to have VectorImage use a decoder observer when it is doing no decoding at all.

::: image/src/imgStatusTracker.h
@@ +198,5 @@
> +    mozilla::image::FrameDisposalMethod mDisposalMethod;
> +    int32_t mTimeout;
> +    mozilla::image::FrameBlendMethod mBlendMethod;
> +  };
> +  nsTArray<FrameInfo> mFrameInfo;

I don't like recording an array of FrameInfo here. This should just be associated with the Frame and recorded there. Instead this duplicates stuff we have written down elsewhere.

Comment 67

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

Suggestion: s/mozilla::image:://

Seems like all of this stuff is in the mozilla::image namespace anyway.

Comment 68

[Joe Drew (not getting mail)]

Attachment: part 5: stop doing .setFrameFoo directly from decoders v3

Comment 69

[Joe Drew (not getting mail)]

Attachment: part 6: track image metadata in a separate object, and sync it to the image once decoding is done

Comment 70

[Joe Drew (not getting mail)]

Comment on attachment 694050 [details] [diff] [review]
part 7: track image properties in imgStatusTracker

Now that properties are tracked in the ImageMetadata object, there is no part 7.

Comment 71

[Joe Drew (not getting mail)]

Attachment: part 8: remove explicit mBlockingOnload

Rebased, carrying forward r+

Comment 72

[Joe Drew (not getting mail)]

Attachment: Part 9: Remove imgDecoderObserver::OnDataAvailable

Comment 73

[Joe Drew (not getting mail)]

Attachment: Part 9.5: Add imgDecoderObserver::OnStartFrame

Comment 74

[Joe Drew (not getting mail)]

Attachment: Part 10: Record invalidations to the latest frame

Comment 75

[Joe Drew (not getting mail)]

Attachment: Part 11: Explicitly record whether an image is animated

Comment 76

[Joe Drew (not getting mail)]

Attachment: Part 12: Make a not-notifying implementation of imgDecoderObserver

Comment 77

[Joe Drew (not getting mail)]

Attachment: Part 13: Notify a particular state

Comment 78

[Joe Drew (not getting mail)]

Attachment: Part 14: Calculate the difference between two imgStatusTrackers, notify for that difference, and apply it to the source

Comment 79

[Joe Drew (not getting mail)]

Attachment: Part 15: Add SetObserver method to Decoder instead of initializing it in the constructor

Comment 80

[Joe Drew (not getting mail)]

Attachment: Part 16: Heap-allocate DecodeRequests so we know when we're still decoding an image.

Comment 81

[Joe Drew (not getting mail)]

Attachment: Part 17: Make a new imgStatusTracker per decode event, and replay its changes to observers after decoders are done

Comment 82

[Joe Drew (not getting mail)]

I spent the whole week digging out from brokenness caused by refactorings, only finally getting it right today during the afternoon. The current status is that everything seems to work OK, other than perhaps sending some duplicate UnblockOnload notifications.

Next: publish frames to decoders and have the decoder base class set the frame-specific attributes directly on those frames.

Comment 83

[Joe Drew (not getting mail)]

Attachment: Part 17: Make a new imgStatusTracker per decode event, and replay its changes to observers after decoders are done

This version of the patch works correctly for animated images, and also for images that decode in a single write call to the decoder (i.e., small images).

Comment 84

[Joe Drew (not getting mail)]

Attachment: Part 18: Move mImageData and similar members to the base Decoder class

This patch moves mImageData and similar member variables, previously of leaf classes, into their base Decoder class.

Comment 85

[Joe Drew (not getting mail)]

Attachment: Part 19: Make animated image formats ask for new frames

This patch makes animated image formats pause their decoding and then ask their base class for a new frame at the appropriate time. It doesn't yet deal with the common case, which is single-frame images asking for a frame of a particular size; that will come next week.

Comment 86

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

Comment on attachment 704106 [details] [diff] [review]
part 5: stop doing .setFrameFoo directly from decoders v3

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

::: image/decoders/nsPNGDecoder.cpp
@@ +69,5 @@
> +
> +#ifdef PNG_APNG_SUPPORTED
> +// get the timeout and frame disposal method for the current frame
> +static AnimFrameInfo
> +GetAnimFrameInfo(png_structp aPNG, png_infop aInfo)

Since this is a stateless function that constructs an object and always succeeds, this looks like a constructor for AnimFrameInfo to me. (Alternatively, one could get rid of AnimFramInfo totally and use std::tuple/std::tie, but that sort of thing doesn't seem to be accepted Mozilla style.)

@@ +193,5 @@
>  
>    // We can't use mPNG->num_frames_read as it may be one ahead.
>    if (numFrames > 1) {
> +    PostInvalidation(mFrameRect);
> +  }

Local style in this file seems to be to use no braces for single line if's.

@@ +198,2 @@
>  
> +  if (png_get_valid(mPNG, mInfo, PNG_INFO_acTL)) {

Same brace issue here.

::: image/src/Decoder.h
@@ +131,5 @@
>  
>    // Use HistogramCount as an invalid Histogram ID
>    virtual Telemetry::ID SpeedHistogram() { return Telemetry::HistogramCount; }
>  
> +  enum FrameAlpha {

This enumeration appears both here and in RasterImage.h. The type in RasterImage seems to be the one that's actually used, so let's get rid of this one.

@@ +159,4 @@
>    // notifications, and does internal book-keeping.
>    void PostFrameStart();
> +
> +  // Called by decoders when they begin a frame. Informs the image, sends

Should be "when they end a frame".

Comment 87

[Joe Drew (not getting mail)]

In general I've been unconditionally using braces for single-line ifs, in an effort to make things sane little by little.

Comment 88

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

Comment on attachment 704112 [details] [diff] [review]
Part 9: Remove imgDecoderObserver::OnDataAvailable

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

Excellent.

Comment 89

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

Comment on attachment 704118 [details] [diff] [review]
Part 13: Notify a particular state

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

::: image/src/imgStatusTracker.cpp
@@ +413,5 @@
>    NS_DispatchToCurrentThread(ev);
>  }
>  
> +/* static */ void
> +imgStatusTracker::SyncNotifyState(imgRequestProxy* proxy, Image* image, uint32_t state, nsIntRect& dirtyRect, bool hadLastPart)

The only way the image argument here is used is to check whether it is null. It'd be better to just change the argument to a bool and cast it in the caller function. That ensures that we only give this function access to the minimum amount of state it needs to do its job.

@@ +475,4 @@
>    if (mImage) {
>      // OnFrameUpdate
>      // XXX - Should only send partial rects here, but that needs to
>      // wait until we fix up the observer interface

Having this comment here doesn't make sense anymore, as this is no longer where we send OnFrameUpdate.

@@ +475,5 @@
>    if (mImage) {
>      // OnFrameUpdate
>      // XXX - Should only send partial rects here, but that needs to
>      // wait until we fix up the observer interface
>      nsIntRect r;

This duplicate declaration of |r|, which shadows the outer one, shouldn't be here.

::: image/src/imgStatusTracker.h
@@ +185,5 @@
>    friend class imgStatusTrackerNotifyingObserver;
>  
>    void FireFailureNotification();
>  
> +  static void SyncNotifyState(imgRequestProxy* proxy,

There's no benefit to making this a private static function since it doesn't manipulate anything other than its arguments. Make it a standalone function and don't declare it in the header.

Comment 90

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

Comment on attachment 704119 [details] [diff] [review]
Part 14: Calculate the difference between two imgStatusTrackers, notify for that difference, and apply it to the source

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

Not a reviewer for this patch, but here are a couple of comments anyway. =)

::: image/src/imgStatusTracker.cpp
@@ +285,5 @@
> +    //  - mRequestRunnable, because it won't be nulled out when the
> +    //    mRequestRunnable's Run function eventually gets called.
> +    //  - mProperties, because we don't need it and it'd just point at the same
> +    //    object
> +    //  - mConsumers, because we don't need to talk to consumers

It's worth asking if the duplicated imgStatusTracker is really the same type as the original.

@@ +470,5 @@
>  void
> +imgStatusTracker::SyncAndSyncNotifyDifference(imgStatusTracker* other)
> +{
> +  uint32_t diffState = ~mState & other->mState;
> +  bool unblockedOnload = mState & stateBlockingOnload && !(other->mState & stateBlockingOnload);

It would be good for the duplicated imgStatusTracker to assert if it gets a BlockOnload notification unless its |mBlockingOnload| was already true when it got copy constructed, because there's no way for the duplicated imgStatusTracker to actually block onload.

Comment 91

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

Comment on attachment 704120 [details] [diff] [review]
Part 15: Add SetObserver method to Decoder instead of initializing it in the constructor

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

::: image/src/Decoder.cpp
@@ +42,2 @@
>    // No re-initializing
>    NS_ABORT_IF_FALSE(!mInitialized, "Can't re-initialize a decoder!");

Are there ever cases where it should be legal to have a null mObserver by the time Init() / InitSharedDecoder() is called? If not, we should assert about that in both functions.

::: image/src/Decoder.h
@@ +91,5 @@
>    }
>  
> +  void SetObserver(imgDecoderObserver* aObserver)
> +  {
> +    mObserver = aObserver;

Is it safe and expected to change the observer at any time? If not, there should be some assertions here to clarify when it's appropriate to call SetObserver. Also, is it OK if aObserver is null?

Comment 92

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

Lookin' good so far, Joe!

Comment 93

[Joe Drew (not getting mail)]

I have managed to fix my onload blocking problems, but I'm currently fighting a very strange crash while running mochitest-browser-chrome:

==9349== Invalid read of size 8
==9349==    at 0x6C0F9B4: nsAsyncDOMEvent::Run() (nsAsyncDOMEvent.cpp:63)
==9349==    by 0x73DB6BF: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
==9349==    by 0x73AFFAC: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:238)
==9349==    by 0x7217DC5: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:82)
==9349==    by 0x73FC856: MessageLoop::Run() (message_loop.cc:208)
==9349==    by 0x716ABC3: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
==9349==    by 0x70418F5: nsAppStartup::Run() (nsAppStartup.cpp:288)
==9349==    by 0x681B5B3: XREMain::XRE_mainRun() (nsAppRunner.cpp:3823)
==9349==    by 0x681B7D4: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:3890)
==9349==    by 0x681BA10: XRE_main (nsAppRunner.cpp:4093)
==9349==    by 0x402021: do_main(int, char**, nsIFile*) (nsBrowserApp.cpp:185)
==9349==    by 0x40213D: main (nsBrowserApp.cpp:377)
==9349==  Address 0x48cfd9f8 is 24 bytes inside a block of size 208 free'd
==9349==    at 0x402B5F9: free (vg_replace_malloc.c:446)
==9349==    by 0x6B9FD29: nsNodeUtils::LastRelease(nsINode*) (nsNodeUtils.cpp:258)
==9349==    by 0x6BC6E00: mozilla::dom::FragmentOrElement::Release() (FragmentOrElement.cpp:1685)
==9349==    by 0x6BC88CD: ContentUnbinder::UnbindSubtree(nsIContent*) (nsCOMPtr.h:449)
==9349==    by 0x6BC88AB: ContentUnbinder::UnbindSubtree(nsIContent*) (FragmentOrElement.cpp:1003)
==9349==    by 0x6BC93A6: ContentUnbinder::Run() (FragmentOrElement.cpp:1016)
==9349==    by 0x73DB6BF: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
==9349==    by 0x73AFFAC: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:238)
==9349==    by 0x7217DC5: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:82)
==9349==    by 0x73FC856: MessageLoop::Run() (message_loop.cc:208)
==9349==    by 0x716ABC3: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
==9349==    by 0x70418F5: nsAppStartup::Run() (nsAppStartup.cpp:288)
==9349==
==9349== Invalid read of size 8
==9349==    at 0x73AD29C: nsQueryInterface::operator()(nsID const&, void**) const (nsCOMPtr.cpp:14)
==9349==    by 0x73AD31A: nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) (nsCOMPtr.cpp:56)
==9349==    by 0x6B4ADC9: GetEventAndTarget(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, nsIDOMEvent**, nsIDOMEventTarget**) (nsCOMPtr.h:556)
==9349==    by 0x6B4DBEF: nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) (nsContentUtils.cpp:3528)
==9349==    by 0x6B4DC53: nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) (nsContentUtils.cpp:3503)
==9349==    by 0x6C0F9DD: nsAsyncDOMEvent::Run() (nsAsyncDOMEvent.cpp:41)
==9349==    by 0x73DB6BF: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
==9349==    by 0x73AFFAC: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:238)
==9349==    by 0x7217DC5: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:82)
==9349==    by 0x73FC856: MessageLoop::Run() (message_loop.cc:208)
==9349==    by 0x716ABC3: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
==9349==    by 0x70418F5: nsAppStartup::Run() (nsAppStartup.cpp:288)
==9349==  Address 0x48cfd9e0 is 0 bytes inside a block of size 208 free'd
==9349==    at 0x402B5F9: free (vg_replace_malloc.c:446)
==9349==    by 0x6B9FD29: nsNodeUtils::LastRelease(nsINode*) (nsNodeUtils.cpp:258)
==9349==    by 0x6BC6E00: mozilla::dom::FragmentOrElement::Release() (FragmentOrElement.cpp:1685)
==9349==    by 0x6BC88CD: ContentUnbinder::UnbindSubtree(nsIContent*) (nsCOMPtr.h:449)
==9349==    by 0x6BC88AB: ContentUnbinder::UnbindSubtree(nsIContent*) (FragmentOrElement.cpp:1003)
==9349==    by 0x6BC93A6: ContentUnbinder::Run() (FragmentOrElement.cpp:1016)
==9349==    by 0x73DB6BF: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
==9349==    by 0x73AFFAC: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:238)
==9349==    by 0x7217DC5: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:82)
==9349==    by 0x73FC856: MessageLoop::Run() (message_loop.cc:208)
==9349==    by 0x716ABC3: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
==9349==    by 0x70418F5: nsAppStartup::Run() (nsAppStartup.cpp:288)

The only thing I can think of is that maybe onload blockers could interact with the content unbinder. But I have no idea how or whether they interact, so I'm asking Kyle for a hand with that particular thing.

Comment 94

[Joe Drew (not getting mail)]

Attachment: Part 17: Make a new imgStatusTracker per decode event, and replay its changes to observers after decoders are done

Unfortunately, this patch is starting to become a dumping ground of correctness fixes.

Comment 95

[Olli Pettay [:smaug][[e-mail]]]

Something bad here
==9349==    at 0x402B5F9: free (vg_replace_malloc.c:446)
==9349==    by 0x6B9FD29: nsNodeUtils::LastRelease(nsINode*) (nsNodeUtils.cpp:258)

Are you missing AddRef somewhere?

Comment 96

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That is double plus ungood.  Can you figure out what the type of the node we're destroying in nsNodeUtils::LastRelease is?

Comment 97

[Joe Drew (not getting mail)]

Program received signal SIGTRAP, Trace/breakpoint trap.
Run (this=0x1a8dbc50) at /home/joe/src/mozilla-central/content/events/src/nsAsyncDOMEvent.cpp:35
35	    nsIDocument* doc = mEventNode->OwnerDoc();
(gdb) p *this
$2 = (nsLoadBlockingAsyncDOMEvent) {<nsAsyncDOMEvent> = {<nsRunnable> = {<nsIRunnable> = {<nsISupports> = {_vptr.nsISupports = 0x861a280 <vtable for nsLoadBlockingAsyncDOMEvent+16>}, <No data fields>}, 
      mRefCnt = {mValue = 1}}, mEventNode = {<nsCOMPtr_base> = {mRawPtr = 0x322cbd40}, <No data fields>}, mEvent = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}, 
    mEventType = {<nsAString_internal> = {mData = 0x322cff08, mLength = 4, mFlags = 5}, <No data fields>}, mBubbles = false, mDispatchChromeOnly = false}, mBlockedDoc = {<nsCOMPtr_base> = {
      mRawPtr = 0x26226730}, <No data fields>}}
(gdb) p mEventNode
$4 = {<nsCOMPtr_base> = {mRawPtr = 0x322cbd40}, <No data fields>}
(gdb) p mEventNode.mRawPtr
$5 = (mozilla::dom::EventTarget *) 0x322cbd40
(gdb) p *mEventNode.mRawPtr
$6 = (mozilla::dom::EventTarget) {<nsIDOMEventTarget> = {<nsISupports> = {_vptr.nsISupports = 0x85f94f0 <vtable for mozilla::dom::EventTarget+16>}, <No data fields>}, <nsWrapperCache> = {
    _vptr.nsWrapperCache = 0x85dc2a0 <vtable for nsWrapperCache+16>, mWrapperPtrBits = 2}, <No data fields>}

Comment 98

[Joe Drew (not getting mail)]

Attachment: Part 20: Always run size decodes before full decodes

This patch makes us always finish off a size decode before running full decodes. This will make it possible for us to preallocate image frames to send to decoders.

Comment 99

[Joe Drew (not getting mail)]

These patches seem to expose bug 835814, which is making testing trickier.

Comment 100

[Justin Lebar (not reading bugmail)]

Comment on attachment 704117 [details] [diff] [review]
Part 12: Make a not-notifying implementation of imgDecoderObserver

Sorry these reviews are going slowly; tef blockers blah blah.

Comment 101

[Joe Drew (not getting mail)]

Attachment: Part 19: Make animated image formats ask for new frames

This version fixes a few bugs in the previous version.

Comment 102

[Joe Drew (not getting mail)]

Attachment: Part 19: Make animated image formats ask for new frames

i am not good at copying files

Comment 103

[Joe Drew (not getting mail)]

Attachment: Part 17: Make a new imgStatusTracker per decode event, and replay its changes to observers after decoders are done

Now with fewer bugs!

Comment 104

[Joe Drew (not getting mail)]

Attachment: Part 19: Make animated image formats ask for new frames

There were some very, very difficult to debug bugs in the GIF decoder. giflib is looking mighty tempting about now.

Comment 105

[Joe Drew (not getting mail)]

Attachment: Part 20: Always run size decodes before full decodes for images we store source data for

Comment 106

[Joe Drew (not getting mail)]

Attachment: part 21: Even when we're not storing source data, run a size decode first

This patch extends size decodes also to images that we don't store source data for (chrome://, etc). We hold on to the source data until we get a size, then flush that source data to the decoder and throw it away.

Comment 107

[Joe Drew (not getting mail)]

Attachment: part 22: preallocate all frames before going into a decoder

Now that we always have the size information before going into a decoder, we can preallocate frames before doing the full decode, meaning that we never have to EnsureFrame() from a decoder. (If a decoder wants something other than ARGB32 for its first frame, it needs to ask for it from the decoder framework, then return to Write() - just as animated image formats need to do for all subsequent frames.)

Comment 108

[Joe Drew (not getting mail)]

Attachment: part 23: handle errors correctly

Some various and sundry error handling that makes us pass tests more effectively.

Comment 109

[Joe Drew (not getting mail)]

Speaking of tests, here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=2f981cc6138e

(Fingers crossed - they have been *very* orange before now.)

Comment 110

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

Comment on attachment 709344 [details] [diff] [review]
part 22: preallocate all frames before going into a decoder

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

Looks good! In addition to the comments before, in general I'd do a quick check for any other leftover calls to PostFrameStart and make sure that only Decoder.cpp is calling it.

::: image/decoders/nsBMPDecoder.cpp
@@ +142,5 @@
> +        if (mUseAlphaData) {
> +          PostFrameStop(RasterImage::kFrameHasAlpha);
> +        } else {
> +          PostFrameStop(RasterImage::kFrameOpaque);
> +        }

Is there a reason why the condition isn't |mUseAlphaData && mHaveAlphaData|?

Also, up to you, but the ternary operator is pretty readable and less verbose for this kind of condition.

@@ +328,3 @@
>              PostDecoderError(NS_ERROR_FAILURE);
>              return;
>          }

Why are we reporting this error here? Shouldn't this be handled by AllocateFrame and the top-level loop in |Decoder::Write|?

::: image/decoders/nsIconDecoder.cpp
@@ +77,5 @@
>            break;
>          }
>  
> +        if (!mImageData) {
> +          PostDecoderError(NS_ERROR_OUT_OF_MEMORY);

Again, not sure this check should be here.

@@ +82,5 @@
>            return;
>          }
>  
>          // Tell the superclass we're starting a frame
>          PostFrameStart();

I don't think we should still be calling PostFrameStart here, given that it is automatically called in |Decoder::Write| now.

::: image/decoders/nsJPEGDecoder.cpp
@@ +371,5 @@
>  
>      /* Used to set up image size so arrays can be allocated */
>      jpeg_calc_output_dimensions(&mInfo);
>  
> +    if (!mImageData) {

Again, not sure this check should be here. Do we really need to set |mState = JPEG_ERROR|? None of the other decoders bother with this sort of thing.

::: image/src/Decoder.cpp
@@ +93,2 @@
>    // Pass the data along to the implementation
> +  WriteInternal(aBuffer, aCount, rv);

This chunk of code is almost identical to the loop below, except that we don't call PostFrameStart here. But we are starting the first frame, aren't we? It's inconsistent that PostFrameStart isn't called every frame. Instead, set the initial state of mFrameCount so that calling PostFrameStart is OK, and then get rid of this chunk of code and just rely on the loop.

You may want to merge AllocateFrame with PostFrameStart, since once this change is made they will always be called as a pair.

::: image/src/Decoder.h
@@ +203,5 @@
>    void PostDecoderError(nsresult aFailCode);
>  
> +  // Try to allocate a frame as described in mNewFrameData and return the
> +  // status code from that attempt. Clears mNewFrameData.
> +  nsresult AllocateFrame();

Nobody actually calls AllocateFrame except Decoder itself, right? It's not part of the API to subclasses, so it should be private. But see above; maybe we don't need it anyway.

::: image/src/RasterImage.cpp
@@ +2595,5 @@
> +    // frame.  By default, we create an ARGB frame with no offset. If decoders
> +    // need a different type, they need to ask for it themselves.
> +    mDecoder->NeedNewFrame(0, 0, 0, mSize.width, mSize.height,
> +                           gfxASurface::ImageFormatARGB32);
> +  }

I'd consider making this comment shorter and instead using that vertical space to document the arguments to the call with inline comments. (You could document both of the offset arguments as a pair.)

Comment 111

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

Comment on attachment 709340 [details] [diff] [review]
Part 19: Make animated image formats ask for new frames

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

::: image/decoders/nsGIFDecoder2.cpp
@@ +571,5 @@
> +    // We've just gotten the frame we asked for. Time to use the data we
> +    // stashed away.
> +    len = mGIFStruct.bytes_in_hold;
> +    buf = p;
> +    q = buf;

Combine these two assignments into one line. (i.e. |q = buf = p|) That'll make the relationship between these variables more clear.

::: image/decoders/nsPNGDecoder.cpp
@@ +293,5 @@
> +    // We might not really know what caused the error, but it makes more
> +    // sense to blame the data.
> +    PostDataError();
> +
> +    png_destroy_read_struct(&mPNG, &mInfo, NULL);

Can we let the destructor take care of that?

::: image/src/Decoder.cpp
@@ +350,5 @@
> +  // Decoders should never call NeedNewFrame without yielding back to Write().
> +  MOZ_ASSERT(!mNewFrameData);
> +
> +  // We don't want images going back in time or skipping frames.
> +  MOZ_ASSERT(framenum == mFrameCount || framenum == (mFrameCount + 1));

Instead of this assertion, why not make it impossible to write wrong code? There could be two methods - |AddNewFrame| and |ReallocateCurrentFrame| or something along those lines - neither of which needs a |framenum| argument. Seems more self-documenting to me.

::: image/src/Decoder.h
@@ +145,5 @@
>     * only these methods.
>     */
>    virtual void InitInternal();
> +  virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> +                             nsresult aNewFrameResult = NS_OK) = 0;

As I've suggested in other review comments, ideally we'd deal with errors in allocating a new frame directly in |Decoder::Write|. If there aren't any cases we can't deal with, drop the |aNewFrameResult| parameter.

@@ +242,5 @@
> +    gfxASurface::gfxImageFormat mFormat;
> +    uint8_t mPaletteDepth;
> +  };
> +  nsAutoPtr<NewFrameData> mNewFrameData;
> +

The constructor for NewFrameData doesn't really add any value, especially since this type is private to this class and you initialize every member of the struct every time. Get rid of the constructor and initialize the struct using an initializer list. (In C++11 these have all the power of constructors anyway, if you ever need it.)

Also, given that decoders themselves are transient objects, I don't think the tradeoffs favor dynamically allocating memory for each update to mNewFrameData. Get rid of the nsAutoPtr and make mNewFrameData stored by value directly in the Decoder. You'll also need a boolean to indicate whether you've already dealt with the current mNewFrameData. I wouldn't bother clearing it; just update the boolean.

Comment 112

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

Comment on attachment 709345 [details] [diff] [review]
part 23: handle errors correctly

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

::: image/src/RasterImage.cpp
@@ +1838,5 @@
> +  if (mDecodeRequest) {
> +    mDecodeRequest->mStatusTracker->GetDecoderObserver()->OnStopRequest(lastPart, aStatus);
> +  } else {
> +    mStatusTracker->GetDecoderObserver()->OnStopRequest(lastPart, aStatus);
> +  }

Ideally OnStopRequest would only be called from one place. Can we avoid moving this stuff into RasterImage/VectorImage? For example, would it be wrong to make |RasterImage::GetStatusTracker| return mDecodeRequest->mStatusTracker if mDecodeRequest exists, and mStatusTracker otherwise?

Comment 113

[Justin Lebar (not reading bugmail)]

Joe, I'm really (really) sorry, but I don't think I'm going to have time to review this stuff for at least two weeks; I have some critical b2g stuff that landed on my plate that needs to get fixed a month ago.

:(

Comment 114

[Joe Drew (not getting mail)]

Attachment: part 16.5: add an OnDiscard helper to imgStatusTracker

Comment 115

[Joe Drew (not getting mail)]

Attachment: part 18.5: Count complete frames in Decoder::Finish, not just whatever imgFrame objects RasterImage has around

Comment 116

[Joe Drew (not getting mail)]

Attachment: Part 16: Heap-allocate DecodeRequests so we know when we're still decoding an image.

Comment 117

[Joe Drew (not getting mail)]

Attachment: Part 14: Calculate the difference between two imgStatusTrackers, notify for that difference, and apply it to the source

Comment 118

[Joe Drew (not getting mail)]

Attachment: Part 17: Make a new imgStatusTracker per decode event, and replay its changes to observers after decoders are done

This patch has a bunch of fixes that come from using some of this code for the first time, unfortunately.

Comment 119

[Joe Drew (not getting mail)]

Attachment: Part 20: Always run size decodes before full decodes for images we store source data for

Comment 120

[Joe Drew (not getting mail)]

Attachment: part 21: Even when we're not storing source data, run a size decode first

Comment 121

[Joe Drew (not getting mail)]

Attachment: part 23: handle errors correctly

I made some significant changes to this patch to fix some other error cases, so I'm asking for review again. Hope that's ok! :)

Comment 122

[Joe Drew (not getting mail)]

Seth - I swear I'm not ignoring your review comments and questions; I've just been sick the last couple of days, and otherwise head-down trying to green up test results. I'll get back to make this stuff landable once it's correct. :)

Comment 123

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

No worries Joe! Sorry for the slow reviews; I took some PTO last week.

Comment 124

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

Comment on attachment 711521 [details] [diff] [review]
part 16.5: add an OnDiscard helper to imgStatusTracker

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

Looks good.

Comment 125

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

Comment on attachment 711529 [details] [diff] [review]
Part 16: Heap-allocate DecodeRequests so we know when we're still decoding an image.

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

At a meta level, are we switching to heap allocating DecodeRequests just so we can use whether mDecodeRequest is null as a boolean value? If so, I'd suggest considering whether a boolean or Maybe<T> might not do the job just as well.

::: image/src/RasterImage.cpp
@@ +3320,5 @@
>  RasterImage::DecodeWorker::MarkAsASAP(RasterImage* aImg)
>  {
> +  // We can be marked as ASAP before we've been asked to decode. If we are,
> +  // create the request so we have somewhere to write down our status.
> +  if (aImg->mDecodeRequest) {

Should be |if (!aImg->mDecodeRequest)|, right?

@@ +3369,5 @@
>    }
>  }
>  
>  void
> +RasterImage::DecodeWorker::CreateRequestForImage(RasterImage* aImg)

I'd suggest calling this |EnsureDecodeRequest| or something similar, and fold the |if (!aImg->mDecodeRequest)| test into the method, since we always perform that test before calling it anyway.

@@ +3415,5 @@
>  {
> +  // If we haven't got a decode request, we're not currently decoding. (Having
> +  // a decode request doesn't imply we *are* decoding, though.)
> +  if (aImg->mDecodeRequest) {
> +    if (aImg->mDecodeRequest->isInList()) {

Can we ensure the invariant that when |aImg->mDecodeRequest| is true, |aImg->mDecodeRequest->isInList()| is true?

Comment 126

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

Comment on attachment 711531 [details] [diff] [review]
Part 14: Calculate the difference between two imgStatusTrackers, notify for that difference, and apply it to the source

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

::: image/src/imgStatusTracker.cpp
@@ +471,5 @@
> +imgStatusTracker::SyncAndSyncNotifyDifference(imgStatusTracker* other)
> +{
> +  uint32_t diffState = ~mState & other->mState;
> +  bool unblockedOnload = mState & stateBlockingOnload && !(other->mState & stateBlockingOnload);
> +  bool foundError = mImageStatus == imgIRequest::STATUS_ERROR;

Probably this should consult |other| in some way - seems like this might have been moved from after the synchronization to back here.

@@ +478,5 @@
> +  // with the other tracker.
> +
> +  // First, actually synchronize our state.
> +  mValidRect = mValidRect.Union(other->mValidRect);
> +  mState |= other->mState;

Is this safe? At least stateBlockingOnload can get set, but then later unset, so it's not safe to combine these values with the | operator. Probably stateBlockingOnload should not be part of mState at all since it doesn't have the same semantics as the other flags, unless you are sure that we will never block onload again once we unblock it, in which case we could add a second stateFinishedBlockingOnload flag or similar.

Comment 127

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

(In reply to Seth Fowler [:seth] from comment #125)
> Can we ensure the invariant that when |aImg->mDecodeRequest| is true,
> |aImg->mDecodeRequest->isInList()| is true?

To clarify, what I mean by this is that it would be nice if we _could_ ensure that invariant (which _seems_ like it should always be true, but I'm not sure) and turn the isInList() check into an assert or get rid of it.

Comment 128

[Joe Drew (not getting mail)]

I don't think we can (though I agree it'd be nice). In particular, we always create our DecodeRequest on initialization so we can store metadata in it, but we don't actually RequestDecode() until we've either been drawn or the end of the data has been received & we kick off the size decode.

Comment 129

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

Comment on attachment 711533 [details] [diff] [review]
Part 17: Make a new imgStatusTracker per decode event, and replay its changes to observers after decoders are done

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

This all looks good overall. Sorry for the deluge of comments; it's only because I care. =)

One thing that I think is worth thinking about in general here is whether we really want to clone the entire imgStatusTracker. I've had a hunch for a while that the cloned object should have a different type. I now suspect that, in addition, the cloned object should expose a narrower interface than the real imgStatusTracker. Clones only care about decoder notifications, right? I think things could be simpler and less errorprone if they only had that functionality to start with.

It might be that the cleanest way forward is to break imgStatusTracker into two parts, one of which contains the other. Another option is to leave the existing imgStatusTracker with roughly the same structure as what it has now and create a new type entirely. I'm not saying either of these approaches is necessarily the right thing, but I keep getting the vibe that it's possible this would be a cleaner route every time I review one of these patches.

::: image/src/Decoder.cpp
@@ +132,5 @@
>      }
>    }
> +
> +  // Set image metadata before calling DecodingComplete, because DecodingComplete calls Optimize().
> +  mImageMetadata.SetOnImage(&mImage);

This got moved from PostDecodeDone, but the calls to |mImageMetadata.SetLoopCount| and |mImageMetadata.SetIsNonPremultiplied| didn't get transferred, and are still there. That means we're setting different metadata here than we were there, and it potentially means that the last time PostDecodeDone gets called, the updates there never get transferred to the image with SetOnImage, unless I'm missing something.

::: image/src/RasterImage.cpp
@@ +2584,4 @@
>    mDecoder->SetSizeDecode(aDoSizeDecode);
>    mDecoder->SetDecodeFlags(mFrameDecodeFlags);
>    mDecoder->Init();
> +  if (!mDecodeRequest) {

Is this reachable? We just created an mDecodeRequest if it didn't exist on line 2580.

@@ +3261,5 @@
>      return;
>  
>    // If we're mid-decode, shut down the decoder.
> +  if (mDecoder) {
> +    FinishedSomeDecoding(this, eShutdownIntent_Error);

For consistency this should be |RasterImage::FinishedSomeDecoding|, or change the other calls.

@@ +3322,5 @@
>  }
>  #endif
>  
> +/* static */ void
> +RasterImage::FinishedSomeDecoding(RasterImage* aImage,

Though this is a static method, by passing in an instance it is sort of implicitly an instance method. Why not make it one?

@@ +3340,5 @@
> +  bool done = false;
> +
> +  if (image->mDecoder) {
> +    // If the decode finished, tell the image and shut down the decoder.
> +    if (image->IsDecodeFinished() || aIntent != eShutdownIntent_Done) {

This |if| statement took me quite a bit of thought to understand, because the state |!image->IsDecodeFinished() && aIntent == eShutdownIntent_Done| sounds like it should be inconsistent. AFAICT the actual deal here is that eShutdownIntent_Done is just being used to signal "no error so far" in the case where |image->IsDecodedFinished| returns false. There should be a comment about this. If you can find a different way to do it that's more clear without the comment, even better.

@@ +3370,5 @@
> +      }
> +    }
> +  }
> +
> +  // If it have any frames, tell the image what happened to the most recent

"If it have" -> "If it has"

@@ +3697,5 @@
> + : mImage(image)
> +{}
> +
> +void
> +RasterImage::DecodeDoneWorker::DidSomeDecoding(RasterImage* image)

The relationship between |DidSomeDecoding| and |FinishedSomeDecoding| isn't obvious from the name, and the names are a bit too similar. Rename |DidSomeDecoding|. |NotifyFinishedSomeDecoding|? Verbose, I know. =(

::: image/src/imgStatusTracker.cpp
@@ +471,5 @@
>  
>  void
>  imgStatusTracker::SyncAndSyncNotifyDifference(imgStatusTracker* other)
>  {
> +  // We must not modify or notify for the load state, which happens from Necko callbacks.

Maybe we should store that state in a separate flag variable? It'd be best to keep flags with the same semantics together and separate flags with different semantics, and it would simplify the code in SyncAndSyncNotifyDifference.

@@ +481,5 @@
>    // Now that we've calculated the difference in state, synchronize our state
>    // with the other tracker.
>  
>    // First, actually synchronize our state.
> +  mValidRect = other->mValidRect;

Is the reason we just unconditionally overwrite most of our local state with |other|'s because we don't expect these values to change locally? If so, let's assert that somewhere. (Here is probably not the right place.) (Also see my comment below related to noting when we're a cloned imgStatusTracker and using that for asserts.)

@@ +488,5 @@
>    mHadLastPart = other->mHadLastPart;
>  
> +  // The error state is sticky and overrides all other bits.
> +  if (mImageStatus == imgIRequest::STATUS_ERROR ||
> +      other->mImageStatus == imgIRequest::STATUS_ERROR) {

Consider doing the bitwise or operation unconditionally first so this check devolves to |if (mImageStatus & imgIRequest::STATUS_ERROR)|. I wish that bitwise or was sufficient on its own, but this status value is read by external code, and it doesn't seem worthwhile to try to update all of that code over this issue.

@@ -845,5 @@
>  
>  void
>  imgStatusTracker::RecordUnblockOnload()
>  {
> -  MOZ_ASSERT(mState & stateBlockingOnload);

If this was true before but isn't true anymore _only_ for the clones, I think we should assert that. For this and other reasons we should probably mark clones as being clones somewhere. Maybe |MOZ_ASSERT(mIsClone || mState & stateBlockingOnload)|?

::: image/src/imgStatusTracker.h
@@ +183,5 @@
>  
>    inline imgDecoderObserver* GetDecoderObserver() { return mTrackerObserver.get(); }
>  
>    imgStatusTracker* CloneForRecording();
> +  void SyncAndSyncNotifyDifference(imgStatusTracker* other);

It'd be great if this method had a name that didn't use "Sync" in two different ways. I can't think of a great alternative, though. If we could split it into two methods, one that synchronized and returned the difference, and one that did the notification, that would make things easier.

Comment 130

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

Comment on attachment 711535 [details] [diff] [review]
Part 20: Always run size decodes before full decodes for images we store source data for

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

::: image/src/RasterImage.cpp
@@ +2542,5 @@
>    NS_ABORT_IF_FALSE(!DiscardingActive(), "Discard Timer active in InitDecoder()!");
>  
> +  // If we're doing a two-pass decode, make sure we actually get size before
> +  // doing a full decode.
> +  if (StoringSourceData() && !aDoSizeDecode) {

Isn't it also true that if we're not storing source data, we should never do a size decode? Might be good to assert that as well.

Comment 131

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

Heh, actually that review comment must be wrong in light of the title of the next patch!

Comment 132

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

Comment on attachment 711539 [details] [diff] [review]
part 23: handle errors correctly

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

::: image/src/RasterImage.cpp
@@ +1816,5 @@
>  
>  nsresult
> +RasterImage::OnImageDataComplete(nsIRequest* aRequest, nsISupports* aCtx, nsresult aStatus)
> +{
> +  nsresult rv = OnImageDataCompleteCore(aRequest, aCtx, aStatus);

This looks familiar. =) Indeed, a lot of things in this patch look familar. Since bug 704059 includes similar changes and will land before this bug, might want to rebase this on top of those changes.

@@ +2585,5 @@
>  
>    // If we're doing a two-pass decode, make sure we actually get size before
>    // doing a full decode.
> +  if (!aDoSizeDecode && !mHasSize) {
> +    return NS_ERROR_FAILURE;

Is this an expected possibility rather than a coding error now?

::: image/src/imgDecoderObserver.h
@@ +118,5 @@
> +
> +  /**
> +   * Called when an image is realized to be in error state.
> +   */
> +  virtual void OnError() = 0;

Great!! This was needed.

::: image/src/imgStatusTracker.cpp
@@ +772,5 @@
>    mState &= ~stateDecodeStarted;
>    mState &= ~stateDecodeStopped;
>    mState &= ~stateRequestStopped;
>    mState &= ~stateBlockingOnload;
> +  mState &= ~stateImageIsAnimated;

I think this section would be considerably clearer if written as |mState &= stateHasSize & stateFrameStopped| or whichever flags you want to keep, and documented as to why those flags are kept. As the reader, I have to ask myself: is the preservation of those flags intentional? Same goes for mImageStatus above.

::: image/test/unit/async_load_tests.js
@@ -128,5 @@
>    return function channelLoadStop(imglistener, aRequest) {
> -    // We absolutely must not get imgIScriptedNotificationObserver::onStopRequest
> -    // after nsIRequestObserver::onStopRequest has fired. If we do that, we've
> -    // broken people's expectations by delaying events from a channel we were given.
> -    do_check_eq(streamlistener.requestStatus & STOP_REQUEST, 0);

Are you sure this is a problem? If I understand correctly, we intentionally do exactly this in bug 704059, because we have to spin the event loop after an SVG loads to allow the resources embedded in its document to load.

IMO when an image has logically finished loading (that is, when LOAD_COMPLETE has fired) shouldn't be required to map exactly to when Necko's OnStopRequest fires. (Imagine if we allow SVGs to load external resources from the same origin, which has been proposed before! That would _really_ break this.)

Comment 133

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #132)

> ::: image/test/unit/async_load_tests.js
> @@ -128,5 @@
> >    return function channelLoadStop(imglistener, aRequest) {
> > -    // We absolutely must not get imgIScriptedNotificationObserver::onStopRequest
> > -    // after nsIRequestObserver::onStopRequest has fired. If we do that, we've
> > -    // broken people's expectations by delaying events from a channel we were given.
> > -    do_check_eq(streamlistener.requestStatus & STOP_REQUEST, 0);
> 
> Are you sure this is a problem? 

Note the - preceding :)

Comment 134

[Milan Sreckovic [:milan] (needinfo for best results)]

This is, by the way "multi-threaded image decoding", rather than just "off main thread".  Not that OMT isn't a great thing in the first place, but we do expect to go for the thread-pool approach and decode more than one image at the time...

Comment 135

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Major content use case to stay responsive while images load.

Comment 136

[Joe Drew (not getting mail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #135)
> Major content use case to stay responsive while images load.

FWIW, we already decode images asynchronously whenever possible, so unless we are blocked on I/O or have more than one core, this isn't going to make us any more responsive.

Comment 137

[Randell Jesup [:jesup] (needinfo me)]

> FWIW, we already decode images asynchronously whenever possible, so unless we are blocked on I/O or have more than one core, this isn't going to make us any more responsive.

I'll note that multi-core is working it's way down to middle-of-the-road smartphones and almost all tablets already.  It's hard (impossible?) to find a desktop sold in the last few years with 1 core.

Comment 138

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #136)
> FWIW, we already decode images asynchronously whenever possible, so unless
> we are blocked on I/O or have more than one core, this isn't going to make
> us any more responsive.
async or separate thread? Since doing work in another thread lets main thread to process
user input etc so that does improve responsiveness. Async, yet main-thread, isn't nearly as good.

Comment 139

[Joe Drew (not getting mail)]

(In reply to Olli Pettay [:smaug] from comment #138)
> async or separate thread? Since doing work in another thread lets main
> thread to process
> user input etc so that does improve responsiveness. Async, yet main-thread,
> isn't nearly as good.

We do 5ms of decoding work on the main thread, then repost ourselves to do 5 more ms, etc.

Comment 140

[Randell Jesup [:jesup] (needinfo me)]

I should note that purposely blocking mainthread for 5ms at a time can negatively affect other things that for memory/locking/locking GC'd objects must bounce through mainthread while doing media processing or interacting with network protocols.  There are a bunch of things in webrtc that have to proxy through mainthread for these reasons, and even 5ms can impact snappiness.  I suspect also that other MediaStreamGraph and perhaps WebAudio uses might get impacted by this - not so much the main polling loops, but start/stop/control/etc.

That said, 5ms isn't horrible - but I'd prefer it off the mainthread if there's no major regression from doing so.  The more done off the mainthread, the better!!!!  Doubly or triply so on modern processors where you can get a boost to compensate for the cross-thread traffic.

Comment 141

[Justin Lebar (not reading bugmail)]

> FWIW, we already decode images asynchronously whenever possible

With the exception to "whenever possible" that we spend a few ms sync-decoding images when they first start decoding, bug 845147.  /That/ bug is a large demonstrated responsiveness win for scrolling (with bug 689623) and tab switching (with or without bug 689623) under certain circumstances.

Comment 142

[David Teller [:Yoric] - still alive but not very active]

Actually, given that promise the web that we can run at 60fps, recall that we only ever have at most 15ms to do everything we need to do between two |requestAnimationFrame| events. In this context, I have the intuition that 5ms is acceptable but still borderline.

Comment 143

[Joe Drew (not getting mail)]

It (probably) won't be necessary to decode synchronously at all for responsiveness once we have this multithreaded decoding, especially on multicore machines. That'll all come later once we start tuning, though.

Comment 144

[Joe Drew (not getting mail)]

It (probably) won't be necessary to decode synchronously at all for responsiveness once we have this multithreaded decoding, especially on multicore machines. That'll all come later once we start tuning, though.

Comment 145

[Joe Drew (not getting mail)]

Attachment: part 24: handle SVG images in the new world

Comment 146

[Joe Drew (not getting mail)]

Attachment: part 25: set metadata directly on frames

Instead of using mImage from Decoder, we need to know exactly what frame we're using so we can set metadata directly on it.

Comment 147

[Joe Drew (not getting mail)]

Attachment: part 26: set the size of images on ImageMetadata objects

We can't set the size directly on images from decoders; instead, use the handy ImageMetadata object and set it once the decoding process has passed it back.

Comment 148

[Joe Drew (not getting mail)]

Attachment: part 27: allocate frames asynchronously

Decoder currently allocates frames synchronously (though the decoder itself doesn't know that). We need to pass events back and forth, though, so the main thread is the only thing that allocates frames.

Comment 149

[Joe Drew (not getting mail)]

Attachment: part 28: multithread decoding using a thread pool

This is the patch that actually starts multiple threads. Unfortunately, due to us wanting to not have to run a separate event queue, it also removes the concept of "ASAP" decode requests. I think that'll be acceptable due to the fact that most of the time we're going to have multiple threads idle, so won't have any waiting for any decode. Time will tell, though.

Currently I set the number of threads to number of processors - 1, with a minimum of 1 thread; that might also want tuning.

Comment 150

[Joe Drew (not getting mail)]

Attachment: part 29: add prefs for multithreading

To aid debugging and tuning, and as a release valve, I've added prefs for whether we should multithread at all, and how many threads we should use if so (defaulting to the number of processors - 1 number I came up with out of thin air).

Comment 151

[Justin Lebar (not reading bugmail)]

> Unfortunately, due to us wanting to not have to run a separate event queue, it also removes the 
> concept of "ASAP" decode requests.

I'm going to hope this is OK because in the post-fixups-to-bug 689623 world, we shouldn't decode images that are not within or near the viewport.

Comment 152

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

Comment on attachment 722383 [details] [diff] [review]
part 25: set metadata directly on frames

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

Looks good.

::: image/src/Decoder.cpp
@@ +314,5 @@
>    }
>  
> +  mCurrentFrame->SetFrameDisposalMethod(aDisposalMethod);
> +  mCurrentFrame->SetTimeout(aTimeout);
> +  mCurrentFrame->SetBlendMethod(aBlendMethod);

Shouldn't you remove RasterImage::SetFrameHasNoAlpha and friends?

Comment 153

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

Comment on attachment 722381 [details] [diff] [review]
part 24: handle SVG images in the new world

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

Not my review but just FYI: this will look a bit different (in a good way!) after bug 847630, which has already landed.

Comment 154

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

Comment on attachment 722385 [details] [diff] [review]
part 26: set the size of images on ImageMetadata objects

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

::: image/src/ImageMetadata.h
@@ +46,5 @@
> +  void SetSize(int32_t width, int32_t height)
> +  {
> +    mWidth = width;
> +    mHeight = height;
> +    mHasSize = true;

It might be nicer to have SetSize take an nsIntSize (especially since RasterImage uses an nsIntSize internally). Even better would be to store this value in ImageMetadata as a Maybe<nsIntSize> and get rid of mHasSize.

::: image/src/RasterImage.cpp
@@ +3444,5 @@
>        Telemetry::Accumulate(Telemetry::IMAGE_DECODE_CHUNKS, request->mChunkCount);
>      }
>  
> +    if (!image->mHasSize && image->mDecoder->HasSize()) {
> +      image->mDecoder->SetSizeOnImage();

This seems rather roundabout. Why don't we just get the size from the decoder and call RasterImage::SetSize ourselves here?

Comment 155

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #153)
> Comment on attachment 722381 [details] [diff] [review]
> part 24: handle SVG images in the new world
> 
> Review of attachment 722381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not my review but just FYI: this will look a bit different (in a good way!)
> after bug 847630, which has already landed.

Yeah. "Saved conflicts in VectorImage.cpp.rej." :(

Comment 156

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #154)
> Even better would be to store
> this value in ImageMetadata as a Maybe<nsIntSize> and get rid of mHasSize.

I *continuously* forget that Maybe<> exists.

Comment 157

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

Comment on attachment 722393 [details] [diff] [review]
part 28: multithread decoding using a thread pool

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

I am excited that we've finally gotten to this point! I think this patch needs to get a bit more rigorous, though. (See my last comment.) I'd like to look at it again once that's addressed. It'd be easier for me to evaluate if I could see the entire file and not just the diff; is your patch queue for this public?

::: image/src/RasterImage.cpp
@@ +2648,5 @@
>      case eDecoderType_icon:
>        mDecoder = new nsIconDecoder(*this);
> +      // The icon decoder accesses OS APIs, which we can't rely on being
> +      // threadsafe.
> +      mDecoder->SetMainThreadOnly(true);

Shouldn't the decoder itself know if it's threadsafe?

@@ +3024,5 @@
>      nsresult rv = ShutdownDecoder(eShutdownIntent_Done);
>      CONTAINER_ENSURE_SUCCESS(rv);
> +
> +    // You can't call FinishedSomeDecoding with the decoding lock held.
> +    MutexAutoUnlock unlock(mDecodingMutex);

It's costly to lock and unlock so it would be highly preferable to make it so that FinishedSomeDecoding _requires_ the lock to be held if that's the common case, especially since most of the time it takes the lock itself. (see line 3516; I assume it's the common case that image->mDecoder is non-null)

@@ +3485,5 @@
>                                    DecodeRequest* aRequest /* = nullptr */)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  // We need to be able to lock and unlock this monitor.
> +  aImage->mDecodingMutex.AssertNotCurrentThreadOwns();

Remove this. It doesn't do anything anyway. In debug mode, AFAIK the deadlock detector should catch a double lock.

@@ +3595,5 @@
>  {
>    if (!sSingleton) {
>      sSingleton = new DecodeWorker();
>      ClearOnShutdown(&sSingleton);
>    }

Assert inside this |if| block that we're on the main thread.

@@ +3690,5 @@
>    } else {
> +    {
> +      // You can't call FinishedSomeDecoding with the decoding lock held.
> +      MutexAutoUnlock unlock(aImg->mDecodingMutex);
> +      RasterImage::FinishedSomeDecoding(aImg);

This is too much lock flip flopping. Just in this method we potentially do: lock(3673) -> unlock(3679) -> lock(~3679) -> unlock(3693) -> lock(~3693) -> unlock(~3673). And there are even more lock acquisitions inside methods like FinishedSomeDecoding, so it's even worse than this.

@@ +3722,3 @@
>  {
> +  RasterImage* image = mRequest->mImage;
> +  MutexAutoLock imglock(image->mDecodingMutex);

Am I missing something or does this mean that we're holding image->mDecodingMutex the entire time we're decoding? So if anything happens on the main thread that tries to grab that mutex (e.g. AddSourceData) we block the main thread until the decode job finishes?

@@ +3800,5 @@
>    if (aImg->mDecoder && aImg->mDecoder->NeedsNewFrame()) {
>      FrameNeededWorker::GetNewFrame(aImg);
>    } else {
> +    // You can't call FinishedSomeDecoding with the decoding lock held.
> +    MutexAutoUnlock unlock(aImg->mDecodingMutex);

Again, this is too much lock flip flopping. Just in this method we potentially do: lock(3782) -> unlock(3788) -> lock(~3788) -> unlock(3804) -> lock(~3804) -> unlock(~3782), not counting lock acquisitions inside the called methods.

@@ +3929,1 @@
>    nsCOMPtr<DecodeDoneWorker> worker = new DecodeDoneWorker(image, request);

nsRefPtr

::: image/src/RasterImage.h
@@ +447,2 @@
>     */
> +  class DecodeWorker : public nsIObserver

I _really_ think this class needs to be renamed. DecodeWorker is an administrative class that manages a thread pool and dispatches work to it. This is more like DecodePool than DecodeWorker. (Not sure it even belongs in the same source file as RasterImage at this point, really.)

@@ +493,3 @@
>  
>      enum DecodeType {
>        DECODE_TYPE_NORMAL,

Can we call this something else, like DECODE_TYPE_DEADLINE? "NORMAL" isn't really very descriptive.

@@ +745,5 @@
>  
>    // Source data members
>    FallibleTArray<char>       mSourceData;
>    nsCString                  mSourceDataMimeType;
> +  mozilla::Mutex             mDecodingMutex;

It's unclear exactly what mDecodingMutex protects. Specify exactly which data members are protected by mDecodingMutex and the invariants that are to be maintained when the lock isn't held, and document it in a comment here. That should make it a lot easier to ensure there aren't problems. This is the most important thing that this patch needs.

Comment 158

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

Comment on attachment 722397 [details] [diff] [review]
part 29: add prefs for multithreading

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

Looks good. r=me if gDecodingThreadLimit is made to do something. =)

::: image/src/RasterImage.cpp
@@ +3603,5 @@
> +  if (gMultithreadedDecoding) {
> +    mThreadPool = do_CreateInstance(NS_THREADPOOL_CONTRACTID);
> +    if (mThreadPool) {
> +      mThreadPool->SetName(NS_LITERAL_CSTRING("ImageDecoder"));
> +      mThreadPool->SetThreadLimit(std::max(PR_GetNumberOfProcessors() - 1, 1));

Should make use of gDecodingThreadLimit here.

@@ +3655,5 @@
>      }
>  
>      aImg->mDecodeRequest->mRequestStatus = DecodeRequest::REQUEST_PENDING;
>      nsRefPtr<DecodeJob> job = new DecodeJob(aImg->mDecodeRequest);
> +    if (!gMultithreadedDecoding || !mThreadPool || aImg->mDecoder->GetMainThreadOnly()) {

!gMultithreadedDecoding => !mThreadPool, right? Can probably leave this as it was.

Comment 159

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #158)
> @@ +3655,5 @@
> >      }
> >  
> >      aImg->mDecodeRequest->mRequestStatus = DecodeRequest::REQUEST_PENDING;
> >      nsRefPtr<DecodeJob> job = new DecodeJob(aImg->mDecodeRequest);
> > +    if (!gMultithreadedDecoding || !mThreadPool || aImg->mDecoder->GetMainThreadOnly()) {
> 
> !gMultithreadedDecoding => !mThreadPool, right? Can probably leave this as
> it was.

It can change live; I did this for flexibility.

Comment 160

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

Comment on attachment 722386 [details] [diff] [review]
part 27: allocate frames asynchronously

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

This all looks good, though I wish we could just allocate frames off the main thread. That'd simplify this whole thing quite a bit.

::: image/decoders/nsBMPDecoder.cpp
@@ +228,5 @@
>  
>      // GetNumFrames is called to ensure that if at this point mPos == mLOH but
>      // we have no data left to process, the next time WriteInternal is called
>      // we won't enter this condition again.
> +    if (mPos == mLOH && !HasSize()) {

This comment needs updating.

::: image/src/RasterImage.cpp
@@ +427,5 @@
> +    // we didn't call ShutdownDecoder and we need to do it manually.
> +    if (mFrames.Length() > 0) {
> +      imgFrame *curframe = mFrames.ElementAt(mFrames.Length() - 1);
> +      curframe->UnlockImageData();
> +    }

Is there no way we can just call ShutdownDecoder here? If none of the existing shutdown intents will work, how about adding a new one?

@@ +3358,1 @@
>      decodeFinished = true;

The new code comes right in the middle of these two connected comments. It'd be good to reword the comments.

@@ +3655,5 @@
> +    // this request to the back of the priority list.
> +    if (aImg->mDecoder &&
> +        !aImg->mError &&
> +        !aImg->IsDecodeFinished() &&
> +        aImg->mSourceData.Length() > aImg->mBytesDecoded) {

This fairly complex condition also occurs at line 3680 and line 3768. It should get a function of its own.

@@ +3713,5 @@
>  
>      uint32_t bytesDecoded = image->mBytesDecoded - oldByteCount;
>  
> +    // If the decoder needs a new frame, enqueue an event to get it; that event
> +    // will enqueue another decode request when it's done.

These repeated comments could be simplified or eliminated if FrameNeededWorker::GetNewFrame was called something more like FrameNeededWorker::GetNewFrameAndDecodeAgain.

::: image/src/RasterImage.h
@@ +418,5 @@
>      /* The number of chunks it took to decode this image. */
>      int32_t mChunkCount;
>  
> +    /* The status of the attempt to allocate a frame. */
> +    nsresult mStatus;

This should have a more specific name than mStatus, especially since in the next patch you add mRequestStatus.

@@ +420,5 @@
>  
> +    /* The status of the attempt to allocate a frame. */
> +    nsresult mStatus;
> +
> +    /* True if we are being called having just allocated a new frame. */

"we are being called" is a bit unclear. Maybe more like "True if a new frame has been allocated, but DecodeSomeData hasn't yet been called to flush data to it"?

Comment 161

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #89)
> There's no benefit to making this a private static function since it doesn't
> manipulate anything other than its arguments. Make it a standalone function
> and don't declare it in the header.

Problem is that imgStatusTracker is a friend to imgRequestProxy, since it's the only thing that should be calling the On* functions. So unfortunately, this needs to stay a private static function.

Comment 162

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #91)
> Comment on attachment 704120 [details] [diff] [review]
> Part 15: Add SetObserver method to Decoder instead of initializing it in the
> constructor
> ::: image/src/Decoder.h
> @@ +91,5 @@
> >    }
> >  
> > +  void SetObserver(imgDecoderObserver* aObserver)
> > +  {
> > +    mObserver = aObserver;
> 
> Is it safe and expected to change the observer at any time? If not, there
> should be some assertions here to clarify when it's appropriate to call
> SetObserver. Also, is it OK if aObserver is null?

No and no!

Comment 163

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #125)
> ::: image/src/RasterImage.cpp
> @@ +3320,5 @@
> >  RasterImage::DecodeWorker::MarkAsASAP(RasterImage* aImg)
> >  {
> > +  // We can be marked as ASAP before we've been asked to decode. If we are,
> > +  // create the request so we have somewhere to write down our status.
> > +  if (aImg->mDecodeRequest) {
> 
> Should be |if (!aImg->mDecodeRequest)|, right?

Yes!

> 
> @@ +3369,5 @@
> >    }
> >  }
> >  
> >  void
> > +RasterImage::DecodeWorker::CreateRequestForImage(RasterImage* aImg)
> 
> I'd suggest calling this |EnsureDecodeRequest| or something similar, and
> fold the |if (!aImg->mDecodeRequest)| test into the method, since we always
> perform that test before calling it anyway.

I ended up just removing the function altogether; it's unnecessary.

> @@ +3415,5 @@
> >  {
> > +  // If we haven't got a decode request, we're not currently decoding. (Having
> > +  // a decode request doesn't imply we *are* decoding, though.)
> > +  if (aImg->mDecodeRequest) {
> > +    if (aImg->mDecodeRequest->isInList()) {
> 
> Can we ensure the invariant that when |aImg->mDecodeRequest| is true,
> |aImg->mDecodeRequest->isInList()| is true?

The reverse holds, but that's it. And since I'm removing the lists altogether with multithreading, not much point in adding asserts.

Comment 164

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #129)
> One thing that I think is worth thinking about in general here is whether we
> really want to clone the entire imgStatusTracker. I've had a hunch for a
> while that the cloned object should have a different type. I now suspect
> that, in addition, the cloned object should expose a narrower interface than
> the real imgStatusTracker. Clones only care about decoder notifications,
> right? I think things could be simpler and less errorprone if they only had
> that functionality to start with.

So the problem with splitting this into two objects is that, fundamentally, imgStatusTrackers are supposed to be both status and recorder-of-status. And further, they can be used from different places in the same way - e.g., OnStartRequest. I'm OK with us exploring this in future work, though.

> > +/* static */ void
> > +RasterImage::FinishedSomeDecoding(RasterImage* aImage,
> 
> Though this is a static method, by passing in an instance it is sort of
> implicitly an instance method. Why not make it one?

Yeah, good point. Changed.

> @@ +481,5 @@
> >    // Now that we've calculated the difference in state, synchronize our state
> >    // with the other tracker.
> >  
> >    // First, actually synchronize our state.
> > +  mValidRect = other->mValidRect;
> 
> Is the reason we just unconditionally overwrite most of our local state with
> |other|'s because we don't expect these values to change locally? If so,
> let's assert that somewhere. 

Our local state can actually change, actually, which is why in future versions of this we tend to or in and/or union where appropriate.

> 
> @@ +488,5 @@
> >    mHadLastPart = other->mHadLastPart;
> >  
> > +  // The error state is sticky and overrides all other bits.
> > +  if (mImageStatus == imgIRequest::STATUS_ERROR ||
> > +      other->mImageStatus == imgIRequest::STATUS_ERROR) {
> 
> Consider doing the bitwise or operation unconditionally first so this check
> devolves to |if (mImageStatus & imgIRequest::STATUS_ERROR)|.

Nice.

> 
> @@ -845,5 @@
> >  
> >  void
> >  imgStatusTracker::RecordUnblockOnload()
> >  {
> > -  MOZ_ASSERT(mState & stateBlockingOnload);
> 
> If this was true before but isn't true anymore _only_ for the clones, I
> think we should assert that. For this and other reasons we should probably
> mark clones as being clones somewhere. Maybe |MOZ_ASSERT(mIsClone || mState
> & stateBlockingOnload)|?

The point of this change is actually to make it safe for Record[Un]BlockOnload to be called multiple times, which simplifies Observer code.

> ::: image/src/imgStatusTracker.h
> @@ +183,5 @@
> >  
> >    inline imgDecoderObserver* GetDecoderObserver() { return mTrackerObserver.get(); }
> >  
> >    imgStatusTracker* CloneForRecording();
> > +  void SyncAndSyncNotifyDifference(imgStatusTracker* other);
> 
> It'd be great if this method had a name that didn't use "Sync" in two
> different ways. I can't think of a great alternative, though. If we could
> split it into two methods, one that synchronized and returned the
> difference, and one that did the notification, that would make things easier.

I don't really see a way to do this easily due to the number of pieces of state. :(

Other changes were made and/or are obsolete in future patches.

Comment 165

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #111)
> ::: image/decoders/nsPNGDecoder.cpp
> > +    png_destroy_read_struct(&mPNG, &mInfo, NULL);
> 
> Can we let the destructor take care of that?

Yes, except that'll change the memory behaviour from what we have now, so perhaps we should do that later.

> ::: image/src/Decoder.cpp
> @@ +350,5 @@
> > +  // Decoders should never call NeedNewFrame without yielding back to Write().
> > +  MOZ_ASSERT(!mNewFrameData);
> > +
> > +  // We don't want images going back in time or skipping frames.
> > +  MOZ_ASSERT(framenum == mFrameCount || framenum == (mFrameCount + 1));
> 
> Instead of this assertion, why not make it impossible to write wrong code?
> There could be two methods - |AddNewFrame| and |ReallocateCurrentFrame| or
> something along those lines - neither of which needs a |framenum| argument.
> Seems more self-documenting to me.

We use EnsureFrame to be able to reuse memory buffers to limit flickering and memory churn in the multipart/x-mixed-replace case, so unfortunately this isn't possible in a clean way.

> 
> ::: image/src/Decoder.h
> @@ +145,5 @@
> >     * only these methods.
> >     */
> >    virtual void InitInternal();
> > +  virtual void WriteInternal(const char* aBuffer, uint32_t aCount,
> > +                             nsresult aNewFrameResult = NS_OK) = 0;
> 
> As I've suggested in other review comments, ideally we'd deal with errors in
> allocating a new frame directly in |Decoder::Write|. If there aren't any
> cases we can't deal with, drop the |aNewFrameResult| parameter.

Good change.


> 
> @@ +242,5 @@
> > +    gfxASurface::gfxImageFormat mFormat;
> > +    uint8_t mPaletteDepth;
> > +  };
> > +  nsAutoPtr<NewFrameData> mNewFrameData;
> > +
> 
> The constructor for NewFrameData doesn't really add any value, especially
> since this type is private to this class and you initialize every member of
> the struct every time. Get rid of the constructor and initialize the struct
> using an initializer list. (In C++11 these have all the power of
> constructors anyway, if you ever need it.)
> 
> Also, given that decoders themselves are transient objects, I don't think
> the tradeoffs favor dynamically allocating memory for each update to
> mNewFrameData. Get rid of the nsAutoPtr and make mNewFrameData stored by
> value directly in the Decoder. You'll also need a boolean to indicate
> whether you've already dealt with the current mNewFrameData. I wouldn't
> bother clearing it; just update the boolean.

Good idea; also done.

Comment 166

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #110)
> ::: image/decoders/nsBMPDecoder.cpp
> @@ +142,5 @@
> > +        if (mUseAlphaData) {
> > +          PostFrameStop(RasterImage::kFrameHasAlpha);
> > +        } else {
> > +          PostFrameStop(RasterImage::kFrameOpaque);
> > +        }
> 
> Is there a reason why the condition isn't |mUseAlphaData && mHaveAlphaData|?

Not particularly. Changed.
 
> Also, up to you, but the ternary operator is pretty readable and less
> verbose for this kind of condition.

Boo to the ternary operator forever!!!

> 
> @@ +328,3 @@
> >              PostDecoderError(NS_ERROR_FAILURE);
> >              return;
> >          }
> 
> Why are we reporting this error here? Shouldn't this be handled by
> AllocateFrame and the top-level loop in |Decoder::Write|?

I've made the AllocateFrame/top-level-loop change, but I'm going to leave this error checking in too, just for good measure.

> You may want to merge AllocateFrame with PostFrameStart, since once this
> change is made they will always be called as a pair.

I just called it from AllocateFrame.


> Nobody actually calls AllocateFrame except Decoder itself, right? It's not
> part of the API to subclasses, so it should be private. But see above; maybe
> we don't need it anyway.

FrameNeededWorker will.

Comment 167

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #152)
> Shouldn't you remove RasterImage::SetFrameHasNoAlpha and friends?

Yup

Comment 168

[Daniel Holbert [:dholbert]]

Comment on attachment 722381 [details] [diff] [review]
part 24: handle SVG images in the new world

Sorry for the delay on this! It slipped off my radar, but a bugzilla-auto-nag-email helpfully reminded me of it.

>   // If there's an error already, we need to fire OnStopRequest on our
>   // imgStatusTracker immediately. We might not get another chance.
>   if (mError || NS_FAILED(finalStatus)) {
>-    GetStatusTracker().OnStopRequest(aLastPart, finalStatus);
>+    GetStatusTracker().GetDecoderObserver()->OnStopRequest(aLastPart, finalStatus);
>+    GetStatusTracker().SyncNotifyDecodeState();
>     return finalStatus;

Why doesn't this one need a clone? Probably worth adding a comment to briefly explain that, since nearly every other usage of the status tracker in this patch involves first making a clone.  (Looks like the only other non-cloning spot is where we call OnStopFrame.)

>diff --git a/image/src/imgStatusTracker.cpp b/image/src/imgStatusTracker.cpp
> void
>+imgStatusTracker::OnStopFrame()
>+{
>+  RecordStopFrame();
>+
>+  /* notify the kids */
>+  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
>+  while (iter.HasMore()) {
>+    SendStopFrame(iter.GetNext());
>+  }

If this function is really *only* there to invoke onstopframe-callbacks in tests (per the comment quoted in the .h file, below), then do we really need that "RecordStopFrame()" call?  If we're serious about discouraging usage of this function, maybe we should just notify the kids and not do anything else.  Maybe RecordStopFrame is necessary for something else, though; fine if it is.

>--- a/image/src/imgStatusTracker.h
>+++ b/image/src/imgStatusTracker.h
>@@ -163,16 +163,19 @@ public:
>+  // This is called only by VectorImage, and only to ensure tests work
>+  // properly. Do not use it.
>+  void OnStopFrame();

Presumably you're talking about the chrome-privileged mochitests that set up explicit "OnStopFrame" listeners to watch for animation-triggered invalidations and take snapshots until it looks correct, right?

I thought we had tests like that for animated GIFs, too (added in jwir3's refresh-driver-timing-for-animated-gifs project) -- do those work without needing to invoke OnStopFrame on the status-tracker?

r=me, anyway -- ideally with comments added to explain why we don't need a clone in the spots where we don't clone.

Comment 169

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #160)
> @@ +3655,5 @@
> > +    // this request to the back of the priority list.
> > +    if (aImg->mDecoder &&
> > +        !aImg->mError &&
> > +        !aImg->IsDecodeFinished() &&
> > +        aImg->mSourceData.Length() > aImg->mBytesDecoded) {
> 
> This fairly complex condition also occurs at line 3680 and line 3768. It
> should get a function of its own.

Unfortunately the conditions are sufficiently different that refactoring them like this wasn't clearly better :(

> 
> ::: image/src/RasterImage.h
> > +    /* The status of the attempt to allocate a frame. */
> > +    nsresult mStatus;
> 
> This should have a more specific name than mStatus, especially since in the
> next patch you add mRequestStatus.

I just removed it. :)

Comment 170

[Joe Drew (not getting mail)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #166)
> (In reply to Seth Fowler [:seth] from comment #110)
> > ::: image/decoders/nsBMPDecoder.cpp
> > @@ +142,5 @@
> > > +        if (mUseAlphaData) {
> > > +          PostFrameStop(RasterImage::kFrameHasAlpha);
> > > +        } else {
> > > +          PostFrameStop(RasterImage::kFrameOpaque);
> > > +        }
> > 
> > Is there a reason why the condition isn't |mUseAlphaData && mHaveAlphaData|?
> 
> Not particularly. Changed.

Turns out that this is actually wrong — or at least, our tests depend on it. Reverted.

Comment 171

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #157)
> ::: image/src/RasterImage.cpp
> @@ +2648,5 @@
> >      case eDecoderType_icon:
> >        mDecoder = new nsIconDecoder(*this);
> > +      // The icon decoder accesses OS APIs, which we can't rely on being
> > +      // threadsafe.
> > +      mDecoder->SetMainThreadOnly(true);
> 
> Shouldn't the decoder itself know if it's threadsafe?

Really good point. I forgot about polymorphism I guess.

Anyways, it turns out that the actual nsIconDecoder is perfectly safe on background threads; it's the *stream* that needs to happen on the main thread. So I just threw this out.

> @@ +3024,5 @@
> >      nsresult rv = ShutdownDecoder(eShutdownIntent_Done);
> >      CONTAINER_ENSURE_SUCCESS(rv);
> > +
> > +    // You can't call FinishedSomeDecoding with the decoding lock held.
> > +    MutexAutoUnlock unlock(mDecodingMutex);
> 
> It's costly to lock and unlock so it would be highly preferable to make it
> so that FinishedSomeDecoding _requires_ the lock to be held if that's the
> common case, especially since most of the time it takes the lock itself.

Yeah, this makes sense. We only need to be unlocked to fire notifications and request decode, so I also split SyncAndSyncNotification into two functions, in direct contradiction to comment 164. (Named: CalculateAndApplyDifference and SyncNotifyDifference.) 

> (see line 3516; I assume it's the common case that image->mDecoder is
> non-null)
> 
> @@ +3485,5 @@
> >                                    DecodeRequest* aRequest /* = nullptr */)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +  // We need to be able to lock and unlock this monitor.
> > +  aImage->mDecodingMutex.AssertNotCurrentThreadOwns();
> 
> Remove this. It doesn't do anything anyway. In debug mode, AFAIK the
> deadlock detector should catch a double lock.

I was sad when I discovered it doesn't do anything. But I would have left it in here for documentation purposes, except that I made FinishedSomeDecoding require the lock instead of require it *not* be locked.

> @@ +3722,3 @@
> >  {
> > +  RasterImage* image = mRequest->mImage;
> > +  MutexAutoLock imglock(image->mDecodingMutex);
> 
> Am I missing something or does this mean that we're holding
> image->mDecodingMutex the entire time we're decoding? So if anything happens
> on the main thread that tries to grab that mutex (e.g. AddSourceData) we
> block the main thread until the decode job finishes?

Yes, unfortunately. There's no way around this that I can think of - AddSourceData can realloc and thus change the pointer that mSourceData points to, and so it needs to be locked while we're decoding.

(I did consider and reject the idea of copying out the memory that we're going to decode.)

Luckily it seems this only causes big problems in particularly unlucky cases. And the Networking team has semi-immediate plans to make it better by letting data be delivered to a different thread - bug 497003. Then we'll be able to block a background thread on adding new data, letting the main thread carry on its merry way.

> ::: image/src/RasterImage.h
> @@ +447,2 @@
> >     */
> > +  class DecodeWorker : public nsIObserver
> 
> I _really_ think this class needs to be renamed. DecodeWorker is an
> administrative class that manages a thread pool and dispatches work to it.
> This is more like DecodePool than DecodeWorker. (Not sure it even belongs in
> the same source file as RasterImage at this point, really.)

Sure, DecodePool it is.
 
> Can we call this something else, like DECODE_TYPE_DEADLINE? "NORMAL" isn't
> really very descriptive.

DECODE_TYPE_UNTIL_TIME?
 
> @@ +745,5 @@
> >  
> >    // Source data members
> >    FallibleTArray<char>       mSourceData;
> >    nsCString                  mSourceDataMimeType;
> > +  mozilla::Mutex             mDecodingMutex;
> 
> It's unclear exactly what mDecodingMutex protects. Specify exactly which
> data members are protected by mDecodingMutex and the invariants that are to
> be maintained when the lock isn't held, and document it in a comment here.
> That should make it a lot easier to ensure there aren't problems. This is
> the most important thing that this patch needs.

Will do.

Comment 172

[Joe Drew (not getting mail)]

Attachment: part 28: multithread decoding using a thread pool v2

Implemented changes as requested!

Comment 173

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2205fa1da0
https://hg.mozilla.org/integration/mozilla-inbound/rev/98bab71808e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/384efd041a37
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d8ebb29543
https://hg.mozilla.org/integration/mozilla-inbound/rev/94af4b2dd182
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6078996517b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbb93a483d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/781b89454236
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7db2c001c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5567f140abb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/096b05d298ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bad4198e33c
https://hg.mozilla.org/integration/mozilla-inbound/rev/761015312a0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0a6850c887
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef71ebfb90a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c86eb1ff8916
https://hg.mozilla.org/integration/mozilla-inbound/rev/682938749810
https://hg.mozilla.org/integration/mozilla-inbound/rev/16cd97be2846
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b96c5907a1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c280517b6ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf71e7e1efc
https://hg.mozilla.org/integration/mozilla-inbound/rev/e47cbcb88803
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0683dc77a1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2fc100f05f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f978369c50

Comment 174

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a2205fa1da0
https://hg.mozilla.org/mozilla-central/rev/98bab71808e5
https://hg.mozilla.org/mozilla-central/rev/384efd041a37
https://hg.mozilla.org/mozilla-central/rev/45d8ebb29543
https://hg.mozilla.org/mozilla-central/rev/94af4b2dd182
https://hg.mozilla.org/mozilla-central/rev/a6078996517b
https://hg.mozilla.org/mozilla-central/rev/5fbb93a483d6
https://hg.mozilla.org/mozilla-central/rev/781b89454236
https://hg.mozilla.org/mozilla-central/rev/8f7db2c001c9
https://hg.mozilla.org/mozilla-central/rev/5567f140abb3
https://hg.mozilla.org/mozilla-central/rev/096b05d298ad
https://hg.mozilla.org/mozilla-central/rev/4bad4198e33c
https://hg.mozilla.org/mozilla-central/rev/761015312a0b
https://hg.mozilla.org/mozilla-central/rev/7d0a6850c887
https://hg.mozilla.org/mozilla-central/rev/ef71ebfb90a0
https://hg.mozilla.org/mozilla-central/rev/c86eb1ff8916
https://hg.mozilla.org/mozilla-central/rev/682938749810
https://hg.mozilla.org/mozilla-central/rev/16cd97be2846
https://hg.mozilla.org/mozilla-central/rev/2b96c5907a1b
https://hg.mozilla.org/mozilla-central/rev/9c280517b6ea
https://hg.mozilla.org/mozilla-central/rev/3cf71e7e1efc
https://hg.mozilla.org/mozilla-central/rev/e47cbcb88803
https://hg.mozilla.org/mozilla-central/rev/e0683dc77a1b
https://hg.mozilla.org/mozilla-central/rev/0a2fc100f05f
https://hg.mozilla.org/mozilla-central/rev/d1f978369c50

Comment 175

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

(In reply to Joe Drew (:JOEDREW! \o/) from comment #171)
> Yes, unfortunately. There's no way around this that I can think of -
> AddSourceData can realloc and thus change the pointer that mSourceData
> points to, and so it needs to be locked while we're decoding.

That's reasonable. This is something that can be incrementally improved on, anyway, so it needn't hold up the bug. One item of potentially low-hanging fruit is to only grab the lock if the realloc is needed, though that would require some API reshuffling, so maybe it's not as low-hanging as it seems. =)

Comment 176

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

Comment on attachment 726960 [details] [diff] [review]
part 28: multithread decoding using a thread pool v2

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

Looks good!

::: image/src/RasterImage.cpp
@@ +3491,5 @@
> +    // If we have no request, we have not yet created a decoder, but we still
> +    // need to send out notifications.
> +    if (request) {
> +      image->mStatusTracker->SyncNotifyDifference(diff);
> +      request->mRequestStatus = DecodeRequest::REQUEST_INACTIVE;

This must have just been set on line 3483, right? (Admittedly prior to the lock release.) Setting mDecodeRequest->mRequestStatus without the lock held looks dangerous to me anyway.

::: image/src/RasterImage.h
@@ +486,3 @@
>  
>      enum DecodeType {
> +      DECODE_TYPE_UNTIL_TIME,

Nice name.

::: image/src/imgStatusTracker.h
@@ -192,5 @@
>  
>    inline imgDecoderObserver* GetDecoderObserver() { return mTrackerObserver.get(); }
>  
>    imgStatusTracker* CloneForRecording();
> -  void SyncAndSyncNotifyDifference(imgStatusTracker* other);

Glad to see that method go. =)

Comment 177

[Joe Drew (not getting mail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/275cd395f9fa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4b22851976

Comment 178

[Richard Smallman]

Ever since this update I've noticed sometimes images don't load completely. The issue has been happening to some of my other friends off a technology forum.
Sorry if this is  in the wrong spot. I'm new.

Comment 179

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

You're talking about bug #853337 which will be fixed in next Nightly update.

Comment 180

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/275cd395f9fa
https://hg.mozilla.org/mozilla-central/rev/9e4b22851976

Comment 181

[Trevor Saunders (:tbsaunde)]

backed out for busting linux tp
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab4a115f06c

Comment 182

[Phil Ringnalda (:philor)]

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

Comment 183

[Joe Drew (not getting mail)]

Repushed with fixes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2bdda8c3ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/1541b7a03e17

Comment 184

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/9e2bdda8c3ca
https://hg.mozilla.org/mozilla-central/rev/1541b7a03e17

Comment 185

[Willy_ Foo_Foo]

https://bugzilla.mozilla.org/show_bug.cgi?id=854803

Comment 186

[Virgil Dicu [:virgil] [QA]]

With these patches, should there be a noticeable improvement in image loading time in Firefox? Is there any way I can measure that in Aurora?
Are there any other areas which would welcome manual testing?

Comment 187

[Alex Keybl [:akeybl]]

This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email [e-mail] asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

Comment 188

[Jared Wein [:jaws] (please needinfo? me)]

See comment 186.

Comment 189

[Joe Drew (not getting mail)]

(In reply to Virgil Dicu [:virgil] [QA] from comment #186)
> With these patches, should there be a noticeable improvement in image
> loading time in Firefox? Is there any way I can measure that in Aurora?
> Are there any other areas which would welcome manual testing?

It depends entirely upon the testcase. Try one I cooked up: https://www.dropbox.com/sh/b4v1g8izmeajc0k/U53Hyab7FY

Note that you'll probably want to load it from Apache rather than the python web server that I suggest with run.sh - the Python web server is single-threaded, so it's not a realistic workload.

Comment 190

[Virgil Dicu [:virgil] [QA]]

(In reply to Joe Drew (:JOEDREW! \o/) (Unburying, use needinfo) from comment #189)
> (In reply to Virgil Dicu [:virgil] [QA] from comment #186)
> > With these patches, should there be a noticeable improvement in image
> > loading time in Firefox? Is there any way I can measure that in Aurora?
> > Are there any other areas which would welcome manual testing?
> 
> It depends entirely upon the testcase. Try one I cooked up:
> https://www.dropbox.com/sh/b4v1g8izmeajc0k/U53Hyab7FY
> 
> Note that you'll probably want to load it from Apache rather than the python
> web server that I suggest with run.sh - the Python web server is
> single-threaded, so it's not a realistic workload.

Thanks!

248.051 seconds - non - multithread
139.087 seconds - multithread

Used Apache on a Mac OS 10.8.3 - same conditions for both builds.

I'll set this to Verified considering that.