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.

Sign up for GitHub

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

Jump to bottom

CWinSystemRpi: ensure that we register the ALSA sink as well as PiSink is needed for external DACs #13290

Closed
wants to merge 1 commit into from
Closed

CWinSystemRpi: ensure that we register the ALSA sink as well as PiSink is needed for external DACs #13290

wants to merge 1 commit into from

Conversation

samnazarko
Copy link
Contributor

@samnazarko samnazarko commented Jan 2, 2018 •
edited
Loading

Description

It seems that after c0064a9, the AESink is tied to the windowing system. The ALSA sink does not seem to be registered elsewhere on RBP, which means that external DACs (USB and I2S) are not enumerated.

The patch was necessary to re-register ALSA sinks on RBP.

How Has This Been Tested?

Tested in OSMC v18 nightlies for Raspberry Pi with positive reports: https://discourse.osmc.tv/t/testing-kodi-18-leia-builds-for-raspberry-pi/20631/632

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@FernetMenta
Copy link
Contributor

ALSA is optional and can only be registerd if present

@samnazarko
Copy link
Contributor Author

So what is the solution for multiple sinks? Is there a cmake option for this that I have missed? Won't that just give us #ifdef again..

@Rechi
Copy link
Member

Rechi commented Jan 2, 2018

The include and the register have to be surrounded with #ifdef HAS_ALSA, like it is done X11, gbm and wayland.

@samnazarko
Copy link
Contributor Author

OK -- I've added that and updated the PR.

@FernetMenta
Copy link
Contributor

that is ugly ifdeffery now. the other platforms use a OptinalsRegister. Currently every platform has its own but this is supposed to be refactored into a common one for Linux.

@samnazarko
Copy link
Contributor Author

Indeed. I suspect the point of this whole cleanup was to remove the need for #ifdef.

@FernetMenta
Copy link
Contributor

Yes. At the time we did this, there was no xbmc/platform/linux.
The solution is to move windowing/X11/OptinalsReg (I think that has all optionals) to platform/linux
Then drop all platform specific ones and use the common one.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 1, 2018
@Rechi Rechi force-pushed the readd-alsa-pi branch 2 times, most recently from bf2d815 to 7eef23d Compare February 5, 2018 15:42
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 5, 2018
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 6, 2018
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 8, 2018
@nsenica nsenica mentioned this pull request Sep 19, 2018
Merged
13 tasks
@samnazarko
CWinSystemRpi: ensure that we register the ALSA sink as well as PiSink.
7b3c63d 
This is needed for external DACs

Signed-off-by: Sam Nazarko <email@samnazarko.co.uk>
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Sep 25, 2018
@jenkins4kodi
Copy link
Contributor

@samnazarko this needs a rebase

@lrusak
Copy link
Contributor

lrusak commented Sep 25, 2018

closing in favour of #14453

@lrusak lrusak closed this Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FernetMenta FernetMenta Awaiting requested review from FernetMenta

@popcornmix popcornmix Awaiting requested review from popcornmix

@fritsch fritsch Awaiting requested review from fritsch

Assignees
No one assigned
Labels
Rebase needed PR that does not apply/merge cleanly to current base branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@samnazarko @FernetMenta @Rechi @jenkins4kodi @lrusak