Closed Bug 399925 Opened 12 years ago Closed 12 years ago

GIF decoder needs to allow its data to be discarded

Categories

(Core :: ImageLib, defect, P2)

defect

Tracking

()

RESOLVED WONTFIX
mozilla1.9

People

(Reporter: pavlov, Assigned: joe)

References

Details

(Keywords: testcase)

Attachments

(3 files, 3 obsolete files)

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.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee: nobody → pavlov
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.
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...
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.
Depends on: 403364
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.)
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.
Assignee: pavlov → alfredkayser
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.
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.
Attachment #296207 - Flags: review?(pavlov)
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.
Attachment #296207 - Attachment is obsolete: true
Attachment #297176 - Flags: review?(pavlov)
Attachment #296207 - Flags: review?(pavlov)
Attachment #297176 - Flags: review?(pavlov) → review+
Attachment #297176 - Flags: superreview?(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.
Attachment #297176 - Flags: superreview?(tor) → superreview+
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.
Attachment #297176 - Attachment is obsolete: true
So, also the 15s discard timer is no longer in this patch.
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
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: GIF decoder needs to allow its data to be discarded. → GIF decoder needs to allow its data to be discarded
Target Milestone: --- → mozilla1.9 M11
Backed this patch out, as it caused Tp crashes on Linux bl-bldlnx03 Dep fx-linux-tbox perf test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 418791
When bug 418791 has been checked in, we could test this again.
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta3 → mozilla1.9beta5
Nope, still crashes Tp. We need to get a stack trace. :(

Backed out again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please make sure to add whatever is crashing this to crashtests when this lands.
--> mozilla1.9, reset if you think this needs to block beta 5.
Target Milestone: mozilla1.9beta5 → mozilla1.9
Alfred, are you actively working on the crashes?
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...
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.
Assignee: alfredkayser → joe
Status: REOPENED → NEW
Please, go ahead!
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.
Attachment #298751 - Attachment is obsolete: true
Attachment #311708 - Flags: review?(pavlov)
Attachment #311708 - Flags: review?(pavlov) → review+
Keywords: checkin-needed
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.
Attachment #311899 - Flags: review?(pavlov)
Flags: in-testsuite?
Is the timeout a pref?  If so, could the test twiddle this pref to a lower value and reduce the delay?
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.
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
Status: NEW → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Any reason the test wasn't checked in?
(In reply to comment #30)
> Any reason the test wasn't checked in?

It's waiting for review from stuart...
Attachment #311899 - Flags: review?(pavlov) → review+
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
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
I have disabled mochitest in libpr0n for the time being because that testcase is crashing on Linux
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.
Status: RESOLVED → REOPENED
Flags: in-testsuite+ → in-testsuite?
Resolution: FIXED → ---
Depends on: 425941
> +    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.
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.
Ugh, I can't reproduce the *crash*.
We wouldn't hold the release for this.  wanted1.9.0.x+
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → WONTFIX
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.
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.
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.