godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.89k stars 20.15k forks source link

WebRTCPeerConnection emitting ICE candidates before answer is given. #39272

Closed ASavchenkov closed 4 years ago

ASavchenkov commented 4 years ago

Godot version: 3.2.1 Mono

OS/device including version:

Windows 10 Home Version 10.0.18362 Build 18362

Issue description:

Actual Behavior: When SetLocalDescription is called on a WebRTCPeerConnection with type="offer", and that WebRTCPeerConnection has already been added to a WebRTCMultiplayer, the connection emits ice candidates.

Expected Behavior: The WebRTCPeerConnection should not emit ICE candidates until an sdp of type="answer" is used (according to the documentation).

Steps to reproduce:

` using Godot; using System;

public class WebRTCTest : Spatial
{

    public WebRTCMultiplayer RTCMP = new WebRTCMultiplayer();
    private WebRTCPeerConnection conn = new WebRTCPeerConnection();

    // Called when the node enters the scene tree for the first time.
    public override void _Ready()
    {
        GD.Print("running ready");

        RTCMP.Initialize(1,false);
        GetTree().NetworkPeer = RTCMP;

        conn.Connect("session_description_created",this, "_OfferCreated");
        conn.Connect("ice_candidate_created", this, "_ICECreated");

        conn.Initialize();
        RTCMP.AddPeer(conn,2);
        conn.CreateOffer();

    }

    public void _OfferCreated(string type, string sdp)
    {
        conn.SetLocalDescription(type, sdp);
    }

    public void _ICECreated(string media, int index, string name)
    {
        GD.Print("This shouldn't be getting called.");
    }

    // Called every frame. 'delta' is the elapsed time since the previous frame.
    public override void _Process(float delta)
    {

    }
}

`

Minimal reproduction project:

WebRTCErr.zip

Faless commented 4 years ago

Confirmed, the documentation is wrong, WebRTCPeerConnection emits ice_candidate_created after set_local_description for both types of descriptions.

ASavchenkov commented 4 years ago

Thanks for the quick reply! The reason I thought this might not be a doc issue is because this behavior only occurs when the WebRTCPeerConnection has been added to a WebRTCMultiplayer object.

If I manually poll the connection as such: ' using Godot; using System;

public class WebRTCTest : Spatial
{

    // public WebRTCMultiplayer RTCMP = new WebRTCMultiplayer();
    private WebRTCPeerConnection conn = new WebRTCPeerConnection();

    // Called when the node enters the scene tree for the first time.
    public override void _Ready()
    {
        GD.Print("running ready");

        // RTCMP.Initialize(1,false);
        // GetTree().NetworkPeer = RTCMP;

        conn.Connect("session_description_created",this, "_OfferCreated");
        conn.Connect("ice_candidate_created", this, "_ICECreated");

        conn.Initialize();
        // RTCMP.AddPeer(conn,2);
        conn.CreateOffer();

    }

    public void _OfferCreated(string type, string sdp)
    {
        conn.SetLocalDescription(type, sdp);
    }

    public void _ICECreated(string media, int index, string name)
    {
        GD.Print("This shouldn't be getting called.");
    }

    // Called every frame. 'delta' is the elapsed time since the previous frame.
    public override void _Process(float delta)
    {
        conn.Poll();
    }
}

' It behaves as the current documentation says it should.

Additionally, if you try to add an ICE candidate to a WebRTCPeerConnection that hasn't set both SDPs, it will error out, implying that the documentation is actually correct and the behavior is not.

Is it just assumed that ice candidates will take long enough to generate that the other peer will have enough time to generate an answer and set it's local description? because it doesn't seem to be a valid one in my experience. WebRTCErr.zip

Faless commented 4 years ago

You are right, I was confused since firefox generates an empty candidate with the following code:

var rtc = new RTCPeerConnection()
rtc.onicecandidate = (c) => console.log("candidate1", c)
rtc.createOffer().then(offer => rtc.setLocalDescription(offer))
Faless commented 4 years ago

Or actually... The reason why you are not getting any candidate without the multiplayer API is that you need to create at least one data channel for the connection to work. I.e.:

In JS:

var rtc = new RTCPeerConnection();
rtc.createDataChannel("label");
rtc.onicecandidate = (c) => console.log("candidate1", c)
rtc.createOffer().then(offer => rtc.setLocalDescription(offer))

With Godot:

extends Control

var RTCMP = WebRTCMultiplayer.new()
var conn = WebRTCPeerConnection.new()
var channel = null

func _ready():
    print("running ready");

    RTCMP.initialize(1,false)
    #get_tree().network_peer = RTCMP

    conn.connect("session_description_created",self, "_OfferCreated")
    conn.connect("ice_candidate_created", self, "_ICECreated")

    #conn.initialize()
    #RTCMP.add_peer(conn,2)
    channel = conn.create_data_channel("test")
    print(conn.create_offer())

func _OfferCreated(type, sdp):
    conn.set_local_description(type, sdp)

func _ICECreated(media, index, name):
    print("This should be called.")
    printt(media, index, name)

func _process(delta):
    conn.poll()

Now strangely, this works in HTML5 exports (both FF and Chrome), but does not seem to work for me with the desktop platform. I suspect the problem might actually be in the webrtc native plugin.

EDIT: updated GDScript so it shows it works on both platforms.

ASavchenkov commented 4 years ago

Copy that. To confirm:

The current behavior is correct.

As long as at least one channel exists, setting the local descriptor should trigger ICE candidate generation, so once this is reflected in the documentation everything should be dandy.

If that's all correct then all I can say is thank you!