Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2586803003:
Enable creation of offscreen contexts which own their backing surface. (Closed)
|
Created:
4 years ago by klausw Modified:
3 years, 11 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, haraken, jam, kalyank, mlamouri+watch-content_chromium.org, ozone-reviews_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable creation of offscreen contexts which own their backing surface.
This is adapted from bajones's patch from issue 2461803002 at patchset 1
(http://crrev.com/2461803002#ps1)
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.
Includes support for canvas attributes such as "antialias", and adds a "low
priority" GL context option so that postprocessing such as in WebVR frame
submit at normal priority won't get blocked on the next frame being drawn.
BUG=655722
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/2586803003
Cr-Commit-Position: refs/heads/master@{#442448}
Committed: https://chromium.googlesource.com/chromium/src/+/46b9f6aaaf5851014c2474365734e71ffe9367a0
Patch Set 1 #Patch Set 2 : Rebase to 356cd8379b3f2b06bd17716f88a2568791b4c60a #Patch Set 3 : Rebase on top of crrev.com/2616723002 #Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : Rebase, mark dependency #Patch Set 7 : (ownsurface) Add EGL_IMG_context_priority extension presence check #Patch Set 8 : Avoid segfault when there is no script context #
Total comments: 2
Patch Set 9 : (ownsurface) context priority workaround, update comments, VLOG->DVLOG #
Total comments: 2
Patch Set 10 : (ownsurface) Remove unused declaration #Patch Set 11 : (ownsurface) add low_priority to buffer_traits #
Total comments: 4
Patch Set 12 : Rebase, update comment for alpha semantics #Messages
Total messages: 61 (29 generated)
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
klausw@chromium.org changed reviewers: + bajones@chromium.org, jbauman@chromium.org
This is an updated fork of bajones's original version, see also https://codereview.chromium.org/2461803002#msg20 for history. The "Depends on Patchset" is set to the GLFormat refactor that I split out separately. Please let me know what you think - we are separately looking into adding additional test coverage.
klausw@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please review changes in gpu_command_buffer_traits_multi.h and Platform.h . This is a refreshed fork of bajones's patch that you previously reviewed, see https://codereview.chromium.org/2461803002#msg20 for context.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/05 05:51:59, klausw wrote: > mailto:dcheng@chromium.org: Please review changes in gpu_command_buffer_traits_multi.h > and Platform.h . > > This is a refreshed fork of bajones's patch that you > previously reviewed, see https://codereview.chromium.org/2461803002#msg20 for > context. BTW, I think I help how to do the presence check for EGL_IMG_context_priority (https://www.khronos.org/registry/egl/extensions/IMG/EGL_IMG_context_priority.txt) properly. As noted in the TODO comment for patchset 7, the extension doesn't seem to be showing in the list inside Chrome, and the separate OpenGL-ES Info tool also doesn't show it: https://play.google.com/store/apps/details?id=nl.svdree.glesinfo However, the extension seems to be mandatory for Daydream-ready devices, and as far as I can tell it's actually present and working, just not being reported. See https://source.android.com/compatibility/android-cdd.html .
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
please update BUG to something narrower (and not closed :P)
Description was changed from ========== Enable creation of offscreen contexts which own their backing surface. This is adapted from bajones's patch from issue 2461803002 at patchset 1 (http://crrev.com/2461803002#ps1) 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. Includes support for canvas attributes such as "antialias", and adds a "low priority" GL context option so that postprocessing such as in WebVR frame submit at normal priority won't get blocked on the next frame being drawn. BUG=389343 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 ========== to ========== Enable creation of offscreen contexts which own their backing surface. This is adapted from bajones's patch from issue 2461803002 at patchset 1 (http://crrev.com/2461803002#ps1) 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. Includes support for canvas attributes such as "antialias", and adds a "low priority" GL context option so that postprocessing such as in WebVR frame submit at normal priority won't get blocked on the next frame being drawn. BUG=655722 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 ==========
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/05 20:04:02, mthiesse wrote: > please update BUG to something narrower (and not closed :P) Done, switched to https://crbug.com/655722 .
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... gpu/ipc/service/gpu_command_buffer_stub.cc:628: use_virtualized_gl_context_ = false; We probably need to actually fail context creation if use_virtualized_gl_context_ is true in this case. Otherwise we'll hit a lot of driver bugs (which is why we have those workarounds in the first place).
https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... gpu/ipc/service/gpu_command_buffer_stub.cc:628: use_virtualized_gl_context_ = false; On 2017/01/06 03:09:08, jbauman wrote: > We probably need to actually fail context creation if > use_virtualized_gl_context_ is true in this case. Otherwise we'll hit a lot of > driver bugs (which is why we have those workarounds in the first place). I don't have a strong opinion about this, but the pre-existing code right above this (line 608) just sets use_virtualized_gl_context_ to false if the onscreen buffer's format is incompatible with the default surface, without raising an error if it was originally set, and that's a fairly similar scenario. What happens when it returns false here? Does WebGL content simply fail to render, and does it report this back to the user in some way?
klausw@chromium.org changed reviewers: + aelias@chromium.org
aelias@chromium.org: Please review changes in content/renderer/renderer_blink_platform_impl.cc dcheng@chromium.org: Please review changes in gpu/ipc/common/gpu_command_buffer_traits_multi.h
On 2017/01/05 07:04:57, klausw wrote: > BTW, I think I help how to do the presence check for EGL_IMG_context_priority > (https://www.khronos.org/registry/egl/extensions/IMG/EGL_IMG_context_priority.txt) > properly. As noted in the TODO comment for patchset 7, the extension doesn't > seem > to be showing in the list inside Chrome, and the separate OpenGL-ES Info tool > also doesn't show it: > https://play.google.com/store/apps/details?id=nl.svdree.glesinfo > > However, the extension seems to be mandatory for Daydream-ready devices, and > as far as I can tell it's actually present and working, just not being reported. > See https://source.android.com/compatibility/android-cdd.html . FYI, I added fallback logic that checks for two other extensions that were recently added for Daydream support, and assume that they implies support for context priority. I filed https://github.com/googlevr/gvr-android-sdk/issues/330 to try to figure out what's going on with this extension.
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
content/renderer lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gpu/command_buffer and modules/webgl LGTM % one question. https://codereview.chromium.org/2586803003/diff/160001/gpu/ipc/common/gpu_com... File gpu/ipc/common/gpu_command_buffer_traits_multi.h (right): https://codereview.chromium.org/2586803003/diff/160001/gpu/ipc/common/gpu_com... gpu/ipc/common/gpu_command_buffer_traits_multi.h:148: IPC_STRUCT_TRAITS_MEMBER(own_offscreen_surface) Don't we need to add `low_priority` here?
On 2017/01/06 06:13:00, klausw wrote: > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > File gpu/ipc/service/gpu_command_buffer_stub.cc (right): > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > gpu/ipc/service/gpu_command_buffer_stub.cc:628: use_virtualized_gl_context_ = > false; > On 2017/01/06 03:09:08, jbauman wrote: > > We probably need to actually fail context creation if > > use_virtualized_gl_context_ is true in this case. Otherwise we'll hit a lot of > > driver bugs (which is why we have those workarounds in the first place). > > I don't have a strong opinion about this, but the pre-existing code right above > this (line 608) just sets use_virtualized_gl_context_ to > false if the onscreen buffer's format is incompatible with the default > surface, without raising an error if it was originally set, and that's > a fairly similar scenario. Yeah, that seems bad too. Maybe aelias@ knows if it's likely that we've run into issues with this. > > What happens when it returns false here? Does WebGL content simply fail > to render, and does it report this back to the user in some way? It will fail to create the WebGL context, which the application will detect and can display an error message for.
https://codereview.chromium.org/2586803003/diff/160001/gpu/ipc/common/gpu_com... File gpu/ipc/common/gpu_command_buffer_traits_multi.h (right): https://codereview.chromium.org/2586803003/diff/160001/gpu/ipc/common/gpu_com... gpu/ipc/common/gpu_command_buffer_traits_multi.h:148: IPC_STRUCT_TRAITS_MEMBER(own_offscreen_surface) On 2017/01/06 23:14:04, bajones wrote: > Don't we need to add `low_priority` here? I've added it for consistency. In practice I don't think it matters, looks like the attributes from the helper get used in two different paths. Some (including low_priority) are forked into gl::GLContextAttributes by service_utils's GenerateGLContextAttribs, others are passed down to gpu_command_buffer_stub as init_params.attribs, but in this case this doesn't look at the low_priority attribute.
On 2017/01/06 23:58:15, jbauman wrote: > On 2017/01/06 06:13:00, klausw wrote: > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > File gpu/ipc/service/gpu_command_buffer_stub.cc (right): > > > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > gpu/ipc/service/gpu_command_buffer_stub.cc:628: use_virtualized_gl_context_ = > > false; > > On 2017/01/06 03:09:08, jbauman wrote: > > > We probably need to actually fail context creation if > > > use_virtualized_gl_context_ is true in this case. Otherwise we'll hit a lot > of > > > driver bugs (which is why we have those workarounds in the first place). > > > > I don't have a strong opinion about this, but the pre-existing code right > above > > this (line 608) just sets use_virtualized_gl_context_ to > > false if the onscreen buffer's format is incompatible with the default > > surface, without raising an error if it was originally set, and that's > > a fairly similar scenario. > > Yeah, that seems bad too. Maybe aelias@ knows if it's likely that we've run into > issues with this. > > > > What happens when it returns false here? Does WebGL content simply fail > > to render, and does it report this back to the user in some way? > > It will fail to create the WebGL context, which the application will detect and > can display an error message for. Would it be acceptable to do this as a followup? If yes, I'll file a bug for it and link it in comments. I'm hesitant to change pre-existing behavior as part of this CL without a clear understanding of the impact, and I think it would be safer to do so as an isolated change. I worry that this could cause regressions where some device + site combinations were working in practice (despite disabling the virtualized context) but would now always get an error due to failing the sanity check.
On 2017/01/07 00:13:59, klausw wrote: > On 2017/01/06 23:58:15, jbauman wrote: > > On 2017/01/06 06:13:00, klausw wrote: > > > > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > > File gpu/ipc/service/gpu_command_buffer_stub.cc (right): > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > > gpu/ipc/service/gpu_command_buffer_stub.cc:628: use_virtualized_gl_context_ > = > > > false; > > > On 2017/01/06 03:09:08, jbauman wrote: > > > > We probably need to actually fail context creation if > > > > use_virtualized_gl_context_ is true in this case. Otherwise we'll hit a > lot > > of > > > > driver bugs (which is why we have those workarounds in the first place). > > > > > > I don't have a strong opinion about this, but the pre-existing code right > > above > > > this (line 608) just sets use_virtualized_gl_context_ to > > > false if the onscreen buffer's format is incompatible with the default > > > surface, without raising an error if it was originally set, and that's > > > a fairly similar scenario. > > > > Yeah, that seems bad too. Maybe aelias@ knows if it's likely that we've run > into > > issues with this. > > > > > > What happens when it returns false here? Does WebGL content simply fail > > > to render, and does it report this back to the user in some way? > > > > It will fail to create the WebGL context, which the application will detect > and > > can display an error message for. > > Would it be acceptable to do this as a followup? If yes, I'll file a bug for it > and link it in comments. I'm hesitant to change pre-existing behavior as part of > > this CL without a clear understanding of the impact, and I think it would be > safer > to do so as an isolated change. > > I worry that this could cause regressions where some device + site combinations > were working in practice (despite disabling the virtualized context) but would > now > always get an error due to failing the sanity check. The problem is that this could cause hard-to-root-cause regressions when virtual contexts are suddenly disabled on a lot of systems that need them (if webvr is enabled for a site). At least this would make it more obvious what's going on (if you also put a log message in for this case). I don't think you have to change the existing behavior for the rgb565 check above. Though maybe with that we're getting lucky and few systems both need virtual contexts and are using 565.
On 2017/01/07 00:29:01, jbauman wrote: > On 2017/01/07 00:13:59, klausw wrote: > > On 2017/01/06 23:58:15, jbauman wrote: > > > On 2017/01/06 06:13:00, klausw wrote: > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > > > File gpu/ipc/service/gpu_command_buffer_stub.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > > > gpu/ipc/service/gpu_command_buffer_stub.cc:628: > use_virtualized_gl_context_ > > = > > > > false; > > > > On 2017/01/06 03:09:08, jbauman wrote: > > > > > We probably need to actually fail context creation if > > > > > use_virtualized_gl_context_ is true in this case. Otherwise we'll hit a > > lot > > > of > > > > > driver bugs (which is why we have those workarounds in the first place). > > > > > > > > I don't have a strong opinion about this, but the pre-existing code right > > > above > > > > this (line 608) just sets use_virtualized_gl_context_ to > > > > false if the onscreen buffer's format is incompatible with the default > > > > surface, without raising an error if it was originally set, and that's > > > > a fairly similar scenario. > > > > > > Yeah, that seems bad too. Maybe aelias@ knows if it's likely that we've run > > into > > > issues with this. > > > > > > > > What happens when it returns false here? Does WebGL content simply fail > > > > to render, and does it report this back to the user in some way? > > > > > > It will fail to create the WebGL context, which the application will detect > > and > > > can display an error message for. > > > > Would it be acceptable to do this as a followup? If yes, I'll file a bug for > it > > and link it in comments. I'm hesitant to change pre-existing behavior as part > of > > > > this CL without a clear understanding of the impact, and I think it would be > > safer > > to do so as an isolated change. > > > > I worry that this could cause regressions where some device + site > combinations > > were working in practice (despite disabling the virtualized context) but would > > now > > always get an error due to failing the sanity check. > > The problem is that this could cause hard-to-root-cause regressions when virtual > contexts are suddenly disabled on a lot of systems that need them (if webvr is > enabled for a site). At least this would make it more obvious what's going on > (if you also put a log message in for this case). > > I don't think you have to change the existing behavior for the rgb565 check > above. Though maybe with that we're getting lucky and few systems both need > virtual contexts and are using 565. Argh. I tried failing with "return false" if use_virtualized_gl_context_ is set, and this completely breaks our intended use case. Apparently Android always uses a virtualized context: // MailboxManagerSync synchronization correctness currently depends on having // only a single context. See crbug.com/510243 for details. use_virtualized_gl_context_ |= channel_->mailbox_manager()->UsesSync(); The sync bug was forked to http://crbug.com/554268 and is still unfixed, it's unclear if anyone is working on this. The original bug appeared to just affect WebView, I'm not sure if it would be ok to use the not-virtualized context on non-WebView Chrome if the virtualized context was only activated due to the UsesSync flag and not due to driver bugs.
On 2017/01/07 01:36:47, klausw wrote: > On 2017/01/07 00:29:01, jbauman wrote: > > On 2017/01/07 00:13:59, klausw wrote: > > > On 2017/01/06 23:58:15, jbauman wrote: > > > > On 2017/01/06 06:13:00, klausw wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > > > > File gpu/ipc/service/gpu_command_buffer_stub.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/gpu/ipc/service/gpu_co... > > > > > gpu/ipc/service/gpu_command_buffer_stub.cc:628: > > use_virtualized_gl_context_ > > > = > > > > > false; > > > > > On 2017/01/06 03:09:08, jbauman wrote: > > > > > > We probably need to actually fail context creation if > > > > > > use_virtualized_gl_context_ is true in this case. Otherwise we'll hit > a > > > lot > > > > of > > > > > > driver bugs (which is why we have those workarounds in the first > place). > > > > > > > > > > I don't have a strong opinion about this, but the pre-existing code > right > > > > above > > > > > this (line 608) just sets use_virtualized_gl_context_ to > > > > > false if the onscreen buffer's format is incompatible with the default > > > > > surface, without raising an error if it was originally set, and that's > > > > > a fairly similar scenario. > > > > > > > > Yeah, that seems bad too. Maybe aelias@ knows if it's likely that we've > run > > > into > > > > issues with this. > > > > > > > > > > What happens when it returns false here? Does WebGL content simply fail > > > > > to render, and does it report this back to the user in some way? > > > > > > > > It will fail to create the WebGL context, which the application will > detect > > > and > > > > can display an error message for. > > > > > > Would it be acceptable to do this as a followup? If yes, I'll file a bug for > > it > > > and link it in comments. I'm hesitant to change pre-existing behavior as > part > > of > > > > > > this CL without a clear understanding of the impact, and I think it would be > > > safer > > > to do so as an isolated change. > > > > > > I worry that this could cause regressions where some device + site > > combinations > > > were working in practice (despite disabling the virtualized context) but > would > > > now > > > always get an error due to failing the sanity check. > > > > The problem is that this could cause hard-to-root-cause regressions when > virtual > > contexts are suddenly disabled on a lot of systems that need them (if webvr is > > enabled for a site). At least this would make it more obvious what's going on > > (if you also put a log message in for this case). > > > > I don't think you have to change the existing behavior for the rgb565 check > > above. Though maybe with that we're getting lucky and few systems both need > > virtual contexts and are using 565. > > Argh. I tried failing with "return false" if use_virtualized_gl_context_ is set, > and this completely breaks our intended use case. Apparently Android always uses > a virtualized context: > > // MailboxManagerSync synchronization correctness currently depends on having > // only a single context. See crbug.com/510243 for details. > use_virtualized_gl_context_ |= channel_->mailbox_manager()->UsesSync(); > > The sync bug was forked to http://crbug.com/554268 and is still unfixed, it's > unclear if anyone is working on this. > > The original bug appeared to just affect WebView, I'm not sure if it would be ok > to use the not-virtualized context on non-WebView Chrome if the virtualized > context was only activated due to the UsesSync flag and not due to driver bugs. I thought we would only use a MailboxManagerSync on webview, because that's the only place where --enable-threaded-texture-mailboxes is set. Is there something in regular chrome that's causing it to use MailboxManagerSync? I think not using virtual contexts there is fine because we're not doing any cross-thread rendering where the extra synchronization is necessary. Maybe someday we'll be able to convince android drivers to support EGL_ANGLE_flexible_surface_compatibility so we don't have to deal with this.
Good point, I was assuming this is where it's coming from, but Brandon pointed out that Qualcomm drivers have this workaround enabled, including ~all current Daydream devices. https://cs.chromium.org/chromium/src/gpu/config/gpu_driver_bug_list_json.cc?q... "cr_bugs": [289461], "description": "Non-virtual contexts on Qualcomm sometimes cause out-of-order frames", "os": { "type": "android" }, "gl_vendor": "Qualcomm.*", "features": [ "use_virtualized_gl_contexts" ] I'll add some extra debugging to confirm. What's the best way to proceed here? I'm hoping it's ok to use the non-virtualized context for WebVR. Looks like http://crbug.com/289461 was a somewhat speculative fix from 2013, and I'm not convinced that the actual cause was a driver bug and that it's still applicable. For example, in the mailbox manager sync bug, the underlying issue seemed to be misuse of fence objects. On Fri, Jan 6, 2017 at 5:55 PM, <jbauman@chromium.org> wrote: > On 2017/01/07 01:36:47, klausw wrote: > > On 2017/01/07 00:29:01, jbauman wrote: > > > On 2017/01/07 00:13:59, klausw wrote: > > > > On 2017/01/06 23:58:15, jbauman wrote: > > > > > On 2017/01/06 06:13:00, klausw wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/ > gpu/ipc/service/gpu_command_buffer_stub.cc > > > > > > File gpu/ipc/service/gpu_command_buffer_stub.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/ > gpu/ipc/service/gpu_command_buffer_stub.cc#newcode628 > > > > > > gpu/ipc/service/gpu_command_buffer_stub.cc:628: > > > use_virtualized_gl_context_ > > > > = > > > > > > false; > > > > > > On 2017/01/06 03:09:08, jbauman wrote: > > > > > > > We probably need to actually fail context creation if > > > > > > > use_virtualized_gl_context_ is true in this case. Otherwise > we'll > hit > > a > > > > lot > > > > > of > > > > > > > driver bugs (which is why we have those workarounds in the > first > > place). > > > > > > > > > > > > I don't have a strong opinion about this, but the pre-existing > code > > right > > > > > above > > > > > > this (line 608) just sets use_virtualized_gl_context_ to > > > > > > false if the onscreen buffer's format is incompatible with the > default > > > > > > surface, without raising an error if it was originally set, and > that's > > > > > > a fairly similar scenario. > > > > > > > > > > Yeah, that seems bad too. Maybe aelias@ knows if it's likely that > we've > > run > > > > into > > > > > issues with this. > > > > > > > > > > > > What happens when it returns false here? Does WebGL content > simply > fail > > > > > > to render, and does it report this back to the user in some way? > > > > > > > > > > It will fail to create the WebGL context, which the application > will > > detect > > > > and > > > > > can display an error message for. > > > > > > > > Would it be acceptable to do this as a followup? If yes, I'll file a > bug > for > > > it > > > > and link it in comments. I'm hesitant to change pre-existing > behavior as > > part > > > of > > > > > > > > this CL without a clear understanding of the impact, and I think it > would > be > > > > safer > > > > to do so as an isolated change. > > > > > > > > I worry that this could cause regressions where some device + site > > > combinations > > > > were working in practice (despite disabling the virtualized context) > but > > would > > > > now > > > > always get an error due to failing the sanity check. > > > > > > The problem is that this could cause hard-to-root-cause regressions > when > > virtual > > > contexts are suddenly disabled on a lot of systems that need them (if > webvr > is > > > enabled for a site). At least this would make it more obvious what's > going > on > > > (if you also put a log message in for this case). > > > > > > I don't think you have to change the existing behavior for the rgb565 > check > > > above. Though maybe with that we're getting lucky and few systems both > need > > > virtual contexts and are using 565. > > > > Argh. I tried failing with "return false" if use_virtualized_gl_context_ > is > set, > > and this completely breaks our intended use case. Apparently Android > always > uses > > a virtualized context: > > > > // MailboxManagerSync synchronization correctness currently depends on > having > > // only a single context. See crbug.com/510243 for details. > > use_virtualized_gl_context_ |= channel_->mailbox_manager()->UsesSync(); > > > > The sync bug was forked to http://crbug.com/554268 and is still > unfixed, it's > > unclear if anyone is working on this. > > > > The original bug appeared to just affect WebView, I'm not sure if it > would be > ok > > to use the not-virtualized context on non-WebView Chrome if the > virtualized > > context was only activated due to the UsesSync flag and not due to driver > bugs. > > I thought we would only use a MailboxManagerSync on webview, because > that's the > only place where --enable-threaded-texture-mailboxes is set. Is there > something > in regular chrome that's causing it to use MailboxManagerSync? I think not > using > virtual contexts there is fine because we're not doing any cross-thread > rendering where the extra synchronization is necessary. > > Maybe someday we'll be able to convince android drivers to support > EGL_ANGLE_flexible_surface_compatibility so we don't have to deal with > this. > > https://codereview.chromium.org/2586803003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Good point, I was assuming this is where it's coming from, but Brandon pointed out that Qualcomm drivers have this workaround enabled, including ~all current Daydream devices. https://cs.chromium.org/chromium/src/gpu/config/gpu_driver_bug_list_json.cc?q... "cr_bugs": [289461], "description": "Non-virtual contexts on Qualcomm sometimes cause out-of-order frames", "os": { "type": "android" }, "gl_vendor": "Qualcomm.*", "features": [ "use_virtualized_gl_contexts" ] I'll add some extra debugging to confirm. What's the best way to proceed here? I'm hoping it's ok to use the non-virtualized context for WebVR. Looks like http://crbug.com/289461 was a somewhat speculative fix from 2013, and I'm not convinced that the actual cause was a driver bug and that it's still applicable. For example, in the mailbox manager sync bug, the underlying issue seemed to be misuse of fence objects. On Fri, Jan 6, 2017 at 5:55 PM, <jbauman@chromium.org> wrote: > On 2017/01/07 01:36:47, klausw wrote: > > On 2017/01/07 00:29:01, jbauman wrote: > > > On 2017/01/07 00:13:59, klausw wrote: > > > > On 2017/01/06 23:58:15, jbauman wrote: > > > > > On 2017/01/06 06:13:00, klausw wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/ > gpu/ipc/service/gpu_command_buffer_stub.cc > > > > > > File gpu/ipc/service/gpu_command_buffer_stub.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2586803003/diff/140001/ > gpu/ipc/service/gpu_command_buffer_stub.cc#newcode628 > > > > > > gpu/ipc/service/gpu_command_buffer_stub.cc:628: > > > use_virtualized_gl_context_ > > > > = > > > > > > false; > > > > > > On 2017/01/06 03:09:08, jbauman wrote: > > > > > > > We probably need to actually fail context creation if > > > > > > > use_virtualized_gl_context_ is true in this case. Otherwise > we'll > hit > > a > > > > lot > > > > > of > > > > > > > driver bugs (which is why we have those workarounds in the > first > > place). > > > > > > > > > > > > I don't have a strong opinion about this, but the pre-existing > code > > right > > > > > above > > > > > > this (line 608) just sets use_virtualized_gl_context_ to > > > > > > false if the onscreen buffer's format is incompatible with the > default > > > > > > surface, without raising an error if it was originally set, and > that's > > > > > > a fairly similar scenario. > > > > > > > > > > Yeah, that seems bad too. Maybe aelias@ knows if it's likely that > we've > > run > > > > into > > > > > issues with this. > > > > > > > > > > > > What happens when it returns false here? Does WebGL content > simply > fail > > > > > > to render, and does it report this back to the user in some way? > > > > > > > > > > It will fail to create the WebGL context, which the application > will > > detect > > > > and > > > > > can display an error message for. > > > > > > > > Would it be acceptable to do this as a followup? If yes, I'll file a > bug > for > > > it > > > > and link it in comments. I'm hesitant to change pre-existing > behavior as > > part > > > of > > > > > > > > this CL without a clear understanding of the impact, and I think it > would > be > > > > safer > > > > to do so as an isolated change. > > > > > > > > I worry that this could cause regressions where some device + site > > > combinations > > > > were working in practice (despite disabling the virtualized context) > but > > would > > > > now > > > > always get an error due to failing the sanity check. > > > > > > The problem is that this could cause hard-to-root-cause regressions > when > > virtual > > > contexts are suddenly disabled on a lot of systems that need them (if > webvr > is > > > enabled for a site). At least this would make it more obvious what's > going > on > > > (if you also put a log message in for this case). > > > > > > I don't think you have to change the existing behavior for the rgb565 > check > > > above. Though maybe with that we're getting lucky and few systems both > need > > > virtual contexts and are using 565. > > > > Argh. I tried failing with "return false" if use_virtualized_gl_context_ > is > set, > > and this completely breaks our intended use case. Apparently Android > always > uses > > a virtualized context: > > > > // MailboxManagerSync synchronization correctness currently depends on > having > > // only a single context. See crbug.com/510243 for details. > > use_virtualized_gl_context_ |= channel_->mailbox_manager()->UsesSync(); > > > > The sync bug was forked to http://crbug.com/554268 and is still > unfixed, it's > > unclear if anyone is working on this. > > > > The original bug appeared to just affect WebView, I'm not sure if it > would be > ok > > to use the not-virtualized context on non-WebView Chrome if the > virtualized > > context was only activated due to the UsesSync flag and not due to driver > bugs. > > I thought we would only use a MailboxManagerSync on webview, because > that's the > only place where --enable-threaded-texture-mailboxes is set. Is there > something > in regular chrome that's causing it to use MailboxManagerSync? I think not > using > virtual contexts there is fine because we're not doing any cross-thread > rendering where the extra synchronization is necessary. > > Maybe someday we'll be able to convince android drivers to support > EGL_ANGLE_flexible_surface_compatibility so we don't have to deal with > this. > > https://codereview.chromium.org/2586803003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Brandon was right, debugging shows that workarounds().use_virtualized_gl_contexts is true and mailbox_manager()->UsesSync() is false in the WebVR-on-Android use case. Sorry about the red herring. So we do have the flag on, just not for the reason I was assuming.
On 2017/01/07 02:09:47, klausw wrote: > Good point, I was assuming this is where it's coming from, but Brandon > pointed out that Qualcomm drivers have this workaround enabled, including > ~all current Daydream devices. > > https://cs.chromium.org/chromium/src/gpu/config/gpu_driver_bug_list_json.cc?q... > > "cr_bugs": [289461], "description": "Non-virtual contexts on > Qualcomm sometimes cause out-of-order frames", "os": { > "type": "android" }, "gl_vendor": "Qualcomm.*", > "features": [ "use_virtualized_gl_contexts" ] > I'll add some extra debugging to confirm. > > What's the best way to proceed here? I'm hoping it's ok to use the > non-virtualized context for WebVR. Looks like http://crbug.com/289461 > was a somewhat speculative fix from 2013, and I'm not convinced that > the actual cause was a driver bug and that it's still applicable. For > example, in the mailbox manager sync bug, the underlying issue seemed > to be misuse of fence objects. > Maybe newer drivers have fixed whatever has caused this. We could try (in a separate patch) unblacklisting this on newer driver versions (or at least newer Android versions) to see if that works. Otherwise we might have to figure out how to work around the non-virtual-context synchronization in this driver, because it seems like that could break a lot of things.
Context virtualization is a big hammer we've applied to lots of drivers. It often makes everything somewhat better w.r.t performance, memory usage and stability metrics because most Android drivers aren't designed for the "flip at high frequency back and forth between two contexts sharing resources in the same process" use model which is somewhat unique to Chrome. It's basically a better, safer default than using multiple contexts. That said the problems with multiple-actual-contexts have also been usually of low/moderate severity and show up in aggregate. So I'd be OK with flipping the flag when WebVR is enabled (likely to happen rarely and on high-end devices) but I'm not too interested in adjusting the blacklist at scale.
On 2017/01/09 20:35:22, aelias wrote: > Context virtualization is a big hammer we've applied to lots of drivers. It > often makes everything somewhat better w.r.t performance, memory usage and > stability metrics because most Android drivers aren't designed for the "flip at > high frequency back and forth between two contexts sharing resources in the same > process" use model which is somewhat unique to Chrome. It's basically a better, > safer default than using multiple contexts. > > That said the problems with multiple-actual-contexts have also been usually of > low/moderate severity and show up in aggregate. So I'd be OK with flipping the > flag when WebVR is enabled (likely to happen rarely and on high-end devices) but > I'm not too interested in adjusting the blacklist at scale. Unfortunately we can't specifically restrict this to "when WebVR is enabled". WebVR just uses a normal "webgl" canvas context, at this point there's unfortunately no way to tell at context creation time that the application intends to use this context for WebVR. The current approach requires assuming that potentially any WebGL context may need this, but since WebVR is controlled by an origin trial we limit it to cases where the web site has opted in or the user has manually set the "Enable WebVR" flag in their browser. My understanding is that even if some or all WebGL contexts opt out of virtualization, the majority of Chrome including the compositor would still continue using their virtualized contexts as usual. According to https://crbug.com/289461#c16 the underlying issue was no longer visible after adding glFinish in NativeViewSurfaceEGL::SwapBuffers and this seemed to be a case of missing synchronization. In that case, it sounds fairly unlikely that having WebGL contexts opt out of virtualization would trigger glitches within the rest of Chrome that still uses virtualization, right? Also, the pre-existing case where virtualization got disabled for RGB565 mode on low-spec devices seemed similar, and AFAIK hasn't been causing issues. However, I don't know how often this code path was getting hit.
On 2017/01/09 at 22:00:56, klausw wrote: > My understanding is that even if some or all WebGL contexts opt out of virtualization, the majority of Chrome including the compositor would still continue using their virtualized contexts as usual. According to https://crbug.com/289461#c16 the underlying issue was no longer visible after adding glFinish in NativeViewSurfaceEGL::SwapBuffers and this seemed to be a case of missing synchronization. In that case, it sounds fairly unlikely that having WebGL contexts opt out of virtualization would trigger glitches within the rest of Chrome that still uses virtualization, right? Yeah, it seems OK to have WebGL opt out of virtualization more often. The problems leading us to add the blacklist were typically involving the renderer CC and browser CC having different contexts. A practical compromise would be to split out canvases so that they have separate contexts while all CC instances always share the same context. I'm not sure if the approach in this patch does that, or if there is a risk that disabling the workaround in that spot would have the side effect of splitting off renderer CC as well?
On 2017/01/09 22:14:56, aelias wrote: > Yeah, it seems OK to have WebGL opt out of virtualization more often. The > problems leading us to add the blacklist were typically involving the renderer > CC and browser CC having different contexts. A practical compromise would be to > split out canvases so that they have separate contexts while all CC instances > always share the same context. I'm not sure if the approach in this patch does > that, or if there is a risk that disabling the workaround in that spot would > have the side effect of splitting off renderer CC as well? As far as I can tell this won't have any effect on the renderer/browser CC. This patch does more-or-less what you said, splitting all WebGL canvases into their own contexts but leaving all the compositor contexts alone. (And currently only on pages that subscribe to the Origin Trial or for users that fiddled with about:flags).
OK, sounds good to me then.
On 2017/01/09 22:56:53, bajones wrote: > On 2017/01/09 22:14:56, aelias wrote: > > Yeah, it seems OK to have WebGL opt out of virtualization more often. The > > problems leading us to add the blacklist were typically involving the renderer > > CC and browser CC having different contexts. A practical compromise would be > to > > split out canvases so that they have separate contexts while all CC instances > > always share the same context. I'm not sure if the approach in this patch > does > > that, or if there is a risk that disabling the workaround in that spot would > > have the side effect of splitting off renderer CC as well? > > As far as I can tell this won't have any effect on the renderer/browser CC. This > patch does more-or-less what you said, splitting all WebGL canvases into their > own contexts but leaving all the compositor contexts alone. (And currently only > on pages that subscribe to the Origin Trial or for users that fiddled with > about:flags). lgtm. We'll probably hear about it if this has bad effects on pages in that origin trial.
ipc lgtm https://codereview.chromium.org/2586803003/diff/200001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2586803003/diff/200001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1033: attributes.alpha_size = -1; Nit: I think this is the default value already (https://cs.chromium.org/chromium/src/gpu/command_buffer/common/gles2_cmd_util...), so setting it seems unnecessary. https://codereview.chromium.org/2586803003/diff/200001/gpu/ipc/service/gpu_co... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2586803003/diff/200001/gpu/ipc/service/gpu_co... gpu/ipc/service/gpu_command_buffer_stub.cc:629: // Currently, we always get alpha channe by default. Nit: channe => channel
https://codereview.chromium.org/2586803003/diff/200001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2586803003/diff/200001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1033: attributes.alpha_size = -1; On 2017/01/09 23:14:57, dcheng wrote: > Nit: I think this is the default value already > (https://cs.chromium.org/chromium/src/gpu/command_buffer/common/gles2_cmd_util...), > so setting it seems unnecessary. This matches the pre-patch logic, and in this case I think it's clearer to be explicit. It doesn't care about alpha (leaving it implementation defined, cf. comment in gpu_command_buffer_stub) while it specifically doesn't want depth/stencil/antialiasing. https://codereview.chromium.org/2586803003/diff/200001/gpu/ipc/service/gpu_co... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2586803003/diff/200001/gpu/ipc/service/gpu_co... gpu/ipc/service/gpu_command_buffer_stub.cc:629: // Currently, we always get alpha channe by default. On 2017/01/09 23:14:57, dcheng wrote: > Nit: channe => channel Done, I've also reworded the comment a bit to be clearer.
The CQ bit was checked by klausw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, bajones@chromium.org, dcheng@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2586803003/#ps220001 (title: "Rebase, update comment for alpha semantics")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484005014975960,
"parent_rev": "20e4bfed4baf6f6eac0d7142bb9e763bc11512e4", "commit_rev":
"46b9f6aaaf5851014c2474365734e71ffe9367a0"}
Message was sent while issue was closed.
Description was changed from ========== Enable creation of offscreen contexts which own their backing surface. This is adapted from bajones's patch from issue 2461803002 at patchset 1 (http://crrev.com/2461803002#ps1) 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. Includes support for canvas attributes such as "antialias", and adds a "low priority" GL context option so that postprocessing such as in WebVR frame submit at normal priority won't get blocked on the next frame being drawn. BUG=655722 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 ========== to ========== Enable creation of offscreen contexts which own their backing surface. This is adapted from bajones's patch from issue 2461803002 at patchset 1 (http://crrev.com/2461803002#ps1) 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. Includes support for canvas attributes such as "antialias", and adds a "low priority" GL context option so that postprocessing such as in WebVR frame submit at normal priority won't get blocked on the next frame being drawn. BUG=655722 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/2586803003 Cr-Commit-Position: refs/heads/master@{#442448} Committed: https://chromium.googlesource.com/chromium/src/+/46b9f6aaaf5851014c2474365734... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/46b9f6aaaf5851014c2474365734...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/2628963002/ by mthiesse@chromium.org. The reason for reverting is: Causes out-of-order frames and tearing in WebVR.. |
