[AE/osxsink] - fix possible device changed loop when using passthrough #4975

Merged
merged 4 commits into from Jul 14, 2014

Conversation

Projects
None yet
4 participants
Owner

Memphiz commented Jul 1, 2014

Damnit - that one slipped in when i added the registration to the default output device changed callback.

@jmarshalnz for review - Gotham material sadly.

Owner

Memphiz commented Jul 1, 2014

jenkins build this please

@Memphiz Memphiz added Gotham Fix labels Jul 1, 2014

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Jul 1, 2014

xbmc/cores/AudioEngine/Sinks/AESinkDARWINOSX.cpp
std::string formatString;
CLog::Log(LOGDEBUG, "%s: Selected stream[%u] - id: 0x%04X, Physical Format: %s %s", __FUNCTION__, (unsigned int)0, (unsigned int)outputStream, StreamDescriptionToString(outputFormat, formatString), m_outputBitstream ? "bitstreamed passthrough" : "");
- SetHogMode(passthrough);
+ SetHogMode(passthrough != PassthroughModeNone);
+
+ //in case this is a dedicated passthroughformat
+ //unregister the default output device changed callback
+ //because setting an encoded format might change the default output
+ //device - and we will end up looping with output device change ping pong
+ if (passthrough == PassthroughModeNative)
+ RegisterDefaultOutputDeviceChangedCB(false, this);
+ else
+ RegisterDefaultOutputDeviceChangedCB(true, this);
@jmarshallnz

jmarshallnz Jul 1, 2014

Member

Would setting a non-encoded format after setting an encoded format also change the default output (potentially?)

@Memphiz

Memphiz Jul 1, 2014

Owner

yes ... damn :D this would change the default output device back to what it was before ... need to check again why i didn't see that issue during testing ... (it must be there imo).

@Memphiz

Memphiz Jul 1, 2014

Owner

I didn't see it because once ActiveAE i done playing back it closes the stream which restores the previous streamformat (which is likely unencoded) before the next initialize then reactivates the callback. Though i don't like it really somehow :( - ideas?

@jmarshallnz

jmarshallnz Jul 1, 2014

Member

I guess you need to know when the format is changed from passthrough <-> non-passthrough, and if that is the case, unregister?

It seems that this is a workaround to the real fix though - what is the order of the callbacks being fired, and how are we responding to them that is causing the problem?

@Memphiz

Memphiz Jul 1, 2014

Owner

XBMC opens the device with gui sound format - this saves the old format of the device and sets the gui sound format.

Lets assume the old format is unencoded (anything else would give us trouble anyway as we can't unhog when other processes hogged the device into encoded mode).

Open dts passthrough:

  1. XBMC - Initialize sets encoded format and opens the stream
  2. OSX - default output device is changed to internal because of that because a device with encoded format can't be the default
  3. OSX - fires the callback
  4. XBMC - triggers reenumeration
  5. XBMC (ActiveAE) - closes the stream for enumeration
  6. XBMC (Osxsink) - restores the initial format on close stream to unencoded
  7. OSX switches the default ouput device back to our HDMI
  8. OSX fires the callback
  9. XBMC - triggers reenumeration
  10. XBMC (ActiveAE) - closes the current stream which restores to previous which was encoded (not sure about that - but doesn't really matter) - or it restores to unencoded again
  11. XBMC (ActiveAE) - calls initialize because it still wants to open the passthrough DTS stream
  12. continue at 1
@jmarshallnz

jmarshallnz Jul 1, 2014

Member

Thanks. Is there any possibility we can detect what is going on in 3 and 4? Do we need to re-enumerate if all that is happened is the default output device is changed (as I presume we're not using that device here?)

@Memphiz

Memphiz Jul 1, 2014

Owner

We need to enumerate on default output device change. That is why i added this notification in the first place ;). We could though disable the callback whenever the format is altered by us (basically just disable it during the whole initialize call and during the restore call when closing the stream). Not sure if this helps?

@jmarshallnz

jmarshallnz Jul 1, 2014

Member

Sure, but you don't want the enumeration to occur in this case, right? As that's what is causing the problem?

@Memphiz

Memphiz Jul 1, 2014

Owner

Yep exactly ... only thing i can think of is what i said before ... disable the callback whenever we change the format ... will try this when i get some more time again ...

Memphiz added some commits Jul 1, 2014

@Memphiz Memphiz [AE/osxsink/coreaudio] - split registration for the devicelist change…
…d callback and registration for the default output device changed callback and

move the coreaudio callback registration methods to CCoreAudioDevice and add possibility to suppress the defaultdevicechanged callback for a given time
8168471
@Memphiz Memphiz [AE/osxsink] - open the device before setting hogmode (as sethogmode …
…relies on it!)
3976819
@Memphiz Memphiz [osx/coreaudio] - suppress default output device changed callbacks fo…
…r 2 seconds when setting physical and virtual formats
db138cd
Owner

Memphiz commented Jul 2, 2014

@jmarshallnz how about that approach - it works fine here and is totally inside the coreaudiospecific stuff. Yes alot of statics - but that callback is a singleton in coreaudio - thats why we have it all static here ;) - At least the solution is hidden from the sink which i kinda like.

Member

jmarshallnz commented Jul 2, 2014

It's better in that it's hidden, and if that's the best we can do then fair enough, but it still seems a bit hacky and open to issues if that 2 seconds isn't quite right.

When the default device callback triggers due to encoded/not encoded format setting, don't we know that we're not using the default device anyway? At this point the actual device list hasn't changed, right? It's just been reordered so the device which was default is now something else. So we don't really need to re-enumerate the AE devicelist I wouldn't think - or at least we don't want or need to reset the entire engine as the current setup is fine.

The ideal is that we can distinguish these cases in the callback and not reset AE unnecessarily.

Owner

Memphiz commented Jul 3, 2014

I don't see a way to detect if we are the initiator of the callback or not from inside the callback.

If we are not the one hogging the device and shifting it into encoded mode (but an other app) we really wanna know that there is a new default device - incase we are using the old default device we can then switch to the new default device - else we would just loose audio.

All flagging from our osx sink thread doesn't work because the callback is initiated from apps mainloop - meaning it is async to our "setphysicalformat" and we are already out of our method when that callback is triggered (though 2000ms are plenty of time - its some 100ms after we did the change i guess - when the callback arrives).

Member

FernetMenta commented Jul 3, 2014

does the protection mechanism of ActiveAE notwork? if it receives more than 2 change events in a second it discards the event. this should break the loop.

Owner

Memphiz commented Jul 3, 2014

@FernetMenta i would say there is no protection in effect - see:

https://dl.dropboxusercontent.com/u/30371861/xbmc-ae-reenum-loop.log

Member

FernetMenta commented Jul 3, 2014

@Memphiz I was never able to test this myself: https://github.com/xbmc/xbmc/blob/master/xbmc/cores/AudioEngine/Engines/ActiveAE/ActiveAE.cpp#L451
I have looked into the log it it seems it stays below the threshold of 2 events in a second. One second was just an arbitrary number. You can adjust this to your needs.

Owner

Memphiz commented Jul 3, 2014

@FernetMenta yep i can adapt it and indeed breaks the loop - but it behaves not as good as my approach because for example if i adapt to more then 2 events in 3 seconds it loops 3 seconds before beeing stopped and audio startup is delayed by that time:

21:00:38 T:2956054528 DEBUG: CActiveAE - device change event
21:00:46 T:2956054528 DEBUG: CActiveAE - device change event
21:00:46 T:2956054528 DEBUG: CActiveAE - device change event
21:00:47 T:2956054528 DEBUG: CActiveAE - device change event
21:00:47 T:2956054528 DEBUG: CActiveAE - device change event
21:00:47 T:2956054528 WARNING: CActiveAE - received 3 device change events within 3 second

Of course i could reduce your code to ms precision - but mhhh it fires stupid senseless callbacks which my approach prevent in the first place ...

Member

FernetMenta commented Jul 3, 2014

I did not suggest to drop your change. I was only curious why it got into this endless loop. @fritsch experienced the same problem with the pulse sink and had commented this type of event. Maybe we can find a common solution for all sinks.

Owner

Memphiz commented Jul 3, 2014

Really interesting that pulse reacts similar when kicking a device into encoded mode. Well your code works as intended - its just that the osx reality in this scenario just does a good job to workaround your timings ;)

Owner

Memphiz commented Jul 3, 2014

Well not sure if you have any different idea - the only thing i can think of for some sort of generic solution would be to have the suppress feature as part of the ActiveAE API and let the sink set a suppress time which makes the engine ignore calls to CAEFactory::DeviceChange(); for that time ...

Member

jmarshallnz commented Jul 3, 2014

@Memphiz do we not have access to the current device and/or the current device list when the callback comes in?

Owner

Memphiz commented Jul 3, 2014

yes - but either i am stupid or we are still having a communication problem what the benefit of this fact is? :p

Member

FernetMenta commented Jul 3, 2014

iirc for the pulse sink we tried to unregister the callback for the time of initialization. Don't remember if this worked or not for any reason. If we know that we trigger this event internally we should be able to catch it, no?

Owner

Memphiz commented Jul 3, 2014

No. Initialize start - deregister callback - initialize which triggers the callback - register callback at end of initialisation - callback fires because it is async to our thread ... basically its not ensure that it would fire while we are in initialize - it most likely fires shortly after ...

Member

jmarshallnz commented Jul 3, 2014

Not sure it could be handled in the sink now that I think about it. If the device callback occurs, we want to be able to tell AE about the new devicelist. But, there's no need I think for AE to tear down the sink if it doesn't need to. As I understand it, the event we don't care about is when the device list hasn't changed, rather just which device is default has changed (and we're not using it). i.e. it's like new devices have been added, but we're not using them nor do we plan to use them, so the current sink is still OK to be used, just the device list within AE should be updated to reflect the changes. Does that sound right?

Owner

Memphiz commented Jul 3, 2014

You mean we don't use the default device because in the passthrough device list in the gui there is no default but a dedicated device selected - i think thats what i didn't get till now ...

Member

jmarshallnz commented Jul 3, 2014

I dunno. I thought the problem was the default device changes by the OS and we're not using it at the time?

Is the problem that we're using the default device (i.e. device named "default" in AE) which is actually a pseudonym for one of the other devices, and the OS goes and changes the "default" device so that something else is now labelled "default" in AE?

Owner

Memphiz commented Jul 3, 2014

e.x. default device is HDMI and xbmc has default selected - we now open passthrough - hdmi is kicked into encoded mode - osx changes default device to internal speaker - we keep using hdmi because we initiated the passthrough.(thatbis the plan - the callback needs to be suppressed - or deinit/init cycle has to be suppressed in that case)

scenario 2 - default is hdmi - we are warching a movie without passthrough - plugin headphones - osx changes default device to headphones - callback - xbmc reeunmerates and deinitializes - then initializes the default device which is now headphones...

what makes you think that we are not using the default device - i for myself have it always set to default...

Member

jmarshallnz commented Jul 3, 2014

Yup, that's what I thought. It's all due to the default device being a pseudonym for the real thing (hdmi in your first example). If AE knew that default is really hdmi, then when it received the request to re-enumerate, it'd know that while default is now something else, the current sink is using hdmi anyway, which hasn't changed, so no need to enumerate. It can detect this as the device list is exactly the same as it was before (no new devices, just the "default" is now pointing to something else).

In your second example again default is changed, but now it points to a new device that wasn't there previously, so we do want to switch to it.

Whether this is possible to detect either in AE or in the sink, I dunno :)

Member

FernetMenta commented Jul 4, 2014

iirc the pulse sink fires the event regardless of a change of the default device. AE can't enumerate while a sink is initialized, some sinks have to open the device.

Why don't you just eat the device change event in the sink for those situations? Or do some pre-evaluation and decide it you want to pass it on the AE. AE has to handle this event in a generic way for all sinks.

Owner

Memphiz commented Jul 4, 2014

I think eating the event is what this pull request tries to do. But its not doable without that timer because we don't know when the callback is called exactly (but it is pretty sure that it happens inside of a 2seconds window after setting the format) - as of the log - its always in the same second.

@jmarshallnz last example. default is hdmi - i am listening to unencoded mp3 music - another app opens the hdmi device and puts it in encoded mode. Default device changes to internal speaker - and so does my mp3 music. No new device - just the default device changed - and still a valid use case for the reinit / reenumeration. (you can test this by setting hdmi to the encoded format in audio midi setup while listening to an mp3 in xbmc...

Member

jmarshallnz commented Jul 4, 2014

Righto, I'm convinced :)

jenkins build this please

Owner

Memphiz commented Jul 14, 2014

@jmarshallnz whats with that one - did it hit 13.2? the build error was unrelated iirc

Member

jmarshallnz commented Jul 14, 2014

Doesn't seem to have been merged to master.

jenkins build this please

jmarshallnz merged commit 927ad96 into xbmc:master Jul 14, 2014

1 check failed

default Merged build #1027 failed in 4 min 53 sec
Details

MartijnKaijser added this to the Helix 14.0-alpha1 milestone Jul 16, 2014

Member

jmarshallnz commented Jul 19, 2014

@Memphiz looks like this is completely non-trivial for Gotham. Going to have to give it a pass for 13.2 I think.

Owner

Memphiz commented Jul 20, 2014

it is trivial - only 2 commits from this pr are needed - backport pr incoming...

jmarshallnz removed the Gotham label Aug 1, 2014

Memphiz deleted the Memphiz:osx_fix_devicechanged_recursion branch May 22, 2016

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