Adapt plugin for general Membrane use cases #1
Conversation
| @@ -58,18 +67,28 @@ defmodule Membrane.RTSP.Source do | |||
| accepted_format: _any, | |||
| availability: :on_request | |||
|
|
|||
| defmodule ReadyNotifier do | |||
| @moduledoc false | |||
There was a problem hiding this comment.
let's add a comment on why it's needed
lib/membrane_rtsp_plugin/source.ex
Outdated
| ] | ||
| common_spec ++ | ||
| Enum.map(min_port..max_port, fn port -> | ||
| child({:udp_source, make_ref()}, %Membrane.UDP.Source{local_port_no: port}) |
There was a problem hiding this comment.
Why do we listen on all the ports from the range?
There was a problem hiding this comment.
this range includes only ports negotiated with setup messages, see connection_manager.ex:147, I'll make it more readable
There was a problem hiding this comment.
I think we should only choose the free ports though
There was a problem hiding this comment.
There was this slight concern, that the original version checked for a free port only during the SETUP phase and didn't claim it, so theoretically the port could be taken by some other process after SETUP and before the UDP sources are spawned. To eliminate this possibility entirely we would need to add an option to UDP Source to pass an already connected socket to it, but that could complicate the code a bit.
I planned to do that for the second, contributing PR, would you like it here too? |
| %{state | keep_alive_timer: start_keep_alive_timer(state)} | ||
| tracks | ||
| |> Enum.with_index() | ||
| |> Enum.reduce_while({:ok, []}, fn {track, idx}, {:ok, set_up_tracks} -> |
There was a problem hiding this comment.
you can use Bunch.Enum.try_reduce_while
There was a problem hiding this comment.
I don't think it's necessary, since I want to break prematurely only on error
* Make the plugin work using official elements * Crash on connection failure * Improve docs, refactor * Adjust tests, remove restarting functionalities * Migrate rtsp_decapsulator from rtp_plugin to here * Change behavior to not allow for taken ports * Add typespecs
* Adapt plugin for general Membrane use cases (#1) * Make the plugin work using official elements * Crash on connection failure * Improve docs, refactor * Adjust tests, remove restarting functionalities * Migrate rtsp_decapsulator from rtp_plugin to here * Change behavior to not allow for taken ports * Add typespecs * Receive responses to all requests for auth * Use released rtp_plugin * Try rerequesting * Use membrane_core 1.1, fix typo
closes membraneframework/membrane_core#787 (comment)
This PR:
Membrane.RTSP.Source.TCPand instead usesMembrane.TCP.Source, which was tweaked to use socket's active mode and have much better CPU performance.Membrane.RTSP.Source.DecapsulatorwithMembrane.RTSP.TCP.Decapsulator, which has since been updated to pick up interleaved RTSP messages more reliably and forward packets from all channels (including RTCP packets).Membrane.RTSP.Source.Transport.TCPWrapper, because the functionality of passing socket control has since been added toMembrane.RTSP.:udptransport, from choosing randomly from a fixed, wide range, to choosing successively from a range passed via options. This decision was made to prevent situations where a port is free during theSETUPphase but taken by a different, unrelated process duringPLAYphase. The ability to pass the port range via options is also needed when the ports available are limited by things like docker mappings or firewall.Notes:
RTSPsession to theTCP.Sourcegoes as follows:TCP.Sourcesends{:request_socket_control, socket, pid}notification toRTSP.SourceRTSP.SourcecallsRTSP.Source.ConnectionManager.transfer_socket_control/2withTCP.Source's pidRTSP.Source.ConnectionManagercallsRTSP.transfer_socket_control/2withTCP.Source's pidRTSPcalls:gen_tcp.controlling_process/2with the socket handle andTCP.Source's pid