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

Replace old APIs with explicit addTransceivers #50

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

lherman-cs
Copy link
Contributor

Issue #49

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@MixMasterMitch MixMasterMitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes themselves look good, but my concern is around browser compatibility.

addTransceiver is not as well supported (FF 59, Chrome 69, Edge 79) as createAnswer is (FF 22, Chrome 51, Edge 15). What can we do about this? Does adaptor.js handle this?

@MixMasterMitch
Copy link
Contributor

Also, another comment: in the README there is a section demonstrating creating an offer/answer. Please update that part too.

@lherman-cs
Copy link
Contributor Author

The changes themselves look good, but my concern is around browser compatibility.

addTransceiver is not as well supported (FF 59, Chrome 69, Edge 79) as createAnswer is (FF 22, Chrome 51, Edge 15). What can we do about this? Does adaptor.js handle this?

That's a good point. AFAIK, adapter.js doesn't handle this. I'll spend some time to see if there's a good solution for this. I think this should be a common issue.

@hassanctech
Copy link
Contributor

Looking here: https://webrtchacks.github.io/adapter/adapter-latest.js I do see addTransceiver here, is there anything else we need or is this ready to merge now, @MixMasterMitch ?

@sirknightj
Copy link
Contributor

addTransceiver is not as well supported (FF 59, Chrome 69, Edge 79) as createAnswer is (FF 22, Chrome 51, Edge 15). What can we do about this? Does adaptor.js handle this?

As of today, addTransceiver is supported by all except opera, and createAnswer is supported by all.

@niyatim23 niyatim23 changed the base branch from master to develop December 5, 2023 16:49
@sirknightj
Copy link
Contributor

I tried this locally.. it causes the ICE candidate pairs to not get printed correctly (tested on Chrome). I tried the regular P2P and also WebRTC ingestion, both with the same result.

Current JS test page:

[2023-12-05T23:29:22.522Z] [DEBUG] Chosen candidate pair (video): {
  "local": {
    "candidate": "candidate:55146457 1 udp 1686052607 76.146.x.x 53818 typ srflx raddr 192.168.x.x rport 53818 generation 0 ufrag Zkze network-id 1 network-cost 10",
    "sdpMid": "",
    "sdpMLineIndex": 0,
    "usernameFragment": "Zkze"
  },
  "remote": {
    "candidate": "candidate:7 1 udp 503316991 34.221.x.x 53685 typ relay raddr 10.0.x.x rport 36972 generation 0 ufrag TLaJSljd2IHYAwZbqcaDxD3L5xK0EWfN",
    "sdpMid": "",
    "sdpMLineIndex": 0,
    "usernameFragment": "TLaJSljd2IHYAwZbqcaDxD3L5xK0EWfN"
  }
}

Changes from this PR:

[2023-12-05T23:30:29.422Z] [ERROR] Failed to fetch the candidate pair!

Looks like the way the candidate pair gets logged needs to change as well to go with this approach.

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

Successfully merging this pull request may close these issues.

None yet

5 participants