Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2365943005:
Cap memory usage in webp catch up (Closed)
DescriptionCap memory usage in webp catch up
Currently, the Blink decoders get purged between each call to decode.
However, when the decoder needs to catch up, the cached frames are not
purged. They accumulate until the catch up has completed, which causes a
memory spike.
This may not be as much of a problem for webp, which likely has a
shorter catch up distance than gif (since it is more likely to resume
from a near-by keyframe).
Nevertheless, the memory spike should be avoided.
BUG=636205
Committed: https://crrev.com/cfcf81cfe56e8e34b30222d25d0e9b5eaaaf4b45
Cr-Commit-Position: refs/heads/master@{#424331}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Simplifying overflow checks. #Patch Set 3 : Simplifying another piece. #
Total comments: 11
Patch Set 4 : Rebasing. #Patch Set 5 : Moving aggressive purge update into demuxer update. #
Total comments: 2
Patch Set 6 : Moving purge check out of conditional branch. #Messages
Total messages: 28 (11 generated)
cblume@chromium.org changed reviewers: + scroggo@chromium.org
PTAL This is basically the WEBP version of what we did for GIF: https://crrev.com/2075093002
skal@google.com changed reviewers: + skal@google.com
https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:536: return; indent is off https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:539: const uint64_t frameMemoryUsage = frameArea * 4; // 4 bytes per pixel you could simplify the test to just check that the result is representable: if (frameMemoryUsage / 4 != frameArea) { // overflow occurred m_purgeAggressively = true; return; } https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:552: const uint64_t totalMemoryUsage = frameMemoryUsage * index; and here too: if (totalMemoryUsage / index != frameMemoryUsage) { m_purgeAggressively = true; return; }
https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); don't you want to reset m_purgeAggressively to false here, after clearing the cache?
https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); On 2016/09/24 17:15:53, skal wrote: > don't you want to reset m_purgeAggressively to false here, after clearing the > cache? No, we want m_purgeAggressively to remain. The idea is supposed to be: 1.) Assume there is enough space to cache everything. 2.) As we decode, we may learn caching all the image frames requires more space than m_maxDecodedBytes. 3.) At that point, we *could* keep only some frames cached. But ideally, the choice of which frames to cache would be done by the caller. 4.) So rather than arbitrarily cache some frames, only hold onto keyframes we will need soon and purge everything else. This sort of permanently puts it into a "fast-for-forward-animation" caching mode while trimming down memory. It is perhaps the best we can do at this granularity. https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:536: return; On 2016/09/24 17:14:27, skal wrote: > indent is off Done. https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:539: const uint64_t frameMemoryUsage = frameArea * 4; // 4 bytes per pixel On 2016/09/24 17:14:27, skal wrote: > you could simplify the test to just check that the result is representable: > > if (frameMemoryUsage / 4 != frameArea) { // overflow occurred > m_purgeAggressively = true; > return; > } Oh, I see what you mean. First initialize it: frameMemoryUsage = frameArea * 4; Then check that it didn't overflow with: if (frameMemoryUsage / 4 != frameArea) { Yeah, that is a much better idea. I like that a lot. Thanks! I'll go add this to the GIF decoder as well. https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:552: const uint64_t totalMemoryUsage = frameMemoryUsage * index; On 2016/09/24 17:14:27, skal wrote: > and here too: > > if (totalMemoryUsage / index != frameMemoryUsage) { > m_purgeAggressively = true; > return; > } This one is different. This one is checking against m_maxDecodedBytes, which is the memory limit the decoder shouldn't have been surpassing. This is where we find if caching all the frames would be too large.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
From the commit message: > This may not be as much of a problem for webp, which likely has a > shorter catch up distance than gif (since it is more likely to resume > from a near-by keyframe). > > Nevertheless, the memory spike should be avoided. I'm a little skeptical. If the memory spike is a problem in WEBP, and this addresses it, then by all means we should have it. But it sounds like it may not actually be necessary. https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); On 2016/09/25 05:30:05, cblume wrote: > On 2016/09/24 17:15:53, skal wrote: > > don't you want to reset m_purgeAggressively to false here, after clearing the > > cache? > > No, we want m_purgeAggressively to remain. > > The idea is supposed to be: > 1.) Assume there is enough space to cache everything. > 2.) As we decode, we may learn caching all the image frames requires more space > than m_maxDecodedBytes. > 3.) At that point, we *could* keep only some frames cached. But ideally, the > choice of which frames to cache would be done by the caller. > 4.) So rather than arbitrarily cache some frames, only hold onto keyframes we > will need soon and purge everything else. > > This sort of permanently puts it into a "fast-for-forward-animation" caching > mode while trimming down memory. It is perhaps the best we can do at this > granularity. This is probably worth a comment. I had the same question on the GIF version. https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:514: void WEBPImageDecoder::updateAggressivePurging(size_t index) This looks to be the exact same method as in GIFImageDecoder (after you update it in crrev.com/2371563002). Once APNG is in, we may want the same thing there. I think this method belongs in ImageDecoder.
https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:538: const uint64_t totalMemoryUsage = frameMemoryUsage * index; this multiply is still even more likely to overflow that then one at 532. You should also check that the presentation if the result is exact (before comparing to m_maxDecodedBytes), with something like: if (totalMemoryUsage / frameMemoryUsage != index) { // overflow occurred ... } (sorry i misplaced the line of my previous similar comment)
urvang@chromium.org changed reviewers: + urvang@chromium.org
https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); I think you need to purge after line#444. Otherwise, the decoding of 'i'th frame may NOT have completed yet, which means that the frame at (i-1) may still be needed (to decode rest of the pixels of ith frame).
>I'm a little skeptical. If the memory spike is a problem in WEBP, and this addresses it, then by all means we should have it. But it sounds like it may not actually be necessary. WEBP is much less likely to cause the problem we saw in the GIF decoder. This is because WEBP is (we think) more likely to have a near by keyframe. So it will decode fewer frames when catching up. We saw problems on the GIF decoder where catching up cached so many frames we would OOM. That said, I think even if WEBP typically has a near by keyframe, I don't think it is required. The worst-case-scenario could be as bad as we saw on GIF. https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); On 2016/09/26 12:04:48, scroggo_chromium wrote: > On 2016/09/25 05:30:05, cblume wrote: > > On 2016/09/24 17:15:53, skal wrote: > > > don't you want to reset m_purgeAggressively to false here, after clearing > the > > > cache? > > > > No, we want m_purgeAggressively to remain. > > > > The idea is supposed to be: > > 1.) Assume there is enough space to cache everything. > > 2.) As we decode, we may learn caching all the image frames requires more > space > > than m_maxDecodedBytes. > > 3.) At that point, we *could* keep only some frames cached. But ideally, the > > choice of which frames to cache would be done by the caller. > > 4.) So rather than arbitrarily cache some frames, only hold onto keyframes we > > will need soon and purge everything else. > > > > This sort of permanently puts it into a "fast-for-forward-animation" caching > > mode while trimming down memory. It is perhaps the best we can do at this > > granularity. > > This is probably worth a comment. I had the same question on the GIF version. Good idea. I added a comment to the header above m_purgeAggressively's definition. I also added this to the base ImageDecoder as per other code review comments. You can see it over at: https://codereview.chromium.org/2376033005/ https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); On 2016/09/26 18:37:05, urvang wrote: > I think you need to purge after line#444. > Otherwise, the decoding of 'i'th frame may NOT have completed yet, which means > that the frame at (i-1) may still be needed (to decode rest of the pixels of ith > frame). Done. I should do this on the GIF decoder as well. I updated the GIFImageDecoder version over at https://crrev.com/2382143004 https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:514: void WEBPImageDecoder::updateAggressivePurging(size_t index) On 2016/09/26 12:04:48, scroggo_chromium wrote: > This looks to be the exact same method as in GIFImageDecoder (after you update > it in crrev.com/2371563002). Once APNG is in, we may want the same thing there. > I think this method belongs in ImageDecoder. Good call. I've moved this into ImageDecoder in https://crrev.com/2376033005 https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:538: const uint64_t totalMemoryUsage = frameMemoryUsage * index; On 2016/09/26 12:22:51, skal wrote: > this multiply is still even more likely to overflow that then one at 532. > > You should also check that the presentation if the result is exact > (before comparing to m_maxDecodedBytes), with something like: > if (totalMemoryUsage / frameMemoryUsage != index) { // overflow occurred > ... > } > > > (sorry i misplaced the line of my previous similar comment) Ah, got ya. Fixed. I've moved this into ImageDecoder in https://crrev.com/2376033005
I believe you'll submit the other patch -- moving common stuff to ImageDecoder -- and rebase this on top of that. So, will wait for the rebase. https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); On 2016/10/01 11:06:23, cblume wrote: > On 2016/09/26 18:37:05, urvang wrote: > > I think you need to purge after line#444. > > Otherwise, the decoding of 'i'th frame may NOT have completed yet, which means > > that the frame at (i-1) may still be needed (to decode rest of the pixels of > ith > > frame). > > Done. I don't see this change yet. Forgot to upload?
https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:416: updateAggressivePurging(index); You only need to call this method when number of frames (seen so far) changes, right? Then, you can do this only at the end of UpdateDemuxer() at line#240.
https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:416: updateAggressivePurging(index); On 2016/10/03 21:07:23, urvang wrote: > You only need to call this method when number of frames (seen so far) changes, > right? > Then, you can do this only at the end of UpdateDemuxer() at line#240. Sounds good. It looks like updateDemuxer() is called from frameCount(), which is called from a handful of locations. And updateDemuxer() exits early after it is called for each setData(). So presumably then updateDemuxer() is run once per incoming data set (as data arrives). Is my understanding correct? https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: clearCacheExceptFrame(*i); On 2016/10/03 19:12:36, urvang wrote: > On 2016/10/01 11:06:23, cblume wrote: > > On 2016/09/26 18:37:05, urvang wrote: > > > I think you need to purge after line#444. > > > Otherwise, the decoding of 'i'th frame may NOT have completed yet, which > means > > > that the frame at (i-1) may still be needed (to decode rest of the pixels of > > ith > > > frame). > > > > Done. > > I don't see this change yet. Forgot to upload? Oh, yeah. Sorry.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:416: updateAggressivePurging(index); On 2016/10/10 21:18:38, cblume wrote: > On 2016/10/03 21:07:23, urvang wrote: > > You only need to call this method when number of frames (seen so far) changes, > > right? > > Then, you can do this only at the end of UpdateDemuxer() at line#240. > > Sounds good. > > It looks like updateDemuxer() is called from frameCount(), which is called from > a handful of locations. And updateDemuxer() exits early after it is called for > each setData(). So presumably then updateDemuxer() is run once per incoming data > set (as data arrives). Is my understanding correct? Yes, updateDemuxer() updates the demuxer and related metadata (like frame count) whenever we have received some new data (since the last update). https://codereview.chromium.org/2365943005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:238: size_t frameCount = WebPDemuxGetI(m_demux, WEBP_FF_FRAME_COUNT); No, this is not the right place. "!isDecodedSizeAvailable()" would be true only once for an image, because overall width/height of an image comes at the beginning of a WebP file. (Note: this is the canvas width/height, and not that of a given frame). So, this logic happen unconditionally, and so, it should go at the end of updateDemuxer() (just before "return true").
https://codereview.chromium.org/2365943005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2365943005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:238: size_t frameCount = WebPDemuxGetI(m_demux, WEBP_FF_FRAME_COUNT); On 2016/10/10 21:30:50, urvang wrote: > No, this is not the right place. "!isDecodedSizeAvailable()" would be true only > once for an image, because overall width/height of an image comes at the > beginning of a WebP file. (Note: this is the canvas width/height, and not that > of a given frame). > > So, this logic happen unconditionally, and so, it should go at the end of > updateDemuxer() (just before "return true"). Done.
lgtm
The CQ bit was checked by cblume@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Cap memory usage in webp catch up Currently, the Blink decoders get purged between each call to decode. However, when the decoder needs to catch up, the cached frames are not purged. They accumulate until the catch up has completed, which causes a memory spike. This may not be as much of a problem for webp, which likely has a shorter catch up distance than gif (since it is more likely to resume from a near-by keyframe). Nevertheless, the memory spike should be avoided. BUG=636205 ========== to ========== Cap memory usage in webp catch up Currently, the Blink decoders get purged between each call to decode. However, when the decoder needs to catch up, the cached frames are not purged. They accumulate until the catch up has completed, which causes a memory spike. This may not be as much of a problem for webp, which likely has a shorter catch up distance than gif (since it is more likely to resume from a near-by keyframe). Nevertheless, the memory spike should be avoided. BUG=636205 Committed: https://crrev.com/cfcf81cfe56e8e34b30222d25d0e9b5eaaaf4b45 Cr-Commit-Position: refs/heads/master@{#424331} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cfcf81cfe56e8e34b30222d25d0e9b5eaaaf4b45 Cr-Commit-Position: refs/heads/master@{#424331} |
