From 7891a85485ab05d5de72a496637f3b91653b2c60 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 e66e27b813..a4f3ffce18 100644 --- a/lib/dash/dash_parser.js +++ b/lib/dash/dash_parser.js @@ -401,6 +401,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} */ @@ -423,7 +441,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. @@ -442,6 +459,7 @@ shaka.dash.DashParser = class { this.lowLatencyMode_ = this.playerInterface_.isLowLatencyMode(); } } + if (this.lowLatencyMode_) { presentationTimeline.setAvailabilityTimeOffset( this.minTotalAvailabilityTimeOffset_); @@ -455,22 +473,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) { @@ -705,8 +707,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 = @@ -1737,9 +1743,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; @@ -1762,6 +1769,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 5657f177ec..c75ad725a0 100644 --- a/test/dash/dash_parser_live_unit.js +++ b/test/dash/dash_parser_live_unit.js @@ -1289,6 +1289,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 dfeda5468c..3e96dc0688 100644 --- a/test/dash/dash_parser_manifest_unit.js +++ b/test/dash/dash_parser_manifest_unit.js @@ -983,7 +983,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