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

Membrane WebRTC plugin based on ex_webrtc #1

Merged
merged 17 commits into from
Apr 4, 2024
Merged

Membrane WebRTC plugin based on ex_webrtc #1

merged 17 commits into from
Apr 4, 2024

Conversation

mat-hek
Copy link
Member

@mat-hek mat-hek commented Mar 25, 2024

@mat-hek mat-hek changed the title Impl Membrane WebRTC plugin based on ex_webrtc Mar 25, 2024
@mickel8 mickel8 requested a review from LVala March 25, 2024 16:22
lib/membrane_webrtc/ex_webrtc_sink.ex Outdated Show resolved Hide resolved
lib/membrane_webrtc/ex_webrtc_source.ex Outdated Show resolved Hide resolved
lib/membrane_webrtc/ex_webrtc_source.ex Outdated Show resolved Hide resolved
lib/membrane_webrtc/source.ex Outdated Show resolved Hide resolved
end

@impl true
def handle_child_notification(:connected, :webrtc, _ctx, state) do
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, a comment on why you don't handle :failed state might be useful

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we should handle the failed state :P I think we should raise in that case. Any idea how I can trigger the failed state in a test?

Copy link
Member

Choose a reason for hiding this comment

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

When PC moves to the failed it exits with {:shutdown, reason} so you will be notified by your monitor

examples/assets/browser_to_file/script.js Outdated Show resolved Hide resolved
Copy link
Member

@FelonEkonom FelonEkonom left a comment

Choose a reason for hiding this comment

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

Put private modules into nested directories, to stop having such a flat files structure. It will make distinction between public and private modules much clearer

@typedoc """
Messages sent and received if `message_format` is `ex_webrtc`.
"""
@type ex_webrtc_message :: ExWebRTC.ICECandidate.t() | ExWebRTC.SessionDescription.t()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@type ex_webrtc_message :: ExWebRTC.ICECandidate.t() | ExWebRTC.SessionDescription.t()
@type ex_webrtc_message :: ICECandidate.t() | SessionDescription.t()

Comment on lines 78 to 79
opts = Keyword.validate!(opts, message_format: :ex_webrtc, pid: self())
opts = Map.new(opts) |> Map.put(:is_element, false)
Copy link
Member

Choose a reason for hiding this comment

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

you can have one big pipeline here

end

@impl true
def handle_info({:signal, _from_pid, message}, %{peer_b: %{pid: nil}} = state) do
Copy link
Member

Choose a reason for hiding this comment

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

What if %{peer_b: nil} = state?

Comment on lines 131 to 135
queue
|> Enum.reverse()
|> Enum.each(&send_peer(state.peer_a, state.peer_b, &1))

{:noreply, state}
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want to reset the queue?

Comment on lines 94 to 98
@impl true
def handle_info(:ping, state) do
Process.send_after(self(), :ping, 30_000)
{:push, {:text, Jason.encode!(%{type: "ping", data: ""})}, state}
end
Copy link
Member

Choose a reason for hiding this comment

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

Is it leftover?

|> child(get_depayloader(kind, state))
|> bin_output(pad_ref)
else
get_child(:webrtc) |> via_out(pad_ref) |> bin_output(pad_ref)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you skip options: [kind: kind] in via_out?

lib/membrane_webrtc/ex_webrtc_sink.ex Outdated Show resolved Hide resolved
lib/membrane_webrtc/ex_webrtc_source.ex Outdated Show resolved Hide resolved
lib/membrane_webrtc/ex_webrtc_source.ex Outdated Show resolved Hide resolved
@@ -19,7 +20,17 @@ const start_connection = async (ws) => {
ws.send(JSON.stringify({ type: "ice_candidate", data: event.candidate }));
};

const localStream = await navigator.mediaDevices.getUserMedia(mediaConstraints);
pc.onconnectionstatechange = () => {
Copy link
Member

Choose a reason for hiding this comment

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

To be even more correct, we should check against pc.connectionState, like here

Comment on lines 199 to 201
PeerConnection.get_transceivers(pc)
|> Enum.filter(&(&1.direction == :sendrecv))
|> Enum.each(&PeerConnection.set_transceiver_direction(pc, &1.id, :recvonly))
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this shouldn't be needed as by default, when a remote sdp offer is applied, a new transceiver is created with :recvonly direction

@@ -109,8 +93,13 @@ defmodule Membrane.WebRTC.ExWebRTCSource do
{:connected, pad} ->
{[buffer: {pad, buffer}], state}

{:queue, queue} ->
{[], %{state | output_tracks: %{output_tracks | id => {:queue, [buffer | queue]}}}}
{:awaiting, track} ->
Copy link
Member

Choose a reason for hiding this comment

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

This should never happen

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean you're sure it never happens?

lib/membrane_webrtc/simple_websocket_server.ex Outdated Show resolved Hide resolved
Comment on lines +2 to +15
@moduledoc """
A simple WebSocket server spawned by `Membrane.WebRTC.Source`
and `Membrane.WebRTC.Sink`. It accepts a single connection
and passes the messages between the client and a Membrane
element.

The messages sent and received by the server are JSON-encoded
`t:Membrane.WebRTC.SignalingChannel.json_data_message/0`.
Additionally, the server sends a `{type: "keep_alive", data: ""}`
messages to prevent the WebSocket from being closed.

Examples of configuring and interacting with the server can
be found in the `examples` directory.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Is it a part of public API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only docs and types are. I needed a place to put them, otherwise they'd have to be in both source and sink

@mat-hek mat-hek merged commit e60b06b into master Apr 4, 2024
3 checks passed
@mat-hek mat-hek deleted the impl branch June 13, 2024 10:15
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.

Wrap elixir_webrtc in a Membrane element
4 participants