obs-browser: Groundwork to enable Chrome 4430/arm64/Apple silicon #310

Developer-Ecosystem-Engineering · 2021-08-03T21:26:23Z

Update for custom obs texture sharing support used in CEF
Since these CEF patches are not available on any particularly OBS Project repository, instructions are provided here

PREFIX=/Users/youruser/Documents/cef-4430
mkdir -p $PREFIX
cd $PREFIX
mkdir cef
cd cef
mkdir automate
mkdir chromium_git
git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
export PATH=$PREFIX/cef/depot_tools/:$PATH
cd automate
curl -O https://bitbucket.org/chromiumembedded/cef/raw/master/tools/automate/automate-git.py
cd ../chromium_git
python ../automate/automate-git.py --download-dir=$PREFIX/cef/chromium_git --depot-tools-dir=$PREFIX/cef/depot_tools --no-distrib --no-build --arm64-build --branch=4430
cd cef 
patch -p1  < viz_osr_d3d_4430.patch
cd ../chromium/src/cef/ 
./cef_create_projects.sh
cd ..
mv out/Release_GN_x64 out/Release_GN_arm64
vim out/Release_GN_arm64/args.gn
**edit** target_cpu="arm64"
ninja -C out/Release_GN_arm64 cef
cd cef/tools 
./make_distrib.sh --ninja-build --minimal --arm64-build
cd ../binary_distrib

This should produce "cef_binary_90.6.7+g19ba721+chromium-90.0.4430.212_macosarm64_minimal.zip" which will be used as a part of the OBS Studio build and will also need these OBS Browser changes.

Description

When using CEF 4430 or later, we should use the texture sharing/gpu compositing feature that OBS Browser relies on, otherwise behaves like previously

Motivation and Context

OBS Studio relies on a custom patch to CEF in order to perform certain features in OBS Browser. Until these patches are integrated into an Apple silicon variant of CEF, OBS Studio isn't at feature parity natively on Apple silicon.

How Has This Been Tested?

Tested on M1 iMac and M1 MBP
macOS 11.5.1

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 · 2021-08-05T00:19:55Z

Please split the changes for newer CEF support to a separate commit from the hardware acceleration related changes.

Additionally, please be sure to run clang-format.

Developer-Ecosystem-Engineering · 2021-08-05T18:08:58Z

Please split the changes for newer CEF support to a separate commit from the hardware acceleration related changes.

I'm not sure we follow what you want split, what lines are you wanting as their own PR?

We may need to modify this PR further to scope it to only run against the arm64 CEF variant and exclude x86. Will capture your feedback + clang in that subsequent change thank you.

WizardCM

what lines are you wanting as their own PR?

Not as their own PR, but as a separate commit in this PR. The idea is that each commit should be a single "functional" change, and it should be possible to revert them independently.

I've added a couple comments on the areas that should be their own commit, likely simply titled "Enable basic CEF 4430 support", whereas everything else would be "Enable better texture support on CEF ARM versions" as while these are related to each other, they have a different purpose.

Also, separate note - has this been tested on Windows? If scoping this to ARM removes any effect to Windows, then ignore this question.

Either way, thank you for your efforts. :)

obs-browser-source.cpp

browser-client.cpp

obs-browser-plugin.cpp

NextNextStep · 2021-08-16T19:00:18Z

hi @Developer-Ecosystem-Engineering I'm attempting to build your PR, but I'm unable to find the patch you shared in your instructions: viz_osr_d3d_4430.patch. Has it been uploaded as part of a PR across one of the various OBS repositories?

Developer-Ecosystem-Engineering · 2021-08-17T18:01:42Z

hi @Developer-Ecosystem-Engineering I'm attempting to build your PR, but I'm unable to find the patch you shared in your instructions: viz_osr_d3d_4430.patch. Has it been uploaded as part of a PR across one of the various OBS repositories?

Hi @gnuherdnix, that patch can be found here https://bitbucket.org/chromiumembedded/cef/pull-requests/285, thats not our work but appears to be some custom work to support texture sharing in OBS Studio to improve performance and reduce CPU overhead on x86. It doesn't appear to be technically necessary on Apple silicon, but we are still reviewing some tests and data based on feedback from the team. We will need to rework this PR based on some feedback, that should happen in a bit.

pabloko · 2021-08-17T19:21:05Z

Hi @Developer-Ecosystem-Engineering, this patch is not a performance enhancement, what it really does, is to bring back the OnAcceleratedPaint from CefRenderHandler, wich allows to access a gpu texture instead having to map raw RGBA pixels in memory, but most importantly, allows that each frame can be requested, this is very important for video in order to prevent tearing. In windows it uses DirectX11 shared handles and IOSurface on apple.
Do somebody know if this patch is already on the main windows obs-browser dist?

RytoEX · 2021-08-17T19:29:26Z

Hi @Developer-Ecosystem-Engineering, this patch is not a performance enhancement, what it really does, is to bring back the OnAcceleratedPaint from CefRenderHandler, wich allows to access a gpu texture instead having to map raw RGBA pixels in memory, but most importantly, allows that each frame can be requested, this is very important for video in order to prevent tearing. In windows it uses DirectX11 shared handles and IOSurface on apple.
Do somebody know if this patch is already on the main windows obs-browser dist?

@pabloko
On Windows, we use a prebuilt patched version of CEF 3770 (x64, x86). On macOS, we use a prebuilt patched version of CEF 4183.

For all intents and purposes, to the end user, enabling hardware accelerated browser sources is a performance enhancement because it allows them to have fluid 60 FPS complex browser-based sources and overlays. We're not talking about increasing the efficiency of a loop or function when we use the phrase in this context.

NextNextStep · 2021-08-18T17:34:57Z

@RytoEX out of curiosity, could you point me to where the build infrastructure and patches are stored for the CEF prebuilts used for Mac & Windows? I haven't been able to find them and wanted to try building locally.

@Developer-Ecosystem-Engineering Thanks for the link. Regarding IOSurfaces, in the example at hand where we render onto the IOSurface backed GL texture from the CEF process, and then read from that same texture for rendering to the scene in OBS, are we managing to work with the same pointer to on-device GPU memory and avoid CPU<->GPU memory copies?

My understanding is that IOSurface allows transparent access to the surface data and will copy whether needed from GPU or CPU memory, but I'm wondering if we effectively avoid CPU copies when we are only reading & writing to the GPU memory buffer across the two processes.

RytoEX · 2021-08-18T20:23:55Z

@RytoEX out of curiosity, could you point me to where the build infrastructure and patches are stored for the CEF prebuilts used for Mac & Windows? I haven't been able to find them and wanted to try building locally.

We don't have a build infrastructure for our CEF prebuilts. They were generously built by contributors so that we could host them so that people don't have to rebuild them when building obs-studio or obs-browser.

I don't believe we have any custom CEF patches publicly posted. As @Developer-Ecosystem-Engineering has said, the texture sharing patch we use is that unmerged CEF PR. It's possible that we modified it further to get it to actually work, but I didn't work on it, so I can't speak to the details.

Developer-Ecosystem-Engineering · 2021-08-19T18:02:09Z

clang-format re-run on all the changes
CEF version and HW Accel changes are split into two distinct commits
Removed debug log

RytoEX · 2021-08-19T19:00:41Z

obs-browser-source.cpp

+			// CEF 4430 doesn't seem to work correctly when init'd with shared texture on arm64:
+			//  - we never see OnAcceleratedPaint called
+			//  - everything is black
+			// so we'll disable that for the time being


There was a comment on CEF PR 285 recently about OnPaint was being called instead of OnAcceleratedPaint and they had to disable the sandbox to get this to work.
https://bitbucket.org/chromiumembedded/cef/pull-requests/285/reimplement-shared-texture-support-for-viz#comment-243274341

Interesting, nice find, will take a look! Not sure if you want potential M1 users to take a look at Apple silicon OBS Browser with/without this change. Won't be able to get to it today.

RytoEX

It looks like the first two commits cancel each other out. Please squash your commits so that only the relevant commits remain.

Developer-Ecosystem-Engineering · 2021-08-19T21:10:19Z

It looks like the first two commits cancel each other out. Please squash your commits so that only the relevant commits remain.

Apologies, should be resolved now, didn't mean to close the PR!

RytoEX

Use imperative mood for the commit message subjects.

For example:

Add support for CEF 4430

I can't properly think of a way to rephrase the first commit message subject.

Wrap the commit message bodies at 72 characters. End sentences in the commit message body with periods.

A clarifying point about this:

On Apple silicon its not necessary to use the custom CEF patch which enables texture sharing on x86, to achieve 60 fps. Its wired up but disabled (enabling it has no effect on arm64 && APPLE)

Some users use many browser sources at once, sometimes running heavy workloads (bit cups used to be the common example). Hardware acceleration and texture sharing reduce CPU load overall in addition to achieving 60 FPS, not just aiming to allow one browser source to reach 60 FPS.

obs-browser-plugin.cpp

Developer-Ecosystem-Engineering · 2021-08-19T21:39:52Z

Some users use many browser sources at once, sometimes running heavy workloads (bit cups used to be the common example). Hardware acceleration and texture sharing reduce CPU load overall in addition to achieving 60 FPS, not just aiming to allow one browser source to reach 60 FPS.

Hardware acceleration is still enabled. If you disable it in CEF, you'll definitely notice even on M1.

On Apple silicon its not necessary to use the custom CEF patch which enables texture sharing on x86 to achieve 60 fps. This change hooks it up, but leaves it disabled. Enabling it has no effect on __arm64__ && __APPLE__

Changes to support using 4430 (arm64 support) within OBS Browser.

Set assignment to multiple lines per project guidelines

RytoEX

Heads up, there are merge conflicts.

Am I understanding correctly that this PR's main change is changing the disable-gpu-compositing CEF/Chromium switch from relying on the value of the enable-gpu command line switch to using the value of the "Enable Browser Source Hardware Acceleration" option and preventing failures when using CEF >= 4430?

RytoEX · 2021-09-17T05:47:43Z

obs-browser-plugin.cpp

 		 << "." << std::to_string(obs_pat);

-#if CHROME_VERSION_BUILD >= 4472
+#if CHROME_VERSION_BUILD >= 4430


It looks like this is correcting a mistake in #297. According to the linked CEF commit, this is the correct version. I'd prefer this change as its own commit because "Add support for CEF 4430" only barely applies to this change, and I was staring at this change for far too long wondering why it was being changed and what it had to do with adding support for CEF 4430. Honestly, this could even be PR'd and merged separately, because it's a straightforward and simple fix for a mistake that can be merged separately from this.

RytoEX · 2021-09-17T05:50:01Z

obs-browser-source.cpp

-			obs_get_base_effect((hwaccel) ? OBS_EFFECT_DEFAULT_RECT
-						      : OBS_EFFECT_DEFAULT);
+		bool useDefaultRect = hwaccel;
+#if CHROME_VERSION_BUILD >= 4430 && defined(__APPLE__) && defined(__arm64__)


Does this need defined(__APPLE__) if it's wrapped in an #ifdef __APPLE__ ?

RytoEX · 2021-09-17T06:12:10Z

obs-browser-source.cpp

+#if CHROME_VERSION_BUILD >= 4430 && defined(__APPLE__) && defined(__arm64__)
+			// CEF 4430 doesn't seem to work correctly when init'd with shared texture on arm64:
+			//  - we never see OnAcceleratedPaint called
+			//  - everything is black
+			// so we'll disable that for the time being
+			tex_sharing_avail = false;
+#endif


Just noticed this was within obs_enter_graphics(). Though it is just one extra assignment, we try to do as little possible inside the graphics context as possible. This looks like it can safely be done after obs_leave_graphics(). Or you could rearrange the ifdefs, but it's probably less complicated to just move this one.

RytoEX · 2022-05-03T16:05:25Z

Does this still need to be open?

Developer-Ecosystem-Engineering · 2022-05-03T18:27:05Z

I believe @PatTheMav integrated the spirit of these changes into his PR already and CEF is building 'elsewhere' to be included as a binary payload for arm64 variants of OBS. Would be good for him to confirm he's happy with it and it needs no further iteration.

PatTheMav · 2022-05-03T19:01:33Z

Yeah we've mainly used the CEF build instructions. We still need to revisit the coupling of enabling GPU compositing in CEF and texture sharing (because even with the latter disabled, CEF seems to still do the former on my M1 Mac).

But I'm happy to solve/revisit this in the scope of another PR.

Developer-Ecosystem-Engineering · 2022-05-03T19:30:09Z

Closing then since this was integrated via #290

Lets see what the feedback is on the M1 OBS build w.r.t. CEF and what needs further work, if anything.

Developer-Ecosystem-Engineering mentioned this pull request Aug 3, 2021

CI: Add OBS browser + Apple silicon support obsproject/obs-studio#5063

Closed

6 tasks

WizardCM added Enhancement New feature or improvement Bug Fix Non-breaking change which fixes an issue labels Aug 3, 2021

WizardCM requested changes Aug 7, 2021

View reviewed changes

obs-browser-source.cpp Outdated Show resolved Hide resolved

browser-client.cpp Outdated Show resolved Hide resolved

obs-browser-plugin.cpp Show resolved Hide resolved

RytoEX reviewed Aug 19, 2021

View reviewed changes

RytoEX requested changes Aug 19, 2021

View reviewed changes

Developer-Ecosystem-Engineering closed this Aug 19, 2021

Developer-Ecosystem-Engineering force-pushed the apple-silicon-support-arm64 branch from 97e7368 to c5ba29f Compare August 19, 2021 21:04

Developer-Ecosystem-Engineering reopened this Aug 19, 2021

RytoEX requested changes Aug 19, 2021

View reviewed changes

obs-browser-plugin.cpp Outdated Show resolved Hide resolved

Developer-Ecosystem-Engineering added 2 commits August 19, 2021 15:13

Integrate texture sharing for arm64 CEF

b994a43

On Apple silicon its not necessary to use the custom CEF patch which enables texture sharing on x86 to achieve 60 fps. This change hooks it up, but leaves it disabled. Enabling it has no effect on __arm64__ && __APPLE__

Add support for CEF 4430

2b2f9b5

Changes to support using 4430 (arm64 support) within OBS Browser.

Developer-Ecosystem-Engineering force-pushed the apple-silicon-support-arm64 branch from 75f2391 to 2b2f9b5 Compare August 19, 2021 22:18

Break out multiple assignments

1ba42bf

Set assignment to multiple lines per project guidelines

Developer-Ecosystem-Engineering requested review from RytoEX and WizardCM September 16, 2021 17:55

RytoEX requested changes Sep 17, 2021

View reviewed changes

Developer-Ecosystem-Engineering closed this May 3, 2022

obs-browser: Groundwork to enable Chrome 4430/arm64/Apple silicon #310

obs-browser: Groundwork to enable Chrome 4430/arm64/Apple silicon #310

Uh oh!

Conversation

Developer-Ecosystem-Engineering commented Aug 3, 2021

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

WizardCM commented Aug 5, 2021

Uh oh!

Developer-Ecosystem-Engineering commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WizardCM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NextNextStep commented Aug 16, 2021

Uh oh!

Developer-Ecosystem-Engineering commented Aug 17, 2021

Uh oh!

pabloko commented Aug 17, 2021

Uh oh!

RytoEX commented Aug 17, 2021

Uh oh!

NextNextStep commented Aug 18, 2021

Uh oh!

RytoEX commented Aug 18, 2021

Uh oh!

Developer-Ecosystem-Engineering commented Aug 19, 2021

Uh oh!

RytoEX Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

Developer-Ecosystem-Engineering Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

RytoEX left a comment

Choose a reason for hiding this comment

Uh oh!

Developer-Ecosystem-Engineering commented Aug 19, 2021

Uh oh!

RytoEX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Developer-Ecosystem-Engineering commented Aug 19, 2021

Uh oh!

RytoEX left a comment

Choose a reason for hiding this comment

Uh oh!

RytoEX Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

RytoEX Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

RytoEX Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

RytoEX commented May 3, 2022

Uh oh!

Developer-Ecosystem-Engineering commented May 3, 2022

Uh oh!

PatTheMav commented May 3, 2022

Uh oh!

Developer-Ecosystem-Engineering commented May 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Developer-Ecosystem-Engineering commented Aug 5, 2021 •

edited

Loading