-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add JVM Support #103
base: main
Are you sure you want to change the base?
Add JVM Support #103
Conversation
@aschulz90 great job. Let me take some time to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a couple of comments.
webrtc-kmp/src/jvmMain/kotlin/com/shepeliev/webrtckmp/LocalAudioStreamTrack.kt
Outdated
Show resolved
Hide resolved
webrtc-kmp/src/jvmMain/kotlin/com/shepeliev/webrtckmp/PeerConnectionExt.kt
Outdated
Show resolved
Hide resolved
webrtc-kmp/build.gradle.kts
Outdated
name = deps.webrtc.java.get().name, | ||
version = deps.webrtc.java.get().version, | ||
classifier = "macos-x86_64" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got on my Mac M2
java.lang.UnsatisfiedLinkError: /private/var/folders/sp/8h47vr6s72v4hngsrxrfwpzm0000gn/T/libwebrtc-java1779612920915696582.dylib: dlopen(/private/var/folders/sp/8h47vr6s72v4hngsrxrfwpzm0000gn/T/libwebrtc-java1779612920915696582.dylib, 0x0001): tried: '/private/var/folders/sp/8h47vr6s72v4hngsrxrfwpzm0000gn/T/libwebrtc-java1779612920915696582.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64')), '/System/Volumes/Preboot/Cryptexes/OS/private/var/folders/sp/8h47vr6s72v4hngsrxrfwpzm0000gn/T/libwebrtc-java1779612920915696582.dylib' (no such file), '/private/var/folders/sp/8h47vr6s72v4hngsrxrfwpzm0000gn/T/libwebrtc-java1779612920915696582.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64'))
But it's okay if I comment this declaration. I'll try to figure out how to solve this if you don't have M1/M2 at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have a M1 Mac Mini, so no camera or microphone to test. But also got it at least running, by, like you mentioned, removing the x86 dependency.
I think adding this native dependencies would need to be done by the implementing application. It still worked after I removed the import from the library build.gradle and added it to the sample app build.gradle.
I will remove the import from the library and update the README to reflect that it is neccessary for implementing applications to add the right dependency on their own (like you need to add the WebRTC pod to iOS apps right now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to leave import with classifier in order to run JVM tests in CI. I guess we could solve it as following:
val osName = System.getProperty("os.name").lowercase()
val hostOS = if (osName.contains("mac")) {
"macos"
} else if (osName.contains("linux")) {
"linux"
} else if (osName.contains("windows")) {
"windows"
} else {
throw IllegalStateException("Unsupported OS: $osName")
}
val hostArch = System.getProperty("os.arch").lowercase()
jvmMainApi(deps.webrtc.java)
jvmMainImplementation(
group = deps.webrtc.java.get().group!!,
name = deps.webrtc.java.get().name,
version = deps.webrtc.java.get().version,
classifier = "$hostOS-$hostArch"
)
It works well on my Mac. However, I'd like to test it on Windows/Linux too. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the CI tests in this repo? Then as jvmTestImplementation
? I am not that familiar with publishing artifacts and am not sure what the published artifact with one of them defined as a dependency would look like.
But maybe the webrtc-kmp
jvm artifact could also be published with every classifier and dependency separately instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add a switch for the architecture, because "os.arch" returned "amd64" on my windows device.
val hostArch = when(val arch = System.getProperty("os.arch").lowercase()) {
"amd64" -> "x86_64"
else -> arch
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move tests to the commonTest
source set so that they run on all platforms in CI.
However, I think we'll have problem with MediaDevicesTests
. It will fail on different platforms as Android, iOS and Mac require permissions to access microphone or camera. Perhaps, we could leave it but add @Ignore
annotation for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. The MediaDevicesTests
where just fo testing the implementation.
I've tried to run the JVM sample app on Mac. Unfortunately, there is exception getting user media:
Also, JVM tests fail on Mac with It's really great to add JVM platform, however I think we should make it work properly on any OS (windows, macos, linux). |
@shepeliev @aschulz90 Any updates on this? |
@tamimattafi |
@aschulz90 Hello! I have both Mac on Intel and M1, I can help on Saturday if it's suitable for you |
@aschulz90 We tried on both M1 and Intel macs, here's a report: 1. Sample run from
|
@tamimattafi thanks for the tests. Unfortunately, I don't have any quick answer for these issues on mac. It definitely requires more researching. I'll back to this as soon as be able. |
Yeah, I haven't accounted for permissions on desktop. A quick search wasn't that conclusive on how to request them for java applications on macOS. Unfortunately I don't think I can be of much help in that area. |
@tamimattafi - Are you using OpenJDK by any chance? And perhaps running this from the IDE and not a built package? Check this: https://bugs.openjdk.org/browse/JDK-8272639 They've used the
|
@tricknology Yes indeed, I'm using OpenJDK 20 on my machine, and JetBrains Runtime version 17 on the IDE. And indeed I run the sample from My guess is, as you stated that it is better to build a package and install it properly for proper testing with permissions. |
@tamimattafi I feel like there should be a way to pass options to the compiler in run config. You might also specify a makefile to do the same.. unfortunately I'm not exactly sure on the step-by-step. |
This should be fixed now. |
@its-Chiedu There is a way to pass every detail including info.plist However, I'm having some issues building a .dmg using java 17 and java 20, I will look through the issue when I find some free time. |
@shepeliev I have merged the latest changes from |
Unfortunately, still have an exception on both of Intel and M2 macs:
I guess some problem is in native WebRTC SDK that embeded with WebRTC java lib.
I think we could start with the first option for now. Java background devs help is appreciated :) |
Any updates? 🤔 |
Probably still blocked by requiring a way to request camera/microphone permissions on macOS JVM. |
I only tested this on a Windows PC connecting with an Android device. Video/Screensharing and Audio works with the sample apps. DataChannel was only tested via Junit.
Since this relies on webrtc-java for native interop, it should theoretically work on any platform it supports as well.