crow-misia / libmediasoup-android

libmediasoupclient for Android
Apache License 2.0
10 stars 9 forks source link

Nice piece of work... #14

Closed neilyoung closed 1 year ago

neilyoung commented 1 year ago

I came across your project after having a bit of a hard time with the "official mediasoup-android" lib, which seems to be abandoned.

This lib is stuck at M94, max M98, but that was it (since Google "decided" to no longer support the "use_custom_libcxx" flag).

So I was surprised to find this nugget here. Thanks for sharing. It took me 30 minutes to remove the old android libmediasoup solution in my Java project and replace it by your lib.

I have some minor important "issues" and two more severe:

Would be great if you could provide some hints, how to tackle this.

neilyoung commented 1 year ago

I found the reason for H.264 not working. It's a pretty nice bug chain. I'm willing to share details or even create a PR, if you are interested.

crow-misia commented 1 year ago

Excellent. Please share more details.

neilyoung commented 1 year ago

Oh, I just now found your add on regarding RTCComponentFactory and MediaConstraintsOption. This is different from how it was in this project https://github.com/haiyangwu/mediasoup-client-android. Maybe using this renders my patches obsolete, let me check that.

So please all following is with the proviso that the use of your supplementary classes does not shift the whole picture.

Anyway, I created my own PeerConnectionFactory (HardwareEncoders only) and provided this to

mMediasoupDevice = new Device(mPeerConnectionUtils.mPeerConnectionFactory);

My Kotlin is bad, but I thought this provided PeerConnectionFactory was not taken over into the local variable in the constructor of the device.

https://github.com/crow-misia/libmediasoup-android/blob/c8f74f5cad64c51381fbfa1d8fd9335db0ec4d8d/core/src/main/java/io/github/crow_misia/mediasoup/Device.kt#L151

I changed this, so that I have the custom PeerConnectionFactory object in the local variable of Device.kt of the same name. It is used later.

The following call:

mMediasoupDevice.load(String.valueOf(getRouterRtpCapabilityResponse));

finally ends up in nativeLoad, from which it dives into core/deps/libmediasoupclient/src/Device.cpp, function Load.

While all load functions up to here just took a string rtpCapabilities, now for the first time an additional PeerConnection::Options object appears, which is - according to the traces - a nullptr, because nobody was giving that to the function. But exactly this object is supposed to carry my custom peerConnectionFactory:

void Device::Load(json routerRtpCapabilities, const PeerConnection::Options* peerConnectionOptions)

In fact, the PeerConnectionFactory is currently provided to the lib a couple of lines later, when the "createSendTransport" is called. That means that it comes too late and all means to determine the local codec caps have been run already. H.264 hardware codec was ruled out already.

So I patched all responsible files and headers to carry the PeerConnectionFactory to the final load function in core/deps/libmediasoupclient/src/Device.cpp earlier. The patch is for sure not that elegant, since it re-uses JavaToNativeOptions and needs the rtcConfig for that, but I guess you will see, what my aim was. It works for me.

While reviewing this, I think now, that even if I would use your way of dealing with the PeerConnectionFactory (mentioned above), it would come to the same end, if not changed like so or similar.

Here is the patch.

diff --git a/core/src/main/java/io/github/crow_misia/mediasoup/Device.kt b/core/src/main/java/io/github/crow_misia/mediasoup/Device.kt
index 3d64c9a..f377312 100644
--- a/core/src/main/java/io/github/crow_misia/mediasoup/Device.kt
+++ b/core/src/main/java/io/github/crow_misia/mediasoup/Device.kt
@@ -1,6 +1,7 @@
 package io.github.crow_misia.mediasoup

 import org.webrtc.PeerConnection
+import org.webrtc.PeerConnection.RTCConfiguration
 import org.webrtc.PeerConnectionFactory

 /**
@@ -30,9 +31,9 @@ class Device(
     /**
      * Initialize the Device.
      */
-    fun load(routerRtpCapabilities: String) {
+    fun load(routerRtpCapabilities: String, configuration: RTCConfiguration) {
         checkDeviceExists()
-        nativeLoad(nativeDevice, routerRtpCapabilities)
+        nativeLoad(nativeDevice, routerRtpCapabilities, configuration, this.peerConnectionFactory.nativePeerConnectionFactory )
     }

     /**
@@ -119,7 +120,7 @@ class Device(
     private external fun nativeIsLoaded(nativeDevice: Long): Boolean
     private external fun nativeGetRtpCapabilities(nativeDevice: Long): String
     private external fun nativeGetSctpCapabilities(nativeDevice: Long): String
-    private external fun nativeLoad(nativeDevice: Long, routerRtpCapabilities: String)
+    private external fun nativeLoad(nativeDevice: Long, routerRtpCapabilities: String, configuration: PeerConnection.RTCConfiguration, peerConnectionFactory: Long)
     private external fun nativeCanProduce(nativeDevice: Long, kind: String): Boolean
     private external fun nativeCreateSendTransport(
         nativeDevice: Long,
@@ -148,6 +149,6 @@ class Device(
     ): RecvTransport
 }

-fun PeerConnectionFactory.createDevice(): Device {
-    return Device(this)
+fun PeerConnectionFactory.createDevice(peerConnectionFactory: PeerConnectionFactory): Device {
+    return Device(peerConnectionFactory)
 }
\ No newline at end of file
diff --git a/core/src/main/jni/device.cpp b/core/src/main/jni/device.cpp
index c50836e..d40a588 100644
--- a/core/src/main/jni/device.cpp
+++ b/core/src/main/jni/device.cpp
@@ -75,13 +75,16 @@ extern "C"
       .value_or(nullptr);
   }

-  JNI_DEFINE_METHOD(void, Device, nativeLoad, jlong j_device, jstring j_routerRtpCapabilities)
+  JNI_DEFINE_METHOD(void, Device, nativeLoad, jlong j_device, jstring j_routerRtpCapabilities, jobject j_configuration, jlong j_peerConnectionFactory)
   {
     MSC_TRACE();

     handleNativeCrashNoReturn(env, [&]() {
       auto capabilities = JavaToNativeString(env, JavaParamRef<jstring>(env, j_routerRtpCapabilities));
-      reinterpret_cast<Device*>(j_device)->Load(json::parse(capabilities));
+        PeerConnection::Options options;
+        JavaToNativeOptions(env, JavaParamRef<jobject>(env, j_configuration), j_peerConnectionFactory, options);
+
+        reinterpret_cast<Device*>(j_device)->Load(json::parse(capabilities), &options);
     });
   }

diff --git a/core/src/main/jni/device.h b/core/src/main/jni/device.h
index 64e260f..278d532 100644
--- a/core/src/main/jni/device.h
+++ b/core/src/main/jni/device.h
@@ -26,7 +26,7 @@ extern "C"

   JNI_DEFINE_METHOD(jstring, Device, nativeGetSctpCapabilities, jlong j_device);

-  JNI_DEFINE_METHOD(void, Device, nativeLoad, jlong j_device, jstring j_routerRtpCapabilities);
+  JNI_DEFINE_METHOD(void, Device, nativeLoad, jlong j_device, jstring j_routerRtpCapabilities, jobject j_configuration, jlong j_peerConnectionFactory);

   JNI_DEFINE_METHOD(jboolean, Device, nativeCanProduce, jlong j_device, jstring j_kind);
crow-misia commented 1 year ago

Released v0.13.0 with your fixes.

Added RTCConfiguration to load method.

The PeerConnectionFactory.createDevice method is just a helper method, so there is no point in adding parameters. It is equivalent to new Device(peerConnectionFactory).

neilyoung commented 1 year ago

Thanks. Will give it a try.

The PeerConnectionFactory.createDevice method is just a helper method, so there is no point in adding parameters. It is equivalent to new Device(peerConnectionFactory)

I agree. If the peerConnectiomFactory given in the constructor is available during the Device.load function, there is no need to provide it otherwise. But IMHO this was not the case before my admittedly clumsy patch. I know this for sure since I was dependent on a hardware encoder, which I instantiated in my peerConnectiomFactory. That was not available at nativeLoad.

neilyoung commented 1 year ago

Perfect. That works. Thanks for fixing it.

Two things, though:

These lines for instance

        WebRtcLogger.d(TAG, "decoderFactory: %s", decoderFactory)
        WebRtcLogger.d(TAG, "encoderFactory: %s", encoderFactory)

        decoderFactory.supportedCodecs.forEach {
            WebRtcLogger.d(TAG, "decoderFactory supported codec: %s %s", it.name, it.params)
        }
        encoderFactory.supportedCodecs.forEach {
            WebRtcLogger.d(TAG, "encoderFactory supported codec: %s %s", it.name, it.params)
        }

are rendering this output:

07-03 12:03:13.128 25089 25089 D VotixSoup-RTCComponentFactory: decoderFactory: %s
07-03 12:03:13.128 25089 25089 D VotixSoup-RTCComponentFactory: encoderFactory: %s
07-03 12:03:13.141 25089 25089 D VotixSoup-RTCComponentFactory: decoderFactory supported codec: %s %s
07-03 12:03:13.141 25089 25089 D VotixSoup-RTCComponentFactory: decoderFactory supported codec: %s %s
07-03 12:03:13.146 25089 25089 D VotixSoup-RTCComponentFactory: encoderFactory supported codec: %s %s
07-03 12:03:13.146 25089 25089 D VotixSoup-RTCComponentFactory: encoderFactory supported codec: %s %s

and some more samples available, but I'm sure you see it :)

crow-misia commented 1 year ago

What value is in the parameter logHandler of MediasoupClient.initialize?

I checked with io.github.crow_misia.webrtc.log.DefaultLogHandler and it was replaced without problems.

If you have implemented a custom LogHandler, please check your implementation

2023-07-05 18:13:06.163 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  decoderFactory: org.webrtc.DefaultVideoDecoderFactory@831cabb
2023-07-05 18:13:06.163 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  encoderFactory: org.webrtc.DefaultVideoEncoderFactory@44948b5
2023-07-05 18:13:06.273 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  decoderFactory supported codec: VP8 {}
2023-07-05 18:13:06.273 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  decoderFactory supported codec: VP9 {}
2023-07-05 18:13:06.273 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  decoderFactory supported codec: AV1 {}
2023-07-05 18:13:06.273 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  decoderFactory supported codec: H264 {level-asymmetry-allowed=1, profile-level-id=42e01f, packetization-mode=1}
2023-07-05 18:13:06.274 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  encoderFactory supported codec: VP8 {}
2023-07-05 18:13:06.274 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  encoderFactory supported codec: VP9 {}
2023-07-05 18:13:06.274 24699-24699 RTCComponentFactory     org.mediasoup.droid.demo             D  encoderFactory supported codec: AV1 {}
neilyoung commented 1 year ago
       LogHandler logHandler = new LogHandler() {
            @Override
            public void log(int priority, @Nullable String tag, @Nullable Throwable throwable, @Nullable String message, @NonNull Object... objects) {
               if (debug)
                   Log.d("VotixSoup-"+tag, message);
            }
        };

       MediasoupClient.initialize(this, logHandler, false, null,  Logging.Severity.LS_NONE);

Edit: I suppose I have to ack the objects...

neilyoung commented 1 year ago

Yepp. Then it works. Thanks