Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WinSystems: Use EqualsNoCase which is wanted for identifying sinks #13171

Merged
merged 1 commit into from
Dec 11, 2017

Conversation

fritsch
Copy link
Member

@fritsch fritsch commented Dec 11, 2017

evil and madness will happen

@fritsch
Copy link
Member Author

fritsch commented Dec 11, 2017

Added the others - mmh - that code duplication really sucks :-)

@fritsch fritsch changed the title WinSystemX11GLContext: Don't pass empty string into CompareNoCase WinSystem: Properly use CompareNoCase Dec 11, 2017
@garbear
Copy link
Member

garbear commented Dec 11, 2017

Better would be StringUtils::EqualsNoCase()?

@fritsch
Copy link
Member Author

fritsch commented Dec 11, 2017

@garbear thx very much - yes! That's better.

@fritsch fritsch changed the title WinSystem: Properly use CompareNoCase WinSystems: Use EqualsNoCase which is wanted for identifying sinks Dec 11, 2017
@fritsch
Copy link
Member Author

fritsch commented Dec 11, 2017

jenkins build and merge

@FernetMenta
Copy link
Contributor

thanks.
jenkins seems to have his mind somewhere else these days, so I merge in behalf of him

@FernetMenta FernetMenta merged commit bc44908 into xbmc:master Dec 11, 2017
@Rechi Rechi added Component: Audio Type: Fix non-breaking change which fixes an issue Platform: Linux v18 Leia labels Dec 11, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Dec 11, 2017
@MilhouseVH
Copy link
Contributor

MilhouseVH commented Dec 12, 2017

After this PR, most audio devices are no longer listed/available in Linux.

Kodi on Ubuntu 17.10 (Radeon HD graphics card):
Without this PR: http://sprunge.us/RKLb
WITH this PR: http://sprunge.us/KiFj (devices 3-11 missing)

Without PR:
s1

With PR:
s2

Same with LibreELEC (Skylake NUC):
Without this PR: http://sprunge.us/OLFi
WITH this PR: http://sprunge.us/TXEe

@FernetMenta
Copy link
Contributor

I don't see an issue here. With this PR audio defaults to PulseAudio (which is correct), before it was using ALSA. If you want to use ALSA, you have to set AE_SINK=ALSA

@MilhouseVH
Copy link
Contributor

So Kodi was using ALSA before, which is the mistake that this PR corrects? If so no problem, but this seems to be a change in default behaviour (whether intentional or not).

@FernetMenta
Copy link
Contributor

The default behaviour before the factory cleanup was trying PA first.

@MilhouseVH
Copy link
Contributor

Not sure what Kodi was (or should have) been doing, but I'm building Kodi for Ubuntu without any build options (ie. everything default, no additional patches or custom configuration) and until this PR Kodi would find all my audio devices, now after this PR it doesn't.

@MilhouseVH
Copy link
Contributor

MilhouseVH commented Dec 12, 2017

Maybe it needs StartsWith instead of Equals?

@FernetMenta
Copy link
Contributor

and until this PR Kodi would find all my audio devices, now after this PR it doesn't.

this is not correct. It does find the PA devices.

@MilhouseVH
Copy link
Contributor

See pictures I posted. There is unquestionably a change in default behaviour caused by this PR.

@FernetMenta
Copy link
Contributor

anyway, this has ever been intended behaviour.

@fritsch
Copy link
Member Author

fritsch commented Dec 12, 2017

@MilhouseVH yes - finding ONLY pulseaudio devices is exactly what we want. This is the behaviour e.g. v17 and all other v18 builds have before the "factory changes". Reason is, that pulse and ALSA provide the same devices but in different form.

As you see in your screenshot, before this fix here you did NOT have any PULSE devices at all (only the ALSA wrappers). Btw. don't mix that with LibreELEC - here PULSE / ALSA is patched so that the BT pulse device is added on top.

@MilhouseVH
Copy link
Contributor

Thanks @fritsch, my concern was only that there seemed to be a change in default behaviour, but as I don't remember how Kodi 17 behaved it sounds like this is restoring the original default behaviour! I guess for Ubuntu I'll now set the AE_SINK=ALSA environment variable.

Currently in LE we've dropped the ALSA/Pulse patch due to these recent AE changes (last couple of releases) so LE currently behaves the same as plain Linux. We'll reintroduce the patch shortly, however, which hopefully restores the behaviour we require.

@FernetMenta
Copy link
Contributor

I guess for Ubuntu I'll now set the AE_SINK=ALSA environment variable.

This only makes sense if you disable PA while running Kodi. If PA is running ALSA gets routed through the PA server which is crap.

@fritsch
Copy link
Member Author

fritsch commented Dec 12, 2017

That's only true for the default / pulse device, which is a pseudo alsa device - for an exclusive device the big fight for who owns the device starts and will end bad :-). Imagine we suspend the device, cause of idleing - pulseaudio takes it - and boom, we don't get it back. Or on startup pulse already has it, we cannot open it and so on.

But I am also interested why you need AE_SINK=ALSA on a Desktop

@FernetMenta
Copy link
Contributor

That's another reason why we don't register ALSA when PA is active.

@MilhouseVH
Copy link
Contributor

But I am also interested why you need AE_SINK=ALSA on a Desktop

I possibly don't. I don't use this Ubuntu system for Kodi very much (it's my build system) and simply noticed the change of behaviour (which apparently only occurred in the last few days) and because audio devices appeared then disappeared I was no longer sure how my audio had been set up previously (last time I needed to change it was a while ago). So, mucho confusion on my part. PulseAudio seems to be working. I could have sworn in the past I had the HDMI devices listed too, but obviously not.

@fritsch
Copy link
Member Author

fritsch commented Dec 12, 2017

No problem. Thanks for reporting back.

@gismo2004
Copy link

not sure if there is "no" problem.
Im using a intel NUC with kodi-standalone, ubuntu 16.4 and no pulseaudio. for some reason my sounddevice is not shown any more. (thus no sound at all)

root@NUC:~# aplay -l **** List of PLAYBACK Hardware Devices **** card 0: HDMI [HDA Intel HDMI], device 3: HDMI 0 [HDMI 0] Subdevices: 1/1 Subdevice #0: subdevice #0 card 0: HDMI [HDA Intel HDMI], device 7: HDMI 1 [HDMI 1] Subdevices: 1/1 Subdevice #0: subdevice #0 card 1: PCH [HDA Intel PCH], device 0: ALC283 Analog [ALC283 Analog] Subdevices: 1/1 Subdevice #0: subdevice #0
and playing: speaker-test -c 6 -D hw:0,3
is giving me sound as expected.

So something has changed that does not like my setup :-/

any hints?

@fritsch
Copy link
Member Author

fritsch commented Dec 13, 2017

Hint is: No debuglog no issue - and the forum is the place to report ... this is a devspace.

On topic: We seem to miss system.h include and therefore the ifdefs end up negative.

@fritsch
Copy link
Member Author

fritsch commented Dec 13, 2017

Fix is coming here: #13181 - what a side effect of other cleanup :-) quite fragile our system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Audio Platform: Linux Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants