Enable building with CEF 6261 & 6367 (Chromium 122 & 124) #434
Conversation
WizardCM
force-pushed
the
cef-upgrade-122
branch
from
b8838ad
to
94aeed1
Compare
|
Slightly off-topic, but you might be interested to know. |
|
@WizardCM do you want to include the texture flip override for builds above 6261 in this PR as well (as discussed off-thread)? As the texture shared with us is originally created for video encoding, it's pre-flipped already. |
This allows compiling OBS Browser with Chromium 122-based CEF versions. Sources: - https://github.com/chromiumembedded/cef/blame/d3a483ef59f8f40c624967361e05edda413bbf5b/libcef_dll/ctocpp/browser_ctocpp.h#L57 - https://bitbucket.org/chromiumembedded/cef/commits/f3b570c
This allows compiling OBS Browser with Chromium 124-based CEF versions. This also enables basic support for the new, official, shared texture API, purely because building without it would be *more* difficult. Sources: - https://bitbucket.org/chromiumembedded/cef/commits/260dd0ca2
WizardCM
force-pushed
the
cef-upgrade-122
branch
from
94aeed1
to
68bb8eb
Compare
WizardCM
changed the title
|
@RytoEX @PatTheMav Per prior discussion, this PR now makes the necessary adjustments (including |
| @@ -415,7 +427,11 @@ void BrowserClient::OnAcceleratedPaint(CefRefPtr<CefBrowser>, | |||
| UpdateExtraTexture(); | |||
| obs_leave_graphics(); | |||
|
|
|||
| #if CHROME_VERSION_BUILD >= 6367 | |||
There was a problem hiding this comment.
I doubt the correctness, although it might be true for now because the underlying FrameSinkVideoCapturer will resurrect the frame if there's no change, but I don't remember whether it will still clone the handle and change the value (although it still points to same texture). See Export Handle
Also, even the textures are different, there are still chances that CloneHandle will return same value of two different texture.
In conclusion, I suggest not record this, and do not early return if new last_handle == info.shared_texture_handle to prevent potential frame skips.
There was a problem hiding this comment.
Is this a theoretical or confirmed problem? If this is definitely creating an observable problem after building against the latest version of CEF, then we should address it now. If this is merely a theoretical problem that has not been confirmed, then I don't want to spend too much time dwelling on it. I'd rather make it possible to make builds against CEF 122/124+ and figure out what problems exist and fix them.
There was a problem hiding this comment.
Although IOSurface pointers could be valid for this as discussed with @PatTheMav , DXGI handles are not. I think we should just remove this line and remove last_handle. Re-importing every frame just cost 50~100us and I think we can omit it.
The problem is this is a real theoretical problem and we will not know if underlying chromium code changes behaviour. The suggested behaviour doesn't let us "cache".
Considering a frame skip could be rare and hard to notice, I think we should just disable such "optimization" and do the absolute right thing, prevent letting someone in the world wondering why frames are skipped.
If someone have time to write a reliable test to confirm such skip are valid on all platform, I'll be in favor, otherwise I think is it worthless to take the risk considering the cost is so small.
There was a problem hiding this comment.
A problem can't be both "real" and "theoretical". I'm asking if there's an observable failure/error when building with this code against CEF 124+. As it is right now, we cannot test obs-browser with CEF 124+ in any meaningful way without this PR, which is my concern at the moment. I'm not saying "don't ever fix this". I'm saying "does this need to be fixed before merging", because otherwise we can't meaningfully test CEF 124+ without specifically pulling this PR locally.
There was a problem hiding this comment.
Theoretically, yes. You can also anaylze the code and get the same conclusion. So I think we would better remove this >= 6367 to take no risk.
| void *shared_handle) | ||
| #endif | ||
| { | ||
| if (type != PET_VIEW) { | ||
| // TODO Overlay texture on top of bs->texture |
There was a problem hiding this comment.
Also left a word here. This should be removed >= 6367 as it has no guarantee on both macOS and Windows that same handle = same content. On Linux this handle is no more valid because of that complex native pixmap structure.
#ifndef _WIN32
if (shared_handle == bs->last_handle)
return;
#endif
There was a problem hiding this comment.
As with the other one, is this a theoretical or confirmed problem?
Description
This allows compiling OBS Browser with Chromium 124-based CEF versions.
This also adds basic support for the new, official, shared textures API on both Windows and macOS.
Sources:
Fun note; Remote debugging now requires
--remote-allow-origins=*otherwise Remote Debugging immediately disconnects - even for localhost.Motivation and Context
At some point we'd like to upgrade to modern CEF. To do that, obs-browser needs to compile.
How Has This Been Tested?
Launch OBS with a CEF 6261 or 6367 build. Have not tested downgrading CEF to our currently supported versions.
Only tested on Windows.
Types of changes
Checklist: