servo / surfman

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

Fix eglGetProcAddress for core OpenGL entrypoints #94

Merged

emilio merged 2 commits into servo:master from MortimerGoro:egl_dlsym

Apr 2, 2017

Collaborator

MortimerGoro commented Mar 30, 2017 •

edited

Hi,

The upgrade to the new gleam has broken offscreen EGL based contexts & Android WebGL on Servo. This is because eglProcAddress can't be used to resolve core OpenGL function entry points on many implementations. Loading core functions with eglProcAddress is only possible in EGL 1.5.

MortimerGoro force-pushed the MortimerGoro:egl_dlsym branch from d4e4065 to b8cfd78


        Fix eglGetProcAddress for core OpenGL entrypoints

59f2701

MortimerGoro force-pushed the MortimerGoro:egl_dlsym branch from b8cfd78 to 59f2701

kvark reviewed

  
      @@ -38,5 +40,8 @@ gdi32-sys = "0.2"
    
        user32-sys = "0.2"

        kernel32-sys = "0.2"

        [target.'cfg(any(target_os="macos", target_os="windows"))'.dependencies]

        [target.'cfg(any(target_os="macos", target_os="windows", target_os="android"))'.dependencies]

        lazy_static = "0.2"

kvark Mar 30, 2017

Member

interesting, so we can have a dependency mentioned twice?

src/platform/with_egl/native_gl_context.rs Outdated

  
        lazy_static! {

            static ref GL_LIB: Option<lib::Library>  = {

                if cfg!(target_os = "android") {

                    lib::Library::new("libGLESv2.so").ok()

kvark Mar 30, 2017

Member

This doesn't support run-time selection of the GL backend. E.g. one may want to run with GLES on Linux desktop.

MortimerGoro Mar 30, 2017

Author Collaborator

That would require a change in the API because get_proc_address trait is static and we can't know the user preference. We could work on that on a separate issue/PR, it's less critical in this moment.

emilio Mar 31, 2017

Member

Perhaps we should be using or_else and not platform-cfgs?

Library::new("libGLESv2.so").or_else(|| lib::Library::new("libGL.so"))?

emilio reviewed

src/platform/with_egl/native_gl_context.rs Outdated

  
                    let addr = CString::new(addr.as_bytes()).unwrap().as_ptr();

                    egl::GetProcAddress(addr as *const _) as *const ()

                    if let Some(ref lib) = *GL_LIB {

                        let symbol: lib::Symbol<unsafe extern fn()> = lib.get(addr.as_bytes()).unwrap();

emilio Mar 31, 2017

Member

Perhaps returning ptr::null instead of unwrapping?

src/platform/with_egl/native_gl_context.rs Outdated

  
                    egl::GetProcAddress(addr as *const _) as *const ()

                    if let Some(ref lib) = *GL_LIB {

                        let symbol: lib::Symbol<unsafe extern fn()> = lib.get(addr.as_bytes()).unwrap();

                        return *symbol.deref() as *const();

emilio Mar 31, 2017

Member

nit: space after const

src/platform/with_egl/native_gl_context.rs Outdated

  
        lazy_static! {

            static ref GL_LIB: Option<lib::Library>  = {

                if cfg!(target_os = "android") {

                    lib::Library::new("libGLESv2.so").ok()

emilio Mar 31, 2017

Member

Perhaps we should be using or_else and not platform-cfgs?

Library::new("libGLESv2.so").or_else(|| lib::Library::new("libGL.so"))?

MortimerGoro force-pushed the MortimerGoro:egl_dlsym branch 2 times, most recently from eb403c5 to 06e074e

Collaborator Author

MortimerGoro commented Mar 31, 2017 •

edited

Curiosity: using or_else or other closuse patterns within the lazy_static macro causes some obscure errors: possibly related issue here rust-lang/rust#24680

emilio reviewed

src/platform/with_egl/native_gl_context.rs Outdated

  
                } else {

                    lib::Library::new("libGL.so").ok()

                }

               let names = ["libGLESv2.so", "libGLES.so", "libGLESv3.so"];

emilio Mar 31, 2017

Member

Still need libGL?

src/platform/with_egl/native_gl_context.rs Outdated

  
               let names = ["libGLESv2.so", "libGLES.so", "libGLESv3.so"];

               for name in &names {

                   let lib = lib::Library::new(name);

                   if lib.is_ok() {

emilio Mar 31, 2017

Member

if let Ok(lib) = lib::Library::new(name) {
    return Some(lib);
}

May be slightly clearer, your call.


        Avoid dlsym unwrap. Add more possible library names.

a48fcdc

MortimerGoro force-pushed the MortimerGoro:egl_dlsym branch from 06e074e to a48fcdc

Collaborator Author

MortimerGoro commented Mar 31, 2017 •

edited

@emilio I added libGL.so for desktop GL on linux. Ideally we should have a way to choose the desired library version instead of the predefined order.

emilio merged commit 0e1dbd1 into servo:master

1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed

Details

Member

emilio commented Apr 2, 2017

LGTM, thanks :)

MortimerGoro mentioned this pull request

Fix Android issues: update servo-glutin & offscreen_gl_context servo/servo#16237

Merged

3 of 5 tasks complete

bors-servo added a commit to servo/servo that referenced this pull request


        Auto merge of #16237 - MortimerGoro:update_glutin_offscreen, r=jdm

436d0dd

Fix Android issues: update servo-glutin & offscreen_gl_context

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/16237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request


        Auto merge of #16237 - MortimerGoro:update_glutin_offscreen, r=jdm

f113d69

Fix Android issues: update servo-glutin & offscreen_gl_context

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/16237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request


        Auto merge of #16237 - MortimerGoro:update_glutin_offscreen, r=jdm

f659d43

Fix Android issues: update servo-glutin & offscreen_gl_context

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/16237)
<!-- Reviewable:end -->

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request


        servo: Merge #16237 - Fix Android issues: update servo-glutin & offsc…

7c55171

…reen_gl_context (from MortimerGoro:update_glutin_offscreen); r=jdm

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: f659d43bd403e4ac1305990ba704f23b4a6cd5e6

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 76e38ca77cc9227bfaa6f30cc6445cdfcbacf29c

mcmanus pushed a commit to mcmanus/gecko that referenced this pull request


        servo: Merge #16237 - Fix Android issues: update servo-glutin & offsc…

…reen_gl_context (from MortimerGoro:update_glutin_offscreen); r=jdm

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: f659d43bd403e4ac1305990ba704f23b4a6cd5e6

Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request


        servo: Merge #16237 - Fix Android issues: update servo-glutin & offsc…

9ab88d2

…reen_gl_context (from MortimerGoro:update_glutin_offscreen); r=jdm

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: f659d43bd403e4ac1305990ba704f23b4a6cd5e6

JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request


        servo: Merge #16237 - Fix Android issues: update servo-glutin & offsc…

22fa8f0

…reen_gl_context (from MortimerGoro:update_glutin_offscreen); r=jdm

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: f659d43bd403e4ac1305990ba704f23b4a6cd5e6

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request


        servo: Merge #16237 - Fix Android issues: update servo-glutin & offsc…

0de5ac7

…reen_gl_context (from MortimerGoro:update_glutin_offscreen); r=jdm

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: f659d43bd403e4ac1305990ba704f23b4a6cd5e6

UltraBlame original commit: 7f9f301f0bb21e330116c34f393514bcf20bc451

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request


        servo: Merge #16237 - Fix Android issues: update servo-glutin & offsc…

28a6e46

…reen_gl_context (from MortimerGoro:update_glutin_offscreen); r=jdm

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: f659d43bd403e4ac1305990ba704f23b4a6cd5e6

UltraBlame original commit: 7f9f301f0bb21e330116c34f393514bcf20bc451

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request


        servo: Merge #16237 - Fix Android issues: update servo-glutin & offsc…

c545789

…reen_gl_context (from MortimerGoro:update_glutin_offscreen); r=jdm

<!-- Please describe your changes on the following line: -->
See servo/surfman#94 and servo/glutin#121

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: f659d43bd403e4ac1305990ba704f23b4a6cd5e6

UltraBlame original commit: 7f9f301f0bb21e330116c34f393514bcf20bc451

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.