|
|
DescriptionWebVR unstuffing: delay "rendered" notification
In order to prevent queue stuffing from overeager frame creation, ensure
that we never have more than one frame being rendered while the next
frame is being prepared in Javascript. The rendering time should include
GVR overhead to avoid overcommitting rendering resources, so this change
moves the "rendering done, ok to submit the next frame" notification
past GVR submit.
BUG=723962
Review-Url: https://codereview.chromium.org/2897583002
Cr-Commit-Position: refs/heads/master@{#474147}
Committed: https://chromium.googlesource.com/chromium/src/+/4493e8ab56406db9d3eb2a640662e0e95b383b4a
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #
Total comments: 1
Patch Set 4 : Rebase #Patch Set 5 : Revert pending_time_ change #Patch Set 6 : Rebase, "depends on" is submitted #Patch Set 7 : Rebase #Patch Set 8 : Rebase and squash #Patch Set 9 : Rebase #Messages
Total messages: 21 (12 generated)
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== WebVR unstuffing: delay "rendered" notification In order to prevent queue stuffing from overeager frame creation, ensure that we never have more than one frame being rendered while the next frame is being prepared in Javascript. The rendering time should include GVR overhead to avoid overcommitting rendering resources, so this change moves the "rendering done, ok to submit the next frame" notification past GVR submit. To maximize parallelism, move the "wait for previous render" sync point as late as possible in the frame creation code, so that it happens immediately before trying to submit. Since it's now expected that the rAF timing will drift away from VSync, ensure that a late pending vsync is generated with a correct current time instead of a saved time from the most recent VSync. BUG=723962 ========== to ========== WebVR unstuffing: delay "rendered" notification In order to prevent queue stuffing from overeager frame creation, ensure that we never have more than one frame being rendered while the next frame is being prepared in Javascript. The rendering time should include GVR overhead to avoid overcommitting rendering resources, so this change moves the "rendering done, ok to submit the next frame" notification past GVR submit. Since it's now expected that the rAF timing will drift away from VSync, ensure that a late pending vsync is generated with a correct current time instead of a saved time from the most recent VSync. BUG=723962 ==========
klausw@chromium.org changed reviewers: + bajones@chromium.org
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1491: base::TimeDelta time = base::TimeTicks::Now() - vsync_timebase_; I don't think we want to do this with the timestamps. The purpose of storing the timestamp is to ensure we get a consistent interval in the timestamps so that animations feel smooth - even if they're slightly behind what the current timestamp is. What you could do is check if the pending timestamp is more than a frame old, and up it by one vsync interval though.
On 2017/05/23 00:14:17, mthiesse wrote: > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell_gl.cc:1491: base::TimeDelta time = > base::TimeTicks::Now() - vsync_timebase_; > I don't think we want to do this with the timestamps. The purpose of storing the > timestamp is to ensure we get a consistent interval in the timestamps so that > animations feel smooth - even if they're slightly behind what the current > timestamp is. > > What you could do is check if the pending timestamp is more than a frame old, > and up it by one vsync interval though. Is there a reasonable way we could measure this? Arguably both of them are wrong in different ways. The more correct approach would be to try to use an approximation of the time at which the frame will end up being displayed, but we don't know this unless we use a variation of the "GVR oracle" which could try to predict that. My gut feeling is that I think it's more accurate to use the best approximation of the actual time the animation starts instead of clamping it to vsync intervals, but I'm willing to be convinced otherwise. The main timing variation is due to when the JS-submitted frame ends up being submitted to GVR and if this is before or after the halfway-between-vsyncs latch point, and if there's more queued buffers at that time.
On 2017/05/23 00:47:05, klausw wrote: > On 2017/05/23 00:14:17, mthiesse wrote: > > > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > > > > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > > chrome/browser/android/vr_shell/vr_shell_gl.cc:1491: base::TimeDelta time = > > base::TimeTicks::Now() - vsync_timebase_; > > I don't think we want to do this with the timestamps. The purpose of storing > the > > timestamp is to ensure we get a consistent interval in the timestamps so that > > animations feel smooth - even if they're slightly behind what the current > > timestamp is. > > > > What you could do is check if the pending timestamp is more than a frame old, > > and up it by one vsync interval though. > > Is there a reasonable way we could measure this? Arguably both of them are wrong > in different ways. The more correct approach would be to try to use an > approximation of the time at which the frame will end up being displayed, but we > don't know this unless we use a variation of the "GVR oracle" which could try to > predict that. My gut feeling is that I think it's more accurate to use the best > approximation of the actual time the animation starts instead of clamping it to > vsync intervals, but I'm willing to be convinced otherwise. > > The main timing variation is due to when the JS-submitted frame ends up being > submitted to GVR and if this is before or after the halfway-between-vsyncs latch > point, and if there's more queued buffers at that time. FWIW, based on brief single-blind testing just now (thanks Brian!), the original code and this change seem fairly equivalent. I do get a significant improvement in smoothness by forcing animation to start at vsync (basically treating pending_vsync_ as always false), dropping framerate to 30/20/15fps as needed, but that would be a crude approach without properly adjusting the animation start time, and it would need developer outreach to get people to target 30fps instead of "as fast as possible" if not reaching 60fps. I'll try the whole-vsync variation you suggested separately.
Description was changed from ========== WebVR unstuffing: delay "rendered" notification In order to prevent queue stuffing from overeager frame creation, ensure that we never have more than one frame being rendered while the next frame is being prepared in Javascript. The rendering time should include GVR overhead to avoid overcommitting rendering resources, so this change moves the "rendering done, ok to submit the next frame" notification past GVR submit. Since it's now expected that the rAF timing will drift away from VSync, ensure that a late pending vsync is generated with a correct current time instead of a saved time from the most recent VSync. BUG=723962 ========== to ========== WebVR unstuffing: delay "rendered" notification In order to prevent queue stuffing from overeager frame creation, ensure that we never have more than one frame being rendered while the next frame is being prepared in Javascript. The rendering time should include GVR overhead to avoid overcommitting rendering resources, so this change moves the "rendering done, ok to submit the next frame" notification past GVR submit. BUG=723962 ==========
On 2017/05/23 01:04:19, klausw wrote: > On 2017/05/23 00:47:05, klausw wrote: > > On 2017/05/23 00:14:17, mthiesse wrote: > > > > > > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > > > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > > > > > > > > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > > > chrome/browser/android/vr_shell/vr_shell_gl.cc:1491: base::TimeDelta time = > > > base::TimeTicks::Now() - vsync_timebase_; > > > I don't think we want to do this with the timestamps. The purpose of storing > > the > > > timestamp is to ensure we get a consistent interval in the timestamps so > that > > > animations feel smooth - even if they're slightly behind what the current > > > timestamp is. > > > > > > What you could do is check if the pending timestamp is more than a frame > old, > > > and up it by one vsync interval though. > > > > Is there a reasonable way we could measure this? Arguably both of them are > wrong > > in different ways. The more correct approach would be to try to use an > > approximation of the time at which the frame will end up being displayed, but > we > > don't know this unless we use a variation of the "GVR oracle" which could try > to > > predict that. My gut feeling is that I think it's more accurate to use the > best > > approximation of the actual time the animation starts instead of clamping it > to > > vsync intervals, but I'm willing to be convinced otherwise. > > > > The main timing variation is due to when the JS-submitted frame ends up being > > submitted to GVR and if this is before or after the halfway-between-vsyncs > latch > > point, and if there's more queued buffers at that time. > > FWIW, based on brief single-blind testing just now (thanks Brian!), the original > code > and this change seem fairly equivalent. I do get a significant improvement in > smoothness by forcing animation to start at vsync (basically treating > pending_vsync_ > as always false), dropping framerate to 30/20/15fps as needed, but that would be > a > crude approach without properly adjusting the animation start time, and it would > need developer outreach to get people to target 30fps instead of "as fast as > possible" > if not reaching 60fps. > > I'll try the whole-vsync variation you suggested separately. PTAL. I've reverted the pending_time_ change, it's now back to the original behavior. As discussed in today's meeting, I'll separately do some tests with running animations at integer multiples of the vsync interval only. I added the "move time forward to next vsync" change you suggested, but I never saw a whole vsync of time difference in testing, the new scheduling logic from the depends-on patch + this one helps prevent that. So I think it's not really useful to add a special case for this.
On 2017/05/23 19:04:51, klausw wrote: > On 2017/05/23 01:04:19, klausw wrote: > > On 2017/05/23 00:47:05, klausw wrote: > > > On 2017/05/23 00:14:17, mthiesse wrote: > > > > > > > > > > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > > > > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2897583002/diff/40001/chrome/browser/android/... > > > > chrome/browser/android/vr_shell/vr_shell_gl.cc:1491: base::TimeDelta time > = > > > > base::TimeTicks::Now() - vsync_timebase_; > > > > I don't think we want to do this with the timestamps. The purpose of > storing > > > the > > > > timestamp is to ensure we get a consistent interval in the timestamps so > > that > > > > animations feel smooth - even if they're slightly behind what the current > > > > timestamp is. > > > > > > > > What you could do is check if the pending timestamp is more than a frame > > old, > > > > and up it by one vsync interval though. > > > > > > Is there a reasonable way we could measure this? Arguably both of them are > > wrong > > > in different ways. The more correct approach would be to try to use an > > > approximation of the time at which the frame will end up being displayed, > but > > we > > > don't know this unless we use a variation of the "GVR oracle" which could > try > > to > > > predict that. My gut feeling is that I think it's more accurate to use the > > best > > > approximation of the actual time the animation starts instead of clamping it > > to > > > vsync intervals, but I'm willing to be convinced otherwise. > > > > > > The main timing variation is due to when the JS-submitted frame ends up > being > > > submitted to GVR and if this is before or after the halfway-between-vsyncs > > latch > > > point, and if there's more queued buffers at that time. > > > > FWIW, based on brief single-blind testing just now (thanks Brian!), the > original > > code > > and this change seem fairly equivalent. I do get a significant improvement in > > smoothness by forcing animation to start at vsync (basically treating > > pending_vsync_ > > as always false), dropping framerate to 30/20/15fps as needed, but that would > be > > a > > crude approach without properly adjusting the animation start time, and it > would > > need developer outreach to get people to target 30fps instead of "as fast as > > possible" > > if not reaching 60fps. > > > > I'll try the whole-vsync variation you suggested separately. > > PTAL. I've reverted the pending_time_ change, it's now back to the original > behavior. As discussed in today's meeting, I'll separately do some tests > with running animations at integer multiples of the vsync interval only. > > I added the "move time forward to next vsync" change you suggested, but I > never saw a whole vsync of time difference in testing, the new scheduling > logic from the depends-on patch + this one helps prevent that. So I think > it's not really useful to add a special case for this. To clarify, I had implemented "move time forward to next vsync" logic for testing purposes, but I reverted that change when logging showed that it never actually had to adjust the timing in the test-slow-render.html sample.
lgtm
The CQ bit was checked by klausw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2897583002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495586107601740, "parent_rev": "ef53b0adb151b2eb77e4c5dcda0d9cea47e97a6d", "commit_rev": "4493e8ab56406db9d3eb2a640662e0e95b383b4a"}
Message was sent while issue was closed.
Description was changed from ========== WebVR unstuffing: delay "rendered" notification In order to prevent queue stuffing from overeager frame creation, ensure that we never have more than one frame being rendered while the next frame is being prepared in Javascript. The rendering time should include GVR overhead to avoid overcommitting rendering resources, so this change moves the "rendering done, ok to submit the next frame" notification past GVR submit. BUG=723962 ========== to ========== WebVR unstuffing: delay "rendered" notification In order to prevent queue stuffing from overeager frame creation, ensure that we never have more than one frame being rendered while the next frame is being prepared in Javascript. The rendering time should include GVR overhead to avoid overcommitting rendering resources, so this change moves the "rendering done, ok to submit the next frame" notification past GVR submit. BUG=723962 Review-Url: https://codereview.chromium.org/2897583002 Cr-Commit-Position: refs/heads/master@{#474147} Committed: https://chromium.googlesource.com/chromium/src/+/4493e8ab56406db9d3eb2a640662... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/4493e8ab56406db9d3eb2a640662... |