Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(510)

Issue 2075093002: Don't cache GIF frames if it would be too large. (Closed)

Created:
4 years, 6 months ago by cblume
Modified:
4 years, 5 months ago
Reviewers:
scroggo_chromium, boliu
CC:
chromium-reviews, blink-reviews, boliu
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't cache GIF frames if it would be too large. The GIF decoder now respects m_maxDecodedBytes and checks for overflow. If the GIF would go over the memory limit then cached frames provide no benefit. By the time the animation loops we would have been forced to purge those frames. So caching any extra (not currently needed) frames would be a waste. In that case, run lean and purge aggressively. R=scroggo@chromium.org BUG=619173 Committed: https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 Cr-Commit-Position: refs/heads/master@{#402536}

Patch Set 1 #

Total comments: 16

Patch Set 2 : Changes to overflow detection and comments #

Total comments: 18

Patch Set 3 : Fixing build errors. #

Patch Set 4 : Handling cases overflow-infinity. Clarifying. #

Total comments: 8

Patch Set 5 : Moving code to private member function. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -0 lines) Patch
M third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp View 1 2 3 4 4 chunks +49 lines, -0 lines 0 comments Download

Messages

Total messages: 49 (16 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075093002/1
4 years, 6 months ago (2016-06-17 09:35:16 UTC) #2
cblume
PTAL
4 years, 6 months ago (2016-06-17 09:43:42 UTC) #3
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-17 11:26:46 UTC) #5
scroggo_chromium
> The GIF decoder now respects the memory limit. > Which limit? It looks like ...
4 years, 6 months ago (2016-06-20 17:59:40 UTC) #6
cblume
https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/1/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode312 third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:312: // image memory usage. On 2016/06/20 17:59:40, scroggo_chromium wrote: ...
4 years, 6 months ago (2016-06-21 01:00:52 UTC) #7
cblume
>> The GIF decoder now respects the memory limit. >> > >Which limit? It looks ...
4 years, 6 months ago (2016-06-21 01:49:11 UTC) #8
scroggo_chromium
On 2016/06/21 01:49:11, cblume wrote: > >> The GIF decoder now respects the memory limit. ...
4 years, 6 months ago (2016-06-21 15:46:08 UTC) #9
cblume
On 2016/06/21 15:46:08, scroggo_chromium wrote: > That said, the comparison for overflow is somewhat arbitrary, ...
4 years, 6 months ago (2016-06-22 01:23:06 UTC) #11
scroggo_chromium
On 2016/06/22 01:23:06, cblume wrote: > On 2016/06/21 15:46:08, scroggo_chromium wrote: > > That said, ...
4 years, 6 months ago (2016-06-22 15:10:04 UTC) #12
cblume
On 2016/06/22 15:10:04, scroggo_chromium wrote: > Maybe m_maxDecodedBytes should be set to whatever limit we ...
4 years, 6 months ago (2016-06-22 19:02:11 UTC) #13
cblume
Thank you for waiting on this while I took care of other fires. I think ...
4 years, 5 months ago (2016-06-27 18:24:51 UTC) #15
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2075093002/40001
4 years, 5 months ago (2016-06-27 18:42:22 UTC) #17
scroggo_chromium
https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode311 third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:311: // the image would use too much memory. Maybe ...
4 years, 5 months ago (2016-06-27 19:04:32 UTC) #18
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-06-27 20:05:59 UTC) #20
cblume
https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/20001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode311 third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:311: // the image would use too much memory. On ...
4 years, 5 months ago (2016-06-27 23:11:18 UTC) #22
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2075093002/60001
4 years, 5 months ago (2016-06-27 23:12:28 UTC) #23
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-06-28 00:52:15 UTC) #25
scroggo_chromium
https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode305 third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:305: if (!m_purgeAggressively) { How do you feel about making ...
4 years, 5 months ago (2016-06-28 15:55:28 UTC) #26
cblume
https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2075093002/diff/60001/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp#newcode305 third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:305: if (!m_purgeAggressively) { On 2016/06/28 15:55:28, scroggo_chromium wrote: > ...
4 years, 5 months ago (2016-06-28 16:53:33 UTC) #27
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2075093002/80001
4 years, 5 months ago (2016-06-28 16:54:19 UTC) #29
scroggo_chromium
lgtm
4 years, 5 months ago (2016-06-28 16:56:16 UTC) #30
cblume
On 2016/06/28 16:56:16, scroggo_chromium wrote: > lgtm Thanks for reviewing so diligently. I'm red in ...
4 years, 5 months ago (2016-06-28 19:53:36 UTC) #31
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-06-28 20:07:00 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2075093002/80001
4 years, 5 months ago (2016-06-28 20:11:59 UTC) #35
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
4 years, 5 months ago (2016-06-28 20:12:02 UTC) #37
boliu
committer rubberstamp lgtm
4 years, 5 months ago (2016-06-28 20:22:05 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2075093002/80001
4 years, 5 months ago (2016-06-28 20:24:09 UTC) #41
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 5 months ago (2016-06-28 20:36:35 UTC) #43
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9 Cr-Commit-Position: refs/heads/master@{#402536}
4 years, 5 months ago (2016-06-28 20:39:29 UTC) #45
aleksandar.stojiljkovic
On 2016/06/28 20:39:29, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as ...
4 years, 5 months ago (2016-06-28 21:22:31 UTC) #46
cblume
On 2016/06/28 21:22:31, aleksandar.stojiljkovic wrote: > On 2016/06/28 20:39:29, commit-bot: I haz the power wrote: ...
4 years, 5 months ago (2016-06-28 21:42:02 UTC) #47
aleksandar.stojiljkovic
On 2016/06/28 21:42:02, cblume wrote: > On 2016/06/28 21:22:31, aleksandar.stojiljkovic wrote: > > On 2016/06/28 ...
4 years, 5 months ago (2016-06-28 22:00:30 UTC) #48
cblume
4 years, 5 months ago (2016-06-28 23:36:48 UTC) #49
Message was sent while issue was closed.
On 2016/06/28 22:00:30, aleksandar.stojiljkovic wrote:
> On 2016/06/28 21:42:02, cblume wrote:
> > On 2016/06/28 21:22:31, aleksandar.stojiljkovic wrote:
> > > On 2016/06/28 20:39:29, commit-bot: I haz the power wrote:
> > > > Patchset 5 (id:??) landed as
> > > > https://crrev.com/9899eade00c556b8086b74867f02edf8c98cb4b9
> > > > Cr-Commit-Position: refs/heads/master@{#402536}
> > > 
> > > I don’t know if it OK to post this to already closed review.
> > > 
> > > Nice find and a good patch. Some input maybe for follow up patch…
> > > 
> > > Discussion about linking this  cache to cc cache makes a lot of sense
since
> > some
> > > of the frames in later loops could be already cached from the previous
> > animation
> > > loops and in addition that would also remove memcpy (bitmap.copyPixelsTo)
> from
> > > ImageFrameDecoder::decodeAndScale [1]
> > > 
> > > About purgeAggressively – we could try to do this for every animation and
> > remove
> > > the calculation? GIFImageDecoder::clearCacheExceptFrame(size_t
> > clearExceptFrame)
> > > keeps all the frames required for animation frames with higher index. Some
> > > discussion here with pkasting@ about it:
> > > https://codereview.chromium.org/1574283002
> > > 
> > >  [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
> > 
> > It is always okay to discuss. :)
> > We can also discuss over on the bug:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=619173
> > 
> > I agree 100% about linking the decoder's cache to CC's cache. In fact, we
> > discussed that above in comment #11
> > https://codereview.chromium.org/2075093002/#msg11
> > That is a more major change, which will take more work. This is more of a
> > for-now solution.
> > 
> > When aggressively purging, it will eventually call clearCacheExceptFrame().
So
> > we should still keep the frames that are needed. And it does this per-frame.
> So
> > next frame, it calls clearCacheExceptFrame() again to allow
> > clearCacheExceptFrame() to continue to hold onto the frames it thinks would
be
> > useful. This takes advantage of the wisdom inside clearCacheExceptFrame().
We
> > actually discussed that as well in comment #13
> > https://codereview.chromium.org/2075093002/#msg13
> > 
> > Is that what you meant? I may not have understood well.
> 
> Yes, correct like you said in comment #13. After clearCacheExceptFrame() we
> still keep the frames (it keeps one or two depending on dispose method) that
are
> needed - so it should be fine to call clearCacheExceptFrame() always after
> decoding a frame. 
> 
> I meant to replace code:
>  if (m_purgeAggressively) 
>      clearCacheExceptFrame(*i); 
> 
> with just:
>  clearCacheExceptFrame(*i);

Ah.

If the image is small, we can keep it in cache without taking much of a memory
hit.
I only really want to ignore the cache when we couldn't cache the whole image
anyway.


Right now, each image has its own cache limit so this goes back to the CC cache
topic. It would be better if CC maintained a total cache limit for images,
rather than a per-image cache limit. 1000 small images could actually use a lot
of memory via their caches. But we'll address this when we consolidate caches.

Powered by Google App Engine
This is Rietveld 408576698