First iteration of RTSP endpoint functionality

[sgfn]

No description provided.

[mickel8]

Coud you please rebase branches at first so that there are only your changes?

[sgfn]

Rebased. Sorry for the inconvenience

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (sgfn/RTC-145-rtsp-endpoint@d815eb3). Click here to learn what that means.
The diff coverage is n/a.

@@                      Coverage Diff                      @@
##             sgfn/RTC-145-rtsp-endpoint     #251   +/-   ##
=============================================================
  Coverage                              ?   60.13%           
=============================================================
  Files                                 ?       41           
  Lines                                 ?     1994           
  Branches                              ?        0           
=============================================================
  Hits                                  ?     1199           
  Misses                                ?      795           
  Partials                              ?        0           

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d815eb3...0a21e41. Read the comment docs.

[sgfn]

Fixes:

• Everything outlined by @mickel8 in his review
• Resolve race condition between :new_rtp_stream and :rtsp_setup_complete messages
• Add proposed solution to attempt to create client-side NAT binding

@mickel8 is currently unavailable; everything outlined by him in the review has been fixed

[daniel-jodlos]

1. I don't think this should be a task. I don't think it has any serious implications, but just looks weird and is very much against what is said about Tasks in the documentation https://hexdocs.pm/elixir/1.12/Task.html
2. Does it even need to be another process? I think you could use Process.send_after here instead of spawning another process

[sgfn]

This was a leftover from the original ConnectionManager used in rtsp_to_hls demo -- I didn't touch the parts that worked, but ofc can take a shot at refactoring

[sgfn]

I think I see why this was done as a separate process. It's problematic because it executes RTSP.get_parameter, which is a call -- so, worst case scenario (connection loss), ConnectionManager is blocked for 5 seconds, after which it crashes

    14:28:27.593 [error] GenServer #PID<0.2841.0> terminating
     ** (stop) exited in: GenServer.call(#PID<0.2842.0>, {:execute, %Membrane.RTSP.Request{method: "GET_PARAMETER", path: nil, headers: [], body: ""}}, 5000)
         ** (EXIT) time out
         (elixir 1.14.2) lib/gen_server.ex:1038: GenServer.call/3

[...]

The way I see it, if we don't use a separate process, we either have to change the implementation of membrane_rtsp, rescue or let it crash. If we rescue and handle, ConnectionManager will still block for 5 secs, which right now isn't a big issue, but could become one in the future. If we let it crash, we'll be unable to directly inform Engine's parent about the reason -- no way to send :disconnected message. Of course, further changes could be made to handle ConnectionManager crashing... etc etc.

I'm not a big fan of either of these solutions, tbh. I would propose instead to either leave the task as-is, or create another GenServer module which will be responsible solely for the keep-alives, and thus free to crash at will. Is that an option you'd be alright with?

[daniel-jodlos]

That's perfectly okay, but I think you would expect to see spawn_monitor/1 here instead of Process.spawn/2