The default bug view has changed. See this FAQ.

Add Pref to limit the size of the canvas image cache

RESOLVED FIXED in Firefox 23

Status

()

Core
Canvas: 2D
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 verified, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [MemShrink][fixed-in-birch])

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 742102 [details] [diff] [review]
patch
Assignee: nobody → gal
(Assignee)

Updated

4 years ago
Blocks: 854799
(Assignee)

Comment 2

4 years ago
Blocks a TEF+ blocker, so this one is TEF+ too.
(Assignee)

Updated

4 years ago
Attachment #742102 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 3

4 years ago
mSize should be set to 0 in the constructor, I updated the patch to do so
(Assignee)

Updated

4 years ago
Whiteboard: [MemShrink:P1]
(Assignee)

Comment 4

4 years ago
Created attachment 742111 [details] [diff] [review]
updated patch
Attachment #742102 - Attachment is obsolete: true
Attachment #742102 - Flags: review?(justin.lebar+bug)
> +  // 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.
(Assignee)

Comment 6

4 years ago
right, lemme fix that
(Assignee)

Comment 7

4 years ago
Created attachment 742113 [details] [diff] [review]
patch
Attachment #742111 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #742113 - Flags: review?(justin.lebar+bug)
Attachment #742113 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccdc2beb09ae
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
blocking-b2g: --- → tef+
Target Milestone: --- → mozilla23
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.
Backed out for crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7338d59869c3

https://tbpl.mozilla.org/php/getParsedLog.php?id=22263282&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22263522&tree=Mozilla-Inbound
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.

Updated

4 years ago
Blocks: 861965
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?
(Assignee)

Comment 14

4 years ago
I definitely caused that crash.
(Assignee)

Comment 15

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

Comment 21

4 years ago
jdm, cool, I will remove those parts
(Assignee)

Comment 22

4 years ago
Created attachment 742347 [details] [diff] [review]
patch, minus redundant memory pressure purging
Attachment #742113 - Attachment is obsolete: true
Attachment #742184 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #742347 - Flags: review?(josh)
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+
(Assignee)

Comment 24

4 years ago
jdm reviewed the removal, jlebar already reviewed the canvasimagecache changes
(Assignee)

Updated

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

Comment 25

4 years ago
http://hg.mozilla.org/projects/birch/rev/52a6a51d2dca
Whiteboard: [MemShrink] → [MemShrink][fixed-in-birch]
(Assignee)

Comment 26

4 years ago
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!
https://hg.mozilla.org/mozilla-central/rev/52a6a51d2dca
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c9fed3a62c90
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/dd973b9cd52f
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

Comment 32

4 years ago
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.