Read compositor VSync information from platform, when possible

Read compositor VSync information from platform, when possible

The current query of VSync information through the GL context can be unreliable
on platforms that can dynamically disable vblanks, or multi-monitor setups.
Preferentially query the VSync information through the platform windowing
system (presently: XRandR on CrOS) when possible.

BUG=328953
TEST=local build, run on CrOS snow

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250250

[sheu, 2014-01-25 01:48:03]

oshima@: I'd like you to take a first look at this, at your convenience.  

Points to note:
* We now expect the "best" resolution to be returned by
DisplayManager::GetSelectedResolutionForDisplayId() if it has been selected. 
Previously, if the "best" resolution was selected, no resolution was returned,
* Added support for specifying refresh rates in
DisplayInfo::CreateFromSpecWithID().  Should be backwards-compatible (of course)

PTAL.

[danakj, 2014-01-27 18:43:53]

+some scheduling folks FYI

[oshima, 2014-01-27 18:53:21]

We exclude the best resolution because we don't want to keep it in persistent
state.

Can you either
1) Add best resolution flag to DisplayMode and don't store it if it's the best
resolution, or
2) Add GetCurrentDisplayMode() method which returns the DisplayMode for the
current display.

https://codereview.chromium.org/138903025/diff/210001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/138903025/diff/210001/ash/display/display_inf...
ash/display/display_info.h:21: struct ASH_EXPORT Resolution {
We probably should rename this to DisplayMode as it has more than resolution
now.

https://codereview.chromium.org/138903025/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/138903025/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences.cc:141:
dict_value->GetDouble("refresh-rate", &refresh_rate);
Unless you want to use this to select output mode, you don't have to save this
in persistent state.

https://codereview.chromium.org/138903025/diff/210001/chromeos/display/output...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/138903025/diff/210001/chromeos/display/output...
chromeos/display/output_configurator.h:181: float* refresh_rate) const = 0;
Looks like you're not using refresh_rate in output configurator. Do you need
this here?

[sheu, 2014-01-28 21:30:41]

Done.  The CL gets a little bigger.

PTAL

https://chromiumcodereview.appspot.com/138903025/diff/210001/ash/display/disp...
File ash/display/display_info.h (right):

https://chromiumcodereview.appspot.com/138903025/diff/210001/ash/display/disp...
ash/display/display_info.h:21: struct ASH_EXPORT Resolution {
On 2014/01/27 18:53:22, oshima wrote:
> We probably should rename this to DisplayMode as it has more than resolution
> now.

Done.

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
chrome/browser/chromeos/display/display_preferences.cc:141:
dict_value->GetDouble("refresh-rate", &refresh_rate);
On 2014/01/27 18:53:22, oshima wrote:
> Unless you want to use this to select output mode, you don't have to save this
> in persistent state.

My problem here is that RegisterDisplayProperty creates a new Resolution and
sets the display to that resolution.  If we don't provide a refresh_rate, then
it should default to 0 Hz and will stay there until the next time native modes
are passed in or SetDisplayResolution is called.  So until then we don't have
refresh rate information.

What would you suggest instead of storing the refresh rate?

https://chromiumcodereview.appspot.com/138903025/diff/210001/chromeos/display...
File chromeos/display/output_configurator.h (right):

https://chromiumcodereview.appspot.com/138903025/diff/210001/chromeos/display...
chromeos/display/output_configurator.h:181: float* refresh_rate) const = 0;
On 2014/01/27 18:53:22, oshima wrote:
> Looks like you're not using refresh_rate in output configurator. Do you need
> this here?

Done.

[oshima, 2014-01-28 22:17:08]

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
chrome/browser/chromeos/display/display_preferences.cc:141:
dict_value->GetDouble("refresh-rate", &refresh_rate);
On 2014/01/28 21:30:42, sheu wrote:
> On 2014/01/27 18:53:22, oshima wrote:
> > Unless you want to use this to select output mode, you don't have to save
this
> > in persistent state.
> 
> My problem here is that RegisterDisplayProperty creates a new Resolution and
> sets the display to that resolution.  If we don't provide a refresh_rate, then
> it should default to 0 Hz and will stay there until the next time native modes
> are passed in or SetDisplayResolution is called.  So until then we don't have
> refresh rate information.
> 
> What would you suggest instead of storing the refresh rate?

I believe your problem was that you didn't add the mode in
DisplayManager::OnNativeDisplayChanged. Please see my comment there.

You should also provide some rate as default because chrome can boot with zero
displays. (in which case, it'll use the same display info created from empty
spec)

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
File ash/display/display_info.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
ash/display/display_info.cc:131: native = true;
This will make the specified resolution always the native one, right?
We should just use the best one as a native resolution instead. (and that's
consistent with real impl)

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
File ash/display/display_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
ash/display/display_manager.cc:578: display_modes_.erase(iter->id());
since we changed to remember the best mode, you need to add the mode here.

[sheu, 2014-01-29 00:14:10]

Updated.  PTAL

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
chrome/browser/chromeos/display/display_preferences.cc:141:
dict_value->GetDouble("refresh-rate", &refresh_rate);
On 2014/01/28 22:17:08, oshima wrote:
> On 2014/01/28 21:30:42, sheu wrote:
> > On 2014/01/27 18:53:22, oshima wrote:
> > > Unless you want to use this to select output mode, you don't have to save
> this
> > > in persistent state.
> > 
> > My problem here is that RegisterDisplayProperty creates a new Resolution and
> > sets the display to that resolution.  If we don't provide a refresh_rate,
then
> > it should default to 0 Hz and will stay there until the next time native
modes
> > are passed in or SetDisplayResolution is called.  So until then we don't
have
> > refresh rate information.
> > 
> > What would you suggest instead of storing the refresh rate?
> 
> I believe your problem was that you didn't add the mode in
> DisplayManager::OnNativeDisplayChanged. Please see my comment there.
> 
> You should also provide some rate as default because chrome can boot with zero
> displays. (in which case, it'll use the same display info created from empty
> spec)

I've added a default, but I'm not sure what you mean about adding the mode.  I
think I already did -- see comment there.

A related question: RegisterDisplayProperty() seems just to be a way to
short-circuit the native display detection and insert a modeline into the
DisplayManager.  Is this the correct interpretation?

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
File ash/display/display_info.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
ash/display/display_info.cc:131: native = true;
On 2014/01/28 22:17:09, oshima wrote:
> This will make the specified resolution always the native one, right?
> We should just use the best one as a native resolution instead. (and that's
> consistent with real impl)

OK, will do.

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
File ash/display/display_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/500001/ash/display/disp...
ash/display/display_manager.cc:578: display_modes_.erase(iter->id());
On 2014/01/28 22:17:09, oshima wrote:
> since we changed to remember the best mode, you need to add the mode here.

Not sure what you mean here.  I'm adding the mode at l.576

[oshima, 2014-01-29 01:15:25]

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/210001/chrome/browser/c...
chrome/browser/chromeos/display/display_preferences.cc:141:
dict_value->GetDouble("refresh-rate", &refresh_rate);
On 2014/01/29 00:14:10, sheu wrote:
> On 2014/01/28 22:17:08, oshima wrote:
> > On 2014/01/28 21:30:42, sheu wrote:
> > > On 2014/01/27 18:53:22, oshima wrote:
> > > > Unless you want to use this to select output mode, you don't have to
save
> > this
> > > > in persistent state.
> > > 
> > > My problem here is that RegisterDisplayProperty creates a new Resolution
and
> > > sets the display to that resolution.  If we don't provide a refresh_rate,
> then
> > > it should default to 0 Hz and will stay there until the next time native
> modes
> > > are passed in or SetDisplayResolution is called.  So until then we don't
> have
> > > refresh rate information.
> > > 
> > > What would you suggest instead of storing the refresh rate?
> > 
> > I believe your problem was that you didn't add the mode in
> > DisplayManager::OnNativeDisplayChanged. Please see my comment there.
> > 
> > You should also provide some rate as default because chrome can boot with
zero
> > displays. (in which case, it'll use the same display info created from empty
> > spec)
> 
> I've added a default, but I'm not sure what you mean about adding the mode.  I
> think I already did -- see comment there.

Maybe I misread the code, but if you indeed added it, then you shouldn't have to
store the refresh rate. Here is roughly what is happening.

1) RegisterDisplayProperty sets the display mode stored in the previous run.
2) In OutputConfigrator::Start, it will read the selected resolution from
DisplayManager and
  configure the displays.
3) Which will call DisplayManager::OnNativeDisplayChanged, which will update the
   display_mode_ with the correct refresh rate.
4) DisplayController creates a root window and updates the refresh rate using
  the DisplayMode stored in DisplayManager.

If it's not working, then something else must be broken.

> A related question: RegisterDisplayProperty() seems just to be a way to
> short-circuit the native display detection and insert a modeline into the
> DisplayManager.  Is this the correct interpretation?

I'm not sure what you mean by "short-circuit". This is necessary to properly
restore
the display settings (from previous run) when chrome starts. That is, you need
to know
which resolution chrome should configure the display to when it starts.

[sheu, 2014-01-29 05:42:15]

Alrighty, updated.  I hoped I've addressed all the points.  Thank you for
explaining the use of RegisterDisplayProperty().

PTAL.

[oshima, 2014-01-29 19:00:35]

just a few more nits.

https://codereview.chromium.org/138903025/diff/560001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/138903025/diff/560001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:41: // one comes first.
can you update the comment?

https://codereview.chromium.org/138903025/diff/560001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:67: // Add the resolution if it
isn't already present and override interlaced
s/resolution/DisplayMode

https://codereview.chromium.org/138903025/diff/560001/chrome/browser/chromeos...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/138903025/diff/560001/chrome/browser/chromeos...
chrome/browser/chromeos/display/display_preferences_unittest.cc:381:
EXPECT_TRUE(property->GetFloat("refresh-rate", &refresh_rate);
you don't need this right?

https://codereview.chromium.org/138903025/diff/560001/chromeos/display/output...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/138903025/diff/560001/chromeos/display/output...
chromeos/display/output_configurator.h:177: // Hz to select output mode for the
given display id.
revert this change?

https://codereview.chromium.org/138903025/diff/560001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/138903025/diff/560001/ui/compositor/composito...
ui/compositor/compositor.cc:197: schedule_draw_factory_(this) {
initialize authoritative_vsync_interval_

[sheu, 2014-01-29 22:43:35]

piman@: PTAL at content/browser/compositor/browser_compositor_output_surface.cc
and ui/compositor/compositor.* and let me know if this is the optimal way to do
this.

https://chromiumcodereview.appspot.com/138903025/diff/560001/ash/display/disp...
File ash/display/display_change_observer_chromeos.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/560001/ash/display/disp...
ash/display/display_change_observer_chromeos.cc:41: // one comes first.
On 2014/01/29 19:00:35, oshima wrote:
> can you update the comment?

Done.

https://chromiumcodereview.appspot.com/138903025/diff/560001/ash/display/disp...
ash/display/display_change_observer_chromeos.cc:67: // Add the resolution if it
isn't already present and override interlaced
On 2014/01/29 19:00:35, oshima wrote:
> s/resolution/DisplayMode

Done.

https://chromiumcodereview.appspot.com/138903025/diff/560001/chrome/browser/c...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/560001/chrome/browser/c...
chrome/browser/chromeos/display/display_preferences_unittest.cc:381:
EXPECT_TRUE(property->GetFloat("refresh-rate", &refresh_rate);
On 2014/01/29 19:00:35, oshima wrote:
> you don't need this right?

Done.

https://chromiumcodereview.appspot.com/138903025/diff/560001/ui/compositor/co...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/560001/ui/compositor/co...
ui/compositor/compositor.cc:197: schedule_draw_factory_(this) {
On 2014/01/29 19:00:35, oshima wrote:
> initialize authoritative_vsync_interval_

The default constructor initializes to a 0 internal value already.  Should this
still be necessary?

[oshima, 2014-01-29 22:45:31]

lgtm

https://chromiumcodereview.appspot.com/138903025/diff/560001/ui/compositor/co...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/560001/ui/compositor/co...
ui/compositor/compositor.cc:197: schedule_draw_factory_(this) {
On 2014/01/29 22:43:36, sheu wrote:
> On 2014/01/29 19:00:35, oshima wrote:
> > initialize authoritative_vsync_interval_
> 
> The default constructor initializes to a 0 internal value already.  Should
this
> still be necessary?

Ah, it's TimeDelta. nvm.

[sheu, 2014-02-03 21:53:02]

Ping!  I'd like to get thoughts on the compositor change from compositor folks.

[brianderson, 2014-02-03 22:56:03]

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:124: if
(GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode)) {
Will this be called every frame? If not, how often will it be called?

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:126: base::TimeTicks(),
Why isn't there timebase information here?

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:129: true);
Is this the only place where the authoritative interval is true?

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
ui/compositor/compositor.cc:440: }
The if/else above means that we always set overwrite interval to the last
authoritative interval, even if interval is a valid value.

Is that what you intended? Why?

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
ui/compositor/compositor.cc:442: if (!timebase.is_null()) {
Is this check actually needed?

On Windows, we currently always set the timebase to 0, which means we'll never
tell the Renderers of an updated vsync interval.

Some background on why: Windows provides the vsync timebase in a high-res
timebase, but scheduling is done in a low-res timebase. To avoid
inconsistencies, we are using a timebase of 0 until scheduling is also high-res.

[piman, 2014-02-03 23:01:38]

I'll defer to Brian about the subtleties of the vsync info. I'll do a
sanity check & rubber-stamp after he's happy.


On Mon, Feb 3, 2014 at 2:56 PM, <brianderson@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc
> File ash/display/display_controller.cc (right):
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc#newcode124
> ash/display/display_controller.cc:124: if
> (GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode))
> {
> Will this be called every frame? If not, how often will it be called?
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc#newcode126
> ash/display/display_controller.cc:126: base::TimeTicks(),
> Why isn't there timebase information here?
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc#newcode129
> ash/display/display_controller.cc:129: true);
> Is this the only place where the authoritative interval is true?
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ui/compositor/compositor.cc
> File ui/compositor/compositor.cc (right):
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ui/compositor/compositor.cc#newcode440
> ui/compositor/compositor.cc:440: }
> The if/else above means that we always set overwrite interval to the
> last authoritative interval, even if interval is a valid value.
>
> Is that what you intended? Why?
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ui/compositor/compositor.cc#newcode442
> ui/compositor/compositor.cc:442: if (!timebase.is_null()) {
> Is this check actually needed?
>
> On Windows, we currently always set the timebase to 0, which means we'll
> never tell the Renderers of an updated vsync interval.
>
> Some background on why: Windows provides the vsync timebase in a
> high-res timebase, but scheduling is done in a low-res timebase. To
> avoid inconsistencies, we are using a timebase of 0 until scheduling is
> also high-res.
>
> https://chromiumcodereview.appspot.com/138903025/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sheu, 2014-02-03 23:02:39]

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:124: if
(GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode)) {
On 2014/02/03 22:56:04, Brian Anderson wrote:
> Will this be called every frame? If not, how often will it be called?

This should be called only when monitor configuration changes.

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:126: base::TimeTicks(),
On 2014/02/03 22:56:04, Brian Anderson wrote:
> Why isn't there timebase information here?

We don't have access to timebase information here, so we can't assign it a
value.

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:129: true);
On 2014/02/03 22:56:04, Brian Anderson wrote:
> Is this the only place where the authoritative interval is true?

Yes.

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
ui/compositor/compositor.cc:440: }
On 2014/02/03 22:56:04, Brian Anderson wrote:
> The if/else above means that we always set overwrite interval to the last
> authoritative interval, even if interval is a valid value.
> 
> Is that what you intended? Why?

Yes this is intentional.  The idea is that we can keep feeding the compositor
timebase values, but it will use the authoritative one instead if there is one. 
In the particular case I'm considering, the EGL context supplies us with the
timebase and an interval that may be inaccurate; so if the window manager can
supply a better interval, we'd rather use that.

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
ui/compositor/compositor.cc:442: if (!timebase.is_null()) {
On 2014/02/03 22:56:04, Brian Anderson wrote:
> Is this check actually needed?
> 
> On Windows, we currently always set the timebase to 0, which means we'll never
> tell the Renderers of an updated vsync interval.
> 
> Some background on why: Windows provides the vsync timebase in a high-res
> timebase, but scheduling is done in a low-res timebase. To avoid
> inconsistencies, we are using a timebase of 0 until scheduling is also
high-res.

Would it make more sense to add to the ui::Compositor different entry points for
OnUpdateVSyncParameters() and, say, a SetAuthoritativeInterval() or some such? 
It seems a little odd to have this carried in the compositor, so I'm not sure
where the proper place would be to put it.

[piman, 2014-02-03 23:17:10]

On Mon, Feb 3, 2014 at 3:02 PM, <sheu@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc
> File ash/display/display_controller.cc (right):
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc#newcode124
> ash/display/display_controller.cc:124: if
> (GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode))
> {
> On 2014/02/03 22:56:04, Brian Anderson wrote:
>
>> Will this be called every frame? If not, how often will it be called?
>>
>
> This should be called only when monitor configuration changes.
>
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc#newcode126
> ash/display/display_controller.cc:126: base::TimeTicks(),
> On 2014/02/03 22:56:04, Brian Anderson wrote:
>
>> Why isn't there timebase information here?
>>
>
> We don't have access to timebase information here, so we can't assign it
> a value.
>
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ash/display/display_controller.cc#newcode129
> ash/display/display_controller.cc:129: true);
> On 2014/02/03 22:56:04, Brian Anderson wrote:
>
>> Is this the only place where the authoritative interval is true?
>>
>
> Yes.
>
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ui/compositor/compositor.cc
> File ui/compositor/compositor.cc (right):
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ui/compositor/compositor.cc#newcode440
> ui/compositor/compositor.cc:440: }
> On 2014/02/03 22:56:04, Brian Anderson wrote:
>
>> The if/else above means that we always set overwrite interval to the
>>
> last
>
>> authoritative interval, even if interval is a valid value.
>>
>
>  Is that what you intended? Why?
>>
>
> Yes this is intentional.  The idea is that we can keep feeding the
> compositor timebase values, but it will use the authoritative one
> instead if there is one.  In the particular case I'm considering, the
> EGL context supplies us with the timebase and an interval that may be
> inaccurate; so if the window manager can supply a better interval, we'd
> rather use that.
>
>
> https://chromiumcodereview.appspot.com/138903025/diff/
> 600001/ui/compositor/compositor.cc#newcode442
> ui/compositor/compositor.cc:442: if (!timebase.is_null()) {
> On 2014/02/03 22:56:04, Brian Anderson wrote:
>
>> Is this check actually needed?
>>
>
>  On Windows, we currently always set the timebase to 0, which means
>>
> we'll never
>
>> tell the Renderers of an updated vsync interval.
>>
>
>  Some background on why: Windows provides the vsync timebase in a
>>
> high-res
>
>> timebase, but scheduling is done in a low-res timebase. To avoid
>> inconsistencies, we are using a timebase of 0 until scheduling is also
>>
> high-res.
>
> Would it make more sense to add to the ui::Compositor different entry
> points for OnUpdateVSyncParameters() and, say, a
> SetAuthoritativeInterval() or some such?  It seems a little odd to have
> this carried in the compositor, so I'm not sure where the proper place
> would be to put it.
>

It may be cleaner to have an explicit SetAuthoritativeInterval().
Ideally this logic would be in the BrowserCompositorOutputSurface, but it's
hard to expose it outside of content/ in a meaningful way, so maybe
Compositor is a reasonable place.


> https://chromiumcodereview.appspot.com/138903025/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-02-03 23:18:57]

On Mon, Feb 3, 2014 at 3:16 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Mon, Feb 3, 2014 at 3:02 PM, <sheu@chromium.org> wrote:
>
>>
>> https://chromiumcodereview.appspot.com/138903025/diff/
>> 600001/ash/display/display_controller.cc
>> File ash/display/display_controller.cc (right):
>>
>> https://chromiumcodereview.appspot.com/138903025/diff/
>> 600001/ash/display/display_controller.cc#newcode124
>> ash/display/display_controller.cc:124: if
>> (GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode))
>> {
>> On 2014/02/03 22:56:04, Brian Anderson wrote:
>>
>>> Will this be called every frame? If not, how often will it be called?
>>>
>>
>> This should be called only when monitor configuration changes.
>>
>>
>> https://chromiumcodereview.appspot.com/138903025/diff/
>> 600001/ash/display/display_controller.cc#newcode126
>> ash/display/display_controller.cc:126: base::TimeTicks(),
>> On 2014/02/03 22:56:04, Brian Anderson wrote:
>>
>>> Why isn't there timebase information here?
>>>
>>
>> We don't have access to timebase information here, so we can't assign it
>> a value.
>>
>>
>> https://chromiumcodereview.appspot.com/138903025/diff/
>> 600001/ash/display/display_controller.cc#newcode129
>> ash/display/display_controller.cc:129: true);
>> On 2014/02/03 22:56:04, Brian Anderson wrote:
>>
>>> Is this the only place where the authoritative interval is true?
>>>
>>
>> Yes.
>>
>>
>> https://chromiumcodereview.appspot.com/138903025/diff/
>> 600001/ui/compositor/compositor.cc
>> File ui/compositor/compositor.cc (right):
>>
>> https://chromiumcodereview.appspot.com/138903025/diff/
>> 600001/ui/compositor/compositor.cc#newcode440
>> ui/compositor/compositor.cc:440: }
>> On 2014/02/03 22:56:04, Brian Anderson wrote:
>>
>>> The if/else above means that we always set overwrite interval to the
>>>
>> last
>>
>>> authoritative interval, even if interval is a valid value.
>>>
>>
>>  Is that what you intended? Why?
>>>
>>
>> Yes this is intentional.  The idea is that we can keep feeding the
>> compositor timebase values, but it will use the authoritative one
>> instead if there is one.  In the particular case I'm considering, the
>> EGL context supplies us with the timebase and an interval that may be
>> inaccurate; so if the window manager can supply a better interval, we'd
>> rather use that.
>>
>>
>> https://chromiumcodereview.appspot.com/138903025/diff/
>> 600001/ui/compositor/compositor.cc#newcode442
>> ui/compositor/compositor.cc:442: if (!timebase.is_null()) {
>> On 2014/02/03 22:56:04, Brian Anderson wrote:
>>
>>> Is this check actually needed?
>>>
>>
>>  On Windows, we currently always set the timebase to 0, which means
>>>
>> we'll never
>>
>>> tell the Renderers of an updated vsync interval.
>>>
>>
>>  Some background on why: Windows provides the vsync timebase in a
>>>
>> high-res
>>
>>> timebase, but scheduling is done in a low-res timebase. To avoid
>>> inconsistencies, we are using a timebase of 0 until scheduling is also
>>>
>> high-res.
>>
>> Would it make more sense to add to the ui::Compositor different entry
>> points for OnUpdateVSyncParameters() and, say, a
>> SetAuthoritativeInterval() or some such?  It seems a little odd to have
>> this carried in the compositor, so I'm not sure where the proper place
>> would be to put it.
>>
>
> It may be cleaner to have an explicit SetAuthoritativeInterval().
> Ideally this logic would be in the BrowserCompositorOutputSurface, but
> it's hard to expose it outside of content/ in a meaningful way, so maybe
> Compositor is a reasonable place.
>

Mmh, except that Compositor/ will only feed its observers with that info,
i.e. the RenderWidgetHostViewAura for the renderers, not the output surface
for the compositor. Which means the browser and renderer may use a
different rate and that's not a good thing.

>
>
>> https://chromiumcodereview.appspot.com/138903025/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-02-04 01:39:50]

I would prefer to see a separate entry point which would clarify and simplify
the logic, but I don't know the best location for that entry point.

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:124: if
(GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode)) {
On 2014/02/03 23:02:40, sheu wrote:
> On 2014/02/03 22:56:04, Brian Anderson wrote:
> > Will this be called every frame? If not, how often will it be called?
> 
> This should be called only when monitor configuration changes.

Does it happen in all the following cases:
When a window changes monitors.
When the monitor lowers its frequency to save power.
When the user manually selects a monitor config.

Also, does this code path only apply to ChromeOS?

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ui/compositor/co...
ui/compositor/compositor.cc:440: }
On 2014/02/03 23:02:40, sheu wrote:
> On 2014/02/03 22:56:04, Brian Anderson wrote:
> > The if/else above means that we always set overwrite interval to the last
> > authoritative interval, even if interval is a valid value.
> > 
> > Is that what you intended? Why?
> 
> Yes this is intentional.  The idea is that we can keep feeding the compositor
> timebase values, but it will use the authoritative one instead if there is
one. 
> In the particular case I'm considering, the EGL context supplies us with the
> timebase and an interval that may be inaccurate; so if the window manager can
> supply a better interval, we'd rather use that.

How is the interval inaccurate? Is there a way to test for inaccuracy?

[oshima, 2014-02-04 02:07:53]

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:124: if
(GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode)) {
On 2014/02/04 01:39:51, Brian Anderson wrote:
> On 2014/02/03 23:02:40, sheu wrote:
> > On 2014/02/03 22:56:04, Brian Anderson wrote:
> > > Will this be called every frame? If not, how often will it be called?
> > 
> > This should be called only when monitor configuration changes.
> 
> Does it happen in all the following cases:
> When a window changes monitors.

No

> When the monitor lowers its frequency to save power.

Currently no, although it's not difficult to support this.
(just need to check refresh rate in DisplayManager::UpdateDisplays)

> When the user manually selects a monitor config.

Yes

> 
> Also, does this code path only apply to ChromeOS?

Yes

[brianderson, 2014-02-04 02:14:35]

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/600001/ash/display/disp...
ash/display/display_controller.cc:124: if
(GetDisplayManager()->GetSelectedModeForDisplayId(display.id(), &mode)) {
On 2014/02/04 02:07:53, oshima wrote:
> On 2014/02/04 01:39:51, Brian Anderson wrote:
> > On 2014/02/03 23:02:40, sheu wrote:
> > > On 2014/02/03 22:56:04, Brian Anderson wrote:
> > > > Will this be called every frame? If not, how often will it be called?
> > > 
> > > This should be called only when monitor configuration changes.
> > 
> > Does it happen in all the following cases:
> > When a window changes monitors.
> 
> No

Is that beca

> 
> > When the monitor lowers its frequency to save power.
> 
> Currently no, although it's not difficult to support this.
> (just need to check refresh rate in DisplayManager::UpdateDisplays)
> 
> > When the user manually selects a monitor config.
> 
> Yes
> 
> > 
> > Also, does this code path only apply to ChromeOS?
> 
> Yes
> 

For all instances where the answer is "No", is the answer "Yes" for the original
code path?

[sheu, 2014-02-04 03:23:21]

Updated.  Requesting suggestions in 2 parts.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
content/browser/renderer_host/render_widget_host_view_aura.cc:3516:
compositor->vsync_manager()->AddObserver(this);
Not sure where I should be adding and removing this as an observer as the vsync
manager, that would guarantee that we don't miss an add/remove.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
content/browser/renderer_host/render_widget_host_view_aura.cc:3544:
compositor->vsync_manager()->RemoveObserver(this);
See above.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
File ui/compositor/compositor_vsync_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:36: // hang the UI.
This is probably not the best place to kill the process, since it nuke the
browser process at this point.  Doing this in FrameRateController would also
kill the browser process, since the browser process also has one.

Should I try:
1. Killing in CompositorOutputSurface (on the renderer side), and
2. Somehow terminating the GPU process in BrowserCompositorOutputSurface (on the
browser side)?

A bit of duplication of code there.  Suggestions welcome.

[piman, 2014-02-04 04:51:00]

https://chromiumcodereview.appspot.com/138903025/diff/800001/ash/display/disp...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ash/display/disp...
ash/display/display_controller.cc:128: mode.refresh_rate));
I think there's some cases where refresh_rate could be 0, in which case we
should skip the SetAuthoritativeVSyncInterval altogether.

https://chromiumcodereview.appspot.com/138903025/diff/800001/chrome/browser/u...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/chrome/browser/u...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:238:
resolution_info->SetInteger("height", resolution.height());
nit: it'd be nice to surface the refresh rate here since we have it.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
content/browser/compositor/browser_compositor_output_surface.cc:22:
ui::Compositor* compositor)
nit: pass in the vsync_manager() to the constructor, to make it explicit in the
interface that we're not keeping a reference to the Compositor (which lives on
the UI thread) on the compositor thread.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
content/browser/renderer_host/render_widget_host_view_aura.cc:3516:
compositor->vsync_manager()->AddObserver(this);
On 2014/02/04 03:23:21, sheu wrote:
> Not sure where I should be adding and removing this as an observer as the
vsync
> manager, that would guarantee that we don't miss an add/remove.

This is I think the right place.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
content/browser/renderer_host/render_widget_host_view_aura.cc:3544:
compositor->vsync_manager()->RemoveObserver(this);
On 2014/02/04 03:23:21, sheu wrote:
> See above.

Here too.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/renderer...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/renderer...
content/renderer/gpu/compositor_output_surface.cc:142:
OnUpdateVSyncParametersFromView);
nit: gratuitous rename? Not sure which "FromView" refers to.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
File ui/compositor/compositor_vsync_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:23: authoritative_vsync_interval_ =
interval;
nit: I think we should update the observers at this point too.
We can keep the timebase when UpdateVSyncParameters was called.

The risk is that a very bogus interval might get through from the VSyncProvider,
which would then essentially prevent the compositor from swapping again (since
the deadline would be far into the future), which means we wouldn't call
UpdateVSyncParameters again to update the interval (since that's triggered by
the swap).

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:36: // hang the UI.
On 2014/02/04 03:23:21, sheu wrote:
> This is probably not the best place to kill the process, since it nuke the
> browser process at this point.  Doing this in FrameRateController would also
> kill the browser process, since the browser process also has one.
> 
> Should I try:
> 1. Killing in CompositorOutputSurface (on the renderer side), and
> 2. Somehow terminating the GPU process in BrowserCompositorOutputSurface (on
the
> browser side)?
> 
> A bit of duplication of code there.  Suggestions welcome.

If anything, the assert should go to SetAuthoritativeVSyncInterval, so that it
triggers when we have a chance to look at the stack, rather than too late.
Ideally this would actually go up to the source of the refresh interval.

We mainly introduced this assert in the GPU process to catch:
1- bogus drivers that send an invalid rate even though it returns success
2- bogus assumption in the code that estimates the rate with successive
timebase.

2 doesn't apply for the "authoritative vsync interval" that comes from RR, and
Stéphane tells me that if the RR info is bogus, there's probably a lot more
things going wrong.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:50:
observer_list_->AddObserver(observer);
It would be reasonable to Notify the observer here, and/or have a way for the
observer to query the current timebase/interval, so that the RWHVA can update
the renderer as it changes screens.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
File ui/compositor/compositor_vsync_manager.h (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.h:60: ~CompositorVSyncManager();
nit: methods before fields.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/gl/sync_contr...
File ui/gl/sync_control_vsync_provider.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/gl/sync_contr...
ui/gl/sync_control_vsync_provider.cc:121: last_good_interval_ = new_interval;
I'd rather we either leave the LOG(FATAL) here, or we remove the logic to
estimate the rate from the intervals altogether.

[sheu, 2014-02-04 22:01:43]

Updated.  piman@: PTAL

https://chromiumcodereview.appspot.com/138903025/diff/800001/ash/display/disp...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ash/display/disp...
ash/display/display_controller.cc:128: mode.refresh_rate));
On 2014/02/04 04:51:01, piman wrote:
> I think there's some cases where refresh_rate could be 0, in which case we
> should skip the SetAuthoritativeVSyncInterval altogether.

And the div-by-zero.  Done.

https://chromiumcodereview.appspot.com/138903025/diff/800001/chrome/browser/u...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/chrome/browser/u...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:238:
resolution_info->SetInteger("height", resolution.height());
On 2014/02/04 04:51:01, piman wrote:
> nit: it'd be nice to surface the refresh rate here since we have it.

Sure.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/browser/...
content/browser/compositor/browser_compositor_output_surface.cc:22:
ui::Compositor* compositor)
On 2014/02/04 04:51:01, piman wrote:
> nit: pass in the vsync_manager() to the constructor, to make it explicit in
the
> interface that we're not keeping a reference to the Compositor (which lives on
> the UI thread) on the compositor thread.

I was under the impression that passing a raw pointer makes this kind of
implication in Chrome-land.  But passing vsync_manager() is probably better
anyhow.

Done.

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/renderer...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/renderer...
content/renderer/gpu/compositor_output_surface.cc:142:
OnUpdateVSyncParametersFromView);
On 2014/02/04 04:51:01, piman wrote:
> nit: gratuitous rename? Not sure which "FromView" refers to.

A little bit convoluted.  See comment in BrowserCompositorOutputSurface.  Short
answer here: because the BrowserCompositorOutputSurface version has been renamed
to OnUpdateVSyncParametersFromGpu().

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
File ui/compositor/compositor_vsync_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:23: authoritative_vsync_interval_ =
interval;
On 2014/02/04 04:51:01, piman wrote:
> nit: I think we should update the observers at this point too.
> We can keep the timebase when UpdateVSyncParameters was called.
> 
> The risk is that a very bogus interval might get through from the
VSyncProvider,
> which would then essentially prevent the compositor from swapping again (since
> the deadline would be far into the future), which means we wouldn't call
> UpdateVSyncParameters again to update the interval (since that's triggered by
> the swap).

Sure, we'll do that.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:36: // hang the UI.
On 2014/02/04 04:51:01, piman wrote:
> On 2014/02/04 03:23:21, sheu wrote:
> > This is probably not the best place to kill the process, since it nuke the
> > browser process at this point.  Doing this in FrameRateController would also
> > kill the browser process, since the browser process also has one.
> > 
> > Should I try:
> > 1. Killing in CompositorOutputSurface (on the renderer side), and
> > 2. Somehow terminating the GPU process in BrowserCompositorOutputSurface (on
> the
> > browser side)?
> > 
> > A bit of duplication of code there.  Suggestions welcome.
> 
> If anything, the assert should go to SetAuthoritativeVSyncInterval, so that it
> triggers when we have a chance to look at the stack, rather than too late.
> Ideally this would actually go up to the source of the refresh interval.
> 
> We mainly introduced this assert in the GPU process to catch:
> 1- bogus drivers that send an invalid rate even though it returns success
> 2- bogus assumption in the code that estimates the rate with successive
> timebase.
> 
> 2 doesn't apply for the "authoritative vsync interval" that comes from RR, and
> Stéphane tells me that if the RR info is bogus, there's probably a lot more
> things going wrong.

I've moved the check so that it's run in both this and the
SetAuthoritativeVSyncInterval cases.  The question still remains though -- do we
want to kill the browser process if we hit this?  Previously it was just the GPU
process.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:50:
observer_list_->AddObserver(observer);
On 2014/02/04 04:51:01, piman wrote:
> It would be reasonable to Notify the observer here, and/or have a way for the
> observer to query the current timebase/interval, so that the RWHVA can update
> the renderer as it changes screens.

Done.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
File ui/compositor/compositor_vsync_manager.h (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.h:60: ~CompositorVSyncManager();
On 2014/02/04 04:51:01, piman wrote:
> nit: methods before fields.

Done.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/gl/sync_contr...
File ui/gl/sync_control_vsync_provider.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/gl/sync_contr...
ui/gl/sync_control_vsync_provider.cc:121: last_good_interval_ = new_interval;
On 2014/02/04 04:51:01, piman wrote:
> I'd rather we either leave the LOG(FATAL) here, or we remove the logic to
> estimate the rate from the intervals altogether.

We can't leave the LOG(FATAL) here -- that is the bug we're trying to fix, i.e.
in the case that we get a bogus interval here we can still rely on the interval
queried from the platform display manager (e.g. XRandR).

And we still need to leave the rate estimation in here for platforms that don't
currently run Ash (and for which we have no XRandR hookup).

[sheu, 2014-02-04 22:05:12]

https://chromiumcodereview.appspot.com/138903025/diff/920001/chrome/browser/u...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/138903025/diff/920001/chrome/browser/u...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:241:
display_modes[i].refresh_rate);
Will fix braces in rebase before submit.

[sheu, 2014-02-04 22:44:31]

Fixed.  PTAL

[piman, 2014-02-04 22:52:53]

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/renderer...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/content/renderer...
content/renderer/gpu/compositor_output_surface.cc:142:
OnUpdateVSyncParametersFromView);
On 2014/02/04 22:01:45, sheu wrote:
> On 2014/02/04 04:51:01, piman wrote:
> > nit: gratuitous rename? Not sure which "FromView" refers to.
> 
> A little bit convoluted.  See comment in BrowserCompositorOutputSurface. 
Short
> answer here: because the BrowserCompositorOutputSurface version has been
renamed
> to OnUpdateVSyncParametersFromGpu().

Note: BrowserCompositorOutputSurface doesn't derive from
CompositorOutputSurface, so there's no conflict or anything.

"View" in the renderer context usually refers to RenderView or WebView, which is
mostly unrelated to the OutputSurface, so it's weird. We usually name message
handlers after the messages they handle. If you really feel like you need to
rename this, FromBrowser would carry clearer information than FromView.

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
File ui/compositor/compositor_vsync_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/800001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:36: // hang the UI.
On 2014/02/04 22:01:45, sheu wrote:
> On 2014/02/04 04:51:01, piman wrote:
> > On 2014/02/04 03:23:21, sheu wrote:
> > > This is probably not the best place to kill the process, since it nuke the
> > > browser process at this point.  Doing this in FrameRateController would
also
> > > kill the browser process, since the browser process also has one.
> > > 
> > > Should I try:
> > > 1. Killing in CompositorOutputSurface (on the renderer side), and
> > > 2. Somehow terminating the GPU process in BrowserCompositorOutputSurface
(on
> > the
> > > browser side)?
> > > 
> > > A bit of duplication of code there.  Suggestions welcome.
> > 
> > If anything, the assert should go to SetAuthoritativeVSyncInterval, so that
it
> > triggers when we have a chance to look at the stack, rather than too late.
> > Ideally this would actually go up to the source of the refresh interval.
> > 
> > We mainly introduced this assert in the GPU process to catch:
> > 1- bogus drivers that send an invalid rate even though it returns success
> > 2- bogus assumption in the code that estimates the rate with successive
> > timebase.
> > 
> > 2 doesn't apply for the "authoritative vsync interval" that comes from RR,
and
> > Stéphane tells me that if the RR info is bogus, there's probably a lot more
> > things going wrong.
> 
> I've moved the check so that it's run in both this and the
> SetAuthoritativeVSyncInterval cases.  The question still remains though -- do
we
> want to kill the browser process if we hit this?  Previously it was just the
GPU
> process.

I don't think we want to kill the browser process in response to a bogus GPU
message.
Previously, killing the GPU process would lose the contexts and would unblock
the draw loop in case the interval was off the charts. User would see a minor
glitch, but could otherwise continue. This would be a lot more dramatic,
possibly losing data.

https://chromiumcodereview.appspot.com/138903025/diff/920001/ui/compositor/co...
File ui/compositor/compositor_vsync_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/920001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:24: NotifyObservers(last_timebase_,
authoritative_vsync_interval_);
nit: release the lock before NotifyObservers, to make sure we don't deadlock if
the observer reenters.

https://chromiumcodereview.appspot.com/138903025/diff/920001/ui/compositor/co...
ui/compositor/compositor_vsync_manager.cc:58: last_interval_ = interval;
acquire lock while modifying these 2.

[piman, 2014-02-04 22:54:15]

LGTM with naming nit.

[sheu, 2014-02-04 23:41:01]

More OWNERs checks.

mukai@: chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
sky@: trivial removal of an entry point.  ui/aura/bench/bench_main.cc

[sheu, 2014-02-04 23:41:15]

PTAL, thanks!

[piman, 2014-02-04 23:44:26]

lgtm

[Jun Mukai, 2014-02-04 23:57:35]

lgtm++

[sky, 2014-02-05 01:03:58]

ui/aura/bench/bench_main.cc LGTM

[sheu, 2014-02-05 01:30:54]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-05 03:48:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/1380001

[commit-bot: I haz the power, 2014-02-05 03:49:11]

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

[commit-bot: I haz the power, 2014-02-05 03:49:12]

Failed to apply patch for
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
  Hunk #1 succeeded at 7 with fuzz 1.
  Hunk #2 succeeded at 69 (offset -1 lines).
  Hunk #3 FAILED at 81.
  Hunk #4 succeeded at 114 (offset -2 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc.rej

Patch:      
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
Index:
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
diff --git
a/content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
b/content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
index
9bc2a01d4c67e38849b202e49f7f9e0501f64f84..c12f1d6212f9780016e3e7cc393ab706dd89fb23
100644
---
a/content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
+++
b/content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
@@ -7,6 +7,7 @@
 #include "content/browser/compositor/browser_compositor_output_surface_proxy.h"
 #include
"content/browser/compositor/software_browser_compositor_output_surface.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/compositor/compositor.h"
 #include "ui/compositor/test/context_factories_for_test.h"
 #include "ui/gfx/vsync_provider.h"
 #include "ui/gl/gl_implementation.h"
@@ -69,7 +70,6 @@ class SoftwareBrowserCompositorOutputSurfaceTest : public
testing::Test {
  protected:
   scoped_ptr<content::BrowserCompositorOutputSurface> output_surface_;
 
-  scoped_ptr<base::MessageLoop> message_loop_;
   scoped_ptr<ui::Compositor> compositor_;
 
   IDMap<content::BrowserCompositorOutputSurface> surface_map_;
@@ -81,7 +81,6 @@ class SoftwareBrowserCompositorOutputSurfaceTest : public
testing::Test {
 SoftwareBrowserCompositorOutputSurfaceTest::
     SoftwareBrowserCompositorOutputSurfaceTest() {
   CHECK(gfx::InitializeStaticGLBindings(gfx::kGLImplementationOSMesaGL));
-  message_loop_.reset(new base::MessageLoopForUI);
 }
 
 SoftwareBrowserCompositorOutputSurfaceTest::
@@ -116,8 +115,7 @@ SoftwareBrowserCompositorOutputSurfaceTest::CreateSurface(
           device.Pass(),
           1,
           &surface_map_,
-          compositor_->GetCompositorMessageLoop(),
-          compositor_->AsWeakPtr()));
+          compositor_->vsync_manager()));
 }
 
 TEST_F(SoftwareBrowserCompositorOutputSurfaceTest, NoVSyncProvider) {

[sheu, 2014-02-05 08:47:57]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-05 08:52:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/1590001

[commit-bot: I haz the power, 2014-02-05 09:35:15]

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

[commit-bot: I haz the power, 2014-02-05 09:35:17]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[sheu, 2014-02-05 10:03:39]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-05 10:08:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/1910001

[commit-bot: I haz the power, 2014-02-05 10:45:23]

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

[commit-bot: I haz the power, 2014-02-05 10:45:25]

Retried try job too often on mac_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[sheu, 2014-02-05 23:07:12]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-05 23:11:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/2130001

[commit-bot: I haz the power, 2014-02-06 01:09:42]

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

[commit-bot: I haz the power, 2014-02-06 01:09:43]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[sheu, 2014-02-06 01:16:16]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-06 01:17:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/2130001

[commit-bot: I haz the power, 2014-02-06 02:27:39]

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

[commit-bot: I haz the power, 2014-02-06 02:27:40]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[sheu, 2014-02-06 23:13:50]

I didn't realize display_preferences_unittest was in unit_tests, not
browser_tests.  (Didn't even know that unit_tests existed!)

oshima@: I'd be much obligdd if you could diff ash/display/display_manager.cc
again and give me an OK on that.  Patch set 14 against patch set 11 should do.

[oshima, 2014-02-06 23:30:24]

On 2014/02/06 23:13:50, sheu wrote:
> I didn't realize display_preferences_unittest was in unit_tests, not
> browser_tests.  (Didn't even know that unit_tests existed!)
> 
> oshima@: I'd be much obligdd if you could diff ash/display/display_manager.cc
> again and give me an OK on that.  Patch set 14 against patch set 11 should do.

Give me 30min.

[piman, 2014-02-07 00:16:09]

lgtm

[oshima, 2014-02-07 03:07:12]

https://codereview.chromium.org/138903025/diff/2470001/ash/display/display_ma...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/138903025/diff/2470001/ash/display/display_ma...
ash/display/display_manager.cc:578: old_mode_iter->second = *display_modes_iter;
I don't understand why you need to skip existing one. Can you explain? Could  be
that native flag is lost somewhere in between?

[sheu, 2014-02-07 03:22:49]

https://codereview.chromium.org/138903025/diff/2470001/ash/display/display_ma...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/138903025/diff/2470001/ash/display/display_ma...
ash/display/display_manager.cc:578: old_mode_iter->second = *display_modes_iter;
On 2014/02/07 03:07:14, oshima wrote:
> I don't understand why you need to skip existing one. Can you explain? Could 
be
> that native flag is lost somewhere in between?

It turns out that it's nothing to do with the native flag.  |old_mode_iter| is
the search for the display mode by display ID; the behavior we want to preserve
from the original version is that we don't write an entry if there is no display
by that ID.

[oshima, 2014-02-07 15:43:25]

you probably need to update display_manager_unittest.cc as well.

https://codereview.chromium.org/138903025/diff/2490001/ash/display/display_ma...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/138903025/diff/2490001/ash/display/display_ma...
ash/display/display_manager.cc:581: }
Does the following work?

if (display_modes_iter == display_modes.end())
  display_modes_.erase(iter->id());
else if (display_modes_.find(iter->id()) != display_modes_.end())
  display_modes_[iter->id()] = *display_modes_iter;

[sheu, 2014-02-07 21:35:34]

You're right about the unittest, of course.  Updated.  oshima@: PTAL

https://chromiumcodereview.appspot.com/138903025/diff/2490001/ash/display/dis...
File ash/display/display_manager.cc (right):

https://chromiumcodereview.appspot.com/138903025/diff/2490001/ash/display/dis...
ash/display/display_manager.cc:581: }
On 2014/02/07 15:43:27, oshima wrote:
> Does the following work?
> 
> if (display_modes_iter == display_modes.end())
>   display_modes_.erase(iter->id());
> else if (display_modes_.find(iter->id()) != display_modes_.end())
>   display_modes_[iter->id()] = *display_modes_iter;
> 

That's two lookups on display_modes_, vs. one in the code above as it stands. 
Maybe a little microoptimization, but ok in this case?

I'll change it if you think it's really unclear.

[oshima, 2014-02-07 21:48:05]

On 2014/02/07 21:35:34, sheu wrote:
> You're right about the unittest, of course.  Updated.  oshima@: PTAL
> 
>
https://chromiumcodereview.appspot.com/138903025/diff/2490001/ash/display/dis...
> File ash/display/display_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/138903025/diff/2490001/ash/display/dis...
> ash/display/display_manager.cc:581: }
> On 2014/02/07 15:43:27, oshima wrote:
> > Does the following work?
> > 
> > if (display_modes_iter == display_modes.end())
> >   display_modes_.erase(iter->id());
> > else if (display_modes_.find(iter->id()) != display_modes_.end())
> >   display_modes_[iter->id()] = *display_modes_iter;
> > 
> 
> That's two lookups on display_modes_, vs. one in the code above as it stands. 
> Maybe a little microoptimization, but ok in this case?
> 
> I'll change it if you think it's really unclear.

This only happens when you change the display configuration, so readability wins
this case.

[sheu, 2014-02-10 20:50:50]

Udpated.  CQing this.

[sheu, 2014-02-10 20:51:04]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-02-10 20:51:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/3330001

[commit-bot: I haz the power, 2014-02-10 22:50:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/3330001

[commit-bot: I haz the power, 2014-02-10 23:45:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/138903025/3330001

[commit-bot: I haz the power, 2014-02-11 00:35:35]

Change committed as 250250

[sheu, 2014-02-12 21:44:22]

A revert of this CL has been created in
https://chromiumcodereview.appspot.com/161413002/ by sheu@chromium.org.

The reason for reverting is: Reverting due to Windows crashes.  See:

http://crbug.com/343199.