Issue 1925533003: High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

aleksandar.stojiljkovic

Description was changed from ========== Don't recreate/redecode SkImages for high-res (GIF, WEBP...) animations. Patch is ...

4 years, 7 months ago (2016-04-27 08:14:08 UTC) #1

Description was changed from

==========
Don't recreate/redecode SkImages for high-res (GIF, WEBP...) animations.

Patch is addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.

- Out of memory fix
===================
Originally, decoded high res GIF frames were not cached in BitmapImage, leading
to re-creating SkImages and filling up SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueId and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by limited caching of high res animated frames
in BitmapImage cache.

@f(malita)'s concern from [2], [3] is addressed, too: memory used with this page
is
under the original limit.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original  : ~500MB
This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Cached SkImages in BitmapImage don't need to be re-decoded. This covers both
Skia's discardable cache and SIDC (SoftwareImageDecodeController). Pixel data
disposal in Skia's discardable memory is something SkImage is not aware of until
accessing PixelRef (unpinned ashmem) - keeping the references in cache is at
least trying to avoid redecoding.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- destroyDecodedDataIfNecessary replaced by MRU access tracking
===============================================================
Per process BitmapImage cache added to track (MRU) access to animated
BitmapImage frames. It gives some finer control when gif animation frames
should be disposed instead of disposing them always based on 5MB rule.
Cache defines quota for animated GIF frames in one process and quota per
animation too - allowing caching only first frames mitting the quota (instead
of last recently used).

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

to

==========
Don't recreate/redecode SkImages for high-res (GIF, WEBP...) animations.

This cannot be submitted yet as the patch here is using base MRUHash.
While resolving this, I'd like to get some early feedback to resolve other
issues,
too. Thanks. 

Patch is addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.

- Out of memory fix
===================
Originally, decoded high res GIF frames were not cached in BitmapImage, leading
to re-creating SkImages and filling up SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueId and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by limited caching of high res animated frames
in BitmapImage cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
under the original limit.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original  : ~500MB
This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Cached SkImages in BitmapImage don't need to be re-decoded. This covers both
Skia's discardable cache and SIDC (SoftwareImageDecodeController). Pixel data
disposal in Skia's discardable memory is something SkImage is not aware of until
accessing PixelRef (unpinned ashmem) - keeping the references in cache is at
least trying to avoid redecoding.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- destroyDecodedDataIfNecessary replaced by MRU access tracking
===============================================================
Per process BitmapImage cache added to track (MRU) access to animated
BitmapImage frames. It gives some finer control when gif animation frames
should be disposed instead of disposing them always based on 5MB rule.
Cache defines quota for animated GIF frames in one process and quota per
animation too - allowing caching only first frames mitting the quota (instead
of last recently used).

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

aleksandar.stojiljkovic

aleksandar.stojiljkovic@intel.com changed reviewers: + fmalita@chromium.org, pkasting@chromium.org, senorblanco@chromium.org, vmpstr@chromium.org

4 years, 7 months ago (2016-04-27 08:14:08 UTC) #2

Peter Kasting

From your CL description: "SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_ cache key - clearing it causes ...

4 years, 7 months ago (2016-04-28 23:07:55 UTC) #4

From your CL description:

"SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages have
different uniqueId and keep filling up the cache with duplicates of same frames,
evicting useful content."

Why can't we instead fix this so that the ID of the image in the cache is based
on the actual image content, or at the very least on some sort of composite of
which image + frame number this came from?  Then the images in that cache would
not be evicted by duplicate copies.  That seems much better than hacking around
the problem, which is what it seems like this does.

The idea of adding Yet Another Caching Layer to BitmapImage makes me very
uneasy.  We have multiple caching layers already, and as noted in some of the
comments below, I'm concerned about the size of the cache here, the interaction
with the "required previous frame" stuff, etc.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:47: const size_t
kMaxCacheSizeForAnimation = 1024 * 1024 * 24;
These limits seem not only arbitrary but rather alarmingly large for e.g. mobile
devices, especially if they're per-renderer-process.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:59: void
EraseBitmap(blink::BitmapImage* pBitmapImage)
Nit: Use Google style for param naming, no Hungarian notation (multiple places)

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:87:
base::CheckedNumeric<intptr_t> signedFramesSize(image_it->second);
intptr_t is not an appropriate type to use for a signed count of bytes.  And why
do you want a signed type here anyway?  It looks as if the idea of this is to
add a (possibly negative) delta to a size_t and ensure you get a (positive)
size_t out.  In that case you should do all the math with checked size_ts.

And do you even need the checked numeric stuff?  When can these sorts of
functions be called with bogus values?  Isn't normal unchecked math good enough?

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:230: // it is needed
to notify on any change before and after the call.
Sorry, I don't understand what this comment is saying.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:736: // re-decode
and re-caching for them.
This doesn't seem like a good policy.

If you wanted to cache a fixed set of frames per image, caching the first frames
makes the least sense since they're basically guaranteed to be the cheapest to
redecode.

What you want to do is adjust the caching policy based on the frame disposal
methods (which are analyzed to create the "required previous frame" fields). 
Basically, if every frame can be decoded with no required previous frames, then
the only benefit to caching is in saving the decode of the particular frames
which get cached; you can pick any frames you want from the image, or none (if
this allows you to spend your cache budget elsewhere).

Similarly, if you never skip decoding any frames, then as long as you've always
preserved the required previous frame for the next frames, every frame decode
basically occurs in isolation and you can again just cache whatever.

But when frames are built atop previous frames, and you might end up skipping
frames when decoding, then you basically want to cache "keyframes" such that you
limit the maximum length of such sequences to small numbers.  This ensures that
seeking through the image will never cause you to have to decode lots and lots
of frames.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:109: private:
Nit: While here: It's weird to have two private: sections next to each other...
I'd combine these and ensure everything matches the Google style guide
declaration order (the enum, then constructors, then other methods, etc.).  I'd
also probably default to putting any static methods either above all or below
all non-static methods with at least one blank line between.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp (right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:41: #define
BITMAP_IMAGE_TEST 1
Don't add this.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:162:
TEST_F(BitmapImageTest, destroyAllDecodedData)
Nit: Remove "All"?

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:310: int
frameSize = static_cast<int>(m_image->size().area() *
sizeof(ImageFrame::PixelData));
Nit: Should be size_t with no cast since that's what you pass to
setAnimationCacheSizeForTesting() (2 places)

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:314: uint32_t
image0v1 = frameAtIndex(0)->uniqueID();
Nit: Inline into next statement

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:317: uint32_t
image1v1 = frameAtIndex(1)->uniqueID();
Nit: Inline into next statement

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:321: uint32_t
image2v1 = frameAtIndex(2)->uniqueID();
Nit: Inline into next statement

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:332: // Load
another animation that would evict first animation frames
Nit: Trailing period

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:346:
EXPECT_NE(image0v1, image0v2);
Nit: Compare to |image0| so |image0v1| above can be inlined into that EXPECT

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:349: // size
even second itself is under per animation limit size.
This sentence has grammar issues severe enough that I don't understand what it's
trying to say.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:351:
EXPECT_NE(second0v1, second0v2);
Nit: Compare to |second0| so |second0v1| above can be inlined into that EXPECT

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:363: // is first
for removal in MRU cache), frame index 2 added to it and cached.
Similarly this whole comment has significant grammar issues.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:366:
EXPECT_NE(image0v3, image0v4);
Nit: Compare to |image0v2| so |image0v3| above can be inlined into that EXPECT

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:368:
EXPECT_EQ(second0v3, second0v4);
Nit: Compare to |second0v2| so |second0v3| above can be inlined into that EXPECT

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:373: uint32_t
image0v5 = frameAtIndex(0)->uniqueID();
Nit: Inline into next statement

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:37: struct
DeferredFrameData {
This probably needs comments about why you can't use FrameData (maybe leaving
some fields uninitialized?) and how you selected which fields to preserve.

Furthermore, it seems like FrameData is pretty much only used by BitmapImage
after this change, in which case I'd consider moving it to BitmapImage.cpp.

aleksandar.stojiljkovic

Description was changed from ========== Don't recreate/redecode SkImages for high-res (GIF, WEBP...) animations. This cannot ...

4 years, 7 months ago (2016-04-29 16:39:58 UTC) #5

Description was changed from

==========
Don't recreate/redecode SkImages for high-res (GIF, WEBP...) animations.

This cannot be submitted yet as the patch here is using base MRUHash.
While resolving this, I'd like to get some early feedback to resolve other
issues,
too. Thanks. 

Patch is addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.

- Out of memory fix
===================
Originally, decoded high res GIF frames were not cached in BitmapImage, leading
to re-creating SkImages and filling up SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueId and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by limited caching of high res animated frames
in BitmapImage cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
under the original limit.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original  : ~500MB
This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Cached SkImages in BitmapImage don't need to be re-decoded. This covers both
Skia's discardable cache and SIDC (SoftwareImageDecodeController). Pixel data
disposal in Skia's discardable memory is something SkImage is not aware of until
accessing PixelRef (unpinned ashmem) - keeping the references in cache is at
least trying to avoid redecoding.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- destroyDecodedDataIfNecessary replaced by MRU access tracking
===============================================================
Per process BitmapImage cache added to track (MRU) access to animated
BitmapImage frames. It gives some finer control when gif animation frames
should be disposed instead of disposing them always based on 5MB rule.
Cache defines quota for animated GIF frames in one process and quota per
animation too - allowing caching only first frames mitting the quota (instead
of last recently used).

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

to

==========
High CPU and increased memory usage fix high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
under the original limit.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original  : ~500MB
This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

aleksandar.stojiljkovic

Description was changed from ========== High CPU and increased memory usage fix high-res (GIF, WEBP...) ...

4 years, 7 months ago (2016-04-29 17:00:02 UTC) #6

Description was changed from

==========
High CPU and increased memory usage fix high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
under the original limit.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original  : ~500MB
This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

to

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
under the original limit.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original  : ~500MB
This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

aleksandar.stojiljkovic

On 2016/04/28 23:07:55, Peter Kasting wrote: > From your CL description: > > "SkImage::uniqueID() is ...

4 years, 7 months ago (2016-04-29 17:15:50 UTC) #7

aleksandar.stojiljkovic

patch#2 looks like a proper way to fix it if skia patch gets accepted: https://codereview.chromium.org/1928403002/ ...

4 years, 7 months ago (2016-04-29 17:17:43 UTC) #8

patch#2 looks like a proper way to fix it if skia patch gets accepted:
https://codereview.chromium.org/1928403002/
"Enable generating SkImage with the same uniqueID in SkImageGenerator subclass"


On 2016/04/28 23:07:55, Peter Kasting wrote:
> From your CL description:
> 
> "SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
> cache key - clearing it causes that cache is filled for every loop and every
> frame rendered (always decoding and adding to cache). Next loop's SkImages
have
> different uniqueId and keep filling up the cache with duplicates of same
frames,
> evicting useful content."
> 
> Why can't we instead fix this so that the ID of the image in the cache is
based
> on the actual image content, or at the very least on some sort of composite of
> which image + frame number this came from?  Then the images in that cache
would
> not be evicted by duplicate copies.  That seems much better than hacking
around
> the problem, which is what it seems like this does.
> 
> The idea of adding Yet Another Caching Layer to BitmapImage makes me very
> uneasy.  We have multiple caching layers already, and as noted in some of the
> comments below, I'm concerned about the size of the cache here, the
interaction
> with the "required previous frame" stuff, etc.

Yes, this wasn't good - and it was also not fixing the issue just reducing.
Patch #2 fixes it.

There wasn't a problem with "required previous frame", except increased memory
in BitmapImage as decoder caches bitmap data copy.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:47: const size_t
kMaxCacheSizeForAnimation = 1024 * 1024 * 24;
On 2016/04/28 23:07:54, Peter Kasting wrote:
> These limits seem not only arbitrary but rather alarmingly large for e.g.
mobile
> devices, especially if they're per-renderer-process.

Done.
Second patch removes this code and also reduces holding references in
BitmapImages (frames are not cached anymore) but only the most recently accessed
frame.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:59: void
EraseBitmap(blink::BitmapImage* pBitmapImage)
On 2016/04/28 23:07:54, Peter Kasting wrote:
> Nit: Use Google style for param naming, no Hungarian notation (multiple
places)

Done.
removed the code in second patch.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:87:
base::CheckedNumeric<intptr_t> signedFramesSize(image_it->second);
On 2016/04/28 23:07:54, Peter Kasting wrote:
> intptr_t is not an appropriate type to use for a signed count of bytes.  And
why
> do you want a signed type here anyway?  It looks as if the idea of this is to
> add a (possibly negative) delta to a size_t and ensure you get a (positive)
> size_t out.  In that case you should do all the math with checked size_ts.
> 
> And do you even need the checked numeric stuff?  When can these sorts of
> functions be called with bogus values?  Isn't normal unchecked math good
enough?


Done.
Removed the code in second patch.
Needs to be fixed in other places in chromium where I copied the approach from:
e.g. void ImageBuffer::updateGPUMemoryUsage() const

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:109: private:
On 2016/04/28 23:07:54, Peter Kasting wrote:
> Nit: While here: It's weird to have two private: sections next to each
other...
> I'd combine these and ensure everything matches the Google style guide
> declaration order (the enum, then constructors, then other methods, etc.). 
I'd
> also probably default to putting any static methods either above all or below
> all non-static methods with at least one blank line between.

Done.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp (right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:41: #define
BITMAP_IMAGE_TEST 1
On 2016/04/28 23:07:54, Peter Kasting wrote:
> Don't add this.

Done.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:162:
TEST_F(BitmapImageTest, destroyAllDecodedData)
On 2016/04/28 23:07:54, Peter Kasting wrote:
> Nit: Remove "All"?

Done.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:310: int
frameSize = static_cast<int>(m_image->size().area() *
sizeof(ImageFrame::PixelData));
On 2016/04/28 23:07:54, Peter Kasting wrote:
> Nit: Should be size_t with no cast since that's what you pass to
> setAnimationCacheSizeForTesting() (2 places
This method is removed in patch#2 so Done applies to all ot them bellow.

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:37: struct
DeferredFrameData {
On 2016/04/28 23:07:55, Peter Kasting wrote:
> This probably needs comments about why you can't use FrameData (maybe leaving
> some fields uninitialized?) and how you selected which fields to preserve.
> 
> Furthermore, it seems like FrameData is pretty much only used by BitmapImage
> after this change, in which case I'd consider moving it to BitmapImage.cpp.

After second patch these two diverged further -  DeferredFrameData got uniqueID
and FrameData has 2 flag fields that DeferredFrameData doesn't need.
Didn't move FrameData to BitmapFrame.cpp as it is used from BitmapImageTest.cpp
too.

aleksandar.stojiljkovic

Skia part of this feature landed: https://codereview.chromium.org/1928403002 Thanks @reed. @pkasting, since you LGTM-ed original patch ...

4 years, 7 months ago (2016-05-02 08:55:27 UTC) #9

scroggo

scroggo@google.com changed reviewers: + scroggo@google.com

4 years, 7 months ago (2016-05-02 14:07:24 UTC) #10

scroggo

Drive-by review: I came here by way of crrev.com/1928403002, and wanted to see how it's ...

4 years, 7 months ago (2016-05-02 14:07:25 UTC) #11

scroggo

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode315 third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: m_frameData[index].m_uniqueID = m_frameData[index].m_isComplete ? image->uniqueID() : 0; On 2016/05/02 ...

4 years, 7 months ago (2016-05-02 16:42:25 UTC) #12

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
m_frameData[index].m_uniqueID = m_frameData[index].m_isComplete ?
image->uniqueID() : 0;
On 2016/05/02 14:07:25, scroggo wrote:
> I'm surprised this makes a difference. As far as I can tell, m_isComplete will
> always be false. It only ever gets set to
> m_actualDecoder->frameIsCompleteAtIndex(), which only returns true if
> m_actualDecoder's frame status is set to FrameComplete. IIUC, that would mean
> that m_actualDecoder did a full decode, which defeats the point of deferred
> decoding. Or am I missing something?

I was going to suggest that you change the check for m_isComplete to
m_allDataReceived. That will do the right thing for the last frame, but not for
an earlier frame, which may be complete even though we have not received all the
data. (AFAICT, it's still more correct than m_isComplete, which I think will
never report true.)

Another option that might be interesting - hang on to the generator in this
class. Although that makes me wonder why the client did not hang on to it. My
guess is that it received more data, so it assumed that its old SkImage (holding
the generator) was invalid (i.e. not complete, and a newer one could be). This
class isn't currently in a better position to know the completeness of an
individual frame. But it could at least return the same
SkImage/DecodingImageGenerator if no more data had been received. (The
SkImage/DecodingImageGenerator themselves are small, I think, so I wouldn't be
too worried about extra objects hanging around. Neither of them are holding onto
any pixels, and the encoded data is just a ref on what is already owned by
DeferredImageDecoder.)

> 
> (Also, I think the intent of the zero is to use
> SkImageGenerator::kNeedNewImageUniqueID, right? That is protected, so you
cannot
> use it directly, but maybe you should mention it in a comment?)

aleksandar.stojiljkovic

Description was changed from ========== High CPU and increased memory usage fix for high-res (GIF, ...

4 years, 7 months ago (2016-05-02 16:44:17 UTC) #13

Description was changed from

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
under the original limit.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
Original  : ~500MB
This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

to

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
now about 3 times lower.
    
Memory consumption (Windows "process working set" [4]) with page [3]:
    Original  : ~730MB
    This patch: ~250MB

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
    Original  : ~500MB
    This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

aleksandar.stojiljkovic

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode53 third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:53: bool m_isComplete : 1; On 2016/05/02 14:07:25, scroggo wrote: ...

4 years, 7 months ago (2016-05-02 16:44:37 UTC) #14

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:53: bool
m_isComplete : 1;
On 2016/05/02 14:07:25, scroggo wrote:
> Does declaring this 1 bit make a difference here? It is between a float and a
> size_t, so won't it need to occupy a full byte?

Yes, "bool m_isComplete : 1" takes 4 bytes (checked it on Windows) as it is not
packed,... and same as it would take if not declared as a bit flag. As it looks
confusing, it is removed.

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
m_frameData[index].m_uniqueID = m_frameData[index].m_isComplete ?
image->uniqueID() : 0;
On 2016/05/02 14:07:25, scroggo wrote:
> I'm surprised this makes a difference. As far as I can tell, m_isComplete will
> always be false. It only ever gets set to
> m_actualDecoder->frameIsCompleteAtIndex(), which only returns true if
> m_actualDecoder's frame status is set to FrameComplete. IIUC, that would mean
> that m_actualDecoder did a full decode, which defeats the point of deferred
> decoding. Or am I missing something?

Leon, thanks again for finding this.
It was working partially on testing used - during partial decoding (so while
data is not fully received) activateLazyDecoding would get called multiple times
setting m_isComplete to true to those frames that were decoded. For other it was
false.

But, this was the reason why there was no significant improvement for page [3].
Instead of using 730MB it would take about 700MB.
Now, after patch #3 it is about 250MB.

[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...

> 
> (Also, I think the intent of the zero is to use
> SkImageGenerator::kNeedNewImageUniqueID, right? That is protected, so you
cannot
> use it directly, but maybe you should mention it in a comment?)

OK. There is now enum in DecodingImageGenerator that is proxying to
SkImageGenerator protected enum.

aleksandar.stojiljkovic

On 2016/05/02 16:42:25, scroggo wrote: > https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp > File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp > (right): > > https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode315 ...

4 years, 7 months ago (2016-05-02 16:56:49 UTC) #15

On 2016/05/02 16:42:25, scroggo wrote:
>
https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
> (right):
> 
>
https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
> m_frameData[index].m_uniqueID = m_frameData[index].m_isComplete ?
> image->uniqueID() : 0;
> On 2016/05/02 14:07:25, scroggo wrote:
> > I'm surprised this makes a difference. As far as I can tell, m_isComplete
will
> > always be false. It only ever gets set to
> > m_actualDecoder->frameIsCompleteAtIndex(), which only returns true if
> > m_actualDecoder's frame status is set to FrameComplete. IIUC, that would
mean
> > that m_actualDecoder did a full decode, which defeats the point of deferred
> > decoding. Or am I missing something?
> 
> I was going to suggest that you change the check for m_isComplete to
> m_allDataReceived. That will do the right thing for the last frame, but not
for
> an earlier frame, which may be complete even though we have not received all
the
> data. (AFAICT, it's still more correct than m_isComplete, which I think will
> never report true.)
Please check patch set #3 - while cached value (m_isComplete) was working in
random cases getting the state for currently decoded frame is resolving caching
for complete frames during partial decoding.


> 
> Another option that might be interesting - hang on to the generator in this
> class. 

static SkImage* SkImage::NewFromGenerator(generator) takes ownership of
generator - to it is disposed once the SkImage is disposed.
 
Although that makes me wonder why the client did not hang on to it. My
> guess is that it received more data, so it assumed that its old SkImage
(holding
> the generator) was invalid (i.e. not complete, and a newer one could be). This
> class isn't currently in a better position to know the completeness of an
> individual frame. But it could at least return the same
> SkImage/DecodingImageGenerator if no more data had been received. (The
> SkImage/DecodingImageGenerator themselves are small, I think, so I wouldn't be
> too worried about extra objects hanging around. Neither of them are holding
onto
> any pixels, and the encoded data is just a ref on what is already owned by
> DeferredImageDecoder.)
> 
> > 
> > (Also, I think the intent of the zero is to use
> > SkImageGenerator::kNeedNewImageUniqueID, right? That is protected, so you
> cannot
> > use it directly, but maybe you should mention it in a comment?)

scroggo

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode315 third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: m_frameData[index].m_uniqueID = m_frameData[index].m_isComplete ? image->uniqueID() : 0; On 2016/05/02 ...

4 years, 7 months ago (2016-05-02 17:46:27 UTC) #16

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
m_frameData[index].m_uniqueID = m_frameData[index].m_isComplete ?
image->uniqueID() : 0;
On 2016/05/02 16:44:37, aleksandar.stojiljkovic wrote:
> On 2016/05/02 14:07:25, scroggo wrote:
> > I'm surprised this makes a difference. As far as I can tell, m_isComplete
will
> > always be false. It only ever gets set to
> > m_actualDecoder->frameIsCompleteAtIndex(), which only returns true if
> > m_actualDecoder's frame status is set to FrameComplete. IIUC, that would
mean
> > that m_actualDecoder did a full decode, which defeats the point of deferred
> > decoding. Or am I missing something?
> 
> Leon, thanks again for finding this.
> It was working partially on testing used - during partial decoding (so while
> data is not fully received) activateLazyDecoding would get called multiple
times
> setting m_isComplete to true to those frames that were decoded. For other it
was
> false.

Would you please walk me through how m_isComplete ever gets set to true? AFAICT,
it only gets set to m_actualDecoder->frameIsCompleteAtIndex(). In turn, that can
only be true if we called m_actualDecoder->frameBufferAtIndex() (and the frame
was complete). But that is only called inside createFrameAtIndex, which also
calls prepareLazyDecodedFrames and activateLazyDecoding. The only way we call
frameBufferAtIndex is if m_frameData does not include the index in question, but
as I understand it, unless deferred decoding is disabled (or it's an ICO, or the
size is not available), we'll always add the frame to m_frameData (in
prepareLazyDecodedFrames). If the size was not available in
prepareLazyDecodedFrames, then calling frameBufferAtIndex should get no further,
since it is using the same data.

But if you're correct, and m_isComplete *is* getting set to true, I think that
is a bug - we're decoding on the UI thread. (FWIW, I find the use of
m_actualDecoder confusing. As I understand it, it *should* never be used to do a
frame decode (except if deferred is disabled or the image is an ICO), but I
haven't been able to convince my self that it is not for certain. (On a side
note, for GIF m_actualDecoder is used to parse the whole file, which I think is
necessary in order to know how many frames there are.)

> 
> But, this was the reason why there was no significant improvement for page
[3].
> Instead of using 730MB it would take about 700MB.
> Now, after patch #3 it is about 250MB.
> 
> [3]
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...

Glad my suggestion helped! I'm still not convinced we need to look at
m_isComplete, though.

> 
> > 
> > (Also, I think the intent of the zero is to use
> > SkImageGenerator::kNeedNewImageUniqueID, right? That is protected, so you
> cannot
> > use it directly, but maybe you should mention it in a comment?)
> 
> OK. There is now enum in DecodingImageGenerator that is proxying to
> SkImageGenerator protected enum.

Great, thanks! I think that is clearer :)

> > Another option that might be interesting - hang on to the generator in this
> > class. 
> 
> static SkImage* SkImage::NewFromGenerator(generator) takes ownership of
> generator - to it is disposed once the SkImage is disposed.

Sorry, I was imprecise. I meant hold on to the SkImage (ref-counted) which in
turn holds onto the DecodingImageGenerator.

aleksandar.stojiljkovic

On 2016/05/02 17:46:27, scroggo wrote: > https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp > File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp > (right): > > https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode315 ...

4 years, 7 months ago (2016-05-02 18:04:13 UTC) #17

On 2016/05/02 17:46:27, scroggo wrote:
>
https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
> (right):
> 
>
https://codereview.chromium.org/1925533003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
> m_frameData[index].m_uniqueID = m_frameData[index].m_isComplete ?
> image->uniqueID() : 0;
> On 2016/05/02 16:44:37, aleksandar.stojiljkovic wrote:
> > On 2016/05/02 14:07:25, scroggo wrote:
> > > I'm surprised this makes a difference. As far as I can tell, m_isComplete
> will
> > > always be false. It only ever gets set to
> > > m_actualDecoder->frameIsCompleteAtIndex(), which only returns true if
> > > m_actualDecoder's frame status is set to FrameComplete. IIUC, that would
> mean
> > > that m_actualDecoder did a full decode, which defeats the point of
deferred
> > > decoding. Or am I missing something?
> > 
> > Leon, thanks again for finding this.
> > It was working partially on testing used - during partial decoding (so while
> > data is not fully received) activateLazyDecoding would get called multiple
> times
> > setting m_isComplete to true to those frames that were decoded. For other it
> was
> > false.
> 
> Would you please walk me through how m_isComplete ever gets set to true?
AFAICT,
> it only gets set to m_actualDecoder->frameIsCompleteAtIndex(). In turn, that
can
> only be true if we called m_actualDecoder->frameBufferAtIndex() (and the frame
> was complete). But that is only called inside createFrameAtIndex, which also
> calls prepareLazyDecodedFrames and activateLazyDecoding. The only way we call
> frameBufferAtIndex is if m_frameData does not include the index in question,
but
> as I understand it, unless deferred decoding is disabled (or it's an ICO, or
the
> size is not available), we'll always add the frame to m_frameData (in
> prepareLazyDecodedFrames). If the size was not available in
> prepareLazyDecodedFrames, then calling frameBufferAtIndex should get no
further,
> since it is using the same data.
> 
> But if you're correct, and m_isComplete *is* getting set to true, I think that
> is a bug - we're decoding on the UI thread. (FWIW, I find the use of
> m_actualDecoder confusing. As I understand it, it *should* never be used to do
a
> frame decode (except if deferred is disabled or the image is an ICO), but I
> haven't been able to convince my self that it is not for certain. (On a side
> note, for GIF m_actualDecoder is used to parse the whole file, which I think
is
> necessary in order to know how many frames there are.)

I'll cover this in offline email.

> 
> > 
> > But, this was the reason why there was no significant improvement for page
> [3].
> > Instead of using 730MB it would take about 700MB.
> > Now, after patch #3 it is about 250MB.
> > 
> > [3]
> >
>
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
> 
> Glad my suggestion helped! I'm still not convinced we need to look at
> m_isComplete, though.
Note that the latest patch is not using m_isComplete anymore - it is using
m_actualDecoder->frameIsCompleteAtIndex(index).

 
> > 
> > > 
> > > (Also, I think the intent of the zero is to use
> > > SkImageGenerator::kNeedNewImageUniqueID, right? That is protected, so you
> > cannot
> > > use it directly, but maybe you should mention it in a comment?)
> > 
> > OK. There is now enum in DecodingImageGenerator that is proxying to
> > SkImageGenerator protected enum.
> 
> Great, thanks! I think that is clearer :)
> 
> 
> > > Another option that might be interesting - hang on to the generator in
this
> > > class. 
> > 
> > static SkImage* SkImage::NewFromGenerator(generator) takes ownership of
> > generator - to it is disposed once the SkImage is disposed.
> 
> Sorry, I was imprecise. I meant hold on to the SkImage (ref-counted) which in
> turn holds onto the DecodingImageGenerator.

Previous code was caching SkImages in BitmapImage and was the cause of increased
memory consumption. Important not to cache them.

aleksandar.stojiljkovic

message: > On 2016/05/02 17:46:27, scroggo wrote: > > Would you please walk me through ...

4 years, 7 months ago (2016-05-02 20:52:47 UTC) #18

Peter Kasting

I'm basically staying silent here until you both agree you're OK with whatever the latest ...

4 years, 7 months ago (2016-05-03 00:52:01 UTC) #19

scroggo_chromium

scroggo@chromium.org changed reviewers: + scroggo@chromium.org

4 years, 7 months ago (2016-05-03 13:08:51 UTC) #20

scroggo_chromium

Peter, go ahead. I have a small concern (stated in my comment, below), but I ...

4 years, 7 months ago (2016-05-03 13:08:52 UTC) #21

Peter, go ahead. I have a small concern (stated in my comment, below), but I now
understand why using frameIsCompleteAtIndex makes sense.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder &&
m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() :
DecodingImageGenerator::NeedNewImageUniqueID;
> Found the reason for misunderstanding - frameIsCompleteAtIndex is returning
true
> when framedata is fully received, not when it is fully decoded. 
> So the code in this patch [1] works as described - once the frame is complete
it
> is OK to reuse uniqueID and use cached content from SIDC/skia cache referenced
> by uniqueID.
> 
> [1]
> m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder &&
> m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() :
> DecodingImageGenerator::NeedNewImageUniqueID;
> 
> 
> See also:
> 
> ImageDecoder.h:
> // Whether or not the frame is fully received.
> virtual bool frameIsCompleteAtIndex(size_t) const;
> 
> GIFImageDecoder (true returned when frame data successfully parsed, not
decoded)
> bool GIFImageDecoder::frameIsCompleteAtIndex(size_t index) const
> {
>     return m_reader && (index < m_reader->imagesCount()) &&
> m_reader->frameContext(index)->isComplete();
> }

Thanks, yes, I forgot to look at the GIF decoder override :-/

So yes, looking at m_actualDecoder->frameIsCompleteAtIndex() makes sense here.
Now that I understand it better, I think you actually *do* want to use
m_isComplete (which caches m_actualDecoder->frameIsCompleteAtIndex()). If
looking at m_allDataReceived helps (and you indicated that it does), then use
it, too, although I would expect that if all data had been received we would
have also parsed the frame. So that might be worth looking into.

aleksandar.stojiljkovic

On 2016/05/03 13:08:52, scroggo_chromium wrote: > Peter, go ahead. I have a small concern (stated ...

4 years, 7 months ago (2016-05-03 22:38:56 UTC) #22

On 2016/05/03 13:08:52, scroggo_chromium wrote:
> Peter, go ahead. I have a small concern (stated in my comment, below), but I
now
> understand why using frameIsCompleteAtIndex makes sense.
> 
>
https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
> (right):
> 
>
https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
> m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder &&
> m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() :
> DecodingImageGenerator::NeedNewImageUniqueID;
> > Found the reason for misunderstanding - frameIsCompleteAtIndex is returning
> true
> > when framedata is fully received, not when it is fully decoded. 
> > So the code in this patch [1] works as described - once the frame is
complete
> it
> > is OK to reuse uniqueID and use cached content from SIDC/skia cache
referenced
> > by uniqueID.
> > 
> > [1]
> > m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder &&
> > m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() :
> > DecodingImageGenerator::NeedNewImageUniqueID;
> > 
> > 
> > See also:
> > 
> > ImageDecoder.h:
> > // Whether or not the frame is fully received.
> > virtual bool frameIsCompleteAtIndex(size_t) const;
> > 
> > GIFImageDecoder (true returned when frame data successfully parsed, not
> decoded)
> > bool GIFImageDecoder::frameIsCompleteAtIndex(size_t index) const
> > {
> >     return m_reader && (index < m_reader->imagesCount()) &&
> > m_reader->frameContext(index)->isComplete();
> > }
> 
> Thanks, yes, I forgot to look at the GIF decoder override :-/
> 
> So yes, looking at m_actualDecoder->frameIsCompleteAtIndex() makes sense here.
> Now that I understand it better, I think you actually *do* want to use
> m_isComplete (which caches m_actualDecoder->frameIsCompleteAtIndex()). If
> looking at m_allDataReceived helps (and you indicated that it does), then use
> it, too, although I would expect that if all data had been received we would
> have also parsed the frame. So that might be worth looking into.

Thanks. Good that we clarified it. I doublechecked it, too.

>Peter, go ahead. I have a small concern (stated in my comment, below),
No problem, verified that the value is the same as cached value here, so
reverted (m_actualDecoder && m_actualDecoder->frameIsCompleteAtIndex()) to
m_frameData[index].isComplete in patchset #4.

Peter Kasting

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp#newcode72 third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:72: , m_frameIndex(0) Here and below: Initialize members in the ...

4 years, 7 months ago (2016-05-04 03:01:07 UTC) #23

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:72: ,
m_frameIndex(0)
Here and below: Initialize members in the same order they were declared.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:144: RefPtr<SkImage>
image = m_source.createFrameAtIndex(index);
Note that Skia is currently switching to sk_sp and I don't know whether
RefPtr<SkImage> and the like are still desirable in that world.  Make sure you
understand whether this is how the code is supposed to be going forward before
adding this.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:221:
m_frame.clear();
I think this would be more clearly associated with the loop above if you instead
changed that loop body to:

    if (m_frames[i].m_haveMetadata && !m_frames[i].m_isComplete) {
        m_frames[i].clear(true);
        if (i == m_frameIndex)
            m_frame.clear();
    }

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:338: return
cacheFrame(index);
It seems like this is a long-winded way of writing:

    if (index == m_frameIndex && m_frame) {
        RefPtr<SkImage> image = m_frame;
        return image.release();
    }

    return cacheFrame(index);

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:553:
m_animationFinished = false;
Should we clear m_frame here?

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:77: void
stopAnimation();
It's confusing that we mix overrides and non-overrides together.  Can we move
them so all the overrides are above or below all the non-overrides?

Hopefully this will make it easier to tell in the future if any of these can be
removed as well (e.g. do the three non-overrides above this all need to exist
still?).

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:94: // some room in
the image cache.
Seems like this comment should be on the base class declaration of this function
and not here.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:114: static void
setAnimationCacheSizeForTesting(size_t maxCacheSize, size_t
maxAnimationSizeInCache);
Please add descriptive comments on new functions you add.  (A lot of Blink
inherits WebKit's policy of not documenting functions, but let's not extend that
to any new code.)

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:129: // Decodes and
caches a frame. Never accessed except internally.
While here: Why is "Never accessed except internally" needed when this is in the
private section?

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:162: RefPtr<SkImage>
m_frame; // One frame is cached (the most recently accessed) also for
animations.
"A cached copy of the most recently-accessed frame"?  I don't know what "also
for animations" means.

I would name this and the next member like m_cachedFrame or something.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:51: // Make
kNeedNeedNewImageUniqueID accessible.
Extra "need".  Also, you should qualify this with SkImageGenerator:: to be clear
to readers.

But frankly, I think better than this would be to make the value public on
SkImageGenerator.  If callers of this class are expected to use unique IDs which
are actually the ones used by SkImageGenerator it seems reasonable to expose
this magic value publicly there.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:53:
NeedNewImageUniqueID = kNeedNewImageUniqueID
This should still be named with a leading k.  Use a qualifier to disambiguate
the two?

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder &&
m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() :
DecodingImageGenerator::NeedNewImageUniqueID;
On 2016/05/03 13:08:52, scroggo_chromium wrote:
> If
> looking at m_allDataReceived helps (and you indicated that it does), then use
> it, too, although I would expect that if all data had been received we would
> have also parsed the frame. So that might be worth looking into.

I also would think that if m_allDataReceived, then either the frame should be
complete at this index, or else it's wrong to access uniqueID().  So I think
that condition check ought not to be needed?

Also, shouldn't m_uniqueID already be kNeedNewImageUniqueID unless the frame was
already marked complete?  So it seems like this could be rewritten more like:

    if (m_actualDecoder && m_actualDecoder->frameIsCompleteAtIndex(index))
        m_frameData[index].m_uniqueID = image->uniqueID();

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.h:104: //
Carries only frame state and other information. Does not carry bitmap.
This comment seems superfluous given that it's no longer talking about a set of
objects that have a bitmap field.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/FrameData.cpp (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/FrameData.cpp:49: m_haveMetadata =
false;
I wonder if m_haveMetadata buys much anymore.  Seems like it used to help
distinguish between when we'd decoded but thrown away the frame bitmap, and when
we hadn't decoded.  But now since we never have a frame bitmap, it seems like
this field could just disappear, and users of this object could just have a
FrameData object or not...

Though I guess that would mean a Vector<FrameData> would have to become a
Vector<std::unique_ptr<FrameData>>, which is probably worse.  So maybe this
should stay but just become m_initialized or something?

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/FrameData.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/FrameData.h:54: bool m_hasAlpha : 1;
While here: I'm slightly skeptical the memory savings from packing the bits
matter.  Do we really have so many FrameDatas sitting around at one time that
the bytes we save here add up to anything?  I would think the perf cost of
setting/reading individual bits would be a bigger deal than the memory savings
(though probably those are miniscule as well).

aleksandar.stojiljkovic

Patch Set 5 : review #23 fixes Thanks @pkasting https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp#newcode144 third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:144: ...

4 years, 7 months ago (2016-05-04 20:56:30 UTC) #24

Patch Set 5 : review #23 fixes
Thanks @pkasting

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:144: RefPtr<SkImage>
image = m_source.createFrameAtIndex(index);
On 2016/05/04 03:01:06, Peter Kasting wrote:
> Note that Skia is currently switching to sk_sp and I don't know whether
> RefPtr<SkImage> and the like are still desirable in that world.  Make sure you
> understand whether this is how the code is supposed to be going forward before
> adding this.

I found several patches related to switch to sk_sp on other places in blink,
none of them yet in FrameData and BitmapImage or tests using this.
Change would be significant and make this review more laborious - I prefer to do
refactoring sk_sp -> RefPtr in follow up patch.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:221:
m_frame.clear();
On 2016/05/04 03:01:06, Peter Kasting wrote:
> I think this would be more clearly associated with the loop above if you
instead
> changed that loop body to:
> 
>     if (m_frames[i].m_haveMetadata && !m_frames[i].m_isComplete) {
>         m_frames[i].clear(true);
>         if (i == m_frameIndex)
>             m_frame.clear();
>     }

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:338: return
cacheFrame(index);
On 2016/05/04 03:01:06, Peter Kasting wrote:
> It seems like this is a long-winded way of writing:
> 
>     if (index == m_frameIndex && m_frame) {
>         RefPtr<SkImage> image = m_frame;
>         return image.release();
>     }
> 
>     return cacheFrame(index);

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:553:
m_animationFinished = false;
On 2016/05/04 03:01:06, Peter Kasting wrote:
> Should we clear m_frame here?

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:77: void
stopAnimation();
On 2016/05/04 03:01:07, Peter Kasting wrote:
> It's confusing that we mix overrides and non-overrides together.  Can we move
> them so all the overrides are above or below all the non-overrides?
> 
> Hopefully this will make it easier to tell in the future if any of these can
be
> removed as well (e.g. do the three non-overrides above this all need to exist
> still?).

Done.
They are all still used (for testing).

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:94: // some room in
the image cache.
On 2016/05/04 03:01:06, Peter Kasting wrote:
> Seems like this comment should be on the base class declaration of this
function
> and not here.

Descriptio applies to this class functionality (framebuffer cache) - another
subclass (SVGImage) has it's own.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:114: static void
setAnimationCacheSizeForTesting(size_t maxCacheSize, size_t
maxAnimationSizeInCache);
On 2016/05/04 03:01:06, Peter Kasting wrote:
> Please add descriptive comments on new functions you add.  (A lot of Blink
> inherits WebKit's policy of not documenting functions, but let's not extend
that
> to any new code.)

Done.
This one is removed.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:129: // Decodes and
caches a frame. Never accessed except internally.
On 2016/05/04 03:01:06, Peter Kasting wrote:
> While here: Why is "Never accessed except internally" needed when this is in
the
> private section?

Done. Removed.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:162: RefPtr<SkImage>
m_frame; // One frame is cached (the most recently accessed) also for
animations.
On 2016/05/04 03:01:07, Peter Kasting wrote:
> "A cached copy of the most recently-accessed frame"?  I don't know what "also
> for animations" means.
> 
> I would name this and the next member like m_cachedFrame or something.

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:51: // Make
kNeedNeedNewImageUniqueID accessible.
On 2016/05/04 03:01:07, Peter Kasting wrote:
> Extra "need".  Also, you should qualify this with SkImageGenerator:: to be
clear
> to readers.
> 
> But frankly, I think better than this would be to make the value public on
> SkImageGenerator.  If callers of this class are expected to use unique IDs
which
> are actually the ones used by SkImageGenerator it seems reasonable to expose
> this magic value publicly there.

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:53:
NeedNewImageUniqueID = kNeedNewImageUniqueID
On 2016/05/04 03:01:07, Peter Kasting wrote:
> This should still be named with a leading k.  Use a qualifier to disambiguate
> the two?

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder &&
m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() :
DecodingImageGenerator::NeedNewImageUniqueID;
On 2016/05/04 03:01:07, Peter Kasting wrote:
> On 2016/05/03 13:08:52, scroggo_chromium wrote:
> > If
> > looking at m_allDataReceived helps (and you indicated that it does), then
use
> > it, too, although I would expect that if all data had been received we would
> > have also parsed the frame. So that might be worth looking into.
> 
> I also would think that if m_allDataReceived, then either the frame should be
> complete at this index, or else it's wrong to access uniqueID().  So I think
> that condition check ought not to be needed?
> 
> Also, shouldn't m_uniqueID already be kNeedNewImageUniqueID unless the frame
was
> already marked complete?  So it seems like this could be rewritten more like:
> 
>     if (m_actualDecoder && m_actualDecoder->frameIsCompleteAtIndex(index))
>         m_frameData[index].m_uniqueID = image->uniqueID();
 
It is important to have this check:
if (m_allDataReceived || m_frameData[index].m_isComplete)
m_isComplete has diffefrent meaning for gif animations or e.g. one frame JPEG.
reusing uniqueid is important there with jpegs too - when m_allDataReceived is
received but m_isComplete is false. verified this claim with debugger.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.h:104: //
Carries only frame state and other information. Does not carry bitmap.
On 2016/05/04 03:01:07, Peter Kasting wrote:
> This comment seems superfluous given that it's no longer talking about a set
of
> objects that have a bitmap field.

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/FrameData.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/FrameData.h:54: bool m_hasAlpha : 1;
On 2016/05/04 03:01:07, Peter Kasting wrote:
> While here: I'm slightly skeptical the memory savings from packing the bits
> matter.  Do we really have so many FrameDatas sitting around at one time that
> the bytes we save here add up to anything?  I would think the perf cost of
> setting/reading individual bits would be a bigger deal than the memory savings
> (though probably those are miniscule as well).

Often there are GIfs with 100+ frames, ig having e.g. 10 of those per page -
8KB. I've seen patches adding this on purpose to other files, too.

scroggo_chromium

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode315 third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder && m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() ...

4 years, 7 months ago (2016-05-04 21:15:55 UTC) #25

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315:
m_frameData[index].m_uniqueID = (m_allDataReceived || (m_actualDecoder &&
m_actualDecoder->frameIsCompleteAtIndex(index))) ? image->uniqueID() :
DecodingImageGenerator::NeedNewImageUniqueID;
On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote:
> On 2016/05/04 03:01:07, Peter Kasting wrote:
> > On 2016/05/03 13:08:52, scroggo_chromium wrote:
> > > If
> > > looking at m_allDataReceived helps (and you indicated that it does), then
> use
> > > it, too, although I would expect that if all data had been received we
would
> > > have also parsed the frame. So that might be worth looking into.
> > 
> > I also would think that if m_allDataReceived, then either the frame should
be
> > complete at this index, or else it's wrong to access uniqueID().  So I think
> > that condition check ought not to be needed?
> > 
> > Also, shouldn't m_uniqueID already be kNeedNewImageUniqueID unless the frame
> was
> > already marked complete?  So it seems like this could be rewritten more
like:
> > 
> >     if (m_actualDecoder && m_actualDecoder->frameIsCompleteAtIndex(index))
> >         m_frameData[index].m_uniqueID = image->uniqueID();
>  
> It is important to have this check:
> if (m_allDataReceived || m_frameData[index].m_isComplete)
> m_isComplete has diffefrent meaning for gif animations or e.g. one frame JPEG.
> reusing uniqueid is important there with jpegs too - when m_allDataReceived is
> received but m_isComplete is false. verified this claim with debugger.

Ah, of course! I find it very confusing that frameIsCompleteAtIndex() returns a
different conceptual meaning depending on whether we're talking about a
multi-frame image or a single frame image. Maybe we really want a method called
frameHasAllData (that's a terrible name, but I think it almost captures my idea)
which would return m_allDataReceived in single frame images and whether the
frame has been parsed in GIF (and something analogous in WEBP). That should be a
separate change, if we go that route.

(We really don't care here whether the image is "Complete" - what we want to
know is - might it change later? Even if the stream is truncated, we want to
keep reusing the same ID once we've processed the entire stream.)

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:219:
m_cachedFrame.clear();
Should we set m_cachedFrameIndex to an invalid value?

aleksandar.stojiljkovic

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp#newcode219 third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:219: m_cachedFrame.clear(); On 2016/05/04 21:15:54, scroggo_chromium wrote: > Should we ...

4 years, 7 months ago (2016-05-04 21:28:07 UTC) #26

scroggo_chromium

Changes in DeferredImageDecoder/DecodingImageGenerator LGTM

4 years, 7 months ago (2016-05-06 14:08:45 UTC) #27

Peter Kasting

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp#newcode553 third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:553: m_animationFinished = false; On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote: > ...

4 years, 7 months ago (2016-05-07 01:58:29 UTC) #28

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:553:
m_animationFinished = false;
On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote:
> On 2016/05/04 03:01:06, Peter Kasting wrote:
> > Should we clear m_frame here?
> 
> Done.

For clarity, I wasn't very sure whether we should or not.  Perhaps keeping
m_cachedFrame would save time once we start animating again?  Though if we don't
start again, perhaps it's a waste of memory unless it's a cache of the first
frame?  So should we clear it iff it is not the first frame?

In any case I'd add a comment on why we should clear this (and, if necessary,
when).

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:77: void
stopAnimation();
On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote:
> On 2016/05/04 03:01:07, Peter Kasting wrote:
> > It's confusing that we mix overrides and non-overrides together.  Can we
move
> > them so all the overrides are above or below all the non-overrides?
> > 
> > Hopefully this will make it easier to tell in the future if any of these can
> be
> > removed as well (e.g. do the three non-overrides above this all need to
exist
> > still?).
> 
> Done.
> They are all still used (for testing).

Still need to move sizeRespectingOrientation(), currentFrameOrientation(), and
createWithOrientationForTesting() out of the overrides.

In the private section, draw() and startAnimation() are overrides mixed in with
everything else.

Also, you should move the implementation in the .cpp file around at the same
time you move the .h file contents, to keep them in the same order.

Feel free to do all this mechnical reordering as a separate patch to avoid
confusion.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:51: // Make
kNeedNeedNewImageUniqueID accessible.
On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote:
> On 2016/05/04 03:01:07, Peter Kasting wrote:
> > Extra "need".  Also, you should qualify this with SkImageGenerator:: to be
> clear
> > to readers.
> > 
> > But frankly, I think better than this would be to make the value public on
> > SkImageGenerator.  If callers of this class are expected to use unique IDs
> which
> > are actually the ones used by SkImageGenerator it seems reasonable to expose
> > this magic value publicly there.
> 
> Done.

Any comment on the idea of exposing SkImageGenerator::kNeedNewImageUniqueID
publicly directly?

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/FrameData.cpp (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/FrameData.cpp:49: m_haveMetadata =
false;
On 2016/05/04 03:01:07, Peter Kasting wrote:
> I wonder if m_haveMetadata buys much anymore.  Seems like it used to help
> distinguish between when we'd decoded but thrown away the frame bitmap, and
when
> we hadn't decoded.  But now since we never have a frame bitmap, it seems like
> this field could just disappear, and users of this object could just have a
> FrameData object or not...
> 
> Though I guess that would mean a Vector<FrameData> would have to become a
> Vector<std::unique_ptr<FrameData>>, which is probably worse.  So maybe this
> should stay but just become m_initialized or something?

Did you have any thoughts on all this?

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:332: return
m_cachedFrame;
Does this implicitly ref m_cachedFrame and then pass back the new ref?  (I want
to make sure we don't pass back the only ref, and I'm not very familiar with the
Blink smart pointer types.)

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:139: // It may look
unusual that there's no start animation call as public API.
This sentence (maybe this whole comment?) seems misplaced now that this is
non-public, and there is a startAnimation() call that's private.

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: if
(m_allDataReceived || m_frameData[index].m_isComplete)
Apparently Leon understands how these differ, but I don't.  While you mentioned
that m_isComplete has different meanings on different image types, I don't
recall what the differences are, and I wonder that such differences are really
desirable.  Can we get to a world where we need only check m_isComplete here? 
If not this at least needs a comment explaining to the reader why this needs to
be as it is.

aleksandar.stojiljkovic

patch set #6: Comments. Thanks @pkasting. https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp#newcode72 third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:72: , m_frameIndex(0) On ...

4 years, 7 months ago (2016-05-07 19:50:53 UTC) #29

patch set #6: Comments. Thanks @pkasting.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:72: ,
m_frameIndex(0)
On 2016/05/04 03:01:06, Peter Kasting wrote:
> Here and below: Initialize members in the same order they were declared.

Done.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:553:
m_animationFinished = false;
On 2016/05/07 01:58:29, Peter Kasting wrote:
> On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote:
> > On 2016/05/04 03:01:06, Peter Kasting wrote:
> > > Should we clear m_frame here?
> > 
> > Done.
> 
> For clarity, I wasn't very sure whether we should or not.  Perhaps keeping
> m_cachedFrame would save time once we start animating again?  Though if we
don't
> start again, perhaps it's a waste of memory unless it's a cache of the first
> frame?  So should we clear it iff it is not the first frame?
> 
> In any case I'd add a comment on why we should clear this (and, if necessary,
> when).

Sorry, the way I handled this clear() was obscure - original code cleared frame
reference from BitmapImage by calling destroyDecodedDataIfNecessary().
It is correct to clear it since this code is called e.g. when all ImageResource
clients get removed to cleanup. 
This is only clearing BitmapImage reference while copy of current frame is still
kept in decoder (decoder is disposed on ~BitmapImage ...
~ImageFrameGenerator()).
Thanks.

[1]https://gist.github.com/astojilj/fbecf7a901a46d774eb9c0253a18eb4c

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:77: void
stopAnimation();
On 2016/05/07 01:58:29, Peter Kasting wrote:
> On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote:
> > On 2016/05/04 03:01:07, Peter Kasting wrote:
> > > It's confusing that we mix overrides and non-overrides together.  Can we
> move
> > > them so all the overrides are above or below all the non-overrides?
> > > 
> > > Hopefully this will make it easier to tell in the future if any of these
can
> > be
> > > removed as well (e.g. do the three non-overrides above this all need to
> exist
> > > still?).
> > 
> > Done.
> > They are all still used (for testing).
> 
> Still need to move sizeRespectingOrientation(), currentFrameOrientation(), and
> createWithOrientationForTesting() out of the overrides.
> 
> In the private section, draw() and startAnimation() are overrides mixed in
with
> everything else.
> 
> Also, you should move the implementation in the .cpp file around at the same
> time you move the .h file contents, to keep them in the same order.
> 
> Feel free to do all this mechnical reordering as a separate patch to avoid
> confusion.

Thanks, makes sense. I would like to do this together with planned refactoring
RefPtr -> sk_sp.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h
(right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:51: // Make
kNeedNeedNewImageUniqueID accessible.
On 2016/05/07 01:58:29, Peter Kasting wrote:
> On 2016/05/04 20:56:29, aleksandar.stojiljkovic wrote:
> > On 2016/05/04 03:01:07, Peter Kasting wrote:
> > > Extra "need".  Also, you should qualify this with SkImageGenerator:: to be
> > clear
> > > to readers.
> > > 
> > > But frankly, I think better than this would be to make the value public on
> > > SkImageGenerator.  If callers of this class are expected to use unique IDs
> > which
> > > are actually the ones used by SkImageGenerator it seems reasonable to
expose
> > > this magic value publicly there.
> > 
> > Done.
> 
> Any comment on the idea of exposing SkImageGenerator::kNeedNewImageUniqueID
> publicly directly?

I think SkImageGenerator should stay as it is now -
SkImageGenerator::kNeedNewImageUniqueID is used only in protected method
(constructor is protected) and this const parameter used only in constructor
should be protected, too. It is client choice here to use it outside protected
class code so client needs to deal with this.

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/FrameData.cpp (right):

https://codereview.chromium.org/1925533003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/FrameData.cpp:49: m_haveMetadata =
false;
On 2016/05/07 01:58:29, Peter Kasting wrote:
> On 2016/05/04 03:01:07, Peter Kasting wrote:
> > I wonder if m_haveMetadata buys much anymore.  Seems like it used to help
> > distinguish between when we'd decoded but thrown away the frame bitmap, and
> when
> > we hadn't decoded.  But now since we never have a frame bitmap, it seems
like
> > this field could just disappear, and users of this object could just have a
> > FrameData object or not...
> > 
> > Though I guess that would mean a Vector<FrameData> would have to become a
> > Vector<std::unique_ptr<FrameData>>, which is probably worse.  So maybe this
> > should stay but just become m_initialized or something?
> 
> Did you have any thoughts on all this?

I agree that current m_haveMetadata is better than
Vector<std::unique_ptr<FrameData>>:
- it doesn't require additional space (as with other bit flags here occupies
total one sizeof(size_t).
- don't know if there is a way to have placement new with unique_ptr but don't
like redirecting to frame data allocated in non adjacent locations on the heap
when enumerating through all frames. Don't want to oversell this either - first
argument with "no space required" looks strong enough.

About m_haveMetadata vs m_initialized - don't have strong opinion.
m_haveMetadata makes more sense now then before since now we have no
RefPtr<SkImage> in FrameData but only m_uniqueID which is metadata (and
RefPtr<SkImage> m_frame wasn't).
In following patches I would suggest to remove all of this and getting the data
from the source.

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:332: return
m_cachedFrame;
On 2016/05/07 01:58:29, Peter Kasting wrote:
> Does this implicitly ref m_cachedFrame and then pass back the new ref?  (I
want
> to make sure we don't pass back the only ref, and I'm not very familiar with
the
> Blink smart pointer types.)

Yes, see PasRefPtr.h.
 template <typename U> PassRefPtr(const RefPtr<U>&,
EnsurePtrConvertibleArgDecl(U, T));
It is cheaper than to create RefPtr and call .release but basically the same.
Anyway, this all should go away soon.

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:139: // It may look
unusual that there's no start animation call as public API.
On 2016/05/07 01:58:29, Peter Kasting wrote:
> This sentence (maybe this whole comment?) seems misplaced now that this is
> non-public, and there is a startAnimation() call that's private.
Rephrased the comment.

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: if
(m_allDataReceived || m_frameData[index].m_isComplete)
On 2016/05/07 01:58:29, Peter Kasting wrote:
> Apparently Leon understands how these differ, but I don't.  While you
mentioned
> that m_isComplete has different meanings on different image types, I don't
> recall what the differences are, and I wonder that such differences are really
> desirable.  Can we get to a world where we need only check m_isComplete here? 
> If not this at least needs a comment explaining to the reader why this needs
to
> be as it is.

It comes from this discussion with @hclam [1] and intention to have this API
used only for multiframe images.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...

It is not used for multiframe images only and parsing if frame is complete is
done for GIF and there is different implementation for Webp. Don't know how to
handle ICO.
Anyway, I put a comment explaining why this is needed and suggest to work in
separate patch on making ImageDecoder::frameIsCompleteAtIndex to return
m_isAllDataReceived instead of m_frameBufferCache[index].status() ==
ImageFrame::FrameComplete.
If I do it in this patch I feel it is additional risk and think it is better to
scope that to dedicated patch later or before this one, whatever you suggest.

aleksandar.stojiljkovic

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode315 third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: if (m_allDataReceived || m_frameData[index].m_isComplete) On 2016/05/07 19:50:53, aleksandar.stojiljkovic wrote: ...

4 years, 7 months ago (2016-05-09 09:33:03 UTC) #30

Peter Kasting

LGTM https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode315 third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: if (m_allDataReceived || m_frameData[index].m_isComplete) On 2016/05/09 09:33:02, aleksandar.stojiljkovic ...

4 years, 7 months ago (2016-05-09 22:31:25 UTC) #31

LGTM

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: if
(m_allDataReceived || m_frameData[index].m_isComplete)
On 2016/05/09 09:33:02, aleksandar.stojiljkovic wrote:
> On 2016/05/07 19:50:53, aleksandar.stojiljkovic wrote:
> > On 2016/05/07 01:58:29, Peter Kasting wrote:
> > > Apparently Leon understands how these differ, but I don't.  While you
> > mentioned
> > > that m_isComplete has different meanings on different image types, I don't
> > > recall what the differences are, and I wonder that such differences are
> really
> > > desirable.  Can we get to a world where we need only check m_isComplete
> here? 
> > > If not this at least needs a comment explaining to the reader why this
needs
> > to
> > > be as it is.
> > 
> > It comes from this discussion with @hclam [1] and intention to have this API
> > used only for multiframe images.
> > 
> >
>
https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
> > 
> > It is not used for multiframe images only and parsing if frame is complete
is
> > done for GIF and there is different implementation for Webp. Don't know how
to
> > handle ICO.
> > Anyway, I put a comment explaining why this is needed and suggest to work in
> > separate patch on making ImageDecoder::frameIsCompleteAtIndex to return
> > m_isAllDataReceived instead of m_frameBufferCache[index].status() ==
> > ImageFrame::FrameComplete.
> > If I do it in this patch I feel it is additional risk and think it is better
> to
> > scope that to dedicated patch later or before this one, whatever you
suggest.
> > 
> 
> Uploaded the patch (that is fixing frameIsCompleteAtIndex) for review:
> https://codereview.chromium.org/1962563002/

So once that lands this can be changed to just call that instead?  If so, I
would probably go ahead and do that now, remove the comment (not needed once the
code is just calling that function), and wait to land until that other CL lands.

https://codereview.chromium.org/1925533003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:88: bool
isAllDataReceived() const { return m_allDataReceived; }
Nit: When you move the declarations of functions, you need to reorder their
definitions in the .cpp file to match in the same CL.  If you don't want to do
that in this CL it's fine to do separately, but in that case don't move the
declarations in the .h file around either.

https://codereview.chromium.org/1925533003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:138: // There's no
start animation call because we start and stop animating
Eh?  startAnimation() is just a couple lines above this!

I would remove this comment entirely, along with the blank line above it; or
else maybe move the last sentence up to after "// Animation." above.

aleksandar.stojiljkovic

On 2016/05/09 22:31:25, Peter Kasting wrote: > LGTM > > https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp > File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp > ...

4 years, 7 months ago (2016-05-11 10:33:14 UTC) #32

On 2016/05/09 22:31:25, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
> (right):
> 
>
https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:315: if
> (m_allDataReceived || m_frameData[index].m_isComplete)
> On 2016/05/09 09:33:02, aleksandar.stojiljkovic wrote:
> > On 2016/05/07 19:50:53, aleksandar.stojiljkovic wrote:
> > > On 2016/05/07 01:58:29, Peter Kasting wrote:
> > > > Apparently Leon understands how these differ, but I don't.  While you
> > > mentioned
> > > > that m_isComplete has different meanings on different image types, I
don't
> > > > recall what the differences are, and I wonder that such differences are
> > really
> > > > desirable.  Can we get to a world where we need only check m_isComplete
> > here? 
> > > > If not this at least needs a comment explaining to the reader why this
> needs
> > > to
> > > > be as it is.
> > > 
> > > It comes from this discussion with @hclam [1] and intention to have this
API
> > > used only for multiframe images.
> > > 
> > >
> >
>
https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
> > > 
> > > It is not used for multiframe images only and parsing if frame is complete
> is
> > > done for GIF and there is different implementation for Webp. Don't know
how
> to
> > > handle ICO.
> > > Anyway, I put a comment explaining why this is needed and suggest to work
in
> > > separate patch on making ImageDecoder::frameIsCompleteAtIndex to return
> > > m_isAllDataReceived instead of m_frameBufferCache[index].status() ==
> > > ImageFrame::FrameComplete.
> > > If I do it in this patch I feel it is additional risk and think it is
better
> > to
> > > scope that to dedicated patch later or before this one, whatever you
> suggest.
> > > 
> > 
> > Uploaded the patch (that is fixing frameIsCompleteAtIndex) for review:
> > https://codereview.chromium.org/1962563002/
> 
> So once that lands this can be changed to just call that instead?  If so, I
> would probably go ahead and do that now, remove the comment (not needed once
the
> code is just calling that function), and wait to land until that other CL
lands.
Don't know yet if that would be the only change after second one lands, but that
is the plan..

Prefer not to make the patches dependent - I'm testing them separately and which
ever gets ready first I would change this part when submitting the second.

https://codereview.chromium.org/1925533003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/1925533003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:88: bool
isAllDataReceived() const { return m_allDataReceived; }
On 2016/05/09 22:31:25, Peter Kasting wrote:
> Nit: When you move the declarations of functions, you need to reorder their
> definitions in the .cpp file to match in the same CL.  If you don't want to do
> that in this CL it's fine to do separately, but in that case don't move the
> declarations in the .h file around either.

Difficult as there is already missmatch in order of methods in cph and header
file. I merged 2 private sections, kept the order in cpp file and reordered
change methods in header file. Full reordering is good to do after refactoring -
the plan is to remove FrameData usage in BitmapImage.

https://codereview.chromium.org/1925533003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:138: // There's no
start animation call because we start and stop animating
On 2016/05/09 22:31:25, Peter Kasting wrote:
> Eh?  startAnimation() is just a couple lines above this!
> 
> I would remove this comment entirely, along with the blank line above it; or
> else maybe move the last sentence up to after "// Animation." above.

Done.

aleksandar.stojiljkovic

On 2016/05/11 18:39:45, Peter Kasting wrote: > Still LGTM @fmalita, @senorblanco - could you please ...

4 years, 7 months ago (2016-05-11 21:57:27 UTC) #34

aleksandar.stojiljkovic

aleksandar.stojiljkovic@intel.com changed reviewers: + chrishtr@chromium.org, schenney@chromium.org

4 years, 7 months ago (2016-05-19 14:47:07 UTC) #35

aleksandar.stojiljkovic

senorblanco@ and fmalita@ seem busy. Checking if other OWNERS from platform/graphics are available. chrishtr@, schenney@, ...

4 years, 7 months ago (2016-05-19 14:47:09 UTC) #36

f(malita)

On 2016/05/19 14:47:09, aleksandar.stojiljkovic wrote: > senorblanco@ and fmalita@ seem busy. > Checking if other ...

4 years, 7 months ago (2016-05-24 15:31:08 UTC) #37

f(malita)

https://codereview.chromium.org/1925533003/diff/140001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/1925533003/diff/140001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp#newcode319 third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:319: m_frameData[index].m_uniqueID = image->uniqueID(); Nit: before assigning the new ID ...

4 years, 7 months ago (2016-05-24 15:31:21 UTC) #38

aleksandar.stojiljkovic

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

4 years, 7 months ago (2016-05-25 09:37:39 UTC) #39

aleksandar.stojiljkovic

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org, pkasting@chromium.org, fmalita@chromium.org Link ...

4 years, 7 months ago (2016-05-25 09:37:40 UTC) #40

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925533003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925533003/160001

4 years, 7 months ago (2016-05-25 09:38:01 UTC) #41

aleksandar.stojiljkovic

Description was changed from ========== High CPU and increased memory usage fix for high-res (GIF, ...

4 years, 7 months ago (2016-05-25 09:47:35 UTC) #42

Description was changed from

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with this page
is
now about 3 times lower.
    
Memory consumption (Windows "process working set" [4]) with page [3]:
    Original  : ~730MB
    This patch: ~250MB

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
    Original  : ~500MB
    This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

to

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with page is
now about 3 times lower*.

*) Note:
After rebasing to the latest, memory consumption with page [3] is the same
as in version without the patch (measured on Windows).
Meanwhile, SIDC budget got increased and now I verified that this
patch makes SIDC cache used better - there is no redecoding of every frame
since they are cached in SIDC cache. After some time, depending on number
of GIFs in viewport, it happens that there is no decoding at all while
rendering - all the decoded bitmaps are fetched from SIDC cache.
Without this patch, this isn't a case - there is re-decoding.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
    Original  : ~500MB
    This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

commit-bot: I haz the power

Description was changed from ========== High CPU and increased memory usage fix for high-res (GIF, ...

4 years, 7 months ago (2016-05-25 11:26:25 UTC) #43

Message was sent while issue was closed.

Description was changed from

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with page is
now about 3 times lower*.

*) Note:
After rebasing to the latest, memory consumption with page [3] is the same
as in version without the patch (measured on Windows).
Meanwhile, SIDC budget got increased and now I verified that this
patch makes SIDC cache used better - there is no redecoding of every frame
since they are cached in SIDC cache. After some time, depending on number
of GIFs in viewport, it happens that there is no decoding at all while
rendering - all the decoded bitmaps are fetched from SIDC cache.
Without this patch, this isn't a case - there is re-decoding.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
    Original  : ~500MB
    This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

to

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with page is
now about 3 times lower*.

*) Note:
After rebasing to the latest, memory consumption with page [3] is the same
as in version without the patch (measured on Windows).
Meanwhile, SIDC budget got increased and now I verified that this
patch makes SIDC cache used better - there is no redecoding of every frame
since they are cached in SIDC cache. After some time, depending on number
of GIFs in viewport, it happens that there is no decoding at all while
rendering - all the decoded bitmaps are fetched from SIDC cache.
Without this patch, this isn't a case - there is re-decoding.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
    Original  : ~500MB
    This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

commit-bot: I haz the power

Description was changed from ========== High CPU and increased memory usage fix for high-res (GIF, ...

4 years, 7 months ago (2016-05-25 11:27:31 UTC) #45

Message was sent while issue was closed.

Description was changed from

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.


- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with page is
now about 3 times lower*.

*) Note:
After rebasing to the latest, memory consumption with page [3] is the same
as in version without the patch (measured on Windows).
Meanwhile, SIDC budget got increased and now I verified that this
patch makes SIDC cache used better - there is no redecoding of every frame
since they are cached in SIDC cache. After some time, depending on number
of GIFs in viewport, it happens that there is no decoding at all while
rendering - all the decoded bitmaps are fetched from SIDC cache.
Without this patch, this isn't a case - there is re-decoding.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
    Original  : ~500MB
    This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.
 
- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750
==========

to

==========
High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations.

Patch dependens on skia patch:
https://codereview.chromium.org/1928403002

Addressing issues identified with initial patch:
crrev.com/1857543002: Don't recreate SkImages for high-res (GIF, WEBP...)
animations.
by implementing this on different way.

- Out of memory fix
===================
Originally, high res GIF frames are re-created and re-decoded in BitmapImage
during playback , leading to re-creating SkImages and filling up
SoftwareImageDecoderController cache:
The problem caused here (re-decoding and increased memory allocation) is that
SkImage::uniqueID() is used as SoftwareImageDecodeController::decoded_images_
cache key - clearing it causes that cache is filled for every loop and every
frame rendered (always decoding and adding to cache). Next loop's SkImages
have different uniqueID and keep filling up the cache with duplicates of
same frames, evicting useful content. Detailed information about the issue
is in comment here [1]

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=165750#c13

Patch here mitigates the issue by using same uniqueId which makes the cache
utilized, whether it is cc SIDC or skia discardable memory cache.

@fmalita's concern from [2], [3] is addressed, too: memory used with page is
now about 3 times lower*.

*) Note:
After rebasing to the latest, memory consumption with page [3] is the same
as in version without the patch (measured on Windows).
Meanwhile, SIDC budget got increased and now I verified that this
patch makes SIDC cache used better - there is no redecoding of every frame
since they are cached in SIDC cache. After some time, depending on number
of GIFs in viewport, it happens that there is no decoding at all while
rendering - all the decoded bitmaps are fetched from SIDC cache.
Without this patch, this isn't a case - there is re-decoding.

Memory consumption (Windows "process working set" [4]) with bug 165750 is:
    Original  : ~500MB
    This patch: ~ 60MB

[2] https://chromiumcodereview.appspot.com/1857543002/#msg10
[3]
http://www.androidpolice.com/2014/10/20/animation-bonanza-android-5-0-lollipo...
[4] https://chromiumcodereview.appspot.com/1857543002/#msg13

- Avoid redecoding
==================
Now there is no re-decoding as there is no cache miss when accessing cached
decoded bitmap.

- destroyDecodedData(true) -> destroyDecodedData
================================================
There is no need to have destroyDecodedData(false) and it is removed.
Since original patch [5], pixel data caching got moved to skia discardable
memory and recently to SoftwareImageDecodeController.
As ImageDecoder already caches only one (2 in some cases) frame, this code's
effect was to remove SkImage for animations where all combined frames data is
over 5 MB.

[5] original patch, @pkasting, Dec 2008
https://chromium.googlesource.com/chromium/src/+/f868b0862a903e2a4ffa0fc97508...

- Remove holding SkImage reference for every frame in BitmapImage
=================================================================
No need to to it also for low size animations - if decoded bitmap is cached in
skia or SIDC then there would be no redecoding. One frame is cached per
BitmapImage (whether it is single or multiple frame) to speedup access.

- destroyDecodedDataIfNecessary replaced by always caching only one frame.
==========================================================================
This removes unnecessary cache layer in BitmapImage::m_frames[].m_frame - if
decoded bitmap is cached in skia or SIDC then there would be no redecoding.

- FrameData -> DeferredFrameData in DeferredImageDecoder
========================================================
DeferredImageDecoder doesn't use m_frame, m_haveMetadata and m_hasAlpha from
FrameData so slimmed down struct introduced with required fields only.

- virtual void stopAnimation() removed.
=======================================
Only called for BitmapImage class. There is no usage for this method in Image
and SVGImage so removed it from there.

- BitmapImage::destroyDecodedData() override private->public
============================================================
virtual Image::destroyDecodedData that this one is overriding is public
from the beginning [6]. instead of downcasting to Image (where the method is
public) and calling it made overrides public in subclasses too.
Purpose of it is unclear in original patch [6] where it replaced invalidateData.

[6] original patch, @hyatt, Mar 2007
https://chromium.googlesource.com/chromium/src/+/d9bbf3787106e2620e8f51f1d371...

BUG=165750

Committed: https://crrev.com/c823053729b5a8839ee32b23c3142f3cd2955bf9
Cr-Commit-Position: refs/heads/master@{#395863}
==========

commit-bot: I haz the power

4 years, 7 months ago (2016-05-25 11:27:34 UTC) #46

Message was sent while issue was closed.

Patchset 9 (id:??) landed as
https://crrev.com/c823053729b5a8839ee32b23c3142f3cd2955bf9
Cr-Commit-Position: refs/heads/master@{#395863}

Issue 1925533003: High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Beter approach (use same uniqueId for same SkImage) - thanks @pkasting. #

Patch Set 3 : @scroggo, thanks a lot - this makes page [3] to use 250MB (was 750MB) #

Patch Set 4 : minor - revert back to cached m_isComplete #

Patch Set 5 : review #23 fixes #

Patch Set 6 : Comments related to review #28 #

Patch Set 7 : BitmapImage::isAllDataReceived and hasColorProfile back to public #

Patch Set 8 : Nits related to review #31 #

Patch Set 9 : Rebasing. DCHECK. Thanks fmalita@. #

Messages