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

Honor go live config audio channels property #10855

Conversation

palana
Copy link
Contributor

@palana palana commented Jun 14, 2024

Description

Changes libobs to allow setting per (audio) encoder speaker layout, and updates the multitrack video output to configure its audio encoders based on the received manifest

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?

Twitch Enhanced Broadcasting beta (since v22)

Types of changes

  • New feature (non-breaking change which adds functionality)
  • 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.

@RytoEX RytoEX requested review from tt2468, derrod and RytoEX June 14, 2024 17:42
@RytoEX RytoEX self-assigned this Jun 14, 2024
@RytoEX RytoEX added the Enhancement Improvement to existing functionality label Jun 14, 2024
@RytoEX RytoEX added this to the OBS Studio 30.2 milestone Jun 14, 2024
docs/sphinx/reference-encoders.rst Outdated Show resolved Hide resolved
libobs/obs-encoder.c Outdated Show resolved Hide resolved
libobs/obs.h Outdated Show resolved Hide resolved
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.

This seems like a pretty big change that I'm not super comfortable making at this stage of the beta. And it's implicit behaviour that's also not really visible to the user.

For now I'd rather we just error out if the configured number of channels on the client side does not match what is expected from the encoder configuration in the JSON response.

We can then revisit this for 31.0. I imagine not too many people will run into this anyway.

@tt2468
Copy link
Member

tt2468 commented Jun 14, 2024

I agree with @derrod that given how late in the stage we are, this should hold off until a later version. I also think that something like this may be better implemented as a generic "audio conversion" mode like the obs_output_set_audio_conversion() feature for raw outputs.

@palana
Copy link
Contributor Author

palana commented Jun 17, 2024

This seems like a pretty big change that I'm not super comfortable making at this stage of the beta. And it's implicit behaviour that's also not really visible to the user.

For now I'd rather we just error out if the configured number of channels on the client side does not match what is expected from the encoder configuration in the JSON response.

We can then revisit this for 31.0. I imagine not too many people will run into this anyway.

@derrod: we could probably make this fairly visible via the "incompatible settings" dialog even; will discuss internally how we want to handle this

I also think that something like this may be better implemented as a generic "audio conversion" mode like the obs_output_set_audio_conversion() feature for raw outputs.

@tt2468: possibly, though neither audio nor video encoders currently have an outside API for specifying a full audio/video conversion. hard to say if this should change since validation of a full conversion parameter set is somewhat more involved than just validating single parameters, though the underlying mechanism (new "fake" video_t/audio_t) probably has to stay the same to at least allow (binary) api compatibility/transparency

@palana
Copy link
Contributor Author

palana commented Jun 18, 2024

@derrod: opened #10874 which goes a bit beyond "showing an error message", but should be a smaller set of changes overall

@palana palana force-pushed the ruwen/audio-encoder-channel-resampling-upstream branch from b440147 to a0f5acb Compare July 16, 2024 12:56
@RytoEX
Copy link
Member

RytoEX commented Jul 29, 2024

@derrod Time to revisit?

@palana palana force-pushed the ruwen/audio-encoder-channel-resampling-upstream branch from a0f5acb to a3295be Compare August 1, 2024 11:22
@RytoEX
Copy link
Member

RytoEX commented Aug 7, 2024

@derrod Any remaining feedback on this?

@derrod
Copy link
Member

derrod commented Aug 8, 2024

I'm personally still not a fan of silently changing this for the user, but implementation wise it looks fine.

@RytoEX
Copy link
Member

RytoEX commented Aug 8, 2024

I'm personally still not a fan of silently changing this for the user, but implementation wise it looks fine.

As far as I can tell, this is only changed on the stream output and only if Multitrack Video is being used, which reduces the impact a bit, but I do see your point.

It looks like there is a logged message that the channels are being set. Would you prefer some indication in the UI that audio channels on the stream output may change? Or is your issue that you do not think the stream output audio channels should change from the user setting at all?

@derrod
Copy link
Member

derrod commented Aug 8, 2024

It looks like there is a logged message that the channels are being set. Would you prefer some indication in the UI that audio channels on the stream output may change? Or is your issue that you do not think the stream output audio channels should change from the user setting at all?

I think we should tell the user to fix their settings (i.e. what we do now), rather than silently changing them. This is also the case with other incompatible settings such as 120 FPS, even though similarly we could use the FPS divisor feature to automatically fix that.

There could be an argument for being able to record 5.1 while streaming in stereo, but I think that that should be implemented similarly to how output scaling works, where it is an explicit setting.

@RytoEX
Copy link
Member

RytoEX commented Aug 8, 2024

Might need @Warchamp7 to weigh in on the UX concern here.

@tt2468
Copy link
Member

tt2468 commented Aug 10, 2024

I agree that if there is existing behavior that can cause TEB to throw a start error on incompatible settings, that this should do that too, rather than "fixing" the setting behind the scenes. And I still stand behind the idea that an encoder audio conversion system backed by swresample would prove more versatile and useful now and down the line. A dialog could, in theory, give the option to "Use Recommended (Stereo)" and then turn on such a system.

@palana
Copy link
Contributor Author

palana commented Aug 12, 2024

It looks like there is a logged message that the channels are being set. Would you prefer some indication in the UI that audio channels on the stream output may change? Or is your issue that you do not think the stream output audio channels should change from the user setting at all?

I think we should tell the user to fix their settings (i.e. what we do now), rather than silently changing them. This is also the case with other incompatible settings such as 120 FPS, even though similarly we could use the FPS divisor feature to automatically fix that.

we'd like to make get client configuration aware of when users see the current message, will file a follow up/alternative PR if that's the consensus (rather than automatic resampling)

@Warchamp7
Copy link
Member

It looks like there is a logged message that the channels are being set. Would you prefer some indication in the UI that audio channels on the stream output may change? Or is your issue that you do not think the stream output audio channels should change from the user setting at all?

I think we should tell the user to fix their settings (i.e. what we do now), rather than silently changing them. This is also the case with other incompatible settings such as 120 FPS, even though similarly we could use the FPS divisor feature to automatically fix that.

There could be an argument for being able to record 5.1 while streaming in stereo, but I think that that should be implemented similarly to how output scaling works, where it is an explicit setting.

I agree with Rodney here on pretty much all points. I do not like silently changing settings and I do not like creating inconsistency in behaviour when it comes to invalid settings.

@palana
Copy link
Contributor Author

palana commented Aug 16, 2024

It looks like there is a logged message that the channels are being set. Would you prefer some indication in the UI that audio channels on the stream output may change? Or is your issue that you do not think the stream output audio channels should change from the user setting at all?

I think we should tell the user to fix their settings (i.e. what we do now), rather than silently changing them. This is also the case with other incompatible settings such as 120 FPS, even though similarly we could use the FPS divisor feature to automatically fix that.
There could be an argument for being able to record 5.1 while streaming in stereo, but I think that that should be implemented similarly to how output scaling works, where it is an explicit setting.

I agree with Rodney here on pretty much all points. I do not like silently changing settings and I do not like creating inconsistency in behaviour when it comes to invalid settings.

@Warchamp7, @derrod, @RytoEX: I've filed #11141 as a tweak to current behavior

@palana palana closed this Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants