Add PhoenixSignaling #23
Conversation
…mentation and specs. Don't use plain Signaling module in channel implementation
…able. Move current example to websocket_signaling subdirectory. Implement phoenix signaling example
| defmodule Membrane.WebRTC.PhoenixSignaling do | ||
| @moduledoc """ | ||
| Provides signaling capabilities for WebRTC connections through Phoenix channels. | ||
| """ | ||
| use GenServer | ||
| alias Membrane.WebRTC.Signaling | ||
|
|
||
| @typedoc """ | ||
| A type representing an unique identifier that is used to distinguish between different Phoenix Signaling | ||
| instances. | ||
| """ | ||
| @type signaling_id :: String.t() | ||
|
|
||
| @spec start(term()) :: GenServer.on_start() | ||
| def start(args) do | ||
| GenServer.start(__MODULE__, args, name: __MODULE__) | ||
| end | ||
|
|
||
| @spec start_link(term()) :: GenServer.on_start() | ||
| def start_link(args) do | ||
| GenServer.start_link(__MODULE__, args, name: __MODULE__) | ||
| end | ||
|
|
||
| @impl true | ||
| def init(_args) do | ||
| {:ok, %{signaling_map: %{}}} | ||
| end | ||
|
|
||
| @impl true | ||
| def handle_call({:get_or_create, signaling_id}, _from, state) do | ||
| case Map.get(state.signaling_map, signaling_id) do | ||
| nil -> | ||
| signaling = Signaling.new() | ||
| state = put_in(state, [:signaling_map, signaling_id], signaling) | ||
| {:reply, signaling, state} | ||
|
|
||
| signaling -> | ||
| {:reply, signaling, state} | ||
| end | ||
| end | ||
|
|
||
| @impl true | ||
| def handle_call({:get, signaling_id}, _from, state) do | ||
| {:reply, Map.get(state.signaling_map, signaling_id), state} | ||
| end | ||
|
|
||
| @doc """ | ||
| Returns an instance of a Phoenix Signaling associated with given signaling ID. | ||
| """ | ||
| @spec new(signaling_id()) :: Signaling.t() | ||
| def new(signaling_id) do | ||
| get_or_create(signaling_id) | ||
| end | ||
|
|
||
| @doc """ | ||
| Registers Phoenix.Channel process as WebRTC signaling peer | ||
| so that it can send and receive signaling messages. | ||
| """ | ||
| @spec register_channel(signaling_id(), pid() | nil) :: :ok | ||
| def register_channel(signaling_id, channel_pid \\ nil) do | ||
| channel_pid = channel_pid || self() | ||
| signaling = get_or_create(signaling_id) | ||
| Signaling.register_peer(signaling, message_format: :json_data, pid: channel_pid) | ||
| end | ||
|
|
||
| @doc """ | ||
| Sends a signal message via the Phoenix Signaling instance associated with given signaling ID. | ||
| """ | ||
| @spec signal(signaling_id(), Signaling.message_content()) :: :ok | no_return() | ||
| def signal(signaling_id, msg) do | ||
| signaling = get!(signaling_id) | ||
| Signaling.signal(signaling, msg) | ||
| end | ||
|
|
||
| defp get_or_create(signaling_id) do | ||
| GenServer.call(__MODULE__, {:get_or_create, signaling_id}) | ||
| end | ||
|
|
||
| defp get!(signaling_id) do | ||
| case GenServer.call(__MODULE__, {:get, signaling_id}) do | ||
| nil -> | ||
| raise "Couldn't find signaling instance associated with signaling_id: #{inspect(signaling_id)}" | ||
|
|
||
| signaling -> | ||
| signaling | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
For me it is little confusing, that one module at the same time
- works more or less like
Registry - provides functionalities similar to
WebRTC.Singlaing
I would split these two functionalities to separate modules.
Another thing that is little confusing for me, is that if someone calls PhoenixSignaling.register_channel/2 then he will receive {Singlaing, ...} messages instead of {PhoenixSignaling, ...}. Maybe setting the first element of these tuples to :membrane_webrtc_signaling or something like that would be a good solution of this problem? @mat-hek WDYT?
There was a problem hiding this comment.
BTW this module could be wrapped into if Code.ensure_loaded?(Phoenix) do as well
There was a problem hiding this comment.
BTW this module could be wrapped into if
Code.ensure_loaded?(Phoenix)do as well
It does not have any Phoenix dependency so it doesn't hurt us to leave it that way, but I can wrap it with Code.ensure_loaded?(Phoenix) for the sake of consistency
There was a problem hiding this comment.
:membrane_webrtc_signaling as the first element of the tuple sounds good for me
There was a problem hiding this comment.
I've also moved the GenServer capabilities from the PhoenixSignaling into custom Registry module
There was a problem hiding this comment.
I've changed the structure of the {Signaling, ...} tuple into {:membrane_webrtc_signaling, ...}
Co-authored-by: Feliks Pobiedziński <[email protected]>
Co-authored-by: Feliks Pobiedziński <[email protected]>
Co-authored-by: Feliks Pobiedziński <[email protected]>
FelonEkonom
left a comment
FelonEkonom
left a comment
There was a problem hiding this comment.
Please ensure that docs of modules wrapped with Code.ensure_loaded?/1 are properly generated. If no, we could move it to the separate repo (maybe membrane_webrtc_plugin_phoenix?) or we have to find out the way to ensure that Phoenix is loaded during docs generation
| defmodule PhoenixSignaling do | ||
| @moduledoc """ | ||
| PhoenixSignaling keeps the contexts that define your domain | ||
| and business logic. | ||
|
|
||
| Contexts are also responsible for managing your data, regardless | ||
| if it comes from the database, an external API or others. | ||
| """ | ||
| end |
There was a problem hiding this comment.
Actually it's a file automatically generated with mix phx.new and unfortunately it's called similarly to Membrane.WebRTC.PhoenixSignaling because the whole Phoenix exemplary project is called phoenix_signaling (see the directory name)
As discussed, |
…r_create functions in the PhoenixSignaling Registry module
…e_webrtc_signaling
…to date with the new struture
This PR:
Membrane.WebRTC.PhoenixSignalingMembrane.WebRTC.PhoenixSignaling.ChannelandMembrane.WebRTC.PhoenixSignaling.Socket{Signaling, ...}to{:membrane_webrtc_signaling, ...}closes membraneframework/membrane_core#941