diff options
author | enne <enne@chromium.org> | 2016-03-14 15:58:35 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2016-03-14 23:00:13 +0000 |
commit | 4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7 (patch) | |
tree | 556327d4ec2c3d82ef852a33c7bb100fa7e58997 | |
parent | 56047cef021402a70701ca7c81897db542be731d (diff) | |
download | chromium_src-4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7.zip chromium_src-4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7.tar.gz chromium_src-4a7c4f1f0ea5708d7a166a103d85d162ef3ef4c7.tar.bz2 |
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
Review URL: https://codereview.chromium.org/1789803003
Cr-Commit-Position: refs/heads/master@{#381105}
-rw-r--r-- | content/public/android/javatests/src/org/chromium/content/browser/VSyncMonitorTest.java | 17 | ||||
-rw-r--r-- | ui/android/java/src/org/chromium/ui/VSyncMonitor.java | 39 |
2 files changed, 0 insertions, 56 deletions
diff --git a/content/public/android/javatests/src/org/chromium/content/browser/VSyncMonitorTest.java b/content/public/android/javatests/src/org/chromium/content/browser/VSyncMonitorTest.java index 7b34461..65153f7 100644 --- a/content/public/android/javatests/src/org/chromium/content/browser/VSyncMonitorTest.java +++ b/content/public/android/javatests/src/org/chromium/content/browser/VSyncMonitorTest.java @@ -127,21 +127,4 @@ public class VSyncMonitorTest extends InstrumentationTestCase { assertTrue(monitor.getVSyncPeriodInMicroseconds() < 1000000 / 30); } } - - @MediumTest - public void testVSyncActivationFromIdle() throws InterruptedException { - // Check that the vsync period roughly matches the timestamps that the monitor generates. - VSyncDataCollector collector = new VSyncDataCollector(1); - VSyncMonitor monitor = createVSyncMonitor(collector); - - requestVSyncMonitorUpdate(monitor); - collector.waitTillDone(); - assertTrue(collector.isDone()); - - long period = monitor.getVSyncPeriodInMicroseconds() / 1000; - long delay = SystemClock.uptimeMillis() - collector.mLastVSyncCpuTimeMillis; - - // The VSync should have activated immediately instead of at the next real vsync. - assertTrue(delay < period); - } } diff --git a/ui/android/java/src/org/chromium/ui/VSyncMonitor.java b/ui/android/java/src/org/chromium/ui/VSyncMonitor.java index 0396135..281ed14 100644 --- a/ui/android/java/src/org/chromium/ui/VSyncMonitor.java +++ b/ui/android/java/src/org/chromium/ui/VSyncMonitor.java @@ -51,8 +51,6 @@ public class VSyncMonitor { // If the monitor is activated after having been idle, we synthesize the first vsync to reduce // latency. private final Handler mHandler = new Handler(); - private final Runnable mSyntheticVSyncRunnable; - private long mLastVSyncCpuTimeNano; /** * Constructs a VSyncMonitor @@ -73,7 +71,6 @@ public class VSyncMonitor { @Override public void doFrame(long frameTimeNanos) { TraceEvent.begin("VSync"); - mHandler.removeCallbacks(mSyntheticVSyncRunnable); if (useEstimatedRefreshPeriod && mConsecutiveVSync) { // Display.getRefreshRate() is unreliable on some platforms. // Adjust refresh period- initial value is based on Display.getRefreshRate() @@ -88,16 +85,6 @@ public class VSyncMonitor { TraceEvent.end("VSync"); } }; - mSyntheticVSyncRunnable = new Runnable() { - @Override - public void run() { - TraceEvent.begin("VSyncSynthetic"); - mChoreographer.removeFrameCallback(mVSyncFrameCallback); - final long currentTime = getCurrentNanoTime(); - onVSyncCallback(estimateLastVSyncTime(currentTime), currentTime); - TraceEvent.end("VSyncSynthetic"); - } - }; mGoodStartingPointNano = getCurrentNanoTime(); } @@ -137,7 +124,6 @@ public class VSyncMonitor { assert mHaveRequestInFlight; mInsideVSync = true; mHaveRequestInFlight = false; - mLastVSyncCpuTimeNano = currentTimeNanos; try { if (mListener != null) { mListener.onVSync(this, frameTimeNanos / NANOSECONDS_PER_MICROSECOND); @@ -151,31 +137,6 @@ public class VSyncMonitor { if (mHaveRequestInFlight) return; mHaveRequestInFlight = true; mConsecutiveVSync = mInsideVSync; - // There's no way to tell if we're currently in the scope of a - // choregrapher frame callback, which might in turn allow us to honor - // the vsync callback in the current frame. Thus, we eagerly post the - // frame callback even when we post a synthetic frame callback. If the - // frame callback is honored before the synthetic callback, we simply - // remove the synthetic callback. - postSyntheticVSyncIfNecessary(); mChoreographer.postFrameCallback(mVSyncFrameCallback); } - - private void postSyntheticVSyncIfNecessary() { - // TODO(jdduke): Consider removing synthetic vsyncs altogether if - // they're found to be no longer necessary. - final long currentTime = getCurrentNanoTime(); - // Only trigger a synthetic vsync if we've been idle for long enough and the upcoming real - // vsync is more than half a frame away. - if (currentTime - mLastVSyncCpuTimeNano < 2 * mRefreshPeriodNano) return; - if (currentTime - estimateLastVSyncTime(currentTime) > mRefreshPeriodNano / 2) return; - mHandler.post(mSyntheticVSyncRunnable); - } - - private long estimateLastVSyncTime(long currentTime) { - final long lastRefreshTime = mGoodStartingPointNano - + ((currentTime - mGoodStartingPointNano) / mRefreshPeriodNano) - * mRefreshPeriodNano; - return lastRefreshTime; - } } |