Pass initial size and GPU preference via context attributes

Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already the case for GPU process contexts (CommandBufferProxyImpl), make it valid for the other types (GLInProcessContext, tests) by handling the logic it in GLES2DecoderImpl. This will also allow us to later remove allocating unneeded textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts (where it's not used), or when the default framebuffer isn't used (most contexts except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't matter (android, tests), whereas most of the code (compositor, helper contexts) should be choosing the integrated one, for consistency. In particular, pixel tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Committed: https://crrev.com/d488e8b4f505630ef5b4cb7368a34fee8f28d480
Cr-Original-Commit-Position: refs/heads/master@{#402955}
Cr-Commit-Position: refs/heads/master@{#403238}

[piman, 2016-06-29 02:03:35]

Description was changed from

==========
Pass initial size and GPU preference via context attributes

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

to

==========
Pass initial size and GPU preference via context attributes

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-06-29 02:03:36]

Description was changed from

==========
Pass initial size and GPU preference via context attributes

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Pass initial size and GPU preference via context attributes

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

[piman, 2016-06-29 03:17:56]

Description was changed from

==========
Pass initial size and GPU preference via context attributes

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

to

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

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

[piman, 2016-06-29 03:24:52]

piman@chromium.org changed reviewers:
+ boliu@chromium.org, dcheng@chromium.org, fsamuel@chromium.org,
sievers@chromium.org, sky@chromium.org, wez@chromium.org

[piman, 2016-06-29 03:24:53]

sievers: Please review
fsamuel: OWNER for components/mus/gles2/command_buffer_driver.cc and
components/mus/surfaces/surfaces_context_provider.cc
sky: OWNER for components/mus/public/cpp/lib/gles2_context.cc
wez: OWNER for blimp/client/feature/compositor/blimp_context_provider.cc
boliu: OWNER for android_webview/browser/aw_render_thread_context_provider.cc
dcheng: OWNER for gpu/ipc/common/  (note, I updated the OWNERS file there per
new PRESUBMIT)

[sky, 2016-06-29 03:27:42]

LGTM

[Fady Samuel, 2016-06-29 10:04:31]

lgtm

[dcheng, 2016-06-29 10:14:19]

ipc lgtm

[danakj, 2016-06-29 18:25:53]

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

[danakj, 2016-06-29 18:25:54]

drive-by LGTM

https://codereview.chromium.org/2107783003/diff/20001/components/mus/gles2/co...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/2107783003/diff/20001/components/mus/gles2/co...
components/mus/gles2/command_buffer_driver.cc:119: // TODO(piman): virtual
contexts, gpu preference.
can remove preference here :)

https://codereview.chromium.org/2107783003/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/2107783003/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:103:
DCHECK(attribs.offscreen_framebuffer_size.width() >= 0 &&
nit: 2 dchecks, and GE

[Wez, 2016-06-29 20:11:00]

blimp/ LGTM

[piman, 2016-06-29 20:12:06]

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

[piman, 2016-06-29 20:12:20]

The CQ bit was unchecked by piman@chromium.org

[piman, 2016-06-29 20:27:04]

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

[commit-bot: I haz the power, 2016-06-29 20:27:45]

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

[piman, 2016-06-29 20:28:52]

https://codereview.chromium.org/2107783003/diff/20001/components/mus/gles2/co...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/2107783003/diff/20001/components/mus/gles2/co...
components/mus/gles2/command_buffer_driver.cc:119: // TODO(piman): virtual
contexts, gpu preference.
On 2016/06/29 18:25:54, danakj wrote:
> can remove preference here :)

So, I wouldn't say it's handled yet, the client still can't select the
gpu_preference, but we're ever so slightly closer to that. I moved and clarified
the TODO.

https://codereview.chromium.org/2107783003/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/2107783003/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gl_in_process_context.cc:103:
DCHECK(attribs.offscreen_framebuffer_size.width() >= 0 &&
On 2016/06/29 18:25:54, danakj wrote:
> nit: 2 dchecks, and GE

Done.

[boliu, 2016-06-29 21:45:49]

lgtm

[commit-bot: I haz the power, 2016-06-29 21:50:15]

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

[commit-bot: I haz the power, 2016-06-29 21:50:17]

Dry run: This issue passed the CQ dry run.

[piman, 2016-06-29 22:02:16]

The CQ bit was checked by piman@chromium.org

[piman, 2016-06-29 22:02:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org, dcheng@chromium.org, danakj@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2107783003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-29 22:07:32]

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-06-29 22:26:20]

Description was changed from

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

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

to

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

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

[commit-bot: I haz the power, 2016-06-29 22:26:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 22:27:41]

Description was changed from

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

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

to

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Cr-Commit-Position: refs/heads/master@{#402955}
==========

[commit-bot: I haz the power, 2016-06-29 22:27:44]

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

[Pete Williamson, 2016-06-29 23:18:14]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2106103005/ by petewil@chromium.org.

The reason for reverting is: Sheriff reverting under suspicion of causing a Win8
build break.  It seems to be in gles2, and none of the other changelists changed
gles2

Build break logs here:
https://build.chromium.org/p/chromium.win/builders/Win8%20GYP%20%28dbg%29/bui...

Relevant parts of logs:
FAILED: gles2_utils.dll gles2_utils.dll.lib gles2_utils.dll.pdb 
E:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-with-manifests
environment.x86 True gles2_utils.dll "E:\b\depot_tools\python276_bin\python.exe
gyp-win-tool link-wrapper environment.x86 False link.exe /nologo
/IMPLIB:gles2_utils.dll.lib /DLL /OUT:gles2_utils.dll @gles2_utils.dll.rsp" 2
mt.exe rc.exe
"obj\gpu\command_buffer\gles2_utils.gles2_utils.dll.intermediate.manifest"
obj\gpu\command_buffer\gles2_utils.gles2_utils.dll.generated.manifest
gles2_utils.gles2_cmd_utils.obj : error LNK2019: unresolved external symbol
"__declspec(dllimport) public: __thiscall gfx::Size::Size(void)"
(__imp_??0Size@gfx@@QAE@XZ) referenced in function "public: __thiscall
gpu::gles2::ContextCreationAttribHelper::ContextCreationAttribHelper(void)"
(??0ContextCreationAttribHelper@gles2@gpu@@QAE@XZ)

gles2_utils.dll : fatal error LNK1120: 1 unresolved externals.

[piman, 2016-06-29 23:31:17]

On 2016/06/29 23:18:14, Pete Williamson wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2106103005/ by mailto:petewil@chromium.org.
> 
> The reason for reverting is: Sheriff reverting under suspicion of causing a
Win8
> build break.  It seems to be in gles2, and none of the other changelists
changed
> gles2

Yes, this is most likely the culprit. I think it's just a missing gyp change
corresponding to the gn change.

[piman, 2016-06-29 23:33:03]

Description was changed from

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Cr-Commit-Position: refs/heads/master@{#402955}
==========

to

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Cr-Commit-Position: refs/heads/master@{#402955}
==========

[piman, 2016-06-30 03:04:31]

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

[commit-bot: I haz the power, 2016-06-30 03:04:45]

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

[piman, 2016-06-30 03:05:58]

The CQ bit was unchecked by piman@chromium.org

[piman, 2016-06-30 03:33:15]

danakj: PTAL, added a gyp fix.

[danakj, 2016-06-30 18:22:31]

LGTM

[piman, 2016-06-30 18:24:34]

The CQ bit was checked by piman@chromium.org

[piman, 2016-06-30 18:24:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org, boliu@chromium.org, dcheng@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2107783003/#ps100001
(title: "gyp fix")

[commit-bot: I haz the power, 2016-06-30 18:25:05]

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-06-30 19:07:15]

Description was changed from

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Cr-Commit-Position: refs/heads/master@{#402955}
==========

to

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Cr-Commit-Position: refs/heads/master@{#402955}
==========

[commit-bot: I haz the power, 2016-06-30 19:07:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-30 19:07:45]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 19:10:43]

Description was changed from

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Cr-Commit-Position: refs/heads/master@{#402955}
==========

to

==========
Pass initial offscreen size and GPU preference via context attributes

This makes client code simpler, because all those things are passed through the
same places.

This CL also cleans up a few things:
1- make (0, 0) a valid size for offscreen contexts more generally. It is already
the case for GPU process contexts (CommandBufferProxyImpl), make it valid for
the other types (GLInProcessContext, tests) by handling the logic it in
GLES2DecoderImpl. This will also allow us to later remove allocating unneeded
textures.
2- many callers were setting an arbitrary dummy size, even for onscreen contexts
(where it's not used), or when the default framebuffer isn't used (most contexts
except Pepper and EGL).
3- many callers were arbitrarily choosing the discrete GPU, when it doesn't
matter (android, tests), whereas most of the code (compositor, helper contexts)
should be choosing the integrated one, for consistency. In particular, pixel
tests should be consistent with production.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11f4e35b660f4f5cbaed65a0a7f181b8426e877e
Committed: https://crrev.com/d488e8b4f505630ef5b4cb7368a34fee8f28d480
Cr-Original-Commit-Position: refs/heads/master@{#402955}
Cr-Commit-Position: refs/heads/master@{#403238}
==========

[commit-bot: I haz the power, 2016-06-30 19:10:46]

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