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

Add Enhanced Broadcasting support for Linux #11541

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

lexano-ivs
Copy link
Contributor

@lexano-ivs lexano-ivs commented Nov 20, 2024

Description

TEB (Twitch Enhanced Broadcasting) requires the client to gather information about the host, such as which GPU(s) are available, memory sizes, OS version, etc..., and send that information to a server which then responds with a configuration set for the client to use with multitrack encoding. TEB originally supported Windows, and this PR adds support for Linux-based operating systems.

TEB on Linux supports NVIDIA and AMD GPUs, and Intel GPUs as of 2024.12.17. Intel GPUs with Linux seem to work at least with a flatpak build. For NVIDIA GPUs, NVENC is used and the configurations from the server are currently identical between Windows and Linux. For AMD GPUs, AMF is used on Windows and VAAPI is used on Linux; AMF on Linux is not currently supported for TEB. AMF on Linux might be supported in the future.

Motivation and Context

From the client encoding perspective, Enhanced Broadcasting was originally envisioned to support Windows, Linux, and MacOS. This PR addresses the market need to expand support for Linux-based users.

How Has This Been Tested?

The code has been tested in the TEB beta program and also by the primary developer (@lexano-ivs) on a host setup specifically to test TEB on Linux. This host includes:

  • iGPU (Intel)
  • NVIDIA 4060
  • AMD 6650 XT and AMD 7800 XT
  • Intel Arc A770
  • Multi-boot system with Ubuntu 24.04, Linux Mint 22, and EndeavourOS (Arch-based)

The code was tested against multiple configurations, including single & dual dGPU configurations, swapping the primary/secondary GPU between NVIDIA and AMD, and across the 3 Linux operating systems. DEB-based builds, local builds, and flatpak builds were also tested.

Refer to TEB Installation Instructions for additional context.

Types of changes

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

Copy link
Collaborator

@tytan652 tytan652 Nov 20, 2024

Choose a reason for hiding this comment

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

Changing the encoder to fit TEB is really not good at all.
Encoder properties are not API that is common between them, the change should be on TEB side not on the encoder that stands out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think an argument could be made that this makes the VAAPI encoder more consistent with other encoders. Our implementation of other encoders typically try to use strings rather than integers specifically to prevent issues not just with TEB, but similar situations in general where you want to set it programmatically anyway (scripts, frontends, or whatever other cases), or when you want to manually edit config files or use json directly for whatever reason. We should have had it string-based from the beginning (in my humble opinion at least).

The one issue with this change is that settings migration will have to be added for this if we want to do it this way.

So I agree with this change personally, but we need a migration path if we want to change these settings.

I'd probably advocate changing the setting's string name from "profile" to something like "profile_v2" or "profile_str" for migration, and then in the update function where it reads the settings, check to see if new new profile setting exists. If it does not exist, and the old setting exists, then just use that, otherwise use the new setting. At least that's what I would do. It would be simple and easy.

i.e.

int profile = default_val_or_something_here_maybe;

if (!obs_data_has_user_value(settings, "profile_v2") && obs_data_has_user_value(settings, "profile")) {
    profile = obs_data_get_int(settings, "profile);
    // use the integer value
} else {
   const char *profile_str = obs_data_get_string(settings, "profile_v2");
   if (astrcmpi(profile_str, h264_main) == 0) {
       profile = AV_PROFILE_H264_MAIN;
   } else if { // etc
   }
   // parse the string value to integer
}

or something like that. Very rough code here, maybe somebody has a better suggestion but something like that presumably.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is decided/wished to keep those changes, it needs to be split in another PR with a clear motivation (and TEB is not one in my opinion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the latest set of commits, I reverted the VAAPI profile handling to being integer instead of string-based, and converted the incoming profile string to the corresponding integer, in the UI code. I think the VAAPI code should be using strings as well to be consistent with the other APIs. I can open another PR for this as needed.

One related issue I ran into while trying to implement a migration path is in vaapi_device_modified(). The function currently does not have access to the codec type, hence the switch statement is blindly stepping through the profiles for all codecs. Eventually this might fail because the codec profile definitions are not all unique across AVC, HEVC, and AV1.

In short, there is enough work & risk here to make this a separate PR.


static bool get_distribution_info(string &distro, string &version)
{
ifstream file("/etc/os-release");
Copy link
Collaborator

@tytan652 tytan652 Nov 20, 2024

Choose a reason for hiding this comment

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

In a Flatpak context, this file should not be relied on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks tytan652. Is there something else within a flatpak that can be relied upon?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The presence of /.flatpak-info and it's content can be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I'll look into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added parsing of the /.flatpak-info in addition to /etc/os-release.

@WizardCM WizardCM added Linux Affects Linux New Feature New feature or plugin labels Nov 24, 2024
Copy link
Collaborator

@Lain-B Lain-B left a comment

Choose a reason for hiding this comment

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

In addition to my comment on Tytan's comment about VAAPI settings migration, these are the only issues I see so far.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, so for the all of the UI/system-info-posix.cpp changes, there seems to be a bit of a mix between using raw pointers and references.

static void trim_ws(std::string &s)
static bool get_distribution_info(string &distro, string &version)
static bool get_cpu_freq(uint32_t *cpu_freq)
static bool get_drm_card_info(uint8_t *card_idx, struct drm_card_info *dci)
static void adjust_gpu_model(std::string *model)
bool compare_match_strength(const drm_card_info &a, const drm_card_info &b)

I'm not entirely sure why pointers were used in certain cases here like this versus references. In C this would be understandable but in C++ this just adds unnecessarily unsafe code. Might want to fix these (and any other similar cases in here I haven't spotted) to use references.

If you need the ability to pass nullptr (which I don't quite see), that would also make sense, but I would probably try to see if that's absolutely necessary, and if so, to check to see if there's a safer way of passing that (e.g. std::optional).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I've cleaned this up with the latest commits. It's more consistent now and using references instead of pointers.

}
}

bool compare_match_strength(const drm_card_info &a, const drm_card_info &b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless this function is used in a different translation unit, might want to mark it as static.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also recommend checking other newly added non-exported functions elsewhere where this situation might apply, but this is just the one I noticed at first glance.

Copy link
Member

Choose a reason for hiding this comment

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

Unless this function is used in a different translation unit, might want to mark it as static.

Or for better C++-isms, put them into an anonymous (unnamed) namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put almost all functions in an anonymous namespace and dropped "static" for these. It's cleaner from perspective. Thanks for the suggestion, it makes sense.

Add graphics APIs to obtain the GPU driver version and renderer
strings, as well as GDDR memory sizes. The GDDR memory sizes
include the dmem (dedicated memory on the GPU) and smem
(shared CPU memory used by the GPU but resides in the CPU DDR).

The version and renderer strings are needed for identification
purposes, for example enhanced broadcasting used by Twitch, to
associate the GPU used by OBS with the PCIe-based identification
values such as device_id and vendor_id.
`os_get_free_size()` was simply returning 0. For Linux,
implement the free size calculation based on the `sysinfo()`
system call.
`get_rc_mode()` compares the incoming rate control mode string
to the .name member of rc_mode_t, and the table entries are
all upper-case. This caused a crash when the incoming string
is set to "cbr" instead of "CBR". Make the string comparison
case insensitive.
VAAPI encoders deviate from other encoders (e.g. AMF, NVENC) with
the "profile" setting being an integer instead of a string. With
enhanced broadcasting, "profile" is signalled as a string. Convert
the string-based profile to the appropriate integer-based profile
for VAAPI encoders as a workaround, until VAAPI supports string-based
"profile" (if ever).
Enhanced broadcasting requires system information to be gathered
on the client and submitted to the GetClientConfiguration request
in order to obtain a valid response from the server. This commit
adds support for gathering the required information on Linux-based
systems.
distro = "";
version = "";

if (std::filesystem::exists(systemd_file)) {
Copy link
Collaborator

@tytan652 tytan652 Dec 19, 2024

Choose a reason for hiding this comment

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

Wrong priority, Flatpak should be checked first.

Runtime can add an os-release file for compatibility, but has been proven to be an issue (e.g. 6.2 was still showing 5.15 in a lsb-release made for compat) , let's avoid it to happen again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux Affects Linux New Feature New feature or plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants