[osxsink/iossink] - don't block addpackets if ca didn't do the initial d... #4322

Merged
merged 1 commit into from Mar 7, 2014

Projects

None yet

6 participants

@Memphiz
Team Kodi member

...ata pull - but timeout gracefully for signaling an error condition to the engine.

This should fix a race/deadlock when unplugging cables/switching of amp/tv ...

@Memphiz Memphiz added the Gotham label Mar 3, 2014
@Memphiz
Team Kodi member

batman build this please (this is a jenkins test trigger for merging this pr into the Gotham branch instead of master ...)

@jmarshallnz jmarshallnz commented on an outdated diff Mar 3, 2014
xbmc/cores/AudioEngine/Sinks/AESinkDARWINIOS.cpp
else
- condVar.wait(lock, 900 * frames / m_sampleRate);
+ signaled = condVar.wait(lock, 900 * frames / m_sampleRate);
@jmarshallnz
jmarshallnz Mar 3, 2014

Not sure if we want to signal a failure here - a failure here is not terminal as we're specifically waiting less than the number of frames, which isn't really a problem. At worst we return 0 here.

IMO the INT_MAX should only be done in the !m_started branch.

Another thing is how will spurious wakeups come into it? Are they uncommon enough that we don't need to worry about it? At worse, what happens when we return INT_MAX: Is the sink reinit'd ?

@davilla

spurious wakeups ? I thought we dealt with those many years ago.

@jmarshallnz
Team Kodi member

This is using a ConditionalVariable, not CEvent (so as to not muck around with locks in the CA callbacks) which doesn't take care of wake ups I think.

I doubt it's an issue, but a couple of suggestions would be:
1. Change the timeout to 100ms rather than 500ms.
2. Return write_frames rather than INT_MAX.
3. Rely on AEActiveSink calling us 5 times, thus totalling 500ms before it signals an error.

(This is assuming that returning INT_MAX in the case we get a spurious wakeup is a problem - if not, then fair enough, just return INT_MAX in that case only).

@Memphiz
Team Kodi member

@fritsch or @FernetMenta any comment on spurious wakeup? ^^

@jmarshallnz will change the m_started branch to not signal an error but return write_frames as before.

@FernetMenta
Team Kodi member

the condition variable is slightly abused because there is no predicate to check. In this case spurious wakeups should be considered possible,

@FernetMenta
Team Kodi member

after having discussed this with fritsch I think it is best to add a timer to the sink. reset the timer when data could be delivered to the ringbuffer and signal error (INT_MAX) after time X. probability is low but there may be 5 spurious wakeups in a row.

@Memphiz
Team Kodi member

Updated - doesn't make much sense to me - but that is what i understood from the irc (which was disturbed by a lengthish "no time" by me :( ). Hope this thingy won't result in a bubble gum until i really get the approach here.

@FernetMenta FernetMenta commented on an outdated diff Mar 5, 2014
xbmc/cores/AudioEngine/Sinks/AESinkDARWINIOS.cpp
else
- condVar.wait(lock, 900 * frames / m_sampleRate);
+ signaled = condVar.wait(lock, 900 * frames / m_sampleRate);
@FernetMenta
FernetMenta Mar 5, 2014

I would drop signaled and use the timer here too

@Memphiz
Team Kodi member

jenkins build this please

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Mar 6, 2014
xbmc/cores/AudioEngine/Sinks/AESinkDARWINOSX.cpp
if (!m_started)
- condVar.wait(lock);
- else
- condVar.wait(lock, 900 * frames / m_format.m_sampleRate);
+ timeout = 500;
+
+ // we are using a timer here for beeing sure for timeouts
+ // condvar can be woken spuriously as signaled
+ XbmcThreads::EndTime timer(timeout);
+ condVar.wait(lock, timeout);
+ if (timer.IsTimePast())
+ return INT_MAX;
@jmarshallnz
jmarshallnz Mar 6, 2014

When m_started is true, we're waiting less time than we are feeding data (900 * frames / m_format.m_sampleRate = 0.9*data_in_seconds). This has the potential of timing out even though we could afford to wait a bit longer (it's unlikely, but could happen?) Unless we know that this branch is causing problems, I'd tend to not return an error in that case - we're waiting only 10-11ms in this branch normally. Thus, I'd either increase the time we wait or drop the error if m_started is true.

@Memphiz
Memphiz Mar 6, 2014

yeah it was a suggestion from fernetmenta to use the timer there too ... but we shouldn't fix whats not broken ... i'm with you here ...

@Memphiz Memphiz [osxsink/iossink] - don't block addpackets if ca didn't do the initia…
…l data pull - but timeout gracefully for signaling an error condition to the engine (use a timer for detecting a real timeout as condvar can be signaled by spurious interrupts)
ecd891c
@Memphiz
Team Kodi member

updated

@jmarshallnz
Team Kodi member

jenkins build this please

@jmarshallnz jmarshallnz merged commit 7d1eb2d into xbmc:master Mar 7, 2014

1 check passed

Details default Merged build #321 succeeded in 1 hr 2 min
@Memphiz
Team Kodi member

@jmarshallnz please wait with the gotham backport - i want to wait for user feedback first ... (testbuild in the works)

@Memphiz
Team Kodi member

btw i will take care of proper PR to gotham branch for my pull requests - no need for you to cherry-pick .

@Memphiz
Team Kodi member
@FernetMenta
Team Kodi member

the thread is either blocked in AddPackets, Drain, or Deinitialize. can you add some debug logs at entry and exit of the functions and have the use test again?

@FernetMenta
Team Kodi member

If OSX's audio thread stops reading from the buffer, we loop forever in Drain.:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/AudioEngine/Sinks/AESinkDARWINOSX.cpp#L647

@Memphiz
Team Kodi member

I love you for spotting our bugs so quickly :D

@FernetMenta
Team Kodi member

the condVar is not the issue. bytes never gets 0

@Memphiz
Team Kodi member

Yes i know. I just realized that we use the mutex for condvar.wait and on the other link i posted we use the singlelock for condvar.wait (this is something i just spotted and now i don't know which one is correct or if it doesn't matter at all).

The real fix is here:

Memphiz@593c763

Doing a test build and let the user test before doing a PR ...

@FernetMenta
Team Kodi member

you should pass mutex, not the singlelock

@MartijnKaijser MartijnKaijser added this to the H* 14.0-alpha1 milestone Mar 7, 2014
@jmarshallnz jmarshallnz removed the Gotham label Mar 8, 2014
@t-nelson

@Memphiz

@jmarshallnz please wait with the gotham backport - i want to wait for user feedback first ... (testbuild in the works)

So?

@Memphiz Memphiz deleted the Memphiz:noblockaddpackets branch May 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment