Commit graph

92 commits

Author SHA1 Message Date
Tulio Beloqui
9af6ce974a rtpjitterbuffer: fixed stall on gap when using rtx
Co-authored-by: Håvard Graff <havard@pexip.com>
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/1055>
2021-08-16 09:51:05 +00:00
Havard Graff
d75c678479 rtpjitterbuffer: fix divide-by-zero
The estimated packet-duration can sometimes end up as zero, and dividing
by that is never a good idea...

The test reproduces the scenario, and the fix is easy.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/966>
2021-04-25 02:21:04 +02:00
Havard Graff
1368b4214b rtpjitterbuffer: clean up and improve missing packets handling
* Try to make variable and function names more clear.
* Add plenty of comments describing the logic step-by-step.
* Improve the logging around this, making the logs easier to read and
  understand when debugging these issues.

* Revise the logic of packets that are actually beyond saving in doing
  the following:
1. Do an optimistic estimation of which packets can still arrive.
2. Based on this, find which packets (and duration) are now hopelessly
   lost.
3. Issue an immediate lost-event for the hopelessly lost and then add
   lost/rtx timers for the ones we still hope to save, meaning that if
   they are to arrive, they will not be discarded.

* Revise the use of rtx-delay:
  Earlier the rtx-delay would vary, depending on the pts of the latest
  packet and the estimated pts of the packet it being issued a RTX for,
  but now that we aim to estimate the PTS of the missing packet accurately,
  the RTX delay should remain the same for all packets.
  Meaning: If the packet have a PTS of X, the delay in asked for a RTX
  for this packet is always a constant X + delay, not a variable one.

* Finally ensure that the chaotic "check-for-stall" tests uses timestamps
  that starts from 0 to make them easier to debug.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/952>
2021-04-24 13:53:58 +00:00
Havard Graff
63c7a9ae43 rtpjitterbuffer: don't send multiple instant RTX for the same packet
Due to us not properly acknowleding the time when the last RTX was sent
when scheduling a new one, it can easily happen that due to the packet
you are requesting have a PTS that is slightly old (but not too old when
adding the latency of the jitterbuffer), both its calculated second and
third (etc.) timeout could already have passed. This would lead to a burst
of RTX requests, which acts completely against its purpose, potentially
spending a lot more bandwidth than needed.

This has been properly reproduced in the test:
test_rtx_not_bursting_requests

The good news is that slightly re-thinking the logic concerning
re-requesting RTX, made it a lot simpler to understand, and allows us
to remove two members of the RtpTimer which no longer serves any purpose
due to the refactoring. If desirable the whole "delay" concept can actually
be removed completely from the timers, and simply just added to the timeout
by the caller of the API. But that can be a change for a another time.

The only external change (other than the improved behavior around bursting
RTX) is that the "delay" field now stricly represents the delay between
the PTS of the RTX-requested packet and the time it is requested on,
whereas before this calculation was more about the theoretical calculated
delay. This is visible in three other RTX-tests where the delay had
to be adjusted slightly. I am confident however that this change is
correct.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/789>
2020-10-28 01:22:24 +01:00
Havard Graff
981d0c02de rtpjitterbuffer: don't use RTX packets in rate-calc and reset-logic
The problem was this:

Due to the highly irregular arrival of RTX-packet the max-misorder variable
could be pushed very low. (-10).

If you then at some point get a big in the sequence-numbers (62 in the
test) you end up sending RTX-requests for some of those packets, and then
if the sender answers those requests, you are going to get a bunch of
RTX-packets arriving. (-13 and then 5 more packets in the test)

Now, if max-misorder is pushed very low at this point, these RTX-packets
will trigger the handle_big_gap_buffer() logic, and because they arriving
so neatly in order, (as they would, since they have been requested like
that), the gst_rtp_jitter_buffer_reset() will be called, and two things
will happen:
1. priv->next_seqnum will be set to the first RTX packet
2. the 5 RTX-packet will be pushed into the chain() function

However, at this point, these RTX-packets are no longer valid, the
jitterbuffer has already pushed lost-events for these, so they will now
be dropped on the floor, and never make it to the waiting loop-function.

And, since we now have a priv->next_seqnum that will never arrive
in the loop-function, the jitterbuffer is now stalled forever, and will
not push out another buffer.

The proposed fixes:
1. Don't use RTX in calculation of the packet-rate.
2. Don't use RTX in large-gap logic, as they are likely to be dropped.
2020-04-16 17:06:31 +02:00
Havard Graff
9f1062dc05 rtpjitterbuffer: various test-improvements
Mainly generalize all the latest tests that have found various stalls
in the jitterbuffer, so that they only consist of a series of packets
with various seqnum/rtptime/rtx combinations, arriving at a specific time.

This means future tests can be more easily written to prove certain
behavior does not cause stalls.

Also fix the warning on windows:
warning C4244: 'initializing': conversion from 'double' to 'gint', possible loss of data
2020-03-31 04:01:38 +02:00
Havard Graff
f1ff80ced0 rtpjitterbuffer: remove the concept of "already-lost"
This is a concept that only applies when a buffer arrives in the chain
function, and it has already been scheduled as part of a "multi"-lost
timer.

However, "multi"-lost timers are now a thing of the past, making this
whole concept superflous, and this buffer is now simply counted as "late",
having already been pushed out (albeit as a lost-event).
2020-03-20 13:17:20 +00:00
Havard Graff
5dacf366c0 rtpjitterbuffer: immediately insert a lost-event on multiple lost packets
There is a problem with the code today, where a single timer will
be scheduled for a series of lost packets, and then if the first packet
in that series arrives, it will cause a rescheduling of that timer, going
from a "multi"-timer to a single-timer, causing a lot of the packets
in that timer to be unaccounted for, and creating a situation in where
the jitterbuffer will never again push out another packet.

This patch solves the problem by instead of scheduling those lost packets
as another timer, it instead asks to have that lost-event pushed straight
out.

This very much goes with the intent of the code here: These packets are
so desperately late that no cure exists, and we might as well get the
lost-event out of the way and get on with it.

This change has some interesting knock-on effect being presented in
later commits. It completely removes the concept of "already-lost", so
that is why that test has been disabled in this commit, to be
removed later.
2020-03-20 13:17:20 +00:00
Havard Graff
d045b40db9 rtpjitterbuffer: rework large-gap tests
Make sure to set the time the buffer is supposed to arrive at, so
as not to trigger an artificial situation.
2020-03-20 13:17:20 +00:00
Havard Graff
9eaf084d7a test/check: split out rtptimerqueue-tests in a separate file 2020-03-20 13:17:20 +00:00
Havard Graff
026223cde2 rtpjitterbuffer: fix stalling when resetting timers
When calling gst_rtp_jitter_buffer_reset you pass in a seqnum.

This is considered the starting-point for a new stream.

However, the old behavior would unref this buffer, basically lying to
the thread that is pushing out buffers saying that it can expect
this buffer, when it would never arrive. The resulting effect being no
more buffer pushed out of the jitterbuffer, and it would buffer
incoming data indefinitely.

By instead inserting the buffer in the gap_packets queue, the _reset()
function will take responsibility for using that as the first buffer
of the new stream.

Fixes #703
2020-03-04 12:55:52 +01:00
Havard Graff
54524b08dc rtpjitterbuffer: make test more stable 2019-11-29 18:33:10 +00:00
Havard Graff
87457a862d rtpjitterbuffer: make sure not to drop packets based on skew
One of the jitterbuffers functions is to try and make sense of weird
network behavior.

It is quite unhelpful for the jitterbuffer to start dropping packets
itself when what you are trying to achieve is better network resilience.

In the case of a skew, this could often mean the sender has restarted
in some fashion, and then dropping the very first buffer of this "new"
stream could often mean missing valuable information, like in the case
of video and I-frames.

This patch simply reverts back to the old behavior, prior to 8d955fc32b
and includes the simplest test I could write to demonstrate the behavior,
where a single packet arrives "perfectly", then a 50ms gap happens,
and then two more packets arrive in perfect order after that.

# Conflicts:
#	tests/check/elements/rtpjitterbuffer.c
2019-11-02 23:05:32 +00:00
Aaron Boxer
46989dca96 documentation: fix a number of typos 2019-10-05 22:38:11 +00:00
Simon Arnling Bååth
8173596ed2 gstrtpjitterbuffer: Custom messages when dropping packets
This commit adds custom element messages for when gstrtpjitterbuffer
drops an incoming rtp packets due to for example arriving too late.
Applications can listen to these messages on the bus which enables
actions to be taken when packets are dropped due to for example high
network jitter.

Two properties has been added, one to enable posting drop messages and
one to set a minimum time between each message to enable throttling the
posting of messages as high drop rates.
2019-10-04 20:31:56 +00:00
Olivier Crête
a24596423a rtpjitterbuffer: Cancel timers instead of just unlocking loop thread
When the queue is full (and adding more packets would risk a seqnum
roll-over), the best approach is to just start pushing out packets
from the other side.  Just pushing out the packets results in the
timers being left hanging with old seqnums, so it's safer to just
execute them immediately in this case. It does limit the timer space
to the time it takes to receiver about 32k packets, but without
extended sequence number, this is the best RTP can do.

This also results in the test no longer needed to have timeouts or
timers as pushing packets in drives everything.

Fixes #619
2019-09-28 07:47:54 -04:00
Nicolas Dufresne
d4c6c335c5 rtpjitterbuffer: No need to wake the timer thread on head changes
If the jitterbuffer head change, there is no need to systematically
wakeup the timer thread. The timer thread will be waken up on if
an earlier timeout has been pushed. This prevent some more spurious
wakeup when the system is loaded. As a side effect, cranking the clock
may set the clock at an earlier position.
2019-09-27 17:34:04 -04:00
Nicolas Dufresne
37742cd36d rtptimerqueue: Consolidate a data structure for timers
Implement a single timer queue for all timers. The goal is to always use
ordered queues for storing timers. This way, extracting timers for
execution becomes O(1). This also allow separating the clock wait
scheduling from the timer itself and ensure that we only wake up the
timer thread when strictly needed.

The knew data structure is still O(n) on insertions and reschedule,
but we now use proximity optimization so that normal cases should be
really fast. The GList structure is also embeded intot he RtpTimer
structure to reduce the number of allocations.
2019-09-27 17:34:04 -04:00
Nicolas Dufresne
a53ffb6e11 tests: jitterbuffer: Demacroify some helpers
There is no reason for these to be macros anymore. This makes the
test helper much more readable.
2019-09-27 13:02:16 -04:00
Havard Graff
4d4e8f99b9 rtpjitterbuffer: Add unit test for unsolicited rtx affecting skew 2019-07-03 06:23:07 -06:00
Thomas Bluemel
8d955fc32b rtpjitterbuffer: Only calculate skew or reset if no gap.
In the case of reordered packets, calculating skew would cause
pts values to be off. Only calculate skew when packets come
in as expected. Also, late RTX packets should not trigger
clock skew adjustments.

Fixes #612
2019-07-03 06:23:07 -06:00
Mathieu Duponchelle
ebe2756434 jitterbuffer: unset DTS on output buffers 2019-06-14 16:02:59 +02:00
Mikhail Fludkov
ec5fa49631 rtpjitterbuffer: late packets shouldn't affect PTS of the following packet
If, say, a rtx-packet arrives really late, this can have a dramatic
effect on the jitterbuffer clock-skew logic, having it being reset
and losing track of the current dts-to-pts calculations, directly affecting
the packets that arrive later.

This is demonstrated in the test, where a RTX packet is pushed in really
late, and without this patch the last packet will have its PTS affected
by this, where as a late RTX packet should be redundant information, and
not affect anything.
2019-06-13 11:55:10 +02:00
Mikhail Fludkov
b9c3e354ee rtpjitterbuffer: fix rtx delay calulation when large packet spacing 2019-06-12 11:39:32 +02:00
Stian Selnes
6269ed49ab rtpjitterbuffer: Fix delay for EXPECTED timers added by gaps
This patch corrects the delay set on EXPECTED timers that are added when
processing gaps. Previously the delay could be too small so that
'timout + delay' was much less than 'now', causing the following retries
to be scheduled too early. (They were sent earlier than
rtx-retry-timeout after the previous timeout.)
2019-06-12 11:39:32 +02:00
Havard Graff
8ed7ab178b rtpjitterbuffer: don't try and calculate packet-rate if seqnum are jumping
Turns out that the "big-gap"-logic of the jitterbuffer has been horribly
broken.

For people using lost-events, an RTP-stream with a gap in sequencenumbers,
would produce exactly that many lost-events immediately.
So if your sequence-numbers jumped 20000, you would get 20000 lost-events
in your pipeline...

The test that looks after this logic "test_push_big_gap", basically
incremented the DTS of the buffer equal to the gap that was introduced,
so that in fact this would be more of a "large pause" test, than an
actual gap/discontinuity in the sequencenumbers.

Once the test was modified to not increment DTS (buffer arrival time) with
a similar gap, all sorts of crazy started happening, including adding
thousands of timers, and the logic that should have kicked in, the
"handle_big_gap_buffer"-logic, was not called at all, why?

Because the number max_dropout is calculated using the packet-rate, and
the packet-rate logic would, in this particular test, report that
the new packet rate was over 400000 packets per second!!!

I believe the right fix is to don't try and update the packet-rate if
there is any jumps in the sequence-numbers, and only do these calculations
for nice, sequential streams.
2019-06-12 11:39:31 +02:00
Havard Graff
dd422f0b7f rtpjitterbuffer: fix unused variables 2019-06-12 11:39:31 +02:00
Tim-Philipp Müller
081da67444 tests: rtpjitterbuffer: fix leaks in new test_push_eos() test 2019-03-06 17:28:57 +00:00
Olivier Crête
6530fa53f2 rtp jitterbuffer test: Test for queue filling 2019-02-11 23:41:14 +00:00
Olivier Crête
59d398b66c rtpjitterbuffer tests: Validate the number of buffers 2018-12-14 12:10:16 +00:00
Olivier Crête
d857522237 rtpjitterbuffer: Run all timers immediately on EOS
When the EOS event is received, run all timers immediately and avoid
pushing the EOS downstream before this has been run. This ensures that
the lost packet statistics are accurate.
2018-12-14 12:10:16 +00:00
Olivier Crête
c6e8325945 rtpjitterbuffer test: Stop jitterbuffer before pads to avoid race
The teardown of the pads checks the refcount, but there are timers
inside the jitterbuffer that can push things, so if we're not lucky,
things could be pushed while the pads are being shut down. Putting the
jitterbuffer to NULL first avoids this.
2018-12-14 12:10:16 +00:00
Tim-Philipp Müller
781b5ac781 tests: rtpjitterbuffer: fix compiler warning due to c99-ism
rtpjitterbuffer.c:592:3: error: ‘for’ loop initial declarations are only allowed in C99 mode
2017-01-09 19:04:04 +00:00
Havard Graff
0a81f71df5 tests/jitterbuffer: Major refactoring and cleanups
* Changed PCMU->TEST for common macros
* Changed verify-functions (lost & rtx) into macros.
* Remove option to add marker-bit for test-buffers (not used anywhere)
* Add new push_test_buffer function that makes sure there are correlation
  between dts and the time on the clock. (classic test-mistake)
* Established a generic starting-point for tests with the
  construct_deterministic_initial_state function and use it where
  applicable, which removes lots of "boilerplate" everywhere.
* Add basic lost-event test
* Remove as much "magic constants" as possible.
* Remove 3 tests that no longer are testing anything that others don't,
  and was completely unmaintainable.
* Remove unnecessary use of the testclock
* Verify each test is testing what it actually says it does (and modify
  where it doesn't)

In general, make the tests much smaller, better, more maintainable and
readable.

https://bugzilla.gnome.org/show_bug.cgi?id=774409
2016-12-14 15:00:37 +02:00
Sebastian Dröge
63938ef730 gst: Don't declare variables inside the for loop header
This is a C99 feature.
2016-12-13 22:32:46 +02:00
Havard Graff
1a4393fb4d rtpjitterbuffer: fix timer-reuse bug
When doing rtx, the jitterbuffer will always add an rtx-timer for the next
sequence number.

In the case of the packet corresponding to that sequence number arriving,
that same timer will be reused, and simply moved on to wait for the
following sequence number etc.

Once an rtx-timer expires (after all retries), it will be rescheduled as
a lost-timer instead for the same sequence number.

Now, if this particular sequence-number now arrives (after the timer has
become a lost-timer), the reuse mechanism *should* now set a new
rtx-timer for the next sequence number, but the bug is that it does
not change the timer-type, and hence schedules a lost-timer for that
following sequence number, with the result that you will have a very
early lost-event for a packet that might still arrive, and you will
never be able to send any rtx for this packet.

Found by Erlend Graff - erlend@pexip.com

https://bugzilla.gnome.org/show_bug.cgi?id=773891
2016-11-04 16:56:56 +02:00
Havard Graff
fb9c75db36 rtpjitterbuffer: fix lost-event using dts instead of pts
The lost-event was using a different time-domain (dts) than the outgoing
buffers (pts). Given certain network-conditions these two would become
sufficiently different and the lost-event contained timestamp/duration
that was really wrong. As an example GstAudioDecoder could produce
a stream that jumps back and forth in time after receiving a lost-event.

The previous behavior calculated the pts (based on the rtptime) inside the
rtp_jitter_buffer_insert function, but now this functionality has been
refactored into a new function rtp_jitter_buffer_calculate_pts that is
called much earlier in the _chain function to make pts available to
various calculations that wrongly used dts previously
(like the lost-event).

There are however two calculations where using dts is the right thing to
do: calculating the receive-jitter and the rtx-round-trip-time, where the
arrival time of the buffer from the network is the right metric
(and is what dts in fact is today).

The patch also adds two tests regarding B-frames or the
“rtptime-going-backwards”-scenario, as there were some concerns that this
patch might break this behavior (which the tests shows it does not).
2016-11-04 16:51:20 +02:00
Havard Graff
bea35f97c8 rtpjitterbuffer: fix bug in reschedule_timer
The new timeout is always going to be (timeout + delay), however, the
old behavior compared the current timeout to just (timeout), basically
being (delay) off.

This would happen if rtx-delay == rtx-retry-timeout, with the result that
a second rtx attempt for any buffers would be scheduled immediately instead
of after rtx-delay ms.

Simply calculate (new_timeout = timeout + delay) and then use that instead.

https://bugzilla.gnome.org/show_bug.cgi?id=773905
2016-11-04 16:40:14 +02:00
Tim-Philipp Müller
e6d188967a tests: fix indentation 2016-09-15 09:53:07 +01:00
Havard Graff
f440b074b1 rtpjitterbuffer: improved rtx-rtt averaging
The basic idea is this:
1. For *larger* rtx-rtt, weigh a new measurement as before
2. For *smaller* rtx-rtt, be a bit more conservative and weigh a bit less
3. For very large measurements, consider them "outliers"
   and count them a lot less

The idea being that reducing the rtx-rtt is much more harmful then
increasing it, since we don't want to be underestimating the rtt of the
network, and when using this number to estimate the latency you need for
you jitterbuffer, you would rather want it to be a bit larger then a bit
smaller, potentially losing rtx-packets. The "outlier-detector" is there
to prevent a single skewed measurement to affect the outcome too much.
On wireless networks, these are surprisingly common.

https://bugzilla.gnome.org/show_bug.cgi?id=769768
2016-09-14 19:37:50 -04:00
Stian Selnes
f8238f0a9f rtpjitterbuffer: Detect whether to assume equidistant spacing when loss
Assuming equidistant packet spacing when that's not true leads to more
loss than necessary in the case of reordering and jitter. Typically this
is true for video where one frame often consists of multiple packets
with the same rtp timestamp. In this case it's better to assume that the
missing packets have the same timestamp as the last received packet, so
that the scheduled lost timer does not time out too early causing the
packets to be considered lost even though they may arrive in time.

https://bugzilla.gnome.org/show_bug.cgi?id=769768
2016-09-14 19:37:50 -04:00
Stian Selnes
2eb7383816 rtpjitterbuffer: Don't request rtx if 'now' is past retry period
There is no need to schedule another EXPECTED timer if we're already
past the retry period. Under normal operation this won't happen, but if
there are more timers than the jitterbuffer is able to process in
real-time, scheduling more timers will just make the situation worse.
Instead, consider this packet as lost and move on. This scenario can
occur with high loss rate, low rtt and high configured latency.

https://bugzilla.gnome.org/show_bug.cgi?id=769768
2016-09-14 19:37:50 -04:00
Stian Selnes
ab49dfd0b2 rtpjitterbuffer: Fix lost duration when gap after lost timer
This patch fixes an issue with the estimated gap duration when there is
a gap immediately after a lost timer has been processed. Previously
there was a discrepancy beteen the gap in seqnum and gap in dts which
would cause wrong calculated duration. The issue would only be seen with
retranmission enabled since when it's disabled lost timers are only
created when a packet is received and the actual gap length and last dts
is known.

https://bugzilla.gnome.org/show_bug.cgi?id=769768
2016-09-14 19:37:50 -04:00
Havard Graff
8087a8a31c rtpjitterbuffer: Improved expected-timer handling when gap > 0
https://bugzilla.gnome.org/show_bug.cgi?id=769768
2016-09-14 19:37:50 -04:00
Stian Selnes
38a7545003 rtpjitterbuffer: Major improvements for RTX stats
Stats should also be collected for unsuccessful packets.

rtx-rtt is very important for determining the necessary configured
latency on the jitterbuffer. It's especially important to be able to
increase the latency when retransmitted packets arrive too late and are
considered lost. This patch includes these late packets in the
calculation of the various rtx stats, making them more correct and
useful.

Also in the case where the original packet arrives after a NACK is sent,
the received RTX packet should update the stats since it provides useful
information about RTT.

The RTT is only updated if and only if all requested retranmissions are
received. That way the RTT is guaranteed to make sense. If not we don't
know which request the packet is a response to and the RTT may be bogus.
A consequence of this patch is that RTT is not updated for a request
when one of the RTX packets for that seqnum is lost, but that since
measured RTT will be more accurate.

The implementation store the RTX information from the timed out timers
and use this when the retransmitted packet arrives. For performance
these timers are stored separately from the "normal" timers in order to
not impact performance (see attached performance test).

https://bugzilla.gnome.org/show_bug.cgi?id=769768
2016-09-14 19:37:50 -04:00
Havard Graff
1b868cc9b1 rtpjitterbuffer: Add and expose more stats and increase testing of it
Add num-pushed and num-lost.
Expose num-late, num-duplicates and avg-jitter.

https://bugzilla.gnome.org/show_bug.cgi?id=769768
2016-09-14 19:37:50 -04:00
Sebastian Dröge
a1eefe23de rtpjitterbuffer: Fix unit test by disabling adaptive misorder/dropout calculations
Need to set max-misorder-time and max-dropout-time to 0 so the
jitterbuffer does not base them on packet rate calculations.
If it does, out gap is big enough to be considered a new stream and
we wait for a few consecutive packets just to be sure

https://bugzilla.gnome.org/show_bug.cgi?id=751311
2016-08-18 09:58:58 +03:00
Havard Graff
8f7962e1c3 rtpjitterbuffer: Fix stall when receiving already lost packet
When a packet arrives that has already been considered lost as part of a
large gap the "lost timer" for this will be cancelled. If the remaining
packets of this large gap never arrives, there will be missing entries
in the queue and the loop function will keep waiting for these packets
to arrive and never push another packet, effectively stalling the
pipeline.

The proposed fix conciders parts of a large gap definitely lost (since
they are calculated from latency) and ignores the late arrivals.

In practice the issue is rare since large gaps are scheduled immediately,
and for the stall to happen the late arrival needs to be processed
before this times out.

https://bugzilla.gnome.org/show_bug.cgi?id=765933
2016-05-06 14:32:42 +03:00
Tim-Philipp Müller
7335d03070 tests: fix indentation 2016-02-19 14:44:11 +00:00
Havard Graff
69436d5a61 tests: rtpjitterbuffer: port testharness to GstHarness and cleanup/improve
Probably found a bug as well, in that there are some timestamps in
there that are looking very wrong. (marked with FIXME)

https://bugzilla.gnome.org/show_bug.cgi?id=762267
2016-02-19 14:44:02 +00:00