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

Initial support for running compositor_render on web #683

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

noituri
Copy link
Member

@noituri noituri commented Aug 22, 2024

Issue: #653

@noituri noituri force-pushed the @noituri/compositor-wasm branch from 74ac1fa to a4abd45 Compare August 23, 2024 12:22
compositor_render/src/transformations/layout/params.rs Outdated Show resolved Hide resolved
compositor_render/src/wgpu/ctx.rs Outdated Show resolved Hide resolved
schemars = { git = "https://github.com/membraneframework-labs/schemars", rev = "a5ad1f9", features = [
"preserve_order",
] }
shared_memory = "0.12.4"
wgpu = { version = "0.19.1", default-features = false, features = [
Copy link
Member

Choose a reason for hiding this comment

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

What are you actually changing here, is this disabling some default features, if so which?

Copy link
Member Author

@noituri noituri Aug 23, 2024

Choose a reason for hiding this comment

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

By default wgpu has webgpu feature enabled which has higher priority than webgl feature. Currently, compositor does not run on webgpu due to lack of support for push constants on webgpu (they are not emulated like on webgl)

Edit:
Technically, chrome supports push constants on webgpu but it's experimental and needs a special flag enabled. I'm not sure about Firefox

Copy link
Member Author

@noituri noituri Aug 23, 2024

Choose a reason for hiding this comment

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

Also, I added fragile-send-sync-non-atomic-wasm feature, which makes the wgpu types Send (by default they are not Send and Sync on wasm). This feature is slightly unsafe because on wasm those types should not be shared between threads and it removes the compile time checks.

But, since we target web browsers which run the app on single thread it should not be problem for us. LayoutProvider needs to be Send because compositor_pipeline uses threads. We could also solve it by making 2 LayoutProviders (one Send and the other one non-Send) and enabling one based on #[cfg(target_arch = "wasm32")].

I decided to use the first approach with adding the feature because it was just the easier way

compositor_api/Cargo.toml Show resolved Hide resolved
compositor_render/Cargo.toml Outdated Show resolved Hide resolved
compositor_render/src/transformations/layout/params.rs Outdated Show resolved Hide resolved
@noituri noituri force-pushed the @noituri/compositor-wasm branch from 921cea1 to 1c05a8d Compare August 26, 2024 12:38
@noituri noituri requested a review from wkozyra95 August 26, 2024 12:38
@noituri noituri force-pushed the @noituri/compositor-wasm branch from 1c05a8d to 5cdf8b4 Compare August 26, 2024 12:48
Copy link
Member

@wkozyra95 wkozyra95 left a comment

Choose a reason for hiding this comment

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

I would recommend rethinking file structure. e.g.

  • you have utils file that only contains stuff related to wgpu
  • you have app module that seems to sth top level, but it is only used in utils package

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
compositor_web/src/wasm.rs Outdated Show resolved Hide resolved
@wkozyra95
Copy link
Member

wkozyra95 commented Aug 29, 2024

Do not merge everything into this branch, if this is ready then merge it to master

@noituri noituri merged commit 3828d0b into master Aug 29, 2024
3 checks passed
@noituri noituri deleted the @noituri/compositor-wasm branch August 29, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants