Reduce jank from favicon downscaling.

Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG=644573
Committed: https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d
Cr-Commit-Position: refs/heads/master@{#417190}

[aelias_OOO_until_Jul13, 2016-09-05 05:02:22]

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

[commit-bot: I haz the power, 2016-09-05 05:02:36]

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-05 05:50:52]

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

[commit-bot: I haz the power, 2016-09-05 05:50:53]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-09-06 04:28:08]

aelias@chromium.org changed reviewers:
+ sky@chromium.org

[aelias_OOO_until_Jul13, 2016-09-06 04:28:15]

Hi sky@, PTAL.

[sky, 2016-09-06 20:19:39]

sky@chromium.org changed reviewers:
+ brettw@chromium.org

[sky, 2016-09-06 20:19:40]

I'm worried about the effects of changing to fast png encode of favicons on
desktop. Your motivation is to change android, but it effects desktop as well
where AFAIK the same concern is not as prevalent. Also, favicons aren't
necessarily small things. Doesn't android potentially show some of the larger
touch icons?
+brettw for thoughts on switching favicons to FastEncodeBGRASkBitmap.

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
components/favicon_base/favicon_util.h:39: favicon_base::FaviconRawBitmapResult
ShoehornFaviconToSize(
Showhorn? ResizeFaviconBitmapResult?

[aelias_OOO_until_Jul13, 2016-09-06 20:38:16]

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

[aelias_OOO_until_Jul13, 2016-09-06 20:38:37]

Description was changed from

==========
Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and makes the following optimizations:
1. The operation is now done on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)
2. Switched to the "Fast" PNG encoder, since on Android CPU time is more
important than saving space on these typically small/transient favicons.

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG=641517
==========

to

==========
Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG=641517
==========

[commit-bot: I haz the power, 2016-09-06 20:39:04]

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

[aelias_OOO_until_Jul13, 2016-09-06 20:46:43]

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
components/favicon_base/favicon_util.h:39: favicon_base::FaviconRawBitmapResult
ShoehornFaviconToSize(
On 2016/09/06 at 20:19:39, sky wrote:
> Showhorn? ResizeFaviconBitmapResult?

I wanted to use a pejorative word, so I renamed to ResizeFaviconResultSlow.

https://codereview.chromium.org/2313563002/diff/20001/components/favicon_base...
File components/favicon_base/favicon_util.cc (right):

https://codereview.chromium.org/2313563002/diff/20001/components/favicon_base...
components/favicon_base/favicon_util.cc:285: if
(!gfx::PNGCodec::EncodeBGRASkBitmap(resized_image.AsBitmap(), false,
OK, if the "Fast" here is the main point of debate, I removed it from PS#2 since
I only measured a ~20% CPU saving from that, and the main benefit of the patch
is the threading change.

I want to explain my underlying assumptions a bit though:
1. Android is more demanding on both CPU and RAM than desktop, so if the
tradeoff works for us it's probably the right one everywhere.
2. Doing all this decompression/recompression of PNGs seems like a bad practice
since the CPU cost is extravagant.  The image needs to be fully uncompressed for
display anyway so it's not obvious what is really achieved by the recompression
here.  I'm pretty sure there exist some deeper favicon caching policy
improvements we could be doing that would be more efficient overall, but for now
the threading fix is still a cherry-pickable strict improvement.

[commit-bot: I haz the power, 2016-09-06 21:46:52]

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

[commit-bot: I haz the power, 2016-09-06 21:46:53]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-06 21:50:45]

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
components/favicon_base/favicon_util.h:39: favicon_base::FaviconRawBitmapResult
ShoehornFaviconToSize(
On 2016/09/06 20:46:42, aelias wrote:
> On 2016/09/06 at 20:19:39, sky wrote:
> > Showhorn? ResizeFaviconBitmapResult?
> 
> I wanted to use a pejorative word, so I renamed to ResizeFaviconResultSlow.

That argument could be applied to any function that could take a while. Resizing
is inherently slow. I prefer no 'slow' here.

[aelias_OOO_until_Jul13, 2016-09-06 22:10:30]

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

[aelias_OOO_until_Jul13, 2016-09-06 22:10:49]

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/2313563002/diff/1/components/favicon_base/fav...
components/favicon_base/favicon_util.h:39: favicon_base::FaviconRawBitmapResult
ShoehornFaviconToSize(
On 2016/09/06 at 21:50:45, sky wrote:
> On 2016/09/06 20:46:42, aelias wrote:
> > On 2016/09/06 at 20:19:39, sky wrote:
> > > Showhorn? ResizeFaviconBitmapResult?
> > 
> > I wanted to use a pejorative word, so I renamed to ResizeFaviconResultSlow.
> 
> That argument could be applied to any function that could take a while.
Resizing is inherently slow. I prefer no 'slow' here.

OK, done.

[commit-bot: I haz the power, 2016-09-06 22:11:16]

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

[brettw, 2016-09-06 22:35:27]

Is my feedback still necessary here? I'm unclear on what's being discussed.

[sky, 2016-09-06 22:44:54]

The reason I added you to this review has been removed. But I think
it's worthwhile having the discussion. Specifically should we convert
favicon resizing code to use FastEncodeBGRASkBitmap.

  -Scott

On Tue, Sep 6, 2016 at 3:35 PM,  <brettw@chromium.org> wrote:
> Is my feedback still necessary here? I'm unclear on what's being discussed.
>
> https://codereview.chromium.org/2313563002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-09-06 23:03:26]

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

[commit-bot: I haz the power, 2016-09-06 23:03:27]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-09-07 01:29:42]

Description was changed from

==========
Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG=641517
==========

to

==========
Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG=644573
==========

[aelias_OOO_until_Jul13, 2016-09-07 21:57:33]

On 2016/09/06 at 22:44:54, sky wrote:
> The reason I added you to this review has been removed. But I think
> it's worthwhile having the discussion. Specifically should we convert
> favicon resizing code to use FastEncodeBGRASkBitmap.

I think we would probably need to do an audit of lifetime, number and RAM usage
of favicons in typical scenarios to have an informed discussion on this.  I was
probably premature in trying to switch this to "Fast" since I don't really know
the full impact on Android yet either, it was just a drive-by change that seemed
like a good idea.

It's something I can revisit with the data in hand if and when favicons show up
as a problem in speed/RAM profiling again.  In the meantime, is this threading
change OK to land?

[sky, 2016-09-07 23:28:22]

https://codereview.chromium.org/2313563002/diff/40001/components/favicon/core...
File components/favicon/core/favicon_service.cc (right):

https://codereview.chromium.org/2313563002/diff/40001/components/favicon/core...
components/favicon/core/favicon_service.cc:348:
ResizeFaviconBitmapResult(favicon_bitmap_results, desired_size_in_pixel));
Why do we need this in both HistoryBackend and here? Won't HistoryBackend cover
this?

https://codereview.chromium.org/2313563002/diff/40001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/2313563002/diff/40001/components/favicon_base...
components/favicon_base/favicon_util.h:39: favicon_base::FaviconRawBitmapResult
ResizeFaviconBitmapResult(
Add description.

[aelias_OOO_until_Jul13, 2016-09-08 01:32:54]

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

[aelias_OOO_until_Jul13, 2016-09-08 01:33:08]

https://codereview.chromium.org/2313563002/diff/40001/components/favicon/core...
File components/favicon/core/favicon_service.cc (right):

https://codereview.chromium.org/2313563002/diff/40001/components/favicon/core...
components/favicon/core/favicon_service.cc:348:
ResizeFaviconBitmapResult(favicon_bitmap_results, desired_size_in_pixel));
On 2016/09/07 at 23:28:22, sky wrote:
> Why do we need this in both HistoryBackend and here? Won't HistoryBackend
cover this?

The sticking point is that favicon_client_->GetFaviconForNativeApplicationURL(),
not HistoryBackend, wind up producing the input for this callback on Chrome for
iOS.  And in general, HistoryBackend seems to have the contract of providing
best-effort reasonable results rather than a strong guarantee of a size.  So I'm
thinking of this like I'm improving quality of the output of HistoryBackend, but
this method is the hard backstop guaranteeing output of a particular size for
correctness.

https://codereview.chromium.org/2313563002/diff/40001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/2313563002/diff/40001/components/favicon_base...
components/favicon_base/favicon_util.h:39: favicon_base::FaviconRawBitmapResult
ResizeFaviconBitmapResult(
On 2016/09/07 at 23:28:22, sky wrote:
> Add description.

Done.

[commit-bot: I haz the power, 2016-09-08 01:33:21]

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-08 02:31:16]

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

[commit-bot: I haz the power, 2016-09-08 02:31:17]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-08 04:19:25]

LGTM

[aelias_OOO_until_Jul13, 2016-09-08 04:22:26]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2016-09-08 04:22:53]

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-08 04:26:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 04:28:28]

Description was changed from

==========
Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG=644573
==========

to

==========
Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG=644573

Committed: https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d
Cr-Commit-Position: refs/heads/master@{#417190}
==========

[commit-bot: I haz the power, 2016-09-08 04:28:29]

Patchset 4 (id:??) landed as
https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d
Cr-Commit-Position: refs/heads/master@{#417190}