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).
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.
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
We do not have a way to know the format modifiers to use with string
functions provided by the system. G_GUINT64_FORMAT and other string
modifiers only work for glib string formatting functions. We cannot
use them for string functions provided by the stdlib. See:
https://developer.gnome.org/glib/stable/glib-Basic-Types.html#glib-Basic-Types.description
```
../gst/rtpmanager/gstrtpjitterbuffer.c: In function 'gst_jitter_buffer_sink_parse_caps':
../gst/rtpmanager/gstrtpjitterbuffer.c:1523:32: error: unknown conversion type character 'l' in format [-Werror=format=]
|| sscanf (mediaclk, "direct=%" G_GUINT64_FORMAT, &clock_offset) != 1)
^~~~~~~~~~
In file included from /home/nirbheek/cerbero/build/dist/windows_x86/include/glib-2.0/glib/gtypes.h:32,
from /home/nirbheek/cerbero/build/dist/windows_x86/include/glib-2.0/glib/galloca.h:32,
from /home/nirbheek/cerbero/build/dist/windows_x86/include/glib-2.0/glib.h:30,
from /home/nirbheek/cerbero/build/dist/windows_x86/include/gstreamer-1.0/gst/gst.h:27,
from /home/nirbheek/cerbero/build/dist/windows_x86/include/gstreamer-1.0/gst/rtp/gstrtpbuffer.h:27,
from ../gst/rtpmanager/gstrtpjitterbuffer.c:108:
/home/nirbheek/cerbero/build/dist/windows_x86/lib/glib-2.0/include/glibconfig.h:69:28: note: format string is defined here
#define G_GUINT64_FORMAT "llu"
^
../gst/rtpmanager/gstrtpjitterbuffer.c:1523:32: error: too many arguments for format [-Werror=format-extra-args]
|| sscanf (mediaclk, "direct=%" G_GUINT64_FORMAT, &clock_offset) != 1)
^~~~~~~~~~
```
See also: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/379
By passing `NULL` to `g_signal_new` instead of a marshaller, GLib will
actually internally optimize the signal (if the marshaller is available
in GLib itself) by also setting the valist marshaller. This makes the
signal emission a bit more performant than the regular marshalling,
which still needs to box into `GValue` and call libffi in case of a
generic marshaller.
Note that for custom marshallers, one would use
`g_signal_set_va_marshaller()` with the valist marshaller instead.
The do_expected_timeout() function may release the JBUF_LOCK, so we need
to check if nothing wanted the timer thread to exit after this call.
The side effect was that we may endup going back into waiting for a timer
which will cause arbitrary delay on tear down (or deadlock when test
clock is used).
Fixes#653
JBUF_WAIT_QUEUE drops the JBUF_LOCK, which means the stop condition
for the chain function may have changed (change_state to NULL). Check
this immediately after the wait so that we don't delay shutting down.
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.
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
The timer passed to update_timers may be from the stats timer. At the
moment, we could endup rescheduling (reusing) that timer onto the normal
timer queue, unschedul it as if it was from the normal timer queue or
duplicate it into the stats timer queue again. This was protected before
as the with the fact the stats timer didn't have a valid idx.
As the offset is already applied now, we need to update and reschedule
all timers each time the offset is changed. I'm not sure who expect this
to be retro-actively applied, but there was a unit test for it.
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.
In this patch we now make use of the new RtpTimerQueue instead of the
old GArray. This required a lot of changes all over the place, some of
the important changes are that `timer->timeout` is no longer a PTS but
the actual timeout. This was required to get the RtpTimerQueue sorting
right. The applied offset is saved as `timer->offset`, this allow
retreiving back the PTS when needed.
The clockid updates only happens once per incoming packet. If the
currently schedule timer is before the earliest timer in the queue, we
no longer wakeup the thread. This way, if other timers get setup in the
meantime, this will reduce the number of wakup.
The timer loop code has been mostly rewritten, though the behaviour of
running the lost timers first has been kept (even though there is no
test to show what would be the side effect of doing this differently).
Fixes#608
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.
This moves the RtpJitterBufferStructure type, alloc, free into
rtpjitterbuffer.c/h implementation. jitterbuffer.c strictly rely on
the fact this structure is compatible with GList, and so it make more
sense to keep encapsulate it. Also, anything that could possibly
reduce the amount of code in the element is a win.
In order to support that move, a function pointer to free the data
was added. This also allow making the free function option when
flushing the jitterbuffer.
This helps understanding which function modify the Timerdata
and which one does not. This is not always obvious from thelper
name considering recalculate_timer() does not.
If an rtx packet arrives that hasn't been requested (it might
have been requested from prior to a reset), ignore it so that
it doesn't inadvertently trigger a clock skew.
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
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.
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.)
If it goes over 2^15 packets, it will think it has rolled over
and start dropping all packets. So make sure the seqnum distance is not too big.
But let's not limit it to a number that is too small to avoid emptying it
needlessly if there is a spurious huge sequence number, let's allow at
least 10k packets in any case.
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.
After EOS is received, it is pointless to wait for further events,
specially waiting on timers. This patches fixes two cases where we could
wait instead of returning GST_FLOW_EOS and trigger a spin of the loop
function when EOS is queued, regardless if this EOS is the queue head or
not.
Doesn't do anything fancy yet, but still avoids lots of
unnecessary locking/unlocking that would happen if the
default chain_list fallback function in GstPad got invoked.
Instant large changes to ts_offset may cause timestamps to move
backwards and also cause visible effects in media playback. The new
option max-ts-offset-adjustment lets the application control the rate to
apply changes to ts_offset.
https://bugzilla.gnome.org/show_bug.cgi?id=784002
When set this property will allow the jitterbuffer to start delivering
packets as soon as N most recent packets have consecutive seqnum. A
faststart-min-packets of zero disables this feature. This heuristic is
also used in rtpsource which implements the probation mechanism and a
similar heuristic is used to handle long gaps.
https://bugzilla.gnome.org/show_bug.cgi?id=769536
In function rtp_jitter_buffer_calculate_pts: If gap in incoming RTP
timestamps is more than (3 * jbuf->clock_rate) we call
rtp_jitter_buffer_reset_skew which resets pts to 0. So components down
the pipeline (playes, mixers) just skip frames/samples until pts becomes
equal to pts before gap.
In version 1.10.2 and before this checking was bypassed for packets with
"estimated dts", and gaps were handled correctly.
https://bugzilla.gnome.org/show_bug.cgi?id=778341
When providing items with a seqnum, there is a (very small) probability
that an element with the same seqnum already exists. Don't forget
to free that item if it wasn't inserted.
And avoid returning undefined values when dealing with duplicate items
