Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277883003:
gl: Disable SGI_video_sync by default. (Closed)
|
Created:
4 years, 4 months ago by brianderson Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongl: Disable SGI_video_sync by default.
Disables the SGI_video_sync extension and assumes a
vsync interval of 59.9Hz on Linux.
Also adds an --enable-sgi-video-sync command line flag to
re-enable the extension.
Use of the extension on a thread other than the main GPU
thread is encountering issues with Chrome's sandbox for
certain combinations of Linux window managers and NVIDIA
drivers.
Before this patch, we were assuming a frequency of 60Hz
on Linux and re-aligning the phase every 5 seconds.
Since we can no longer align the phase, we assume a
frequency of 59.9 Hz. On a 60Hz monitor, this avoids
driver backpressure on the GPU thread at the cost of
skipping a frame every 10 seconds.
BUG=636644
Committed: https://crrev.com/22774dfbaceb5f616183b3d6135264b458c9ed0a
Cr-Commit-Position: refs/heads/master@{#414342}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Stub -> Fixed; pull up command line check; #
Total comments: 2
Patch Set 3 : forward flag #
Total comments: 3
Patch Set 4 : accidentally an s #
Messages
Total messages: 46 (17 generated)
The CQ bit was checked by brianderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
brianderson@chromium.org changed reviewers: + danakj@chromium.org, piman@chromium.org, sunnyps@chromium.org
Ptal.
https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc File ui/gl/gl_surface_glx.cc (right): https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... ui/gl/gl_surface_glx.cc:515: // not common. The investigation for bug https://bugs.chromium.org/p/chromium/issues/detail?id=262437 (where we would reduce frame rate to 30Hz when the monitor was anywhere below 60) showed that it's very common unfortunately. 50 or 55Hz is probably a safer lower bound (at the cost of more missed frames). We used to have a UMA histogram for the refresh rate (GPU.AcceleratedSurfaceRefreshRate) but apparently it broke eons ago.
https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h File ui/gfx/vsync_provider.h (right): https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h#new... ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public VSyncProvider { nit: FixedVsyncProvider or something? Stub implies it does nothing at all IMO. https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc File ui/gl/gl_surface_glx.cc (right): https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... ui/gl/gl_surface_glx.cc:409: if (!g_glx_get_msc_rate_oml_supported && g_glx_sgi_video_sync_supported) Do you want to do this without the command line? Should we maybe just make g_glx_sgi_video_sync_supported = false when the command line isnt present? https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... ui/gl/gl_surface_glx.cc:515: // not common. On 2016/08/24 21:04:29, piman wrote: > The investigation for bug > https://bugs.chromium.org/p/chromium/issues/detail?id=262437 (where we would > reduce frame rate to 30Hz when the monitor was anywhere below 60) showed that > it's very common unfortunately. 50 or 55Hz is probably a safer lower bound (at > the cost of more missed frames). > > We used to have a UMA histogram for the refresh rate > (GPU.AcceleratedSurfaceRefreshRate) but apparently it broke eons ago. Why not just grab the actual refresh rate here? We have an x display. It looks like this gives an int instead of something more accurate, but maybe good enough? int event, error; int fps = 50; if (XRRQueryExtension(&event, &error)) { XRRScreenConfiguration *conf = XRRGetScreenInfo(g_display, parent_window_); fps = XRRConfigCurrentRate(conf); XRRFreeScreenConfigInfo(conf); }
On 2016/08/24 22:20:50, danakj wrote: > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h > File ui/gfx/vsync_provider.h (right): > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h#new... > ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public > VSyncProvider { > nit: FixedVsyncProvider or something? Stub implies it does nothing at all IMO. > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc > File ui/gl/gl_surface_glx.cc (right): > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > ui/gl/gl_surface_glx.cc:409: if (!g_glx_get_msc_rate_oml_supported && > g_glx_sgi_video_sync_supported) > Do you want to do this without the command line? Should we maybe just make > g_glx_sgi_video_sync_supported = false when the command line isnt present? > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > ui/gl/gl_surface_glx.cc:515: // not common. > On 2016/08/24 21:04:29, piman wrote: > > The investigation for bug > > https://bugs.chromium.org/p/chromium/issues/detail?id=262437 (where we would > > reduce frame rate to 30Hz when the monitor was anywhere below 60) showed that > > it's very common unfortunately. 50 or 55Hz is probably a safer lower bound (at > > the cost of more missed frames). > > > > We used to have a UMA histogram for the refresh rate > > (GPU.AcceleratedSurfaceRefreshRate) but apparently it broke eons ago. > > Why not just grab the actual refresh rate here? We have an x display. It looks > like this gives an int instead of something more accurate, but maybe good > enough? > > int event, error; > int fps = 50; > if (XRRQueryExtension(&event, &error)) { > XRRScreenConfiguration *conf = XRRGetScreenInfo(g_display, parent_window_); > fps = XRRConfigCurrentRate(conf); > XRRFreeScreenConfigInfo(conf); > } So I tried patching this in and what I get is 50 from this function call. My mode says its 60 though. % xrandr Screen 0: minimum 8 x 8, current 2560 x 1600, maximum 16384 x 16384 DVI-I-0 disconnected (normal left inverted right x axis y axis) DVI-I-1 connected primary 2560x1600+0+0 (normal left inverted right x axis y axis) 641mm x 400mm 2560x1600 60.0*+ 1920x1200 59.9 1920x1080 60.0 ... When I use 1920x1200, I also get 50 back. I'm not sure which is true.
On 2016/08/24 22:35:02, danakj wrote: > On 2016/08/24 22:20:50, danakj wrote: > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h > > File ui/gfx/vsync_provider.h (right): > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h#new... > > ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public > > VSyncProvider { > > nit: FixedVsyncProvider or something? Stub implies it does nothing at all IMO. > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc > > File ui/gl/gl_surface_glx.cc (right): > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > > ui/gl/gl_surface_glx.cc:409: if (!g_glx_get_msc_rate_oml_supported && > > g_glx_sgi_video_sync_supported) > > Do you want to do this without the command line? Should we maybe just make > > g_glx_sgi_video_sync_supported = false when the command line isnt present? > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > > ui/gl/gl_surface_glx.cc:515: // not common. > > On 2016/08/24 21:04:29, piman wrote: > > > The investigation for bug > > > https://bugs.chromium.org/p/chromium/issues/detail?id=262437 (where we would > > > reduce frame rate to 30Hz when the monitor was anywhere below 60) showed > that > > > it's very common unfortunately. 50 or 55Hz is probably a safer lower bound > (at > > > the cost of more missed frames). > > > > > > We used to have a UMA histogram for the refresh rate > > > (GPU.AcceleratedSurfaceRefreshRate) but apparently it broke eons ago. > > > > Why not just grab the actual refresh rate here? We have an x display. It looks > > like this gives an int instead of something more accurate, but maybe good > > enough? > > > > int event, error; > > int fps = 50; > > if (XRRQueryExtension(&event, &error)) { > > XRRScreenConfiguration *conf = XRRGetScreenInfo(g_display, parent_window_); > > fps = XRRConfigCurrentRate(conf); > > XRRFreeScreenConfigInfo(conf); > > } > > So I tried patching this in and what I get is 50 from this function call. > > My mode says its 60 though. > > % xrandr > Screen 0: minimum 8 x 8, current 2560 x 1600, maximum 16384 x 16384 > DVI-I-0 disconnected (normal left inverted right x axis y axis) > DVI-I-1 connected primary 2560x1600+0+0 (normal left inverted right x axis y > axis) 641mm x 400mm > 2560x1600 60.0*+ > 1920x1200 59.9 > 1920x1080 60.0 > ... > > When I use 1920x1200, I also get 50 back. I'm not sure which is true. Furthermore, the following to list all possible rates only lists a single rate, which is 50: Rotation rotation; SizeID size_id = XRRConfigCurrentConfiguration(conf, &rotation); int nrates; short* rates = XRRConfigRates(conf, size_id, &nrates);
On Wed, Aug 24, 2016 at 3:43 PM, <danakj@chromium.org> wrote: > On 2016/08/24 22:35:02, danakj wrote: > > On 2016/08/24 22:20:50, danakj wrote: > > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/ > vsync_provider.h > > > File ui/gfx/vsync_provider.h (right): > > > > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/ > vsync_provider.h#newcode32 > > > ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public > > > VSyncProvider { > > > nit: FixedVsyncProvider or something? Stub implies it does nothing at > all > IMO. > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/ > gl_surface_glx.cc > > > File ui/gl/gl_surface_glx.cc (right): > > > > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/ > gl_surface_glx.cc#newcode409 > > > ui/gl/gl_surface_glx.cc:409: if (!g_glx_get_msc_rate_oml_supported && > > > g_glx_sgi_video_sync_supported) > > > Do you want to do this without the command line? Should we maybe just > make > > > g_glx_sgi_video_sync_supported = false when the command line isnt > present? > > > > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/ > gl_surface_glx.cc#newcode515 > > > ui/gl/gl_surface_glx.cc:515: // not common. > > > On 2016/08/24 21:04:29, piman wrote: > > > > The investigation for bug > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=262437 (where > we > would > > > > reduce frame rate to 30Hz when the monitor was anywhere below 60) > showed > > that > > > > it's very common unfortunately. 50 or 55Hz is probably a safer lower > bound > > (at > > > > the cost of more missed frames). > > > > > > > > We used to have a UMA histogram for the refresh rate > > > > (GPU.AcceleratedSurfaceRefreshRate) but apparently it broke eons > ago. > > > > > > Why not just grab the actual refresh rate here? We have an x display. > It > looks > > > like this gives an int instead of something more accurate, but maybe > good > > > enough? > > > > > > int event, error; > > > int fps = 50; > > > if (XRRQueryExtension(&event, &error)) { > > > XRRScreenConfiguration *conf = XRRGetScreenInfo(g_display, > parent_window_); > > > fps = XRRConfigCurrentRate(conf); > > > XRRFreeScreenConfigInfo(conf); > > > } > > > > So I tried patching this in and what I get is 50 from this function call. > > > > My mode says its 60 though. > > > > % xrandr > > Screen 0: minimum 8 x 8, current 2560 x 1600, maximum 16384 x 16384 > > DVI-I-0 disconnected (normal left inverted right x axis y axis) > > DVI-I-1 connected primary 2560x1600+0+0 (normal left inverted right x > axis y > > axis) 641mm x 400mm > > 2560x1600 60.0*+ > > 1920x1200 59.9 > > 1920x1080 60.0 > > ... > > > > When I use 1920x1200, I also get 50 back. I'm not sure which is true. > > Furthermore, the following to list all possible rates only lists a single > rate, > which is 50: > > Rotation rotation; > SizeID size_id = XRRConfigCurrentConfiguration(conf, &rotation); > int nrates; > short* rates = XRRConfigRates(conf, size_id, &nrates); > Surely xrandr also uses the XRandR extension?? It's odd it would give a different result? Does it change if you disable the sandbox? > > https://codereview.chromium.org/2277883003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/24 at 22:35:02, danakj wrote: > On 2016/08/24 22:20:50, danakj wrote: > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h > > File ui/gfx/vsync_provider.h (right): > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h#new... > > ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public > > VSyncProvider { > > nit: FixedVsyncProvider or something? Stub implies it does nothing at all IMO. > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc > > File ui/gl/gl_surface_glx.cc (right): > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > > ui/gl/gl_surface_glx.cc:409: if (!g_glx_get_msc_rate_oml_supported && > > g_glx_sgi_video_sync_supported) > > Do you want to do this without the command line? Should we maybe just make > > g_glx_sgi_video_sync_supported = false when the command line isnt present? > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > > ui/gl/gl_surface_glx.cc:515: // not common. > > On 2016/08/24 21:04:29, piman wrote: > > > The investigation for bug > > > https://bugs.chromium.org/p/chromium/issues/detail?id=262437 (where we would > > > reduce frame rate to 30Hz when the monitor was anywhere below 60) showed that > > > it's very common unfortunately. 50 or 55Hz is probably a safer lower bound (at > > > the cost of more missed frames). > > > > > > We used to have a UMA histogram for the refresh rate > > > (GPU.AcceleratedSurfaceRefreshRate) but apparently it broke eons ago. > > > > Why not just grab the actual refresh rate here? We have an x display. It looks > > like this gives an int instead of something more accurate, but maybe good > > enough? > > > > int event, error; > > int fps = 50; > > if (XRRQueryExtension(&event, &error)) { > > XRRScreenConfiguration *conf = XRRGetScreenInfo(g_display, parent_window_); > > fps = XRRConfigCurrentRate(conf); > > XRRFreeScreenConfigInfo(conf); > > } > > So I tried patching this in and what I get is 50 from this function call. > > My mode says its 60 though. > > % xrandr > Screen 0: minimum 8 x 8, current 2560 x 1600, maximum 16384 x 16384 > DVI-I-0 disconnected (normal left inverted right x axis y axis) > DVI-I-1 connected primary 2560x1600+0+0 (normal left inverted right x axis y axis) 641mm x 400mm > 2560x1600 60.0*+ > 1920x1200 59.9 > 1920x1080 60.0 > ... > > When I use 1920x1200, I also get 50 back. I'm not sure which is true. I think the chromoting code does the right thing here: https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_scr...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/24 at 22:47:16, sunnyps wrote: > On 2016/08/24 at 22:35:02, danakj wrote: > > On 2016/08/24 22:20:50, danakj wrote: > > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h > > > File ui/gfx/vsync_provider.h (right): > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h#new... > > > ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public > > > VSyncProvider { > > > nit: FixedVsyncProvider or something? Stub implies it does nothing at all IMO. > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc > > > File ui/gl/gl_surface_glx.cc (right): > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > > > ui/gl/gl_surface_glx.cc:409: if (!g_glx_get_msc_rate_oml_supported && > > > g_glx_sgi_video_sync_supported) > > > Do you want to do this without the command line? Should we maybe just make > > > g_glx_sgi_video_sync_supported = false when the command line isnt present? > > > > > > https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... > > > ui/gl/gl_surface_glx.cc:515: // not common. > > > On 2016/08/24 21:04:29, piman wrote: > > > > The investigation for bug > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=262437 (where we would > > > > reduce frame rate to 30Hz when the monitor was anywhere below 60) showed that > > > > it's very common unfortunately. 50 or 55Hz is probably a safer lower bound (at > > > > the cost of more missed frames). > > > > > > > > We used to have a UMA histogram for the refresh rate > > > > (GPU.AcceleratedSurfaceRefreshRate) but apparently it broke eons ago. > > > > > > Why not just grab the actual refresh rate here? We have an x display. It looks > > > like this gives an int instead of something more accurate, but maybe good > > > enough? > > > > > > int event, error; > > > int fps = 50; > > > if (XRRQueryExtension(&event, &error)) { > > > XRRScreenConfiguration *conf = XRRGetScreenInfo(g_display, parent_window_); > > > fps = XRRConfigCurrentRate(conf); > > > XRRFreeScreenConfigInfo(conf); > > > } > > > > So I tried patching this in and what I get is 50 from this function call. > > > > My mode says its 60 though. > > > > % xrandr > > Screen 0: minimum 8 x 8, current 2560 x 1600, maximum 16384 x 16384 > > DVI-I-0 disconnected (normal left inverted right x axis y axis) > > DVI-I-1 connected primary 2560x1600+0+0 (normal left inverted right x axis y axis) 641mm x 400mm > > 2560x1600 60.0*+ > > 1920x1200 59.9 > > 1920x1080 60.0 > > ... > > > > When I use 1920x1200, I also get 50 back. I'm not sure which is true. > > I think the chromoting code does the right thing here: https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_scr... Sorry, that's desktop aura code.
On 2016/08/24 22:43:58, danakj wrote: > Furthermore, the following to list all possible rates only lists a single rate, > which is 50: > > Rotation rotation; > SizeID size_id = XRRConfigCurrentConfiguration(conf, &rotation); > int nrates; > short* rates = XRRConfigRates(conf, size_id, &nrates); Oh ok here's some more info: http://stackoverflow.com/questions/37995551/discrepancy-between-command-line-... % xrandr --q1 SZ: Pixels Physical Refresh *0 2560 x 1600 ( 643mm x 402mm ) *50 1 1920 x 1200 ( 482mm x 301mm ) 51 2 1920 x 1080 ( 482mm x 271mm ) 52 3 1680 x 1050 ( 422mm x 264mm ) 53 4 1600 x 1200 ( 402mm x 301mm ) 54 5 1440 x 900 ( 362mm x 226mm ) 55 This is absolute noise before 1.2 it looks like. Version 1.2 of the protocol has better info apparently. https://github.com/raboof/xrandr/blob/master/xrandr.c has some code which I'm reading.
On Wed, Aug 24, 2016 at 3:52 PM, <danakj@chromium.org> wrote: > On 2016/08/24 22:43:58, danakj wrote: > > Furthermore, the following to list all possible rates only lists a single > rate, > > which is 50: > > > > Rotation rotation; > > SizeID size_id = XRRConfigCurrentConfiguration(conf, &rotation); > > int nrates; > > short* rates = XRRConfigRates(conf, size_id, &nrates); > > Oh ok here's some more info: > http://stackoverflow.com/questions/37995551/discrepancy-between-command- > line-xrandr-and-own-code > > % xrandr --q1 > SZ: Pixels Physical Refresh > *0 2560 x 1600 ( 643mm x 402mm ) *50 > 1 1920 x 1200 ( 482mm x 301mm ) 51 > 2 1920 x 1080 ( 482mm x 271mm ) 52 > 3 1680 x 1050 ( 422mm x 264mm ) 53 > 4 1600 x 1200 ( 402mm x 301mm ) 54 > 5 1440 x 900 ( 362mm x 226mm ) 55 > > This is absolute noise before 1.2 it looks like. Version 1.2 of the > protocol has > better info apparently. > > https://github.com/raboof/xrandr/blob/master/xrandr.c has some code which > I'm > reading. > ui/display/chromeos/x11/native_display_delegate_x11.cc also has code for Chrome OS > > https://codereview.chromium.org/2277883003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK this protocol is actually insane, the number of lists of things and how to map to the current refresh rate is ridiculous. But anyways it left me wondering if 50hz is actually that common or we measured it wrong.
On Wed, Aug 24, 2016 at 4:22 PM, <danakj@chromium.org> wrote: > OK this protocol is actually insane, the number of lists of things and how > to > map to the current refresh rate is ridiculous. > > But anyways it left me wondering if 50hz is actually that common or we > measured > it wrong. > It was measured on Windows with IDirect3DDevice9::GetDisplayMode, and the data last I looked at it (>2y ago) seemed sane, but IIRC the most common refresh rate was actually 59, not 60, and there was a reasonably long tail on either side. I don't think we have any reason to believe it was wrong. In non-NTSC countries, TVs are typically 50Hz not 60, and 50Hz monitors are actually common. > > https://codereview.chromium.org/2277883003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/24 23:37:25, piman wrote: > On Wed, Aug 24, 2016 at 4:22 PM, <mailto:danakj@chromium.org> wrote: > > > OK this protocol is actually insane, the number of lists of things and how > > to > > map to the current refresh rate is ridiculous. > > > > But anyways it left me wondering if 50hz is actually that common or we > > measured > > it wrong. > > > > It was measured on Windows with IDirect3DDevice9::GetDisplayMode, and the > data last I looked at it (>2y ago) seemed sane, but IIRC the most common > refresh rate was actually 59, not 60, and there was a reasonably long tail > on either side. I don't think we have any reason to believe it was wrong. > > In non-NTSC countries, TVs are typically 50Hz not 60, and 50Hz monitors are > actually common. Ah ok, well I don't believe we'll do something better than a constant before tomorrow so LGTM % piman. > > > > > https://codereview.chromium.org/2277883003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
For monitors below <= 59Hz I don't think targeting 59.9 Hz will introduce a regression. I think we are already doing a bad job at ToT because we try to produce at 60Hz and only use SGI_video_sync once every 5 seconds. We'd have to use it every vsync to work well <= 59 Hz. I chose 59.9 because I remember a lot of display saying they are 60 Hz really being just under 60 Hz. In the bug piman referred to, apparently 59.94 is the number that comes up often : http://www.tvtechnology.com/media-systems/0191/whats-the-difference-between-f....
https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h File ui/gfx/vsync_provider.h (right): https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h#new... ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public VSyncProvider { On 2016/08/24 22:20:50, danakj wrote: > nit: FixedVsyncProvider or something? Stub implies it does nothing at all IMO. Will change the name. https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc File ui/gl/gl_surface_glx.cc (right): https://codereview.chromium.org/2277883003/diff/1/ui/gl/gl_surface_glx.cc#new... ui/gl/gl_surface_glx.cc:409: if (!g_glx_get_msc_rate_oml_supported && g_glx_sgi_video_sync_supported) On 2016/08/24 22:20:50, danakj wrote: > Do you want to do this without the command line? Should we maybe just make > g_glx_sgi_video_sync_supported = false when the command line isnt present? Sounds like a good idea.
The CQ bit was checked by brianderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2277883003/diff/20001/ui/gl/gl_switches.cc File ui/gl/gl_switches.cc (right): https://codereview.chromium.org/2277883003/diff/20001/ui/gl/gl_switches.cc#ne... ui/gl/gl_switches.cc:87: const char kEnableSgiVideoSync[] = "enable-sgi-video-sync"; Please add this to kGLSwitchesCopiedFromGpuProcessHost below, otherwise the flag will not make it to the GPU process.
The CQ bit was checked by brianderson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Wdyt of 59.9? I can investigate using xrandr and maybe cherry-pick to the branch. https://codereview.chromium.org/2277883003/diff/20001/ui/gl/gl_switches.cc File ui/gl/gl_switches.cc (right): https://codereview.chromium.org/2277883003/diff/20001/ui/gl/gl_switches.cc#ne... ui/gl/gl_switches.cc:87: const char kEnableSgiVideoSync[] = "enable-sgi-video-sync"; On 2016/08/25 00:22:52, piman wrote: > Please add this to kGLSwitchesCopiedFromGpuProcessHost below, otherwise the flag > will not make it to the GPU process. Done.
On 2016/08/25 01:46:05, brianderson wrote: > Wdyt of 59.9? > > I can investigate using xrandr and maybe cherry-pick to the branch. I'm not in love... I guess this is probably better than doing nothing as things get pretty broken for some users, so LGTM for now. > > https://codereview.chromium.org/2277883003/diff/20001/ui/gl/gl_switches.cc > File ui/gl/gl_switches.cc (right): > > https://codereview.chromium.org/2277883003/diff/20001/ui/gl/gl_switches.cc#ne... > ui/gl/gl_switches.cc:87: const char kEnableSgiVideoSync[] = > "enable-sgi-video-sync"; > On 2016/08/25 00:22:52, piman wrote: > > Please add this to kGLSwitchesCopiedFromGpuProcessHost below, otherwise the > flag > > will not make it to the GPU process. > Done.
https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.cc File ui/gfx/vsync_provider.cc (left): https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.cc#ol... ui/gfx/vsync_provider.cc:5: #include "ui/gfx/image/image_skia_source.h" git cl upload marked this as a copy of ui/gfx/image/image_skia_source.cc for some reason. https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h File ui/gfx/vsync_provider.h (right): https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.h#new... ui/gfx/vsync_provider.h:32: class GFX_EXPORT VSyncProviderStub : public VSyncProvider { Do we even need a stub vsync provider? Having no vsync provider also works - we rely on the value of BeginFrameArgs::DefaultInterval in that case. https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h File ui/gfx/vsync_provider.h (right): https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h... ui/gfx/vsync_provider.h:32: class GFX_EXPORT FixedVSyncProvider : public VSyncProvider { Do we even need FixedVSyncProvider? If a vsync provider is not present the synthetic BFS will use BeginFrameArgs::DefaultInterval (16667 us) so just modifying that should be sufficient, no?
https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h File ui/gfx/vsync_provider.h (right): https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h... ui/gfx/vsync_provider.h:32: class GFX_EXPORT FixedVSyncProvider : public VSyncProvider { On 2016/08/25 01:59:29, sunnyps wrote: > Do we even need FixedVSyncProvider? If a vsync provider is not present the > synthetic BFS will use BeginFrameArgs::DefaultInterval (16667 us) so just > modifying that should be sufficient, no? BeginFrameArgs::DefaultInterval is used all over the place tho, right? I find changing that for branching to be scary.
On 2016/08/25 at 02:04:52, danakj wrote: > https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h > File ui/gfx/vsync_provider.h (right): > > https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h... > ui/gfx/vsync_provider.h:32: class GFX_EXPORT FixedVSyncProvider : public VSyncProvider { > On 2016/08/25 01:59:29, sunnyps wrote: > > Do we even need FixedVSyncProvider? If a vsync provider is not present the > > synthetic BFS will use BeginFrameArgs::DefaultInterval (16667 us) so just > > modifying that should be sufficient, no? > > BeginFrameArgs::DefaultInterval is used all over the place tho, right? I find changing that for branching to be scary. TBH I don't think that using 59.9 Hz instead of 60 Hz will be a problem in practice. I think just disabling the use of sgi video sync is good enough.
On 2016/08/25 at 02:19:57, sunnyps wrote: > On 2016/08/25 at 02:04:52, danakj wrote: > > https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h > > File ui/gfx/vsync_provider.h (right): > > > > https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h... > > ui/gfx/vsync_provider.h:32: class GFX_EXPORT FixedVSyncProvider : public VSyncProvider { > > On 2016/08/25 01:59:29, sunnyps wrote: > > > Do we even need FixedVSyncProvider? If a vsync provider is not present the > > > synthetic BFS will use BeginFrameArgs::DefaultInterval (16667 us) so just > > > modifying that should be sufficient, no? > > > > BeginFrameArgs::DefaultInterval is used all over the place tho, right? I find changing that for branching to be scary. > > TBH I don't think that using 59.9 Hz instead of 60 Hz will be a problem in practice. I think just disabling the use of sgi video sync is good enough. Er, I meant using 60 Hz instead of 59.9 Hz i.e. leaving the code as is except disabling the sgi video sync code.
On 2016/08/25 at 02:19:57, sunnyps wrote: > On 2016/08/25 at 02:04:52, danakj wrote: > > https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h > > File ui/gfx/vsync_provider.h (right): > > > > https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h... > > ui/gfx/vsync_provider.h:32: class GFX_EXPORT FixedVSyncProvider : public VSyncProvider { > > On 2016/08/25 01:59:29, sunnyps wrote: > > > Do we even need FixedVSyncProvider? If a vsync provider is not present the > > > synthetic BFS will use BeginFrameArgs::DefaultInterval (16667 us) so just > > > modifying that should be sufficient, no? > > > > BeginFrameArgs::DefaultInterval is used all over the place tho, right? I find changing that for branching to be scary. > > TBH I don't think that using 59.9 Hz instead of 60 Hz will be a problem in practice. I think just disabling the use of sgi video sync is good enough. Er, I meant using 60 Hz instead of 59.9 Hz i.e. leaving the code as is except disabling the sgi video sync code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
60Hz may work better - I'm not sure. Given the lack of data I'm going to stick with 59.9Hz. https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.cc File ui/gfx/vsync_provider.cc (left): https://codereview.chromium.org/2277883003/diff/1/ui/gfx/vsync_provider.cc#ol... ui/gfx/vsync_provider.cc:5: #include "ui/gfx/image/image_skia_source.h" On 2016/08/25 01:59:29, sunnyps wrote: > git cl upload marked this as a copy of ui/gfx/image/image_skia_source.cc for > some reason. Yeah. A little weird, but should be harmless. Not sure what's going on - it doesn't show up this way on my local commit. https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h File ui/gfx/vsync_provider.h (right): https://codereview.chromium.org/2277883003/diff/40001/ui/gfx/vsync_provider.h... ui/gfx/vsync_provider.h:32: class GFX_EXPORT FixedVSyncProvider : public VSyncProvider { On 2016/08/25 02:04:52, danakj wrote: > On 2016/08/25 01:59:29, sunnyps wrote: > > Do we even need FixedVSyncProvider? If a vsync provider is not present the > > synthetic BFS will use BeginFrameArgs::DefaultInterval (16667 us) so just > > modifying that should be sufficient, no? > > BeginFrameArgs::DefaultInterval is used all over the place tho, right? I find > changing that for branching to be scary. I did it this way because I like the decision to use a fixed provider to be explicit and in the same place where the other vsync providers are being selected, but Dana makes a good point about branching. Going to keep this as-is for now.
The CQ bit was checked by brianderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2277883003/#ps40001 (title: "forward flag")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by brianderson@chromium.org
The CQ bit was checked by brianderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2277883003/#ps60001 (title: "accidentally an s")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== gl: Disable SGI_video_sync by default. Disables the SGI_video_sync extension and assumes a vsync interval of 59.9Hz on Linux. Also adds an --enable-sgi-video-sync command line flag to re-enable the extension. Use of the extension on a thread other than the main GPU thread is encountering issues with Chrome's sandbox for certain combinations of Linux window managers and NVIDIA drivers. Before this patch, we were assuming a frequency of 60Hz on Linux and re-aligning the phase every 5 seconds. Since we can no longer align the phase, we assume a frequency of 59.9 Hz. On a 60Hz monitor, this avoids driver backpressure on the GPU thread at the cost of skipping a frame every 10 seconds. BUG=636644 ========== to ========== gl: Disable SGI_video_sync by default. Disables the SGI_video_sync extension and assumes a vsync interval of 59.9Hz on Linux. Also adds an --enable-sgi-video-sync command line flag to re-enable the extension. Use of the extension on a thread other than the main GPU thread is encountering issues with Chrome's sandbox for certain combinations of Linux window managers and NVIDIA drivers. Before this patch, we were assuming a frequency of 60Hz on Linux and re-aligning the phase every 5 seconds. Since we can no longer align the phase, we assume a frequency of 59.9 Hz. On a 60Hz monitor, this avoids driver backpressure on the GPU thread at the cost of skipping a frame every 10 seconds. BUG=636644 Committed: https://crrev.com/22774dfbaceb5f616183b3d6135264b458c9ed0a Cr-Commit-Position: refs/heads/master@{#414342} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/22774dfbaceb5f616183b3d6135264b458c9ed0a Cr-Commit-Position: refs/heads/master@{#414342} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
