Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

AE: Stop playing sounds when going to Suspend for all platforms (fixes #14211) #2511

Merged
merged 1 commit into from

3 participants

Peter Frühberger davilla Geoffrey McRae
Peter Frühberger
Collaborator

Discussed with @DDDamian here: http://trac.xbmc.org/ticket/14211#comment:9

It is currently not tested on windows - so it would be nice if someone could test it.
This changes global behaviour. So all sounds are freed on all systems that use SoftAE before Suspend.

davilla
Collaborator

@DDDamian is our AE windows expert, if he approves, he can hit the green button.

Geoffrey McRae
Collaborator

Looks good to me, but do you realise this does not free sounds at all?, it just removes their buffers from the playing list, they still reside in memory.

Peter Frühberger
Collaborator

Okay, as they don't come in again - we must also really free them.

Geoffrey McRae
Collaborator

this change is worse, you are now just duping code. Why must you free them in suspend? they are kept in RAM so that the latency on playback is kept very low. Suspend is all about stopping the threads and saving CPU, not RAM.

Peter Frühberger
Collaborator

The reason for delete here is, that I don't want to loose them, as they come out of the m_isplaying list - i did not see where they are properly destructed and thought you original comment:

it just removes their buffers from the playing list, they still reside in memory.

wanted to beware us from a memleak, therefore I added the delete. So please clarify.

Geoffrey McRae
Collaborator

Playing sounds is just a pointer to the sound buffer and the index of where it is up to, it is not an allocated buffer. All sounds are pre-loaded and pre-rendered into the sink format by CAESound via CGUI(something). A sink restart reformats these pre-loaded samples to the sink's output format.

Peter Frühberger
Collaborator

Thanks - I read the code, while waiting for your answer - the delete here even harms. Your explanation makes this clear. I will restore the original Code with just the StopAllSounds() - if somebody else reads this: use FreeSound(ss->owner) would really remove them and all references on them in the other relevant structures.

So I won't freesound then and keep it. Thanks again.

Geoffrey McRae
Collaborator

if somebody else reads this: use FreeSound(ss->owner) would really remove them and all references on them in the other relevant structures.

And even then, FreeSound should only be called by the code that created the sound object, you do not want to delete the sound object out from under it's owner (read: double free segfault)

davilla davilla merged commit 2a0c731 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 29, 2013
  1. Peter Frühberger
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 2 deletions.
  1. +3 −2 xbmc/cores/AudioEngine/Engines/SoftAE/SoftAE.cpp
5 xbmc/cores/AudioEngine/Engines/SoftAE/SoftAE.cpp
View
@@ -975,8 +975,10 @@ bool CSoftAE::Suspend()
{
CLog::Log(LOGDEBUG, "CSoftAE::Suspend - Suspending AE processing");
m_isSuspended = true;
+
+ StopAllSounds();
+
CSingleLock streamLock(m_streamLock);
-
for (StreamList::iterator itt = m_playingStreams.begin(); itt != m_playingStreams.end(); ++itt)
{
CSoftAEStream *stream = *itt;
@@ -985,7 +987,6 @@ bool CSoftAE::Suspend()
streamLock.Leave();
#if defined(TARGET_LINUX)
/*workaround sinks not playing sound after resume */
- StopAllSounds();
bool ret = true;
if(m_sink)
{
Something went wrong with that request. Please try again.