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

Add/edge support #1151

Merged
merged 4 commits into from
Mar 1, 2018
Merged

Add/edge support #1151

merged 4 commits into from
Mar 1, 2018

Conversation

kekkokk
Copy link
Contributor

@kekkokk kekkokk commented Feb 9, 2018

In march will be released this fix:
Edge Support

This allows Licode to support it.
Note: the Edge publisher side in VP8 still sucks, even in p2p.
So for an "usable" environment we have to wait that Edge fixes its vp8 encoder

[] It needs and includes Unit Tests
[] It includes documentation for these changes in /doc.

this force adapterjs to use max bundle and generate a single ufrag for both transceivers.
need to set end-of-candidates in the sdp generated by Licode to avoid the adapterjs timeout.
this makes Edge to start ice immediately.
Copy link
Contributor

@jcague jcague left a comment

Choose a reason for hiding this comment

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

It looks good, have you tested this PR with Chrome/Firefox/Safari?

@@ -17,6 +17,7 @@ const BaseStack = (specInput) => {

that.pcConfig = {
iceServers: [],
bundlePolicy: 'max-bundle',
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tested this property with Chrome/Firefox? My only concern is not to break things that are working with the browsers that we currently support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this breaks p2p firefox, sorry I forgot to update the pr. this must be moved only in EdgeStack, maybe in a separate pr in the future.

@@ -68,7 +68,9 @@ function getMediaInfoFromDescription(info, sdp, mediaType) {

let ice = info.getICECredentials(mediaType);
if (ice) {
media.setICE(new ICEInfo(ice[0], ice[1]));
const thisIceInfo = new ICEInfo(ice[0], ice[1]);
thisIceInfo.setEndOfCandidates('end-of-candidates');
Copy link
Contributor

Choose a reason for hiding this comment

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

does this break something in Chrome/Firefox?

Copy link
Contributor Author

@kekkokk kekkokk Feb 20, 2018

Choose a reason for hiding this comment

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

this is bulletproof. Also should speed up the ice process a bit and seems a best practice

Choose a reason for hiding this comment

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

This is breaking things in chrome, local and remote stream turns black after merging this change

kekkokk and others added 2 commits February 20, 2018 12:08
apparently firefox doesn't like this in p2p. So will be moved in a separate stack in the future
@jcague jcague merged commit 8a745da into lynckia:master Mar 1, 2018
Arri98 pushed a commit to Arri98/licode that referenced this pull request Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants