After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 739868 - rtpmanager: rtpjitterbuffer fixes and improvements
rtpmanager: rtpjitterbuffer fixes and improvements
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.4.1
Other Linux
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-11-10 09:12 UTC by Miguel París Díaz
Modified: 2015-08-16 13:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-rtpjitterbuffer-implement-get-set-for-new-rtx-min-re.patch (1.53 KB, patch)
2014-11-10 09:13 UTC, Miguel París Díaz
none Details | Review
0002-gstrtpjitterbuffer-ensure-rtx_retry_period-0.patch (1.24 KB, patch)
2014-11-10 09:17 UTC, Miguel París Díaz
committed Details | Review
0003-gstrtpjitterbuffer-fix-expected_dts-calc-in-calculat.patch (1.46 KB, patch)
2014-11-10 09:18 UTC, Miguel París Díaz
committed Details | Review
0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch (1.89 KB, patch)
2014-11-10 09:19 UTC, Miguel París Díaz
none Details | Review
0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch (1.43 KB, patch)
2014-11-10 09:19 UTC, Miguel París Díaz
rejected Details | Review
0006-gstrtpjitterbuffer-detect-disconts-before-a-buffer-i.patch (1.96 KB, patch)
2014-11-10 09:19 UTC, Miguel París Díaz
rejected Details | Review
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch (7.44 KB, patch)
2014-11-10 09:20 UTC, Miguel París Díaz
none Details | Review
0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch (1.46 KB, patch)
2014-11-10 09:20 UTC, Miguel París Díaz
none Details | Review
0009-gstrtpjitterbuffer-add-rtx-max-retries-property.patch (4.23 KB, patch)
2014-11-10 09:21 UTC, Miguel París Díaz
committed Details | Review
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch (3.94 KB, patch)
2014-11-10 09:21 UTC, Miguel París Díaz
none Details | Review
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch (3.94 KB, patch)
2014-11-12 10:02 UTC, Miguel París Díaz
none Details | Review
rtpjitterbuffer: Make the next output buffer discont after resetting the jitterbuffer (1.22 KB, patch)
2015-03-20 17:21 UTC, Sebastian Dröge (slomo)
committed Details | Review
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch (15.63 KB, patch)
2015-03-23 12:08 UTC, Miguel París Díaz
none Details | Review
0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch (1.89 KB, patch)
2015-03-23 12:09 UTC, Miguel París Díaz
none Details | Review
0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch (1.45 KB, patch)
2015-03-23 12:09 UTC, Miguel París Díaz
none Details | Review
0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch (1.89 KB, patch)
2015-03-24 09:33 UTC, Miguel París Díaz
rejected Details | Review
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch (16.10 KB, patch)
2015-03-24 09:33 UTC, Miguel París Díaz
none Details | Review
0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch (2.78 KB, patch)
2015-03-24 09:34 UTC, Miguel París Díaz
rejected Details | Review
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch (4.00 KB, patch)
2015-04-13 09:23 UTC, Miguel París Díaz
committed Details | Review
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch (8.81 KB, patch)
2015-04-23 17:34 UTC, Miguel París Díaz
rejected Details | Review

Description Miguel París Díaz 2014-11-10 09:12:45 UTC
I propose a set of patches to fix some bugs and to improve rtpmanager stack to work in real networks.
I have explained each patch, but if you require it, I will explain them on demand.
Comment 1 Miguel París Díaz 2014-11-10 09:13:51 UTC
Created attachment 290312 [details] [review]
0001-rtpjitterbuffer-implement-get-set-for-new-rtx-min-re.patch
Comment 2 Miguel París Díaz 2014-11-10 09:17:22 UTC
Created attachment 290313 [details] [review]
0002-gstrtpjitterbuffer-ensure-rtx_retry_period-0.patch
Comment 3 Miguel París Díaz 2014-11-10 09:18:21 UTC
Created attachment 290314 [details] [review]
0003-gstrtpjitterbuffer-fix-expected_dts-calc-in-calculat.patch
Comment 4 Miguel París Díaz 2014-11-10 09:19:02 UTC
Created attachment 290315 [details] [review]
0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
Comment 5 Miguel París Díaz 2014-11-10 09:19:19 UTC
Created attachment 290316 [details] [review]
0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch
Comment 6 Miguel París Díaz 2014-11-10 09:19:51 UTC
Created attachment 290317 [details] [review]
0006-gstrtpjitterbuffer-detect-disconts-before-a-buffer-i.patch
Comment 7 Miguel París Díaz 2014-11-10 09:20:09 UTC
Created attachment 290318 [details] [review]
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Comment 8 Miguel París Díaz 2014-11-10 09:20:37 UTC
Created attachment 290319 [details] [review]
0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch
Comment 9 Miguel París Díaz 2014-11-10 09:21:04 UTC
Created attachment 290320 [details] [review]
0009-gstrtpjitterbuffer-add-rtx-max-retries-property.patch
Comment 10 Miguel París Díaz 2014-11-10 09:21:23 UTC
Created attachment 290321 [details] [review]
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch
Comment 11 Miguel París Díaz 2014-11-10 09:22:19 UTC
Comment on attachment 290312 [details] [review]
0001-rtpjitterbuffer-implement-get-set-for-new-rtx-min-re.patch

This patch is already merged into master branch.
Comment 12 Miguel París Díaz 2014-11-10 09:23:21 UTC
Comment on attachment 290313 [details] [review]
0002-gstrtpjitterbuffer-ensure-rtx_retry_period-0.patch

Related to https://bugzilla.gnome.org/show_bug.cgi?id=739344
Comment 13 Miguel París Díaz 2014-11-12 10:02:02 UTC
Created attachment 290483 [details] [review]
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch
Comment 14 Sebastian Dröge (slomo) 2015-03-20 11:35:54 UTC
Comment on attachment 290313 [details] [review]
0002-gstrtpjitterbuffer-ensure-rtx_retry_period-0.patch

Was pushed ages ago:

commit 6daa57868f01da46aa2c6cfc0a4dc8344312e4c1
Author: Miguel París Díaz <mparisdiaz@gmail.com>
Date:   Mon Oct 20 13:00:37 2014 +0200

    rtpjitterbuffer: ensure rtx_retry_period >= 0
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739344
Comment 15 Sebastian Dröge (slomo) 2015-03-20 17:14:59 UTC
Comment on attachment 290316 [details] [review]
0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch

There seems to be a real problem here but instead of increasing the number we should only reset when we're supposed to: i.e. not when a) we lost 100 packets in a row, or b) got a rtx packet with a seqnum gap >100.
Resetting the stats in these two cases seems wrong.


Question now is how we can distinguish between these cases, and the case when we actually want to reset the stats (because we think a new session started?)
Comment 16 Sebastian Dröge (slomo) 2015-03-20 17:21:21 UTC
Created attachment 299971 [details] [review]
rtpjitterbuffer: Make the next output buffer discont after resetting the jitterbuffer

Resetting the jitterbuffer drops all packets and other things, and will cause
a discontinuity in the packets received by the depayloaders. They should now
also flush anything they had pending as the new data will start at a different
position.
Comment 17 Sebastian Dröge (slomo) 2015-03-20 17:22:27 UTC
Comment on attachment 290317 [details] [review]
0006-gstrtpjitterbuffer-detect-disconts-before-a-buffer-i.patch

I don't think this patch is correct.

1) The gap event is wrong. It needs a valid timestamp and duration.
2) not resetting last_popped_seqnum might break other things

The patch is just attached here to always set discont after resetting the jitterbuffer should solve all the problems you saw here. Can you test and confirm?
Comment 18 Sebastian Dröge (slomo) 2015-03-20 17:38:37 UTC
Comment on attachment 290320 [details] [review]
0009-gstrtpjitterbuffer-add-rtx-max-retries-property.patch

Makes sense to me
Comment 19 Sebastian Dröge (slomo) 2015-03-20 17:43:29 UTC
Comment on attachment 290483 [details] [review]
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch

This looks good and also makes sense to me. Only the documentation needs to be a bit more clear :) I would just do that when merging it.

What this does is that setting the property to FALSE will cause it to not estimate when the next packet should arrive and start a retransmission request timer for it. It will still start retransmission request timers if a gap in sequence numbers is detected, just not for future packets before we noticed a gap. Especially we won't request a retransmission now if the sender is just not sending us any packets for some time (e.g. someone just switched off the camera for a while ;) ).
Comment 20 Sebastian Dröge (slomo) 2015-03-20 17:58:24 UTC
Review of attachment 290315 [details] [review]:

This looks correct to me. Adding the retransmissions to the jitter will make it wrong as the rtptime of the retransmissions will be very much in the past (we wanted to have that packet earlier already, right?) but the receive time was just now.

::: gst/rtpmanager/gstrtpjitterbuffer.c
@@ +2213,3 @@
 
+    if (G_LIKELY (gap > -(priv->rtx_delay_reorder))) {
+      calculate_jitter (jitterbuffer, dts, rtptime);

This won't do the right thing if rtx_delay_reorder==-1 ("automatic"). In that case we would ignore all expected packets (gap==0) and also all packets that are one in the future from our expected packet. With rtx_delay_reorder==0 we would also ignore all expected packets.

Otherwise this check seems correct.
Comment 21 Sebastian Dröge (slomo) 2015-03-20 18:05:16 UTC
Comment on attachment 290319 [details] [review]
0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch

I don't think this is correct. We also add timers for future packets (do_next_seqnum), and add timers for packets for which we just have reordering (i.e. a short gap). So assuming that all timers are lost packets is not correct, and this will probably cause the jitterbuffer to reset itself too early.
Comment 22 Sebastian Dröge (slomo) 2015-03-20 18:25:22 UTC
Comment on attachment 290314 [details] [review]
0003-gstrtpjitterbuffer-fix-expected_dts-calc-in-calculat.patch

I think this is also correct. Right above we consider lost_packet packets, each of them having duration, as lost and triggered their timers immediately... below we use expected_dts to schedule retransmission or schedule lost timers for the packets that come after expected_dts.
As we just triggered lost_packets packets as lost, there's no point in scheduling new timers for them and we can just skip over all lost packets.
Comment 23 Sebastian Dröge (slomo) 2015-03-20 18:32:20 UTC
Comment on attachment 290318 [details] [review]
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch

This changes the default behaviour from 3000 as rtp-max-dropout to something adaptive. Not sure that's a good idea, the default should probably stay as is.

Then calculate_n_packets_last_second() returns the length of the queue but you don't use that. You don't need it?

destroy_GstClockTime() should be renamed to something non-camelcase :) clock_time_free() or something.

get_rtp_max_dropout() seems to do the wrong thing if we have less than 1 second of packets so far. If we have more than 1 second it will amount to about 0.5s worth of data, if we have less than 1 second (e.g. X=0.5 seconds), we will only allow 0.25 (X/2) of dropout. I think this code should just return some default if we didn't observe 1 second of packets yet.


I think the dts queue also needs to be reset whenever the jitterbuffer is reset, otherwise it will contain old data that is not relevant anymore.
Comment 24 Miguel París Díaz 2015-03-23 11:11:04 UTC
Review of attachment 290317 [details] [review]:

Replaced by https://bugzilla.gnome.org/review?bug=739868&attachment=299971
Comment 25 Miguel París Díaz 2015-03-23 11:51:48 UTC
(In reply to Sebastian Dröge (slomo) from comment #23)
> Comment on attachment 290318 [details] [review] [review]
> 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
> 
> This changes the default behaviour from 3000 as rtp-max-dropout to something
> adaptive. Not sure that's a good idea, the default should probably stay as
> is.
> 
By default this property has RTP_MAX_DROPOUT (3000) as value, so it is not adaptative because get_rtp_max_dropout returns this value.

> Then calculate_n_packets_last_second() returns the length of the queue but
> you don't use that. You don't need it?
> 
This function returns the length if the user want to use it. In my case I don't use this returned value but use the length of the queue into get_rtp_max_dropout, so I change it to returns void.

> destroy_GstClockTime() should be renamed to something non-camelcase :)
> clock_time_free() or something.
> 
Done!

> get_rtp_max_dropout() seems to do the wrong thing if we have less than 1
> second of packets so far. If we have more than 1 second it will amount to
> about 0.5s worth of data, if we have less than 1 second (e.g. X=0.5
> seconds), we will only allow 0.25 (X/2) of dropout. I think this code should
> just return some default if we didn't observe 1 second of packets yet.
> 
Done!

> 
> I think the dts queue also needs to be reset whenever the jitterbuffer is
> reset, otherwise it will contain old data that is not relevant anymore.
Done!
Comment 26 Miguel París Díaz 2015-03-23 11:53:11 UTC
(In reply to Sebastian Dröge (slomo) from comment #20)
> Review of attachment 290315 [details] [review] [review]:
> 
> This looks correct to me. Adding the retransmissions to the jitter will make
> it wrong as the rtptime of the retransmissions will be very much in the past
> (we wanted to have that packet earlier already, right?) but the receive time
> was just now.
> 
> ::: gst/rtpmanager/gstrtpjitterbuffer.c
> @@ +2213,3 @@
>  
> +    if (G_LIKELY (gap > -(priv->rtx_delay_reorder))) {
> +      calculate_jitter (jitterbuffer, dts, rtptime);
> 
> This won't do the right thing if rtx_delay_reorder==-1 ("automatic"). In
> that case we would ignore all expected packets (gap==0) and also all packets
> that are one in the future from our expected packet. With
> rtx_delay_reorder==0 we would also ignore all expected packets.
> 
> Otherwise this check seems correct.

Done!
Comment 27 Miguel París Díaz 2015-03-23 12:01:50 UTC
(In reply to Sebastian Dröge (slomo) from comment #21)
> Comment on attachment 290319 [details] [review] [review]
> 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch
> 
> I don't think this is correct. We also add timers for future packets
> (do_next_seqnum), and add timers for packets for which we just have
> reordering (i.e. a short gap). So assuming that all timers are lost packets
> is not correct, and this will probably cause the jitterbuffer to reset
> itself too early.
Fixed!
Comment 28 Miguel París Díaz 2015-03-23 12:08:56 UTC
Created attachment 300123 [details] [review]
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Comment 29 Miguel París Díaz 2015-03-23 12:09:25 UTC
Created attachment 300124 [details] [review]
0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
Comment 30 Miguel París Díaz 2015-03-23 12:09:55 UTC
Created attachment 300125 [details] [review]
0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch
Comment 31 Miguel París Díaz 2015-03-23 12:11:03 UTC
(In reply to Sebastian Dröge (slomo) from comment #15)
> Comment on attachment 290316 [details] [review] [review]
> 0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch
> 
> There seems to be a real problem here but instead of increasing the number
> we should only reset when we're supposed to: i.e. not when a) we lost 100
> packets in a row, or b) got a rtx packet with a seqnum gap >100.
> Resetting the stats in these two cases seems wrong.
> 
> 
> Question now is how we can distinguish between these cases, and the case
> when we actually want to reset the stats (because we think a new session
> started?)

Could we accept this patch for the moment and then think about a better solution?
Comment 32 Miguel París Díaz 2015-03-24 09:33:29 UTC
Created attachment 300185 [details] [review]
0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
Comment 33 Miguel París Díaz 2015-03-24 09:33:50 UTC
Created attachment 300186 [details] [review]
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Comment 34 Miguel París Díaz 2015-03-24 09:34:10 UTC
Created attachment 300187 [details] [review]
0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch
Comment 35 Sebastian Dröge (slomo) 2015-04-13 07:12:30 UTC
Comment on attachment 290483 [details] [review]
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch

This one does not apply to latest git master anymore, please update it :) And while at it, please also improve the documentation a bit to be more descriptive. See my other comment about this (comment 19)
Comment 36 Miguel París Díaz 2015-04-13 09:23:11 UTC
Created attachment 301447 [details] [review]
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch
Comment 37 Sebastian Dröge (slomo) 2015-04-14 17:37:47 UTC
(In reply to Miguel París Díaz from comment #31)
> (In reply to Sebastian Dröge (slomo) from comment #15)
> > Comment on attachment 290316 [details] [review] [review] [review]
> > 0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch
> > 
> > There seems to be a real problem here but instead of increasing the number
> > we should only reset when we're supposed to: i.e. not when a) we lost 100
> > packets in a row, or b) got a rtx packet with a seqnum gap >100.
> > Resetting the stats in these two cases seems wrong.
> > 
> > 
> > Question now is how we can distinguish between these cases, and the case
> > when we actually want to reset the stats (because we think a new session
> > started?)
> 
> Could we accept this patch for the moment and then think about a better
> solution?

No, but I think I have a reproducible testcase for this now... and also think I know what the problem is. The fix will only work properly if you use the rtxsend/rtxreceive elements, not rtxqueue as you currently do (which is also not conforming to the RFC).
Comment 38 Sebastian Dröge (slomo) 2015-04-15 14:07:31 UTC
See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug. Please confirm that this is what you're seeing.
Comment 39 Sebastian Dröge (slomo) 2015-04-17 11:42:39 UTC
(In reply to Miguel París Díaz from comment #36)
> Created attachment 301447 [details] [review] [review]
> 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch

For this one, also see bug #748041
Comment 40 Miguel París Díaz 2015-04-20 10:23:53 UTC
(In reply to Sebastian Dröge (slomo) from comment #38)
> See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and
> https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug.
> Please confirm that this is what you're seeing.

The main problem is that RTCP stats are reset into rtpsource
(https://bugzilla.gnome.org/review?bug=739868&attachment=290316).

The point is why RTP_MAX_MISORDER is 100, and not 500, 2000 or 3000?

Moreover, if the solution is based on RFC4588 (using rtprtxreceive and rtprtxsend) we will keep the error if the peer does not support this RFC.
Comment 41 Miguel París Díaz 2015-04-20 10:27:25 UTC
(In reply to Sebastian Dröge (slomo) from comment #39)
> (In reply to Miguel París Díaz from comment #36)
> > Created attachment 301447 [details] [review] [review] [review]
> > 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch
> 
> For this one, also see bug #748041

Your description is very good and matches with my observations. What do you think about providing the patch to the new bug?
Comment 42 Sebastian Dröge (slomo) 2015-04-20 11:49:53 UTC
(In reply to Miguel París Díaz from comment #40)
> (In reply to Sebastian Dröge (slomo) from comment #38)
> > See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and
> > https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug.
> > Please confirm that this is what you're seeing.
> 
> The main problem is that RTCP stats are reset into rtpsource
> (https://bugzilla.gnome.org/review?bug=739868&attachment=290316).
> 
> The point is why RTP_MAX_MISORDER is 100, and not 500, 2000 or 3000?

Because even 100 is already very unlikely to be packet reordering. Changing that will only hide other problems, not solve anything.

Your problem is not packet reordering I think, but delayed RTX. See the other bug :) We should fix that better somehow than changing some magic number.

> Moreover, if the solution is based on RFC4588 (using rtprtxreceive and
> rtprtxsend) we will keep the error if the peer does not support this RFC.

Well, you should not enable RTX if the peer does not support it.
Comment 43 Miguel París Díaz 2015-04-21 20:26:16 UTC
(In reply to Sebastian Dröge (slomo) from comment #42)
> (In reply to Miguel París Díaz from comment #40)
> > (In reply to Sebastian Dröge (slomo) from comment #38)
> > > See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and
> > > https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug.
> > > Please confirm that this is what you're seeing.
> > 
> > The main problem is that RTCP stats are reset into rtpsource
> > (https://bugzilla.gnome.org/review?bug=739868&attachment=290316).
> > 
> > The point is why RTP_MAX_MISORDER is 100, and not 500, 2000 or 3000?
> 
> Because even 100 is already very unlikely to be packet reordering. Changing
> that will only hide other problems, not solve anything.
> 
> Your problem is not packet reordering I think, but delayed RTX. See the
> other bug :) We should fix that better somehow than changing some magic
> number.
> 
Yes, I agree with you and I also think that it is a better solution.

> > Moreover, if the solution is based on RFC4588 (using rtprtxreceive and
> > rtprtxsend) we will keep the error if the peer does not support this RFC.
> 
> Well, you should not enable RTX if the peer does not support it.
The point here is that there are some clients that supports NACK but do not implement RFC4588. So we should take into account this.
Comment 44 Sebastian Dröge (slomo) 2015-04-21 20:27:56 UTC
(In reply to Miguel París Díaz from comment #43)

> > > Moreover, if the solution is based on RFC4588 (using rtprtxreceive and
> > > rtprtxsend) we will keep the error if the peer does not support this RFC.
> > 
> > Well, you should not enable RTX if the peer does not support it.
> The point here is that there are some clients that supports NACK but do not
> implement RFC4588. So we should take into account this.

What do they implement then, how does it work, how is it signalled in the SDP and is there an RFC for that?
Comment 45 Miguel París Díaz 2015-04-21 20:38:42 UTC
One of this client is Firefox (WebRtc). It uses the same SSRC for original-RTP and rtx-RTP. An SDP example:

v=0
o=mozilla...THIS_IS_SDPARTA-37.0.1 772883292565269152 0 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 98:EE:73:69:EC:81:FB:57:FF:19:03:20:5E:1E:4F:5E:CE:A3:6D:80:84:40:ED:E2:A4:47:78:AF:4E:27:56:0F
a=group:BUNDLE sdparta_0 sdparta_1
a=ice-options:trickle
m=audio 9 RTP/SAVPF 109 9 0 8
c=IN IP4 0.0.0.0
a=sendrecv
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=ice-pwd:383e0361d8982a7f5f8d461a58c4ac2c
a=ice-ufrag:1d20324c
a=mid:sdparta_0
a=rtcp-mux
a=rtpmap:109 opus/48000/2
a=rtpmap:9 G722/8000/1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=setup:actpass
a=ssrc:1032149603 cname:{17f63fe4-1f23-4bce-b38f-f41d487ebaeb}
m=video 9 RTP/SAVPF 120 126 97
c=IN IP4 0.0.0.0
a=sendrecv
a=fmtp:120 max-fs=12288;max-fr=60
a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1
a=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1
a=ice-pwd:383e0361d8982a7f5f8d461a58c4ac2c
a=ice-ufrag:1d20324c
a=mid:sdparta_1
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 ccm fir
a=rtcp-fb:126 nack
a=rtcp-fb:126 nack pli
a=rtcp-fb:126 ccm fir
a=rtcp-fb:97 nack
a=rtcp-fb:97 nack pli
a=rtcp-fb:97 ccm fir
a=rtcp-mux
a=rtpmap:120 VP8/90000
a=rtpmap:126 H264/90000
a=rtpmap:97 H264/90000
a=setup:actpass
a=ssrc:3142525711 cname:{17f63fe4-1f23-4bce-b38f-f41d487ebaeb}
Comment 46 Sebastian Dröge (slomo) 2015-04-21 20:43:41 UTC
Ok, but it uses a different PT. That's not ideal but AFAIU also works with rtxreceive/rtxsend and also seems to be "allowed" by the RFC (and discouraged). Just curious but does it also require us to send RTX packets with the same SSRC?
Also even if Firefox would send us RTX packets with the same SSRC and PT, it should work just fine.

FWIW, I'm fairly sure that Firefox' WebRTC stuff works fine with our RTX. I think it was tested for OpenWebRTC (which only uses rtxsend/receive).
Comment 47 Sebastian Dröge (slomo) 2015-04-21 20:48:34 UTC
Oh nevermind, that SDP is not even RTX if I read that correctly.
Comment 48 Sebastian Dröge (slomo) 2015-04-21 20:54:26 UTC
Does Firefox really retransmit packets if you send a generic NACK? That seems to be not defined by any RFC I read so far. RFC4585 for example does not define any semantics at all for that: https://tools.ietf.org/html/rfc4585#section-6.2.1 (it does define them only in combination with e.g. PLI)
Comment 49 Sebastian Dröge (slomo) 2015-04-21 20:59:06 UTC
Basically what I mean is that if I understand this correctly this SDP only tells us that we can send generic NACKs... and then something may happen or not.
And if you on the sender side add an rtxqueue or not, is really your decision. Nothing is required at all here, you only say you can receive generic NACKs (and maybe you use them only for statistical purposes).


Anyway, rtxqueue is still going to work. The latest plans for fixing all this are independent of which elements are used.
Comment 50 Sebastian Dröge (slomo) 2015-04-22 17:51:42 UTC
Comment on attachment 301447 [details] [review]
0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch

commit f81c9a956835247b02ab69509021cb39446840ad
Author: Miguel París Díaz <mparisdiaz@gmail.com>
Date:   Mon Apr 13 11:20:40 2015 +0200

    rtpjitterbuffer: Add "rtx-next-seqnum" property
    
    If this is set to FALSE, rtpjitterbuffer will not request retransmissions for
    future packets based on when they are estimated to arrive.
    
    See also https://bugzilla.gnome.org/show_bug.cgi?id=748041
    
    https://bugzilla.gnome.org/show_bug.cgi?id=739868
Comment 51 Sebastian Dröge (slomo) 2015-04-22 17:52:48 UTC
Comment on attachment 300185 [details] [review]
0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch

Wim said on IRC that we should handle all packets the same here
Comment 52 Sebastian Dröge (slomo) 2015-04-22 17:56:10 UTC
Comment on attachment 300186 [details] [review]
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch

This does not apply to git master anymore, it looks like you accidentially merged another patch in here that is merged in master already.
Comment 53 Miguel París Díaz 2015-04-22 22:02:00 UTC
(In reply to Sebastian Dröge (slomo) from comment #48)
> Does Firefox really retransmit packets if you send a generic NACK? That
> seems to be not defined by any RFC I read so far. RFC4585 for example does
> not define any semantics at all for that:
> https://tools.ietf.org/html/rfc4585#section-6.2.1 (it does define them only
> in combination with e.g. PLI)

Hello Sebastian,
I have done some tests with Firefox and I can confirm that it retransmits the packets if I request them using generic NACKs.
So, we should take into account this case when applying the solutions about we are thinking.
Comment 54 Miguel París Díaz 2015-04-22 22:06:31 UTC
(In reply to Sebastian Dröge (slomo) from comment #51)
> Comment on attachment 300185 [details] [review] [review]
> 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
> 
> Wim said on IRC that we should handle all packets the same here

I don't agree, because of the case of handling RTX packets(the common problem that we are discussing in other patches).
If this is not a good solution, we should look for another one.
Comment 55 Sebastian Dröge (slomo) 2015-04-23 08:38:56 UTC
(In reply to Miguel París Díaz from comment #53)
> (In reply to Sebastian Dröge (slomo) from comment #48)
> > Does Firefox really retransmit packets if you send a generic NACK? That
> > seems to be not defined by any RFC I read so far. RFC4585 for example does
> > not define any semantics at all for that:
> > https://tools.ietf.org/html/rfc4585#section-6.2.1 (it does define them only
> > in combination with e.g. PLI)
>
> I have done some tests with Firefox and I can confirm that it retransmits
> the packets if I request them using generic NACKs.
> So, we should take into account this case when applying the solutions about
> we are thinking.

Sure, there's currently nothing RTX specific planned at all for these things. Nonetheless what Firefox is doing there is completely undefined and it would be better if they would actually implement the standard for that.

And in the case of Firefox you really have no way to distinguish RTX from normal transmissions. If the RFC is implemented, you can distinguish them based on the PT and SSRC, like one of my patches in the other bugs did (which was not the best solution for that problem, but maybe it is for another one).


(In reply to Miguel París Díaz from comment #54)
> (In reply to Sebastian Dröge (slomo) from comment #51)
> > Comment on attachment 300185 [details] [review] [review] [review]
> > 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
> > 
> > Wim said on IRC that we should handle all packets the same here
> 
> I don't agree, because of the case of handling RTX packets(the common
> problem that we are discussing in other patches).
> If this is not a good solution, we should look for another one.

How does this patch relate to any of the others? Wim's argument basically was that there is nothing else we can do than to consider RTX packets also for the jitter calculations. And it might also be just jitter in the end.
What exactly would your patch solve? What's the problem you're observing?
Comment 56 Miguel París Díaz 2015-04-23 17:34:35 UTC
Created attachment 302253 [details] [review]
0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Comment 57 Miguel París Díaz 2015-04-23 17:56:08 UTC
(In reply to Sebastian Dröge (slomo) from comment #55)
> (In reply to Miguel París Díaz from comment #54)
> > (In reply to Sebastian Dröge (slomo) from comment #51)
> > > Comment on attachment 300185 [details] [review] [review] [review] [review]
> > > 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
> > > 
> > > Wim said on IRC that we should handle all packets the same here
> > 
> > I don't agree, because of the case of handling RTX packets(the common
> > problem that we are discussing in other patches).
> > If this is not a good solution, we should look for another one.
> 
> How does this patch relate to any of the others?
I mean that jitterbuffer has problems if manage RTX packets as the "original" packets.
We have been discussing about how to avoid this case using RFC4588, using other PT and/or SSRC in the RTX packets, but we have to take into account that there are clients (as Firefox) that do no do this correctly. So:
 - Could we do something to do GStreamer works fine in these cases?
 - Should we assume this case and keep the current behaviour?

> Wim's argument basically
> was that there is nothing else we can do than to consider RTX packets also
> for the jitter calculations. And it might also be just jitter in the end.
> What exactly would your patch solve? What's the problem you're observing?

This patch aims to avoid using RTX packets for jitter calculation because it is done basing on the last received packet, and this is not real.
Comment 58 Miguel París Díaz 2015-05-13 13:20:07 UTC
Hello again Sebastian,
what do you thing about the new patches?:
 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch 
 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Comment 59 Sebastian Dröge (slomo) 2015-05-13 16:18:08 UTC
I didn't have time yet to think about them again, sorry. They're still on my list though.

But also see the linked bugs, that might solve your problems better and some feedback from you there would be useful.
Comment 60 Sebastian Dröge (slomo) 2015-05-18 15:53:08 UTC
Miguel, can you test if the changes in bug #747922 are helping with your problems (while not using your max-dropout patch).
Comment 61 Miguel París Díaz 2015-05-19 09:53:48 UTC
Hello Sebastian,
I have to adapt my testing tools and reproduce losses scenarios and so on to check it.
I hope do it as soon as I have a time slot.
Comment 62 Miguel París Díaz 2015-07-06 12:02:09 UTC
Review of attachment 300187 [details] [review]:

Rejected in favour this: https://bugzilla.gnome.org/show_bug.cgi?id=751636
Comment 63 Miguel París Díaz 2015-07-06 12:02:45 UTC
Review of attachment 302253 [details] [review]:

Rejected in favour this: https://bugzilla.gnome.org/show_bug.cgi?id=751311
Comment 64 Miguel París Díaz 2015-07-06 12:03:39 UTC
Review of attachment 290316 [details] [review]:

Rejected in favour of this: https://bugzilla.gnome.org/show_bug.cgi?id=751311