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

Revisit how the Android port works. #20912

Merged
merged 6 commits into from Jul 31, 2018

Conversation

Projects
9 participants
@paulrouget
Copy link
Contributor

paulrouget commented Jun 4, 2018

Fix #20855
Fix #18625
Fix #21147

Before polishing and making sure everything works fine (like the VR code, the android-x86 version, the non-android version of the lib, …), I'd like to get some early feedback on the approach.

I recommend reviewing commit by commit.

To test, just follow the regular steps:

    ./mach build -d --android
    ./mach package --dev --android builds servo.apk
    ./mach install --dev --android && ./mach run --android

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@@ -0,0 +1,3 @@
This is a basic wrapper around Servo. While libservo itself (/components/servo/) offers a lot of flexibility,
libsimpleservo (/ports/libsimpleservo/) tries to make it easier to embed Servo, without much configuration needed.
It is limited to only one view (no tabs, no multiple renering area).

This comment has been minimized.

@cbrewster

cbrewster Jun 4, 2018

Member

s/renering area/rendering areas/

@jdm jdm self-assigned this Jun 4, 2018

@jdm jdm added this to In progress in Android MVP Jun 4, 2018

@jdm
Copy link
Member

jdm left a comment

This looks like a really useful improvement :D

use std::path::Path;

fn main() {
println!("cargo:rerun-if-changed=build.rs");

This comment has been minimized.

@jdm

jdm Jun 4, 2018

Member
Note that if the build script itself (or one of its dependencies) changes, then it's rebuilt and rerun unconditionally, so cargo:rerun-if-changed=build.rs is almost always redundant (unless you want to ignore changes in all other files except for build.rs).

From https://doc.rust-lang.org/cargo/reference/build-scripts.html


struct ResourceReader(Box<ReadFileTrait>);
unsafe impl Send for ResourceReader {}
unsafe impl Sync for ResourceReader {}

This comment has been minimized.

@jdm

jdm Jun 4, 2018

Member

This is scary. What if we store Box<ReadFileTrait + Send + Sync> instead?

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 19, 2018

Note to self:

  • make sure /ports/servo still works
  • test Android x86
  • missing support for Event::Suspended
  • test VR
  • test non-Android x86 (add example or port ports/servo to libsimpleservo (can be followup))
  • build ServoView and Surface code in a AAR file along the APK (for Crow and Klar) (can be followup)
  • remove android_injected_glue from all over and dependencies if possible not necessary anymore
@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 19, 2018

x86 build still works as long as EGL 3 is available.

screen shot 2018-06-19 at 08 21 48

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 19, 2018

WebVR build segfaults:

com.mozilla.servo A/libc: Fatal signal 11 (SIGSEGV), code 1, fault addr 0xc in tid 17599 (WebVRThread), pid 17357 (m.mozilla.servo)
@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 19, 2018

@MortimerGoro I might need your help here.

I get a crash in self.service.get_displays() (components/webvr/webvr_thread.rs) which is a rust-webvr method. I'm building with --features googlevr. It works well on master, but with this current PR it segfaults.

This PR changes how Servo runs on Android. Instead of using an empty Android app and load Servo via Glutin, we have a proper Android app that use a GLSurfaceView and load servo as a library and make some calls via JNI. I don't know what get_displays does, do you have any hint of what could go wrong? When can get_displays crash?

In the meantime, I will compile with my own version of rust-webvr see if I can find precisely where it crashes.

@jdm / @MortimerGoro also, in general, do you know how to get a rust stack trace on Android when it crashes?

@MortimerGoro

This comment has been minimized.

Copy link
Contributor

MortimerGoro commented Jun 19, 2018

@paulrouget I suspect that the crash is related to JNI calls which get the activity this way: android::get_app().activity

Could you check that android::get_app() is not null && the activity has a correct value?. If not we'll need to implement a different way to get it or expose a method in rust-webvr to set it from servo.

@MortimerGoro

This comment has been minimized.

Copy link
Contributor

MortimerGoro commented Jun 19, 2018

@paulrouget have you checked if WebGL works ok? IIRC GLSurfaceView was not a very friendly API to create shared contexts

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 19, 2018

@MortimerGoro WebGL works.

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 19, 2018

You were right. I'll try to figure out how to fix this.

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 19, 2018

@MortimerGoro I don't use android rs glue. Should I? Does it even make sense for a library?

If I don't, what's the alternative? We could add some embedder methods to access the activity. Would that be enough?

@MortimerGoro

This comment has been minimized.

Copy link
Contributor

MortimerGoro commented Jun 19, 2018

I think that the best way would be to get rid of android-rs-glue, and pass the JNI JavaVM and Activity pointers to rust-webvr (e.g. in the constructor or initialization methods).

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jun 19, 2018

To answer the question about stack traces, I do the following:

  • copy the logcat output that includes the crash information and unresolved symbols to a temporary file log
  • run ndk-stack -sym target/armv7-linux-androideabi/debug/apk/jniLibs/armeabi-v7a/ -dump log
    (it might be necessary to pass target/armv7-linux-androideabi/debug/apk/obj/local/armeabi-v7a/lib/ for the sym directory instead; I forget which is correct at the moment)

@jdm jdm removed the S-awaiting-review label Jun 19, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 19, 2018

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

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 20, 2018

@MortimerGoro,

I think that the best way would be to get rid of android-rs-glue

Entirely? Looks like it's used extensively.

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 20, 2018

@MortimerGoro what if I initialize the android glue manually (calling main2) by setting ANDROID_APP manually?

@paulrouget paulrouget force-pushed the paulrouget:androidv2 branch from a923e8b to 6a31864 Jul 31, 2018

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jul 31, 2018

@bors-servo r=jdm,mortimergoro,simonsapin

1 similar comment
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jul 31, 2018

@bors-servo r=jdm,mortimergoro,simonsapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 31, 2018

📌 Commit a923e8b has been approved by jdm,mortimergoro,simonsapin

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jul 31, 2018

@bors-servo r=jdm,mortimergoro,simonsapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 31, 2018

📌 Commit 6a31864 has been approved by jdm,mortimergoro,simonsapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 31, 2018

⌛️ Testing commit 6a31864 with merge 95b54ca...

bors-servo added a commit that referenced this pull request Jul 31, 2018

Auto merge of #20912 - paulrouget:androidv2, r=jdm,mortimergoro,simon…
…sapin

Revisit how the Android port works.

Fix #20855
Fix #18625
Fix #21147

Before polishing and making sure everything works fine (like the VR code, the android-x86 version, the non-android version of the lib, …), I'd like to get some early feedback on the approach.

I recommend reviewing commit by commit.

To test, just follow the regular steps:

```
    ./mach build -d --android
    ./mach package --dev --android builds servo.apk
    ./mach install --dev --android && ./mach run --android
```

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] 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/20912)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 31, 2018

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm,mortimergoro,simonsapin
Pushing 95b54ca to master...

@bors-servo bors-servo merged commit 6a31864 into servo:master Jul 31, 2018

3 of 4 checks passed

Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

Android MVP automation moved this from In progress to Done Jul 31, 2018

bors-servo added a commit that referenced this pull request Aug 1, 2018

Auto merge of #21199 - paulrouget:aar, r=MortimerGoro
Publish a AAR with servoview

Depends on #20912

Fix #21127

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] 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/21199)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment