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 6023878 commit e49c849
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 101 deletions.
51 changes: 17 additions & 34 deletions lib/media/quality_observer.js
View file
Expand Up @@ -8,48 +8,40 @@ goog.provide('shaka.media.QualityObserver');

goog.require('shaka.media.IPlayheadObserver');
goog.require('shaka.log');
goog.require('shaka.util.FakeEvent');
goog.require('shaka.util.FakeEventTarget');

/**
* Monitors the quality of content being appended to the source
* buffers and fires onQualityChange events when the media quality
* at the playhead changes.
* Monitors the quality of content being appended to the source buffers and
* fires 'qualitychange' events when the media quality at the playhead changes.
*
* @implements {shaka.media.IPlayheadObserver}
* @final
*/
shaka.media.QualityObserver = class {
shaka.media.QualityObserver = class extends shaka.util.FakeEventTarget {
/**
* Creates a new QualityObserver.
*
* @param {!function():!shaka.extern.BufferedInfo} getBufferedInfo
* Buffered info is needed to purge QualityChanges that are no
* longer relevant.
* Buffered info is needed to purge QualityChanges that are no
* longer relevant.
*/
constructor(getBufferedInfo) {
super();

/**
* @private {!Map.<string,!shaka.media.QualityObserver.ContentTypeState>}
*/
this.contentTypeStates_ = new Map();

/** @private {shaka.media.QualityObserver.EventListener} */
this.onQualityChange_ = (mediaQuality, position) => {};

/** @private function():!shaka.extern.BufferedInfo */
this.getBufferedInfo_ = getBufferedInfo;
}

/**
* Set all the listeners. This overrides any previous calls to |setListeners|.
*
* @param {shaka.media.QualityObserver.EventListener} onQualityChange
*/
setListeners(onQualityChange) {
this.onQualityChange_ = onQualityChange;
}

/** @override */
release() {
this.contentTypeStates_.clear();
super.release();
}

/**
Expand Down Expand Up @@ -169,10 +161,15 @@ shaka.media.QualityObserver = class {
if (this.positionIsBuffered_(
positionInSeconds, qualityAtPosition.contentType)) {
contentTypeState.currentQuality = qualityAtPosition;

shaka.log.debug('Media quality changed at position ' +
positionInSeconds + ' ' + JSON.stringify(qualityAtPosition));
this.onQualityChange_(
qualityAtPosition, positionInSeconds);

const event = new shaka.util.FakeEvent('qualitychange', new Map([
['quality', qualityAtPosition],
['position', positionInSeconds],
]));
this.dispatchEvent(event);
}
}
}
Expand Down Expand Up @@ -237,20 +234,6 @@ shaka.media.QualityObserver = class {
}
};

/**
* @typedef {function(shaka.extern.MediaQualityInfo, number)}
*
* @description
* A callback function used to notify the player when media quality changes
* are detected at the playhead.
*
* The first argument is information about media quality at the playhead
* position.
*
* The second argument is the playhead position in seconds.
*/
shaka.media.QualityObserver.EventListener;

/**
* @typedef {{
* mediaQuality: !shaka.extern.MediaQualityInfo,
Expand Down
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
37 changes: 28 additions & 9 deletions lib/player.js
View file
Expand Up @@ -1694,17 +1694,26 @@ 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);
}
});

this.qualityObserver_ = null;
if (this.config_.streaming.observeQualityChanges) {
this.qualityObserver_ = new shaka.media.QualityObserver(
() => this.getBufferedInfo());
this.qualityObserver_.setListeners((mediaQualityInfo, position) => {

this.qualityObserver_.addEventListener('qualitychange', (event) => {
/** @type {shaka.extern.MediaQualityInfo} */
const mediaQualityInfo = event['quality'];
/** @type {number} */
const position = event['position'];
this.onMediaQualityChange_(mediaQualityInfo, position);
});
}
Expand Down Expand Up @@ -2677,21 +2686,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
5 changes: 4 additions & 1 deletion test/media/quality_observer_unit.js
View file
Expand Up @@ -50,7 +50,10 @@ describe('QualityObserver', () => {
beforeEach(() => {
onQualityChange = jasmine.createSpy('onQualityChange');
observer = new shaka.media.QualityObserver(getBufferedInfo);
observer.setListeners(shaka.test.Util.spyFunc(onQualityChange));
observer.addEventListener('qualitychange', (event) => {
shaka.test.Util.spyFunc(onQualityChange)(
event['quality'], event['position']);
});
emptyBuffer = true;
bufferStart = 0;
bufferEnd = 0;
Expand Down

0 comments on commit e49c849

Please sign in to comment.