[AE] Allow sink to decide how or if it can be suspended #1548

Merged
merged 5 commits into from Nov 30, 2012

Conversation

Projects
None yet
6 participants
Contributor

DDDamian commented Oct 5, 2012

Abstract softSuspend/Resume to sink to let it pre-process or decline a Suspend state.

This replaces previous behaviour of destroying the sink on suspend and allows the sink to shitdown gracefully or decline, as well as force a re-init on Resume if required.

Moved suspend/resume code to inline function to clean up SoftAE::Run() and added better comments

Written in light of reported issue with ALSA and Droid sinks on suspend. WASAPI has been implemented here - other platform help requested!

@ghost ghost assigned gnif and theuni Oct 5, 2012

Owner

Memphiz commented Oct 5, 2012

Shitdown gracefully - lol ymmd

Contributor

DDDamian commented Oct 5, 2012

"Shitdown gracefully - lol ymmd"

Lmfao - intentional??

Member

theuni commented Oct 7, 2012

If the sink knows that it is (or should be) suspended, it will know much better what to do when idle.

I'd really like to see this logic dropped out of the engine and handled in the sinks instead. It's far too complicated trying to keep up with all of the possible states here.

Imo it should go like this:

  • Drop the continuous streaming handling in the engine, sink decides. Some platforms can handle this and some can't, don't try to override them.
  • Always fire data at the sink, even if it's silence.
  • Engine has a number of possible states: starting/stopping/playing/idle/etc. Advertise which.
  • Engine fires a public event when going from idle->playing or the reverse.
  • Sink does a engine->event.Wait() if we're suspended.

If done that way, there's no need for any blind waits, there's no wake/sleep latency, and we're sleeping for the exact right time every time.

Am I missing an obvious reason for keeping this stuff in the engine?

Contributor

DDDamian commented Oct 7, 2012

@theuni - thx for the review. I agree with where you're going on the above, especially about it getting complicated in the engine code thus the inlining for that logic.

Sleeping for the exact wait time is ideal (as long as Resume is immediate).

If I have a concern it would be the narrow window we're in. Not sure I can get the above done (nor allow time for other platforms to comply), and I'm doubtful the above would meet the "bug-fix" criteria in feature-freeze.

If it would be acceptable post-merge-window we can skip this and I can put something together more in line with the above. If not this would be the stop-gap to correct the issues you're seeing with ALSA where the engine does not allow the sink to pre-process that state.

Contributor

davilla commented Oct 8, 2012

if this is going in, it needs to go in as is, there's no time left for the prefect solution, I'd have ALSA skip suspend/resume, it does not need it as there is no exclusive mode.

Contributor

DDDamian commented Oct 8, 2012

In which case the returns in the virtual functions are correct - should not need any ALSA sink modification.

I have not tested on an ALSA platform - let alone one showing issues. Gnif reports none on his setup, but clearly some platforms do not allow sink teardown.

@DDDamian DDDamian closed this Oct 8, 2012

Member

theuni commented Oct 8, 2012

Closed on purpose? I agree with the above. It's not ideal, but at least it fixes the main issues. I can take a crack at Linux/Android tonight or tomorrow.

Contributor

davilla commented Oct 11, 2012

Ping @DDDamian

@ghost ghost reopened this Oct 11, 2012

Member

theuni commented Oct 11, 2012

Looks fine for alsa

Member

theuni commented Oct 12, 2012

@DDDamian I've pushed the necessary changes for Alsa to my repo: https://github.com/theuni/xbmc/commits/SoftSuspendSink . You'll want to cherry-pick the top 3 to avoid picking up the merge of your branch. Alsa now works perfectly (on my hardware at least).

Imo we should be setting the default virtual functions to false though, in order to assume that the sinks can't sleep. That would give back the old behavior.

Member

theuni commented Oct 12, 2012

Ugh, hold on that. Alsa pause seems to mean: lock the whole damn device and render it useless otherwise.

Will investigate.

+ @return false if sink cannot be suspended
+ */
+ virtual bool SoftSuspend() {return true;};
+
@arnova

arnova Nov 8, 2012

Member

I think it would be safer if this was to default to false?!

@DDDamian

DDDamian Nov 8, 2012

Contributor

Agree - if sink code not correct (can't be paused/flushed/released) or functionality not provided false is safer. Will update. Same might be true for SoftResume() below.

Still no solutions offered for ALSA-based systems. Gnif can't repro and I'm guessing on ALSA fixes...if no solution found before betas I'll setup a Linux box or solicit a forum tester on that platform. Something's borked in that sink or kernal if we can't even pause output.

@arnova

arnova Nov 8, 2012

Member

I'm running Linux with ALSA so I could perform some tests... But what's not entirely clear is: what's the issue then with ALSA? And does it only occur with this PR or also with mainline? Only thing I've seen with mainline is that XBMC sometimes hangs when restarting within the same X session of which I suspect it's caused by AE.

@davilla

davilla Nov 8, 2012

Contributor

ALSA issues are hidden if you have CPU ponies, these issues are showing up on CPU limited or embedded (arm) platforms.

@arnova

arnova Nov 8, 2012

Member

I'm running it on an Atom N330, so no real pony, right? But you mean high CPU load or race-issues?

@davilla

davilla Nov 8, 2012

Contributor

make that N330 pretend it's a single core with no HT and you should see any issues :)

@DDDamian

DDDamian Nov 8, 2012

Contributor

@davilla, @theuni - you are both seeing the issue - can you describe it for me? Is it just on resume? What actually happens? Is it only with navsounds or streams?

@arnova

arnova Nov 14, 2012

Member

Since I understand the issues are there regardless (both with and without this PR) shouldn't we just merge this PR so it at least solves the other issues?

+
+ /* check if we need to resume for stream or sound */
+ if (!m_isSuspended && (!m_playingStreams.empty() || !m_playing_sounds.empty()))
+ {
@arnova

arnova Nov 8, 2012

Member

Perhaps I'm missing something, but I don't get this logic.

@DDDamian

DDDamian Nov 8, 2012

Contributor

There's two ways to suspend audio processing:

  1. forced Suspend - used by externalplayer.cpp or addons to force release of audio context so external players or system can use audio device, e.g. when launching a YouTube vid in browser or a BD-menu-capable external player. Signified by m_isSuspended == true.

  2. "soft" Suspend - slips into idle mode 10 seconds after last stream or sound played - reduces battery consumption / CPU load and releases audio context as above. Signified by m_softSuspend == true && 10 sec delay elapsed.

This PR just moves the decision on what suspend states (if any) the platform is capable of (and really, we're just pausing output and waiting for something to play!) to the sink itself.

@DDDamian DDDamian referenced this pull request Nov 21, 2012

Closed

AE + Alsa fixes #1820

+ @return false if sink cannot be suspended
+ */
+ virtual bool SoftSuspend() {return false;};
+
@DDDamian

DDDamian Nov 22, 2012

Contributor

Changed now to default to false so sink cannot be suspended as requested. But this does mean that without softSuspend()/softResume() functions in the ALSA sink it will not benefit from softSuspend (idling when nothing playing). It shouldn't need to be anything fancy as ALSA doesn't take exclusive control like WASAPI or CA, but wanted to point that out.

That's why I originally defaulted to true to take advantage of the wait state....

Contributor

davilla commented Nov 23, 2012

this ready to inject ?

Contributor

DDDamian commented Nov 23, 2012

From win32 perspective yes, but it was never aimed at that. Absolutely requires sign-off from ALSA tester - especially see line note in AESink.h

Member

theuni commented Nov 24, 2012

This is causing tons of buffer underruns with ALSA, I'm poking at it now. Can't rule out local error yet, so I'll report back with what I find. But for now, it's a NAK :(

Member

theuni commented Nov 24, 2012

Ok, disregard previous nack. Results are noisy and random, so it's very likely there's no behavioral difference between the two.

Member

theuni commented Nov 24, 2012

I think we need to just shove it in and get it tested on more hardware. ALSA is fickle, we'll never get this right without a bigger sample size.

Contributor

DDDamian commented Nov 24, 2012

@theuni - are you stable with just your fixes in #1820? There's no way I'd recommend pushing this unless that's the case. This is solely for ALSA, except it reduces latency on a re-awake with navsounds after sleep on all platforms.

NAK without ALSA signoff. Gnif says he can't repro on Linux. From my win32 perspective I dont need this minus the slight latency issue mentioned above when a navsound awakes engine.

Really all we're trying to do here is pause ALSA output and restart later. But if we can't pause ALSA and restart I'm not sure where to go with this. Maybe when that Pivos box arrives.

Member

arnova commented Nov 28, 2012

I'm on ALSA so I could test this but I need to know what to look for...

Contributor

davilla commented Nov 28, 2012

Broken pipes, audio thrashing, high cpu, etc. General SoftAE misbehaving. With/without GUI sounds enabled too. Video/Audio startup and seeks.

Contributor

DDDamian commented Nov 29, 2012

@davilla - I see none of the above on win32. As this stands now with the change in the default values of the virtual functions the sink must override the SoftSuspend() function and return true to gain the benefits of the idle time. If this isn't done your CPU usage will rise as only AddPackets() can block. See the line note in AESink.h.

Member

arnova commented Nov 29, 2012

@davilla: All those issues only on low-powered (ARM) systems like RPI/aTV?

Member

theuni commented Nov 29, 2012

Alsa will block in AddPackets. The snd_pcm_wait() blocks (using select() internally) while waiting for free space in the alsa buffer. So it's not sucking up too much cpu time.

@DDDamian From my experience, Alsa simply isn't reliable enough to bring up/down all the time. I would be thrilled if someone proved me wrong here, but I spent tons of time trying to get it to play nice on my various hardware, with nothing to show for it. Until then, we'll need to leave the SoftSuspend disabled there.

Member

theuni commented Nov 29, 2012

So this has my sign-off, we need it for b2. what we have now is much worse than b1 due to the constant create/destroy of the sink.

davilla added a commit that referenced this pull request Nov 30, 2012

Merge pull request #1548 from DDDamian/SoftSuspendSink
[AE] Allow sink to decide how or if it can be suspended

@davilla davilla merged commit f528cd3 into xbmc:master Nov 30, 2012

Contributor

DDDamian commented Nov 30, 2012

@theuni - thx for all the efforts here. We're both flying a little blind. Hoping this provides some resolution or at least a bridge until some further ALSA work addresses those issues.

Member

arnova commented Nov 30, 2012

Nice to have this finally (before b2) and since we don't really need Suspend for ALSA I think not using it there totally makes sense :-)

Contributor

DDDamian commented Nov 30, 2012

Thx Spiff - nice to see you're still around :)

Owner

Memphiz commented Nov 30, 2012

He seems to hide - did you mixup nicks? Or is there something i need to know? ;)

Member

arnova commented Nov 30, 2012

spiff != arnova last time I checked ;-)

Contributor

DDDamian commented Nov 30, 2012

doh! sorry more caffeine (and less git-spam) lol

tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Apr 30, 2015

Don't include players from resources API.
This Fixes a couple of issues where we tried to use players as servers.

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