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.

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

Fix Sound memory leak, remove Music caching #13461

Merged
merged 1 commit into from Jun 12, 2017

Conversation

Projects
None yet
4 participants
@rob-v
Contributor

rob-v commented Jun 5, 2017

  1. Fix Sound memory leak causing OutOfMemoryException and high RAM usage on each match start
  2. Remove Music caching to avoid unnecessary high RAM usage

Example: 3 times match restart in RA, skipping several music tracks on each start - 327 MB vs 1074 MB:

obrazok

@pchote pchote requested a review from RoosterDragon Jun 5, 2017

@RoosterDragon

Overall approach looks solid here. 👍 after fixups.

Show outdated Hide outdated OpenRA.Game/Sound/Sound.cs
Show outdated Hide outdated OpenRA.Game/Sound/Sound.cs
void ReleaseSound(uint source)
{
AL10.alSourceStop(source);
AL10.alSourcei(source, AL10.AL_BUFFER, 0);

This comment has been minimized.

@RoosterDragon

RoosterDragon Jun 12, 2017

Member

As I read the docs, this breaks the link between the sound and the buffer, but the buffer itself remains alive? If that is true, this doesn't really release anything. Your newly added Dispose on OpenAlSoundSource would actually be freeing the buffers.

If that's true, you could basically delete these new functions, and where you call them just use StopSound or StopAudio if you wanted to just ensure audio was stopped.

If this has another side-effect I'm not aware of, please let me know (maybe add a comment?).

@RoosterDragon

RoosterDragon Jun 12, 2017

Member

As I read the docs, this breaks the link between the sound and the buffer, but the buffer itself remains alive? If that is true, this doesn't really release anything. Your newly added Dispose on OpenAlSoundSource would actually be freeing the buffers.

If that's true, you could basically delete these new functions, and where you call them just use StopSound or StopAudio if you wanted to just ensure audio was stopped.

If this has another side-effect I'm not aware of, please let me know (maybe add a comment?).

This comment has been minimized.

@rob-v

rob-v Jun 12, 2017

Contributor

The buffer remains alive, but releasing it here allows to delete the buffer afterwards.

@rob-v

rob-v Jun 12, 2017

Contributor

The buffer remains alive, but releasing it here allows to delete the buffer afterwards.

This comment has been minimized.

@RoosterDragon

RoosterDragon Jun 12, 2017

Member

Ok, better keep it then :)

@RoosterDragon

RoosterDragon Jun 12, 2017

Member

Ok, better keep it then :)

Show outdated Hide outdated OpenRA.Platforms.Default/OpenAlSoundEngine.cs
Show outdated Hide outdated OpenRA.Platforms.Default/OpenAlSoundEngine.cs

@pchote pchote added this to the Playtest featuring updated HitShapes milestone Jun 12, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 12, 2017

Member

This looks to be a high impact fix, so adding to the playtest milestone.

Member

pchote commented Jun 12, 2017

This looks to be a high impact fix, so adding to the playtest milestone.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 12, 2017

Contributor

Updated.

Contributor

rob-v commented Jun 12, 2017

Updated.

@atlimit8 atlimit8 merged commit 8276b17 into OpenRA:bleed Jun 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8
Member

atlimit8 commented Jun 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment