WebGL: Tidy up buffer allocation logic

WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing. Rather than compute the
formats to use, compute the parameter "should we allocate an
alpha channel".

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2820373003
Cr-Commit-Position: refs/heads/master@{#465898}
Committed: https://chromium.googlesource.com/chromium/src/+/2ecea20b01bdccc4f80893e5e14955bad1e9a554

[ccameron, 2017-04-19 05:56:04]

Description was changed from

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing.

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
==========

to

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing.

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2017-04-19 05:56:05]

Description was changed from

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing.

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing.

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2017-04-19 06:07:50]

Description was changed from

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing.

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing. Rather than compute the
formats to use, compute the parameter "should we allocate an
alpha channel".

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2017-04-19 06:07:50]

ccameron@chromium.org changed reviewers:
+ zmo@chromium.org

[ccameron, 2017-04-19 06:09:03]

ptal -- small change, just makes things more clear

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

https://codereview.chromium.org/2820373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1166: }
This line here is the only logic change -- if we fail to allocate the image, we
should compute the texture parameters.

[Zhenyao Mo, 2017-04-19 17:06:03]

zmo@chromium.org changed reviewers:
+ kbr@chromium.org

[Zhenyao Mo, 2017-04-19 17:06:04]

LGTM

kbr: does this collide with your work on fixing RGB emulation bug?

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

https://codereview.chromium.org/2820373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1186: GLenum
internal_storage_format = GL_NONE;
nit: you can use ternary operator here to simplify the code.

https://codereview.chromium.org/2820373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1194: GLenum
gl_format = GL_NONE;
nit: same here, ternary operator.

[Ken Russell (switch to Gerrit), 2017-04-19 17:25:32]

lgtm

Yes, this will collide with the fix for http://crbug.com/699566 , but that's OK
and can be resolved. I'll rebase on top of this once it lands.

[ccameron, 2017-04-19 18:06:09]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-04-19 18:06:56]

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, 2017-04-19 19:10:25]

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

[commit-bot: I haz the power, 2017-04-19 19:10:26]

Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ccameron, 2017-04-19 19:44:17]

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

[commit-bot: I haz the power, 2017-04-19 19:45:00]

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

[ccameron, 2017-04-19 19:45:42]

Thanks!

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

https://codereview.chromium.org/2820373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:538:
parameters.allocate_alpha_channel = true;
D'oh, this was supposed to be "false".

https://codereview.chromium.org/2820373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1186: GLenum
internal_storage_format = GL_NONE;
On 2017/04/19 17:06:04, Zhenyao Mo wrote:
> nit: you can use ternary operator here to simplify the code.

Done.

https://codereview.chromium.org/2820373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1194: GLenum
gl_format = GL_NONE;
On 2017/04/19 17:06:04, Zhenyao Mo wrote:
> nit: same here, ternary operator.

Done.

[commit-bot: I haz the power, 2017-04-19 21:56:04]

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

[commit-bot: I haz the power, 2017-04-19 21:56:05]

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_...)

[ccameron, 2017-04-20 02:40:36]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-04-20 02:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2820373003/#ps80001
(title: "No incidental bugfix")

[commit-bot: I haz the power, 2017-04-20 02:40:53]

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

[ccameron, 2017-04-20 02:44:33]

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

[commit-bot: I haz the power, 2017-04-20 02:44:43]

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, 2017-04-20 04:18:50]

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

[commit-bot: I haz the power, 2017-04-20 04:18:51]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-04-20 04:20:49]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-04-20 04:21:11]

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, 2017-04-20 04:25:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492662049279890,
"parent_rev": "29fc92dc9444d3012f6259e87dd624bcd5455996", "commit_rev":
"2ecea20b01bdccc4f80893e5e14955bad1e9a554"}

[commit-bot: I haz the power, 2017-04-20 04:26:25]

Description was changed from

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing. Rather than compute the
formats to use, compute the parameter "should we allocate an
alpha channel".

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
WebGL: Tidy up buffer allocation logic

This patch should not change any of the allocation format logic, it
just makes it more clear what it's doing. Rather than compute the
formats to use, compute the parameter "should we allocate an
alpha channel".

But, there is one bugfix where, if we fail to allocate a GLImage, we
we were not re-computing the color buffer parameters. Now we do.

BUG=711107
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2820373003
Cr-Commit-Position: refs/heads/master@{#465898}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ecea20b01bdccc4f80893e5e149...
==========

[commit-bot: I haz the power, 2017-04-20 04:26:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2ecea20b01bdccc4f80893e5e149...