Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1789803003:
Remove synthetic vsync from Android VsyncMonitor (Closed)
|
Created:
4 years, 9 months ago by enne (OOO) Modified:
4 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove synthetic vsync from Android VsyncMonitor
This logic is from the ancient past and apparently is not needed anymore.
This vsync can cause BeginFrames with frame times in the past to occur.
As a part of testing https://codereview.chromium.org/1765723002, this
caused AwContentsGarbageCollectionTest#testCreateAndGcManyTimes to crash
because of this. It would enter a situation where a synthetic vsync
would be sent that tried to predict the last vsync time, but the next
time the choreographer sent a frame time it would be *before* the
synthetic frame time. This causes assertions later on in the pipeline
where it's assumed that all begin frames are monotonically increasing in
frame time.
These invalid begin frames used to be dropped silently by the begin
frame source multiplexer, but that logic is removed in the above patch.
R=skyostil@chromium.org,sievers@chromium.org
Committed: https://crrev.com/4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7
Cr-Commit-Position: refs/heads/master@{#381105}
Committed: https://crrev.com/822b149f097853222eddabf4ae5f6c61f25523a2
Cr-Commit-Position: refs/heads/master@{#381260}
Patch Set 1 #Patch Set 2 : Remove synthetic vsync test #Patch Set 3 : Remove unused variable #
Messages
Total messages: 29 (10 generated)
Description was changed from ========== Remove synthetic vsync from Android VsyncMonitor This logic is from the ancient past and apparently is not needed anymore. This vsync can cause BeginFrames with frame times in the past to occur. As a part of testing https://codereview.chromium.org/1765723002, this caused AwContentsGarbageCollectionTest#testCreateAndGcManyTimes to crash because of this. It would enter a situation where a synthetic vsync would be sent that tried to predict the last vsync time, but the next time the choreographer sent a frame time it would be *before* the synthetic frame time. This causes assertions later on in the pipeline where it's assumed that all begin frames are monotonically increasing in frame time. R=skyostil@chromium.org,sievers@chromium.org ========== to ========== Remove synthetic vsync from Android VsyncMonitor This logic is from the ancient past and apparently is not needed anymore. This vsync can cause BeginFrames with frame times in the past to occur. As a part of testing https://codereview.chromium.org/1765723002, this caused AwContentsGarbageCollectionTest#testCreateAndGcManyTimes to crash because of this. It would enter a situation where a synthetic vsync would be sent that tried to predict the last vsync time, but the next time the choreographer sent a frame time it would be *before* the synthetic frame time. This causes assertions later on in the pipeline where it's assumed that all begin frames are monotonically increasing in frame time. These invalid begin frames used to be dropped silently by the begin frame source multiplexer, but that logic is removed in the above patch. R=skyostil@chromium.org,sievers@chromium.org ==========
lgtm. If we find out we want to still do this 'retroactive vsync' we can reinvent it at a better level (scheduler, for example). Also, FWIU the proactive vsync in RWHVA::OnTouch() should still trigger a real callback (since input event delivery happens right before Choreographer's frame callback).
You might have to remove testVSyncActivationFromIdle in content/public/android/javatests/src/org/chromium/content/browser/VSyncMonitorTest.java
That test still appears to pass for me, which seems like a good sign that this logic is still working as expected without the extra synthetic begin frmae.
On 2016/03/12 00:06:52, enne wrote:
> That test still appears to pass for me, which seems like a good sign that this
> logic is still working as expected without the extra synthetic begin frmae.
Initially I though this test would at least get flaky:
long delay = SystemClock.uptimeMillis() -
collector.mLastVSyncCpuTimeMillis;
// The VSync should have activated immediately instead of at the next
real vsync.
assertTrue(delay < period);
But now I'm wondering if it tests anything at all other than measuring some
arbitrary delay between the last vsync tick having happened (rather than the
time between when it was requested and vsync was signaled).
lgtm to remove. On 2016/03/12 00:45:43, sievers wrote: > On 2016/03/12 00:06:52, enne wrote: > > That test still appears to pass for me, which seems like a good sign that this > > logic is still working as expected without the extra synthetic begin frmae. > > Initially I though this test would at least get flaky: > > long delay = SystemClock.uptimeMillis() - > collector.mLastVSyncCpuTimeMillis; > > // The VSync should have activated immediately instead of at the next > real vsync. > assertTrue(delay < period); > > But now I'm wondering if it tests anything at all other than measuring some > arbitrary delay between the last vsync tick having happened (rather than the > time between when it was requested and vsync was signaled). Yeah that test looks wrong. We should be checking that the time from *requesting* the vsync to it being serviced is less than the period. That won't be strictly the case any more with this patch. For the case that really matters (touch scroll from idle) we'll still be able to hit the same frame as the input thanks to this code proactively requesting the BeginFrame: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
Thanks. I removed that test as well based on y'all's comments.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1789803003/#ps20001 (title: "Remove synthetic vsync test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1789803003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1789803003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
enne@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for OWNERS
On 2016/03/14 22:43:58, enne wrote: > +tedchoc for OWNERS rs lgtm since this is sievers and sami's area
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1789803003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1789803003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove synthetic vsync from Android VsyncMonitor This logic is from the ancient past and apparently is not needed anymore. This vsync can cause BeginFrames with frame times in the past to occur. As a part of testing https://codereview.chromium.org/1765723002, this caused AwContentsGarbageCollectionTest#testCreateAndGcManyTimes to crash because of this. It would enter a situation where a synthetic vsync would be sent that tried to predict the last vsync time, but the next time the choreographer sent a frame time it would be *before* the synthetic frame time. This causes assertions later on in the pipeline where it's assumed that all begin frames are monotonically increasing in frame time. These invalid begin frames used to be dropped silently by the begin frame source multiplexer, but that logic is removed in the above patch. R=skyostil@chromium.org,sievers@chromium.org ========== to ========== Remove synthetic vsync from Android VsyncMonitor This logic is from the ancient past and apparently is not needed anymore. This vsync can cause BeginFrames with frame times in the past to occur. As a part of testing https://codereview.chromium.org/1765723002, this caused AwContentsGarbageCollectionTest#testCreateAndGcManyTimes to crash because of this. It would enter a situation where a synthetic vsync would be sent that tried to predict the last vsync time, but the next time the choreographer sent a frame time it would be *before* the synthetic frame time. This causes assertions later on in the pipeline where it's assumed that all begin frames are monotonically increasing in frame time. These invalid begin frames used to be dropped silently by the begin frame source multiplexer, but that logic is removed in the above patch. R=skyostil@chromium.org,sievers@chromium.org Committed: https://crrev.com/4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7 Cr-Commit-Position: refs/heads/master@{#381105} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7 Cr-Commit-Position: refs/heads/master@{#381105}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1803793004/ by pkotwicz@chromium.org. The reason for reverting is: This CL broke the compile on downstream bots. See https://uberchromegw.corp.google.com/i/internal.client.clank_tot/builders/cla... The relevant part of the compile log: FindBugs run via: java -classpath /b/build/slave/clang-clankium-tot-builder/build/src/third_party/findbugs/lib/findbugs.jar: -Xmx768m -Dfindbugs.home="/b/build/slave/clang-clankium-tot-builder/build/src/third_party/findbugs" -jar /b/build/slave/clang-clankium-tot-builder/build/src/third_party/findbugs/lib/findbugs.jar -textui -sortByClass -pluginList /b/build/slave/clang-clankium-tot-builder/build/src/tools/android/findbugs_plugin/lib/chromiumPlugin.jar -xml:withMessages -auxclasspath /b/build/slave/clang-clankium-tot-builder/build/src/third_party/android_tools/sdk/platforms/android-23/android.jar:/b/build/slave/clang-clankium-tot-builder/build/src/third_party/android_tools/sdk/extras/android/support/multidex/library/libs/android-support-multidex.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/jsr_305_javalib.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/base_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/reporter_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/base_java_test_support.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/mojo_public_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/mojo_bindings_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/device_battery_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/device_bluetooth_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/device_usb_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/device_vibration_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/media_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/mojo_system_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/net_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/ui_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/third_party/android_tools/sdk/extras/android/support/v13/android-support-v13.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/blink_headers_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/capture_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/ui_accessibility_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/midi_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/protobuf_nano_javalib.jar:/b/build/slave/clang-clankium-tot-builder/build/src/third_party/cardboard-java/src/CardboardSample/libs/cardboard.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/content_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/content_java_test_support.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/content_shell_java.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/base_java_unittest_support.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/chromium_apk_content_shell_apk.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/base_javatests.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/device_battery_javatests.jar:/b/build/slave/clang-clankium-tot-builder/build/src/third_party/android_tools/sdk/platforms/android-23/optional/org.apache.http.legacy.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/net_java_test_support.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/net_javatests.jar:/b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/broker_java.jar -exclude /b/build/slave/clang-clankium-tot-builder/build/src/build/android/findbugs_filter/findbugs_exclude.xml /b/build/slave/clang-clankium-tot-builder/build/src/out/Debug/lib.java/chromium_apk_content_shell_test_apk.jar FindBugs reported the following issues: URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD: Unread public/protected field In class org.chromium.content.browser.VSyncMonitorTest$VSyncDataCollector Field org.chromium.content.browser.VSyncMonitorTest$VSyncDataCollector.mLastVSyncCpuTimeMillis At VSyncMonitorTest.java:[line 47] Usually I would just land a fix because the fix is trivial. However, I am away from my desktop.
pkotwicz: How about running FindBugs on upstream bots? Downstream bots /o\ Uploaded a new patch removing that variable and a now-unneeded include.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, sievers@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1789803003/#ps40001 (title: "Remove unused variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1789803003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1789803003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove synthetic vsync from Android VsyncMonitor This logic is from the ancient past and apparently is not needed anymore. This vsync can cause BeginFrames with frame times in the past to occur. As a part of testing https://codereview.chromium.org/1765723002, this caused AwContentsGarbageCollectionTest#testCreateAndGcManyTimes to crash because of this. It would enter a situation where a synthetic vsync would be sent that tried to predict the last vsync time, but the next time the choreographer sent a frame time it would be *before* the synthetic frame time. This causes assertions later on in the pipeline where it's assumed that all begin frames are monotonically increasing in frame time. These invalid begin frames used to be dropped silently by the begin frame source multiplexer, but that logic is removed in the above patch. R=skyostil@chromium.org,sievers@chromium.org Committed: https://crrev.com/4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7 Cr-Commit-Position: refs/heads/master@{#381105} ========== to ========== Remove synthetic vsync from Android VsyncMonitor This logic is from the ancient past and apparently is not needed anymore. This vsync can cause BeginFrames with frame times in the past to occur. As a part of testing https://codereview.chromium.org/1765723002, this caused AwContentsGarbageCollectionTest#testCreateAndGcManyTimes to crash because of this. It would enter a situation where a synthetic vsync would be sent that tried to predict the last vsync time, but the next time the choreographer sent a frame time it would be *before* the synthetic frame time. This causes assertions later on in the pipeline where it's assumed that all begin frames are monotonically increasing in frame time. These invalid begin frames used to be dropped silently by the begin frame source multiplexer, but that logic is removed in the above patch. R=skyostil@chromium.org,sievers@chromium.org Committed: https://crrev.com/4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7 Cr-Commit-Position: refs/heads/master@{#381105} Committed: https://crrev.com/822b149f097853222eddabf4ae5f6c61f25523a2 Cr-Commit-Position: refs/heads/master@{#381260} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/822b149f097853222eddabf4ae5f6c61f25523a2 Cr-Commit-Position: refs/heads/master@{#381260} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
