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

Android embedding v2 #274

Merged
merged 6 commits into from Jun 14, 2020
Merged

Conversation

ened
Copy link
Contributor

@ened ened commented Jun 7, 2020

This migrates the Android side of the plugin to the new Embedding V2 API.

  • The V2 APIs make lifecycle a lot more explicit to plugins, thus should allow for improve code structure and predictable memory usage
  • There may have been a memory leak observed due to missing cleanup methods in the previous implementation (I've been testing with "leakcanary" and "Don't Keep Activities" both set to on
  • The plugin will now lazily initialize the audio manager and most of the RTC classes on the first method call invocation (important when in a multi-engine environment, e.g. when a App utilizes firebase_messaging & flutter-webrtc)
  • Small refactors and decouplings have been done

Functionality has been tested locally on a few phones, but I'll appreciate if others can test as well.

Note: My Android Studio installation probably formats the code different than other contributors. Could the project agree on a standard set of rules? Most of the official flutter code uses google-java-format (a plugin in Android Studio) with the Google rules. Maybe this project could use it, too.

@cloudwebrtc
Copy link
Member

@ened Thank you for your contribution, I will test your modification and merge it without error.

@ened
Copy link
Contributor Author

ened commented Jun 10, 2020

@cloudwebrtc how is your progress? :-)

@cloudwebrtc
Copy link
Member

@ened Just now I did some tests. When I lowered the Android SDK API level to 21 or below, GetUserMedia could not get the camera stream. Does it seem that the SDK migration process caused a bug? The loopback example also doesn't work.
But I use a higher API level, for example, 29, it works fine.

@ened
Copy link
Contributor Author

ened commented Jun 11, 2020

I had some some tests successfully on older Android versions but not extensively.

Can you please share the steps you took exactly and I will try to reproduce them here. Have about 7 different phones to test with, starting on Android 6.

What do you mean "lowered the Android SDK API level to 21"? It means you just ran a test on older phones, right? No code change was involved, right?

Regarding the bug itself, I am 100% sure; it should not have been caused by this, but will test.

@cloudwebrtc
Copy link
Member

I used AVD Manager to create an emulator and selected Android 5.0 (Google APIs) x86_64 bit API 21 image. It seems that I cannot complete the GetUserMedia action, but the master
Version can pop up a permission request confirmation dialog, I guess the permission request process is not performed correctly.

~$ flutter doctor
Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel master, 1.19.0-6.0.pre.57, on Mac OS X 10.15.5 19F101, locale zh-Hans-CN)
 
[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.3)
[✓] Xcode - develop for iOS and macOS (Xcode 11.2.1)
[✓] Chrome - develop for the web
[✓] Android Studio (version 4.0)
[✓] VS Code (version 1.46.0)
[✓] Connected device (4 available)

• No issues found!

@ened
Copy link
Contributor Author

ened commented Jun 13, 2020

@cloudwebrtc ok - I was able to reproduce the problem on a 5.1 device.

Will try to rework the permission handling a bit more and check the loopback example.

@ghenry
Copy link

ghenry commented Jun 13, 2020 via email

@ened
Copy link
Contributor Author

ened commented Jun 13, 2020

@cloudwebrtc can you please share the android studio editor settings this project is using

@ened
Copy link
Contributor Author

ened commented Jun 13, 2020

@ghenry Samsung J3 comes to mind. Because WebRTC also supports audio-only or data channel operation, it’s not completely out of the equation, yet.

@ened
Copy link
Contributor Author

ened commented Jun 13, 2020

@cloudwebrtc have tested again & hopefully fixed the bug rgd. the permissions on pre-M devices.

Successfully tested the example App on these phones:

samsung - crownltexx - SMN960F - 10 (API 29)
samsung - j3xnltexx - SMJ320FN - 511 (API 22)
samsung - kltexx - SMG900F - 601 (API 23)
Sony - F8132 - F8132 - 800 (API 26)
Sony - G3311 - G3311 - 70 (API 24)

This PR now also bumped the webrtc dependency.

@cloudwebrtc
Copy link
Member

@ened I tested API 21 and can capture the camera, I will merge it now.

@cloudwebrtc cloudwebrtc merged commit d61e62b into flutter-webrtc:master Jun 14, 2020
@cloudwebrtc
Copy link
Member

@cloudwebrtc can you please share the android studio editor settings this project is using

I usually use VSCode to directly edit the code, and Android Studio will be started unless necessary for native debugging, so I use the default settings. xD

@ened
Copy link
Contributor Author

ened commented Jun 14, 2020

@cloudwebrtc thx for merging & understood rgd your process.

If you don’t mind I’d reformat using the flutter recommendation (Google-Java-format), and adjust readme and CI scripts. That should make future PRs a bit easier to read, as typically everyone has a different configuration.

@cloudwebrtc
Copy link
Member

cloudwebrtc commented Jun 14, 2020

@ened No problem, we can use standard formats like google (clang-format or java-format) to format the code.

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.

None yet

3 participants