Add add_track/2 function to API
#38
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #38 +/- ##
==========================================
+ Coverage 85.59% 86.72% +1.13%
==========================================
Files 14 14
Lines 611 633 +22
==========================================
+ Hits 523 549 +26
+ Misses 88 84 -4
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
I would once again discuss whether we always want to create sender and receiver.
- now we might end up with different codecs in transceiver and sender
- I don't see any benefits of creating sender and receiver no matter what the direction is
Even if we decide to change the current behavior, I would do this in next PRs
examples/echo/example.exs
Outdated
| {:ok, offer} = PeerConnection.create_offer(pc) | ||
| :ok = PeerConnection.set_local_description(pc, offer) | ||
| dbg(PeerConnection.get_transceivers(pc)) |
lib/ex_webrtc/peer_connection.ex
Outdated
| %RTPTransceiver{ | ||
| kind: ^kind, | ||
| sender: %RTPSender{track: nil}, | ||
| current_direction: cr |
There was a problem hiding this comment.
Nitpick but I am a little bit afraid of too many very short variable names:
- it's hard to ctrl+f on them
- at some point we might end up with a code that is hard to read e.g.
cr,tr,ix - I am perfectly fine though with using those short names when the full name is very long and used very often. E.g. in our code base we very often use
tras a shortcut from transceiver and that's okay because it's common and appears in a lot of places. But when possible and it doesn't have an impact on the line length (i.e. formatter doesn't add a new line because of its length) I would favor a little bit longer names e.g.current_directioncurrent_dircurr_diror justdirection. IMOdirectionis good enough as we use it in a very small context (two lines below)
There was a problem hiding this comment.
I only use short variable names for guards, otherwise, guards get horribly long and usually these variables aren used anywhere else, and if they are, more verbose names make sense. Here that's not really an issue, as the guards are split into multiple lines, but overall, I'm fine with either option.
lib/ex_webrtc/peer_connection.ex
Outdated
| tr = RTPTransceiver.new(kind, track, state.config, direction: :sendrecv) | ||
| {state.transceivers ++ [tr], tr.sender} | ||
|
|
||
| ix -> |
lib/ex_webrtc/peer_connection.ex
Outdated
| @impl true | ||
| def handle_call({:add_track, %MediaStreamTrack{kind: kind} = track}, _from, state) do | ||
| # we ignore the condition that sender has never been used to send | ||
| free_transceiver = |
There was a problem hiding this comment.
Nitpick but that's not really a transceiver
| free_transceiver = | |
| free_transceiver_idx = |
or
| free_transceiver = | |
| tr_idx = |
lib/ex_webrtc/rtp_sender.ex
Outdated
| @@ -23,20 +23,33 @@ defmodule ExWebRTC.RTPSender do | |||
|
|
|||
| @doc false | |||
| @spec new(MediaStreamTrack.t() | nil, RTPCodecParameters.t(), [Extmap.t()]) :: t() | |||
There was a problem hiding this comment.
| @spec new(MediaStreamTrack.t() | nil, RTPCodecParameters.t(), [Extmap.t()]) :: t() | |
| @spec new(MediaStreamTrack.t() | nil, RTPCodecParameters.t() | nil, [Extmap.t()]) :: t() |
| ] | ||
|
|
||
| @doc false | ||
| def new(kind, sender_track, config, options) do |
There was a problem hiding this comment.
Let's always add specs for public functions
Additionally, I made the
echoexample to useadd_track/2.