Workaround for Mac video flickering

Workaround for Mac video flickering

The partial swap support on Mac (GLRendererLayerTree) is brittle because
it allows drawing to an IOSurface while that IOSurface is being used as
a CALayer's contents.

This happens to not cause problems because the compositor usually draws
the entire page in one chunk. If we introduce a glFlush to the middle
of this, then we can end up with partially drawn content appearing
on-screen (in the case of the issue that the bug was filed on, black
tiles behind a YouTube video).

The texture deletion in GLImageIOSurface happens in the middle of the
compositor frame being drawn, and causes an implicit glFlush.

Avoid this deletion and implicit glFlush by scoping the textures used
by GLImageIOSurface to the YUVToRGBConverter structure.

The long-term solution is to have the GLRendererLayerTree not support
partial swap, and make BufferQueue not allow re-use of a backbuffer
until IOSurfaceIsInUse is no longer true.

BUG=611551
Committed: https://crrev.com/ded54a2ab9cf36b048b1d9e26f97397aa54dc46f
Cr-Commit-Position: refs/heads/master@{#394290}

[ccameron, 2016-05-16 22:17:03]

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

[ccameron, 2016-05-16 22:17:04]

This is not ideal, but it is the only way to restore the behavior from
https://crrev.com/385690 on the M51 branch before it gets to stable.

Of note is that in earlier reviews you were asking me "should we really delete
these textures every frame". Perhaps we should make these textures less
transient for other reasons as well.

[Daniele Castagna, 2016-05-17 19:29:37]

On 2016/05/16 at 22:17:04, ccameron wrote:
> This is not ideal, but it is the only way to restore the behavior from
https://crrev.com/385690 on the M51 branch before it gets to stable.
> 
> Of note is that in earlier reviews you were asking me "should we really delete
these textures every frame". Perhaps we should make these textures less
transient for other reasons as well.

What was the answer to that?

The patch LGTM.

https://codereview.chromium.org/1980183002/diff/1/ui/gl/yuv_to_rgb_converter.h
File ui/gl/yuv_to_rgb_converter.h (right):

https://codereview.chromium.org/1980183002/diff/1/ui/gl/yuv_to_rgb_converter....
ui/gl/yuv_to_rgb_converter.h:26: // TODO(ccameron): Fix the underlying but
whereby inoppurtune glFlushes
I think this patch makes sense by itself even if it weren't fixing the bug.
So, while this comment and TODO are useful, I don't think they're necessary
here.
Up to you if you want to leave them or remove them.

https://codereview.chromium.org/1980183002/diff/1/ui/gl/yuv_to_rgb_converter....
ui/gl/yuv_to_rgb_converter.h:38: 
nit: no need for the blank line.

[ccameron, 2016-05-17 22:52:19]

On 2016/05/17 19:29:37, Daniele Castagna wrote:
> On 2016/05/16 at 22:17:04, ccameron wrote:
> > This is not ideal, but it is the only way to restore the behavior from
> https://crrev.com/385690 on the M51 branch before it gets to stable.
> > 
> > Of note is that in earlier reviews you were asking me "should we really
delete
> these textures every frame". Perhaps we should make these textures less
> transient for other reasons as well.
> 
> What was the answer to that?

We probably shouldn't have them be transient.

> The patch LGTM.
> 
> https://codereview.chromium.org/1980183002/diff/1/ui/gl/yuv_to_rgb_converter.h
> File ui/gl/yuv_to_rgb_converter.h (right):
> 
>
https://codereview.chromium.org/1980183002/diff/1/ui/gl/yuv_to_rgb_converter....
> ui/gl/yuv_to_rgb_converter.h:26: // TODO(ccameron): Fix the underlying but
> whereby inoppurtune glFlushes
> I think this patch makes sense by itself even if it weren't fixing the bug.
> So, while this comment and TODO are useful, I don't think they're necessary
> here.
> Up to you if you want to leave them or remove them.

Removed the TODOs.

>
https://codereview.chromium.org/1980183002/diff/1/ui/gl/yuv_to_rgb_converter....
> ui/gl/yuv_to_rgb_converter.h:38: 
> nit: no need for the blank line.

Done.

[ccameron, 2016-05-17 22:52:28]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-05-17 22:52:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1980183002/#ps40001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2016-05-17 22:53:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1980183002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1980183002/40001

[commit-bot: I haz the power, 2016-05-18 00:36:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-18 00:38:32]

Description was changed from

==========
Workaround for Mac video flickering

The partial swap support on Mac (GLRendererLayerTree) is brittle because
it allows drawing to an IOSurface while that IOSurface is being used as
a CALayer's contents.

This happens to not cause problems because the compositor usually draws
the entire page in one chunk. If we introduce a glFlush to the middle
of this, then we can end up with partially drawn content appearing
on-screen (in the case of the issue that the bug was filed on, black
tiles behind a YouTube video).

The texture deletion in GLImageIOSurface happens in the middle of the
compositor frame being drawn, and causes an implicit glFlush.

Avoid this deletion and implicit glFlush by scoping the textures used
by GLImageIOSurface to the YUVToRGBConverter structure.

The long-term solution is to have the GLRendererLayerTree not support
partial swap, and make BufferQueue not allow re-use of a backbuffer
until IOSurfaceIsInUse is no longer true.

BUG=611551
==========

to

==========
Workaround for Mac video flickering

The partial swap support on Mac (GLRendererLayerTree) is brittle because
it allows drawing to an IOSurface while that IOSurface is being used as
a CALayer's contents.

This happens to not cause problems because the compositor usually draws
the entire page in one chunk. If we introduce a glFlush to the middle
of this, then we can end up with partially drawn content appearing
on-screen (in the case of the issue that the bug was filed on, black
tiles behind a YouTube video).

The texture deletion in GLImageIOSurface happens in the middle of the
compositor frame being drawn, and causes an implicit glFlush.

Avoid this deletion and implicit glFlush by scoping the textures used
by GLImageIOSurface to the YUVToRGBConverter structure.

The long-term solution is to have the GLRendererLayerTree not support
partial swap, and make BufferQueue not allow re-use of a backbuffer
until IOSurfaceIsInUse is no longer true.

BUG=611551

Committed: https://crrev.com/ded54a2ab9cf36b048b1d9e26f97397aa54dc46f
Cr-Commit-Position: refs/heads/master@{#394290}
==========

[commit-bot: I haz the power, 2016-05-18 00:38:34]

Patchset 3 (id:??) landed as
https://crrev.com/ded54a2ab9cf36b048b1d9e26f97397aa54dc46f
Cr-Commit-Position: refs/heads/master@{#394290}