Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Potential memory leak w/ DASH live streams and inband EventStreams #3949

Closed
joeyparrish opened this issue Feb 14, 2022 · 0 comments · Fixed by #3957
Closed

Potential memory leak w/ DASH live streams and inband EventStreams #3949

joeyparrish opened this issue Feb 14, 2022 · 0 comments · Fixed by #3957
Assignees
@joeyparrish
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone
v3.3

Comments

@joeyparrish
Copy link
Member

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
v3.3.1

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes

Are you using the demo app or your own custom app?
Demo

If custom app, can you reproduce the issue using our demo app?

What browser and OS are you using?
ChromeOS 96

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?

Reported within Google via Cast bug tracker as b/214881485

What configuration are you using? What is the output of player.getConfiguration()?

N/A

What did you do?

Should be reproducible with the following conditions:

  1. DASH live stream
  2. DVR window (timeShiftBufferDepth) smaller than the range of time represented in the manifest
  3. Inband EventStreams

The EventStream contains regions that are outside the DVR window. The parser adds all regions from the manifest to the timeline (RegionTimeline), then a timer in the timeline filters out the regions that are out of the window. But a second level of tracking (RegionObserver) maintains references to the regions after they have left the window. They are added via a second timer, and never deleted.

The lack of coordination between components is part of the problem. (Timer in RegionTimeline to delete from itself, another timer in PlayheadObserverManager to tell RegionObserver to pull from RegionTimeline.) But there is also a messy callback system in RegionTimeline and RegionObserver, when we could be using events.

Using events would allow RegionTimeline to easily signal RegionObserver to release deleted regions. And coordinating the adding and filtering phases of RegionTimeline would prevent the addition of regions outside of the window.

What did you expect to happen?
Regions should be cleaned up.

What actually happened?

Regions are leaked under the conditions above.
@joeyparrish joeyparrish added type: bug Something isn't working correctly priority: P1 Big impact or workaround impractical; resolve before feature release component: DASH The issue involves the MPEG DASH manifest format labels Feb 14, 2022
@joeyparrish joeyparrish self-assigned this Feb 14, 2022
@github-actions github-actions bot added this to the v3.3 milestone Feb 14, 2022
@joeyparrish joeyparrish mentioned this issue Feb 14, 2022
Merged
11 tasks
joeyparrish added a commit that referenced this issue Feb 15, 2022
@joeyparrish
fix: Add explicit release() for FakeEventTarget (#3950)
f1c1585 
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
@joeyparrish joeyparrish mentioned this issue Feb 15, 2022
Merged
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
refactor: Replace callbacks with events (#3953)
e49c849 
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)
@joeyparrish joeyparrish mentioned this issue Feb 16, 2022
Merged
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Add explicit release() for FakeEventTarget (#3950)
1f9dd98 
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
refactor: Replace callbacks with events (#3953)
c17f5a0 
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)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Add explicit release() for FakeEventTarget (#3950)
edfd974 
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
refactor: Replace callbacks with events (#3953)
4f27936 
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)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Add explicit release() for FakeEventTarget (#3950)
6242622 
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
refactor: Replace callbacks with events (#3953)
e21a172 
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)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Fix memory leak in DASH live streams with inband EventStream (#3957
b7f04cb 
)

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)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Fix memory leak in DASH live streams with inband EventStream (#3957
6eb81da 
)

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)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Fix memory leak in DASH live streams with inband EventStream (#3957
7891a85 
)

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)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Add explicit release() for FakeEventTarget (#3950)
9119535 
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
refactor: Replace callbacks with events (#3953)
c6209fd 
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)
joeyparrish added a commit that referenced this issue Feb 16, 2022
@joeyparrish
fix: Fix memory leak in DASH live streams with inband EventStream (#3957
4389aeb 
)

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)
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Apr 17, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@joeyparrish joeyparrish

Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Milestone
v3.3
Development

Successfully merging a pull request may close this issue.

fix: Fix memory leak in DASH live streams with inband EventStream
1 participant
@joeyparrish