399925 - GIF decoder needs to allow its data to be discarded

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect, P2)

Description

[Stuart Parmenter]

Currently only PNGs and JPEGs set their data as discardable.  We should do this for GIFs as well.  We may want to wait until after we store them as 8bit though.

Comment 1

[Alfred Kayser]

Interesting part for this is the handling of animated GIF's (and animated PNG's), is that the frames may not be discarded when the image is animated. 

It seems that imgContainer needs to adjusted to handle animated images correctly.

Comment 2

[Alfred Kayser]

Also there is no way to indicate from a decoder back to the imgContainer that keeping the mRestoreData around is not needed/usefull as the image is animating, but possible the imgContainer needs itself to be more smart about keeping the mRestoreData. 

Furthermore, it seems that RestoreDiscardedData is trying to reset the discard timer, even if the timer was not set (because it checks for mDiscardable instead of the mDiscardDataDone) (as the timer is only set when mDiscardDataDone is true).

So, to get this discarding to work right for animated frames, we need first clean up and extend this discarding logic to be robust against animated images...

Comment 3

[Alfred Kayser]

Another problem: The decoders (that implement 'Discardable') use 'GetImage' to get the image related to the 'ContainerObserver'. This is either imgILoad (in normal case) or a ContainerLoader (implementing a fake imgILoad) (when the image is reloaded).
That is causing all kinds of confusion, because the decoder then reuses the image object even if is not 'reloading' the image. Possible multiple ways to fix: make imgRequest to not re-use the imageContainer, or make another 'Init' for the decoders that take not a 'ContainerObserver' but a 'Image' object directly (in this way the decoder also know whether it is an initial (incremental) load, or a reload (non-incremental).

See bug 403364.

Comment 4

[Stuart Parmenter]

all image decoders should be able to handle re-using an image container.  it is needed for multipart (this is nothing new -- just not fully implemented everywhere until now.)

Comment 5

[Alfred Kayser]

Indeed, but that is a different scenario: this happens when the same image object is re-used / re-filled with a new image in the multipart decoding, so then the imgRequest/imgLoad really is reused from the cache.
In that scenario, we should create a new image container as the new image in the multipart stream may even be of different size (and/or different number of frames and/or even of different type!). To keep the 'jpeg/multipart' animation trick, the imgRequest needs to be kept, but a new imageContainer can and should be used.

Reconstructing the image frames from the 'RestoreData' is something else, here we work on the same imageContainer, only just reconstructing the frames. For this we need to tell the decoder to only reconstruct the frames in the same imageContainer. Note the current Restore code allocates a separate 'ContainerLoader' to simulate the (re)loading of the image into the current imageContainer.

In summary:
multipart/x-mixed-replace really is about replacing the content of the same image object (could be of different size, type, animation).
Discardable/RestoreData is about rerunning the decoder on the same source data to reconstruct the image frame(s) (and per definition same size, same type, etc).

To keep thing easy (and working) we should look at these scenarios separately.

Comment 6

[Stuart Parmenter]

in many multipart cases it is much better to redraw over the old image data than it is to replace it entirely -- think streaming jpegs where you don't get the whole image at once.. you really want to draw over it.  we explicitly fail in the case of different sizes as real world examples don't do that and it isn't worth supporting.

Comment 7

[Alfred Kayser]

Attachment: V1: Allow GIF image frames to be discarded

This makes the gif image frames discardable.

Meanwhile I found some issues in reloading directly after restoring and some related issues. In total I have identified 20 new 'bugs' to be addressed. Will try to make bugreports for each one or set of them.
 
For this patch, I tried restrict myself to the most critical ones:
1. When Init() is called again (for restoring image frames from the decoder), it must better reset the Discard/Restore flags and reset mAnim and mFrames.
2. Make AddRestoreData and RestoreDataDone not return errors when called while image discarding has been disabled. 
3. Reset the mDiscarded flag before ReloadImages() so that when it is called again from RestoreDiscardedData it does not tries to call it again while it is busy.

Note, for testing purposes I have reduced the timeout from 45 seconds to 15 seconds, this can be reset to 45 when it is all tested thoroughly.

Comment 8

[Alfred Kayser]

Another fix (needed to get the gif restore working) was to make Flush in the gifDecoder return NS_OK instead of NS_ERROR_NOT_IMPLEMENTED.
Made the same change for nsIconDecoder to make the Flush behavior consistent throughout.

Comment 9

[Alfred Kayser]

Attachment: V2: Don't throw away old frames in Init, jpeg/multipart can re-use them (sometimes)

Comment 10

[tor]

Comment on attachment 297176 [details] [diff] [review]
V2: Don't throw away old frames in Init, jpeg/multipart can re-use them (sometimes)

This patch still has the 15s discard timer.  Other than that, sr=tor.

Comment 11

[Alfred Kayser]

Attachment: V3: without the 'imgContainer' part, because that was already checked in with bug 403364

The fixes for imgContainer that were required to get the discarding of animated GIF's working were checked with bug 403364 as it also fixed the APNG discarding issues.

Comment 12

[Alfred Kayser]

So, also the 15s discard timer is no longer in this patch.

Comment 13

[Reed Loden [:reed]]

Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.94; previous revision: 1.93
done
Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.h;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.h,v  <--  nsGIFDecoder2.h
new revision: 1.33; previous revision: 1.32
done
Checking in modules/libpr0n/decoders/icon/nsIconDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconDecoder.cpp,v  <--  nsIconDecoder.cpp
new revision: 1.29; previous revision: 1.28
done

Comment 14

[Reed Loden [:reed]]

Backed this patch out, as it caused Tp crashes on Linux bl-bldlnx03 Dep fx-linux-tbox perf test.

Comment 15

[Alfred Kayser]

When bug 418791 has been checked in, we could test this again.

Comment 16

[Reed Loden [:reed]]

Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.100; previous revision: 1.99
done
Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.h;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.h,v  <--  nsGIFDecoder2.h
new revision: 1.35; previous revision: 1.34
done

Comment 17

[Reed Loden [:reed]]

Nope, still crashes Tp. We need to get a stack trace. :(

Backed out again.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Please make sure to add whatever is crashing this to crashtests when this lands.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

--> mozilla1.9, reset if you think this needs to block beta 5.

Comment 20

[Joe Drew (not getting mail)]

Alfred, are you actively working on the crashes?

Comment 21

[Alfred Kayser]

It is difficult to reproduce the crash, as it only happens during the pageload performance test at the mozilla servers. I don't have that setup.
There are also no crashdumps, so I have no clue where precisely this is happening...

Comment 22

[Joe Drew (not getting mail)]

OK. If you don't mind, I'll take this from you then, because I'm a MoCo employee and have access to the pageload test suite.

Comment 23

[Alfred Kayser]

Please, go ahead!

Comment 24

[Joe Drew (not getting mail)]

Attachment: updated unload patch

This is an updated patch that fixes the crash. It's identical to attachment 298751 [details] [diff] [review] except that it moves the call to AddRestoreData() before the call to GifWrite(). This is necessary because if an entire gif fits in the buffer sent to ProcessData(), GifWrite() will call imgContainer::RestoreDataDone(), which makes it impossible to add any data to the restore data buffer, leaving us with absolutely no restore data and therefore no frames/surfaces.

This would also be a problem in general, since the last call to ProcessData() wouldn't get added to the restore buffer, leading to incomplete frames. I'm not sure why the previous patch didn't cause more problems than it did.

I'll have to look into writing a test for this -- we need a timer of greater than 15s to allow the gif to unload. I'm not sure if reftests can do this.

Comment 25

[Joe Drew (not getting mail)]

Attachment: mochitest for the bug earlier patches caused

This is a mochitest (along with a hand-crafted image that I will attach next) to ensure that unloaded gifs can be reloaded. Its main downside is that it adds a 20 second delay to mochitest runs.

This test passes with my updated unload patch, and fails with v3 of the patch.

Comment 26

[Joe Drew (not getting mail)]

Attachment: image required for test patch

Comment 27

[Boris Zbarsky [:bzbarsky]]

Is the timeout a pref?  If so, could the test twiddle this pref to a lower value and reduce the delay?

Comment 28

[Alfred Kayser]

That value is hardcoded in imgContainer.cpp. I could produce a patch to make this into an environment variable (like the enabling of the discarding itself), but a pref would be better of course... This requires a separate bug.

Comment 29

[Reed Loden [:reed]]

Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.102; previous revision: 1.101
done
Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.h;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.h,v  <--  nsGIFDecoder2.h
new revision: 1.37; previous revision: 1.36
done

Comment 30

[Boris Zbarsky [:bzbarsky]]

Any reason the test wasn't checked in?

Comment 31

[Reed Loden [:reed]]

(In reply to comment #30)
> Any reason the test wasn't checked in?

It's waiting for review from stuart...

Comment 32

[Reed Loden [:reed]]

Checking in modules/libpr0n/test/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/test/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/modules/libpr0n/test/mochitest/Makefile.in,v
done
Checking in modules/libpr0n/test/mochitest/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/test/mochitest/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libpr0n/test/mochitest/bug399925.gif,v
done
Checking in modules/libpr0n/test/mochitest/bug399925.gif;
/cvsroot/mozilla/modules/libpr0n/test/mochitest/bug399925.gif,v  <--  bug399925.gif
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libpr0n/test/mochitest/test_bug399925.html,v
done
Checking in modules/libpr0n/test/mochitest/test_bug399925.html;
/cvsroot/mozilla/modules/libpr0n/test/mochitest/test_bug399925.html,v  <--  test_bug399925.html
initial revision: 1.1
done

Comment 33

[Dave Townsend [:mossop]]

I have disabled mochitest in libpr0n for the time being because that testcase is crashing on Linux

Comment 34

[Reed Loden [:reed]]

I backed out the actual patch to see if it is the cause of bug 425941. If it turns out to not be the cause, I'll reland it.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> +    ok(true, "we got through the drawImage call without an exception being thrown");

There's not much point to that line, since an exception will immediately cause a test failure....

Is there a bug filed on that crash?  There's nothing in that test that a webpage couldn't do, so the crash is a pretty serious issue that perhaps needs to block the relanding of the patch.

Comment 36

[Joe Drew (not getting mail)]

I can't reproduce the patch on a totally up-to-date Linux build. I'll need a stack trace from someone who can reproduce it.

Comment 37

[Joe Drew (not getting mail)]

Ugh, I can't reproduce the *crash*.

Comment 38

[Damon Sicore (:damons)]

We wouldn't hold the release for this.  wanted1.9.0.x+

Comment 39

[Joe Drew (not getting mail)]

When this patch landed, it caused a Tp regression and actually increased memory usage in the Tp pageset. This is probably because, on the modern web, gifs are mostly used for things like fully transparent spacers (that get optimized to simple patterns, with the image data released entirely) and other small things such that the compressed data is actually *larger* than the bitmap.

It's probably possible to enable gif unloading in the subset of cases that it actually helps, but I don't think it's worth the extra time for the (clearly very tiny) memory gain we'll get from it.

Comment 40

[Jesse Ruderman]

Wouldn't the same be true for PNG, especially as more of the web moves from GIF to PNG?  I think we should figure out the appropriate heuristics instead of caching GIF data but not caching PNG data.

Comment 41

[Joe Drew (not getting mail)]

It might be better to get the right heuristics, but I disagree that PNG is going to have the same issues that GIF has now -- unless you can forsee a web where spacer.png is the norm.