865929 - Add Pref to limit the size of the canvas image cache

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Blocks a TEF+ blocker, so this one is TEF+ too.

Comment 3

[Andreas Gal :gal]

mSize should be set to 0 in the constructor, I updated the patch to do so

Comment 4

[Andreas Gal :gal]

Attachment: updated patch

Comment 5

[Justin Lebar (not reading bugmail)]

> +  // 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.

Comment 6

[Andreas Gal :gal]

right, lemme fix that

Comment 7

[Andreas Gal :gal]

Attachment: patch

Comment 8

[Andreas Gal :gal]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccdc2beb09ae

Comment 9

[David Flanagan [:djf]]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[David Flanagan [:djf]]

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.

Comment 12

[David Flanagan [:djf]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Err, why was this resolved when it hit inbound? Also, isn't B2G work supposed to be landing on birch?

Comment 14

[Andreas Gal :gal]

I definitely caused that crash.

Comment 15

[Andreas Gal :gal]

Attachment: properly update cache size, and avoid shutdown crash

Comment 16

[Justin Lebar (not reading bugmail)]

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.  :-/

Comment 17

[David Flanagan [:djf]]

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?

Comment 18

[David Flanagan [:djf]]

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.)

Comment 19

[Justin Lebar (not reading bugmail)]

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.

Comment 20

[Josh Matthews [:jdm]]

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

Comment 21

[Andreas Gal :gal]

jdm, cool, I will remove those parts

Comment 22

[Andreas Gal :gal]

Attachment: patch, minus redundant memory pressure purging

Comment 23

[Josh Matthews [:jdm]]

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.

Comment 24

[Andreas Gal :gal]

jdm reviewed the removal, jlebar already reviewed the canvasimagecache changes

Comment 25

[Andreas Gal :gal]

http://hg.mozilla.org/projects/birch/rev/52a6a51d2dca

Comment 26

[Andreas Gal :gal]

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).

Comment 27

[David Flanagan [:djf]]

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!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52a6a51d2dca

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c9fed3a62c90
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/dd973b9cd52f

Comment 30

[Jim Jeffery not reading bug-mail 1/2/11]

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 >?

Comment 31

[Justin Lebar (not reading bugmail)]

Seems like it's currently in tip of trunk.

https://github.com/mozilla/mozilla-central/blob/HEAD/b2g/app/b2g.js#L31

Comment 32

[Ioana (away)]

The pref is present in Firefox 23 RC - 20130730113002, with the default value '0'.