Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271393003:
Delay setting the compositor view background to null on N (Closed)
DescriptionDelay setting the compositor view background to null on N
We set compositor view background to white while there are no
frames and onSwapBufferCompleted swap it back to null. On N
this creates a black flash on some cold starts where there is
still a frame that is drawn before the frame is actually completed.
BUG=640758
Committed: https://crrev.com/98da165e64a9f09e65962b9b0cf309906be58be4
Cr-Commit-Position: refs/heads/master@{#414292}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use vsync period as well #
Total comments: 4
Patch Set 3 : Nits #
Total comments: 2
Patch Set 4 : Do all calc if > M #
Total comments: 1
Messages
Total messages: 25 (9 generated)
yusufo@chromium.org changed reviewers: + sievers@chromium.org
https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:100: / NANOSECONDS_PER_MILLISECOND; nit: should this round up? https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:301: ? mPresentationDeadlineMillis : 0; maybe we should make it at least a frame to account for the draw commands having taken too long (they *shouldn't* take longer than a frame though). You could get VSyncMonitor.getVSyncPeriodInMicroseconds() through WindowAndroid. And maybe we should also consider broken values returned from getPresentationDeadlineNanos(), like -1 or what not. We could cap it at some high value, like 3 frames maybe.
https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:301: ? mPresentationDeadlineMillis : 0; On 2016/08/24 21:45:10, sievers wrote: > maybe we should make it at least a frame to account for the draw commands having > taken too long (they *shouldn't* take longer than a frame though). > > You could get VSyncMonitor.getVSyncPeriodInMicroseconds() through WindowAndroid. > > And maybe we should also consider broken values returned from > getPresentationDeadlineNanos(), like -1 or what not. We could cap it at some > high value, like 3 frames maybe. so slightly more straightforward and also conservative would be simply rounding up the presentation deadline to the next frame interval. and then capping that to 3 or 4 frames for the case that the api returns some ridiculous value.
On 2016/08/24 22:27:59, sievers wrote: > https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java > (right): > > https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:301: > ? mPresentationDeadlineMillis : 0; > On 2016/08/24 21:45:10, sievers wrote: > > maybe we should make it at least a frame to account for the draw commands > having > > taken too long (they *shouldn't* take longer than a frame though). > > > > You could get VSyncMonitor.getVSyncPeriodInMicroseconds() through > WindowAndroid. > > > > And maybe we should also consider broken values returned from > > getPresentationDeadlineNanos(), like -1 or what not. We could cap it at some > > high value, like 3 frames maybe. > > so slightly more straightforward and also conservative would be simply rounding > up the presentation deadline to the next frame interval. > and then capping that to 3 or 4 frames for the case that the api returns some > ridiculous value.
On 2016/08/25 00:41:44, Yusuf wrote: > On 2016/08/24 22:27:59, sievers wrote: > > > https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java > > (right): > > > > > https://codereview.chromium.org/2271393003/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:301: > > ? mPresentationDeadlineMillis : 0; > > On 2016/08/24 21:45:10, sievers wrote: > > > maybe we should make it at least a frame to account for the draw commands > > having > > > taken too long (they *shouldn't* take longer than a frame though). > > > > > > You could get VSyncMonitor.getVSyncPeriodInMicroseconds() through > > WindowAndroid. > > > > > > And maybe we should also consider broken values returned from > > > getPresentationDeadlineNanos(), like -1 or what not. We could cap it at some > > > high value, like 3 frames maybe. > > > > so slightly more straightforward and also conservative would be simply > rounding > > up the presentation deadline to the next frame interval. > > and then capping that to 3 or 4 frames for the case that the api returns some > > ridiculous value. Done!
https://codereview.chromium.org/2271393003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2271393003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:66: private long mDelayForBackground; nit: how about 'mFramePresentationDelay' w/comment 'a conservative estimate of when a frame is guaranteed to be presented after being submitted' ? https://codereview.chromium.org/2271393003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:200: (long) Math.ceil(presentationDeadline / vsyncPeriod) * vsyncPeriod); need to cast presentationDeadline to double or it will still do an integer divide. or how about: ((presentationDeadline + vsyncPeriod - 1) / vsyncPeriod) * vsyncPeriod
https://codereview.chromium.org/2271393003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2271393003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:66: private long mDelayForBackground; On 2016/08/25 00:51:16, sievers wrote: > nit: how about 'mFramePresentationDelay' w/comment 'a conservative estimate of > when a frame is guaranteed to be presented after being submitted' ? Done. https://codereview.chromium.org/2271393003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:200: (long) Math.ceil(presentationDeadline / vsyncPeriod) * vsyncPeriod); On 2016/08/25 00:51:16, sievers wrote: > need to cast presentationDeadline to double or it will still do an integer > divide. or how about: > ((presentationDeadline + vsyncPeriod - 1) / vsyncPeriod) * vsyncPeriod Done.
The CQ bit was checked by yusufo@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...
lgtm but see comment for api level https://codereview.chromium.org/2271393003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2271393003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:198: long presentationDeadline = display.getPresentationDeadlineNanos() sorry, i missed that this api is L+. how about just moving the check for >N from below to up here and leaving the delay as 0 otherwise (and never do any of this new code)?
https://codereview.chromium.org/2271393003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2271393003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:198: long presentationDeadline = display.getPresentationDeadlineNanos() On 2016/08/25 00:59:30, sievers wrote: > sorry, i missed that this api is L+. how about just moving the check for >N from > below to up here and leaving the delay as 0 otherwise (and never do any of this > new code)? Done.
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2271393003/#ps60001 (title: "Do all calc if > M")
yusufo@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@ for ui/android/
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm - ui/android https://codereview.chromium.org/2271393003/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2271393003/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:160: public long getVsyncPeriodInMillis() { it's odd these two methods are above the constructor but i guess it is consistent with isInsideVsync. We should move them down at some point
The CQ bit was checked by yusufo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Delay setting the compositor view background to null on N We set compositor view background to white while there are no frames and onSwapBufferCompleted swap it back to null. On N this creates a black flash on some cold starts where there is still a frame that is drawn before the frame is actually completed. BUG=640758 ========== to ========== Delay setting the compositor view background to null on N We set compositor view background to white while there are no frames and onSwapBufferCompleted swap it back to null. On N this creates a black flash on some cold starts where there is still a frame that is drawn before the frame is actually completed. BUG=640758 Committed: https://crrev.com/98da165e64a9f09e65962b9b0cf309906be58be4 Cr-Commit-Position: refs/heads/master@{#414292} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/98da165e64a9f09e65962b9b0cf309906be58be4 Cr-Commit-Position: refs/heads/master@{#414292} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
