Mac: Fix black flashing during tab switch

Mac: Fix black flashing during tab switch

This was not a new bug, but rather a worse manifestation of an existing
bug.

The existing bug occurred when a ui::Compositor was recycled. That is,
when the compositor was detached from one NSView and then attached to a
different NSView.

The desired behavior is that, at the instant that the ui::Compositor is
detached (in particular, ImageTransportFactory::
SetCompositorSuspendedForRecycle, called OnCompositorRecycled before
this patch) is called, no more frames should be drawn into the
compositor's output surface.

This was previously accomplished by doing two things
1. Locking the ui::Compositor until the new NSView has a frame for it.
2. Making GpuProcessHostUIShim::OnAcceleratedSurfaceBuffersSwapped
   not draw any frames that come in from the GPU process until after a
   call to GpuBrowserCompositorOutputSurface::SwapBuffers has occurred.
   This is to prevent swaps that were issued by the browser prior to
   the ui::Compositor being recycled from being shown.

The assumption here was that the call to
GpuBrowserCompositorOutputSurface::SwapBuffers would only be in
response to new frame belonging to the new NSView. This is not a valid
assumption now that the ui::Compositor uses a cc::Scheduler. In
particular, the ui::Compositor may draw frames that have already been
committed, even while it is locked. These previously-committed frames
will call SwapBuffers when they are being drawn (which the logic will
erroneously assume means that a frame from the new NSView is ready).

To fix this, change this logic to make the output surface not draw any
frames until the ui::Compositor commits a new frame and
GpuBrowserCompositorOutputSurface::SwapBuffers is called.

Also, the DiscardBackbuffer which is done to ensure that the same
CALayer is not sent to two different tabs is now done immediately
after the ui::Compositor does its commit for the new NSView (but
before it does a draw) instead of being done immediately when the
old NSView is removed.

The old location was incorrect -- if there was a draw in flight, it would
cause the backbuffer to be reallocated, and then the same CALayer
would be used for the two tabs. Also, if the draw was a partial swap,
then that frame would have black in the areas that were not drawn by
the partial swap -- then, because the same CALayer was being used
for the two tabs, we would get a flash of the semi-black frame in the
new tab.

BUG=477413
Committed: https://crrev.com/8d5d7278d7e06214bb87f170cda5c9ce9cf0c325
Cr-Commit-Position: refs/heads/master@{#326181}

[ccameron, 2015-04-21 00:58:10]

ccameron@chromium.org changed reviewers:
+ jbauman@chromium.org

[ccameron, 2015-04-21 01:06:19]

Okay, after some reflection, I think this is the correct way to go. Calling
ui::Compositor::SetVisible(false) isn't enough to prevent in-flight draws from
coming through.

This (which is slightly simpler than the previous incarnation) in that it
- ensures that we discard the backbuffer at an appropriate time (soas to not
create mangled partial swaps)
- doesn't skip the SwapBuffers calls (it just tosses out the CALayers when they
reach the browser).

[jbauman, 2015-04-21 02:22:57]

On 2015/04/21 01:06:19, ccameron wrote:
> Okay, after some reflection, I think this is the correct way to go. Calling
> ui::Compositor::SetVisible(false) isn't enough to prevent in-flight draws from
> coming through.
> 
> This (which is slightly simpler than the previous incarnation) in that it
> - ensures that we discard the backbuffer at an appropriate time (soas to not
> create mangled partial swaps)
> - doesn't skip the SwapBuffers calls (it just tosses out the CALayers when
they
> reach the browser).

Could this still have a race condition where a draw/swapbuffers start/are in
flight, a commit happens, and then the swap for the old frame happens in the GPU
process?

[ccameron, 2015-04-21 03:04:49]

On 2015/04/21 02:22:57, jbauman wrote:
> On 2015/04/21 01:06:19, ccameron wrote:
> > Okay, after some reflection, I think this is the correct way to go. Calling
> > ui::Compositor::SetVisible(false) isn't enough to prevent in-flight draws
from
> > coming through.
> > 
> > This (which is slightly simpler than the previous incarnation) in that it
> > - ensures that we discard the backbuffer at an appropriate time (soas to not
> > create mangled partial swaps)
> > - doesn't skip the SwapBuffers calls (it just tosses out the CALayers when
> they
> > reach the browser).
> 
> Could this still have a race condition where a draw/swapbuffers start/are in
> flight, a commit happens, and then the swap for the old frame happens in the
GPU
> process?

Don't think so -- the commits are throttled by the swaps being acked back to the
browser, so we can be sure that, once the "suspended for recycle" flag is unset,
the next swap is new content

[ccameron, 2015-04-21 22:40:07]

On 2015/04/21 03:04:49, ccameron wrote:
> On 2015/04/21 02:22:57, jbauman wrote:
> > On 2015/04/21 01:06:19, ccameron wrote:
> > > Okay, after some reflection, I think this is the correct way to go.
Calling
> > > ui::Compositor::SetVisible(false) isn't enough to prevent in-flight draws
> from
> > > coming through.
> > > 
> > > This (which is slightly simpler than the previous incarnation) in that it
> > > - ensures that we discard the backbuffer at an appropriate time (soas to
not
> > > create mangled partial swaps)
> > > - doesn't skip the SwapBuffers calls (it just tosses out the CALayers when
> > they
> > > reach the browser).
> > 
> > Could this still have a race condition where a draw/swapbuffers start/are in
> > flight, a commit happens, and then the swap for the old frame happens in the
> GPU
> > process?
> 
> Don't think so -- the commits are throttled by the swaps being acked back to
the
> browser, so we can be sure that, once the "suspended for recycle" flag is
unset,
> the next swap is new content

I looked into the scheduler logic, and while I think this is the case, there
appear to be plans to make this not be the case. I've changed the logic to wait
for a commit and then wait for a swap, and only after that allow drawing.

[jbauman, 2015-04-21 22:43:45]

lgtm

[ccameron, 2015-04-21 23:57:12]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-04-21 23:57:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097223002/20001

[commit-bot: I haz the power, 2015-04-22 00:01:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-22 00:02:42]

Patchset 2 (id:??) landed as
https://crrev.com/8d5d7278d7e06214bb87f170cda5c9ce9cf0c325
Cr-Commit-Position: refs/heads/master@{#326181}