Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
611313003:
Use estimated vsync period on Android (Closed)
DescriptionUse estimated vsync period on Android
Use estimated vsync period to deal with buggy
getRefreshRate() implementations
BUG=418955
Committed: https://crrev.com/4a431e84243bdbdc2c965ca0ebde9ce07505cae5
Cr-Commit-Position: refs/heads/master@{#298699}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review follow-up #
Total comments: 4
Patch Set 3 : Review follow-up, added test for estimated vsync period #
Total comments: 5
Patch Set 4 : Only update refresh period during consecutive vsyncs #
Total comments: 3
Patch Set 5 : Simplified refresh period formula, indents #Patch Set 6 : Compilation fix #
Total comments: 3
Patch Set 7 : Change lastRefreshDurationWeight. Use estimation also when getRefreshRate() is <= 0. #
Total comments: 5
Patch Set 8 : Remove unused constant #
Messages
Total messages: 46 (9 generated)
jmanko@opera.com changed reviewers: + jdduke@chromium.org, miguelg@chromium.org, newt@chromium.org, tedchoc@chromium.org
Proposed fix for invalid vsync period on Android
jdduke@chromium.org changed reviewers: + sievers@chromium.org
Adding sievers@ as a more suitable reviewer for this code.
On 2014/09/30 14:57:30, jdduke wrote: > Adding sievers@ as a more suitable reviewer for this code. Adding Sami as well in case it needs some time zone friendly iterations
skyostil@chromium.org changed reviewers: + mithro@mithis.com
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
+mithro who is looking at frame time estimation. https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:92: // Display.getRefreshRate() is unreliable on some platforms. Can we make this logic conditional on whether getRefreshRate() looks bogus or not? I'd rather trust the system if it can give us a correct interval. https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:99: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; Instead of capping arbitrarily, can we just update the estimate only if we know that we saw two consecutive vsyncs? https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:55: nativeUpdateVSyncPeriod(mNativeWindowAndroid, Instead of adding a new JNI call, please merge this with nativeOnVSync.
https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:92: // Display.getRefreshRate() is unreliable on some platforms. On 2014/09/30 16:04:14, Sami wrote: > Can we make this logic conditional on whether getRefreshRate() looks bogus or > not? I'd rather trust the system if it can give us a correct interval. Acknowledged. https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:99: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; On 2014/09/30 16:04:14, Sami wrote: > Instead of capping arbitrarily, can we just update the estimate only if we know > that we saw two consecutive vsyncs? You mean something like this? long lastRefreshDurationNano = frameTimeNanos - mGoodStartingPointNano; if (lastRefreshDurationNano < 34 * NANOSECONDS_PER_MILLISECOND) mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:55: nativeUpdateVSyncPeriod(mNativeWindowAndroid, On 2014/09/30 16:04:14, Sami wrote: > Instead of adding a new JNI call, please merge this with nativeOnVSync. Acknowledged.
https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:99: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; On 2014/10/01 09:36:51, jmanko wrote: > On 2014/09/30 16:04:14, Sami wrote: > > Instead of capping arbitrarily, can we just update the estimate only if we > know > > that we saw two consecutive vsyncs? > > You mean something like this? > long lastRefreshDurationNano = frameTimeNanos - mGoodStartingPointNano; > if (lastRefreshDurationNano < 34 * NANOSECONDS_PER_MILLISECOND) > mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; That would work. Please also make the |34 * NANOSECONDS_PER_MILLISECOND| a named constant.
Thanks Sami, I've applied your suggestions
Thanks. I've added some comments. Could we have a test for this too? https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:27: private static final long PERIOD_THRESHOLD_NANOSECONDS = NANOSECONDS_PER_SECOND / 30; nit: Maybe call this something like MAX_VALID_VSYNC_PERIOD_NANOSECONDS to make it more obvious what it is for. https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:148: public boolean isVSyncSignalAvailable() { Not needed? https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:295: long vsyncPeriod); nit: vsyncPeriodMicros. https://codereview.chromium.org/611313003/diff/20001/ui/base/android/window_a... File ui/base/android/window_android.cc (right): https://codereview.chromium.org/611313003/diff/20001/ui/base/android/window_a... ui/base/android/window_android.cc:107: jlong Init(JNIEnv* env, jobject obj, jlong vsync_period) { Can we remove the parameter here too?
On 2014/10/01 11:42:50, Sami wrote: > Thanks. I've added some comments. Could we have a test for this too? > > https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... > File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): > > https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:27: private static final > long PERIOD_THRESHOLD_NANOSECONDS = NANOSECONDS_PER_SECOND / 30; > nit: Maybe call this something like MAX_VALID_VSYNC_PERIOD_NANOSECONDS to make > it more obvious what it is for. > > https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:148: public boolean > isVSyncSignalAvailable() { > Not needed? > > https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... > File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): > > https://codereview.chromium.org/611313003/diff/20001/ui/android/java/src/org/... > ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:295: long > vsyncPeriod); > nit: vsyncPeriodMicros. > > https://codereview.chromium.org/611313003/diff/20001/ui/base/android/window_a... > File ui/base/android/window_android.cc (right): > > https://codereview.chromium.org/611313003/diff/20001/ui/base/android/window_a... > ui/base/android/window_android.cc:107: jlong Init(JNIEnv* env, jobject obj, > jlong vsync_period) { > Can we remove the parameter here too? Done. I've added an additional check to existing tests, I hope it's enough, couldn't think of anything better
I'd really like to take a look at this before you land it, however I won't get to reviewing it until tomorrow (it's already almost 1am here). I'll make it a top priority tomorrow, sorry about the delay. On 2 October 2014 00:18, <jmanko@opera.com> wrote: > On 2014/10/01 11:42:50, Sami wrote: > >> Thanks. I've added some comments. Could we have a test for this too? >> > > > https://codereview.chromium.org/611313003/diff/20001/ui/ > android/java/src/org/chromium/ui/VSyncMonitor.java > >> File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): >> > > > https://codereview.chromium.org/611313003/diff/20001/ui/ > android/java/src/org/chromium/ui/VSyncMonitor.java#newcode27 > >> ui/android/java/src/org/chromium/ui/VSyncMonitor.java:27: private static >> final >> long PERIOD_THRESHOLD_NANOSECONDS = NANOSECONDS_PER_SECOND / 30; >> nit: Maybe call this something like MAX_VALID_VSYNC_PERIOD_NANOSECONDS >> to make >> it more obvious what it is for. >> > > > https://codereview.chromium.org/611313003/diff/20001/ui/ > android/java/src/org/chromium/ui/VSyncMonitor.java#newcode148 > >> ui/android/java/src/org/chromium/ui/VSyncMonitor.java:148: public boolean >> isVSyncSignalAvailable() { >> Not needed? >> > > > https://codereview.chromium.org/611313003/diff/20001/ui/ > android/java/src/org/chromium/ui/base/WindowAndroid.java > >> File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): >> > > > https://codereview.chromium.org/611313003/diff/20001/ui/ > android/java/src/org/chromium/ui/base/WindowAndroid.java#newcode295 > >> ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:295: long >> vsyncPeriod); >> nit: vsyncPeriodMicros. >> > > > https://codereview.chromium.org/611313003/diff/20001/ui/ > base/android/window_android.cc > >> File ui/base/android/window_android.cc (right): >> > > > https://codereview.chromium.org/611313003/diff/20001/ui/ > base/android/window_android.cc#newcode107 > >> ui/base/android/window_android.cc:107: jlong Init(JNIEnv* env, jobject >> obj, >> jlong vsync_period) { >> Can we remove the parameter here too? >> > > Done. I've added an additional check to existing tests, I hope it's enough, > couldn't think of anything better > > https://codereview.chromium.org/611313003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
nit: can you update the description that this estimates vsync to deal with buggy getRefreshRate() implementations? https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/1/ui/android/java/src/org/chro... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:99: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; On 2014/10/01 09:39:34, Sami wrote: > On 2014/10/01 09:36:51, jmanko wrote: > > On 2014/09/30 16:04:14, Sami wrote: > > > Instead of capping arbitrarily, can we just update the estimate only if we > > know > > > that we saw two consecutive vsyncs? > > > > You mean something like this? > > long lastRefreshDurationNano = frameTimeNanos - mGoodStartingPointNano; > > if (lastRefreshDurationNano < 34 * NANOSECONDS_PER_MILLISECOND) > > mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; > > That would work. Please also make the |34 * NANOSECONDS_PER_MILLISECOND| a named > constant. Can't this be done easier and in a more robust way because we know whether we requested a follow-up vsync tick after a given vsync tick? I feel like we should rather not use mGoodStartingPointNano if it wasn't set in the previous vsync. https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:100: long lastRefreshDurationNano = frameTimeNanos - mGoodStartingPointNano; +1 to what Sami said. Only doing the adjustments when we are somewhat certain that we are dealing with a buggy Display.getRefreshRate() would be better. https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:102: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; Do you maybe want to use a bigger weight than 0.5 for the moving average mRefreshPeriodNano?
As a temporary fix, I think this change looks okay. However, long term I think we want to forward the information unmodified to a platform independent solution as Android isn't the only platform which has these types of problems. Simon Hong and myself are working on making this happen and can explain further if you are interested in working more in this area. Tim
On 2014/10/02 07:05:40, mithro wrote: > Can't this be done easier and in a more robust way because we know whether we > requested a follow-up vsync tick after a given vsync tick? > I feel like we should rather not use mGoodStartingPointNano if it wasn't set in > the previous vsync. Right, this sounds a little cleaner. Please also remove the URL part from the BUG= line, i.e., "BUG=418955"
On 2014/10/02 09:34:56, Sami wrote: > On 2014/10/02 07:05:40, mithro wrote: > > Can't this be done easier and in a more robust way because we know whether we > > requested a follow-up vsync tick after a given vsync tick? > > I feel like we should rather not use mGoodStartingPointNano if it wasn't set > in > > the previous vsync. > > Right, this sounds a little cleaner. Could you advice a good way to check if the ticks are consecutive? I could be missing something, but I don't see this information in VSyncMonitor. CompositorImpl probably knows, but I'm not really familiar with that code. > > Please also remove the URL part from the BUG= line, i.e., "BUG=418955" Done
https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:102: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; On 2014/10/01 17:48:09, sievers wrote: > Do you maybe want to use a bigger weight than 0.5 for the moving average > mRefreshPeriodNano? Sure, I can change that. Would 0.75 be enough?
On 2014/10/02 12:02:00, jmanko wrote: > Could you advice a good way to check if the ticks are consecutive? > I could be missing something, but I don't see this information in VSyncMonitor. > CompositorImpl probably knows, but I'm not really familiar with that code. One way would be to have a counter for the consecutive vsyncs we've seen and reset that any time the monitor becomes inactive.
On 2014/10/02 12:31:29, Sami wrote: > On 2014/10/02 12:02:00, jmanko wrote: > > Could you advice a good way to check if the ticks are consecutive? > > I could be missing something, but I don't see this information in > VSyncMonitor. > > CompositorImpl probably knows, but I'm not really familiar with that code. > > One way would be to have a counter for the consecutive vsyncs we've seen and > reset that any time the monitor becomes inactive. Thanks, I've found the way to detect consecutive vsyncs
https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:92: mRefreshPeriodNano > MAX_VALID_VSYNC_PERIOD_NANOSECONDS; Indent by +4 spaces. https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:103: mRefreshPeriodNano = (long) ( nit: This would be slightly more efficient as: mRefreshPeriodNano += (long)(lastRefreshDurationWeight * (lastRefreshDurationNano - mRefreshPeriodNano)); https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:204: mConsecutiveVSync = mInsideVSync; This can have some false negatives if the next frame is requested from outside the vsync callback, but I don't think that will generally be a problem.
https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:102: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; On 2014/10/02 12:02:29, jmanko wrote: > On 2014/10/01 17:48:09, sievers wrote: > > Do you maybe want to use a bigger weight than 0.5 for the moving average > > mRefreshPeriodNano? > > Sure, I can change that. Would 0.75 be enough? If you make sure that we don't have outliers (see discussion about other comment and only measuring when we update this if mGoodStartingPointNano is really from the previous vsync) then using the average will actually get us to the best value possible over time. How about just calculating the moving average over the last N values? You only need to track the sum of the last N samples and the values of the last N (= let's say 30 or so) samples. Then curr_avg = (sum - oldest_sample + curr_sample) / N.
https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:102: mRefreshPeriodNano = (mRefreshPeriodNano + lastRefreshDurationNano) / 2; On 2014/10/02 19:51:42, sievers wrote: > On 2014/10/02 12:02:29, jmanko wrote: > > On 2014/10/01 17:48:09, sievers wrote: > > > Do you maybe want to use a bigger weight than 0.5 for the moving average > > > mRefreshPeriodNano? > > > > Sure, I can change that. Would 0.75 be enough? > > If you make sure that we don't have outliers (see discussion about other comment > and only measuring when we update this if mGoodStartingPointNano is really from > the previous vsync) then using the average will actually get us to the best > value possible over time. That's already been done, the period is only updated on consecutive vsyncs. See mConsecutiveVSync usage. > > How about just calculating the moving average over the last N values? > You only need to track the sum of the last N samples and the values of the last > N (= let's say 30 or so) samples. Then curr_avg = (sum - oldest_sample + > curr_sample) / N. > Is this still valid now, with outliers eliminated? Like you wrote above, using all the samples would eventually produce better result than last N average
On 2014/10/02 15:52:31, Sami wrote: > https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... > File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): > > https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:92: mRefreshPeriodNano > > MAX_VALID_VSYNC_PERIOD_NANOSECONDS; > Indent by +4 spaces. > > https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:103: mRefreshPeriodNano = > (long) ( > nit: This would be slightly more efficient as: > > mRefreshPeriodNano += (long)(lastRefreshDurationWeight * > (lastRefreshDurationNano - mRefreshPeriodNano)); Right, thanks. > > https://codereview.chromium.org/611313003/diff/60001/ui/android/java/src/org/... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:204: mConsecutiveVSync = > mInsideVSync; > This can have some false negatives if the next frame is requested from outside > the vsync callback, but I don't think that will generally be a problem. Yes, outside requests may still produce consecutive vsyncs, but that would be hard to check here, and like you wrote, shouldn't be a problem.
lgtm, but let's still let Daniel have one more look at this.
https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:30: private boolean mConsecutiveVSync = false; nit: Can you put a comment that this is a conservative guess? Since it relies on us requesting the next tick from inside the callback, which is currently the case (but there is maybe no good reason to rely on that - it's fine for now I'd guess we'd have to make it more complicated to detect it otherwise... such as always requesting an extra tick internally, without necessarily forwarding it to the requester, before we go idle). https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:85: if (refreshRate <= 0) refreshRate = 60; Should we also handle this case with the estimation now (with the only different that we init mRefreshPeriodNano to 1/60 here while we start with the sketchy-sounding-but-not-totally-incredible value if 0 < getRefreshRate() < 30)? https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:102: float lastRefreshDurationWeight = 0.75f; Sorry I meant to use the bigger weight for mRefreshPeriodNano so that the values end up being smoother and don't jump around. I'd maybe even use lastRefreshDurationWeight = 0.1f. Then the value after 19 samples should match the simple moving average (lastRefreshDurationWeight = 1 - (2 / (N + 1))) for a window size of N = 19.
On 2014/10/03 18:14:54, sievers wrote: > https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... > File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): > > https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:30: private boolean > mConsecutiveVSync = false; > nit: Can you put a comment that this is a conservative guess? Since it relies on > us requesting the next tick from inside the callback, which is currently the > case (but there is maybe no good reason to rely on that - it's fine for now I'd > guess we'd have to make it more complicated to detect it otherwise... such as > always requesting an extra tick internally, without necessarily forwarding it to > the requester, before we go idle). Sure, I'll add a comment. > > https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:85: if (refreshRate <= 0) > refreshRate = 60; > Should we also handle this case with the estimation now (with the only different > that we init mRefreshPeriodNano to 1/60 here while we start with the > sketchy-sounding-but-not-totally-incredible value if 0 < getRefreshRate() < 30)? Yes, estimation should be used in this case as well. I'll add that. > > https://codereview.chromium.org/611313003/diff/100001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:102: float > lastRefreshDurationWeight = 0.75f; > Sorry I meant to use the bigger weight for mRefreshPeriodNano so that the values > end up being smoother and don't jump around. I'd maybe even use > lastRefreshDurationWeight = 0.1f. Then the value after 19 samples should match > the simple moving average (lastRefreshDurationWeight = 1 - (2 / (N + 1))) for a > window size of N = 19. OK. I misunderstood before. Weight 0.1 makes sense.
lgtm thanks!
The CQ bit was checked by jmanko@opera.com
The CQ bit was unchecked by jmanko@opera.com
The CQ bit was checked by jmanko@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/611313003/120001
On 2014/10/06 18:24:33, sievers wrote: > lgtm thanks! Great, thanks for reviewing! How do I land it? There's a "commit" checkbox, but it doesn't seem to do anything
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/10/07 08:42:32, jmanko wrote: > On 2014/10/06 18:24:33, sievers wrote: > > lgtm thanks! > > Great, thanks for reviewing! > How do I land it? There's a "commit" checkbox, but it doesn't seem to do > anything When you check the commit checkbox, the commit queue worker will come along and try and commit it a couple of minutes later. Your patch however failed to land because it looks like you are missing an OWNERS from ui/android/ and ui/base/. You can use "git cl owners" command to try and find reviewers in these areas. Hope that helps! Tim
On 2014/10/07 08:56:21, mithro wrote: > On 2014/10/07 08:42:32, jmanko wrote: > > On 2014/10/06 18:24:33, sievers wrote: > > > lgtm thanks! > > > > Great, thanks for reviewing! > > How do I land it? There's a "commit" checkbox, but it doesn't seem to do > > anything > > When you check the commit checkbox, the commit queue worker will come along and > try and commit it a couple of minutes later. > > Your patch however failed to land because it looks like you are missing an > OWNERS from ui/android/ and ui/base/. You can use "git cl owners" command to > try and find reviewers in these areas. > > Hope that helps! > > Tim That would be jdduke, I've actually added him before. @jdduke: could you please take a look?
Sorry for the delayed review. https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:27: private static final long MAX_VALID_VSYNC_PERIOD_NANOSECONDS = NANOSECONDS_PER_SECOND / 30; I don't think this is used any longer? https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:88: final boolean useEstimatedRefreshPeriod = refreshRate < 30; I see 30 both here and above in the MAX_VALID_VSYNC_PERIOD_NANOSECONDS calculation. If they're at all coupled, could we pull out the 30 constant to somewhere above? Or actually, it looks like the constant at the top is unused? https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:106: mRefreshPeriodNano += (long) (lastRefreshDurationWeight * Hmm, I could be wrong, but don't you need to reweight this like: mRefreshPeriodNano = (1 - lastRefreshDurationWieght) * mRefreshPeriodNano + lastRefreshDurationWeight * (lastRefreshDurationNano - mRefreshPeriodNano)? Otherwise this will just grow without bound? What am I missing?
On 2014/10/08 03:17:03, jdduke wrote: > Sorry for the delayed review. > > https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... > File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): > > https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:27: private static final > long MAX_VALID_VSYNC_PERIOD_NANOSECONDS = NANOSECONDS_PER_SECOND / 30; > I don't think this is used any longer? Right, it was used before, I forgot to remove it > > https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:88: final boolean > useEstimatedRefreshPeriod = refreshRate < 30; > I see 30 both here and above in the MAX_VALID_VSYNC_PERIOD_NANOSECONDS > calculation. If they're at all coupled, could we pull out the 30 constant to > somewhere above? Or actually, it looks like the constant at the top is unused? I'll remove the unused constant, so 30 will only remain here. > > https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/VSyncMonitor.java:106: mRefreshPeriodNano += > (long) (lastRefreshDurationWeight * > Hmm, I could be wrong, but don't you need to reweight this like: > > mRefreshPeriodNano = (1 - lastRefreshDurationWieght) * mRefreshPeriodNano + > lastRefreshDurationWeight * (lastRefreshDurationNano - mRefreshPeriodNano)? > > Otherwise this will just grow without bound? What am I missing? The formula is actually correct. Let's assume a new vsync arrives, with the same duration as current value of mRefreshPeriodNano. With your formula, updated mRefreshPeriodNano would be: (1 - 0.1) * mRefreshPeriodNano + 0.1 * (mRefreshPeriodNano - mRefreshPeriodNano) = 0.9 * mRefreshPeriodNano while it should be unchanged. The formula should be without "- mRefreshPeriodNano" at the end, and that's what I used in Patch set 4. Then it's been simplified with += as suggested by Sami.
https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:106: mRefreshPeriodNano += (long) (lastRefreshDurationWeight * On 2014/10/08 03:17:02, jdduke wrote: > Hmm, I could be wrong, but don't you need to reweight this like: > > mRefreshPeriodNano = (1 - lastRefreshDurationWieght) * mRefreshPeriodNano + > lastRefreshDurationWeight * (lastRefreshDurationNano - mRefreshPeriodNano)? > > Otherwise this will just grow without bound? What am I missing? Hmm, I think that reduces to mRefreshPeriodNano = lastRefreshDurationNano / 2 which doesn't look right :) The current code is reducing the error between the new period and the estimate. The bigger lastRefreshDurationWeight is, the faster the convergence.
lgtm, thanks. https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/611313003/diff/120001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:106: mRefreshPeriodNano += (long) (lastRefreshDurationWeight * On 2014/10/08 10:14:51, Sami wrote: > On 2014/10/08 03:17:02, jdduke wrote: > > Hmm, I could be wrong, but don't you need to reweight this like: > > > > mRefreshPeriodNano = (1 - lastRefreshDurationWieght) * mRefreshPeriodNano + > > lastRefreshDurationWeight * (lastRefreshDurationNano - mRefreshPeriodNano)? > > > > Otherwise this will just grow without bound? What am I missing? > > Hmm, I think that reduces to mRefreshPeriodNano = lastRefreshDurationNano / 2 > which doesn't look right :) The current code is reducing the error between the > new period and the estimate. The bigger lastRefreshDurationWeight is, the faster > the convergence. Of course, sigh, I should actually read the variable names =/ I glossed over the subtraction term and thought it was computing the most recent period, rather than the difference between the most recent period and the smoothed period.
The CQ bit was checked by jmanko@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/611313003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as f62704ac5b986ad8311c9bdeebf79fa5b4d63394
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4a431e84243bdbdc2c965ca0ebde9ce07505cae5 Cr-Commit-Position: refs/heads/master@{#298699} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
