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

Implement RTCPeerConnection.addTransceiver #589

Closed
hthetiot opened this issue Oct 17, 2020 · 20 comments
Closed

Implement RTCPeerConnection.addTransceiver #589

hthetiot opened this issue Oct 17, 2020 · 20 comments
Assignees
Labels
Milestone

Comments

@hthetiot
Copy link
Contributor

hthetiot commented Oct 17, 2020

Versions affected

  • Cordova version (e.g 7.1.0): N/A
  • Cordova iOS version (e.g 5.1.0): N/A
  • Plugin version (e.g 6.0.12): 6.0.14
  • iOS version (e.g 10.2): N/A
  • Xcode version (e.g 11.1 - 11A1027): N/A
  • WebRTC-adapter version (e.g. 7.4.0): N/A
  • WebRTC Framework version (e.g. JSSip 3.1.2): N/A

Description

Currently iosRTC DOES NOT support addTransceiver.

Expected results

Be able to use addTransceiver.

Actual results

addTransceiver is not a fonction.

References:

@hthetiot hthetiot added this to the 6.0.x milestone Oct 17, 2020
@hthetiot
Copy link
Contributor Author

@hthetiot
Copy link
Contributor Author

Related: w3c/webrtc-pc#2024

@hthetiot
Copy link
Contributor Author

Related

- (void)peerConnection:(RTCPeerConnection *)peerConnection

See didStartReceivingOnTransceiver:(RTCRtpTransceiver and use of RTCRtpTransceiver that need to be implemented in PluginRTCConnection

@dgreif
Copy link
Contributor

dgreif commented Oct 30, 2020

@hthetiot thanks for all the reference material. I'm planning to start diving into this today.

@hthetiot
Copy link
Contributor Author

'peerConnection(:didStartReceivingOnTransceiver:)' has been renamed to 'peerConnection(:didStartReceivingOn:)'
Replace 'didStartReceivingOnTransceiver' with 'didStartReceivingOn'

'peerConnection(:didAddReceiver:streams:)' has been renamed to 'peerConnection(:didAdd:streams:)'
Replace 'didAddReceiver' with 'didAdd'

@hthetiot
Copy link
Contributor Author

hthetiot commented Nov 10, 2020

func peerConnection(_ peerConnection: RTCPeerConnection, didStartReceivingOn rtpTransceiver: RTCRtpTransceiver) {
        
        NSLog("PluginRTCPeerConnection | didStartReceivingOn rtpTransceiver")
 }

@hthetiot
Copy link
Contributor Author

Other RTCPeerConnection property and methods that you can use are:

/** Gets all RTCRtpTransceivers associated with this peer connection.
 *  Note: reading this property returns different instances of
 *  RTCRtpTransceiver. Use isEqual: instead of == to compare RTCRtpTransceiver
 *  instances.
 *  This is only available with RTCSdpSemanticsUnifiedPlan specified.
 */
@property(nonatomic, readonly) NSArray<RTCRtpTransceiver *> *transceivers;

/** Adds a transceiver with a sender set to transmit the given track. The kind
 *  of the transceiver (and sender/receiver) will be derived from the kind of
 *  the track.
 */
- (RTCRtpTransceiver *)addTransceiverWithTrack:(RTCMediaStreamTrack *)track;
- (RTCRtpTransceiver *)addTransceiverWithTrack:(RTCMediaStreamTrack *)track
                                          init:(RTCRtpTransceiverInit *)init;

/** Adds a transceiver with the given kind. Can either be RTCRtpMediaTypeAudio
 *  or RTCRtpMediaTypeVideo.
 */
- (RTCRtpTransceiver *)addTransceiverOfType:(RTCRtpMediaType)mediaType;
- (RTCRtpTransceiver *)addTransceiverOfType:(RTCRtpMediaType)mediaType
                                       init:(RTCRtpTransceiverInit *)init;

@dgreif
Copy link
Contributor

dgreif commented Nov 10, 2020

Thanks for the additional info @hthetiot. I started diving into the code for this yesterday, wrapping my head around all the changes needed. Should make some good progress today.

@dgreif
Copy link
Contributor

dgreif commented Nov 13, 2020

@hthetiot sorry for the delay, I took a day off and then had to rework this a couple times to get it as close to spec as possible. I'm very close, but hit an issue when compiling that I can't find a fix for. I'm guessing it's related to the webrtc header files (maybe related to missing RTC_EXPORT on RTCRtpTransceiverInit), but I don't use swift enough to know what might be causing this:

image

This shows up if I try to cast to the RTCRtpTransceiverInit interface, which I need to do when casting my command arguments in iosrtcPlugin.swift and in addTransceiver in PluginRTCPeerConnection.swift. Any idea what might be causing that interface to not be accessible when compiling?

@hthetiot
Copy link
Contributor Author

@hthetiot sorry for the delay, I took a day off and then had to rework this a couple times to get it as close to spec as possible.

No need to apologise, take all the time you need.

Regarding RTCRtpTransceiverInit not sure I can help without running the PR myself feel free to push a PR with Work in progress.

Another point I want to raise is you may check that task/8.0.0 branch since it contains a more recent version of WebRTC.

@dgreif
Copy link
Contributor

dgreif commented Nov 15, 2020

The 8.0.0 header file for transceivers does include the RTC_OBJC_EXPORT annotation for RTCRtpTransceiverInit, so hopefully that means it's properly set up for use.

If you want to try debugging it a little in v6, you can add this line to any of the bridge methods in iosrtcPlugin.swift:

let fakeInit = command.argument(at: 5) as! RTCRtpTransceiverInit

Even if you don't use fakeInit, the cast is enough to get the compiler to look for the class and fail the build. Let me know if you find anything this weekend. Otherwise, I'll definitely try rebasing on the 8.0.0 branch Monday

@dgreif
Copy link
Contributor

dgreif commented Nov 16, 2020

@hthetiot I confirmed it builds successfully when rebased on the 8.0.0 branch. Based on the 8.0.0 requirement, and the fact that I used typescript for the front end files, I'm not sure exactly how I should PR this. Would you like me to do it against the 8.0.0 branch and include everything in master + typescript?

@hthetiot
Copy link
Contributor Author

@dgreif we could step 1 release Typescript in 6.1.0, step 2 rebase master in tasks/8.0.0 then merge your Transceiver.

Did you try 7.0.0.

@hthetiot
Copy link
Contributor Author

Would you like me to do it against the 8.0.0 branch and include everything in master + typescript?

We can do that.

@hthetiot
Copy link
Contributor Author

@dgreif This PR will land on master soon and may impact your Transceiver implementation depending how you coupled it with tracks.

#605

@hthetiot
Copy link
Contributor Author

@dgreif let me know if you want me to help. Feel free to push draft PR of work in progress.

@agelito
Copy link

agelito commented Jan 11, 2021

@hthetiot @dgreif Hello, I've been trying to implement support for addTransceiver as well. I've made some WIP progress (based of task/8.0.0 branch) and wonder if any of you would like to take a look at it? I'm not very experienced with neither Cordova or WebRTC in general, but I've been trying to follow the existing code style and practices.

I don't want to step on anyone's toes (@dgreif ) but since there's not been any activity in this issue since last November I went ahead and started.

Here's a link to the diff of branch I'm working in:
task/8.0.0...agelito:task/589/implement-addtransceiver

What I've done so far:

  • Add addTransceiver function JS -> native.
  • Initial state sent back from native -> JS.
  • Add RTCRtpTransceiver.stop() function.
  • Add RTCRtpTransceiver.direction property.

What I think is remaining:

  • Support sendEncodings in addTransceiver init
  • Support addTransceiver(kind)
  • Couple transceivers with sender/receiver instances.
  • Fix RTCRtpPeerConnection.getTransceivers (currently it creates transceivers based on list of senders/receivers)
  • Update state in JS from native when appropriate (for example direction flag changes or stopped changes)

@hthetiot hthetiot assigned agelito and unassigned dgreif Jan 11, 2021
@hthetiot
Copy link
Contributor Author

thx @agelito feel free to make PR

@agelito
Copy link

agelito commented Jan 11, 2021

@hthetiot Thanks, I created this draft PR: #625

@hthetiot hthetiot removed this from the 8.0.0 milestone Jan 20, 2021
@hthetiot hthetiot added this to the 8.0.x milestone Jan 20, 2021
@hthetiot
Copy link
Contributor Author

hthetiot commented Mar 1, 2021

Landed on task/8.0.0 branch see #545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants