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

frontend: Add audio properties to GetClientConfiguration request #11141

Merged
merged 2 commits into from
Jan 29, 2025
Merged

frontend: Add audio properties to GetClientConfiguration request #11141

merged 2 commits into from
Jan 29, 2025

Conversation

palana
Copy link
Contributor

@palana palana commented Aug 16, 2024

Description

Instead of transparently resampling audio to match GetClientConfiguration's request send information about local audio settings to GetClientConfiguration

Currently this is only checked in OBS Studio after a configuration has already been received, which doesn't allow for potentially different configurations in case e.g. iOS support is not necessary for this particular stream

Motivation and Context

The Twitch iOS app (and/or HLS playback on iOS in general?) requires audio to be stereo or mono; with this change the requested number of channels from the go live config is being honored, so users can still set up their recordings to contain audio in whichever speaker layout they desire, while the format for streamed audio adheres to service requirements

How Has This Been Tested?

Verified audio settings are part of the GetClientConfiguration request

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

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.

@palana palana mentioned this pull request Aug 16, 2024
Closed
6 tasks
@RytoEX RytoEX requested review from Warchamp7, derrod and tt2468 August 16, 2024 17:02
@WizardCM WizardCM added the Enhancement Improvement to existing functionality label Aug 17, 2024
derrod
derrod reviewed Aug 18, 2024
View reviewed changes
Copy link
Member

@derrod derrod left a comment

Choose a reason for hiding this comment

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

Seems mostly fine, but it would probably be better if the buffer size and fixed buffering settings were queries directly from OBS's audio configuration, rather than assumed to be whatever the currently hardcoded values are.

UI/goliveapi-postdata.cpp Outdated Show resolved Hide resolved
@lexano-ivs lexano-ivs force-pushed the ruwen/get-client-config-audio-properties branch from 1da1526 to 3ae6355 Compare August 27, 2024 19:41
@lexano-ivs
Copy link
Contributor

@derrod @tt2468 Please take a look at the commits I just pushed. I added a new function to libobs to get the required audio information and updated the PR to use this new function. If this looks OK, I can squash commits to tidy this up.

@derrod
Copy link
Member

derrod commented Aug 27, 2024

Yeah this seems fine to me.

@RytoEX RytoEX requested a review from derrod August 28, 2024 15:59
@lexano-ivs lexano-ivs force-pushed the ruwen/get-client-config-audio-properties branch from 3ae6355 to 6328a3d Compare August 28, 2024 18:05
@lexano-ivs
Copy link
Contributor

Yeah this seems fine to me.

Thanks @derrod. I've squashed the 3 commits into 2 and pushed the changes. This should be good to go now.

@RytoEX RytoEX self-assigned this Aug 30, 2024
@RytoEX
Copy link
Member

RytoEX commented Sep 6, 2024

I believe @Warchamp7 's concern here, communicated off-thread, is that this only fixes this issue for the Multitrack Video path (Twitch Enhanced Broadcasting), meaning that users who attempt to stream normally without Multitrack Video will still run into this issue with iOS viewers. I'll let him elaborate on his concerns.

@Warchamp7
Copy link
Member

Confirming the above summary from Ryan. This PR specifically adds the ability to query the information from libobs and includes it in the autoconfig POST data, which is fine on it's own. However I want to be sure that the followup to this is not a solution that only applies when using Multitrack Video.

Multitrack Video should leverage a general fix such as services recommendations being updated to support this data, or it can display an error for the user to fix themselves (As in many other cases)

@palana
Copy link
Contributor Author

palana commented Sep 10, 2024

Confirming the above summary from Ryan. This PR specifically adds the ability to query the information from libobs and includes it in the autoconfig POST data, which is fine on it's own. However I want to be sure that the followup to this is not a solution that only applies when using Multitrack Video.

Multitrack Video should leverage a general fix such as services recommendations being updated to support this data, or it can display an error for the user to fix themselves (As in many other cases)

We haven't decided yet on how to followup, but if we make another attempt at resampling we'll try to target both multitrack video and non-multitrack video

@RytoEX
Copy link
Member

RytoEX commented Sep 11, 2024

Confirming the above summary from Ryan. This PR specifically adds the ability to query the information from libobs and includes it in the autoconfig POST data, which is fine on it's own. However I want to be sure that the followup to this is not a solution that only applies when using Multitrack Video.

Multitrack Video should leverage a general fix such as services recommendations being updated to support this data, or it can display an error for the user to fix themselves (As in many other cases)

@Warchamp7 Given that this only adds the data to the POST data, is this acceptable to merge?

@Warchamp7 Warchamp7 self-assigned this Oct 7, 2024
@palana palana force-pushed the ruwen/get-client-config-audio-properties branch from 6328a3d to d7454de Compare October 8, 2024 15:38
lexano-ivs and others added 2 commits January 24, 2025 01:38
@lexano-ivs @dsaedtler
libos: Add obs_get_audio_info2()
28345a0 
The `obs_audio_info2` struct is used in libobs
for resetting audio, however there is a need for
obtaining the additional fields present in the struct
beyond `obs_audio_info`.
@palana @dsaedtler
frontend: Add audio properties to GetClientConfiguration request
cfc5a2f
@dsaedtler dsaedtler force-pushed the ruwen/get-client-config-audio-properties branch from d7454de to cfc5a2f Compare January 24, 2025 00:40
@RytoEX RytoEX changed the title UI: Add audio properties to GetClientConfiguration request frontend: Add audio properties to GetClientConfiguration request Jan 27, 2025
RytoEX
RytoEX approved these changes Jan 27, 2025
View reviewed changes
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Looks okay at a glance.

@RytoEX RytoEX added this to the OBS Studio 31.1 milestone Jan 27, 2025
@RytoEX RytoEX merged commit 9e3d0eb into obsproject:master Jan 29, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lexano-ivs lexano-ivs lexano-ivs left review comments

@Warchamp7 Warchamp7 Warchamp7 approved these changes

@WizardCM WizardCM WizardCM approved these changes

@RytoEX RytoEX RytoEX approved these changes

@tt2468 tt2468 Awaiting requested review from tt2468

@derrod derrod Awaiting requested review from derrod

Assignees

@RytoEX RytoEX

@Warchamp7 Warchamp7

Labels
Enhancement Improvement to existing functionality
Projects
OBS Studio 31.1 PR Considerations
Status: Done
Milestone
OBS Studio 31.1
Development

Successfully merging this pull request may close these issues.
6 participants
@palana @lexano-ivs @derrod @RytoEX @Warchamp7 @WizardCM