Skip to content

Implement GLContextDispatcher trait for NativeGLContexts #432

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 18, 2016
Merged

Implement GLContextDispatcher trait for NativeGLContexts #432

merged 1 commit into from
Oct 18, 2016

Conversation

MortimerGoro
Copy link
Contributor

@MortimerGoro MortimerGoro commented Oct 5, 2016 •
edited by larsbergstrom
Loading

Implement GLContextDispatcher trait for NativeGLContexts. Required to allow GLContext sharing in some implementations like WGL.

See servo/surfman#69 for more info.

This change is Reviewable

@MortimerGoro MortimerGoro changed the title Implement GLContextDispatcher trait for NativeGLContexts, r=emilio Implement GLContextDispatcher trait for NativeGLContexts Oct 5, 2016
emilio
emilio reviewed Oct 5, 2016
View reviewed changes
webrender/src/render_backend.rs
@@ -42,6 +43,7 @@ pub struct RenderBackend {
webgl_contexts: HashMap<WebGLContextId, GLContextWrapper>,
current_bound_webgl_context_id: Option<WebGLContextId>,
enable_recording: bool,
main_thread_dispatcher: Arc<Mutex<Option<Box<RenderDispatcher>>>>
Copy link
Member

Choose a reason for hiding this comment

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

Probably an option of an arc is a better Idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the same type pattern as the one used in RenderNotifier setter

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I see why you need this, but it seems unfortunate.

emilio
emilio reviewed Oct 5, 2016
View reviewed changes
webrender/src/render_backend.rs
@@ -261,7 +265,10 @@ impl RenderBackend {
}
ApiMsg::RequestWebGLContext(size, attributes, tx) => {
if let Some(ref wrapper) = self.webrender_context_handle {
let result = wrapper.new_context(size, attributes);
let dispatcher = Box::new(WebRenderGLDispatcher {
Copy link
Member

Choose a reason for hiding this comment

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

Do this only on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll add a macro. It will avoid unneeded allocations in the platforms that don't use this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@emilio
Copy link
Member

emilio commented Oct 5, 2016

I need to review this really carefully. May you open also a PR with your servo-related changes so it's easy to see the code flow?

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Oct 5, 2016 •
edited
Loading

Still have to rebase the latest servo master, I'll do tomorrow. In the meantime you can check this: MortimerGoro/servo@64f92be, it will be the same code, just changing Dispatcher name to RenderDispatcher.

P.D.: I created another trait called RenderDispatcher in order to not expose internal types of webrender. I assumed that you want that webrender library users don't need to be aware that offscreen_gl_context is used internally. Servo now uses offscreen_gl_context includes, but I thought that might not be true in the future when webrender is set up as default and skia renderer dropped.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #439) made this pull request unmergeable. Please resolve the merge conflicts.

@MortimerGoro MortimerGoro force-pushed the dispatcher branch 2 times, most recently from 3cf4b78 to a189acc Compare October 18, 2016 10:20
@MortimerGoro
Copy link
Contributor Author

Any news on this?

I have a another idea to implement this. We could create and pass the dispatcher directly from WebGLPaintThred in servo, in this call.

wr_api.request_webgl_context(&size, attrs)

Let me know what you think

emilio
emilio approved these changes Oct 18, 2016
View reviewed changes
Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Sorry for the huge delay.

The other idea is nice too, but I think one global dispatcher is fine for now. Looks good to me with that nit addressed, and optionally that comment moved.

Thanks, and sorry again it took so long :)

webrender/src/render_backend.rs
let result = wrapper.new_context(size, attributes);
let dispatcher: Option<Box<GLContextDispatcher>>;
if cfg!(target_os = "windows") {
dispatcher = Some(Box::new(WebRenderGLDispatcher {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

let dispatcher = if cfg!(target_os = "windows") {
    Some(...)
} else {
    None
};

Copy link
Contributor Author

@MortimerGoro MortimerGoro Oct 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I had to specify the type: Option<Box<GLContextDispatcher>> or rust compiler doesn't have enough magic to coerce

webrender/src/renderer.rs
/// Sets the new MainThreadDispatcher.
///
/// Used to dispatch functions to the main thread's event loop.
/// Required to allow GLContext sharing in some implementations like WGL.
Copy link
Member

Choose a reason for hiding this comment

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

probably moving the last part of this comment to the main_thread_dispatcher member definition would make it a bit more helpful.

@MortimerGoro
Copy link
Contributor Author

Done :)

@emilio
Copy link
Member

emilio commented Oct 18, 2016

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 027e224 has been approved by emilio

@bors-servo
Copy link
Contributor

⌛ Testing commit 027e224 with merge b667e13...

bors-servo pushed a commit that referenced this pull request Oct 18, 2016
Auto merge of #432 - MortimerGoro:dispatcher, r=emilio
b667e13 
Implement GLContextDispatcher trait for NativeGLContexts

Implement GLContextDispatcher trait for NativeGLContexts. Required to allow GLContext sharing in some implementations like WGL.

See servo/surfman#69 for more info.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/432)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit 027e224 into servo:master Oct 18, 2016
This was referenced Oct 18, 2016
Merged
Merged
@MortimerGoro MortimerGoro mentioned this pull request Oct 19, 2016
Merged
5 tasks
bors-servo pushed a commit to servo/servo that referenced this pull request Oct 21, 2016
Auto merge of #13840 - MortimerGoro:webrender_dispatcher, r=emilio
4216224 
WebGL support on Windows

<!-- Please describe your changes on the following line: -->
This is the final step to provide WebGL support on Windows ;)

Some Related PRs already merged in webrender and offscreen-gl-context:

servo/surfman#64
servo/webrender#432

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13840)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 4, 2017
@MortimerGoro
servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…
ff7bafc 
…render_dispatcher); r=emilio

<!-- Please describe your changes on the following line: -->
This is the final step to provide WebGL support on Windows ;)

Some Related PRs already merged in webrender and offscreen-gl-context:

servo/surfman#64
servo/webrender#432

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Source-Repo: https://github.com/servo/servo
Source-Revision: 4216224f9cc7f3430db2b59f4d77061824ba7a1e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…
1dab258 
…render_dispatcher); r=emilio

<!-- Please describe your changes on the following line: -->
This is the final step to provide WebGL support on Windows ;)

Some Related PRs already merged in webrender and offscreen-gl-context:

servo/surfman#64
servo/webrender#432

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Source-Repo: https://github.com/servo/servo
Source-Revision: 4216224f9cc7f3430db2b59f4d77061824ba7a1e

UltraBlame original commit: c57538159933d9eb1840437dfa9c9860220dd805
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…
2a6cb8b 
…render_dispatcher); r=emilio

<!-- Please describe your changes on the following line: -->
This is the final step to provide WebGL support on Windows ;)

Some Related PRs already merged in webrender and offscreen-gl-context:

servo/surfman#64
servo/webrender#432

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Source-Repo: https://github.com/servo/servo
Source-Revision: 4216224f9cc7f3430db2b59f4d77061824ba7a1e

UltraBlame original commit: c57538159933d9eb1840437dfa9c9860220dd805
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…
bd8405f 
…render_dispatcher); r=emilio

<!-- Please describe your changes on the following line: -->
This is the final step to provide WebGL support on Windows ;)

Some Related PRs already merged in webrender and offscreen-gl-context:

servo/surfman#64
servo/webrender#432

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Source-Repo: https://github.com/servo/servo
Source-Revision: 4216224f9cc7f3430db2b59f4d77061824ba7a1e

UltraBlame original commit: c57538159933d9eb1840437dfa9c9860220dd805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MortimerGoro @emilio @bors-servo