From 0112f6a4ea0b156e2644da5965e3a65aed7747e0 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Mon, 17 Jun 2013 11:28:42 -0700 Subject: [PATCH 01/13] Improved requestAnimationFrame polyfill Processes all outstanding frame requests per frame, rather than one at a time. --- src/openseadragon.js | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/openseadragon.js b/src/openseadragon.js index c00673c8..5531831b 100644 --- a/src/openseadragon.js +++ b/src/openseadragon.js @@ -1675,6 +1675,7 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ }; } else { var aAnimQueue = [], + processing = [], iRequestId = 0, iIntervalId; @@ -1685,7 +1686,15 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ if ( !iIntervalId ) { iIntervalId = setInterval( function() { if ( aAnimQueue.length ) { - aAnimQueue.shift( )[ 1 ](+new Date()); + // Process all of the currently outstanding frame + // requests, but none that get added during the + // processing. + var time = +new Date(); + processing = aAnimQueue; + aAnimQueue = []; + while ( processing.length ) { + processing.shift()[ 1 ]( time ); + } } else { // don't continue the interval, if unnecessary clearInterval( iIntervalId ); @@ -1700,12 +1709,22 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ // create a mock cancelAnimationFrame function $.cancelAnimationFrame = function( requestId ) { // find the request ID and remove it - for ( var i = 0, j = aAnimQueue.length; i < j; i += 1 ) { + var i, j; + for ( i = 0, j = aAnimQueue.length; i < j; i += 1 ) { if ( aAnimQueue[ i ][ 0 ] === requestId ) { aAnimQueue.splice( i, 1 ); return; } } + + // If it's not in the queue, it may be in the set we're currently + // processing. + for ( i = 0, j = processing.length; i < j; i += 1 ) { + if ( processing[ i ][ 0 ] === requestId ) { + processing.splice( i, 1 ); + return; + } + } }; } })( window ); From 42249602520c0ba0f0f82f7e5fb4d856c2f03983 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Mon, 17 Jun 2013 11:30:31 -0700 Subject: [PATCH 02/13] Fixed another potential timer leak when closing a viewer If the viewer was closed in response to an event raised during the update function, the timer would have continued running. --- src/viewer.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/viewer.js b/src/viewer.js index 624431e4..f328e01e 100644 --- a/src/viewer.js +++ b/src/viewer.js @@ -1468,8 +1468,11 @@ function updateMulti( viewer ) { beginTime = +new Date(); updateOnce( viewer ); - THIS[ viewer.hash ].updateRequestId = scheduleUpdate( viewer, - arguments.callee, beginTime ); + // Request the next frame, unless we've been closed during the updateOnce() + if ( viewer.source ) { + THIS[ viewer.hash ].updateRequestId = scheduleUpdate( viewer, + arguments.callee, beginTime ); + } } function updateOnce( viewer ) { From c6a38dd80280f13b47dae71ceb391c884d846739 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Mon, 17 Jun 2013 11:31:41 -0700 Subject: [PATCH 03/13] Now closing the navigator when the viewer closes At the very least this was causing the navigator to continue to request and process frames even after its viewer had been closed. --- src/viewer.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/viewer.js b/src/viewer.js index f328e01e..03538992 100644 --- a/src/viewer.js +++ b/src/viewer.js @@ -470,7 +470,11 @@ $.extend( $.Viewer.prototype, $.EventHandler.prototype, $.ControlDock.prototype, THIS[ this.hash ].updateRequestId = null; } - if( this.drawer ){ + if ( this.navigator ) { + this.navigator.close(); + } + + if ( this.drawer ) { this.drawer.clearOverlays(); } From e99c126a85c2b61fa9819ec0f401fc772bedce8e Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Mon, 17 Jun 2013 11:51:02 -0700 Subject: [PATCH 04/13] Navigator now updates properly when reopening a viewer Before, if you closed a viewer and then opened a new tilesource into it, the navigator would still have the old tilesource. This patch fixes that. --- src/viewer.js | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/viewer.js b/src/viewer.js index 03538992..3265b027 100644 --- a/src/viewer.js +++ b/src/viewer.js @@ -1153,22 +1153,25 @@ function openTileSource( viewer, source ) { }); //Instantiate a navigator if configured - if ( _this.showNavigator && ! _this.navigator && !_this.collectionMode ){ - _this.navigator = new $.Navigator({ - id: _this.navigatorId, - position: _this.navigatorPosition, - sizeRatio: _this.navigatorSizeRatio, - height: _this.navigatorHeight, - width: _this.navigatorWidth, - // By passing the fully parsed source here, the navigator doesn't - // have to load it again. Additionally, we don't have to call - // navigator.open, as it's implicitly called in the ctor. - tileSources: source, - tileHost: _this.tileHost, - prefixUrl: _this.prefixUrl, - overlays: _this.overlays, - viewer: _this - }); + if ( _this.showNavigator && !_this.collectionMode ){ + // Note: By passing the fully parsed source, the navigator doesn't + // have to load it again. + if ( _this.navigator ) { + _this.navigator.open( source ); + } else { + _this.navigator = new $.Navigator({ + id: _this.navigatorId, + position: _this.navigatorPosition, + sizeRatio: _this.navigatorSizeRatio, + height: _this.navigatorHeight, + width: _this.navigatorWidth, + tileSources: source, + tileHost: _this.tileHost, + prefixUrl: _this.prefixUrl, + overlays: _this.overlays, + viewer: _this + }); + } } //Instantiate a referencestrip if configured From 713fad4224ed4ac04c51fed76842d4807b527162 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Tue, 18 Jun 2013 11:06:43 -0700 Subject: [PATCH 05/13] Exposing _updateRequestId as a semi-private member for testing purposes --- src/viewer.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/viewer.js b/src/viewer.js index 3265b027..7ea9b740 100644 --- a/src/viewer.js +++ b/src/viewer.js @@ -151,7 +151,6 @@ $.Viewer = function( options ) { THIS[ this.hash ] = { "fsBoundsDelta": new $.Point( 1, 1 ), "prevContainerSize": null, - "updateRequestId": null, "animating": false, "forceRedraw": false, "mouseInside": false, @@ -168,6 +167,8 @@ $.Viewer = function( options ) { "onfullscreenchange": null }; + this._updateRequestId = null; + //Inherit some behaviors and properties $.EventHandler.call( this ); $.ControlDock.call( this, options ); @@ -465,9 +466,9 @@ $.extend( $.Viewer.prototype, $.EventHandler.prototype, $.ControlDock.prototype, * @return {OpenSeadragon.Viewer} Chainable. */ close: function ( ) { - if ( THIS[ this.hash ].updateRequestId !== null ){ - $.cancelAnimationFrame( THIS[ this.hash ].updateRequestId ); - THIS[ this.hash ].updateRequestId = null; + if ( this._updateRequestId !== null ) { + $.cancelAnimationFrame( this._updateRequestId ); + this._updateRequestId = null; } if ( this.navigator ) { @@ -1195,7 +1196,7 @@ function openTileSource( viewer, source ) { THIS[ _this.hash ].animating = false; THIS[ _this.hash ].forceRedraw = true; - THIS[ _this.hash ].updateRequestId = scheduleUpdate( _this, updateMulti ); + _this._updateRequestId = scheduleUpdate( _this, updateMulti ); //Assuming you had programatically created a bunch of overlays //and added them via configuration @@ -1468,7 +1469,7 @@ function updateMulti( viewer ) { var beginTime; if ( !viewer.source ) { - THIS[ viewer.hash ].updateRequestId = null; + viewer._updateRequestId = null; return; } @@ -1477,7 +1478,7 @@ function updateMulti( viewer ) { // Request the next frame, unless we've been closed during the updateOnce() if ( viewer.source ) { - THIS[ viewer.hash ].updateRequestId = scheduleUpdate( viewer, + viewer._updateRequestId = scheduleUpdate( viewer, arguments.callee, beginTime ); } } From 862b560cb46f95349b5d70ca27b61fa4504642e8 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Tue, 18 Jun 2013 11:08:16 -0700 Subject: [PATCH 06/13] Basic test no longer uses navigator since we have a suite for that --- test/basic.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/basic.js b/test/basic.js index 701117f2..ea4e9aad 100644 --- a/test/basic.js +++ b/test/basic.js @@ -13,8 +13,7 @@ id: 'example', prefixUrl: '/build/openseadragon/images/', tileSources: '/test/data/testpattern.dzi', - springStiffness: 100, // Faster animation = faster tests - showNavigator: true + springStiffness: 100 // Faster animation = faster tests }); ok(viewer, 'Viewer exists'); From 9a28793e6455954f5f47bcebafd93df00d39239c Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Tue, 18 Jun 2013 11:08:33 -0700 Subject: [PATCH 07/13] Testing source and timer management in basic test --- test/basic.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/basic.js b/test/basic.js index ea4e9aad..a336e25b 100644 --- a/test/basic.js +++ b/test/basic.js @@ -24,6 +24,8 @@ equal(eventSender, viewer, 'Sender of open event was viewer'); ok(eventData, 'Handler also received event data'); ok(viewer.viewport, 'Viewport exists'); + ok(viewer.source, 'source exists'); + ok(viewer._updateRequestId, 'timer is on'); start(); }; @@ -122,9 +124,14 @@ asyncTest('Close', function() { var closeHandler = function() { viewer.removeHandler('close', closeHandler); + ok(!viewer.source, 'no source'); $('#example').empty(); ok(true, 'Close event was sent'); - start(); + ok(!viewer._updateRequestId, 'timer is off'); + setTimeout(function() { + ok(!viewer._updateRequestId, 'timer is still off'); + start(); + }, 100); }; viewer.addHandler('close', closeHandler); From b38e319f2c12e2539445ed6b2234b6fc8a0e6498 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Tue, 18 Jun 2013 11:09:05 -0700 Subject: [PATCH 08/13] Testing closing one image and opening another in navigator suite --- test/navigator.js | 55 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/navigator.js b/test/navigator.js index bb4c06e3..547e39a3 100644 --- a/test/navigator.js +++ b/test/navigator.js @@ -461,4 +461,59 @@ QUnit.config.autostart = false; }); }); + asyncTest('Viewer closing one image and opening another', function() { + var timeWatcher = Util.timeWatcher(); + + viewer = OpenSeadragon({ + id: 'example', + prefixUrl: '/build/openseadragon/images/', + tileSources: '/test/data/testpattern.dzi', + springStiffness: 100, // Faster animation = faster tests + showNavigator: true + }); + + var openHandler1 = function(eventSender, eventData) { + viewer.removeHandler('open', openHandler1); + ok(viewer.navigator, 'navigator exists'); + viewer.navigator.addHandler('open', navOpenHandler1); + }; + + var navOpenHandler1 = function(eventSender, eventData) { + viewer.navigator.removeHandler('open', navOpenHandler1); + equal(viewer.navigator.source, viewer.source, 'viewer and navigator have the same source'); + ok(viewer.navigator._updateRequestId, 'navigator timer is on'); + viewer.addHandler('close', closeHandler1); + viewer.addHandler('open', openHandler2); + viewer.open('/test/data/tall.dzi'); + }; + + var closeHandler1 = function() { + viewer.removeHandler('close', closeHandler1); + ok(true, 'calling open closes the old one'); + equal(viewer.navigator.source, null, 'navigator source has been cleared'); + }; + + var openHandler2 = function(eventSender, eventData) { + viewer.removeHandler('open', openHandler2); + viewer.navigator.addHandler('open', navOpenHandler2); + }; + + var navOpenHandler2 = function(eventSender, eventData) { + viewer.navigator.removeHandler('open', navOpenHandler2); + equal(viewer.navigator.source, viewer.source, 'viewer and navigator have the same source'); + viewer.addHandler('close', closeHandler2); + viewer.close(); + }; + + var closeHandler2 = function() { + viewer.removeHandler('close', closeHandler2); + ok(!viewer.navigator._updateRequestId, 'navigator timer is off'); + setTimeout(function() { + ok(!viewer.navigator._updateRequestId, 'navigator timer is still off'); + timeWatcher.done(); + }, 100); + }; + + viewer.addHandler('open', openHandler1); + }); })(); From dbc8a59ae8c05d51d533d83aee242bb0171c8ec8 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Wed, 19 Jun 2013 09:58:09 -0700 Subject: [PATCH 09/13] Cleaned out dead code in scheduleUpdate and updateMulti MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit … as pointed out by @ventero --- src/viewer.js | 25 ++----------------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/src/viewer.js b/src/viewer.js index 7ea9b740..35ee5e30 100644 --- a/src/viewer.js +++ b/src/viewer.js @@ -1243,23 +1243,7 @@ function openTileSource( viewer, source ) { /////////////////////////////////////////////////////////////////////////////// // Schedulers provide the general engine for animation /////////////////////////////////////////////////////////////////////////////// -function scheduleUpdate( viewer, updateFunc, prevUpdateTime ){ - var currentTime, - targetTime, - deltaTime; - - if ( THIS[ viewer.hash ].animating ) { - return $.requestAnimationFrame( function(){ - updateFunc( viewer ); - } ); - } - - currentTime = +new Date(); - prevUpdateTime = prevUpdateTime ? prevUpdateTime : currentTime; - // 60 frames per second is ideal - targetTime = prevUpdateTime + 1000 / 60; - deltaTime = Math.max( 1, targetTime - currentTime ); - +function scheduleUpdate( viewer, updateFunc ){ return $.requestAnimationFrame( function(){ updateFunc( viewer ); } ); @@ -1465,21 +1449,16 @@ function onContainerEnter( tracker, position, buttonDownElement, buttonDownAny ) /////////////////////////////////////////////////////////////////////////////// function updateMulti( viewer ) { - - var beginTime; - if ( !viewer.source ) { viewer._updateRequestId = null; return; } - beginTime = +new Date(); updateOnce( viewer ); // Request the next frame, unless we've been closed during the updateOnce() if ( viewer.source ) { - viewer._updateRequestId = scheduleUpdate( viewer, - arguments.callee, beginTime ); + viewer._updateRequestId = scheduleUpdate( viewer, updateMulti ); } } From 2e48239056462ba4ce1da96da4b363c916875b5b Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Wed, 19 Jun 2013 09:58:22 -0700 Subject: [PATCH 10/13] Addressing code review comments --- src/openseadragon.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/openseadragon.js b/src/openseadragon.js index 5531831b..69434754 100644 --- a/src/openseadragon.js +++ b/src/openseadragon.js @@ -1686,12 +1686,15 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ if ( !iIntervalId ) { iIntervalId = setInterval( function() { if ( aAnimQueue.length ) { + var time = (new Date()).getTime(); // Process all of the currently outstanding frame // requests, but none that get added during the // processing. - var time = +new Date(); + // Swap the arrays so we don't have to create a new + // array every frame. + var temp = processing; processing = aAnimQueue; - aAnimQueue = []; + aAnimQueue = temp; while ( processing.length ) { processing.shift()[ 1 ]( time ); } @@ -1718,7 +1721,8 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ } // If it's not in the queue, it may be in the set we're currently - // processing. + // processing (if cancelAnimationFrame is called from within a + // requestAnimationFrame callback). for ( i = 0, j = processing.length; i < j; i += 1 ) { if ( processing[ i ][ 0 ] === requestId ) { processing.splice( i, 1 ); From 4022206261fd0ec5890443edac47940463ee3c37 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Wed, 19 Jun 2013 10:03:44 -0700 Subject: [PATCH 11/13] Removed the parens around new Date call, for consistency --- src/openseadragon.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openseadragon.js b/src/openseadragon.js index 69434754..9aa30d76 100644 --- a/src/openseadragon.js +++ b/src/openseadragon.js @@ -1686,7 +1686,7 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ if ( !iIntervalId ) { iIntervalId = setInterval( function() { if ( aAnimQueue.length ) { - var time = (new Date()).getTime(); + var time = new Date().getTime(); // Process all of the currently outstanding frame // requests, but none that get added during the // processing. From 2c2d1486cd1099fcdc374a0bdb6bd949965e514c Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Wed, 19 Jun 2013 10:16:59 -0700 Subject: [PATCH 12/13] Added changelog for #133 --- changelog.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/changelog.txt b/changelog.txt index 2e4d94a3..46f935b4 100644 --- a/changelog.txt +++ b/changelog.txt @@ -4,6 +4,7 @@ OPENSEADRAGON CHANGELOG 0.9.129: (In Progress) * Fixed an error when using navPrevNextWrap on single images (#135) +* Various fixes to our timer handling (#133) 0.9.128: From e0282a3c868b6483d550ac074dd2034f441b1987 Mon Sep 17 00:00:00 2001 From: Ian Gilman Date: Wed, 19 Jun 2013 13:33:01 -0700 Subject: [PATCH 13/13] Stripped whitespace --- src/openseadragon.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openseadragon.js b/src/openseadragon.js index 85e1b746..9146f659 100644 --- a/src/openseadragon.js +++ b/src/openseadragon.js @@ -1688,7 +1688,7 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ if ( aAnimQueue.length ) { var time = new Date().getTime(); // Process all of the currently outstanding frame - // requests, but none that get added during the + // requests, but none that get added during the // processing. // Swap the arrays so we don't have to create a new // array every frame. @@ -1721,7 +1721,7 @@ window.OpenSeadragon = window.OpenSeadragon || function( options ){ } // If it's not in the queue, it may be in the set we're currently - // processing (if cancelAnimationFrame is called from within a + // processing (if cancelAnimationFrame is called from within a // requestAnimationFrame callback). for ( i = 0, j = processing.length; i < j; i += 1 ) { if ( processing[ i ][ 0 ] === requestId ) {