Fix use_image_texture_target inconsistencies

Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9151705c225783e878077fdabcee4b513af7f049
Cr-Commit-Position: refs/heads/master@{#407885}

[ericrk, 2016-07-01 22:35:02]

Description was changed from

==========
Fix use_image_texture_target inconsistencies

BUG=
==========

to

==========
Fix use_image_texture_target inconsistencies

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ericrk, 2016-07-15 20:35:17]

Patchset #2 (id:20001) has been deleted

[ericrk, 2016-07-15 20:35:28]

Patchset #2 (id:40001) has been deleted

[ericrk, 2016-07-15 20:52:14]

Description was changed from

==========
Fix use_image_texture_target inconsistencies

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in the
Browser Process based on it's guess about the BufferUsage of buffers in the
Renderer Process. This guess is almost always wrong, as the Renderer uses
multiple BufferUsages in many cases, and as we add support for more advanced
scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to be a map
from BufferUsage/BufferFormat to texture target, rather than just from
BufferFormat. This allows the renderer to query for the correct target for the
usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just uses a
default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ericrk, 2016-07-15 20:52:14]

ericrk@chromium.org changed reviewers:
+ enne@chromium.org, erikchen@chromium.org

[erikchen, 2016-07-15 21:08:36]

Gracias! lgtm

https://codereview.chromium.org/2120713002/diff/80001/cc/proto/renderer_setti...
File cc/proto/renderer_settings.proto (right):

https://codereview.chromium.org/2120713002/diff/80001/cc/proto/renderer_setti...
cc/proto/renderer_settings.proto:12: optional uint32 buffer_usage = 1;
should these be required?

https://codereview.chromium.org/2120713002/diff/80001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/2120713002/diff/80001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:162: settings_.use_image_texture_targets));
thanks. I was going to do this myself in a follow-up CL. :)

[enne (OOO), 2016-07-15 21:39:32]

(Also can you format your description to wrap at 72 columns <_<)

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
content/renderer/render_thread_impl.cc:426: DCHECK_EQ(fields.size(), 3);
Mmm, is this safe if the dcheck fails? Should it be a CHECK?

I think I'd feel better if the encoding/decoding of command line args were in
some separate utility class that could be unit tested.

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
content/renderer/render_thread_impl.h:662:
cc::RendererSettings::ImageTextureTargetsMap use_image_texture_targets_;
This is maybe a readability nit, but I expect something named "use_foo" to be a
boolean.  This is a usage/format => target map, right?  I realize
"use_video_frame_image_targets" is right below, following the same pattern, but
this is a really weird name to my ears.

[ericrk, 2016-07-15 23:45:47]

Patchset #4 (id:100001) has been deleted

[ericrk, 2016-07-15 23:48:00]

Description was changed from

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in the
Browser Process based on it's guess about the BufferUsage of buffers in the
Renderer Process. This guess is almost always wrong, as the Renderer uses
multiple BufferUsages in many cases, and as we add support for more advanced
scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to be a map
from BufferUsage/BufferFormat to texture target, rather than just from
BufferFormat. This allows the renderer to query for the correct target for the
usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just uses a
default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix use_image_texture_target inconsistencies
========================================================================
Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ericrk, 2016-07-15 23:48:10]

Description was changed from

==========
Fix use_image_texture_target inconsistencies
========================================================================
Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-07-19 17:28:56]

lgtm

[ericrk, 2016-07-19 20:22:43]

forgot to publish :/

https://codereview.chromium.org/2120713002/diff/80001/cc/proto/renderer_setti...
File cc/proto/renderer_settings.proto (right):

https://codereview.chromium.org/2120713002/diff/80001/cc/proto/renderer_setti...
cc/proto/renderer_settings.proto:12: optional uint32 buffer_usage = 1;
On 2016/07/15 21:08:35, erikchen wrote:
> should these be required?

Although these are "required", we use optional for all fields, as using required
makes deprecating fields much harder.

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
content/renderer/render_thread_impl.cc:426: DCHECK_EQ(fields.size(), 3);
On 2016/07/15 21:39:32, enne wrote:
> Mmm, is this safe if the dcheck fails? Should it be a CHECK?
> 
> I think I'd feel better if the encoding/decoding of command line args were in
> some separate utility class that could be unit tested.

Changed these to CHECKs and added a unit test. We should never end up with
invalid data here, so crashing seems appropriate.

Moved this to a helper class (along with serialization function), and added a
unit test.

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/2120713002/diff/80001/content/renderer/render...
content/renderer/render_thread_impl.h:662:
cc::RendererSettings::ImageTextureTargetsMap use_image_texture_targets_;
On 2016/07/15 21:39:32, enne wrote:
> This is maybe a readability nit, but I expect something named "use_foo" to be
a
> boolean.  This is a usage/format => target map, right?  I realize
> "use_video_frame_image_targets" is right below, following the same pattern,
but
> this is a really weird name to my ears.

Renamed this to "buffer_to_texture_target_map"

[ericrk, 2016-07-19 20:24:44]

ericrk@chromium.org changed reviewers:
+ danakj@chromium.org, sievers@chromium.org

[ericrk, 2016-07-19 20:24:45]

+danakj for ui/compositor
+sievers for content/

Thanks!

[danakj, 2016-07-19 20:37:56]

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:162: for (size_t usage_idx = 0;
why size_t and not int?

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:163: usage_idx <
static_cast<size_t>(gfx::BufferUsage::LAST) + 1;
nit: use <= instead of +1

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:166: for (size_t format_idx = 0;
why size_t and not int?

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:167: format_idx <
static_cast<size_t>(gfx::BufferFormat::LAST) + 1;
nit: <= instead of +1

[ericrk, 2016-07-19 21:43:09]

Patchset #6 (id:140001) has been deleted

[ericrk, 2016-07-19 21:47:46]

Thanks for the feedback!

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:162: for (size_t usage_idx = 0;
On 2016/07/19 20:37:56, danakj wrote:
> why size_t and not int?

No reason - previous code used size_t, but I can't see a strong reason why -
changing to int.

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:163: usage_idx <
static_cast<size_t>(gfx::BufferUsage::LAST) + 1;
On 2016/07/19 20:37:56, danakj wrote:
> nit: use <= instead of +1

Done.

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:166: for (size_t format_idx = 0;
On 2016/07/19 20:37:56, danakj wrote:
> why size_t and not int?

Done.

https://codereview.chromium.org/2120713002/diff/60002/ui/compositor/composito...
ui/compositor/compositor.cc:167: format_idx <
static_cast<size_t>(gfx::BufferFormat::LAST) + 1;
On 2016/07/19 20:37:56, danakj wrote:
> nit: <= instead of +1

Done.

[ericrk, 2016-07-19 21:47:57]

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

[commit-bot: I haz the power, 2016-07-19 21:48:41]

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-07-19 21:48:43]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-19 22:11:56]

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

[commit-bot: I haz the power, 2016-07-19 22:11:57]

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

[danakj, 2016-07-19 22:28:09]

LGTM

[ericrk, 2016-07-20 17:41:39]

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

[commit-bot: I haz the power, 2016-07-20 17:42:17]

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-07-20 17:42:19]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-20 18:06:03]

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

[commit-bot: I haz the power, 2016-07-20 18:06:04]

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

[ericrk, 2016-07-21 21:26:48]

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

[commit-bot: I haz the power, 2016-07-21 21:27:08]

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-07-21 21:27:11]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-21 22:04:24]

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

[commit-bot: I haz the power, 2016-07-21 22:04:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[ericrk, 2016-07-22 00:16:24]

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

[ericrk, 2016-07-22 00:17:13]

Description was changed from

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-22 00:17: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-07-22 00:17:28]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-22 01:44:39]

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

[commit-bot: I haz the power, 2016-07-22 01:44:40]

Dry run: This issue passed the CQ dry run.

[ericrk, 2016-07-22 17:42:12]

ping for content review. Thanks!

[ericrk, 2016-07-26 18:40:34]

ericrk@chromium.org changed reviewers:
+ avi@chromium.org

[ericrk, 2016-07-26 18:40:35]

+avi for content owners. Thanks!

[Avi (use Gerrit), 2016-07-26 18:44:47]

LGTM stampity stamp

[ericrk, 2016-07-26 18:51:28]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-07-26 18:51:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, enne@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2120713002/#ps180001
(title: "fix content browsertests")

[commit-bot: I haz the power, 2016-07-26 18:51: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-07-26 19:15:55]

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

[commit-bot: I haz the power, 2016-07-26 19:15:57]

Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[ericrk, 2016-07-26 19:18:53]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2016-07-26 19:19:21]

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-07-26 19:53:33]

Description was changed from

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-26 19:53:35]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-07-26 19:55:43]

Description was changed from

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Fix use_image_texture_target inconsistencies

Currently, LayerTreeSettings::use_image_texture_targets is populated in
the Browser Process based on it's guess about the BufferUsage of buffers
in the Renderer Process. This guess is almost always wrong, as the
Renderer uses multiple BufferUsages in many cases, and as we add support
for more advanced scenarios, this will be wrong more often.

To work around this, this change modifies use_image_texture_targets to
be a map from BufferUsage/BufferFormat to texture target, rather than
just from BufferFormat. This allows the renderer to query for the
correct target for the usage/format combination it is currently using.

Additionally, this change moves this map from LayerTreeSettings to
RendererSettings, at it is also used in display.cc, which currently just
uses a default-initialized list.

BUG=590317
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9151705c225783e878077fdabcee4b513af7f049
Cr-Commit-Position: refs/heads/master@{#407885}
==========

[commit-bot: I haz the power, 2016-07-26 19:55:45]

Patchset 7 (id:??) landed as
https://crrev.com/9151705c225783e878077fdabcee4b513af7f049
Cr-Commit-Position: refs/heads/master@{#407885}