cc: turn on distance field text on animated layers

cc: turn on distance field text on animated layers

PictureLayerImpl
* Rerasterize on every animated frame
* pass animated flag to picture pile

PicturePileImpl
* Store has_transform_animated, used as a transport to get data
  to GPU's RasterBufferImpl

ResourceProvider
* Provide a method to recreate the surface with the correct dff
  flag.
* Provide a method in the lock to set the dff setting
 * This has the side effect that the surface may be created
   twice for a new animated layer, but surface creation is
   cheap, I don't see this as a huge issue

RasterBufferImpl
* In playback, call the method on the lock to assign the dff
  setting that comes from the picture pile.

BUG=372391
Committed: https://crrev.com/6a15e256edc14ad5c6f8d088a82c2f0b421a2d3d
Cr-Commit-Position: refs/heads/master@{#300626}

[hendrikw, 2014-10-15 16:32:26]

hendrikw@chromium.org changed reviewers:
+ reveman@chromium.org, vmpstr@chromium.org

[hendrikw, 2014-10-15 16:34:01]

PTAL, thanks

[reveman, 2014-10-15 17:34:09]

https://codereview.chromium.org/648293006/diff/1/cc/resources/picture_pile_im...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/648293006/diff/1/cc/resources/picture_pile_im...
cc/resources/picture_pile_impl.h:61: bool has_transform_animated() const {
return has_transform_animated_; }
"transform animated" is confusing here. what transform? some transform inside
the picture pile, the scale transform used to raster it or something else? I can
tell by the picture layer code that this refer to the transform used to draw the
layer for which this picture pile represents the contents of but that's
impossible to know by just looking at this file.

I think this needs a comment at a minimum. Possibly a better name..

https://codereview.chromium.org/648293006/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/648293006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1036: CreateSkSurface(id,
use_distance_field_text_);
Let's not bother to create the surface here if you might have to re-create it
later when you know if dff should be used or not anyhow.

https://codereview.chromium.org/648293006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1167:
resource_provider_->use_distance_field_text_ || use_distance_field_text;
what is ResourceProvider::use_distance_field_text_ for? Can we remove it or is
the input parameter not enough?

https://codereview.chromium.org/648293006/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/648293006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:333: void SetUseDistanceFieldText(bool
use_distance_field_text);
How about we change sk_surface() function above to "SkSurface* GetSkSurface(bool
use_distance_field_text)" instead? GetSkSurface would create a new SkSurface or
reuse the existing one depending on the input parameter.

https://codereview.chromium.org/648293006/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:508: SkSurface*
ResourceProvider::CreateSkSurface(ResourceId id,
Not need for  ResourceProvider:: prefix. Though, I don't think we need this
function at all. I think the implementation of this can just move to
ScopedWriteLockGr::GetSkSurface.

[reveman, 2014-10-15 17:36:29]

This looks good in general. Just address some of my comments and I think this is
something we can land.

[ajuma, 2014-10-17 13:36:23]

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

[ajuma, 2014-10-17 13:36:24]

https://codereview.chromium.org/648293006/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/648293006/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1236: return false;
We still need this check, since we don't want to re-rasterize every frame when
using software rasterization.

[hendrikw, 2014-10-20 16:31:09]

Addressed comments.  PTAL thanks

[reveman, 2014-10-20 18:38:30]

https://codereview.chromium.org/648293006/diff/80001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/648293006/diff/80001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:41:
picture_pile->will_be_used_for_transform_animation();
This line should also take LayerTreeSettings::use_distance_field_text into
account. See my resource provider comment for more details.

https://codereview.chromium.org/648293006/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/648293006/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.h:61: bool will_be_used_for_transform_animation()
const {
is it guaranteed that this will be used for a transform animation? would
"likely_to_be_used_for_transform_animation" be better?

https://codereview.chromium.org/648293006/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.h:65: bool will_be_used_for_transform_animation)
{
I don't see 'false' ever being passed to this function. can we remove the
parameter?

https://codereview.chromium.org/648293006/diff/80001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/648293006/diff/80001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1132:
resource_provider_->use_distance_field_text_ || use_distance_field_text;
ResourceProvider::use_distance_field_text_ should be removed and that setting
should be passed to the GpuRasterWorkerPool that is now responsible for making
that decision. Overriding it here is confusing.

[hendrikw, 2014-10-20 19:58:49]

PTAL, moved dff to GRP

[reveman, 2014-10-20 20:27:13]

https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
cc/resources/gpu_raster_worker_pool.cc:43: use_distance_field_text_ ||
&& ?

[hendrikw, 2014-10-20 20:34:11]

https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
cc/resources/gpu_raster_worker_pool.cc:43: use_distance_field_text_ ||
On 2014/10/20 20:27:13, reveman wrote:
> && ?

||. That comes from the global dff flag (--enable-distance-field-text), if it's
on, we're always using dff, if it's off, we only use it for animated layers.

[reveman, 2014-10-20 21:04:26]

https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
cc/resources/gpu_raster_worker_pool.cc:43: use_distance_field_text_ ||
On 2014/10/20 20:34:11, Hendrik wrote:
> On 2014/10/20 20:27:13, reveman wrote:
> > && ?
> 
> ||. That comes from the global dff flag (--enable-distance-field-text), if
it's
> on, we're always using dff, if it's off, we only use it for animated layers.

hm, why do we need both modes? can we remove the global dff flag?

[hendrikw, 2014-10-20 21:45:27]

On 2014/10/20 21:04:26, reveman wrote:
>
https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
> File cc/resources/gpu_raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
> cc/resources/gpu_raster_worker_pool.cc:43: use_distance_field_text_ ||
> On 2014/10/20 20:34:11, Hendrik wrote:
> > On 2014/10/20 20:27:13, reveman wrote:
> > > && ?
> > 
> > ||. That comes from the global dff flag (--enable-distance-field-text), if
> it's
> > on, we're always using dff, if it's off, we only use it for animated layers.
> 
> hm, why do we need both modes? can we remove the global dff flag?

The other flag is there so that Jim and others can test dffs in non-animating
cases (e.g. dffs always on for everything on android)

[reveman, 2014-10-20 22:51:31]

On 2014/10/20 21:45:27, Hendrik wrote:
> On 2014/10/20 21:04:26, reveman wrote:
> >
>
https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
> > File cc/resources/gpu_raster_worker_pool.cc (right):
> > 
> >
>
https://codereview.chromium.org/648293006/diff/100001/cc/resources/gpu_raster...
> > cc/resources/gpu_raster_worker_pool.cc:43: use_distance_field_text_ ||
> > On 2014/10/20 20:34:11, Hendrik wrote:
> > > On 2014/10/20 20:27:13, reveman wrote:
> > > > && ?
> > > 
> > > ||. That comes from the global dff flag (--enable-distance-field-text), if
> > it's
> > > on, we're always using dff, if it's off, we only use it for animated
layers.
> > 
> > hm, why do we need both modes? can we remove the global dff flag?
> 
> The other flag is there so that Jim and others can test dffs in non-animating
> cases (e.g. dffs always on for everything on android)

Ok, cool. LGTM.

[vmpstr, 2014-10-20 23:17:05]

lgtm, but just to cofirm: once enabled for a pile, this flag will always stay
on? So if the layer stops animating, we will draw the dff textures unless we
invalidate?

https://codereview.chromium.org/648293006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/648293006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1129: // If the surface doesn't have the
correct dff setting, recreate the surface
nit: If the surface doesn't exist, or doesn't have the correct dff setting, ...

[vmpstr, 2014-10-20 23:17:07]

lgtm, but just to cofirm: once enabled for a pile, this flag will always stay
on? So if the layer stops animating, we will draw the dff textures unless we
invalidate?

[hendrikw, 2014-10-20 23:23:12]

Correct.  This is to prevent the text from changing appearance once the
animation stops.  I suppose we'll see what performance implications this has
soon.

https://codereview.chromium.org/648293006/diff/100001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/648293006/diff/100001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1129: // If the surface doesn't have the
correct dff setting, recreate the surface
On 2014/10/20 23:17:05, vmpstr wrote:
> nit: If the surface doesn't exist, or doesn't have the correct dff setting,
...

Acknowledged.

[hendrikw, 2014-10-20 23:23:57]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-10-20 23:26:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/648293006/120001

[commit-bot: I haz the power, 2014-10-20 23:34:51]

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

[commit-bot: I haz the power, 2014-10-20 23:34:52]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/71019)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/75872)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hendrikw, 2014-10-21 22:06:15]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-10-21 22:08:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/648293006/140001

[commit-bot: I haz the power, 2014-10-21 23:52:49]

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

[commit-bot: I haz the power, 2014-10-21 23:52:50]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hendrikw, 2014-10-22 00:22:34]

Fixed a gpu raster test that was ensuring that we render to max animation scale
when we have text, we now render at content scale.

See changes in: cc/layers/picture_layer_impl_unittest.cc

[hendrikw, 2014-10-22 00:23:28]

The CQ bit was checked by hendrikw@chromium.org

[vmpstr, 2014-10-22 00:23:31]

still lgtm

[commit-bot: I haz the power, 2014-10-22 00:24:24]

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

[hendrikw, 2014-10-22 00:25:56]

The CQ bit was unchecked by hendrikw@chromium.org

[hendrikw, 2014-10-22 00:30:26]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-10-22 00:33:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/648293006/180001

[commit-bot: I haz the power, 2014-10-22 02:46:38]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-10-22 04:05:41]

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