From 4c80e2a5c0ba9005e7021c0b63a83dd86fea9f2c Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Mon, 14 Nov 2016 15:09:30 -0500 Subject: [PATCH] revert: perf: Cache currentTime and buffered from Flash (#3705) This reverts commit 45ffa810fb7a3e771b708b3882c8f00ea6c9d219. --- src/js/tech/flash-cache.js | 31 ---------------- src/js/tech/flash.js | 45 +++++++++-------------- test/unit/tech/flash-cache.test.js | 50 -------------------------- test/unit/tech/flash.test.js | 57 ++++++++++++++++-------------- 4 files changed, 46 insertions(+), 137 deletions(-) delete mode 100644 src/js/tech/flash-cache.js delete mode 100644 test/unit/tech/flash-cache.test.js diff --git a/src/js/tech/flash-cache.js b/src/js/tech/flash-cache.js deleted file mode 100644 index 1c9af41465..0000000000 --- a/src/js/tech/flash-cache.js +++ /dev/null @@ -1,31 +0,0 @@ -/** - * @file flash-cache.js - * - * Auto-caching method wrapper to avoid calling through to Flash too - * often. - */ - -/** - * Returns a new getter function that returns a cached value if - * invoked multiple times within the specified duration. - * - * @param {Function} getter the function to be cached - * @param {Number} cacheDuration the number of milliseconds to cache - * results for - * @return {Function} a new function that returns cached results if - * invoked multiple times within the cache duration - */ -export default function timeExpiringCache(getter, cacheDuration) { - const result = function cachedGetter() { - const now = new Date().getTime(); - - if (now - result.lastCheckTime_ >= cacheDuration) { - result.lastCheckTime_ = now; - result.cache_ = getter(); - } - return result.cache_; - }; - - result.lastCheckTime_ = -Infinity; - return result; -} diff --git a/src/js/tech/flash.js b/src/js/tech/flash.js index 9e8d276100..dd7ccefa59 100644 --- a/src/js/tech/flash.js +++ b/src/js/tech/flash.js @@ -10,7 +10,6 @@ import * as Dom from '../utils/dom.js'; import * as Url from '../utils/url.js'; import { createTimeRange } from '../utils/time-ranges.js'; import FlashRtmpDecorator from './flash-rtmp'; -import timeExpiringCache from './flash-cache.js'; import Component from '../component'; import window from 'global/window'; import assign from 'object.assign'; @@ -60,34 +59,6 @@ class Flash extends Tech { this.on('seeked', function() { this.lastSeekTarget_ = undefined; }); - - // calling into the SWF can be expensive, especially if Flash is - // busy rendering video frames - // automatically cache commonly used properties for a short period - // of time so that multiple calls within a short time period don't - // all pay a big performance penalty for properties that change - // relatively slowly over time - const getCurrentTimeCached = timeExpiringCache(() => { - return this.el_.vjs_getProperty('currentTime'); - }, 100); - - this.currentTime = (time) => { - // when seeking make the reported time keep up with the requested time - // by reading the time we're seeking to - if (this.seeking()) { - return this.lastSeekTarget_ || 0; - } - - return getCurrentTimeCached(); - }; - this.buffered = timeExpiringCache(() => { - const ranges = this.el_.vjs_getProperty('buffered'); - - if (ranges.length === 0) { - return createTimeRange(); - } - return createTimeRange(ranges[0][0], ranges[0][1]); - }, 100); } /** @@ -242,6 +213,14 @@ class Flash extends Tech { * @return {Number} Current time * @method currentTime */ + currentTime(time) { + // when seeking make the reported time keep up with the requested time + // by reading the time we're seeking to + if (this.seeking()) { + return this.lastSeekTarget_ || 0; + } + return this.el_.vjs_getProperty('currentTime'); + } /** * Get current source @@ -315,6 +294,14 @@ class Flash extends Tech { * @return {TimeRangeObject} * @method buffered */ + buffered() { + const ranges = this.el_.vjs_getProperty('buffered'); + + if (ranges.length === 0) { + return createTimeRange(); + } + return createTimeRange(ranges[0][0], ranges[0][1]); + } /** * Get fullscreen support - diff --git a/test/unit/tech/flash-cache.test.js b/test/unit/tech/flash-cache.test.js deleted file mode 100644 index 031f57c039..0000000000 --- a/test/unit/tech/flash-cache.test.js +++ /dev/null @@ -1,50 +0,0 @@ -/* eslint-env qunit */ -import timeExpiringCache from '../../../src/js/tech/flash-cache.js'; -import sinon from 'sinon'; - -QUnit.module('Flash Cache', { - beforeEach() { - this.clock = sinon.useFakeTimers(); - }, - afterEach() { - this.clock.restore(); - } -}); - -QUnit.test('calls the getter on first invocation', function(assert) { - let calls = 0; - const cached = timeExpiringCache(() => calls++, 100); - - QUnit.equal(cached(), 0, 'returned a value'); - QUnit.equal(calls, 1, 'called the getter'); -}); - -QUnit.test('caches results', function(assert) { - let calls = 0; - const cached = timeExpiringCache(() => calls++, 100); - - for (let i = 0; i < 31; i++) { - QUnit.equal(cached(), 0, 'returned a cached value'); - } - QUnit.equal(calls, 1, 'only called getter once'); -}); - -QUnit.test('refreshes the cache after the result expires', function(assert) { - let calls = 0; - const cached = timeExpiringCache(() => calls++, 1); - - QUnit.equal(cached(), 0, 'returned a value'); - QUnit.equal(cached(), 0, 'returned a cached value'); - QUnit.equal(calls, 1, 'only called getter once'); - this.clock.tick(1); - - QUnit.equal(cached(), 1, 'returned a value'); - QUnit.equal(cached(), 1, 'returned a cached value'); - QUnit.equal(calls, 2, 'called getter again'); - - this.clock.tick(10); - QUnit.equal(calls, 2, 'only refreshes on-demand'); - QUnit.equal(cached(), 2, 'returned a value'); - QUnit.equal(cached(), 2, 'returned a cached value'); - QUnit.equal(calls, 3, 'called getter again'); -}); diff --git a/test/unit/tech/flash.test.js b/test/unit/tech/flash.test.js index 2a5e299fa5..96dcd7e2c2 100644 --- a/test/unit/tech/flash.test.js +++ b/test/unit/tech/flash.test.js @@ -31,7 +31,8 @@ QUnit.test('Flash.canPlaySource', function(assert) { }); QUnit.test('currentTime', function(assert) { - const mockFlash = new MockFlash(); + const getCurrentTime = Flash.prototype.currentTime; + const setCurrentTime = Flash.prototype.setCurrentTime; let seekingCount = 0; let seeking = false; let setPropVal; @@ -39,42 +40,44 @@ QUnit.test('currentTime', function(assert) { let result; // Mock out a Flash instance to avoid creating the swf object - mockFlash.el_ = { - /* eslint-disable camelcase */ - vjs_setProperty(prop, val) { - setPropVal = val; + const mockFlash = { + el_: { + /* eslint-disable camelcase */ + vjs_setProperty(prop, val) { + setPropVal = val; + }, + vjs_getProperty() { + return getPropVal; + } + /* eslint-enable camelcase */ }, - vjs_getProperty() { - return getPropVal; - } - /* eslint-enable camelcase */ - }; - mockFlash.seekable = function() { - return createTimeRange(5, 1000); - }; - mockFlash.trigger = function(event) { - if (event === 'seeking') { - seekingCount++; + seekable() { + return createTimeRange(5, 1000); + }, + trigger(event) { + if (event === 'seeking') { + seekingCount++; + } + }, + seeking() { + return seeking; } }; - mockFlash.seeking = function() { - return seeking; - }; // Test the currentTime getter getPropVal = 3; - result = mockFlash.currentTime(); + result = getCurrentTime.call(mockFlash); assert.equal(result, 3, 'currentTime is retreived from the swf element'); // Test the currentTime setter - mockFlash.setCurrentTime(10); + setCurrentTime.call(mockFlash, 10); assert.equal(setPropVal, 10, 'currentTime is set on the swf element'); assert.equal(seekingCount, 1, 'triggered seeking'); // Test current time while seeking - mockFlash.setCurrentTime(20); + setCurrentTime.call(mockFlash, 20); seeking = true; - result = mockFlash.currentTime(); + result = getCurrentTime.call(mockFlash); assert.equal(result, 20, 'currentTime is retrieved from the lastSeekTarget while seeking'); @@ -84,13 +87,13 @@ QUnit.test('currentTime', function(assert) { assert.equal(seekingCount, 2, 'triggered seeking'); // clamp seeks to seekable - mockFlash.setCurrentTime(1001); - result = mockFlash.currentTime(); + setCurrentTime.call(mockFlash, 1001); + result = getCurrentTime.call(mockFlash); assert.equal(result, mockFlash.seekable().end(0), 'clamped to the seekable end'); assert.equal(seekingCount, 3, 'triggered seeking'); - mockFlash.setCurrentTime(1); - result = mockFlash.currentTime(); + setCurrentTime.call(mockFlash, 1); + result = getCurrentTime.call(mockFlash); assert.equal(result, mockFlash.seekable().start(0), 'clamped to the seekable start'); assert.equal(seekingCount, 4, 'triggered seeking'); });