Skip to content
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.

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

feat: GPU shared texture offscreen rendering #42001

Closed
wants to merge 1 commit into from

Conversation

reitowo
Copy link
Contributor

@reitowo reitowo commented Apr 30, 2024

Description of Change

I managed to add ARGB + kGpuMemoryBuffer support 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.

However, this mode will not compose the popup widgets, but directly provide textures to user.

Details:

  1. Add webPreferences.offscreenUseSharedTexture to enable this feature.
  2. Add texture parameter to paint event of webContent.
  3. Add structure definitions and docs.

Fix: #41972

Checklist

Release Notes

Notes: feat: GPU shared texture offscreen rendering.

Copy link

welcome bot commented Apr 30, 2024

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 30, 2024
@reitowo reitowo force-pushed the main branch 11 times, most recently from 3e3b554 to 433bfc6 Compare April 30, 2024 14:52
@reitowo reitowo force-pushed the main branch 3 times, most recently from 9b15446 to 7acece9 Compare May 1, 2024 02:28
@reitowo reitowo requested a review from a team as a code owner May 1, 2024 03:16
@reitowo
Copy link
Contributor Author

reitowo commented May 1, 2024

Ready for review. Any suggestions? Here are some answers in advance:

  1. Intentionally reused 'paint' event instead of creating a new one, because I don't see mandatory needs of creating a new event, and I prefer adding an extra parameter to 'paint', which also has no breaking change.

  2. offscreenUseSharedTexture might be a little bit long, but I can't figure out a shorter name with same meaning.

  3. Although we cannot do anything about the texture handle in pure node.js, we can write native node modules to import the texture, example

  4. Please confirm that Emit('paint') is a blocking call, as chromium will recycle the textures once the callback returns, so it has to be blocking.

  5. The speed will be much faster than previous hardware accelerated one (~3ms vs <100us), and it has no more words about the software output device which has no GPU support and slow.

@reitowo
Copy link
Contributor Author

reitowo commented May 1, 2024

Tested to be working on Windows, example repo

1.webm

@Hate-Usernames
Copy link

  1. Although we cannot do anything about the texture handle in pure node.js, we can write native node modules to import the texture, example

Speaking as an interested outsider: how possible would it be to make it so that the texture could be used in the renderer process via webGPU with a workflow something like:

Main Process

const appWin = new BrowserWindow({ width: 800, height: 800 })
appWin.loadFile('index.html')

const win = new BrowserWindow({ webPreferences: { offscreen: true, offscreenUseSharedTexture: true } })
win.webContents.on('paint', (event, dirty, image, texture) => {
  appWin.webContents.send('texture', texture)
})
win.loadURL('https://github.com')

Renderer Process

ipcRenderer.on('texture', (_event, source) => {
  // Or similar function for chromium shared textures
  const externalTexture = gpuDevice.importExternalTexture({ source })
  // Use the texture in a webGPU pipeline and eventually
  // render the result to a canvas element
})

I'm biased as this is something I've been dreaming of for a long time, but if it is relatively simple to achieve, and performant, I think it could make this feature a lot more useful and accessible for the majority of the userbase.

@reitowo
Copy link
Contributor Author

reitowo commented May 1, 2024

renderer process via webGPU

Definitely possible. Actually I already did some research when I was walking through dawn's source. At least on Windows you can import a DXGI handle into dawn world (via webgpu native), then you can try convert it to a v8 webgpu texture to use it anywhere. There's no existing js api for this, you need to write one.

@reitowo
Copy link
Contributor Author

reitowo commented May 2, 2024

// PendingRemote instances may be freely moved to another thread/sequence, or
// even transferred to another process via a Mojo interface call (see
// pending_remote syntax in mojom IDL).

I think ipcMain ipcRenderer doesn't support passing a pending_remote mojo? so I think the release callback it not able to pass to another process. The only reason will be supporting async on paint event to let you release the texture after some awaits.

@reitowo reitowo force-pushed the main branch 2 times, most recently from 08563b3 to 870aec1 Compare May 2, 2024 10:30
@reitowo
Copy link
Contributor Author

reitowo commented May 2, 2024

@Hate-Usernames Good news: I managed to let user able to release the texture when they want, which means you can pass the texture to wherever process you want, but releasers need to be maintained in the callback process and cannot be ipc. I think it can already do what we want, nnaintaining that would be easy with few more ipc calls, like posting the texture to the other process, and it replies a release message, then main process call the releaser.

About WebGPU, I think we can make that possible in another PR. I think CEF would also be interested in such capability. It will be awesome to directly use the texture in WebGPU pipelines.

@jkleinsc jkleinsc added the semver/minor backwards-compatible functionality label May 2, 2024
@reitowo
Copy link
Contributor Author

reitowo commented May 4, 2024

I think the CI machines are dead for this PR? 🙂

@reitowo
Copy link
Contributor Author

reitowo commented May 26, 2024

Hi @nornagon ! Sorry for the delay.

I've add tests and made change according to your review. Please do a second round review when you're vacant.

@Hate-Usernames I'll try add that importing feature after this got merged in separate PR.

@reitowo reitowo force-pushed the main branch 3 times, most recently from 81860ae to 153d1ee Compare May 26, 2024 16:34
@reitowo
Copy link
Contributor Author

reitowo commented May 26, 2024

Is there a special handler to serialize the texture object when passing through electron ipc? If so we might able to create a speicalized protocol to pass the ownership?

I also looked into transferrable objects, and it seems not an option because it needs to change blink.

(Though I think managing purely at main process would also be acceptable as it is not complicated, even more simple if #42231 merged.)

@admsev

This comment was marked as spam.

@reitowo
Copy link
Contributor Author

reitowo commented Jun 3, 2024

@Hate-Usernames Good news, I've imported into WebGPU. However, I suspect if this will ever gonna merged into electron because of too hacky, lol.
image

Update: Sadly, not fully useable
image

@samuelmaddock
Copy link
Member

What's the use case for supporting this in WebGPU? Is it something that could be covered by the Element Capture API? cc @Hate-Usernames

docs/api/web-contents.md Show resolved Hide resolved
shell/browser/osr/osr_paint_event.h Show resolved Hide resolved
@@ -6432,6 +6433,66 @@ describe('BrowserWindow module', () => {
});
});

describe('offscreen rendering image', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Given how big this file is, and the size of the OSR feature in general, I think it might be good to move these tests into its own osr-spec.ts file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently I'm busy for refactoring, may I refactor these next time if extra works need to be done?

shell/common/gin_converters/osr_converter.cc Show resolved Hide resolved
@Hate-Usernames
Copy link

What's the use case for supporting this in WebGPU? Is it something that could be covered by the Element Capture API? cc @Hate-Usernames

As I understand it, the getDisplayMedia APIs discard the alpha channel, retaining this would be a key requirement for my use case (related to broadcast graphics).

@samuelmaddock
Copy link
Member

What's the use case for supporting this in WebGPU? Is it something that could be covered by the Element Capture API? cc @Hate-Usernames

As I understand it, the getDisplayMedia APIs discard the alpha channel, retaining this would be a key requirement for my use case (related to broadcast graphics).

You may want to try describing your use case in the spec repo. They have an issue for transparency support.

For Electron, it sounds like what you'd need would be a custom MediaStream constructed from the browser process. Discord added such a feature in their fork. Something similar might be a more viable proposal for your use case.

@reitowo
Copy link
Contributor Author

reitowo commented Jun 12, 2024

Thanks for the info.

There're also some senario when you want to import a texture completely from outside. However it doesn't work with WebGPU (at least SPEC version, not dawn or dawn native) currently.

Copy link
Contributor

github-actions bot commented Jul 7, 2024

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 PRs, so this PR will be closed.

@github-actions github-actions bot closed this Jul 7, 2024
@reitowo
Copy link
Contributor Author

reitowo commented Jul 7, 2024

@samuelmaddock @nornagon Hi, guys. It's been a while cause I'm busy recently.

I've made changes according to your suggestions. Could you do another review? Hope we can merge this soon! :)

The bot closed this PR because I changed package.json and yarn.lock, but it was mandatory for adding that fixture test as dependency.

Could anyone reopen this PR and disable the bot for future close?

@reitowo
Copy link
Contributor Author

reitowo commented Jul 7, 2024

image

Also tested with npm run test -- -g "offscreen\ rendering\ image"

Hope you could see it soon.

@samuelmaddock
Copy link
Member

@reitowo just want you to know I'm still watching this. Currently awaiting folks to take a closer look at #42855 so we can avoid PRs closing again.

There's also an unfortunate limitation of GitHub we weren't aware of until now. After a GitHub PR closes, it can't be reopened if the underlying branch has been rebased. 🥲

Please hold off until the linked PR can be merged, then you'll need to create a new PR due to the limitation. We can pick up the review from there. Sorry about all this.

@reitowo
Copy link
Contributor Author

reitowo commented Jul 16, 2024

Sure! 🥳

@samuelmaddock
Copy link
Member

samuelmaddock commented Jul 17, 2024

@reitowo the PR has been merged so you should be good to open up a new PR with these changes. You may want to check that its been rebased to the latest changes on main

@reitowo
Copy link
Contributor Author

reitowo commented Jul 18, 2024

Done. See #42953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Hardware accelerated off-screen rendering
6 participants