Skip to content

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

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

Closed
Developer-Ecosystem-Engineering wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b994a43
Integrate texture sharing for arm64 CEF
Developer-Ecosystem-Engineering Aug 19, 2021
2b2f9b5
Add support for CEF 4430
Developer-Ecosystem-Engineering Aug 19, 2021
1ba42bf
Break out multiple assignments
Developer-Ecosystem-Engineering Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions browser-app.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,9 @@ void BrowserApp::OnBeforeCommandLineProcessing(
const CefString &, CefRefPtr<CefCommandLine> command_line)
{
if (!shared_texture_available) {
bool enableGPU = command_line->HasSwitch("enable-gpu");
CefString type = command_line->GetSwitchValue("type");

if (!enableGPU && type.empty()) {
if (!gpuCompositing && type.empty()) {
command_line->AppendSwitch("disable-gpu-compositing");
}
}
Expand Down
7 changes: 5 additions & 2 deletions browser-app.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,15 @@ class BrowserApp : public CefApp,
typedef std::map<int, CefRefPtr<CefV8Value>> CallbackMap;

bool shared_texture_available;
bool gpuCompositing;
CallbackMap callbackMap;
int callbackId;

public:
inline BrowserApp(bool shared_texture_available_ = false)
: shared_texture_available(shared_texture_available_)
inline BrowserApp(bool shared_texture_available_ = false,
bool gpuCompositing_ = true)
: shared_texture_available(shared_texture_available_),
gpuCompositing(gpuCompositing_)
{
}

Expand Down
8 changes: 5 additions & 3 deletions obs-browser-plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ static int adapterCount = 0;
static std::wstring deviceId;

bool hwaccel = false;
bool gpuCompositing = true;

/* ========================================================================= */

Expand Down Expand Up @@ -298,7 +299,7 @@ static void BrowserInit(void)
prod_ver << std::to_string(obs_maj) << "." << std::to_string(obs_min)
<< "." << std::to_string(obs_pat);

#if CHROME_VERSION_BUILD >= 4472
#if CHROME_VERSION_BUILD >= 4430
Copy link
Member

@RytoEX RytoEX Sep 17, 2021

Choose a reason for hiding this comment

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

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.

CefString(&settings.user_agent_product) = prod_ver.str();
#else
CefString(&settings.product_version) = prod_ver.str();
Expand Down Expand Up @@ -350,7 +351,7 @@ static void BrowserInit(void)
}
#endif

app = new BrowserApp(tex_sharing_avail);
app = new BrowserApp(tex_sharing_avail, gpuCompositing);

#ifdef _WIN32
CefExecuteProcess(args, app, nullptr);
Expand Down Expand Up @@ -693,7 +694,8 @@ bool obs_module_load(void)

#ifdef SHARED_TEXTURE_SUPPORT_ENABLED
obs_data_t *private_data = obs_get_private_data();
hwaccel = obs_data_get_bool(private_data, "BrowserHWAccel");
gpuCompositing = obs_data_get_bool(private_data, "BrowserHWAccel");
hwaccel = gpuCompositing;

if (hwaccel) {
check_hwaccel_support();
Expand Down
26 changes: 19 additions & 7 deletions obs-browser-source.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,21 @@ bool BrowserSource::CreateBrowser()
if (hwaccel) {
obs_enter_graphics();
tex_sharing_avail = gs_shared_texture_available();
#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
Comment on lines +138 to +141
Copy link
Member

@RytoEX RytoEX Aug 19, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@Developer-Ecosystem-Engineering Developer-Ecosystem-Engineering Aug 19, 2021

Choose a reason for hiding this comment

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

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.

tex_sharing_avail = false;
#endif
Comment on lines +137 to +143
Copy link
Member

@RytoEX RytoEX Sep 17, 2021

Choose a reason for hiding this comment

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

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.

obs_leave_graphics();
}
#else
bool hwaccel = false;
#endif

CefRefPtr<BrowserClient> browserClient = new BrowserClient(
this, hwaccel && tex_sharing_avail, reroute_audio);
this, tex_sharing_avail, reroute_audio);

CefWindowInfo windowInfo;
#if CHROME_VERSION_BUILD < 3071
Expand All @@ -152,7 +159,7 @@ bool BrowserSource::CreateBrowser()
windowInfo.windowless_rendering_enabled = true;

#ifdef SHARED_TEXTURE_SUPPORT_ENABLED
windowInfo.shared_texture_enabled = hwaccel;
windowInfo.shared_texture_enabled = tex_sharing_avail;
#endif

CefBrowserSettings cefBrowserSettings;
Expand All @@ -177,10 +184,10 @@ bool BrowserSource::CreateBrowser()
cefBrowserSettings.default_font_size = 16;
cefBrowserSettings.default_fixed_font_size = 16;

#if ENABLE_LOCAL_FILE_URL_SCHEME
#if ENABLE_LOCAL_FILE_URL_SCHEME && CHROME_VERSION_BUILD < 4430
if (is_local) {
/* Disable web security for file:// URLs to allow
* local content access to remote APIs */
* local content access to remote APIs */
cefBrowserSettings.web_security = STATE_DISABLED;
}
#endif
Expand Down Expand Up @@ -550,9 +557,14 @@ void BrowserSource::Render()

if (texture) {
#ifdef __APPLE__
gs_effect_t *effect =
obs_get_base_effect((hwaccel) ? OBS_EFFECT_DEFAULT_RECT
: OBS_EFFECT_DEFAULT);
bool useDefaultRect = hwaccel;
#if CHROME_VERSION_BUILD >= 4430 && defined(__APPLE__) && defined(__arm64__)
Copy link
Member

@RytoEX RytoEX Sep 17, 2021

Choose a reason for hiding this comment

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

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

useDefaultRect = false;
flip = false;
#endif
gs_effect_t *effect = obs_get_base_effect(
(useDefaultRect) ? OBS_EFFECT_DEFAULT_RECT
: OBS_EFFECT_DEFAULT);
#else
gs_effect_t *effect = obs_get_base_effect(OBS_EFFECT_DEFAULT);
#endif
Expand Down