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

AESinkPulse: Implement Hotplug monitor #16881

Merged
merged 6 commits into from Nov 13, 2019
Merged

Conversation

fritsch
Copy link
Member

@fritsch fritsch commented Nov 8, 2019

This implements hotplugging and device change functionality one level higher.

Use-Case:
Get device changes for pulseaudio even if kodi is idleing and not outputting audio

Reasoning:
Pulse-Sink is suspended and deinitialized after a longer idle period, especially if keep alive always is not used. While kodi is not outputting on the sink a new device is registered. As the ctx and the sink registration towards pulseaudio is not there - kodi does not get knowledge of this event and the new device won't be listed.

Implementation:
AESinkFactory has function ptrs for a long time to register an infrastructure / device whatever you want to call it. That way a sink can initialize its static DriverMonitor and register it with the SinkFactory. This DriverMonitor has its own pulse context and outlives the Sink itself.

@fritsch
Copy link
Member Author

fritsch commented Nov 8, 2019

Huge thanks to @FernetMenta

PS: I tried to use clang-format as good as possible, curious what I have forgotten.

@Rechi
Copy link
Member

Rechi commented Nov 8, 2019

Please rebase to get rid of the changes from #16851 in your commit.

@fritsch
Copy link
Member Author

fritsch commented Nov 8, 2019

Ha, nice one - an --ammend slipped in.

@fritsch fritsch added the Type: Improvement non-breaking change which improves existing functionality label Nov 8, 2019
@fritsch fritsch added this to the Matrix 19.0-alpha 1 milestone Nov 8, 2019
@fritsch
Copy link
Member Author

fritsch commented Nov 9, 2019

I added some commits. As jenkins does not turn green, I need to gather the stuff here a bit, don't want to maintain more than 10 PRs in parallel.

@fritsch
Copy link
Member Author

fritsch commented Nov 9, 2019

jenkins build this please

@fritsch
Copy link
Member Author

fritsch commented Nov 10, 2019

In preparation of using e.g. ALSA + PULSE in combination, like: ALSA for PT / normal audio, but pulse for network-stream, rtp or airplay - it's not good when a pulse device reenumeration would make entire audio engine stop playing audio via another driver. Therefore a string parameter to identify the Driver is used to only reenumerate the driver actually needing reenumeration.

@fritsch fritsch force-pushed the pulsemonitor branch 3 times, most recently from f0765b4 to 0587e9a Compare November 10, 2019 17:15
@fritsch
Copy link
Member Author

fritsch commented Nov 12, 2019

jenkins build this please

I plan to merge this very soon, as it does not effect UWP specifics - I don't consider this as a blocker.

@fritsch
Copy link
Member Author

fritsch commented Nov 12, 2019

jenkins build this please

@fritsch
Copy link
Member Author

fritsch commented Nov 12, 2019

Mergeing tomorrow if no further remarks.

Rechi
Rechi previously requested changes Nov 12, 2019
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the code guidelines.

xbmc/cores/AudioEngine/Sinks/AESinkPULSE.cpp Outdated Show resolved Hide resolved
xbmc/cores/AudioEngine/Sinks/AESinkPULSE.cpp Outdated Show resolved Hide resolved
xbmc/cores/AudioEngine/Sinks/AESinkPULSE.cpp Outdated Show resolved Hide resolved
xbmc/cores/AudioEngine/Sinks/AESinkPULSE.cpp Outdated Show resolved Hide resolved
xbmc/cores/AudioEngine/Sinks/AESinkPULSE.h Show resolved Hide resolved
xbmc/utils/ActorProtocol.cpp Outdated Show resolved Hide resolved
xbmc/utils/ActorProtocol.cpp Outdated Show resolved Hide resolved
xbmc/utils/ActorProtocol.h Outdated Show resolved Hide resolved
xbmc/utils/ActorProtocol.h Outdated Show resolved Hide resolved
xbmc/utils/ActorProtocol.h Outdated Show resolved Hide resolved
@fritsch fritsch force-pushed the pulsemonitor branch 2 times, most recently from 1bc9290 to 66fb0b3 Compare November 12, 2019 20:32
@fritsch
Copy link
Member Author

fritsch commented Nov 12, 2019

@Rechi can you trigger it again. git clang-format of already existing commits is a PITA as all rebases go wrong - I hope I got most.

Edit: I mean the automated clang-format check

@fritsch
Copy link
Member Author

fritsch commented Nov 12, 2019

jenkins build this please

@fritsch fritsch requested a review from Rechi November 12, 2019 20:47
@fritsch fritsch dismissed Rechi’s stale review November 12, 2019 21:01

I think I fixed everything you asked for.

@fritsch
Copy link
Member Author

fritsch commented Nov 12, 2019

jenkins build this please

@fritsch
Copy link
Member Author

fritsch commented Nov 13, 2019

Nice jenkins seems happy (minus UWP), also there is no clang patch generated.

Okay to merge?

@notspiff notspiff merged commit 7ba1415 into xbmc:master Nov 13, 2019
@Rechi
Copy link
Member

Rechi commented Nov 13, 2019

@notspiff please always set the version label and other labels before or right after merging a PR.

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020
AESinkPulse: Implement Hotplug monitor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement non-breaking change which improves existing functionality v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants