[erickskrauch]

Description

This PR reworks UI and frontend implementation in order to draw audio track mix checkboxes/radio buttons dynamically. Here is how it looks when compiled with MAX_AUDIO_MIXES 16:

Motivation and Context

I record gameplay with my friends for further post-production. We are using VBAN Talkie to transmit voice and record it into separate tracks. However, for some multiplayer games, I don't have enough tracks. There is a possible hack to record mono tracks into stereo channels, which would increase the number of separate recordings, but I find this a hack.

I started looking and found that there is a MAX_AUDIO_MIXES constant in the OBS code. By increasing it and compiling the project, I found that the number of tracks increased in only a few places. In most places, there were still 6 tracks.

So I started removing all the hardcode and replacing all the related interfaces with dynamic rendering. I want to make a setting of the number of tracks in OBS itself, so that this value can be changed without recompiling the project. However, I left it for future scope.

Implementation explanation

First of all, I need to say that I've never coded in C++ or Qt on such large projects before. I code in PHP for a living. Be gentle, please 😅

I'm using std::vector rather than static arrays since I'm leaving for future scope the possibility to introduce a setting option that will change the audio tracks count without OBS reassembly. Also, that is the reason why I rely on vector->size() when looping over items.

I had to increase MAX_OUTPUT_AUDIO_ENCODERS variable to 32 (see the explanation in the testing section). I feel like it should be okay because there is allowed 10 video track encodes, but only 6 for audio? Seems like some old constant that limited me from encoding all of the tracks.

How Has This Been Tested?

I have tested it with 1, 2, 8, 16 and 32 tracks. I believe, more than 32 tracks will lead to failure due to the usage of uint32_t for the mixers assignment map. I believe its type can be changed since OBS dropped support for x86 builds, but I need some approval from OBS devs.

I tested recording with simple, advanced simple and advanced ffmpeg modes, with 16 tracks enabled. On my Ryzen 5 3600X, encoding so many tracks doesn't actually raise CPU usage over 5% for the OBS process.

The final file contains all 16 audio tracks with appropriate bitrates and labels, assigned on the advanced audio tracks page.

Types of changes

• Tweak (non-breaking change to improve existing functionality)
• Code cleanup (non-breaking change which makes code smaller or more readable)

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.

[pkviet]

I think that for the vast majority of obs user base, increasing the number of tracks to 32 brings 0 benefit and so is very unreasonable.
The code however cleans up the current one because the hardcoded 6 tracks were really creating redundant code in the UI. So pending further reviewing, I'd be in favour of this vectorization.
But I would not lift the current max to 6 tracks.
Any power user wishing more tracks, is more than welcome to compile their own obs binary by changing the max number to whichever number is desired, or to hire a dev to do so because after this PR it would be trivial.

[pkviet]

Note that you'll be breaking third party plugins due to the API break.

[erickskrauch]

Note that you'll be breaking third party plugins due to the API break.

Why? I tried my best not to change any function signature, listed in .h files. Am I missing something?

[Fenrirthviti]

Because this value currently, is hardcoded and not expected to change. Some plugins/features may expect this hardcoded value, and there is also no handling for backwards compatibility if someone switches between a fork that has a different value. Since this is going from "doesn't change" to "can change" that is considered a breaking change.

[erickskrauch]

Because this value currently, is hardcoded and not expected to change.

I'm sorry, but what is "this value" in your message? The number of tracks on UI? Or the MAX_OUTPUT_AUDIO_ENCODERS value? Or something else? 😅

[Fenrirthviti]

Sorry, "these values" is likely more accurate. OBS expects 6, the API expects 6. You're changing it to be dynamic, not static.

When we say "this an API break" it doesn't mean something is actually broken or not working, it means that something is changing from the API itself that could potentially cause an issue to consumers of said API.

[Fenrirthviti]

Just wanted to follow up on this, we had an internal discussion.

Overall, we like the direction this PR is taking from a code standpoint. It's cleaning up the UI and leaves us in a better and more maintainable state. However, we do not want to increase the number of max channels. As long as this PR remains a code improvement and functions the same at the 6 channels, we feel the proposed code changes are fine.

Further review is still pending, however, as this is just a cursory look and there may be other changes requested. We have quite a backlog, and cannot promise when this will be looked at, but the interest in this direction can at least be affirmed.

[erickskrauch]

However, we do not want to increase the number of max channels. As long as this PR remains a code improvement and functions the same at the 6 channels, we feel the proposed code changes are fine.

At the current stage of implementing my final idea, this change really doesn't make sense. I can revert it, and then raise it again when and if I am allowed to make the functionality of setting the number of audio mixes in the program 🙏

Further review is still pending, however, as this is just a cursory look and there may be other changes requested. We have quite a backlog, and cannot promise when this will be looked at, but the interest in this direction can at least be affirmed.

I just hope it's not later than I myself forget how I figured out the OBS code and made these changes 😅

[Akiyamka]

@Fenrirthviti May I ask, what is the point of limiting the number to 6 channels?

[Fenrirthviti]

As partially explained above, properly supporting more than 6 is not as simple as bumping the number and making some small UI changes. There is a lot to consider for stability, supportability, and reasonable user experience, not to mention plugins and the APIs around OBS itself. The backend systems were not designed to support anything but 6, and have not been tested or validated to do so. It may work, it may not, the point is we are not prepared for that at the moment. Power users who want to increase the number are welcome to try and do so, and anyone compiling OBS themselves understands that they are outside supportability by making code changes. The fact this was defined as a const does not mean it was designed to be a number simple to change, or that it was the only limiting factor. It was set to 6 because that's the max we support. It was not intended to simply be changed.

In any event, I'd prefer to have this conversation elsewhere, and not on this PR. I'd recommend joining the Discord and chatting with us there if you want more information. Thanks for understanding!

[derrod]

The default value here should probably also be adjusted

obs-studio/libobs/obs.c

Lines 2273 to 2275 in 7e894a5

	obs_data_set_default_int(source_data, "mixers", 0x3F);
	mixers = (uint32_t)obs_data_get_int(source_data, "mixers");
	obs_source_set_audio_mixers(source, mixers);

[PatTheMav]

I think this PR is fine as a code cleanup.

Just to earmark this for later, it'd probably be great if each track was properly represented by a container object and all these separate vectors of Qt widgets would actually be encapsulated (because they are invariant) and would interface with the Config object directly.

[erickskrauch]

Just to earmark this for later, it'd probably be great if each track was properly represented by a container object and all these separate vectors of Qt widgets would actually be encapsulated (because they are invariant) and would interface with the Config object directly.

Could you please give me some examples for this? If I understand correctly about encapsulation, you'd like me to make a separate class to manage the checkbox/radio buttons vector. But I don't understand what you mean by "Config object". Maybe somewhere in the codebase there is already an example of the implementation you would like to see? I'll study it and make a similar one.

[PatTheMav]

Just to earmark this for later, it'd probably be great if each track was properly represented by a container object and all these separate vectors of Qt widgets would actually be encapsulated (because they are invariant) and would interface with the Config object directly.

Could you please give me some examples for this? If I understand correctly about encapsulation, you'd like me to make a separate class to manage the checkbox/radio buttons vector. But I don't understand what you mean by "Config object". Maybe somewhere in the codebase there is already an example of the implementation you would like to see? I'll study it and make a similar one.

That change would probably require many other changes in the program first, so that'd be out of scope for this PR. We'd have to do a lot of internal homework first before that change can happen.

[erickskrauch]

This code line (for bitrate/name) is used multiple times across different files. Is there any way right now that I can extract it into some helper function?

std::string trackBitrateConfigKey = "Track" + std::to_string(i + 1) + "Bitrate";

[PatTheMav]

This code line (for bitrate/name) is used multiple times across different files. Is there any way right now that I can extract it into some helper function?

std::string trackBitrateConfigKey = "Track" + std::to_string(i + 1) + "Bitrate";

The main question would be if extracting this into a function has any benefit to the reader (because if the function is simple enough it will probably be inlined by the compiler anyway).

The code effectively evaluates to:

std::basic_string<char> trackBitrateConfigKey = std::operator+(std::operator+("Track", std::to_string(i + 1)), "Bitrate");

Which means you got maybe 6 or more string allocations in there:

• For the conversion of i + 1
• For the conversion of "Track"
• For the concatenated result of the inner std::operator+
• For the conversion of "Bitrate"
• For the concatenated result of the outer std::operator+
• For the copy constructor of trackBitrateConfigKey

If you know the size of "Track" and "Bitrate" (e.g. because you created them as static constexpr std::string_view's) you can get their length at runtime, add them and add 1 more byte for the counter variable, declare trackBitrateConfigKey early and reserve the memory for it.

Then you could do:

namespace {

static constexpr std::string_view trackTemplate = "Track";
static constexpr std::string_view bitrateTemplate = "Bitrate";

std::string buildBitrateConfigKey(unsigned int trackKey)
{
    std::string configKey;
    configKey.reserve(trackTemplate.count() + bitrateTemplate.count() + 1);
    configKey.append(trackTemplate).append(std::to_string(i + 1).append(bitrateTemplate);

    return configKey;
}
}

[...]

    config_set_default_uint(activeConfiguration, "AdvOut", buildBitrateConfigKey(i + 1).c_str(), 160);

Now C++ can break in mysterious ways, but I believe what should happen here is that C++ will use copy-elision and RVO (return value optimisation), meaning that only 2 string allocations take place:
* The temporary string used as prvalue for the call to config_set_default_uint
* The temporary string used by std::to_string

RVO means that the compiler understands that the string declared inside the function is only ever used outside of it, so the code inside the function will actually work directly on the temporary string created for the C function call.

So you get readable code, as little memory allocations as possible, and type safety.

---

What would be even better is if the key would not be Track1Bitrate but TrackBitrate_01. That way you would only need a single template string TrackBitrate_ and would just need to add the zero-padded number, but alas..

[erickskrauch]

Oh, wow, thank you for such a detailed and deep explanation! And it also reminded me that I have to finish this PR 😅

What would be even better is if the key would not be Track1Bitrate but TrackBitrate_01. That way you would only need a single template string TrackBitrate_ and would just need to add the zero-padded number, but alas..

I like the idea, but it will require some config migration. And also that zero-padded implementation. Is it worth it?

[PatTheMav]

Oh, wow, thank you for such a detailed and deep explanation! And it also reminded me that I have to finish this PR 😅

What would be even better is if the key would not be Track1Bitrate but TrackBitrate_01. That way you would only need a single template string TrackBitrate_ and would just need to add the zero-padded number, but alas..

I like the idea, but it will require some config migration. And also that zero-padded implementation. Is it worth it?

That depends on the probability of audio track count - if OBS is never (ever) gonna have more than 10 audio tracks (0-indexed), then no. If there is even a slight chance that it might have, but a much lesser chance that it will ever have more than 100, then 0-padding makes a lot of sense, because the string then has a fixed length:

• Length of "Track" + length of "Bitrate" + 2

You could then make that value also a static constexpr because the compiler very probably will evaluate the number at compile time and use it as a static value throughout.

---

But as suggested by the little "alas" in my original comment, that's not feasible as that would require introducing it as an entirely new config key and migrating all settings to it and all. That's just me waxing poetic about how this could have been designed if given maybe a few more minutes of time to design the implementation and less on just writing it (the whole "programming is theory building, not text manipulation" thing).

It might be worth it, but I'd once again suggest this to be an entirely separate PR. So feel free to implement the "less perfect" suggestion that still uses Track<Whatever>Bitrate.

[gamerjungexd]

I think it is a bit nitpicking of a change about the name here.
I see quite some users who wishes for this feature to manually increase the audio tracks number aswell as myself.

As far as I look at the code it looks fine and such tiny change should/could be resolved in another PR, to at least get this feature moving forward.
I also see all checks where successful and only someone would be needed to solve the conflicts and merge it.

I write this comment as a notification, so this PR isn't forgotten. @PatTheMav
I would hope that someone could merge this. If I would be allowed if you have not enough time for it, I would do it myself. 😅

I appreciate all of your guys work here and hope it is merged soon.

[erickskrauch]

Hi! I've finally got back to this PR. I've rebased my work on the latest master version and resolved all the open discussions.

Review feedback:

• Tab order (@PatTheMav, @Warchamp7) 7f525f9: the dynamically created track checkboxes and radio buttons now receive the correct tab order. It was a bit complicated for radio groups, but I solved it.
• Anonymous namespace (@PatTheMav) b2166fd: all helpers (including the new SyncRadioTabFocus) were moved to the anonymous namespace as was asked.
• Default mixers value (@derrod) c2c4688: the hardcoded 0x3F default in obs_load_source_type(), and the mask used when migrating sources from versions before 23.2.2, are now derived from MAX_AUDIO_MIXES.
• Config key helper (@PatTheMav) 5890890: added GetTrackBitrateConfigKey() and GetTrackNameConfigKey(). I'm not sure that I'm allowed to create new files so you tell me if this is the good solution or I have to roll it back to manual string building in place.

All the new changes are made in separate commits so you can easily review them and approve them or decide to revert them.