Task/589/implement addtransceiver #625

agelito · 2021-01-11T11:44:07Z

Work in progress Branch implementing the addTransceiver function. Created scaffolding so far and have some TODO comments sprinkled in the code that I will deal with next.

Brief summary of things I need to deal with next (might have missed listing some):

Properly handle (void)peerConnection:(RTC_OBJC_TYPE(RTCPeerConnection) *)peerConnection didStartReceivingOnTransceiver:(RTC_OBJC_TYPE(RTCRtpTransceiver) *)transceiver;
Modify getTransceivers function to return list of transceivers.
Attach appropriate sender/receiver references to transceivers.
Update mid in case it's assigned after media stream negotiation.
Support adding transceiver using addTransceiver(kind)
Support sendEncodings in addTransceiver init.

* Create native class for transceivers. * Extend js class for transceivers. * Add RTCPeerConnection.addTransceiver. * Set up basic callback for sending native state to js.

* Invoke the native transceiver stop function. * Allow setting direction from JS. * State event for updating the JS state.

dgreif · 2021-01-11T15:16:32Z

@agelito thanks for taking this on. I had made some decent progress, but was waiting on #599 to be merged so that I could build it out in TypeScript. Now that iOS 14.3 has support for native access to getUserMedia, I'm less eager to get this knocked out. For reference, here's what I had done so far: https://github.com/calebboyd/cordova-plugin-iosrtc/commit/66258494d0e8c013ebb6a600d6bca8d01cffe3b7. The single hardest part is getting addTrack and addTransceiver to return synchronously. Since the calls to Swift are async, you don't get an opprotunity to call the backend and have Swift/C generate the track ids. Happy to answer any questions you have, but I'll let you take it from here 😄

hthetiot · 2021-01-11T15:36:50Z

Now that iOS 14.3 has support for native access to getUserMedia, I'm less eager

It might will work on cordova since cordova server from localhost and getUserMedia require https...

dunnky · 2021-01-11T15:44:09Z

Now that iOS 14.3 has support for native access to getUserMedia, I'm less eager

It might will work on cordova since cordova server from localhost and getUserMedia require https...

FWIW, the HTTPS requirement does appear to be changing soon WebKit/WebKit@ff60f0a

hthetiot · 2021-01-11T16:23:18Z

Now that iOS 14.3 has support for native access to getUserMedia, I'm less eager

It might will work on cordova since cordova server from localhost and getUserMedia require https...

FWIW, the HTTPS requirement does appear to be changing soon WebKit/WebKit@ff60f0a

This really bad idea I'm surprised that Apple does that...

agelito · 2021-01-12T00:30:02Z

@dgreif Thank you, looking at what you've done so far will be of big help. I'll be sure to ask any questions if they come up.

* Added PluginRTCRtpTransceiver#stringToDirection() * Fixed typo rendonly -> sendonly.

agelito · 2021-01-20T13:32:01Z

@hthetiot @dgreif Hello, I've made some additional progress with this, being able to do modify for example transceiver.sender.track.enabled flag in my local test application. I looked a lot at https://github.com/calebboyd/cordova-plugin-iosrtc/commit/66258494d0e8c013ebb6a600d6bca8d01cffe3b7# trying to follow it, since I still don't know well enough when transceiver state is expected to be updated.

Some other functions I still have to test/verify is:
transceiver.direction (set)
transceiver.sender.replaceTrack()
transceiver.stop()

One issue I have currently is keeping track of the tcId variable (used for referencing swift instances of transceiver from JS). It seems @dgreif was using transceiver.receiverId === update.receiver.track.id to keep track of them so I might explore that option instead as well.

hthetiot · 2021-01-20T18:17:17Z

Thank you @agelito this PR is scheduled for 8.0 you have a bit of time and we can releases 8 RCx for testing.

I also found this that may be interesting for you react-native-webrtc/react-native-webrtc#773

agelito · 2021-01-21T01:45:26Z

Thanks @hthetiot I'll check the react-native-webrtc PR as well. Today I'll do some more testing locally, and try to fix any remaining issues. Also see if I can clean up some of the code, especially the areas around addTransceiver (probably move some code out of constructor to make the function arguments in constructor more streamlined).

* Instead of having permanent ID's for the JS -> Transceiver association, just have transient ones until the next transceiver state update. Until better solution can be found. * Reduce complexity of transceiver constructor.

agelito · 2021-01-21T15:35:50Z

Today I've been reworking how the addTransceiver function works and how the ID for mapping JS -> Native is assigned. There's still room for improvements, for example reducing chance of ID collisions by not using random (perhaps plain index would be viable since the ordering of transceivers list isn't supposed to change anyway).

Apart from that I've also been testing, mostly with a local loopback peer connection, but also with a mediasoup based conferencing application. I still received one error on JS side when mediasoup tried to read sender parameters (I think, didn't get much of a callstack). So my natural next step will be to implement the sendEncodings init option and verify that the parameters can be retrieved from JS.

In the mediasoup based conferencing app I'm able to receive video data from a remote peer. But the remote peer isn't able to see video from the iOS device yet. I'll have to keep investigating...

My biggest issue today is that some webview worker seem to crash and reload after approximately 10-15 seconds. I'm not sure if this is because of the added transceivers or if it's because of something else. The only clue I have so far is that it seems to be related to DataChannels. I get tons of DataChannel.sendString log messages before the crash happens. 🤷‍♂️

I'll see if I can provide some actual log output tomorrow (writing from a different computer currently).

hthetiot · 2021-01-21T20:15:26Z

Thank you @agelito for the update.
I want to let you know I plan to split the plugin Inna library after 8.0 release and convince a group that already made a library from iosrtc to join the effort instead.

hthetiot · 2021-01-21T20:23:43Z

For reference: OpenTelecom/WKWebViewRTC#1 (comment)

* Update sender parameters along with transceivers. * Parse and pass along sendEncodings when doing addTransceiver.

agelito · 2021-01-22T09:24:10Z

After adding state update for the sender parameters I can both send and receive video/audio in video conferencing app based on MediaSoup (Safari12) adapter. This probably doesn't qualify as an extensive test for feature completeness though, but seems to be at least somewhat working.

The web view still crashes after around 10 seconds. So still need to investigate that more. Here's some log output I get on the device around when the crash happens, still no idea what might be causing it:
crash-logs.txt

agelito · 2021-01-27T04:12:32Z

I'm fairly confident the crash happens because of some memory leak. com.apple.WebKit.WebContent process reaches well over 1GB before being restarted. I will try to find out what the memory is used for, but my debugging skills on iOS isn't very good (especially for system services).

agelito · 2021-01-29T01:16:25Z

@hthetiot Small status update: I've done some more testing and it seems the crash I'm experienced is not directly related to the code in this PR. It seems to be the cordova.exec call leaking memory, and using MediaSoup that gets called several times each second.

If you're OK with it I can mark this PR ready for review soon?

hthetiot · 2021-01-29T02:40:43Z

@agelito did you identify what api call if called that may cause that, i asume might be getStats, we may implement semaphor

hthetiot · 2021-01-29T02:41:07Z

If you're OK with it I can mark this PR ready for review soon?

@agelito ok for me

hthetiot · 2021-01-29T03:28:14Z

@agelito did you manage this also ?

Properly handle (void)peerConnection:(RTC_OBJC_TYPE(RTCPeerConnection) *)peerConnection didStartReceivingOnTransceiver:(RTC_OBJC_TYPE(RTCRtpTransceiver) *)transceiver;

agelito · 2021-01-29T07:55:07Z

@hthetiot I haven't identified which API call causes it yet. I will keep investigating. In the application I'm using there's both simulcast and callstats.

Regarding (void)peerConnection:(RTC_OBJC_TYPE(RTCPeerConnection) *)peerConnection didStartReceivingOnTransceiver:(RTC_OBJC_TYPE(RTCRtpTransceiver) *)transceiver; I'm not sure exactly what should be done when that event happens. Perhaps I can send the transceivers state update when it happens in case any of the data changed. Do you have any idea?

agelito · 2021-02-01T03:07:47Z

@hthetiot The API called in iosrtc that causes crash seems to be RTCDataChannel.send if I comment out all the exec calls inside that function the app doesn't crash anymore.

olivierb2 · 2021-02-24T15:32:41Z

We just tested this branch with IOS ionic and mediasoup-client, this definitely fix the issue.

hthetiot · 2021-02-24T17:07:13Z

@hthetiot The API called in iosrtc that causes crash seems to be RTCDataChannel.send if I comment out all the exec calls inside that function the app doesn't crash anymore.

That may be a thread related issue or may be i never tested Data-channel on v87

hthetiot · 2021-02-26T13:53:41Z

@agelito Did you try remove the following "setImmediate(() => {" on dataChannel part of this commit:

e6a837d

agelito · 2021-02-26T15:10:07Z

@agelito Did you try remove the following "setImmediate(() => {" on dataChannel part of this commit:

e6a837d

Hey @hthetiot. Yes I did try that but it didn't make any difference 🤷‍♂️ . If you think it should be removed I can remove it.

Note: Although the reason of memory leak is still unknown, it was remedied in our case by removing Callstats library. Callstats was causing a lot of data being sent over data channels.

hthetiot · 2021-03-01T16:03:20Z

@agelito I merged master in 7.0.0 and then again in 8.0.0.
Its not using WebRTC.xcframework. can you test localy to merge and if all good you may sync 8.0.0 in your PR.

hthetiot · 2021-03-01T16:07:18Z

Here is a test branch with master+8+transceiver:

https://github.com/cordova-rtc/cordova-plugin-iosrtc/tree/task/8.0.0-implement-addtransceiver

hthetiot · 2021-03-01T16:25:57Z

Tested with Sylaps for iOS all good for me.

hthetiot · 2021-03-01T16:26:22Z

To test 8.0.0 see #545

hthetiot · 2021-03-01T16:37:40Z

8.0.0 landed on master

hthetiot · 2021-03-01T16:48:46Z

Next release will be 8.0.0.

@agelito feel free to make a PR to add your application here: https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/WHO_USES_IT.md

js/RTCPeerConnection.js

src/PluginRTCPeerConnection.swift

src/iosrtcPlugin.swift

src/PluginRTCPeerConnection.swift

src/PluginRTCRtpTransceiver.swift

hthetiot · 2021-03-01T23:41:56Z

@olivierb2 can you test master.
Thx

axel-calmid added 2 commits January 11, 2021 18:07

Basic transceiver scaffolding.

b7dbce0

* Create native class for transceivers. * Extend js class for transceivers. * Add RTCPeerConnection.addTransceiver. * Set up basic callback for sending native state to js.

Add support for stop and direction.

2db54be

* Invoke the native transceiver stop function. * Allow setting direction from JS. * State event for updating the JS state.

agelito mentioned this pull request Jan 11, 2021

Implement RTCPeerConnection.addTransceiver #589

Closed

hthetiot self-requested a review January 11, 2021 12:31

hthetiot assigned agelito Jan 11, 2021

hthetiot added enhancement webrtc-api webrtc-api related labels Jan 11, 2021

hthetiot added this to the 6.1.0 milestone Jan 11, 2021

axel-calmid added 4 commits January 12, 2021 13:40

Add string to direction utility function.

4a463d5

* Added PluginRTCRtpTransceiver#stringToDirection() * Fixed typo rendonly -> sendonly.

Create native and js transceiver on pc event.

6115210

Add support for addTransceiver(of:)

350aaf1

Associate transceivers with sender/receiver/track.

9a78663

hthetiot modified the milestones: 6.1.0, 8.0.0, 8.0.x Jan 20, 2021

axel-calmid added 2 commits January 21, 2021 23:03

Transient ID's for native transceivers.

dc9802c

* Instead of having permanent ID's for the JS -> Transceiver association, just have transient ones until the next transceiver state update. Until better solution can be found. * Reduce complexity of transceiver constructor.

Fix issue with undefined init object.

4c286a8

Update sender parameters state and parse sendEncodings.

e6a837d

* Update sender parameters along with transceivers. * Parse and pass along sendEncodings when doing addTransceiver.

hthetiot marked this pull request as ready for review January 29, 2021 03:27

hthetiot changed the base branch from task/8.0.0 to master February 26, 2021 13:54

hthetiot changed the base branch from master to task/8.0.0 February 26, 2021 13:55

hthetiot merged commit 0297b39 into cordova-rtc:task/8.0.0 Mar 1, 2021

hthetiot modified the milestones: 8.0.x, 8.0.0 Mar 1, 2021

hthetiot reviewed Mar 1, 2021

View reviewed changes

hthetiot changed the title ~~WIP: Task/589/implement addtransceiver~~ Task/589/implement addtransceiver Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task/589/implement addtransceiver #625

Task/589/implement addtransceiver #625

agelito commented Jan 11, 2021 •

edited

Loading

dgreif commented Jan 11, 2021

hthetiot commented Jan 11, 2021

dunnky commented Jan 11, 2021

hthetiot commented Jan 11, 2021 •

edited

Loading

agelito commented Jan 12, 2021

agelito commented Jan 20, 2021

hthetiot commented Jan 20, 2021

agelito commented Jan 21, 2021

agelito commented Jan 21, 2021

hthetiot commented Jan 21, 2021

hthetiot commented Jan 21, 2021

agelito commented Jan 22, 2021

agelito commented Jan 27, 2021

agelito commented Jan 29, 2021

hthetiot commented Jan 29, 2021

hthetiot commented Jan 29, 2021

hthetiot commented Jan 29, 2021

agelito commented Jan 29, 2021

agelito commented Feb 1, 2021

olivierb2 commented Feb 24, 2021

hthetiot commented Feb 24, 2021

hthetiot commented Feb 26, 2021 •

edited

Loading

agelito commented Feb 26, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

Task/589/implement addtransceiver #625

Task/589/implement addtransceiver #625

Conversation

agelito commented Jan 11, 2021 • edited Loading

dgreif commented Jan 11, 2021

hthetiot commented Jan 11, 2021

dunnky commented Jan 11, 2021

hthetiot commented Jan 11, 2021 • edited Loading

agelito commented Jan 12, 2021

agelito commented Jan 20, 2021

hthetiot commented Jan 20, 2021

agelito commented Jan 21, 2021

agelito commented Jan 21, 2021

hthetiot commented Jan 21, 2021

hthetiot commented Jan 21, 2021

agelito commented Jan 22, 2021

agelito commented Jan 27, 2021

agelito commented Jan 29, 2021

hthetiot commented Jan 29, 2021

hthetiot commented Jan 29, 2021

hthetiot commented Jan 29, 2021

agelito commented Jan 29, 2021

agelito commented Feb 1, 2021

olivierb2 commented Feb 24, 2021

hthetiot commented Feb 24, 2021

hthetiot commented Feb 26, 2021 • edited Loading

agelito commented Feb 26, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

hthetiot commented Mar 1, 2021

agelito commented Jan 11, 2021 •

edited

Loading

hthetiot commented Jan 11, 2021 •

edited

Loading

hthetiot commented Feb 26, 2021 •

edited

Loading