feat: GPU shared texture offscreen rendering #42953
Conversation
There was a problem hiding this comment.
Hello @reitowo! It looks like this pull request touches one of our dependency files, and per our contribution policy we do not accept these types of changes in PRs.
|
Well, it seems the CI machine failed to create a D3D11 device.
Trying to make this optional. |
The change requested was to not have unknown contributors modifying yarn.lock, but the change here is just to add an internal vendorized dependency for specs. It's harmless.
| + // once the GMB is returned from CopyRequest. So there will be no race | ||
| + // condition on that texture. We can request a GMB without a keyed mutex to | ||
| + // accelerate and probably prevent some driver deadlock. | ||
| + if (format_ == PIXEL_FORMAT_ARGB || format_ == PIXEL_FORMAT_ABGR) { |
There was a problem hiding this comment.
Electron's desktopCapturer uses this format. Would it be affected?
There was a problem hiding this comment.
As long as that code use BufferFormatPreference::kDefault which will only requests a CPU only buffer, it won't be affected by this, this patch made changes to renderable_gpu_memory_buffer_video_frame_pool which is a GPU related buffer code.
Even it use GPU texture later (BufferFormatPreference::kPreferGpuMemoryBuffer), it will not be affected if you won't write to the texture again. The patch just remove the keyed mutex on that texture.
PS: Does it mean desktopCapturer also possible to use GPU for acceleration? Just randomly thinking...
|
Rebased conflict. |
|
If someone enables this feature, but neglects to call There is an EmitWarning function that you could use to log this warning. You'd also only want to log it once per process lifetime, to avoid spamming for every |
|
Definetely ok, but not necessary. People use OSR features are expected to receive continuous frames. And since it will be drained only after 10-11 frames, the user will no longer receive any updates, so they must notice this as a bug. Relying on GC is not working because the unknown timing. 10 frames are only 160ms at 60fps and GC are not running that fast. However I think it is best to add that hint when it got GC-ed but not manually released for better understanding whats going on. 👍🏻 (If theres really a way to monitor that) |
|
Implemented it, fun feature. @itsananderson Note the warning won't show until next GC, so its kinda slow under testing enviroment. You can manually trigger GC to see this.
|
|
@reitowo looks like the patch needs to be updated as it's failing to apply. |
|
I noticed that and I already updated the patch.
PS: For future maintaince of this patch please @ me at any time. |
c72e37c to
6bd174d
Compare
|
Fixed the patch and verified to work! Hope we can merge this in next days 😎 |
|
Release Notes Persisted
|
|
Great! Thanks for all your work reviewing this PR and maintaining electron. I'm glad to be able to make this contribution! ❤️ |
|
Just to ask, is it possible to backport this feature to previous Electron versions? (32) |
|
Hi @reitowo, If you would like to backport the PR to previous branches, a releaser can assist by running Trop automation. Backporting a feature PR will automatically put it up for a vote by the Releases working group, whereafter the PR can be accepted or denied on a case-by-case basis. See https://www.electronjs.org/docs/latest/tutorial/electron-versioning#backport-request-process for more info. As an aside (I'm not in the WG), I think the general policy for stable release lines is that PRs with large patches or changes to existing code paths are considered riskier and have a lower chance of being accepted. |
|
Thanks @erickzhao! @reitowo, I'll go ahead and run backports for you to 33-x-y and 32-x-y. To Erick's point, I think we're probably unlikely to backport a change this large into an already existing stable release line, but we can vote and determine whether or not it's a good fit 🙂 |
|
/trop run backport-to 33-x-y |
|
The backport process for this PR has been manually initiated - sending your PR to |
|
I have automatically backported this PR to "33-x-y", please check out #43694 |
|
/trop run backport-to 32-x-y |
|
The backport process for this PR has been manually initiated - sending your PR to |
|
I was unable to backport this PR to "32-x-y" cleanly; |
|
Thanks! I got your point about bp into 32, thats reasonable, and it will still be nice if we can bp into 33. 😉 I'll still try manual bp into 32 and wait for your vote. |
Description of Change
Reopen #42001
I managed to add
ARGB + kGpuMemoryBuffersupport for FrameSinkVideoCapturer in recent upstream changes. Thus, we can finally use zero-copy (actually one, there's a CopyRequest of frame texture) GPU shared texture OSR in chromium apps.This will be the fastest OSR mode than the existing two, and it supports enabling hardware acceleration.
Details:
webPreferences.offscreen.useSharedTextureto enable this feature.texturetopaintevent ofwebContent.Fix: #41972
Checklist
npm testpassesRelease Notes
Notes: feat: GPU accelerated shared texture offscreen rendering.