Mac: Recreate browser-side GL contexts on GPU switch

Mac: Recreate browser-side GL contexts on GPU switch

When a GPU switch is detected, poison all browser-side GL contexts,
which will cause them to be recreated.

Make CompositorSwapBuffers destroy and recreate the GL context
if it has been poisoned. Ensure that the CALayer for the old content
remain in place until the new layer is in place and correctly sized,
to avoid flickering.

BUG=361493
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262901

[ccameron, 2014-04-09 22:06:01]

ptal

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_layer_mac.mm (left):

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:47: [self
removeFromSuperlayer];
This was always done by the caller, so it was redundant.

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:2053: if
(compositing_iosurface_ && !use_core_animation_) {
There is no migration in non-CoreAnimation mode, so this did nothing.

[Zhenyao Mo, 2014-04-09 22:24:23]

In general this looks good, but I am not very familiar with the code this CL
touched, so be very careful.

One thing I highly recommend is, right now the OnGpuSwitch() is being fired in
situations other than a gpu switch.

If you look at GpuDataManagerImplPrivate::HandleGpuSwitch(), there is logic
detecting whether truly it's a different active GPU.  So we should add a param
to OnGpuSwitch() function, and don't trigger this contamination thing if it's
not truly a gpu change.

But if you want to do that in a separate CL, I can stamp this one.

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_context_mac.mm (right):

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_context_mac.mm:15: #include
"content/public/browser/gpu_data_manager.h"
Usually inside content side, we directly use gpu_data_manager_impl.h instead.

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_context_mac.mm:187:
content::GpuDataManager::GetInstance()->AddObserver(this);
nit:no need for content namespace, and use GpuDataManagerImpl instead.

same below.

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:383: void
RemoveLayerFromSuperlayer(
This is never called.

[ccameron, 2014-04-09 22:35:41]

On 2014/04/09 22:24:23, Zhenyao Mo wrote:
> In general this looks good, but I am not very familiar with the code this CL
> touched, so be very careful.
> 
> One thing I highly recommend is, right now the OnGpuSwitch() is being fired in
> situations other than a gpu switch.
> 
> If you look at GpuDataManagerImplPrivate::HandleGpuSwitch(), there is logic
> detecting whether truly it's a different active GPU.  So we should add a param
> to OnGpuSwitch() function, and don't trigger this contamination thing if it's
> not truly a gpu change.
> 
> But if you want to do that in a separate CL, I can stamp this one.
> 
>
https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/compositing_iosurface_context_mac.mm
(right):
> 
>
https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/compositing_iosurface_context_mac.mm:15:
#include
> "content/public/browser/gpu_data_manager.h"
> Usually inside content side, we directly use gpu_data_manager_impl.h instead.
> 
>
https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/compositing_iosurface_context_mac.mm:187:
> content::GpuDataManager::GetInstance()->AddObserver(this);
> nit:no need for content namespace, and use GpuDataManagerImpl instead.
> 
> same below.
> 
>
https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_mac.mm:383: void
> RemoveLayerFromSuperlayer(
> This is never called.

[ccameron, 2014-04-09 22:37:27]

> If you look at GpuDataManagerImplPrivate::HandleGpuSwitch(), there is logic
> detecting whether truly it's a different active GPU.  So we should add a param
> to OnGpuSwitch() function, and don't trigger this contamination thing if it's
> not truly a gpu change.

> But if you want to do that in a separate CL, I can stamp this one.

I'm content to be over-aggressive with re-creating the contexts. The overhead of
doing so is high enough that, if we do it every frame, it's hard to hit 60fps
(but possible), but low enough that it should be imperceptible to the user.

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_context_mac.mm (right):

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_context_mac.mm:15: #include
"content/public/browser/gpu_data_manager.h"
On 2014/04/09 22:24:24, Zhenyao Mo wrote:
> Usually inside content side, we directly use gpu_data_manager_impl.h instead.

Done.

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_context_mac.mm:187:
content::GpuDataManager::GetInstance()->AddObserver(this);
On 2014/04/09 22:24:24, Zhenyao Mo wrote:
> nit:no need for content namespace, and use GpuDataManagerImpl instead.
> 
> same below.

Done.

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/231863002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:383: void
RemoveLayerFromSuperlayer(
On 2014/04/09 22:24:24, Zhenyao Mo wrote:
> This is never called.

This is wrapped up in the ScopedClosureRunner below, which executes it at the
end of the function.

[Zhenyao Mo, 2014-04-09 22:47:24]

LGTM

https://codereview.chromium.org/231863002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/231863002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:591:
[compositing_iosurface_layer_ removeFromSuperlayer];
nit: this could call the helper function above to avoid code duplication.

[ccameron, 2014-04-09 23:15:15]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-04-09 23:16:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/231863002/60001

[commit-bot: I haz the power, 2014-04-09 23:54:20]

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

[commit-bot: I haz the power, 2014-04-09 23:54:21]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[ccameron, 2014-04-09 23:54:38]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-04-09 23:55:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/231863002/60001

[commit-bot: I haz the power, 2014-04-10 02:18:35]

Change committed as 262901