From 6eb81dae36a9cced95bd99f5356b69205c264ee0 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Wed, 16 Feb 2022 14:02:54 -0800 Subject: [PATCH] fix: Fix memory leak in DASH live streams with inband EventStream (#3957) EventStreams in DASH generate TimelineRegionInfo objects, which are then stored in the RegionTimeline and RegionObserver classes. But DashParser would add all regions to RegionTimeline, even if they would be quickly removed again, and RegionObserver would cache some regions from the timeline without ever removing them. This fixes the issue from both of those directions. DashParser will now ignore regions that are outside the DVR window (and therefore would soon be removed from RegionTimeline), and RegionObserver listens to an event on RegionTimeline to clean up its own storage when regions fall outside the DVR window during playback. Closes #3949 (memory leak in DASH live streams with inband EventStream) --- lib/dash/dash_parser.js | 50 +++++++++++++--------- lib/media/region_observer.js | 13 ++++++ lib/media/region_timeline.js | 1 + test/dash/dash_parser_live_unit.js | 43 +++++++++++++++++++ test/dash/dash_parser_manifest_unit.js | 3 +- test/media/region_observer_unit.js | 32 +++++++++++++- test/media/region_timeline_unit.js | 58 ++++++++++++++++---------- 7 files changed, 158 insertions(+), 42 deletions(-) diff --git a/lib/dash/dash_parser.js b/lib/dash/dash_parser.js index 08cc1f2318..9ef9d16190 100644 --- a/lib/dash/dash_parser.js +++ b/lib/dash/dash_parser.js @@ -410,6 +410,24 @@ shaka.dash.DashParser = class { this.config_.dash.autoCorrectDrift); } + presentationTimeline.setStatic(mpdType == 'static'); + + const isLive = presentationTimeline.isLive(); + + // If it's live, we check for an override. + if (isLive && !isNaN(this.config_.availabilityWindowOverride)) { + segmentAvailabilityDuration = this.config_.availabilityWindowOverride; + } + + // If it's null, that means segments are always available. This is always + // the case for VOD, and sometimes the case for live. + if (segmentAvailabilityDuration == null) { + segmentAvailabilityDuration = Infinity; + } + + presentationTimeline.setSegmentAvailabilityDuration( + segmentAvailabilityDuration); + const profiles = mpd.getAttribute('profiles') || ''; /** @type {shaka.dash.DashParser.Context} */ @@ -432,7 +450,6 @@ shaka.dash.DashParser = class { const duration = periodsAndDuration.duration; const periods = periodsAndDuration.periods; - presentationTimeline.setStatic(mpdType == 'static'); if (mpdType == 'static' || !periodsAndDuration.durationDerivedFromPeriods) { // Ignore duration calculated from Period lengths if this is dynamic. @@ -451,6 +468,7 @@ shaka.dash.DashParser = class { this.lowLatencyMode_ = this.playerInterface_.isLowLatencyMode(); } } + if (this.lowLatencyMode_) { presentationTimeline.setAvailabilityTimeOffset( this.minTotalAvailabilityTimeOffset_); @@ -464,22 +482,6 @@ shaka.dash.DashParser = class { 'https://bit.ly/3clctcj for details.'); } - const isLive = presentationTimeline.isLive(); - - // If it's live, we check for an override. - if (isLive && !isNaN(this.config_.availabilityWindowOverride)) { - segmentAvailabilityDuration = this.config_.availabilityWindowOverride; - } - - // If it's null, that means segments are always available. This is always - // the case for VOD, and sometimes the case for live. - if (segmentAvailabilityDuration == null) { - segmentAvailabilityDuration = Infinity; - } - - presentationTimeline.setSegmentAvailabilityDuration( - segmentAvailabilityDuration); - // Use @maxSegmentDuration to override smaller, derived values. presentationTimeline.notifyMaxSegmentDuration(maxSegmentDuration || 1); if (goog.DEBUG) { @@ -714,8 +716,12 @@ shaka.dash.DashParser = class { const eventStreamNodes = XmlUtils.findChildren(periodInfo.node, 'EventStream'); + const availabilityStart = + context.presentationTimeline.getSegmentAvailabilityStart(); + for (const node of eventStreamNodes) { - this.parseEventStream_(periodInfo.start, periodInfo.duration, node); + this.parseEventStream_( + periodInfo.start, periodInfo.duration, node, availabilityStart); } const adaptationSetNodes = @@ -1769,9 +1775,10 @@ shaka.dash.DashParser = class { * @param {number} periodStart * @param {?number} periodDuration * @param {!Element} elem + * @param {number} availabilityStart * @private */ - parseEventStream_(periodStart, periodDuration, elem) { + parseEventStream_(periodStart, periodDuration, elem, availabilityStart) { const XmlUtils = shaka.util.XmlUtils; const parseNumber = XmlUtils.parseNonNegativeInt; @@ -1794,6 +1801,11 @@ shaka.dash.DashParser = class { endTime = Math.min(endTime, periodStart + periodDuration); } + // Don't add unavailable regions to the timeline. + if (endTime < availabilityStart) { + continue; + } + /** @type {shaka.extern.TimelineRegionInfo} */ const region = { schemeIdUri: schemeIdUri, diff --git a/lib/media/region_observer.js b/lib/media/region_observer.js index 0ef0dc20af..e4666d42f8 100644 --- a/lib/media/region_observer.js +++ b/lib/media/region_observer.js @@ -8,6 +8,7 @@ goog.provide('shaka.media.RegionObserver'); goog.require('shaka.media.IPlayheadObserver'); goog.require('shaka.media.RegionTimeline'); +goog.require('shaka.util.EventManager'); goog.require('shaka.util.FakeEvent'); goog.require('shaka.util.FakeEventTarget'); @@ -92,6 +93,15 @@ shaka.media.RegionObserver = class extends shaka.util.FakeEventTarget { invoke: (region, seeking) => this.onEvent_('skip', region, seeking), }, ]; + + /** @private {shaka.util.EventManager} */ + this.eventManager_ = new shaka.util.EventManager(); + + this.eventManager_.listen(this.timeline_, 'regionremove', (event) => { + /** @type {shaka.extern.TimelineRegionInfo} */ + const region = event['region']; + this.oldPosition_.delete(region); + }); } /** @override */ @@ -102,6 +112,9 @@ shaka.media.RegionObserver = class extends shaka.util.FakeEventTarget { // needed. this.oldPosition_.clear(); + this.eventManager_.release(); + this.eventManager_ = null; + super.release(); } diff --git a/lib/media/region_timeline.js b/lib/media/region_timeline.js index 2453ec1884..31bf487141 100644 --- a/lib/media/region_timeline.js +++ b/lib/media/region_timeline.js @@ -29,6 +29,7 @@ shaka.media.RegionTimeline = class extends shaka.util.FakeEventTarget { /** @private {!Set.} */ this.regions_ = new Set(); + /** @private {!function():{start: number, end: number}} */ this.getSeekRange_ = getSeekRange; diff --git a/test/dash/dash_parser_live_unit.js b/test/dash/dash_parser_live_unit.js index 4927040612..2d2b858443 100644 --- a/test/dash/dash_parser_live_unit.js +++ b/test/dash/dash_parser_live_unit.js @@ -1389,6 +1389,49 @@ describe('DashParser Live', () => { .toHaveBeenCalledWith( jasmine.objectContaining({id: '2', startTime: 10, endTime: 25})); }); + + it('will not add timeline regions outside the DVR window', async () => { + const manifest = [ + '', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + '', + ].join('\n'); + + // 45 seconds after the epoch, which is also 45 seconds after the + // presentation started. Only the last 30 seconds are available, from + // time 15 to 45. + Date.now = () => 45 * 1000; + + fakeNetEngine.setResponseText('dummy://foo', manifest); + await parser.start('dummy://foo', playerInterface); + + expect(onTimelineRegionAddedSpy).toHaveBeenCalledTimes(4); + expect(onTimelineRegionAddedSpy).toHaveBeenCalledWith( + jasmine.objectContaining({id: '1'})); + expect(onTimelineRegionAddedSpy).toHaveBeenCalledWith( + jasmine.objectContaining({id: '2'})); + expect(onTimelineRegionAddedSpy).toHaveBeenCalledWith( + jasmine.objectContaining({id: '3'})); + expect(onTimelineRegionAddedSpy).toHaveBeenCalledWith( + jasmine.objectContaining({id: '4'})); + }); }); it('honors clockSyncUri for in-progress recordings', async () => { diff --git a/test/dash/dash_parser_manifest_unit.js b/test/dash/dash_parser_manifest_unit.js index 0e263f5a02..cc3bf6aae0 100644 --- a/test/dash/dash_parser_manifest_unit.js +++ b/test/dash/dash_parser_manifest_unit.js @@ -985,7 +985,8 @@ describe('DashParser Manifest', () => { it('duplicate Representation ids with live', async () => { const source = [ - '', + '', ' ', ' ', ' ', diff --git a/test/media/region_observer_unit.js b/test/media/region_observer_unit.js index 7b888188ab..e6cc388fb8 100644 --- a/test/media/region_observer_unit.js +++ b/test/media/region_observer_unit.js @@ -15,6 +15,9 @@ describe('RegionObserver', () => { /** @type {!shaka.media.RegionObserver} */ let observer; + /** @type {!jasmine.Spy} */ + let onSeekRange; + /** @type {!jasmine.Spy} */ let onEnterRegion; /** @type {!jasmine.Spy} */ @@ -23,12 +26,15 @@ describe('RegionObserver', () => { let onSkipRegion; beforeEach(() => { + onSeekRange = jasmine.createSpy('onSeekRange'); + onSeekRange.and.returnValue({start: 0, end: 100}); + onEnterRegion = jasmine.createSpy('onEnterRegion'); onExitRegion = jasmine.createSpy('onExitRegion'); onSkipRegion = jasmine.createSpy('onSkipRegion'); timeline = new shaka.media.RegionTimeline( - () => { return {start: 0, end: 100}; }); + shaka.test.Util.spyFunc(onSeekRange)); observer = new shaka.media.RegionObserver(timeline); observer.addEventListener('enter', (event) => { @@ -318,6 +324,30 @@ describe('RegionObserver', () => { expect(onSkipRegion).not.toHaveBeenCalled(); }); + // See https://github.com/google/shaka-player/issues/3949 + it('cleans up references to regions', async () => { + const region = createRegion('my-region', 0, 10); + timeline.addRegion(region); + + // Poll so that RegionObserver will store regions. + poll(observer, + /* timeInSeconds= */ 5, + /* seeking= */ false); + + // Hack to get a private member without the compiler complaining: + /** @type {Map} */ + const regionMap = (/** @type {?} */(observer))['oldPosition_']; + expect(regionMap.size).not.toBe(0); + + // Move the seek range so that the region will be removed. + onSeekRange.and.returnValue({start: 20, end: 100}); + // Give the timeline time to filter regions + await shaka.test.Util.delay( + shaka.media.RegionTimeline.REGION_FILTER_INTERVAL * 2); + + expect(regionMap.size).toBe(0); + }); + /** * @param {string} id * @param {number} startTimeSeconds diff --git a/test/media/region_timeline_unit.js b/test/media/region_timeline_unit.js index fbff7cafb4..ceb7c4e17a 100644 --- a/test/media/region_timeline_unit.js +++ b/test/media/region_timeline_unit.js @@ -14,20 +14,27 @@ describe('RegionTimeline', () => { /** @type {!jasmine.Spy} */ let onNewRegion; + /** @type {!jasmine.Spy} */ + let onRemoveRegion; + /** @type {!jasmine.Spy} */ let onSeekRange; beforeEach(() => { - onNewRegion = jasmine.createSpy('onNewRegion'); - onSeekRange = jasmine.createSpy('onSeekRange'); onSeekRange.and.returnValue({start: 0, end: 100}); - timeline = new shaka.media.RegionTimeline( shaka.test.Util.spyFunc(onSeekRange)); + + onNewRegion = jasmine.createSpy('onNewRegion'); timeline.addEventListener('regionadd', (event) => { shaka.test.Util.spyFunc(onNewRegion)(event['region']); }); + + onRemoveRegion = jasmine.createSpy('onRemoveRegion'); + timeline.addEventListener('regionremove', (event) => { + shaka.test.Util.spyFunc(onRemoveRegion)(event['region']); + }); }); afterEach(() => { @@ -87,24 +94,33 @@ describe('RegionTimeline', () => { ]); }); - it('filters out regions that end before the start of the seek range', - async () => { - onSeekRange.and.returnValue({start: 5, end: 100}); - const region1 = createRegion('urn:foo', 'my-region', 0, 3); - const region2 = createRegion('urn:foo', 'my-region', 3, 10); - const region3 = createRegion('urn:foo', 'my-region', 5, 10); - timeline.addRegion(region1); - timeline.addRegion(region2); - timeline.addRegion(region3); - expect(onNewRegion).toHaveBeenCalledTimes(3); - let regions = Array.from(timeline.regions()); - expect(regions.length).toBe(3); - // Give the timeline time to filter regions - await shaka.test.Util.delay( - shaka.media.RegionTimeline.REGION_FILTER_INTERVAL * 2); - regions = Array.from(timeline.regions()); - expect(regions).toEqual([region2, region3]); - }); + it('filters regions that end before the seek range', async () => { + onSeekRange.and.returnValue({start: 5, end: 100}); + + const region1 = createRegion('urn:foo', 'my-region', 0, 3); + const region2 = createRegion('urn:foo', 'my-region', 3, 10); + const region3 = createRegion('urn:foo', 'my-region', 5, 10); + + timeline.addRegion(region1); + timeline.addRegion(region2); + timeline.addRegion(region3); + expect(onNewRegion).toHaveBeenCalledTimes(3); + expect(onNewRegion).toHaveBeenCalledWith(region1); + expect(onNewRegion).toHaveBeenCalledWith(region2); + expect(onNewRegion).toHaveBeenCalledWith(region3); + + let regions = Array.from(timeline.regions()); + expect(regions.length).toBe(3); + + // Give the timeline time to filter regions + await shaka.test.Util.delay( + shaka.media.RegionTimeline.REGION_FILTER_INTERVAL * 2); + + regions = Array.from(timeline.regions()); + expect(regions).toEqual([region2, region3]); + + expect(onRemoveRegion).toHaveBeenCalledWith(region1); + }); /** * @param {string} schemeIdUri