Skip to content

Commit

Permalink
refactor: Replace callbacks with events (#3953)
Browse files Browse the repository at this point in the history 
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)
  • Loading branch information
@joeyparrish
joeyparrish committed Feb 16, 2022
1 parent 9119535 commit c6209fd
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 65 deletions.
59 changes: 26 additions & 33 deletions lib/media/region_observer.js
View file
Expand Up @@ -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
Expand All @@ -26,6 +28,8 @@ shaka.media.RegionObserver = class {
* @param {!shaka.media.RegionTimeline} timeline
*/
constructor(timeline) {
super();

/** @private {shaka.media.RegionTimeline} */
this.timeline_ = timeline;

Expand All @@ -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;
Expand All @@ -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),
},
];
}
Expand All @@ -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 */
Expand All @@ -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);
}

/**
Expand Down
35 changes: 16 additions & 19 deletions lib/media/region_timeline.js
View file
Expand Up @@ -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.<shaka.extern.TimelineRegionInfo>} */
this.regions_ = new Set();
/** @private {!function():{start: number, end: number}} */
Expand All @@ -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();
}

/**
Expand All @@ -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);
}
}

Expand All @@ -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);
}
}
}
Expand Down
29 changes: 21 additions & 8 deletions lib/player.js
View file
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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_);
Expand Down
13 changes: 9 additions & 4 deletions test/media/region_observer_unit.js
View file
Expand Up @@ -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', () => {
Expand Down
4 changes: 3 additions & 1 deletion test/media/region_timeline_unit.js
View file
Expand Up @@ -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(() => {
Expand Down

0 comments on commit c6209fd

Please sign in to comment.