Send vsync timebase updates to the browser compositor

Send vsync timebase updates to the browser compositor

This uses the GLX_OML_sync_control extension to get vsync timebase
and interval information and forward it to the browser compositor 
on Aura, and to the renderer compositor on non-Aura.

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

[Sam Leffler, 2012-10-17 16:07:29]

Looks like GLX_OML_sync_control is returning CLOCK_REALTIME for the system time
component?  Reading the extension doc it appears it should be returning
CLOCK_MONOTONIC which would allow you to remove the conversion.  CLOCK_REALTIME
can be adjusted by system services like ntp so don't think a single conversion
will work.  You might be better off just replacing that time value by a read of
CLOCK_MONOTONIC unless the values are not "current" (don't see how all this
pulls together).

[ajuma, 2012-10-17 16:57:01]

On 2012/10/17 16:07:29, Sam Leffler wrote:
> You might be better off just replacing that time value by a read of
> CLOCK_MONOTONIC unless the values are not "current" (don't see how all this
> pulls together).

The problem is that the value returned is the time of the previous vblank, not
the current time, so we cannot replace it with the current value of
CLOCK_MONOTONIC.

[jamesr, 2012-10-17 17:04:52]

On 2012/10/17 16:57:01, ajuma wrote:
> On 2012/10/17 16:07:29, Sam Leffler wrote:
> > You might be better off just replacing that time value by a read of
> > CLOCK_MONOTONIC unless the values are not "current" (don't see how all this
> > pulls together).
> 
> The problem is that the value returned is the time of the previous vblank, not
> the current time, so we cannot replace it with the current value of
> CLOCK_MONOTONIC.

Unix epoch time is pretty much useless - can we change the extension to give a
CLOCK_MONOTONIC timestamp?

[ajuma, 2012-10-17 17:13:31]

On 2012/10/17 17:04:52, jamesr wrote:
> Unix epoch time is pretty much useless - can we change the extension to give a
> CLOCK_MONOTONIC timestamp?

@marcheu, how feasible is it to change the clock used by the extension?

[piman, 2012-10-17 17:24:46]

https://chromiumcodereview.appspot.com/11195011/diff/10001/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/10001/cc/gl_renderer.h#n...
cc/gl_renderer.h:126: virtual void onUpdateVSyncTime(int64) OVERRIDE;
There already is a mechanism to feed the vsync timing info into the compositor
(WebCompositorOutputSurfaceClient::onVSyncParametersChanged). Can we combine the
2?

https://chromiumcodereview.appspot.com/11195011/diff/10001/content/common/gpu...
File content/common/gpu/image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/10001/content/common/gpu...
content/common/gpu/image_transport_surface.cc:361:
clock_gettime(CLOCK_MONOTONIC, &monotonicTime);
PassThroughImageTransportSurface is cross-platform code, so this won't work.
It sounds like any conversion you'd want to do belongs into GLSurfaceGLX.

[ajuma, 2012-10-17 21:15:22]

https://chromiumcodereview.appspot.com/11195011/diff/10001/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/10001/cc/gl_renderer.h#n...
cc/gl_renderer.h:126: virtual void onUpdateVSyncTime(int64) OVERRIDE;
On 2012/10/17 17:24:47, piman wrote:
> There already is a mechanism to feed the vsync timing info into the compositor
> (WebCompositorOutputSurfaceClient::onVSyncParametersChanged). Can we combine
the
> 2?

See the discussion in the WebKit bug for this issue
(https://bugs.webkit.org/show_bug.cgi?id=99592).

[ajuma, 2012-10-24 16:00:19]

This now uses the existing path into the compositor instead of adding a new one.
I've also moved the time conversion from PassThroughImageTransportSurface over
to GLSurfaceGLX.

[piman, 2012-10-24 16:56:54]

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/gp...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/gp...
content/browser/gpu/gpu_process_host_ui_shim.h:24: #include
"ipc/ipc_channel_proxy.h"
This doesn't seem used in this file. Remove?

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/re...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:231: void
AddFilterOnIOThreadTask(
Up to now, this file didn't have to know about the GPU process or IO threads or
anything IPC-related. Could we move this logic onto the
BrowserGpuChannelHostFactory?

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:316:
output_surface_->OnMessageReceived(message);
As mentioned on the message, we need to deal with >1 surface/screen.
Ideally we would use a single filter / BrowserCompositorOutputSurfaceProxy and
have them route the info to the right BrowserCompositorOutputSurface.

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
content/common/gpu/gpu_messages.h:381:
IPC_MESSAGE_CONTROL2(GpuHostMsg_UpdateVSyncParameters,
Something is missing here, when you have multiple screens (multiple GLSurfaces,
multiple refresh rate/alignment).
You need an identifier to associate this data to the right surface.

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
File content/common/gpu/image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
content/common/gpu/image_transport_surface.cc:359: const int64 intervalTime =
base::Time::kMicrosecondsPerSecond / 60;
This sounds like it belongs to GLSurfaceGLX too.

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/compositor/comp...
ui/compositor/compositor.cc:69: public:
nit: Chrome style please for all of this class.

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface.h...
ui/gl/gl_surface.h:105: // Get the CLOCK_MONOTONIC time (in microseconds) for
the most recent screen
CLOCK_MONOTONIC is a linux-only concept. Maybe the best here is to pass
base::TimeTicks which is a platform independent representation.

At the same time, exposing "media stream counter" and "swap buffer counter"
isn't meaningful in the cross-platform interface if it's not available and not
used anywhere.
We should make this expose the last tick time and the tick rate (or interval),
because that's exactly what we need, and what we may be able to provide on all
platforms.

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface.h...
ui/gl/gl_surface.h:109: virtual bool GetSyncValues(int64* monotonicTime,
nit: chrome style for parameters

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface_g...
File ui/gl/gl_surface_glx.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:241: // be CLOCK_REALTIME. We need the time according to
CLOCK_MONOTONIC,
Where did we test that assumption? Only Intel drivers, or others?

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:243: int64 systemTime;
nit: chrome style, here and everywhere.

[ajuma, 2012-10-26 20:12:39]

Addressed piman@'s comments.

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/gp...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/gp...
content/browser/gpu/gpu_process_host_ui_shim.h:24: #include
"ipc/ipc_channel_proxy.h"
On 2012/10/24 16:56:54, piman wrote:
> This doesn't seem used in this file. Remove?

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/re...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:231: void
AddFilterOnIOThreadTask(
On 2012/10/24 16:56:54, piman wrote:
> Up to now, this file didn't have to know about the GPU process or IO threads
or
> anything IPC-related. Could we move this logic onto the
> BrowserGpuChannelHostFactory?

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:316:
output_surface_->OnMessageReceived(message);
On 2012/10/24 16:56:54, piman wrote:
> As mentioned on the message, we need to deal with >1 surface/screen.
> Ideally we would use a single filter / BrowserCompositorOutputSurfaceProxy and
> have them route the info to the right BrowserCompositorOutputSurface.

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
content/common/gpu/gpu_messages.h:381:
IPC_MESSAGE_CONTROL2(GpuHostMsg_UpdateVSyncParameters,
On 2012/10/24 16:56:54, piman wrote:
> Something is missing here, when you have multiple screens (multiple
GLSurfaces,
> multiple refresh rate/alignment).
> You need an identifier to associate this data to the right surface.

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
File content/common/gpu/image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/content/common/gpu...
content/common/gpu/image_transport_surface.cc:359: const int64 intervalTime =
base::Time::kMicrosecondsPerSecond / 60;
On 2012/10/24 16:56:54, piman wrote:
> This sounds like it belongs to GLSurfaceGLX too.

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/compositor/comp...
ui/compositor/compositor.cc:69: public:
On 2012/10/24 16:56:54, piman wrote:
> nit: Chrome style please for all of this class.

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface.h...
ui/gl/gl_surface.h:105: // Get the CLOCK_MONOTONIC time (in microseconds) for
the most recent screen
On 2012/10/24 16:56:54, piman wrote:
> CLOCK_MONOTONIC is a linux-only concept. Maybe the best here is to pass
> base::TimeTicks which is a platform independent representation.
> 
> At the same time, exposing "media stream counter" and "swap buffer counter"
> isn't meaningful in the cross-platform interface if it's not available and not
> used anywhere.
> We should make this expose the last tick time and the tick rate (or interval),
> because that's exactly what we need, and what we may be able to provide on all
> platforms.

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface.h...
ui/gl/gl_surface.h:109: virtual bool GetSyncValues(int64* monotonicTime,
On 2012/10/24 16:56:54, piman wrote:
> nit: chrome style for parameters

Done.

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface_g...
File ui/gl/gl_surface_glx.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:241: // be CLOCK_REALTIME. We need the time according to
CLOCK_MONOTONIC,
On 2012/10/24 16:56:54, piman wrote:
> Where did we test that assumption? Only Intel drivers, or others?

Only on Intel (I haven't managed to get my hands on a non-Intel device that
supports this extension).

In the latest version, instead of assuming that the returned time is from
CLOCK_REALTIME, we instead try to determine if it's from CLOCK_REALTIME or
CLOCK_MONONTONIC (by checking if it's sufficiently close to the current time
according to either of these clocks).

https://chromiumcodereview.appspot.com/11195011/diff/16001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:243: int64 systemTime;
On 2012/10/24 16:56:54, piman wrote:
> nit: chrome style, here and everywhere.

Done.

[jonathan.backer, 2012-10-26 20:31:03]

http://codereview.chromium.org/11195011/diff/25001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

http://codereview.chromium.org/11195011/diff/25001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:25: #include
"content/public/browser/browser_thread.h"
still need this?

http://codereview.chromium.org/11195011/diff/25001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:291: class
BrowserCompositorOutputSurfaceProxy {
Why an inner class? We're in an anon namespace no?

http://codereview.chromium.org/11195011/diff/25001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:293: static
BrowserCompositorOutputSurfaceProxy* GetInstance() {
Singletons are generally frowned upon (we do have base::Singleton though)

GpuImageTransportFactory should outlive the Proxy. Let it own the proxy.

http://codereview.chromium.org/11195011/diff/25001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:321:
OnMessageReceivedStatic),
Can we bind OnMessageReceived to this and eliminate the static member?

http://codereview.chromium.org/11195011/diff/25001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:377: class
GpuProcessTransportFactory :
Maybe this can own the BrowserCompositorOutputSurfaceProxy and you can
scoped_ptr it? You could pass a Proxy in to the OutputSurface constructor.

http://codereview.chromium.org/11195011/diff/25001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

http://codereview.chromium.org/11195011/diff/25001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:66: // Adapts a pure WebGraphicsContext3D into a
WebCompositorOutputSurface.
Feels like this should have been in an anon namespace at the top (I'm no C++
guru though).

http://codereview.chromium.org/11195011/diff/25001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

http://codereview.chromium.org/11195011/diff/25001/ui/gl/gl_surface_glx.cc#ne...
ui/gl/gl_surface_glx.cc:245: if (g_glx_oml_sync_control_supported) {
I'm concerned that this may be per monitor (in the multimonitor case with two
different video cards --- not a common configuration I know).

[ajuma, 2012-10-29 17:02:26]

Addressed backer@'s comments.

https://chromiumcodereview.appspot.com/11195011/diff/25001/content/browser/re...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/25001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:25: #include
"content/public/browser/browser_thread.h"
On 2012/10/26 20:31:03, jonathan.backer wrote:
> still need this?

No. Removed.

https://chromiumcodereview.appspot.com/11195011/diff/25001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:291: class
BrowserCompositorOutputSurfaceProxy {
On 2012/10/26 20:31:03, jonathan.backer wrote:
> Why an inner class? We're in an anon namespace no?

Yes. Made this a non-inner class now.

https://chromiumcodereview.appspot.com/11195011/diff/25001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:293: static
BrowserCompositorOutputSurfaceProxy* GetInstance() {
On 2012/10/26 20:31:03, jonathan.backer wrote:
> Singletons are generally frowned upon (we do have base::Singleton though)
> 
> GpuImageTransportFactory should outlive the Proxy. Let it own the proxy.

Done.

https://chromiumcodereview.appspot.com/11195011/diff/25001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:321:
OnMessageReceivedStatic),
On 2012/10/26 20:31:03, jonathan.backer wrote:
> Can we bind OnMessageReceived to this and eliminate the static member?

Done.

https://chromiumcodereview.appspot.com/11195011/diff/25001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:377: class
GpuProcessTransportFactory :
On 2012/10/26 20:31:03, jonathan.backer wrote:
> Maybe this can own the BrowserCompositorOutputSurfaceProxy and you can
> scoped_ptr it? You could pass a Proxy in to the OutputSurface constructor.

Done.

https://chromiumcodereview.appspot.com/11195011/diff/25001/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/25001/ui/compositor/comp...
ui/compositor/compositor.cc:66: // Adapts a pure WebGraphicsContext3D into a
WebCompositorOutputSurface.
On 2012/10/26 20:31:03, jonathan.backer wrote:
> Feels like this should have been in an anon namespace at the top (I'm no C++
> guru though).

Done.

https://chromiumcodereview.appspot.com/11195011/diff/25001/ui/gl/gl_surface_g...
File ui/gl/gl_surface_glx.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/25001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:245: if (g_glx_oml_sync_control_supported) {
On 2012/10/26 20:31:03, jonathan.backer wrote:
> I'm concerned that this may be per monitor (in the multimonitor case with two
> different video cards --- not a common configuration I know).

AIUI, since we only XOpenDisplay() once, we're only ever sending GLX calls to a
single GPU.

[piman, 2012-10-29 20:36:39]

Some nits, and a few rather minor things, but we're close!

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/browser/re...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:291: ,
surface_id_(surface_id)
nit: in chrome style, the initializer comas go after the previous initializer,
not before the next one:

: context3D_(context),
  surface_id_(surface_id),
  client_(NULL),
  output_surface_proxy_(output_surface_proxy) {

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/common/gpu...
File content/common/gpu/image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/common/gpu...
content/common/gpu/image_transport_surface.cc:358: #if defined(USE_AURA)
No ifdef AURA in the GPU-process code. Aura is a UI-concept and shouldn't leak
here.
Possibly what you mean is to check whether this surface is a "transport" one
(i.e. representing a renderer), or not?

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/compositor/comp...
File ui/compositor/compositor.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/compositor/comp...
ui/compositor/compositor.h:55: Compositor* compositor) = 0;
I think you don't need this one any more, since its uses are effectively
superseded by CreateOutputSurface. Remove?

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/gl/gl_surface_g...
File ui/gl/gl_surface_glx.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:245: if (g_glx_oml_sync_control_supported) {
nit: early-returns make this easier to read (and to indent!), e.g. 
if (!g_glx_oml_sync_control_supported)
  return false;

if (!glXGetSyncValuesOML(g_display, window_, &system_time,
                         &media_stream_counter, &swap_buffer_counter)) {
  return false;

// rest of the code

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:270: base::Time::kMicrosecondsPerMinute) {
Rather than using an arbitrary time and risk false positives/negatives in case
of adversary conditions (closing the lid, attaching a debugger, etc.), maybe
better would be to get the closest of the 2?
Rather than an arbitrary time, as we have the rate below, we can validate that
the computed delta falls within the the expected rate (modulo some fudge factor
if you wish), and fail in that case?

[ajuma, 2012-10-30 21:04:04]

Addressed piman@'s comments.

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/browser/re...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/browser/re...
content/browser/renderer_host/image_transport_factory.cc:291: ,
surface_id_(surface_id)
On 2012/10/29 20:36:39, piman wrote:
> nit: in chrome style, the initializer comas go after the previous initializer,
> not before the next one:
> 
> : context3D_(context),
>   surface_id_(surface_id),
>   client_(NULL),
>   output_surface_proxy_(output_surface_proxy) {

Done.

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/common/gpu...
File content/common/gpu/image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/content/common/gpu...
content/common/gpu/image_transport_surface.cc:358: #if defined(USE_AURA)
On 2012/10/29 20:36:39, piman wrote:
> No ifdef AURA in the GPU-process code. Aura is a UI-concept and shouldn't leak
> here.
> Possibly what you mean is to check whether this surface is a "transport" one
> (i.e. representing a renderer), or not?

I've removed the ifdef. It turns out that not much code is needed to add support
for the desktop Linux case (to handle the message in the browser and forward the
vsync parameters to the appropriate renderer), so I've added this code in
GpuProcessHostUIShim.

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/compositor/comp...
File ui/compositor/compositor.h (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/compositor/comp...
ui/compositor/compositor.h:55: Compositor* compositor) = 0;
On 2012/10/29 20:36:39, piman wrote:
> I think you don't need this one any more, since its uses are effectively
> superseded by CreateOutputSurface. Remove?

Done.

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/gl/gl_surface_g...
File ui/gl/gl_surface_glx.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:245: if (g_glx_oml_sync_control_supported) {
On 2012/10/29 20:36:39, piman wrote:
> nit: early-returns make this easier to read (and to indent!), e.g. 
> if (!g_glx_oml_sync_control_supported)
>   return false;
> 
> if (!glXGetSyncValuesOML(g_display, window_, &system_time,
>                          &media_stream_counter, &swap_buffer_counter)) {
>   return false;
> 
> // rest of the code

Done.

https://chromiumcodereview.appspot.com/11195011/diff/38001/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:270: base::Time::kMicrosecondsPerMinute) {
On 2012/10/29 20:36:39, piman wrote:
> Rather than using an arbitrary time and risk false positives/negatives in case
> of adversary conditions (closing the lid, attaching a debugger, etc.), maybe
> better would be to get the closest of the 2?
> Rather than an arbitrary time, as we have the rate below, we can validate that
> the computed delta falls within the the expected rate (modulo some fudge
factor
> if you wish), and fail in that case?

I've changed it to use the closest of the 2 (subject to the restriction that the
time is not in the future, since it is supposed to be the time of an event that
has already occurred, even under adversarial conditions).

I've not added a check that the time delta falls within one frame interval,
since we'd need such a large fudge factor to account for something like stepping
through slowly with a debugger that the fudge factor would overwhelm the actual
interval time.

[piman, 2012-10-30 21:27:19]

lgtm

[jonathan.backer, 2012-10-31 13:12:01]

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.c...
ui/gl/gl_surface.cc:151: base::TimeDelta* interval) {
nit: indentation

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_g...
File ui/gl/gl_surface_glx.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:265: clock_gettime(CLOCK_MONOTONIC, &monotonic_time);
Do we need to do this for every run? (i.e. is this expensive)

[ajuma, 2012-10-31 13:50:00]

Addressed backer@'s comment.

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.c...
ui/gl/gl_surface.cc:151: base::TimeDelta* interval) {
On 2012/10/31 13:12:02, jonathan.backer wrote:
> nit: indentation

Fixed.

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_g...
File ui/gl/gl_surface_glx.cc (right):

https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_g...
ui/gl/gl_surface_glx.cc:265: clock_gettime(CLOCK_MONOTONIC, &monotonic_time);
On 2012/10/31 13:12:02, jonathan.backer wrote:
> Do we need to do this for every run? (i.e. is this expensive)

The difference between CLOCK_REALTIME and CLOCK_MONOTONIC isn't guaranteed to be
constant, since CLOCK_REALTIME can be adjusted at any time.

[jonathan.backer, 2012-10-31 13:55:09]

On 2012/10/31 13:50:00, ajuma wrote:
> Addressed backer@'s comment.
> 
> https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.cc
> File ui/gl/gl_surface.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.c...
> ui/gl/gl_surface.cc:151: base::TimeDelta* interval) {
> On 2012/10/31 13:12:02, jonathan.backer wrote:
> > nit: indentation
> 
> Fixed.
> 
>
https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_g...
> File ui/gl/gl_surface_glx.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_g...
> ui/gl/gl_surface_glx.cc:265: clock_gettime(CLOCK_MONOTONIC, &monotonic_time);
> On 2012/10/31 13:12:02, jonathan.backer wrote:
> > Do we need to do this for every run? (i.e. is this expensive)
> 
> The difference between CLOCK_REALTIME and CLOCK_MONOTONIC isn't guaranteed to
be
> constant, since CLOCK_REALTIME can be adjusted at any time.

ui/gl/ OWNERS lgtm

[piman, 2012-10-31 19:49:31]

On Wed, Oct 31, 2012 at 6:12 AM, <backer@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/11195011/diff/**
>
42002/ui/gl/gl_surface.cc<https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.cc>
> File ui/gl/gl_surface.cc (right):
>
> https://chromiumcodereview.**appspot.com/11195011/diff/**
>
42002/ui/gl/gl_surface.cc#**newcode151<https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface.cc#newcode151>
> ui/gl/gl_surface.cc:151: base::TimeDelta* interval) {
> nit: indentation
>
>
> https://chromiumcodereview.**appspot.com/11195011/diff/**
>
42002/ui/gl/gl_surface_glx.cc<https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_glx.cc>
> File ui/gl/gl_surface_glx.cc (right):
>
> https://chromiumcodereview.**appspot.com/11195011/diff/**
>
42002/ui/gl/gl_surface_glx.cc#**newcode265<https://chromiumcodereview.appspot.com/11195011/diff/42002/ui/gl/gl_surface_glx.cc#newcode265>
> ui/gl/gl_surface_glx.cc:265: clock_gettime(CLOCK_MONOTONIC,
> &monotonic_time);
> Do we need to do this for every run? (i.e. is this expensive)
>

Once per frame ought to be fine. It's probably on the same ballpark as
posting a task.


>
>
https://chromiumcodereview.**appspot.com/11195011/<https://chromiumcodereview...
>
> --
>
>
>

[commit-bot: I haz the power, 2012-10-31 20:02:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/11195011/53001

[commit-bot: I haz the power, 2012-11-01 00:56:22]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-01 13:52:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/11195011/60002

[commit-bot: I haz the power, 2012-11-01 15:40:38]

Change committed as 165389