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

UI: Fix crash in Source Toolbar #3996

Merged
merged 1 commit into from
Jan 12, 2021
Merged

Conversation

Chiitoo
Copy link
Contributor

@Chiitoo Chiitoo commented Dec 31, 2020

I guess I might as well push this out here, at least as a draft, for a bit of review and suggestions for improvement.

Description

This adds a check for 'null' in 'mod' and simply 'return' if it's there, to prevent a segmentation fault.

Motivation and Context

This prevents a segmentation fault when, for example, an ALSA source is selected, and adds ALSA devices to the Source Toolbar (this could most likely be done better).

This also fixes the following issue: #3485

How Has This Been Tested?

Tested building and running OBS Studio with all sound backends disabled, as well as with both, or only one of ALSA and PulseAudio enabled on Gentoo Linux.

I don't actually ever use PulseAudio, so more testing with that would be nice. I can only say that I saw more sources available with it installed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@WizardCM WizardCM added Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality labels Dec 31, 2020
@dodgepong
Copy link
Member

These should be two separate pull requests, IMO.

@Chiitoo
Copy link
Contributor Author

Chiitoo commented Dec 31, 2020

I did consider that as well, but, for one, either of them fixes the mentioned issue for me, so I probably couldn't decide which one should be marked as fixing it. :]

If the first commit seems good, the second can be safely dropped, and I'll create another PR for that (if it looks sane enough for that).

Thanks!

@kkartaltepe
Copy link
Collaborator

This makes alsa the default audio monitoring backend which is not appropriate.

@Chiitoo
Copy link
Contributor Author

Chiitoo commented Dec 31, 2020

Right, it seemed to me like that part should do something different, but PulseAudio also seems to work even if ALSA is enabled as well.

That said though, like I mentioned in the initial comment, I don't actually use PulseAudio, so I'm not sure if it actually works (I have one user report about it working though).

It should not mess with any defaults per se, unless I'm missing something (which is entirely possible).

@WizardCM WizardCM added the Linux Affects Linux label Jan 4, 2021
This commit prevents a crash in, for example, Linux configurations
where PulseAudio is disabled, which leads into 'mod' being 'null',
which in turn leads into a segmentation fault when an ALSA source
is selected.

Closes obsproject#3485
@Chiitoo Chiitoo changed the title UI: Fix crash in Source Toolbar and add ALSA support UI: Fix crash in Source Toolbar Jan 4, 2021
@Chiitoo
Copy link
Contributor Author

Chiitoo commented Jan 4, 2021

I split the ALSA support part off since indeed, it might not be ready for a while, and this change alone already prevents the crash (the toolbar just doesn't say anything).

I do wonder a bit if it would make sense to use 'Basic.Settings.Audio.UnknownAudioDevice' there, but not sure it's really worth it.

@Chiitoo Chiitoo marked this pull request as ready for review January 4, 2021 15:56
@kkartaltepe
Copy link
Collaborator

Looks good

@jp9000 jp9000 merged commit 41367bb into obsproject:master Jan 12, 2021
@RytoEX RytoEX added this to the OBS Studio 27.0 milestone Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality Linux Affects Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants