The one side effect I can think of from this code that maybe works, is that when this pref changes, perhaps we then notify WebGLRenderingContextBase of the change, and WebGLRenderingContextBase::multisamplingChanged will call forceContextLost(), causing us to recreate the context. Perhaps we have MSAA off in that new context somehow?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp&l=6216

The GpuDataManagerImplPrivate's method that sets the WebPref above is called from:

RenderViewHostImpl::ComputeWebkitPrefs()
RenderViewHostImpl::OnWebkitPreferencesChanged()
RenderViewHostImpl::GetWebkitPreferences() <- This only calls the above a single time
RenderViewHostImpl::CreateRenderView()

So, that path won't change anything later AFAICT.


RenderViewHostImpl::ComputeWebkitPrefs()
RenderViewHostImpl::OnWebkitPreferencesChanged()
RenderProcessHostImpl::RecomputeAndUpdateWebKitPreferences()
RenderProcessHostImpl::OnGpuSwitched()
GpuSwitchingManager::NotifyGpuSwitched()
GpuDataManagerImplPrivate::HandleGpuSwitch()

This looks like it would cause a lost WebGL context in the case of the GPU changing.

RenderViewHostImpl::ComputeWebkitPrefs()
RenderViewHostImpl::OnWebkitPreferencesChanged()
PrefsTabHelper::OnWebPrefChanged
PrefWatcher::OnWebPrefChanged

This looks like it's a callback when people change stuff in chrome://settings? Changes in there wouldn't change the driver workarounds. It's possible they could change the display count, but I don't see calls to SetDisplayCount that could match that, the only caller I can find comes from inside GpuDataManagerPrivateImpl.

DisplayReconfigCallback on OSX: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gpu/gpu_data_manager_impl_private.cc&l=233

And it's set on init time on OSX by calling CGGetActiveDisplayList: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gpu/gpu_data_manager_impl_private.cc&l=1027

It doesn't appear to be set outside of OSX.

GpuDataManagerImplPrivate::HandleGpuSwitch() sends GpuMsg_GpuSwitched to the GPU process. Perhaps we should have the receiver of that lose any gpu::gles2::CONTEXT_TYPE_WEBGL1 or gpu::gles2::CONTEXT_TYPE_WEBGL2 contexts if the driver bugs from #0 are present?

There is one other caller of CGGetActiveDisplayList, at remoting/host/desktop_resizer_mac.cc:161, which doesn't sound like the GPU process.

There is also no other use of DISABLE_CHROMIUM_FRAMEBUFFER_MULTISAMPLE.

I *think* that means we're not checking for this bug anywhere else in order to disable MSAA on the GL context. So even though changing to > 1 displays would cause the WebGL context to be lost, the new context would not be any different than the old one.

I guess I could make us use the WebSetting from WebGLRenderingContextBase to tell DrawingBuffer to not use MSAA.

Do you want me to fix this ignoring of the driver bug that way for now?

Or does this being broken point to us not needing the workaround now?

Or should I leave it broken, rip out the WebSetting (https://codereview.chromium.org/1849023003) and hope that we'll lose the context some other way in the future?

This code does not sound at all familiar (but then again I have a short memory). I don't remember doing anything specific for multi-monitor.

I added a blacklist entry to disable ES3 multisampling on Qualcomm.  ericrk@ added the code that disables MSAA on Intel, but it's only supposed to do so for compositing contexts, and WebGL is supposed to ignore it (as is Pepper, IIRC).

Is it possible that this is a different workaround, that's specific to multimonitor use?

Yes it is, I thought you might have thoughts because of this TODO: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gpu/gpu_data_manager_impl_private.cc&l=777

The TODO came from https://chromium.googlesource.com/chromium/src/+/06b150ca1c0b879ec345b5ee8005b672c5f62667

Break up disable_multisampling blacklist entry.

Split disable_multisampling blacklist entry into
disable_chromium_framebuffer_multisample and
disable_framebuffer_texture_multisample.

BUG= 449116 

This shouldn't be related my intel MSAA blacklist change - that one just sets a value on context caps which is only used by CC.

To be clear, I think this has been doing nothing for a long time, because the query on the GL context for MSAA doesn't care about the default framebuffer, but WebGL code was assuming that changing the default fbo would change the msaa properties reported by the context.

I'm not trying to find a regression here, it's probably been this way for a long time?

I'm wondering what we want to do.

bajones did the multi-monitor workaround a few years back. It's definitely making a difference when people connect a second monitor to a laptop (which leads to corrupted rendering without this workaround). If it's broken and no one complained, maybe we no longer need this workaround for newer OSX?

As for the DISABLE_CHROMIUM_FRAMEBUFFER_MULTISAMPLE workaround, we handle it at gpu command buffer service side. i.e., with this workaround, we no longer report the related extensions. So there is no way the renderer can use multisampling.  The WebPref is unnecessary.

zmo: can you point out where for me?

also, do we lose the context when we need to do the workaround in some other mechanism currently? (like we did here when monitors > 1) or could we do it another way in the service?

https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer/service/feature_info.cc&l=856

I think the multi-monitor workaround is the only one that we have to kill the entire webgl context (otherwise we may leak vram which is a security threat).

Thanks! It looks like we always use the workaround in the service, rather than changing our decision based on the number of monitors, so we don't need to lose context ever and I agree we can kill the setting entirely.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1599e8b2b3b67e470656d8104b8f020559c9da53

commit 1599e8b2b3b67e470656d8104b8f020559c9da53
Author: danakj <danakj@chromium.org>
Date: Tue Apr 05 00:40:47 2016

Remove the MSAA WebSetting, which is not read or used.

The WebSetting *does* appear to do one thing, which is to lose the
WebGL context when we want to stop using MSAA (based on driver bugs
and having > 1 displays on OSX). However.. we'll make a new context
and continue to use MSAA since we don't make that decision based
on the WebSetting at all.

See  crbug.com/599721  for lots of investigation about this.

R=kbr@chromium.org
BUG= 599721 , 584497 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Review URL: https://codereview.chromium.org/1849023003

Cr-Commit-Position: refs/heads/master@{#385065}

[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/content/browser/gpu/gpu_data_manager_impl_private.cc
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/content/public/common/web_preferences.cc
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/content/public/common/web_preferences.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/content/renderer/render_view_impl.cc
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/core/frame/Settings.cpp
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/core/frame/Settings.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/core/frame/SettingsDelegate.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/web/WebSettingsImpl.cpp
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/Source/web/WebSettingsImpl.h
[modify] https://crrev.com/1599e8b2b3b67e470656d8104b8f020559c9da53/third_party/WebKit/public/web/WebSettings.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5fd131d69d33b19d44b9b251d3f062b6daef3b5c

commit 5fd131d69d33b19d44b9b251d3f062b6daef3b5c
Author: danakj <danakj@chromium.org>
Date: Tue Apr 05 19:17:54 2016

Remove DisplayCount from GpuDataManagerImpl.

This was used for the MSAA blacklist, but which had no effect and was
now removed, so the variable is unused.

R=piman@chomium.org
BUG= 599721 ,  584497 

Review URL: https://codereview.chromium.org/1854383002

Cr-Commit-Position: refs/heads/master@{#385250}

[modify] https://crrev.com/5fd131d69d33b19d44b9b251d3f062b6daef3b5c/content/browser/gpu/gpu_data_manager_impl.cc
[modify] https://crrev.com/5fd131d69d33b19d44b9b251d3f062b6daef3b5c/content/browser/gpu/gpu_data_manager_impl.h
[modify] https://crrev.com/5fd131d69d33b19d44b9b251d3f062b6daef3b5c/content/browser/gpu/gpu_data_manager_impl_private.cc
[modify] https://crrev.com/5fd131d69d33b19d44b9b251d3f062b6daef3b5c/content/browser/gpu/gpu_data_manager_impl_private.h