GetVSyncParameters only once every 5 seconds on Linux

GetVSyncParameters only once every 5 seconds on Linux

Some Linux drivers implement the GLX_SGI_video_sync
extension in a way that eats a ton of CPU resources.
Only call it periodically to reduce CPU usage.

BUG=373489
Committed: https://crrev.com/60cf26c22bf0bb90fa6dbbdef5cb79fe7c305ecb
Cr-Commit-Position: refs/heads/master@{#294483}

[brianderson, 2014-09-11 18:25:13]

brianderson@chromium.org changed reviewers:
+ kbr@chromium.org

[brianderson, 2014-09-11 18:25:14]

ptal

[Ken Russell (switch to Gerrit), 2014-09-11 18:37:34]

Looks good in general, but a couple of questions.

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

https://codereview.chromium.org/566663002/diff/1/ui/gl/gl_surface_glx.cc#newc...
ui/gl/gl_surface_glx.cc:59: #if defined(OS_LINUX)
Are the OS_LINUX #ifdefs really necessary? This entire file should mainly
(only?) be used on Linux -- is Chrome OS the other consumer?

Could a different constant be chosen on Linux vs. non-Linux? See below.

https://codereview.chromium.org/566663002/diff/1/ui/gl/gl_surface_glx.cc#newc...
ui/gl/gl_surface_glx.cc:270: #if defined(OS_LINUX)
Instead of an #ifdef here, could this be written like:
  if (kGetVsyncParametersMinPeriod > 0) {
    ...
  }

and set it to 0 if it shouldn't be applied?

[brianderson, 2014-09-11 18:50:30]

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

https://codereview.chromium.org/566663002/diff/1/ui/gl/gl_surface_glx.cc#newc...
ui/gl/gl_surface_glx.cc:59: #if defined(OS_LINUX)
On 2014/09/11 18:37:34, Ken Russell wrote:
> Are the OS_LINUX #ifdefs really necessary? This entire file should mainly
> (only?) be used on Linux -- is Chrome OS the other consumer?
> 
> Could a different constant be chosen on Linux vs. non-Linux? See below.

I was under the impression the CrOS used this too. Trying to verify.

https://codereview.chromium.org/566663002/diff/1/ui/gl/gl_surface_glx.cc#newc...
ui/gl/gl_surface_glx.cc:270: #if defined(OS_LINUX)
On 2014/09/11 18:37:34, Ken Russell wrote:
> Instead of an #ifdef here, could this be written like:
>   if (kGetVsyncParametersMinPeriod > 0) {
>     ...
>   }
> 
> and set it to 0 if it shouldn't be applied?

Done.

[Ken Russell (switch to Gerrit), 2014-09-11 18:52:01]

Thanks. LGTM regardless of the answer about the top #ifdef.

We should think about how to mock parts of this code and write unit tests for it
going forward.

[brianderson, 2014-09-11 20:25:20]

Stephane confirmed that the file is used on Chrome OS as well, however the
GLX_SGI_video_sync extension isn't used on ChromeOS.

Going to keep the define in any case so it's doubly clear that the workaround
only applies to Linux.

[brianderson, 2014-09-11 20:27:33]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:30:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/566663002/40001

[commit-bot: I haz the power, 2014-09-11 23:00:05]

Committed patchset #3 (id:40001) as c6a16595578971a7e1235b37d80d080c5e37a62e

[commit-bot: I haz the power, 2014-09-11 23:08:09]

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