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

Generic RTCPeerConnection constraint handling #119

Closed
wants to merge 1 commit into from
Closed

Generic RTCPeerConnection constraint handling #119

wants to merge 1 commit into from

Conversation

oNaiPs
Copy link
Contributor

@oNaiPs oNaiPs commented Jan 14, 2016

This commit makes rtc peer connection constraint handling generic. Also it makes it according to the spec, as previously the following dictionary would fail:

var contraints = {
        mandatory: {
            OfferToReceiveVideo: true,
            OfferToReceiveAudio: true
        },
        optional: ...
    };

@ibc
Copy link
Collaborator

ibc commented Jan 14, 2016

I've my concerns about this PR. "according to the spec" is no true as the new spec does no longer use mandatory/opcional, and instead of mandatory.OfferToReceiveAudio it defines offerToReceiveAudio.

Said that, it is also true that the current libwebrtc ObjC implementation still uses the old syntax, but I prefer to move on.

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Jan 14, 2016

Got it. My thinking was that you would be looking at the current Chrome WebRTC spec so we could use same code for iOS or Android (using crosswalk or not).
You're free to close the branch if this is not going the way you want the project to go.
Thanks

@ibc
Copy link
Collaborator

ibc commented Jan 14, 2016

I don't want to close the PR, but instead think about how to make both specs work together. For example, we could use the current libwebrtc ObjC API (mandatory&optional) within the plugin Swift code, and let the JavaScript layer to do the magic. In that way we can accept both the new and old syntax and convert the user given constraints to the old syntax in the JS layer before passing them to the Swift code.

Do you agree?

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Jan 14, 2016

That sounds awesome!

@ibc
Copy link
Collaborator

ibc commented Jan 14, 2016

OK, then let me do this next week. I think that your PR is 100% valid with the suggested design.

@mark-veenstra
Copy link
Contributor

@ibc do you have the time to look at this PR this week?

@ibc
Copy link
Collaborator

ibc commented Mar 8, 2016

Unfortunately merging this PR would require some extra minor changes in the plugin JS layer and also in some of my apps at work, and I do not have time for it in the short term. This is not a "not", but a "later".

@mark-veenstra
Copy link
Contributor

@ibc it seems that this PR solves all of our issues as described in https://groups.google.com/forum/#!topic/cordova-plugin-iosrtc/LXAxyw0x2Wo.
And see next repository: https://github.com/Mobilea/kurento-presenterviewer-ios-withutils

@silkroadnomad
Copy link

I am hunting a similar issue at the moment but have to admit that as stated in the test repository
Being a presenter on Firefox and a viewer in iOS does work
doesn't work for me in production circumstances with TURN and STUN in the public internet.
Have you been testing this too? I will elaborate on that later more and will checkout your PR this week. Thanks!

@mark-veenstra
Copy link
Contributor

@inspiraluna I have been testing this on internet too, but then with the commercial version of Kurento which is in beta now. It did work when being a only a presenter or only a viewer.
But using both still has some issues. We are trying to tackle these issues as we speak. We also are looking into the STUN/TURN options, because it looks related. We are sending the following constraints:

Change the var options here https://github.com/Mobilea/kurento-presenterviewer-ios-withutils/blob/master/www/js/withutils/index.js#L90 to:

      var options = {
        localVideo: videoPresenter,
        onicecandidate : onIceCandidatePresenter
        configuration: {
            iceServers: [
                {
                    'urls': 'turn:XX.XX.XX.XX',
                    'username': 'user',
                    'credential': 'pass'
                },
                {
                    'urls': 'turn:XX.XX.XX.XX?transport=tcp',
                    'username': 'user',
                    'credential': 'pass'
                }
            ],
            'iceTransportPolicy': 'relay'
        }
      };

And change the var options here https://github.com/Mobilea/kurento-presenterviewer-ios-withutils/blob/master/www/js/withutils/index.js#L116 to:

      var options = {
        localVideo: videoViewer,
        onicecandidate : onIceCandidateViewer
        configuration: {
            iceServers: [
                {
                    'urls': 'turn:XX.XX.XX.XX',
                    'username': 'user',
                    'credential': 'pass'
                },
                {
                    'urls': 'turn:XX.XX.XX.XX?transport=tcp',
                    'username': 'user',
                    'credential': 'pass'
                }
            ],
            'iceTransportPolicy': 'relay'
        }
      };

It seems to get a bit better, but still not quite there yet.
On the kurento mailing list you also said that you see missing TURN/STUN connection. Where did you see this? On Kurento Media Server side? Or in iosrtc?

@silkroadnomad
Copy link

I was working with the eface2face fork of iosrtc and was not aware you have another one. I tried yours - works definitely great. thanks a lot!
I used a public turn server and a public kurento media server in both cases.

I also had in my configuration before a stun:173.194.71.127:19302 server. (which should be working just normally)
could also be that the difficulty was there, because I did not change anything in my code. Just in case anybody else falls over such a thing. I did not test this.

browser2ios and ios2browser worked without testing viewer and presenter on the same device.. (but that will probably come soon)

Cheers.
N.

Am 04.04.2016 um 10:19 schrieb Mark Veenstra notifications@github.com:

@inspiraluna https://github.com/inspiraluna I have been testing this on internet too, but then with the commercial version of Kurento which is in beta now. It did work when being a only a presenter or only a viewer.
But using both still has some issues. We are trying to tackle these issues as we speak. We also are looking into the STUN/TURN options, because it looks related. We are sending the following constraints:

Change the var options here https://github.com/Mobilea/kurento-presenterviewer-ios-withutils/blob/master/www/js/withutils/index.js#L90 https://github.com/Mobilea/kurento-presenterviewer-ios-withutils/blob/master/www/js/withutils/index.js#L90 to:

  var options = {
    localVideo: videoPresenter,
    onicecandidate : onIceCandidatePresenter
    configuration: {
        iceServers: [
            {
                'urls': 'turn:XX.XX.XX.XX',
                'username': 'user',
                'credential': 'pass'
            },
            {
                'urls': 'turn:XX.XX.XX.XX?transport=tcp',
                'username': 'user',
                'credential': 'pass'
            }
        ],
        'iceTransportPolicy': 'relay'
    }
  };

And change the var options here https://github.com/Mobilea/kurento-presenterviewer-ios-withutils/blob/master/www/js/withutils/index.js#L116 https://github.com/Mobilea/kurento-presenterviewer-ios-withutils/blob/master/www/js/withutils/index.js#L116 to:

  var options = {
    localVideo: videoViewer,
    onicecandidate : onIceCandidateViewer
    configuration: {
        iceServers: [
            {
                'urls': 'turn:XX.XX.XX.XX',
                'username': 'user',
                'credential': 'pass'
            },
            {
                'urls': 'turn:XX.XX.XX.XX?transport=tcp',
                'username': 'user',
                'credential': 'pass'
            }
        ],
        'iceTransportPolicy': 'relay'
    }
  };

It seems to get a bit better, but still not quite there yet.
On the kurento mailing list you also said that you see missing TURN/STUN connection. Where did you see this? On Kurento Media Server side? Or in iosrtc?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #119 (comment)

Liebe Grüße

Nico

M +49 174 9891949
T +49 8721 128 96 00-0

@mark-veenstra
Copy link
Contributor

I believe this one can be closed down since 4.0.0 has integrated this generic way already.

@ibc
Copy link
Collaborator

ibc commented Apr 10, 2017

I believe this one can be closed down since 4.0.0 has integrated this generic way already.

mmm, I don't think so.

However, that the libwebrtc ObjC wrapper still require createOffer options in the old fashion syntax with mandatory and optional arrays?

@mark-veenstra
Copy link
Contributor

You are right I misread the changes the optional and mandatory arrays are still needed.

@hthetiot hthetiot added this to the 5.0.x milestone Sep 10, 2019
@hthetiot hthetiot self-requested a review September 10, 2019 19:00
@hthetiot hthetiot modified the milestones: 5.0.x, 5.0.3 Sep 15, 2019
@hthetiot
Copy link
Contributor

hthetiot commented Oct 1, 2019

Closed in favor of #394 for v6.0.0

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

Successfully merging this pull request may close these issues.

5 participants