Use GLX_SGI_video_sync to get a vblank signal.

Use GLX_SGI_video_sync to get a vblank signal.

On platforms that support this feature:
- we set up a separate thread
- on a request for vsync timestamp, we block on the vsync on the separate thread
- we grab a timestamp on completion
- we proxy that timestamp back

BUG=none
TEST=by hand on Nvidia, ATI, and Intel drivers


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166989

[jonathan.backer, 2012-11-08 20:55:47]

I've wrapped the additional thread so that we make at most one, but we only
create it on demand, and we destroy it when not in use (think desktop linux
where we close all accelerated tabs).

The part that probes for vsync (the provider and it's shim) has two parts (one
for each thread). They post messages to each other. The shim is created and
destroyed by it's provider. In this sense, it's not strictly necessary that the
provider has a WeakPtr to it's shim (it could have a raw pointer). WeakPtr has
the advantage of checking the it's only derefenced on the thread that it's bound
to.

One thing that I dislike about the current approach is that we create the x
connection (Display*) on one thread and then use it on another. This is done to
work around the GPU sandbox. This seems to work just fine on the drivers where I
tested it.

[piman, 2012-11-08 23:51:16]

https://codereview.chromium.org/11359114/diff/3001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/11359114/diff/3001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface.cc:358:
base::Unretained(helper_.get())));
What guarantees that the helper is still valid when the callback is called?

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface.h#newcode118
ui/gl/gl_surface.h:118: // if the instance of this class is destroyed.
Can you just mention that possibly the callback is called directly from the
function (as opposed to later)? It's fine, but it's possibly surprising.

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:185: window_, &attributes)) {
It makes me somewhat uncomfortable to use the window from another display
without synchronization. Do we know that X has received the command to create
the window already?

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:226: window_, context_);
How do we know that the window hasn't been destroyed yet?

Tab closes -> GLSurface gets destroyed -> signals the browser it can destroy the
window.
If there is a pending GetVSyncParameters callback, isn't it possible that this
runs after the window is destroyed?

[jonathan.backer, 2012-11-09 17:02:35]

I hadn't thought about the cross process synchronization (browser <---> GPU).
Thanks.

https://codereview.chromium.org/11359114/diff/3001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/11359114/diff/3001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface.cc:358:
base::Unretained(helper_.get())));
On 2012/11/08 23:51:16, piman wrote:
> What guarantees that the helper is still valid when the callback is called?

PTITSurface owns ITHelper and GLSurfaceGLX
GLSurfaceGLX owns VsyncProvider

The callback will only be called be called while VsyncProvider is in scope.

I can make this more transparent by having the ITHelper vend a WeakPtr. Your
call.

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface.h#newcode118
ui/gl/gl_surface.h:118: // if the instance of this class is destroyed.
On 2012/11/08 23:51:16, piman wrote:
> Can you just mention that possibly the callback is called directly from the
> function (as opposed to later)? It's fine, but it's possibly surprising.

Done.

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:185: window_, &attributes)) {
On 2012/11/08 23:51:16, piman wrote:
> It makes me somewhat uncomfortable to use the window from another display
> without synchronization. Do we know that X has received the command to create
> the window already?

FWIW, this is not much worse than the current situation.
- the browser process calls XCreateWindow
- GLSurfaceGLX::Initialize accesses this in the GPU process on it's own Display

I will defer the creation of VideoProvider until GLSurfaceGLX::Initialize. Then
this will be no worse.

As for your first question: all we know is that XCreateWindow has been called in
the browser. On Aura linux, we know that there has also been an XFlush. I don't
think that we know any more than that.

On GTK linux, I think that we don't even have a XFlush.

If you want, we can file a separate bug for this. The low road is to XSync after
XCreateWindow. The high road is to block (via
http://www.x.org/releases/X11R7.6/doc/xextproto/sync.html ?) in the GPU process.

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:226: window_, context_);
On 2012/11/08 23:51:16, piman wrote:
> How do we know that the window hasn't been destroyed yet?
> 
> Tab closes -> GLSurface gets destroyed -> signals the browser it can destroy
the
> window.
> If there is a pending GetVSyncParameters callback, isn't it possible that this
> runs after the window is destroyed?

Good point. I've added a cancellation flag and a lock. This will guarantee that
this section won't get hit once the SGIVideoSyncVSyncProvider is destroyed.

[piman, 2012-11-09 18:08:05]

Nice for the cancellation flag. Couple of things left.

https://codereview.chromium.org/11359114/diff/3001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/11359114/diff/3001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface.cc:358:
base::Unretained(helper_.get())));
On 2012/11/09 17:02:35, jonathan.backer wrote:
> On 2012/11/08 23:51:16, piman wrote:
> > What guarantees that the helper is still valid when the callback is called?
> 
> PTITSurface owns ITHelper and GLSurfaceGLX
> GLSurfaceGLX owns VsyncProvider
> 
> The callback will only be called be called while VsyncProvider is in scope.
> 
> I can make this more transparent by having the ITHelper vend a WeakPtr. Your
> call.

Since GLSurfaceGLX is refcounted, it's hard to guarantee (forever) it won't
outlive the helper. If we believe it's true today, and we can assert this, we
should do that, otherwise a WeakPtr is fine.

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/11359114/diff/3001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:185: window_, &attributes)) {
On 2012/11/09 17:02:35, jonathan.backer wrote:
> On 2012/11/08 23:51:16, piman wrote:
> > It makes me somewhat uncomfortable to use the window from another display
> > without synchronization. Do we know that X has received the command to
create
> > the window already?
> 
> FWIW, this is not much worse than the current situation.
> - the browser process calls XCreateWindow
> - GLSurfaceGLX::Initialize accesses this in the GPU process on it's own
Display
> 
> I will defer the creation of VideoProvider until GLSurfaceGLX::Initialize.
Then
> this will be no worse.
> 
> As for your first question: all we know is that XCreateWindow has been called
in
> the browser. On Aura linux, we know that there has also been an XFlush. I
don't
> think that we know any more than that.

On Aura we have a round trip to X right after we create the window, (see
XGetWindowAttributes in RootWindowHostLinux::RootWindowHostLinux)

> 
> On GTK linux, I think that we don't even have a XFlush.

I was almost positive we had a XSync on this path (in the gtk_preserve_window
code), but I can't find it - you may be right (and maybe we've been lucky).

I guess I'm thinking in terms of layers, this is in ui/ that doesn't know
anything about content/ and whatever synchronization we may do there. From the
GLSurfaceGLX client's point of view, it would be legal to create a window,
create the GLSurfaceGLX around it and call GetVSyncParameters - and it'd work in
the GLX_OML_sync_control case. Maybe for now we can just call XSync in the
SGIVideoSyncVSyncProvider constructor, and optimize later with X-SYNCHRONIZATION
if it proves to be too slow.

> 
> If you want, we can file a separate bug for this. The low road is to XSync
after
> XCreateWindow. The high road is to block (via
> http://www.x.org/releases/X11R7.6/doc/xextproto/sync.html ?) in the GPU
process.
>

[jonathan.backer, 2012-11-09 19:10:30]

> Since GLSurfaceGLX is refcounted, it's hard to guarantee (forever) it won't
> outlive the helper. If we believe it's true today, and we can assert this, we
> should do that, otherwise a WeakPtr is fine.

I went WeakPtr. You are right: there's too many things holding on to that ref
count. Even if it is true today, there's no reason to believe that it will be
true tomorrow.
 
> I guess I'm thinking in terms of layers, this is in ui/ that doesn't know
> anything about content/ and whatever synchronization we may do there. From the
> GLSurfaceGLX client's point of view, it would be legal to create a window,
> create the GLSurfaceGLX around it and call GetVSyncParameters - and it'd work
in
> the GLX_OML_sync_control case. Maybe for now we can just call XSync in the
> SGIVideoSyncVSyncProvider constructor, and optimize later with
X-SYNCHRONIZATION
> if it proves to be too slow.

Done.

[piman, 2012-11-09 19:14:41]

https://codereview.chromium.org/11359114/diff/2008/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/11359114/diff/2008/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:184: XSync(g_display, False);
::gfx::g_display?
I think as it it will resolve to the static
SGIVideoSyncProviderThreadShim::g_display which is not the one you want.

[jonathan.backer, 2012-11-09 19:24:55]

https://codereview.chromium.org/11359114/diff/2008/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/11359114/diff/2008/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:184: XSync(g_display, False);
On 2012/11/09 19:14:42, piman wrote:
> ::gfx::g_display?
> I think as it it will resolve to the static
> SGIVideoSyncProviderThreadShim::g_display which is not the one you want.

Done.

[piman, 2012-11-09 19:27:21]

lgtm

[commit-bot: I haz the power, 2012-11-09 20:13:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/11359114/12

[commit-bot: I haz the power, 2012-11-09 22:07:31]

Change committed as 166989