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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by klausw
Modified:
4 months, 1 week 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
Commit queue not available (can’t edit this change).

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 ...
4 months, 2 weeks 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 ...
4 months, 2 weeks ago (2017-02-14 23:31:29 UTC) #5
jochen
I can't approve general histograms changes, just use counter additions. You add four different mechanisms ...
4 months, 2 weeks 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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? ...
4 months, 1 week 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: > ...
4 months, 1 week ago (2017-02-15 22:16:33 UTC) #25
aelias
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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-16 20:16:30 UTC) #28
aelias
content/renderer lgtm
4 months, 1 week 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.
4 months, 1 week 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. ...
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-16 22:37:38 UTC) #35
jbauman
lgtm
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-17 00:43:33 UTC) #38
Mark P
histograms.xml lgtm
4 months, 1 week ago (2017-02-17 06:23:30 UTC) #43
jochen
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 ...
4 months, 1 week 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
4 months, 1 week 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: ...
4 months, 1 week ago (2017-02-17 15:41:19 UTC) #52
rkaplow
lgtm
4 months, 1 week 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)
4 months, 1 week 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
4 months, 1 week 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
4 months, 1 week ago (2017-02-17 19:54:58 UTC) #60
amp
lgtm
4 months, 1 week 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
4 months, 1 week 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: ...
4 months, 1 week 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
4 months, 1 week 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)
4 months, 1 week 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
4 months, 1 week 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 ...
4 months, 1 week 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
4 months, 1 week 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 ...
4 months, 1 week 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
4 months, 1 week 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)
4 months, 1 week 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
4 months, 1 week ago (2017-02-18 17:47:37 UTC) #87
commit-bot: I haz the power
4 months, 1 week 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589