Mac Overlays: Add GPU back-pressure

Mac Overlays: Add GPU back-pressure

Issue a GL fence after every SwapBuffers. Do not call -[CALayer
setContents:] with the content being rendered by that SwapBuffers until
the fence has passed.

Query the previous's frame's GL fence at the beginning of each
SwapBuffers. Also issue a callback to query the GL fence at the
mid-point of the VSync period (if there is no subsequent SwapBuffers,
or if the frame takes more than one VSync to render).

Note that waiting for the GL fence to complete before calling -[CALayer
setContents] is not required for correctness -- only the expected content
(everything before the glFlush) will appear in the layer. Rather, the
reason for waiting for the GL fence is to make the time at which the
content will appear on-screen more reliable.

Because there may be multiple calls to SwapBuffers in flight, store the
data necessary to call -[CALayer setContents] in a PendingSwap
structure. Maintain a queue of these structures.

Because the ImageTransportSurface does not know anything about the
VSync period, send the CGDirectDisplayID for the attached display to the
ImageTransportSurface in the AcceleratedSurfaceMsg_BufferPresented IPC
(which is where the CGL renderer ID is already communicated). Send this
information instead of the raw VSync parameters so that the updates to
all windows on a single display may be coalesced into a single callback
in the future.

Note that this display is the display that is used for vsync by the
RenderWidgetHostViewMac, so if vsync is disabled, it will be 0.

Separate gfx::ScopedSetGLToRealGLApi into its own header file to
simplify header orders.

Access the IOSurface of the GLImageIOSurface directly, rather than
using its ScheduleOverlayPlane method. This simplifies things
immediately, in that we can reason about the underlying IOSurface's
lifetime better than weak pointers to images. It will also simplify
the implementation of partial swap and multiple overlays.

BUG=515696
Committed: https://crrev.com/5c207cac73c835b5f362670d6b46333dfae1f4c4
Cr-Commit-Position: refs/heads/master@{#342548}

Committed: https://crrev.com/01ae4cbc31c1092b8c0ed15ff3a9773badfa278a
Cr-Commit-Position: refs/heads/master@{#342603}

[ccameron, 2015-08-05 03:04:41]

ccameron@chromium.org changed reviewers:
+ kbr@chromium.org, tapted@chromium.org

[ccameron, 2015-08-05 03:04:41]

This turned out to be fairly clean. Let me know if there are any concerns about
the use of the GL_APPLE_fence extension this way.

[tapted, 2015-08-05 07:55:57]

didn't quite finish - gotta run. (lots of the opengl stuff is still over my head
too, but I had a go absorbing it all).

The changes in ui/views/cocoa lg, and I patched this in to do some spot checks
-- they all checked out too.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:56: struct PendingSwap
{
Since it's private, you could forward declare this, and just have

class ImageTransportSurfaceOverlayMac::PendingSwap {
..
};

declared in the .mm

That would improve encapsulation and allow you to inline the
constructor/destructor without upsetting clang's chromium-style plugins. Many of
the added #includes could move to the .mm too.

(and the std::deque should still work with a forward dec since
ImageTransportSurfaceOverlayMac's constructor/destructor are out of line)

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:64: GLuint fence;
could this be a scoped_ptr<GLFence>? (or GLFenceAPPLE). That would break
copyability and upset std::deque though. (just wondering whether there's benefit
reusing code in ui/gl/gl_fence_apple.h, but doesn't really look like it .. maybe
with cc:ScopedPtrDeque?)

Maybe something like, in ~ImageTransportSurfaceOverlayMac(),
DCHECK(pending_swaps_.empty()); is assurance enough that Destroy() cleans
everything up and the fence isn't leaked.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:75: // send the content
to the window server by -[CALayer setContents:]. If
If... (sentence fragment)

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:78: // Post a call to
CheckAndDisplayPendingSwaps at the next midpoint of a
optional: I usually see "Foo()" in comments to refer to functions (a no-parens
"Foo" for a class). More below. But also  there's precedent for no-parens in
this file, so that's fine too.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:19: #define
LOG_GL_ERRORS(msg) \
Not sure a macro is right for this. There's a `ClearGLErrors` in
android_webview/browser/scoped_app_gl_state_restore.cc which is similar.
gl_fence_arb.cc also has something (less similar).

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:118: bool result =
!glIsFenceAPPLE(this_swap.fence) ||
Should `glIsFenceAPPLE` be a DCHECK?

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:136: [layer_
setFrame:gfx::Rect(this_swap.dip_size).ToCGRect()];
I wonder if there's a downside to calling this when the size hasn't changed..
(I'm often surprised with the lack of early exit cases in foundation libraries
when I've traced such things)

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:153:
CheckAndDisplayPendingSwaps(true);
tail recursion scares me a bit - maybe a wrapper function that loops while
!empty?

https://codereview.chromium.org/1273563002/diff/40001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/40001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:115: return
view_->AcceleratedWidgetGetDisplayID();
is this always the same as

[[[[[AcceleratedWidgetGetNSView() window] screen] deviceDescription]
objectForKey:@"NSScreenNumber"] intValue];

(not suggesting we use that - but it might be something to use in
BridgedNativeWidgdet)

[ccameron, 2015-08-05 21:00:43]

Thanks -- updated.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:56: struct PendingSwap
{
On 2015/08/05 07:55:56, tapted wrote:
> Since it's private, you could forward declare this, and just have
> 
> class ImageTransportSurfaceOverlayMac::PendingSwap {
> ..
> };
> 
> declared in the .mm
> 
> That would improve encapsulation and allow you to inline the
> constructor/destructor without upsetting clang's chromium-style plugins. Many
of
> the added #includes could move to the .mm too.
> 
> (and the std::deque should still work with a forward dec since
> ImageTransportSurfaceOverlayMac's constructor/destructor are out of line)

Done.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:64: GLuint fence;
On 2015/08/05 07:55:57, tapted wrote:
> could this be a scoped_ptr<GLFence>? (or GLFenceAPPLE). That would break
> copyability and upset std::deque though. (just wondering whether there's
benefit
> reusing code in ui/gl/gl_fence_apple.h, but doesn't really look like it ..
maybe
> with cc:ScopedPtrDeque?)
> 
> Maybe something like, in ~ImageTransportSurfaceOverlayMac(),
> DCHECK(pending_swaps_.empty()); is assurance enough that Destroy() cleans
> everything up and the fence isn't leaked.

I'll use a GLFenceAPPLE.

I don't really trust the generic GLFence (GL_ARB_sync) on Mac -- I'd rather not
use it if I don't have to (core profile issues need a check).

Also of note is that by inspection, GLFenceAPPLE::ServerWait unnecessarily calls
ClientWait -- all system-wide OpenGL calls are ordered by their glFlush() order.
Doing nothing in ServerWait is sufficient.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:75: // send the content
to the window server by -[CALayer setContents:]. If
On 2015/08/05 07:55:56, tapted wrote:
> If... (sentence fragment)

Meant to discuss force_immediate_display. Added.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:78: // Post a call to
CheckAndDisplayPendingSwaps at the next midpoint of a
On 2015/08/05 07:55:57, tapted wrote:
> optional: I usually see "Foo()" in comments to refer to functions (a no-parens
> "Foo" for a class). More below. But also  there's precedent for no-parens in
> this file, so that's fine too.

Good point. If the style guide has a remark on this, happy to follow it (leaving
for now -- there's a similar inconsistency with |variable_name_| in comments).

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:19: #define
LOG_GL_ERRORS(msg) \
On 2015/08/05 07:55:57, tapted wrote:
> Not sure a macro is right for this. There's a `ClearGLErrors` in
> android_webview/browser/scoped_app_gl_state_restore.cc which is similar.
> gl_fence_arb.cc also has something (less similar).

Changed this to a function.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:118: bool result =
!glIsFenceAPPLE(this_swap.fence) ||
On 2015/08/05 07:55:57, tapted wrote:
> Should `glIsFenceAPPLE` be a DCHECK?

Done.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:136: [layer_
setFrame:gfx::Rect(this_swap.dip_size).ToCGRect()];
On 2015/08/05 07:55:57, tapted wrote:
> I wonder if there's a downside to calling this when the size hasn't changed..
> (I'm often surprised with the lack of early exit cases in foundation libraries
> when I've traced such things)

Sounds like a good idea. Updated.

https://codereview.chromium.org/1273563002/diff/40001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:153:
CheckAndDisplayPendingSwaps(true);
On 2015/08/05 07:55:57, tapted wrote:
> tail recursion scares me a bit - maybe a wrapper function that loops while
> !empty?

Sounds reasonable. Done.

https://codereview.chromium.org/1273563002/diff/40001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/40001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:115: return
view_->AcceleratedWidgetGetDisplayID();
On 2015/08/05 07:55:57, tapted wrote:
> is this always the same as
> 
> [[[[[AcceleratedWidgetGetNSView() window] screen] deviceDescription]
> objectForKey:@"NSScreenNumber"] intValue];
> 
> (not suggesting we use that - but it might be something to use in
> BridgedNativeWidgdet)

Almost always -- the display_id can be set to 0 if vsync is to be disabled. I'll
update the method name to include that detail.

[Ken Russell (switch to Gerrit), 2015-08-05 21:55:24]

This LGTM overall -- your CL description addressed every major question I had
reading through the code (handling multiple overlays in the future, handling
multiple displays, etc.). Couple of minor questions.

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:206: delay);
This will busy-loop while the browser is waiting for the pending swaps to
complete, correct? I wonder whether we can get into pathological situations
where every new swapped frame just misses the fence check above, causing this
delayed task to be issued for every frame.

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:211:
CheckAndDisplayPendingSwaps(true);
Would it better match the intended behavior to actually wait for the
most-recently-issued fence to be completed, rather than just ignoring whether
they've passed or not? I'm not arguing for doing this, only asking. Your current
approach is probably less prone to strange deadlocks.

[tapted, 2015-08-06 00:56:49]

lgtm % nits

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:9: #include
<IOSurface/IOSurface.h>
nit: I think IOSurface, CGLTypes and scoped_cftyperef can move to the .mm

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:197: timebase + (1 +
(now - timebase) / interval) * interval;
nit: indent 2 extra spaces

https://codereview.chromium.org/1273563002/diff/60001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/display_link_mac.cc (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/display_link_mac.cc:31: return NULL;
nit: nullptr

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/gl_gl_api_impleme...
File ui/gl/gl_gl_api_implementation.h (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.h:39: GLApi* GetCurrentGLApi();
remove stray? (I don't think this is defined)

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.cc
File ui/gl/scoped_api.cc (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.cc#new...
ui/gl/scoped_api.cc:22: 
nit: remove extra newlines (maybe this was even the trigger for `git cl patch`
whining about whitespace errors..)

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.h
File ui/gl/scoped_api.h (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.h#newc...
ui/gl/scoped_api.h:21: };
nit: DISALLOW_COPY_AND_ASSIGN(..

[ccameron, 2015-08-06 01:42:46]

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

[ccameron, 2015-08-06 01:42:47]

Thanks!

Adding jbauman@ to stamp gpu/command_buffer/service

Adding dcheng@ for IPC stamp

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.h:9: #include
<IOSurface/IOSurface.h>
On 2015/08/06 00:56:49, tapted wrote:
> nit: I think IOSurface, CGLTypes and scoped_cftyperef can move to the .mm

Done.

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:197: timebase + (1 +
(now - timebase) / interval) * interval;
On 2015/08/06 00:56:49, tapted wrote:
> nit: indent 2 extra spaces

Done.

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:206: delay);
On 2015/08/05 21:55:23, Ken Russell wrote:
> This will busy-loop while the browser is waiting for the pending swaps to
> complete, correct? I wonder whether we can get into pathological situations
> where every new swapped frame just misses the fence check above, causing this
> delayed task to be issued for every frame.

That could very well happen -- the effect of it would be that there would be 1
extra frame of latency before content appears on-screen, which I think is okay.

https://codereview.chromium.org/1273563002/diff/60001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:211:
CheckAndDisplayPendingSwaps(true);
On 2015/08/05 21:55:23, Ken Russell wrote:
> Would it better match the intended behavior to actually wait for the
> most-recently-issued fence to be completed, rather than just ignoring whether
> they've passed or not? I'm not arguing for doing this, only asking. Your
current
> approach is probably less prone to strange deadlocks.

Doing a finish on the GLFenceAPPLE would have this effect.

One thing that I didn't make clear in the CL description (fixing that) is that I
do the GLFence so that the timing of things showing up on-screen is more
predictable (and also for back-pressure).

If you do
  glDoSomethingReallyComplicatedAndSlow()
  glFlush
  [myLayer setContentsTo:complicated_thing_just_drawn]
then you will always see the correct content -- it's just that it may be a while
before it actually appears on-screen (which makes aligning with vsync hard).

https://codereview.chromium.org/1273563002/diff/60001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/display_link_mac.cc (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/display_link_mac.cc:31: return NULL;
On 2015/08/06 00:56:49, tapted wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/gl_gl_api_impleme...
File ui/gl/gl_gl_api_implementation.h (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.h:39: GLApi* GetCurrentGLApi();
On 2015/08/06 00:56:49, tapted wrote:
> remove stray? (I don't think this is defined)

This is needed by ScopedSetGLToRealGLApi (it used to be an effectively-static
function, now it's needed in another file).

[jbauman, 2015-08-06 01:53:10]

gpu/command_buffer/service lgtm, though I'd like one change for
content/common/gpu.

https://codereview.chromium.org/1273563002/diff/80001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/80001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:51:
scoped_ptr<gfx::GLFenceAPPLE> fence;
I'd prefer if you put a std::vector<ui::LatencyInfo> in here, then moved the
latency_info from the ImageTransportSurfaceOverlayMac into it on creation and
from it into the GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params. That way
the latency is a bit more representative of what's actually going on.

[ccameron, 2015-08-06 02:27:39]

Thanks!

https://codereview.chromium.org/1273563002/diff/80001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/80001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:51:
scoped_ptr<gfx::GLFenceAPPLE> fence;
On 2015/08/06 01:53:09, jbauman wrote:
> I'd prefer if you put a std::vector<ui::LatencyInfo> in here, then moved the
> latency_info from the ImageTransportSurfaceOverlayMac into it on creation and
> from it into the GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params. That way
> the latency is a bit more representative of what's actually going on.

Good point -- done.

[dcheng, 2015-08-07 00:28:37]

IPC changes LGTM but one thought about making it easy to convert a move-based
approach later.

https://codereview.chromium.org/1273563002/diff/100001/content/common/gpu/ima...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1273563002/diff/100001/content/common/gpu/ima...
content/common/gpu/image_transport_surface_overlay_mac.h:87:
std::list<scoped_refptr<PendingSwap>> pending_swaps_;
How about using linked_ptr instead of refcounting? Once we can use unique_ptr,
it'll be straightforward to programmatically convert it.

[ccameron, 2015-08-07 22:14:16]

Thanks!

I went through the patch and cleaned up some of the functions (split them into
smaller pieces).

I also found that checking if the previous frame is ready just before issuing
the compositor's glFlush is very helpful for improving smoothness of animations,
so I incorporated this change.

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.cc
File ui/gl/scoped_api.cc (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.cc#new...
ui/gl/scoped_api.cc:22: 
On 2015/08/06 00:56:49, tapted wrote:
> nit: remove extra newlines (maybe this was even the trigger for `git cl patch`
> whining about whitespace errors..)

Done.

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.h
File ui/gl/scoped_api.h (right):

https://codereview.chromium.org/1273563002/diff/60001/ui/gl/scoped_api.h#newc...
ui/gl/scoped_api.h:21: };
On 2015/08/06 00:56:49, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..

Done.

https://codereview.chromium.org/1273563002/diff/100001/content/common/gpu/ima...
File content/common/gpu/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/1273563002/diff/100001/content/common/gpu/ima...
content/common/gpu/image_transport_surface_overlay_mac.h:87:
std::list<scoped_refptr<PendingSwap>> pending_swaps_;
On 2015/08/07 00:28:37, dcheng wrote:
> How about using linked_ptr instead of refcounting? Once we can use unique_ptr,
> it'll be straightforward to programmatically convert it.

Done.

[ccameron, 2015-08-08 05:35:52]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-08-08 05:35:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
kbr@chromium.org, jbauman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1273563002/#ps140001
(title: "And rebase")

[commit-bot: I haz the power, 2015-08-08 05:35:56]

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

[commit-bot: I haz the power, 2015-08-08 06:16:18]

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

[commit-bot: I haz the power, 2015-08-08 06:16:19]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[ccameron, 2015-08-08 18:11:17]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-08-08 18:11:19]

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

[commit-bot: I haz the power, 2015-08-08 18:47:43]

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

[commit-bot: I haz the power, 2015-08-08 18:47:44]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[ccameron, 2015-08-08 19:02:02]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-08-08 19:02:05]

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

[commit-bot: I haz the power, 2015-08-08 19:38:48]

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

[commit-bot: I haz the power, 2015-08-08 19:38:49]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[ccameron, 2015-08-09 00:32:40]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-08-09 00:32:43]

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

[commit-bot: I haz the power, 2015-08-09 01:04:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-09 01:05:28]

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

[Ken Russell (switch to Gerrit), 2015-08-09 18:54:20]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1280033004/ by kbr@chromium.org.

The reason for reverting is: This broke the WebGL 2.0 prototype on Mac OS --
please see https://code.google.com/p/chromium/issues/detail?id=515696#c16 for
details.
.

[ccameron, 2015-08-10 10:14:27]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-08-10 10:14:27]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
dcheng@chromium.org, jbauman@chromium.org, tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1273563002/#ps160001
(title: "Use default fences")

[commit-bot: I haz the power, 2015-08-10 10:14:37]

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

[ccameron, 2015-08-10 10:15:00]

On 2015/08/09 18:54:20, Ken Russell OOO-till-Aug-17 wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/1280033004/ by mailto:kbr@chromium.org.
> 
> The reason for reverting is: This broke the WebGL 2.0 prototype on Mac OS --
> please see https://code.google.com/p/chromium/issues/detail?id=515696#c16 for
> details.
> .

The issue looks to be use of GL_APPLE_fence on core profile. Switched to using
the generic fence structure.

[commit-bot: I haz the power, 2015-08-10 10:44:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-10 10:45:13]

Patchset 9 (id:??) landed as
https://crrev.com/01ae4cbc31c1092b8c0ed15ff3a9773badfa278a
Cr-Commit-Position: refs/heads/master@{#342603}

[Ken Russell (switch to Gerrit), 2015-08-10 16:41:54]

On 2015/08/10 10:15:00, ccameron wrote:
> On 2015/08/09 18:54:20, Ken Russell OOO-till-Aug-17 wrote:
> > A revert of this CL (patchset #8 id:140001) has been created in
> > https://codereview.chromium.org/1280033004/ by mailto:kbr@chromium.org.
> > 
> > The reason for reverting is: This broke the WebGL 2.0 prototype on Mac OS --
> > please see https://code.google.com/p/chromium/issues/detail?id=515696#c16
for
> > details.
> > .
> 
> The issue looks to be use of GL_APPLE_fence on core profile. Switched to using
> the generic fence structure.

Thank you for tracking this down!

[reveman, 2015-08-18 08:44:11]

reveman@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2015-08-18 08:44:12]

https://codereview.chromium.org/1273563002/diff/160001/content/common/gpu/ima...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1273563002/diff/160001/content/common/gpu/ima...
content/common/gpu/image_transport_surface_overlay_mac.mm:148:
static_cast<gfx::GLImageIOSurface*>(pending_overlay_image_);
this static cast seems scary. how do we know that this is a GLImageIOSurface?
and how protect against changes that break that guarantee?

[ccameron, 2015-08-18 19:29:39]

On 2015/08/18 08:44:12, reveman wrote:
>
https://codereview.chromium.org/1273563002/diff/160001/content/common/gpu/ima...
> File content/common/gpu/image_transport_surface_overlay_mac.mm (right):
> 
>
https://codereview.chromium.org/1273563002/diff/160001/content/common/gpu/ima...
> content/common/gpu/image_transport_surface_overlay_mac.mm:148:
> static_cast<gfx::GLImageIOSurface*>(pending_overlay_image_);
> this static cast seems scary. how do we know that this is a GLImageIOSurface?
> and how protect against changes that break that guarantee?

I can create a static GLImageIOSurface::GetFromGLImage which will look up all
created GLImages -- does that sound reasonable?