gh-obsproject/obs-browser
Watch
1
0
Fork
You've already forked obs-browser
 0
Code Issues 37 Pull requests 13 Releases 15 Wiki Activity

Add full support for CEF 4638 #323

Merged
PatTheMav merged 5 commits from cef-4638-update into master 2021-11-27 16:04:39 -06:00
Conversation 21 Commits 5 Files changed 9 +172 -49
PatTheMav commented 2021-10-20 08:58:31 -05:00 (Migrated from github.com)

Description

Introduces necessary fixes for compatibility with CEF 4638 due to API changes within CEF.

Note: This PR stays a draft for now as we need a proper solution to the upstream changes regarding web security disabling for local file support.

Motivation and Context

Make obs-browser compatible with CEF 4638 which is required for M1 support.

How Has This Been Tested?

  • Tested on Apple M1 machine

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.
### Description Introduces necessary fixes for compatibility with CEF 4638 due to API changes within CEF. **Note:** This PR stays a draft for now as we need a proper solution to the upstream changes regarding web security disabling for local file support. ### Motivation and Context Make obs-browser compatible with CEF 4638 which is required for M1 support. ### How Has This Been Tested? * Tested on Apple M1 machine ### Types of changes - Bug fix (non-breaking change which fixes an issue) ### Checklist: - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate. - [x] I have included updates to all appropriate documentation.
🚀 2
WizardCM (Migrated from github.com) reviewed 2021-10-20 22:39:58 -05:00
panel/browser-panel.cpp
WizardCM (Migrated from github.com) commented 2021-10-20 22:39:58 -05:00

This will have to be tested on Windows and Linux.

This will have to be tested on Windows and Linux.
PatTheMav (Migrated from github.com) reviewed 2021-10-22 15:18:42 -05:00
panel/browser-panel.cpp
PatTheMav (Migrated from github.com) commented 2021-10-22 15:18:42 -05:00

It should work fine given that this is CEF's reference implementation now (they unified this call across all OS).

It should work fine given that this is CEF's reference implementation now (they unified this call across all OS).
WizardCM (Migrated from github.com) reviewed 2021-10-26 04:10:15 -05:00
panel/browser-panel.cpp
WizardCM (Migrated from github.com) commented 2021-10-26 04:10:15 -05:00

Verified on both. Looks good to me.

Verified on both. Looks good to me.
PatTheMav commented 2021-11-06 13:11:12 -05:00 (Migrated from github.com)

Added RequestHandler and ResourceRequestHandler callbacks to intercept requests made by a browser source. While this allows us to "detect" CORS requests from local files (via the null Origin header), in my tests I've been able to change other headers and even the URL inside OnBeforeResourceLoad but any changes to the Origin header were ignored.

Added `RequestHandler` and `ResourceRequestHandler` callbacks to intercept requests made by a browser source. While this allows us to "detect" CORS requests from local files (via the `null` Origin header), in my tests I've been able to change other headers and even the URL inside `OnBeforeResourceLoad` but any changes to the Origin header were ignored.
PatTheMav commented 2021-11-06 18:39:45 -05:00 (Migrated from github.com)

After discussion with the team, we came to the following conclusion:

  • Loading local media files for playback works as intended
  • Loading external JS files via <script> works as intended
  • CORS requests from servers that enforce strict CORS policies will fail due to null origin for local URLs

As the latter point mirrors behaviour of current browsers, we deem it a "won't fix" coupled with updated user guidance once we update.

After discussion with the team, we came to the following conclusion: * Loading local media files for playback works as intended * Loading external JS files via `<script>` works as intended * CORS requests from servers that enforce strict CORS policies will fail due to `null` origin for local URLs As the latter point mirrors behaviour of current browsers, we deem it a "won't fix" coupled with updated user guidance once we update.
WizardCM (Migrated from github.com) approved these changes 2021-11-06 18:42:55 -05:00
tt2468 commented 2021-11-20 23:58:59 -06:00 (Migrated from github.com)
  • Ubuntu 20.04 (Xorg)
  • This branch
  • Latest master from obs-studio

Issue A: Building on Ubuntu 20.04 with 4280 fails, with a few errors (See Discord)

Issue B: Every instance of a browser source spams this message to the log, and there is no video: [1120/215034.001000:WARNING:task_impl.cc(31)] No task runner for threadId 0

The more browsers that are added, the more spam there is. The browser can be removed, stopping the spam, but it starts up again when you go to close OBS, and requires killing.

OBS functions as expected when no browser stuff is ever loaded.

- Ubuntu 20.04 (Xorg) - This branch - Latest master from `obs-studio` **Issue A**: Building on Ubuntu 20.04 with 4280 fails, with a few errors (See Discord) **Issue B**: Every instance of a browser source spams this message to the log, and there is no video: `[1120/215034.001000:WARNING:task_impl.cc(31)] No task runner for threadId 0` The more browsers that are added, the more spam there is. The browser can be removed, stopping the spam, but it starts up again when you go to close OBS, and requires killing. OBS functions as expected when no browser stuff is ever loaded.
PatTheMav commented 2021-11-21 05:50:01 -06:00 (Migrated from github.com)

That's weird given that most changes are guarded by a version check for newer CEF versions only, the changes that affect every version is proper use of nullptr and Dillon's change to use cef_window_open_disposition_t from what I could see..

That's weird given that most changes are guarded by a version check for newer CEF versions only, the changes that affect every version is proper use of `nullptr` and Dillon's change to use `cef_window_open_disposition_t` from what I could see..
tt2468 commented 2021-11-21 05:53:23 -06:00 (Migrated from github.com)

@PatTheMav I can try using a fresh dev environment in the morning, just to make sure that there isn't some random issue.

@PatTheMav I can try using a fresh dev environment in the morning, just to make sure that there isn't some random issue.
tt2468 commented 2021-11-21 16:15:28 -06:00 (Migrated from github.com)

Fresh ENV - Building with 4280 still errors, and I also discovered that I was only able to build with the spotify 4638 CEF because of previous cmake cache. I need a download of 4638 with the correct CEF wrapper.

Fresh ENV - Building with 4280 still errors, and I also discovered that I was only able to build with the spotify 4638 CEF because of previous cmake cache. I need a download of 4638 with the correct CEF wrapper.
WizardCM (Migrated from github.com) reviewed 2021-11-22 03:12:23 -06:00
browser-client.hpp
@ -28,6 +29,10 @@ struct BrowserSource;
class BrowserClient : public CefClient,
WizardCM (Migrated from github.com) commented 2021-11-22 03:12:22 -06:00 
#endif
		      public CefContextMenuHandler,
```suggestion #endif public CefContextMenuHandler, ```
WizardCM (Migrated from github.com) requested changes 2021-11-22 03:14:00 -06:00
WizardCM (Migrated from github.com) left a comment

I've submitted a bugfix for the reported issue on older CEF.

I've submitted a bugfix for the reported issue on older CEF.
tt2468 commented 2021-11-23 21:37:26 -06:00 (Migrated from github.com)

Using the latest version:

CEF 4280 builds and works as expected

CEF 4638 builds and works as expected, with the bonus that it doesn't throw any threadrunner errors.

Big thing to note: At least on Ubuntu 20.04, in order to use 4638 you need to use a fresh build environment. For example, having a 4280-based environment then reconfiguring with cmake will cause tons of wacky errors on runtime, and the browser source will simply not work.

Perhaps there is some way to detect an old version of cef in cmakelists, to throw an explicit configuration error if the version changes?

Using the latest version: CEF 4280 builds and works as expected CEF 4638 builds and works as expected, with the bonus that it doesn't throw any threadrunner errors. **Big thing to note:** At least on Ubuntu 20.04, in order to use 4638 you *need* to use a fresh build environment. For example, having a 4280-based environment then reconfiguring with cmake will cause tons of wacky errors on runtime, and the browser source will simply not work. Perhaps there is some way to detect an old version of cef in cmakelists, to throw an explicit configuration error if the version changes?
WizardCM commented 2021-11-23 21:53:02 -06:00 (Migrated from github.com)

Remaining things to test/fix:

  • AJAX local files in "Local file" mode
  • Verify 3770 still works on Windows with these changes
Remaining things to test/fix: * [x] AJAX local files in "Local file" mode * [x] Verify 3770 still works on Windows with these changes
PatTheMav commented 2021-11-24 06:34:53 -06:00 (Migrated from github.com)

Using the latest version:

CEF 4280 builds and works as expected

CEF 4638 builds and works as expected, with the bonus that it doesn't throw any threadrunner errors.

Big thing to note: At least on Ubuntu 20.04, in order to use 4638 you need to use a fresh build environment. For example, having a 4280-based environment then reconfiguring with cmake will cause tons of wacky errors on runtime, and the browser source will simply not work.

Perhaps there is some way to detect an old version of cef in cmakelists, to throw an explicit configuration error if the version changes?

From what I could tell that is by design - CEF_ROOT_DIR only specifies a hint for FindCEF to look for its required files - once they are found, these values are set as cache variables and changing the root directory won't have any effect, as the library is already found.

The intended way is to delete the cache variables associated with CEF (except for the root directory) and re-run CMake again (so FindCEF will find the "new" library and headers).

As for a way to fix this: CEF would have to produce a proper CMake package with version and target configuration - then you could do find_package(CEF 95.xxx.xxx) and specify the exact version you want. As long as that isn't the case you'd have to do version management by hand.

> Using the latest version: > > CEF 4280 builds and works as expected > > CEF 4638 builds and works as expected, with the bonus that it doesn't throw any threadrunner errors. > > **Big thing to note:** At least on Ubuntu 20.04, in order to use 4638 you _need_ to use a fresh build environment. For example, having a 4280-based environment then reconfiguring with cmake will cause tons of wacky errors on runtime, and the browser source will simply not work. > > Perhaps there is some way to detect an old version of cef in cmakelists, to throw an explicit configuration error if the version changes? From what I could tell that is by design - `CEF_ROOT_DIR` only specifies a hint for `FindCEF` to look for its required files - once they are found, these values are set as cache variables and changing the root directory won't have any effect, as the library is already found. The intended way is to delete the cache variables associated with CEF (except for the root directory) and re-run CMake again (so `FindCEF` will find the "new" library and headers). As for a way to fix this: CEF would have to produce a proper CMake package with version and target configuration - then you could do `find_package(CEF 95.xxx.xxx)` and specify the exact version you want. As long as that isn't the case you'd have to do version management by hand.
phedders commented 2021-11-24 08:04:18 -06:00 (Migrated from github.com)

On Wed, 2021-11-24 at 04:35 -0800, Patrick Heyer wrote:

The intended way is to delete the cache

Would you be able to 'remind' (:D) me on the correct or most efficient
way to do that?
Many thanks!

On Wed, 2021-11-24 at 04:35 -0800, Patrick Heyer wrote: > The intended way is to delete the cache Would you be able to 'remind' (:D) me on the correct or most efficient way to do that? Many thanks!
PatTheMav commented 2021-11-24 09:04:05 -06:00 (Migrated from github.com)

On Wed, 2021-11-24 at 04:35 -0800, Patrick Heyer wrote: The intended way is to delete the cache
Would you be able to 'remind' (:D) me on the correct or most efficient way to do that? Many thanks!

Either run ccmake -S . -B [YOUR_BUILD_DIR], then press t to see the advanced entries and delete every entry starting with CEF_ but CEF_ROOT_DIR.

Or edit CMakeCache.txt in your build directory directly and also remove those entries.

> On Wed, 2021-11-24 at 04:35 -0800, Patrick Heyer wrote: The intended way is to delete the cache > Would you be able to 'remind' (:D) me on the correct or most efficient way to do that? Many thanks! Either run `ccmake -S . -B [YOUR_BUILD_DIR]`, then press `t` to see the advanced entries and delete every entry starting with `CEF_` but `CEF_ROOT_DIR`. Or edit `CMakeCache.txt` in your build directory directly and also remove those entries.
WizardCM commented 2021-11-25 04:24:20 -06:00 (Migrated from github.com)

This seems to fix AJAX calls to local files in local file mode, but may break large media files.

diff --git a/browser-scheme.hpp b/browser-scheme.hpp
index 62b4d75e..dd28614f 100644
--- a/browser-scheme.hpp
+++ b/browser-scheme.hpp
@@ -22,7 +22,7 @@
 #include <string>
 #include <fstream>

-#if CHROME_VERSION_BUILD >= 3440
+#if CHROME_VERSION_BUILD >= 3440 && CHROME_VERSION_BUILD < 4638
 #define ENABLE_LOCAL_FILE_URL_SCHEME 1
 #else
 #define ENABLE_LOCAL_FILE_URL_SCHEME 0

Edit: Verified this breaks large media files. But only sometimes?

This seems to fix AJAX calls to local files in local file mode, but may break large media files. ```diff diff --git a/browser-scheme.hpp b/browser-scheme.hpp index 62b4d75e..dd28614f 100644 --- a/browser-scheme.hpp +++ b/browser-scheme.hpp @@ -22,7 +22,7 @@ #include <string> #include <fstream> -#if CHROME_VERSION_BUILD >= 3440 +#if CHROME_VERSION_BUILD >= 3440 && CHROME_VERSION_BUILD < 4638 #define ENABLE_LOCAL_FILE_URL_SCHEME 1 #else #define ENABLE_LOCAL_FILE_URL_SCHEME 0 ``` Edit: Verified this breaks large media files. But only sometimes?
PatTheMav commented 2021-11-25 12:58:47 -06:00 (Migrated from github.com)

Large media file playback seems fine to me, I checked local CORS with your test suite, both XHR and Fetch seem to work fine. Pointing to a non-existing video source led to a hard crash though, as we don't check for file existence before creating a CefStreamReader it seems.

Loading remote will still be blocked by every host that doesn't use Access-Control-Allow-Origin: * but that's kind-of obvious.

Edit: Added a fix for the missing file issue - how did you recreate the large media file issue? I managed to watch an entire TV episode via browser source without issue just now.

Large media file playback seems fine to me, I checked local CORS with your test suite, both XHR and Fetch seem to work fine. Pointing to a non-existing `video` source led to a hard crash though, as we don't check for file existence before creating a `CefStreamReader` it seems. Loading remote will still be blocked by every host that doesn't use `Access-Control-Allow-Origin: *` but that's kind-of obvious. Edit: Added a fix for the missing file issue - how did you recreate the large media file issue? I managed to watch an entire TV episode via browser source without issue just now.
👍 1
WizardCM commented 2021-11-26 17:44:05 -06:00 (Migrated from github.com)

For some reason, the diff below fixes the browser panel resize issue on Linux. If we end up using this fix temporarily, I recommend we scope it to 4638.

diff --git a/panel/browser-panel.cpp b/panel/browser-panel.cpp
index e56565b29..55af1a1bf 100644
--- a/panel/browser-panel.cpp
+++ b/panel/browser-panel.cpp
@@ -427,6 +427,8 @@ void QCefWidgetInternal::Resize()
                changes.height = size.height();
                XConfigureWindow(xDisplay, (Window)handle,
                                 CWX | CWY | CWHeight | CWWidth, &changes);
+               XSync(xDisplay, false);
 #endif
        });

This is probably not the best solution, but it does work.

For some reason, the diff below fixes the browser panel resize issue on Linux. If we end up using this fix temporarily, I recommend we scope it to 4638. ```diff diff --git a/panel/browser-panel.cpp b/panel/browser-panel.cpp index e56565b29..55af1a1bf 100644 --- a/panel/browser-panel.cpp +++ b/panel/browser-panel.cpp @@ -427,6 +427,8 @@ void QCefWidgetInternal::Resize() changes.height = size.height(); XConfigureWindow(xDisplay, (Window)handle, CWX | CWY | CWHeight | CWWidth, &changes); + XSync(xDisplay, false); #endif }); ``` This is probably not the best solution, but it does work.
PatTheMav commented 2021-11-27 11:01:06 -06:00 (Migrated from github.com)

Confirmed the fix, added as additional commit.

Confirmed the fix, added as additional commit.
Sign in to join this conversation.
Reviewers
No reviewers
WizardCM
Labels
Clear labels   
area/accessibility
Improves accessibility of the program

  
area/dependency
Issue is in external library or dependency, not OBS itself

  
Confirmed

   
Crash Report

   
Delayed
Merging has been delayed due to an issue

  
Design Issue
There is an issue with incorporating this pull request's design. It may take some time to finalize.

  
Duplicate

   
Feedback required
Issue does not contain enough information or is missing the template

  
Good first issue
New to contributing to OBS? Start here!

  
kind/bug
Categorizes issue or PR as related to a bug.

  
kind/cleanup
Non-breaking change which makes code smaller or more readable

  
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.

  
platform/linux

   
platform/macos

   
platform/windows

   
priority/high
This is an important PR

  
Question

   
Request for Comments
More feedback & discussion is requested

  
Seeking Other Contributors
This PR needs help from any other developers who are willing to give a crack at this code

  
Seeking Testers

   
Won't Fix / By Design
No labels
area/accessibility
area/dependency
Confirmed
Crash Report
Delayed
Design Issue
Duplicate
Feedback required
Good first issue
kind/bug
kind/cleanup
kind/enhancement
platform/linux
platform/macos
platform/windows
priority/high
Question
Request for Comments
Seeking Other Contributors
Seeking Testers
Won't Fix / By Design
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-obsproject/obs-browser!323
No description provided.