Move GIF decoder's aggressive purge into ImageDecoder

Move GIF decoder's aggressive purge into ImageDecoder

Other animated image formats should have the same aggressive purge
behavior.

So move it out of the GIF decoder and into ImageDecoder.

BUG=651666
Committed: https://crrev.com/ac1c98466364aa2bcdfedbfc8c0d1c73450aab44
Cr-Commit-Position: refs/heads/master@{#423691}

[cblume, 2016-09-30 02:24:09]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 02:24:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 02:35:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 02:35:21]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[cblume, 2016-09-30 17:12:01]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 17:12:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 18:41:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 18:41:41]

Dry run: This issue passed the CQ dry run.

[cblume, 2016-10-01 09:48:44]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 09:48:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cblume, 2016-10-01 10:47:21]

cblume@chromium.org changed reviewers:
+ pkasting@google.com, scroggo@google.com, skal@google.com

[cblume, 2016-10-01 10:47:21]

PTAL

[commit-bot: I haz the power, 2016-10-01 10:58:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 10:58:04]

Dry run: This issue passed the CQ dry run.

[urvang, 2016-10-03 19:18:05]

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

[urvang, 2016-10-03 19:18:06]

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:245: if
(m_purgeAggressively)
So, once we set m_purgeAggressively to true, we'll never set it to false. I
wonder if that is the behavior we want.
(Of course, this patch is just for moving code, so any change to behavior
belongs in a separate patch. But I'm just thinking aloud here.)

[cblume, 2016-10-03 19:39:19]

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:245: if
(m_purgeAggressively)
On 2016/10/03 19:18:06, urvang wrote:
> So, once we set m_purgeAggressively to true, we'll never set it to false. I
> wonder if that is the behavior we want.
> (Of course, this patch is just for moving code, so any change to behavior
> belongs in a separate patch. But I'm just thinking aloud here.)

I believe it is.

Ideally, this would be set once at the very beginning. But we don't have all the
data at the very beginning and cannot make the decision until we have enough
information.

But the point of it is: If we do not have space, we won't ever have space. We'll
never suddenly consume less of the memory budget. The image only ever grows
larger as more data comes in / we decode and find there are more frames.

[urvang, 2016-10-03 19:56:08]

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:245: if
(m_purgeAggressively)
On 2016/10/03 19:39:19, cblume wrote:
> On 2016/10/03 19:18:06, urvang wrote:
> > So, once we set m_purgeAggressively to true, we'll never set it to false. I
> > wonder if that is the behavior we want.
> > (Of course, this patch is just for moving code, so any change to behavior
> > belongs in a separate patch. But I'm just thinking aloud here.)
> 
> I believe it is.
> 
> Ideally, this would be set once at the very beginning. But we don't have all
the
> data at the very beginning and cannot make the decision until we have enough
> information.
> 
> But the point of it is: If we do not have space, we won't ever have space.
We'll
> never suddenly consume less of the memory budget. The image only ever grows
> larger as more data comes in / we decode and find there are more frames.

We call "if (m_purgeAggressively) clearCacheExceptFrame(*i);" at some point. So,
at that point, all except one frame will be cleared from memory. And memory
would be free again to cache some frames, right?

[cblume, 2016-10-03 20:05:35]

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2376033005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:245: if
(m_purgeAggressively)
On 2016/10/03 19:56:07, urvang wrote:
> On 2016/10/03 19:39:19, cblume wrote:
> > On 2016/10/03 19:18:06, urvang wrote:
> > > So, once we set m_purgeAggressively to true, we'll never set it to false.
I
> > > wonder if that is the behavior we want.
> > > (Of course, this patch is just for moving code, so any change to behavior
> > > belongs in a separate patch. But I'm just thinking aloud here.)
> > 
> > I believe it is.
> > 
> > Ideally, this would be set once at the very beginning. But we don't have all
> the
> > data at the very beginning and cannot make the decision until we have enough
> > information.
> > 
> > But the point of it is: If we do not have space, we won't ever have space.
> We'll
> > never suddenly consume less of the memory budget. The image only ever grows
> > larger as more data comes in / we decode and find there are more frames.
> 
> We call "if (m_purgeAggressively) clearCacheExceptFrame(*i);" at some point.
So,
> at that point, all except one frame will be cleared from memory. And memory
> would be free again to cache some frames, right?

You are correct. What I mean is we wouldn't have space for all of the frames.

If all of the frames fit within our memory limit, then we shouldn't purge them*.
But if they wouldn't fit, we need to purge things anyway.

So we keep the keyframe we will need for the next frame to decode. But we purge
the rest.


*Outside of the ImageDecoder, Blink/CC just purges after every frame anyway. So
even though we could keep it all in memory, Chrome doesn't. This is just a bad
design that we're fixing. The root problem is every image has its own budget.
They should be part of a shared image budget.

[cblume, 2016-10-03 21:21:07]

Whoa. A rebase just went horribly wrong. Please ignore that for now. I'll go fix
it.

[Peter Kasting, 2016-10-05 00:20:22]

Description was changed from

==========
Move GIF decoder's aggressive purge into ImageDecoder

Other animated image formats should have the same aggressive purge
behavior.

So move it out of the GIF decoder and into ImageDecoder.

BUG=651666
==========

to

==========
Move GIF decoder's aggressive purge into ImageDecoder

Other animated image formats should have the same aggressive purge
behavior.

So move it out of the GIF decoder and into ImageDecoder.

BUG=651666
==========

[Peter Kasting, 2016-10-05 00:20:22]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[cblume, 2016-10-06 19:04:30]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 19:04:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 20:50:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 20:50:18]

Dry run: This issue passed the CQ dry run.

[cblume, 2016-10-06 21:21:28]

PTAL

If I didn't answer questions from before well enough let me know. :)

[urvang, 2016-10-06 21:25:43]

lgtm

[cblume, 2016-10-06 21:28:45]

The CQ bit was checked by cblume@chromium.org

[commit-bot: I haz the power, 2016-10-06 21:29:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 21:36:25]

Description was changed from

==========
Move GIF decoder's aggressive purge into ImageDecoder

Other animated image formats should have the same aggressive purge
behavior.

So move it out of the GIF decoder and into ImageDecoder.

BUG=651666
==========

to

==========
Move GIF decoder's aggressive purge into ImageDecoder

Other animated image formats should have the same aggressive purge
behavior.

So move it out of the GIF decoder and into ImageDecoder.

BUG=651666
==========

[commit-bot: I haz the power, 2016-10-06 21:36:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-06 21:38:14]

Description was changed from

==========
Move GIF decoder's aggressive purge into ImageDecoder

Other animated image formats should have the same aggressive purge
behavior.

So move it out of the GIF decoder and into ImageDecoder.

BUG=651666
==========

to

==========
Move GIF decoder's aggressive purge into ImageDecoder

Other animated image formats should have the same aggressive purge
behavior.

So move it out of the GIF decoder and into ImageDecoder.

BUG=651666

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

[commit-bot: I haz the power, 2016-10-06 21:38:15]

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