From c6209fd69c5a59df96fae51059a2094a7a825bec Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Tue, 15 Feb 2022 17:23:40 -0800 Subject: [PATCH] refactor: Replace callbacks with events (#3953) Three classes (RegionTimeline, RegionObserver, and QualityObserver) were all designed with callbacks instead of events. A single callback is inflexible compared to events, which allow multiple listeners. We already have a long-standing and robust event system, so why not use it? Issue #3949 (memory leak in DASH live streams with inband EventStream) --- lib/media/region_observer.js | 59 +++++++++++++----------------- lib/media/region_timeline.js | 35 ++++++++---------- lib/player.js | 29 +++++++++++---- test/media/region_observer_unit.js | 13 +++++-- test/media/region_timeline_unit.js | 4 +- 5 files changed, 75 insertions(+), 65 deletions(-) diff --git a/lib/media/region_observer.js b/lib/media/region_observer.js index 33cbf76677..0ef0dc20af 100644 --- a/lib/media/region_observer.js +++ b/lib/media/region_observer.js @@ -8,16 +8,18 @@ goog.provide('shaka.media.RegionObserver'); goog.require('shaka.media.IPlayheadObserver'); goog.require('shaka.media.RegionTimeline'); +goog.require('shaka.util.FakeEvent'); +goog.require('shaka.util.FakeEventTarget'); /** * The region observer watches a region timeline and playhead, and fires events - * (onEnter, onExit, and onSkip) as the playhead moves. + * ('enter', 'exit', 'skip') as the playhead moves. * * @implements {shaka.media.IPlayheadObserver} * @final */ -shaka.media.RegionObserver = class { +shaka.media.RegionObserver = class extends shaka.util.FakeEventTarget { /** * Create a region observer for the given timeline. The observer does not * own the timeline, only uses it. This means that the observer should NOT @@ -26,6 +28,8 @@ shaka.media.RegionObserver = class { * @param {!shaka.media.RegionTimeline} timeline */ constructor(timeline) { + super(); + /** @private {shaka.media.RegionTimeline} */ this.timeline_ = timeline; @@ -39,13 +43,6 @@ shaka.media.RegionObserver = class { */ this.oldPosition_ = new Map(); - /** @private {shaka.media.RegionObserver.EventListener} */ - this.onEnter_ = (region, seeking) => {}; - /** @private {shaka.media.RegionObserver.EventListener} */ - this.onExit_ = (region, seeking) => {}; - /** @private {shaka.media.RegionObserver.EventListener} */ - this.onSkip_ = (region, seeking) => {}; - // To make the rules easier to read, alias all the relative positions. const RelativePosition = shaka.media.RegionObserver.RelativePosition_; const BEFORE_THE_REGION = RelativePosition.BEFORE_THE_REGION; @@ -62,37 +59,37 @@ shaka.media.RegionObserver = class { { weWere: null, weAre: IN_THE_REGION, - invoke: (region, seeking) => this.onEnter_(region, seeking), + invoke: (region, seeking) => this.onEvent_('enter', region, seeking), }, { weWere: BEFORE_THE_REGION, weAre: IN_THE_REGION, - invoke: (region, seeking) => this.onEnter_(region, seeking), + invoke: (region, seeking) => this.onEvent_('enter', region, seeking), }, { weWere: AFTER_THE_REGION, weAre: IN_THE_REGION, - invoke: (region, seeking) => this.onEnter_(region, seeking), + invoke: (region, seeking) => this.onEvent_('enter', region, seeking), }, { weWere: IN_THE_REGION, weAre: BEFORE_THE_REGION, - invoke: (region, seeking) => this.onExit_(region, seeking), + invoke: (region, seeking) => this.onEvent_('exit', region, seeking), }, { weWere: IN_THE_REGION, weAre: AFTER_THE_REGION, - invoke: (region, seeking) => this.onExit_(region, seeking), + invoke: (region, seeking) => this.onEvent_('exit', region, seeking), }, { weWere: BEFORE_THE_REGION, weAre: AFTER_THE_REGION, - invoke: (region, seeking) => this.onSkip_(region, seeking), + invoke: (region, seeking) => this.onEvent_('skip', region, seeking), }, { weWere: AFTER_THE_REGION, weAre: BEFORE_THE_REGION, - invoke: (region, seeking) => this.onSkip_(region, seeking), + invoke: (region, seeking) => this.onEvent_('skip', region, seeking), }, ]; } @@ -105,11 +102,7 @@ shaka.media.RegionObserver = class { // needed. this.oldPosition_.clear(); - // Clear the callbacks so that we don't hold onto any references external - // to this class. - this.onEnter_ = (region, seeking) => {}; - this.onExit_ = (region, seeking) => {}; - this.onSkip_ = (region, seeking) => {}; + super.release(); } /** @override */ @@ -134,20 +127,20 @@ shaka.media.RegionObserver = class { } /** - * Set all the listeners. This overrides any previous calls to |setListeners|. + * Dispatch events of the given type. All event types in this class have the + * same parameters: region and seeking. * - * @param {shaka.media.RegionObserver.EventListener} onEnter - * The callback for when we move from outside a region to inside a region. - * @param {shaka.media.RegionObserver.EventListener} onExit - * The callback for when we move from inside a region to outside a region. - * @param {shaka.media.RegionObserver.EventListener} onSkip - * The callback for when we move from before to after a region or from - * after to before a region. + * @param {string} eventType + * @param {shaka.extern.TimelineRegionInfo} region + * @param {boolean} seeking + * @private */ - setListeners(onEnter, onExit, onSkip) { - this.onEnter_ = onEnter; - this.onExit_ = onExit; - this.onSkip_ = onSkip; + onEvent_(eventType, region, seeking) { + const event = new shaka.util.FakeEvent(eventType, new Map([ + ['region', region], + ['seeking', seeking], + ])); + this.dispatchEvent(event); } /** diff --git a/lib/media/region_timeline.js b/lib/media/region_timeline.js index 7671ac1c8f..2453ec1884 100644 --- a/lib/media/region_timeline.js +++ b/lib/media/region_timeline.js @@ -6,24 +6,27 @@ goog.provide('shaka.media.RegionTimeline'); +goog.require('shaka.util.FakeEvent'); +goog.require('shaka.util.FakeEventTarget'); goog.require('shaka.util.IReleasable'); goog.require('shaka.util.Timer'); /** * The region timeline is a set of unique timeline region info entries. When - * a new entry is added, the |onAddRegion| callback will be called. + * a new entry is added, the 'regionadd' event will be fired. When an entry is + * deleted, the 'regionremove' event will be fired. * * @implements {shaka.util.IReleasable} * @final */ -shaka.media.RegionTimeline = class { +shaka.media.RegionTimeline = class extends shaka.util.FakeEventTarget { /** * @param {!function():{start: number, end: number}} getSeekRange */ constructor(getSeekRange) { - /** @private {function(shaka.extern.TimelineRegionInfo)} */ - this.onAddRegion_ = (region) => {}; + super(); + /** @private {!Set.} */ this.regions_ = new Set(); /** @private {!function():{start: number, end: number}} */ @@ -44,22 +47,9 @@ shaka.media.RegionTimeline = class { /** @override */ release() { - // Prevent us from holding onto any external references via the callback. - this.onAddRegion_ = (region) => {}; this.regions_.clear(); this.filterTimer_.stop(); - } - - /** - * Set the callbacks for events. This will override any previous calls to - * |setListeners|. - * - * @param {function(shaka.extern.TimelineRegionInfo)} onAddRegion - * Set the callback for when we add a new region. This callback will only - * be called when a region is unique (we reject duplicate regions). - */ - setListeners(onAddRegion) { - this.onAddRegion_ = onAddRegion; + super.release(); } /** @@ -72,7 +62,10 @@ shaka.media.RegionTimeline = class { // instead of making the parser track it. if (similarRegion == null) { this.regions_.add(region); - this.onAddRegion_(region); + const event = new shaka.util.FakeEvent('regionadd', new Map([ + ['region', region], + ])); + this.dispatchEvent(event); } } @@ -89,6 +82,10 @@ shaka.media.RegionTimeline = class { // reson to store or process them. if (region.endTime < seekRange.start) { this.regions_.delete(region); + const event = new shaka.util.FakeEvent('regionremove', new Map([ + ['region', region], + ])); + this.dispatchEvent(event); } } } diff --git a/lib/player.js b/lib/player.js index eb06ab7cc1..fb0560b1a4 100644 --- a/lib/player.js +++ b/lib/player.js @@ -1630,8 +1630,11 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // initialize it now even through it appears a little out-of-place. this.regionTimeline_ = new shaka.media.RegionTimeline(() => this.seekRange()); - this.regionTimeline_.setListeners(/* onRegionAdded= */ (region) => { + this.regionTimeline_.addEventListener('regionadd', (event) => { + /** @type {shaka.extern.TimelineRegionInfo} */ + const region = event['region']; this.onRegionEvent_(shaka.Player.EventName.TimelineRegionAdded, region); + if (this.adManager_) { this.adManager_.onDashTimedMetadata(region); } @@ -2522,21 +2525,31 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // Create the region observer. This will allow us to notify the app when we // move in and out of timeline regions. const regionObserver = new shaka.media.RegionObserver(this.regionTimeline_); - const onEnterRegion = (region, seeking) => { + + regionObserver.addEventListener('enter', (event) => { + /** @type {shaka.extern.TimelineRegionInfo} */ + const region = event['region']; this.onRegionEvent_(shaka.Player.EventName.TimelineRegionEnter, region); - }; - const onExitRegion = (region, seeking) => { + }); + + regionObserver.addEventListener('exit', (event) => { + /** @type {shaka.extern.TimelineRegionInfo} */ + const region = event['region']; this.onRegionEvent_(shaka.Player.EventName.TimelineRegionExit, region); - }; - const onSkipRegion = (region, seeking) => { + }); + + regionObserver.addEventListener('skip', (event) => { + /** @type {shaka.extern.TimelineRegionInfo} */ + const region = event['region']; + /** @type {boolean} */ + const seeking = event['seeking']; // If we are seeking, we don't want to surface the enter/exit events since // they didn't play through them. if (!seeking) { this.onRegionEvent_(shaka.Player.EventName.TimelineRegionEnter, region); this.onRegionEvent_(shaka.Player.EventName.TimelineRegionExit, region); } - }; - regionObserver.setListeners(onEnterRegion, onExitRegion, onSkipRegion); + }); // Now that we have all our observers, create a manager for them. const manager = new shaka.media.PlayheadObserverManager(this.video_); diff --git a/test/media/region_observer_unit.js b/test/media/region_observer_unit.js index 1f1bfb7596..7b888188ab 100644 --- a/test/media/region_observer_unit.js +++ b/test/media/region_observer_unit.js @@ -31,10 +31,15 @@ describe('RegionObserver', () => { () => { return {start: 0, end: 100}; }); observer = new shaka.media.RegionObserver(timeline); - observer.setListeners( - /* onEnter= */ shaka.test.Util.spyFunc(onEnterRegion), - /* onExit= */ shaka.test.Util.spyFunc(onExitRegion), - /* onSkip= */ shaka.test.Util.spyFunc(onSkipRegion)); + observer.addEventListener('enter', (event) => { + shaka.test.Util.spyFunc(onEnterRegion)(event['region'], event['seeking']); + }); + observer.addEventListener('exit', (event) => { + shaka.test.Util.spyFunc(onExitRegion)(event['region'], event['seeking']); + }); + observer.addEventListener('skip', (event) => { + shaka.test.Util.spyFunc(onSkipRegion)(event['region'], event['seeking']); + }); }); it('fires enter event when adding a region the playhead is in', () => { diff --git a/test/media/region_timeline_unit.js b/test/media/region_timeline_unit.js index 80aa9af546..fbff7cafb4 100644 --- a/test/media/region_timeline_unit.js +++ b/test/media/region_timeline_unit.js @@ -25,7 +25,9 @@ describe('RegionTimeline', () => { timeline = new shaka.media.RegionTimeline( shaka.test.Util.spyFunc(onSeekRange)); - timeline.setListeners(shaka.test.Util.spyFunc(onNewRegion)); + timeline.addEventListener('regionadd', (event) => { + shaka.test.Util.spyFunc(onNewRegion)(event['region']); + }); }); afterEach(() => {