Add jitter buffer #158
Add jitter buffer #158
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #158 +/- ##
==========================================
+ Coverage 87.44% 88.03% +0.58%
==========================================
Files 40 44 +4
Lines 2231 2356 +125
==========================================
+ Hits 1951 2074 +123
- Misses 280 282 +2
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @opaque t :: %__MODULE__{ | ||
| latency: non_neg_integer(), | ||
| store: PacketStore.t(), | ||
| state: :initial_wait | :timer_set | :timer_not_set |
There was a problem hiding this comment.
I think we could replace this by started? boolean and changing maybe_set_timer to get_next_timeout.
Just return time to the next action.
@spec get_next_timeout(t()) :: non_neg_integer()
defp get_next_timeout(buffer)
defp get_next_timeout(%{started: false} = buffer) do
case PacketStore.first_record_timestamp(buffer.store) do
# If we're inserting the very first packet, set the initial latency timer
nil -> buffer.latency
_ts -> nil
end
end
defp get_next_timeout(%{started: true} = buffer) do
case PacketStore.first_record_timestamp(buffer.store) do
nil ->
nil
timestamp_ms ->
since_insertion = System.monotonic_time(:millisecond) - timestamp_ms
max(0, buffer.latency - since_insertion)
end
endif we also decide that the latency for the first packet is not needed we could simplify this further into
defp get_next_timeout(buffer) do
case PacketStore.first_record_timestamp(buffer.store) do
nil ->
nil
timestamp_ms ->
since_insertion = System.monotonic_time(:millisecond) - timestamp_ms
max(0, buffer.latency - since_insertion)
end
endDoes it make sense to you?
There was a problem hiding this comment.
Not differentiating between having the timer set and unset means we'll be telling the user to set a timer every time a packet is inserted, which to me seems quite unnecessary and wasteful when it can easily be avoided with just a few lines of code.
There was a problem hiding this comment.
PS I love that you chose to include the typespec for the private function here :)
There was a problem hiding this comment.
Not differentiating between having the timer set and unset means we'll be telling the user to set a timer every time a packet is inserted
To be precise, every time when usser inserts something and we are waiting for some packet. In most cases, when packets are ordered, we will immediately flush them.
Anyway, this indeed might not be the bes UX, I didn't notice that earlier 🤔
On the other hand, right now, if someone calls handle_timeout too early, they will get nil as timer and will think everything is okay
There was a problem hiding this comment.
To be precise, every time when usser inserts something and we are waiting for some packet. In most cases, when packets are ordered, we will immediately flush them.
Yeah, you're correct, my bad
On the other hand, right now, if someone calls handle_timeout too early, they will get nil as timer and will think everything is okay
I don't see how that would happen, we're changing the buffer state to :timer_not_set immediately after handle_timeout is called, so all of the timer-setting code is being executed
def handle_timeout(buffer) do
%__MODULE__{buffer | state: :timer_not_set} |> send_packets()
end
sgfn
force-pushed
the
sgfn/jitter-buffer
branch
from
365b353
to
4c330aa
Compare
|
|
||
| alias ExWebRTC.RTP.JitterBuffer.Heap | ||
|
|
||
| test "stores things and reports size" do |
There was a problem hiding this comment.
Nitpick, in Elixir WebRTC we decided to avoid the old Membrane style of describing tests. Sometimes you can still find such tests or it might not be posssible to describe them in other way but in general we favor something like this:
test "Heap.push/1"
test "Heap.root/1"
test "Heap.new/1"
That's how tests in Elixir repo are written. We don't have to change these tests, just for the future
| @@ -0,0 +1,118 @@ | |||
| defmodule ExWebRTC.RTP.JitterBuffer.Heap do | |||
| # seq_numbers = | ||
| # store | ||
| # |> PacketStore.dump() | ||
| # |> Enum.map(& &1.packet.sequence_number) | ||
|
|
||
| # assert seq_numbers == [65_535, 0, 1] | ||
|
|
||
| # indexes = | ||
| # store | ||
| # |> PacketStore.dump() | ||
| # |> Enum.map(& &1.index) | ||
|
|
||
| # assert indexes == [@seq_number_limit - 1, @seq_number_limit, @seq_number_limit + 1] |
The battle-tested (I would assume) implementation from Membrane RTP Plugin