Add support for TURN servers #7

mickel8 · 2021-03-02T10:33:12Z

closes #2

mat-hek · 2021-03-08T11:22:25Z

lib/ex_libnice.ex

+  def set_relay_info(
+        pid,
+        stream_id,
+        component_id,
+        server_ip,
+        server_port,
+        username,
+        password,
+        relay_type


server-specific parameters could be bundled together, reducing the number of arguments and allowing to pass multiple servers at once

allowing enum and :all atom for component_id would come in handy

I have added ability to specify list of component ids. :all is harder because we don't store information about how many components are created.

Don't insist, but storing that shouldn't be hard

mat-hek · 2021-03-08T11:34:37Z

lib/ex_libnice.ex

@@ -66,6 +68,46 @@ defmodule ExLibnice do
    GenServer.cast(pid, {:remove_stream, stream_id})
  end

+  @doc """
+  Sets TURN server. Can be called multiple times for the same component. `relay_type` can be "udp",


you can mention that servers are not overridden

mat-hek · 2021-03-08T11:40:22Z

c_src/ex_libnice/native.spec.exs

@@ -14,6 +14,15 @@ spec add_stream(state, n_components :: unsigned, name :: string) :: {:ok :: labe

 spec remove_stream(state, stream_id :: unsigned) :: {:ok :: label}

+spec set_relay_info(state, stream_id :: unsigned, component_id :: unsigned, server_ip :: string,
+       server_port :: unsigned, username :: string, password :: string, relay_type :: string) ::
+       {:ok :: label, state}


does this need to return state?

mat-hek · 2021-03-08T11:40:28Z

c_src/ex_libnice/native.spec.exs

+       | {:error :: label, :bad_relay_type :: label}
+       | {:error :: label, :failed_to_set_turn :: label}
+
+spec forget_relays(state, stream_id :: unsigned, component_id :: unsigned) :: {:ok :: label, state}


mat-hek · 2021-03-09T09:54:32Z

c_src/ex_libnice/native.spec.exs

@@ -16,11 +16,11 @@ spec remove_stream(state, stream_id :: unsigned) :: {:ok :: label}

 spec set_relay_info(state, stream_id :: unsigned, component_id :: unsigned, server_ip :: string,
       server_port :: unsigned, username :: string, password :: string, relay_type :: string) ::
-       {:ok :: label, state}
+       {:ok :: label}


why not return bare :ok :: label?

:ok :: label results in syntax error but :ok is ok 👍

maybe wrap it in braces (:ok :: label)

mat-hek · 2021-03-09T09:54:49Z

c_src/ex_libnice/native.spec.exs

       | {:error :: label, :bad_relay_type :: label}
       | {:error :: label, :failed_to_set_turn :: label}

-spec forget_relays(state, stream_id :: unsigned, component_id :: unsigned) :: {:ok :: label, state}
+spec forget_relays(state, stream_id :: unsigned, component_id :: unsigned) :: {:ok :: label}


mat-hek · 2021-03-09T09:59:33Z

lib/ex_libnice.ex

+  @type relay_info ::
+          {server_ip :: String.t(), server_port :: integer(), username :: String.t(),
+           password :: String.t(), relay_type :: :udp | :tcp | :tls}


I'd define that as a map or struct, but leave it as is if you consider that an overkill

This would simplify some function clauses but on the other hand imo it's easier for user to specify tuple than writing map with all keys

That's true, but map/struct has important profits for the user also, for example, it's impossible to mistake the order of arguments, or you can provide some defaults, like the default port

True that, I have fixed it. Now it will also be similar to WebRTC JS API

mat-hek · 2021-03-09T10:03:47Z

lib/ex_libnice.ex

+      {:error, _cause} = error ->
+        Logger.warn("""
+        Couldn't set TURN server #{inspect(server_ip)} #{inspect(server_port)} for component:
+        #{inspect(component_id)} in stream: #{inspect(stream_id)}
+        """)


I'd log the cause too

mat-hek · 2021-03-09T10:07:08Z

lib/ex_libnice.ex

+  defp do_set_relay_info(
+         _state,
+         stream_id,
+         component_id,
+         {server_ip, server_port, _username, _password, _relay_type}
+       ) do
+    Logger.warn("""
+    Couldn't set TURN server #{inspect(server_ip)} #{inspect(server_port)} for component: \
+        #{inspect(component_id)} in stream: #{inspect(stream_id)}, reason: bad_relay_type
+    """)
+
+    {:error, :bad_relay_type}
+  end


Let's move that clause up and add a guard when relay_type not in [:udp, :tcp, :tls]. This way we won't accidentally fall into this clause and return a misleading error in the future. I'd log what was passed as the relay type also.

mat-hek · 2021-03-10T09:36:14Z

lib/ex_libnice.ex

@@ -255,7 +255,7 @@ defmodule ExLibnice do
    case Unifex.CNode.call(cnode, :add_stream, [n_components, name]) do
      {:ok, stream_id} ->
        Logger.debug("New stream_id: #{stream_id}")
-        {:reply, {:ok, stream_id}, %{state | n_components: n_components}}
+        {:reply, {:ok, stream_id}, put_in(state.stream_components[stream_id], n_components)}


shouldn't we remove that in remove_stream?

mat-hek · 2021-03-10T09:36:57Z

lib/ex_libnice.ex

+      nil ->
+        Logger.warn("""
+        Couldn't set TURN servers. No components for stream id #{inspect(stream_id)}"
+        """)


Doesn't that mean that there's no such stream?

Actually yes

mat-hek · 2021-03-11T11:13:04Z

test/ex_libnice_test.exs

+               %{
+                 :server_ip => {127, 0, 0, 1},
+                 :server_port => 3478,
+                 :username => "username",
+                 :password => "password",
+                 :relay_type => :udp
+               }


you can use %{key: value} syntax for maps with atom keys

mickel8 added enhancement New feature or request webrtc labels Mar 2, 2021

mickel8 self-assigned this Mar 2, 2021

mickel8 force-pushed the turn-servers branch 3 times, most recently from 0c633a2 to 52f2884 Compare March 4, 2021 11:39

Add support for turn servers

41e89bb

mickel8 force-pushed the turn-servers branch from 52f2884 to 41e89bb Compare March 4, 2021 11:53

mickel8 requested a review from mat-hek March 4, 2021 11:56

mickel8 mentioned this pull request Mar 4, 2021

Add support for TURN servers membraneframework-labs/membrane_libnice_plugin#20

Merged

mickel8 changed the title ~~Add support for turn servers~~ Add support for TURN servers Mar 4, 2021

mat-hek requested changes Mar 8, 2021

View reviewed changes

mickel8 force-pushed the turn-servers branch 2 times, most recently from 0324c76 to def48cf Compare March 8, 2021 18:06

Fixes for CR

ca8d234

mickel8 force-pushed the turn-servers branch from def48cf to ca8d234 Compare March 8, 2021 18:09

mickel8 requested a review from mat-hek March 8, 2021 18:10

mat-hek requested changes Mar 9, 2021

View reviewed changes

mickel8 added 3 commits March 9, 2021 18:15

Refactor native.spec.exs

4a16d73

Refactor code

2433820

Allow passing :all as components for TURN servers

6395013

mickel8 requested a review from mat-hek March 9, 2021 17:53

mat-hek removed their request for review March 10, 2021 09:15

mickel8 added 2 commits March 10, 2021 10:16

Fix :all option when there is more than one stream

bff2b15

Refactor native.spec.exs

b253069

mickel8 requested a review from mat-hek March 10, 2021 09:22

mat-hek reviewed Mar 10, 2021

View reviewed changes

Clean up state after removing stream

6538bac

mickel8 force-pushed the turn-servers branch from 909137f to 6538bac Compare March 10, 2021 10:01

mickel8 added 2 commits March 10, 2021 13:30

Change relay_info type to map

2e1acfd

Change server ip to :inet.ip_address()

ce67095

mickel8 requested a review from mat-hek March 10, 2021 14:26

mat-hek approved these changes Mar 11, 2021

View reviewed changes

Refactor test code

8770229

mickel8 merged commit 4fc4687 into master Mar 11, 2021

mickel8 deleted the turn-servers branch March 11, 2021 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TURN servers #7

Add support for TURN servers #7

mickel8 commented Mar 2, 2021

mat-hek Mar 8, 2021

mickel8 Mar 8, 2021

mat-hek Mar 9, 2021

mat-hek Mar 8, 2021

mat-hek Mar 8, 2021

mat-hek Mar 8, 2021

mat-hek Mar 9, 2021

mickel8 Mar 9, 2021

mat-hek Mar 9, 2021

mat-hek Mar 9, 2021

mat-hek Mar 9, 2021

mickel8 Mar 9, 2021

mat-hek Mar 10, 2021

mickel8 Mar 10, 2021

mat-hek Mar 9, 2021

mat-hek Mar 9, 2021

mat-hek Mar 10, 2021

mat-hek Mar 10, 2021

mickel8 Mar 10, 2021

mat-hek Mar 11, 2021

Add support for TURN servers #7

Add support for TURN servers #7

Conversation

mickel8 commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment