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.
The send path in rtpsession processes the buffer list along the way,
sharing info and stats between packets in the same list, because it
assumes that all packets in a buffer list are from the same frame.
However, in the receiving path packets can arrive in all sorts of
arrangements:
- different sources,
- different frames (different timestamps),
- different types (multiplexed RTP and RTCP, invalid RTP packets).
so a more general approach should be used to correctly support buffer
lists in the receive path.
It turns out that it's simpler and more robust to process buffers
individually inside the rtpsession element even if they come in a buffer
list, and then reassemble a new buffer list when pushing the buffers
downstream.
This avoids complicating the existing code to make all functions
buffer-list-aware with the risk of introducing regressions,
To support buffer lists in the receive path and reduce the "push
overhead" in the pipeline, a new private field named processed_list is
added to GstRtpSessionPrivate, it is set in the chain_list handler and
used in the process_rtp callback; this is to achieve the following:
- iterate over the incoming buffer list;
- process the packets one by one;
- add the valid ones to a new buffer list;
- push the new buffer list downstream.
The processed_list field is reset before pushing a buffer list to be on
the safe side in case a single buffer was to be pushed by upstream
at some later point.
NOTE:
The proposed modifications do not change the behavior of the send path.
The process_rtp callback is called in rtpsource.c by the push_rtp
callback (via source_push_rtp) only when the source is not internal.
So even though push_rtp is also called in the send path, it won't end up
using process_rtp in this case because the source would be internal in
the send path.
The reasoning from above may suggest a future refactoring: push_rtp
might be split to better differentiate the send and receive path.
Forwarding a single segment event from the pad that first gets
chained is incorrect: when that first event was sent by an element
such as x264enc, with its offset start, we end pushing out of segment
buffers for the other pad(s).
Instead, everytime the active pad changes, forward the appropriate
segment event.
Fixes https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/1028
When it is not clear yet if a packet relative to a source should be
pushed, the packet is put into a queue, this happens in two cases:
- the source is still in probation;
- there is a large jump in seqnum, and it is not clear what
the cause is, future packets will help making a guess.
In either case stats about received packets are not updated at all; and
even if they were, when init_seq() is called it resets all receiver
stats, effectively loosing any possible stat about previously received
packets.
Fix this by taking into account the queued packets and update the stats
when calling init_seq().
Since commit c971d1a9a (rtpsource: refactor bitrate estimation,
2010-03-02) bytes_received filed in RTPSourceStats is set but then never
used again, expose it so that it can be used by user code to verify how
many bytes have been received.
According to RFC3550 lower-level headers should be considered for
bandwidth calculation.
See https://tools.ietf.org/html/rfc3550#section-6.2 paragraph 4:
Bandwidth calculations for control and data traffic include
lower-layer transport and network protocols (e.g., UDP and IP) since
that is what the resource reservation system would need to know.
Fix the source data to accommodate that.
Assume UDPv4 over IP for now, this is a simplification but it's good
enough for now.
While at it define a constant and use that instead of a magic number.
NOTE: this change basically reverts the logic of commit 529f443a6
(rtpsource: use payload size to estimate bitrate, 2010-03-02)
If the conflict is detected when sending a packet, then also send an
upstream event to tell the source to reconfigure itself.
Also ignore the collision if we see more than one collision from the same
remote source to avoid problems on loops.
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
Make sure to clear any master clock on the media_clock
before unreffing it to release the timer callback that's
updating the clock and keeping it reffed.
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.)
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.
In this change we now protect the internal srcpads list using the
stream lock and limit usage of the internal stream lock to
preventing data flowing on the other src pad type while creating
and signalling the new pad.
This fixes a deadlock with RTPBin shutdown lock. These two locks would
end up being taken in two different order, which caused a deadlock. More
generally, we should not rely on a streamlock when handling out-of-band
data, so as a side effect, we should not take a stream lock when
iterating internal links.
This is a tiny clarification as the storage was loosely named "storage".
This change clarify that the storage is specificaly used for received RTP
packets. This is unlike the storage found in rtprtxsend that stores a
backlog of sent RTP packets.
We recently added code to remove outdate NACK to avoid using bandwidth
for packet that have no chance of arriving on time. Though, this had a
side effect, which is that it was to get an early RTCP packet with no
feedback into it. This was pretty useless but also had a side effect,
which is that the RTX RTT value would never be updated. So we we stared
having late RTX request due to high RTT, we'd never manage to recover.
This fixes the regression by making sure we keep at least one NACK in
this situation. This is really light on the bandwidth and allow for
quick recover after the RTT have spiked higher then the jitterbuffer
capacity.
Right now, we may call on-new-ssrc after we have processed the first
RTP packet. This prevents properly configuring the source as some
property like "probation" are copied internally for use as a
decreasing counter. For this specific property, it prevents the
application from disabling probation on auxiliary sparse stream.
Probation is harmful on sparse streams since the probation algorithm
assume frequent and contiguous RTP packets.
This introduce a new signal on RTSession, on-sending-nacks is emited
right before the list of seqnums to be nacked are processed and
transformed into FB Nack. This allow implementing custom nacks
handling through another mechanism with APP feedback.
In order to do that, we now split the nacks registration from the actual
FB nack packet construction. We then try and add as many FB Nacks as
possible into the active packets and leave the remaining seqnums in the
RTPSource. In order to avoid sending outdated NACK later on, we save the
seqnum calculated deadline and cleanup the outdated seqnums before the
next RTCP send.
Fixes#583
Calling rtp_session_send_rtcp before marking the source as requiring a
pli/fir/nack meant the rtcp_thread could be scheduled and start running
before the source was updated. This meant the request would not be sent
early but instead was transmitted with the next regular RTCP packet.
Add test for nack generation.
If the current time is equal to the early rtcp time deadline, there is
no need to schedule a timer. This ensure that immediate feedback is
really immediate and simplify implementing unit tests with the test
clock, which stops perfectly on the timeout time.
This fix has been extracted from Pexip feature patch called
"rtpsession: Allow instant transmission of RTCP packets"
One comments in gst_rtp_session_chain_send_rtp_common() is referring to
groups in a buffer list, however this concept of "group" comes from
GStreamer 0.10 and does not exist anymore in GStreamer 1.0, so update the
comment to refer to buffers instead.
The update_receiver_stats() function is called also when sending packets
in rtp_source_send_rtp(), and sending packets may happen using a buffer
list rather than individual buffers.
So update the stats using the actual number of packets sent.
NOTE: this is fine for the receive path too (rtp_process_send_rtp)
because the receive path does not support buffer lists and
pinfo->packets would always be equal to 1 in this case.
This is needed for the case you don't know in advance all the sessions
you will be using, but would like to place all the related AUX element
in the same GstBin. As per current implementation, each time an sender
AUX bin is requested and returned, RTPBin will walk the src pads and
create sessions for these pads.
In the current implementation, if a src pad already have a sessions, it
returns an error and stops. As a side effect, if an AUX bin is reused in
a following AUX bin request, it can only work if the pads are created on
the last request.
This change simply relax the restriction in order to keep walking, and
just ensure that all newly created pads have a sessions.
In commit 28e5f9098 (rtpbin: use PacketInfo for the sender, 2013-09-13)
the rtp_source_send_rtp signature changed but the documentation was not
adjusted to match the new one.
Update the documentation to match the function signature.
Some functions now accept a generic 'gpointer data' parameter because
they can work either on a single buffer or a buffer list.
However the comments were still referring to the old 'GstBuffer *buffer'
parameter, so update the comments to match the actual functions
signature.
So far we assumed that if all sources are bye, this meant we needed to
send an EOS on the RTCP sink. The problem is that this case may happens
if we only had one internal source and it detected a collision.
So now we limit the EOS forwarding to when there is a send_rtp_sink pad
and that this pad has received EOS. We don'tcheck the recv_rtp_sink
since the code does not wait for the bye to be send before sending EOS
to the RTCP src pad.
This is useful when implementing custom retransmission mechanism like
RIST to prevent RTCP from being produces for the retransmitted SSRC.
This would also be used in general for various purpose when customizing
an RTP base pipeline.
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.
This reverts commit dcd3ce9751.
This functionality was implemented for gstopenwebrtc, but it
turned out this was not actually needed for webrtc bundling
support, as shown in webrtcbin. It also doesn't correspond
to any standards.
This is an API break, but nothing should actually depend on
this, at least not for its initial purpose.
Changes in rtpbin.c were reverted manually, to preserve some
refactoring that had occurred in the original commit.
Fixes#537
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.
This is an extra internal recurisve lock use to avoid having to take
both sink pad streams lock all the time. This patch renamed it
INTERLNAL_STREAM_LOCK/UNLOCK() to avoid confusion with possible upstream
GST_PAD API.
This reverts "6f3734c305 rtpssrcdemux: Only forward stick events while
holding the sinkpad stream lock" and actually hold on the internal
stream lock. This prevents in some needed case having a second
streaming thread poping in and messing up event ordering.
While forwarding serialized event, we use gst_pad_forward() function.
In the forward callback (GstPadForwardFunction) we always return
TRUE. Returning true there will stop the dispatching procedure. As a
side effect, only one events is receiving the events. This breaks
when sending EOS from the applicaiton, it also breaks the latency
tracer.
Reset RTPSession when rtpsession changes state from PAUSED to READY.
Without this change, a stored last_rtptime in RTPSource could interfere
with RTP timestamp generation in RTCP Sender Report.
Fixes#510
Always wait with starting the RTCP thread until either a RTP or RTCP
packet is sent or received. Special handling is needed to make sure the
RTCP thread is started when requesting an early RTCP packet.
We want to wait with starting the RTCP thread until it's needed in order
to not send RTCP packets for an inactive source.
https://bugzilla.gnome.org/show_bug.cgi?id=795139
The code before copied GstStructure twice. The first time inside
gst_value_set_structure and the second time in g_value_array_append.
Optimized version does no copies, just transfers ownership to
GValueArray. It takes advantage of the fact that array has already
enough elements preallocated and the memory is zero initialized.
https://bugzilla.gnome.org/show_bug.cgi?id=795139
If obtain_internal_source() returns a source that is not internal it
means there exists a non-internal source with the same ssrc. Such an
ssrc collision should be handled by sending a GstRTPCollision event
upstream and choose a new ssrc, but for now we simply drop the packet.
Trying to process the packet further will cause it to be pushed
usptream (!) since the source is not internal (see source_push_rtp()).
https://bugzilla.gnome.org/show_bug.cgi?id=795139
If there is an external source which is about to timeout and be removed
from the source hashtable and we receive feedback RTCP packet with the
media ssrc of the source, we unlock the session in
rtp_session_process_feedback before emitting 'on-feedback-rtcp' signal
allowing rtcp timer to kick in and grab the lock. It will get rid of
the source and rtp_session_process_feedback will be left with RTPSource
with ref count 0.
The fix is to grab the ref to the RTPSource object in
rtp_session_process_feedback.
https://bugzilla.gnome.org/show_bug.cgi?id=795139
These are the sources we send from, so there is no reason to
report receive statistics for them (as we do not receive on them,
and the remote side has no knowledge of them).
https://bugzilla.gnome.org/show_bug.cgi?id=795139
The code responsible for creating retransmitted buffers
assumed the stored buffer had been created with
rtp_buffer_new_allocate when copying the extension data,
which isn't necessarily the case, for example when
the rtp buffers come from a udpsrc.
https://bugzilla.gnome.org/show_bug.cgi?id=794958
Similar to the get-session and get-internal-session signals,
we expose a get-storage signal in addition to the
get-internal-storage signal to give access to the actual
element for applications that need to set properties on the
element, in particular "size-time"
https://bugzilla.gnome.org/show_bug.cgi?id=794910
Packets with these payload types will be dropped. A use case
for this is FEC, where we want FEC packets to go through the
jitterbuffer, but not be output by rtpbin.
https://bugzilla.gnome.org/show_bug.cgi?id=792696
When the signal returns a floating reference, as its return type
is transfer full, we need to sink it ourselves before passing
it to gst_bin_add (which is transfer floating).
This allows us to unref it in bin_remove_element later on, and
thus to also release the reference we now own if the signal
returns a non-floating reference as well.
As we now still hold a reference to the element when removing it,
we also need to lock its state and setting it to NULL before
unreffing it
Also update the request_aux_sender test.
https://bugzilla.gnome.org/show_bug.cgi?id=792543
When XR packet is detected, warning message leads to misunderstandings.
Until RFC3611 is implemented in gst-plugins-base, the level needs to
be downgraded to avoid confusion.
https://bugzilla.gnome.org/show_bug.cgi?id=789746
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.
Timestamp offsets needs to be checked to detect unrealistic values
caused for example by NTP clocks not in sync. The new parameter
max-ts-offset lets the user decide an upper offset limit. There
are two different cases for checking the offset based on if
ntp-sync is used or not:
1) ntp-sync enabled
Only negative offsest are allowed since a positive offset would
mean that the sender and receiver clocks are not in sync.
Default vaule of max-ts-offset = 0 (disabled)
2) ntp-sync disabled
Both positive and negative offsets are allowed.
Default vaule of max-ts-offset = 3000000000
The reason for different default values is to be backwards
compatible.
https://bugzilla.gnome.org/show_bug.cgi?id=785733
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
* use INFO/DEBUG/LOG/TRACE equaly and meaningfully;
previously rtprtxsend:LOG and rtprtxreceive:LOG would generate
a totally different amount of log traffic and sometimes it was
impossible to see the information you wanted without useless
spam being printed around
* improve the wording, give a reasonable and self-explanatory
amount of information
* print SSRCs in hex
* avoid G_FOO_FORMAT for readability (we are just printing integers)
If one requests the send_rtcp_src_%u pad before a recv_rtcp_sink_%u pad,
the session/pad would never be created and NULL was returned.
Switching the request order would work.
https://bugzilla.gnome.org/show_bug.cgi?id=786718
Callers of the API (rtpsource, rtpjitterbuffer) pass clock_rate
as a signed integer, and the comparison "<= 0" is used against
it, leading me to think the intention was to have the field
be typed as gint32, not guint32.
This led to situations where we could call scale_int with
a MAX_UINT32 (-1) guint32 as the denom, thus raising an
assertion.
https://bugzilla.gnome.org/show_bug.cgi?id=785991
gst_util_uint64_scale_int takes a gint as denom parameter
whereas ctx->clock_rate is a guint32.
It happens when gst_rtp_packet_rate_ctx_reset set clock_rate
to -1.
So just define clock_rate as gint like it is done in rtpsource.h
https://bugzilla.gnome.org/show_bug.cgi?id=784250
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
This debug statement is meant to print the time since the last (early)
RTCP transmission, not the last regular RTCP transmission (which also
happens to be set a few lines above to current_time, so the debug output
is just confusing)
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
If an element queries the number of retransmission buffers pushed
*while* the push is still taking place (and before the object lock
is taken just after) it would end up with the wrong statistic
being reported.
Increment it just before the push, avoids races when getting statistics
https://bugzilla.gnome.org/show_bug.cgi?id=768723
