Blocks a TEF+ blocker, so this one is TEF+ too.
mSize should be set to 0 in the constructor, I updated the patch to do so
Created attachment 742111 [details] [diff] [review] updated patch
> + // Expire the image cache early if its larger than we want it to be. > + while (gImageCache->mSize > size_t(sCanvasImageCacheLimit)) > + gImageCache->AgeOneGeneration(); With sCanvasImageCacheLimit set to 0 outside b2g, I think this disables the cache.
right, lemme fix that
Created attachment 742113 [details] [diff] [review] patch
Attachment #742111 - Attachment is obsolete: true
Attachment #742113 - Flags: review?(justin.lebar+bug)
Attachment #742113 - Flags: review?(justin.lebar+bug) → review+
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
When I apply this patch to b2g-18 and then remove the 3-second timeout in bug 861965, I see two things: 1) When I start gallery the first time, it scans small images, but the first time it comes to a big ~5mp image, the scanning process hangs. Something in the process is not completing. Given the nature of the patch I'm assuming it is the call to drawImage or the subsequent call to canvas.toBlob(). So the gallery hangs while scanning, and the UX is non-responsive: I can't tap on one of the already-scanned thumbnails to view it. 2) If I kill and restart the gallery, then I see the first of the 5mp images, and then it OOMs (but does not actually crash as I was seeing on m-c). When I modified CanvasImageCache::NotifyDrawImage so it returned early, that did solve my bug, so I think we're on the right track here.
I've investigated more carefully what's happening in the gallery app. When I run with this patch, the first large image is processed successfully, except that the thumbnail does not get displayed. (I don't know if the thumbnail is displayed but is blank or if it just doesn't get drawn.) Its the second large image where scanning hangs. And it hangs in the call to canvas.toBlob() where I'm trying to extract a jpeg blob from a canvas I've just done a drawImage() to.
If I add a 10ms setTimeout() between large images, nothing changes. If I make it 1000ms, I get an OOM on the first run rather than blocking in the toBlob() call.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Err, why was this resolved when it hit inbound? Also, isn't B2G work supposed to be landing on birch?
I definitely caused that crash.
Created attachment 742184 [details] [diff] [review] properly update cache size, and avoid shutdown crash
To make sure I understand this: If you'd changed the observer to be a weak ref, would that also have solved the shutdown crashes? I shouldn't have let you get away with those non-virtual overrides. :-/
I don't know anything about all about the canvas image cache, but I'm guessing that it holds on to images passed to drawImage() so that we can pretend that drawImage() is a synchronous function. Currently my gallery code is doing just that: it calls drawImage(), then sets img.src = '' to release the image memory and then calls canvas.toBlob(). So presumably I'm releasing image memory before the draw is done. And if this patch doesn't cache my image because it is so big, what is going to happen? Does some other kind of image locking take care of this case, or is it possible that bad things will happen here? Obviously, I can change the gallery app to only release the image once toBlob() has called its callback, but we can't mandate that all code on the web does that... For large images that we are not going to store in the canvas image cache, can we make drawImage() synchronous?
This latest version of the patch fixes the issue for me in bug 861965. (To be on the safe side, I've also changed my code to free the image after creating the blob.)
MemShrink doesn't pre-prioritize bugs (as e.g. snappy does), because we like to look over the list of new, untriaged bugs at our meetings.
Whiteboard: [MemShrink:P1] → [MemShrink]
For what it's worth, any class that inherits from nsExpirationTracker automatically observes memory-pressure, so the change here should be redundant: http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsExpirationTracker.h#286
jdm, cool, I will remove those parts
Created attachment 742347 [details] [diff] [review] patch, minus redundant memory pressure purging
Comment on attachment 742347 [details] [diff] [review] patch, minus redundant memory pressure purging Review of attachment 742347 [details] [diff] [review]: ----------------------------------------------------------------- This r+ is just approving the removal of the redundant code; I know nothing about CanvasImageCache so it shouldn't be taken as a blessing of the remaining changes.
Attachment #742347 - Flags: review?(josh) → review+
jdm reviewed the removal, jlebar already reviewed the canvasimagecache changes
Summary: canvas image cache isn't hooked up to memory pressure purging, and needs a size limit → Add Pref to limit the size of the canvas image cache
Whiteboard: [MemShrink] → [MemShrink][fixed-in-birch]
comment 17: drawImage does a sync decode if needed, so I think your code is fine. The image cache is strictly to optimize the case where you do two subsequent drawImage calls with the same image within 1-2 seconds (games animating sprites).
Good to know that I don't have to worry about the cache. The current version of the patch still fixes my gallery issue. Thanks!
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago → 6 years ago
Resolution: --- → FIXED
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
Sorry for bug spam, but I and confused whether this bug is actually in m-c builds. Looking tbpl it appears it landed in m-c under cset: https://hg.mozilla.org/mozilla-central/rev/ccdc2beb09ae then backed out in cset: https://hg.mozilla.org/mozilla-central/rev/7338d59869c3 It seems to be that it landed in m-c from a birch->m-c merge as noted in comment #28 then as I noted above it landed in m-c with a different cset, then backed out... Sorry, color me confused - if this has indeed landed correctly in m-c the nevermind me, but seems from what I see this bug is not in m-c and should be re-opened >?
Seems like it's currently in tip of trunk. https://github.com/mozilla/mozilla-central/blob/HEAD/b2g/app/b2g.js#L31
The pref is present in Firefox 23 RC - 20130730113002, with the default value '0'.
status-firefox23: fixed → verified
You need to log in before you can comment on or make changes to this bug.