Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

[AE] Restore navsounds to default soft-suspend mode now used over pre…

…vious AE constant streaming
  • Loading branch information...
commit 27e400f69c83b80c9954125fd6122173f0721b64 1 parent b93fc48
@DDDamian DDDamian authored
View
64 xbmc/cores/AudioEngine/Engines/SoftAE/SoftAE.cpp
@@ -61,6 +61,8 @@ CSoftAE::CSoftAE():
m_running (false ),
m_reOpen (false ),
m_isSuspended (false ),
+ m_softSuspend (false ),
+ m_softSuspendTimer (0 ),
m_sink (NULL ),
m_transcode (false ),
m_rawPassthrough (false ),
@@ -429,7 +431,6 @@ void CSoftAE::InternalOpenSink()
if (!m_rawPassthrough)
{
CSingleLock soundLock(m_soundLock);
- StopAllSounds();
for (SoundList::iterator itt = m_sounds.begin(); itt != m_sounds.end(); ++itt)
(*itt)->Initialize();
}
@@ -452,6 +453,8 @@ void CSoftAE::InternalOpenSink()
m_newStreams.clear();
m_streamsPlaying = !m_playingStreams.empty();
+ m_softSuspend = false;
+
/* notify any event listeners that we are done */
m_reOpen = false;
m_reOpenEvent.Set();
@@ -769,7 +772,6 @@ void CSoftAE::PlaySound(IAESound *sound)
((CSoftAESound*)sound)->GetSampleCount()
};
m_playing_sounds.push_back(ss);
- m_wake.Set();
}
void CSoftAE::FreeSound(IAESound *sound)
@@ -931,27 +933,6 @@ void CSoftAE::Run()
bool hasAudio = false;
while (m_running)
{
- /* idle while in Suspend() state until Resume() called */
- /* idle if nothing to play and user hasn't enabled */
- /* continuous streaming (silent stream) in as.xml */
- while (m_isSuspended ||
- (m_playingStreams.empty() && m_playing_sounds.empty() && !g_advancedSettings.m_streamSilence) &&
- m_running && !m_reOpen)
- {
- if (m_sink)
- {
- /* take the sink lock */
- CExclusiveLock sinkLock(m_sinkLock);
- //m_sink->Drain(); TODO: implement
- m_sink->Deinitialize();
- delete m_sink;
- m_sink = NULL;
- }
- m_wake.WaitMSec(SOFTAE_IDLE_WAIT_MSEC);
- }
-
- m_wake.Reset();
-
bool restart = false;
if ((this->*m_outputStageFn)(hasAudio) > 0)
@@ -974,12 +955,41 @@ void CSoftAE::Run()
restart = true;
}
+ if (m_playingStreams.empty() && m_playing_sounds.empty() && m_streams.empty() &&
+ !m_softSuspend && !g_advancedSettings.m_streamSilence)
@Memphiz Owner
Memphiz added a note

Are you missing m_sounds.empty() here? You check it on line 982 when switching m_softSuspend to false again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ m_softSuspend = true;
+ m_softSuspendTimer = XbmcThreads::SystemClockMillis() + 10000; //10.0 second delay for softSuspend
+ }
+
+ unsigned int curSystemClock = XbmcThreads::SystemClockMillis();
+
+ /* idle while in Suspend() state until Resume() called */
+ /* idle if nothing to play and user hasn't enabled */
+ /* continuous streaming (silent stream) in as.xml */
+ while ((m_isSuspended || (m_softSuspend && (curSystemClock > m_softSuspendTimer))) &&
@theuni Owner
theuni added a note

@DDDamian This has some nasty consequences on my hardware. Constantly de/re-init'ing ALSA leads to high latencies (due to device setup time) and difficult race conditions. This is a very risky default imo. With GUI sounds on, I get a guaranteed crash and smashed stack in less than 20sec.

Do we really need to tear down the sink? In the case where we can softSuspend and silent-stream isn't on, how about we just fall down into the wait event instead? At least for ALSA, the docs and mailing-list suggest that they shouldn't be in exclusive mode for default devices.

@theuni - it sounds like an issue then in the ALSA sink itself. There were two main reasons for doing it this way: 1) WASAPI exclusive is the most widely-used driver out there among the user base and is the highest-quality output, but it takes exclusive control over the audio device by design and 2) to combine the power-savings and device release aspects into something platform-agnostic without a lot of ifdef'ery as this engine is used for everything except Pulse and CA.

I've run with this mode in win32 for several days without issue, which makes me suspect the ALSA sink or deeper into the kernal, and if it runs without issue it should likely be the default mode. Clearly if it's crashing using ALSA a fix there would be ideal. If not and it looks like an inherent issue for that sink/driver then I may have no choice but to either change the default (bad for Android etc) or ifdef for specific platforms.

I know on RPi gimli initially killed the whole engine and thus the sink too which was even less elegant and more likely to cause leaks or stack smashes. As far as the latency issue there might be some effect on the first navsound that wakes the sink - in win32 I handled that by injecting 50ms of silence on the init so the full sound was heard. Thereafter there's no effect on latency.

Is there any useful info from a crash dump? The very easy solution is to ifdef the default for the streamsilence tag to true in advancedsettings for TARGET_LINUX but it doesn't feel like a solution per se.

@theuni Owner
theuni added a note

Setting streamsilence to true wouldn't have the same effect. It's not that we have to always be spewing silence.. the issue is that we constantly destroy/re-create the sink. In ALSA, that means we're constantly grabbing and releasing the device. Imo we should leave the sink alone and just don't pipe data into it.

ALSA is highly async, from what I understand even the creation/destruction is async.. it's just not a nice thing to be doing.

If it's causing otherwise unsolvable issues (memleaks etc) I'd be happy to make this platform dependent, although I was trying to keep SoftAE as universal as possible. Miss having gnif or another Linux dev onboard here. The motivator here for destroying the sink was wasapi which requires release of the context.

Anyone aware of other platforms (Pi, Android, etc) for which this causes issues?

@theuni Owner
theuni added a note

Yes, Android is currently broken as well.

The desired effect here is fine, but we're using a sledgehammer to do it.

How about abstracting it a bit further? Create some new virtuals like SoftSuspend() and SoftResume(), and let the sinks decide what to do with them. Then continue firing null-data at the sink, but it knows that it can sleep, or instantly return.. whatever fits its model better. Seems it'd be far easier to keep tabs on the pipeline if we can isolate the stalls.

In fact, that would bring android more in-line with the others, as it currently implements its own soft-suspend.

I certainly like the abstraction suggested. Davilla was right on this being a can of worms ;) His implementation of the soft suspend is essentially in this code, just without the sink destruction which is causing the issues for non-Win platforms. This is the first I'm hearing of breakage so will make this highest priority.

@theuni Owner
theuni added a note

Thanks. I'm reverting locally and I'll verify that this solves all current problems.

Also, I can help you on the Alsa/Android side, so you're not left to fend for yourself :)

Appreciated ;) it's the Linux side which has been suffering - Memphiz, Davilla and Gimli have been great on keeping OSX, Driod and Pi progressing and now FernetMenta is dipping a toe in so really appreciate your input as always :)

@gnif Collaborator
gnif added a note

@DDDamian - I have been looking into this and the recent patches should resolve the Linux issues with ALSA somewhat. There was a race condition causing the ALSA strangeness, but the issue was not platform dependent and could have exhibited on windows also.

@gnif - great! As mentioned win32 working fine over several days intermittent usage - would be great to hear confirmation that ALSA issues resolved until functionality moved to sinks.

@gnif Collaborator
gnif added a note

@DDDamian - Seems to work fine here since the updates to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ m_running && !m_reOpen && !restart)
+ {
+ if (m_sink)
+ {
+ /* take the sink lock */
+ CExclusiveLock sinkLock(m_sinkLock);
+ //m_sink->Drain(); TODO: implement
+ m_sink->Deinitialize();
+ delete m_sink;
+ m_sink = NULL;
+ }
+ if (!m_playingStreams.empty() || !m_playing_sounds.empty() || m_sounds.empty())
@Memphiz Owner
Memphiz added a note

shoud m_sounds.empty() be !m_sounds.empty() instead? Beside that - are you missing a check to || !m_streams.empty() (you check this in line 958 when enabling the, m_softSuspend flag).

@theuni Owner
theuni added a note

@Memphiz I made that change locally too, I'm quite sure you're correct. unfortunately the destruction of the sink currently makes it impossible to test the before/after behavior.

@arnova Collaborator
arnova added a note

I think I just also stumbled on this one, and fixed it the same way. Made a ticket it for it here: http://trac.xbmc.org/ticket/13508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ m_softSuspend = false;
+ m_wake.WaitMSec(SOFTAE_IDLE_WAIT_MSEC);
+ }
+
/* if we are told to restart */
- if (m_reOpen || restart)
+ if (m_reOpen || restart || !m_sink)
{
CLog::Log(LOGDEBUG, "CSoftAE::Run - Sink restart flagged");
- m_isSuspended = false; // exit Suspend state
InternalOpenSink();
+ m_isSuspended = false; // exit Suspend state
}
}
}
@@ -1000,7 +1010,7 @@ unsigned int CSoftAE::MixSounds(float *buffer, unsigned int samples)
{
// no point doing anything if we have no sounds,
// we do not have to take a lock just to check empty
- if (m_playing_sounds.empty())
+ if (m_playing_sounds.empty() || m_reOpen || !m_sink)
@gnif Collaborator
gnif added a note

The additional checks are not required, if a sound is playing, samples should be taken even if the sink is flagged for re-open or if there is no sink.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return 0;
SoundStateList::iterator itt;
@@ -1042,7 +1052,7 @@ unsigned int CSoftAE::MixSounds(float *buffer, unsigned int samples)
bool CSoftAE::FinalizeSamples(float *buffer, unsigned int samples, bool hasAudio)
{
if (m_soundMode != AE_SOUND_OFF)
- hasAudio |= MixSounds(buffer, samples) > 0;
+ hasAudio |= ((MixSounds(buffer, samples) > 0) || !m_playing_sounds.empty());
@gnif Collaborator
gnif added a note

MixSounds will never return > 0 if m_playing_sounds is empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
/* no need to process if we don't have audio (buffer is memset to 0) */
if (!hasAudio)
View
2  xbmc/cores/AudioEngine/Engines/SoftAE/SoftAE.h
@@ -132,6 +132,8 @@ class CSoftAE : public IThreadedAE
/* internal vars */
bool m_running, m_reOpen, m_isSuspended;
+ bool m_softSuspend; /* latches after last stream or sound played for timer below */
+ unsigned int m_softSuspendTimer; /* time in milliseconds to hold sink open before soft suspend */
CEvent m_reOpenEvent;
CEvent m_wake;
View
3  xbmc/guilib/GUIAudioManager.cpp
@@ -340,8 +340,7 @@ IAESound* CGUIAudioManager::LoadWindowSound(TiXmlNode* pWindowNode, const CStdSt
void CGUIAudioManager::Enable(bool bEnable)
{
// always deinit audio when we don't want gui sounds
- if (g_guiSettings.GetString("lookandfeel.soundskin")=="OFF")
- bEnable = false;
+ bEnable = (g_guiSettings.GetString("lookandfeel.soundskin")=="OFF") ? false : true;
@jmarshallnz Owner

Why bother with the function param at all if you're ignoring it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
CSingleLock lock(m_cs);
m_bEnabled = bEnable;

5 comments on commit 27e400f

@gnif
Collaborator

There is a pretty major bug here that is causing the stack smash... some of the sink re-init logic is not implemented correctly.

When the sink opens, any sounds are re-initialized so that they are re-sampled and down/up-mixed ready for playback. When SoftAE plays a sound, it maintains a list of playing sounds, and the sample it is up to in the buffer of the playing sound. If the channel count or sample rate changes, or the re-sampler decides to create less samples, MixSounds will cause a segfault.

Three possible solutions here:

1) Stop all playing sounds on CSoftAESound re-init
2) Disable GUI sounds completely
3) Revert to the constantly running mode if GUI sounds are enabled.

AE was designed with a constant running mode for a reason, this dirty mode IMO is a horrible hack. XBMC is a HTPC, and is designed to be used as a stand alone program, so having exclusive access 100% of the time is a non-issue. IMO XBMC is not a general desktop media player.

Edit:

An update on this, the stack smashing issues have been fixed as has some of the soft suspend functionality with timeouts during nav sound playback.

@davilla

Constant running mode is a drain on embedded and desktop, the AE run thread runs at a higher priority than rest and it will suck away any idle CPU because the thread will spin if nothing is going on. It simply cannot spin when running at a higher priority.

@gnif
Collaborator

@davilla - Accepted, and happy to work with it, I just don't like it :)

@DDDamian

@gnif - thx for climbing in here! On win it's working fine - the sink is closing gracefully, but it is a sledgehammer approach (although on RPi the whole engine was being killed lol).

There are two factors:
1) CPU drain and battery drain for battery-powered or embedded as davilla suggests and
2) WASAPI exclusive hogs for both external players, roms and background desktop usage (yeah yeah I know ;)

This should address all the above if the sink closes gracefully but I learned at my peril ALSA doesn't. As discussed with TheUni would rather this functionality be moved to the sink via Suspend() functions as it has with the engine, and then we both block gracefully as well as never cycle a hardware buffer with looping audio on some future hardware.

Hoping your fix helps the ALSA side - feel free to cleanup checks you deem unnecessary here - but beware of some odd timing issues simply using m_playingStreams.Empty()/m_playingSounds.Empty() without additional checks - on it's own was not reliable.

Welcome back ;)

@gnif
Collaborator

@DDDamian - The odd issues were the smashed stack due to a buffer overwrite issue during sink teardown/reinit, they are fixed now. ALSA seems to handle the re-open pretty gracefully under my test environment, so I do not see this as being an issue, but I do agree that it should be up to the sink to decide how to do this.

Please sign in to comment.
Something went wrong with that request. Please try again.