Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(172)

Issue 2698573002: Support offscreen contexts which own their backing surface (Closed)

Created:
10 months ago by klausw
Modified:
9 months, 4 weeks ago
CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-vr-reviews_chromium.org, jam, srahim+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

This is an updated version of issue 2586803003 which was reverted. I have removed the problematic "low priority" GL context setting and made the change conditional on a new ""WebVR experimental rendering optimizations" feature flag. When enabled, it uses the own_offscreen_surface flag with WebGL contexts when WebVR is enabled, to allow the surface to be swapped out with a compatible one if used for VR presentation later. This must be done before any WebVR API calls have been made, so it also needs to work for non-WebVR WebGL canvas drawing. The change includes support for canvas attributes such as "antialias". The feature flag is intended to control features which are considered experimental or risky, so that they can be manually enabled for initial testing. It is also in preparation for a finch experiment for future rollout. BUG=655733 CQ_INCLUDE_TRYBOTS=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/2698573002 Cr-Commit-Position: refs/heads/master@{#451484} Committed: https://chromium.googlesource.com/chromium/src/+/89ec62330efc014949d78367b94de89c4a7b5405

Patch Set 1 #

Total comments: 2

Patch Set 2 : Move flag to ENABLE_WEBVR conditional #

Patch Set 3 : Expose the flag to WebKit also #

Total comments: 11

Patch Set 4 : Review comments, merge in crrev 2586803003 #

Total comments: 2

Patch Set 5 : Remove histogram for flag (this is no longer being added) #

Patch Set 6 : Remove obsolete comment, reformat #

Total comments: 2

Patch Set 7 : Add missing alpha_size setting #

Total comments: 6

Patch Set 8 : remove GPU-level own_offscreen_surface attr, fix ctx restore, cleanups #

Patch Set 9 : Remove redundant supportOwnOffscreenSurface ContextAttribute #

Total comments: 2

Patch Set 10 : Revert newline removal #

Patch Set 11 : Add missing LoginCustomFlags histogram entry #

Total comments: 2

Patch Set 12 : Change flag status to "experimental" at jochen@'s request #

Patch Set 13 : Rebase to ToT #

Patch Set 14 : Rebase to ToT #

Patch Set 15 : Rebase again. I'm pretty sure it wasn't me breaking iOS. #

Patch Set 16 : Rebase and retry again #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -13 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
M content/child/runtime_features.cc View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/common/content_features.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_features.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +9 lines, -6 lines 0 comments Download
M gpu/ipc/service/gpu_command_buffer_stub.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +26 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/webgl/WebGLContextAttributeHelpers.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/webgl/WebGLContextAttributeHelpers.cpp View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +29 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebRuntimeFeatures.cpp View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/Platform.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebRuntimeFeatures.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -0 lines 0 comments Download
M ui/gl/gl_surface_egl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 90 (46 generated)
amp
https://codereview.chromium.org/2698573002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2698573002/diff/1/chrome/browser/about_flags.cc#newcode1614 chrome/browser/about_flags.cc:1614: {"enable-webvr-experimental-rendering", Let's only show this when webvr is enabled ...
10 months ago (2017-02-14 23:23:26 UTC) #3
klausw
jochen@: Please review the content/feature list parts and the histograms change. rkaplow@: Finch ambassador. Launch ...
10 months ago (2017-02-14 23:31:29 UTC) #5
jochen (gone - plz use gerrit)
I can't approve general histograms changes, just use counter additions. You add four different mechanisms ...
10 months ago (2017-02-15 10:00:38 UTC) #8
amp
On 2017/02/15 10:00:38, jochen wrote: > I can't approve general histograms changes, just use counter ...
10 months ago (2017-02-15 17:43:35 UTC) #9
amp
https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc#newcode1617 chrome/browser/about_flags.cc:1617: IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_DESCRIPTION, kOsAll, Should we limit this to Android only ...
10 months ago (2017-02-15 17:44:03 UTC) #10
mthiesse
I agree with jochen though that we should definitely get the code landed with the ...
10 months ago (2017-02-15 17:48:07 UTC) #11
klausw
Everyone added: This is an updated version of crrev.com/2586803003 which you had previously reviewed and ...
10 months ago (2017-02-15 19:47:22 UTC) #14
amp
https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc#newcode1617 chrome/browser/about_flags.cc:1617: IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_DESCRIPTION, kOsAll, On 2017/02/15 19:47:22, klausw wrote: > On ...
10 months ago (2017-02-15 20:04:22 UTC) #15
klausw
On 2017/02/15 10:00:38, jochen wrote: > I can't approve general histograms changes, just use counter ...
10 months ago (2017-02-15 20:04:56 UTC) #16
klausw
On 2017/02/15 20:04:56, klausw wrote: > On 2017/02/15 10:00:38, jochen wrote: > > I can't ...
10 months ago (2017-02-15 20:06:52 UTC) #17
amp
On 2017/02/15 20:06:52, klausw wrote: > On 2017/02/15 20:04:56, klausw wrote: > > On 2017/02/15 ...
10 months ago (2017-02-15 20:13:56 UTC) #20
klausw
amp@ wrote: > I think the histogram change is from adding anything to about://flags, even ...
10 months ago (2017-02-15 20:21:16 UTC) #22
mthiesse
https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc#newcode1029 content/renderer/renderer_blink_platform_impl.cc:1029: attributes.depth_size = 0; Are you missing attributes.alpha_size = -1? ...
10 months ago (2017-02-15 21:36:43 UTC) #24
klausw
https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc#newcode1029 content/renderer/renderer_blink_platform_impl.cc:1029: attributes.depth_size = 0; On 2017/02/15 21:36:43, mthiesse wrote: > ...
10 months ago (2017-02-15 22:16:33 UTC) #25
aelias_OOO_until_Jul13
https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc#newcode1021 content/renderer/renderer_blink_platform_impl.cc:1021: if (web_attributes.supportOwnOffscreenSurface) { It doesn't look like this outer ...
10 months ago (2017-02-16 03:25:51 UTC) #26
klausw
dcheng@: I think this no longer needs your review, I've removed the IPC changes. I've ...
10 months ago (2017-02-16 20:06:58 UTC) #27
klausw
On 2017/02/16 20:06:58, klausw wrote: > - remove the GPU-level "own_offscreen_surface" attribute. Instead, ask for ...
10 months ago (2017-02-16 20:16:30 UTC) #28
aelias_OOO_until_Jul13
content/renderer lgtm
10 months ago (2017-02-16 21:55:46 UTC) #31
bajones
modules/webgl LGTM https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h File gpu/command_buffer/common/gles2_cmd_utils.h (left): https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h#oldcode342 gpu/command_buffer/common/gles2_cmd_utils.h:342: Unneeded newline removal.
10 months ago (2017-02-16 22:05:41 UTC) #32
klausw
https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h File gpu/command_buffer/common/gles2_cmd_utils.h (left): https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h#oldcode342 gpu/command_buffer/common/gles2_cmd_utils.h:342: On 2017/02/16 22:05:41, bajones wrote: > Unneeded newline removal. ...
10 months ago (2017-02-16 22:17:10 UTC) #33
klausw
PTAL: jochen@: please review the feature changes in content/*features* and third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 jbauman@: please review the ...
10 months ago (2017-02-16 22:37:38 UTC) #35
jbauman
lgtm
10 months ago (2017-02-17 00:38:19 UTC) #36
klausw
mpearson@: please review the histograms.xml additions for the feature flag. (I mistakenly thought earlier that ...
10 months ago (2017-02-17 00:43:33 UTC) #38
Mark P
histograms.xml lgtm
10 months ago (2017-02-17 06:23:30 UTC) #43
jochen (gone - plz use gerrit)
lgtm with comment https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right): https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode917 third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:917: status: "test", I think this should ...
10 months ago (2017-02-17 08:34:33 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/220001
9 months, 4 weeks ago (2017-02-17 15:40:58 UTC) #51
klausw
Thank you! https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right): https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode917 third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:917: status: "test", On 2017/02/17 08:34:33, jochen wrote: ...
9 months, 4 weeks ago (2017-02-17 15:41:19 UTC) #52
rkaplow
lgtm
9 months, 4 weeks ago (2017-02-17 16:42:08 UTC) #53
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/367236)
9 months, 4 weeks ago (2017-02-17 18:06:13 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/220001
9 months, 4 weeks ago (2017-02-17 18:08:48 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/220001
9 months, 4 weeks ago (2017-02-17 19:54:58 UTC) #60
amp
lgtm
9 months, 4 weeks ago (2017-02-17 20:41:37 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/220001
9 months, 4 weeks ago (2017-02-17 20:41:50 UTC) #64
commit-bot: I haz the power
Failed to apply patch for content/public/common/content_features.h: While running git apply --index -p1; error: patch failed: ...
9 months, 4 weeks ago (2017-02-17 23:02:56 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/260001
9 months, 4 weeks ago (2017-02-17 23:11:44 UTC) #69
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/157830)
9 months, 4 weeks ago (2017-02-18 00:26:55 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/280001
9 months, 4 weeks ago (2017-02-18 01:25:14 UTC) #74
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on ...
9 months, 4 weeks ago (2017-02-18 03:27:42 UTC) #76
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/290001
9 months, 4 weeks ago (2017-02-18 03:37:13 UTC) #79
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on ...
9 months, 4 weeks ago (2017-02-18 05:38:26 UTC) #81
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/290001
9 months, 4 weeks ago (2017-02-18 06:19:04 UTC) #83
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/367807)
9 months, 4 weeks ago (2017-02-18 07:54:05 UTC) #85
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/290001
9 months, 4 weeks ago (2017-02-18 17:47:37 UTC) #87
commit-bot: I haz the power
9 months, 4 weeks ago (2017-02-18 18:48:43 UTC) #90
Message was sent while issue was closed.
Committed patchset #16 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/89ec62330efc014949d78367b94d...

Powered by Google App Engine
This is Rietveld 0eb02b776