GNOME Bugzilla – Bug 751311
rtp: Dynamic dropout / reorder limits
Last modified: 2016-08-29 20:20:48 UTC
Created attachment 305812 [details] [review] rtpsource: Do not try to push NULL buffers Sometimes, rtpsource shows a buffer list verification error. The issue comes from update_receiver_stats function, because ->data is set to NULL due to a bad_sequence number. When we try to pusth ->data set to NULL buffer_list check fails. Attached is a patch to solve the issue.
How can this happen, from where do you get the packets? It would mean that you send a stream with e.g. broken sequence numbers.
From chrome when there are reception losses
But that's on your sender side when packets are sent. Why are you sending packets with broken seqnums? :)
That's true, no idea, payload is directly connected to rtpbin, so nothing special here that could change sequence numbers.
Please debug that first. Do you have a rtxqueue upstream there? That could probably insert quite old packet seqnums again, which could then be dropped by rtpsource and defeat the purpose of rtxqueue.
Hello Sebastian, there are not broken seqnums, the condition that fails is this one: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtpmanager/rtpsource.c?id=1.5.1#n1062 Again, the point here is the value of RTP_MAX_MISORDER (100) as the same that we were discussing here: https://bugzilla.gnome.org/show_bug.cgi?id=739868#c43 I think, that this value comes from this example in RFC3550: https://tools.ietf.org/html/rfc3550#appendix-A.1, where: const int MAX_DROPOUT = 3000; const int MAX_MISORDER = 100; "Typical values for the parameters are shown, based on a maximum misordering time of 2 seconds at 50 packets/second and a maximum dropout of 1 minute." First of all, this is only an example and I do not agree with having a fix vale of 100, why not more or less? For example, take into account that when sending VP8 video at 2Mbps there are about 200 (even more) packets per second, so we have problems with RTX requests with a delay of 500ms (something assumable in real networks, in real use cases).
If you have a RTX delay of 500ms, you can't really do any realtime streaming anymore. So yes, we should probably make those values a) configurable and b) add a mode to automatically select something based on the average packet rate. Jose's patch will only drop the RTX packets then, but that's probably still better than asserting on a NULL buffer.
Again, depends on the use case, the network, etc. The case of 2Mbps is another example, imagine another of 4Mbps, or that you have punctual losses in your network. From my point of view, the best approach is that GStreamer allows developer to decide about their product behaviour. So I like your proposal. Anyway Jose's patch is needed to avoid GStreamer-CRITICAL **: gst_pad_push_list: assertion 'GST_IS_BUFFER_LIST (list)' and then add a property for this value. What do you think?
I would like to have properties for these values, and also some automatic/adaptive calculation based on the average packet rate and bit rate (which should probably come up with 3000/100 for the example case and other things for other...). Or maybe the properties should actually be in time (misordering=2s, dropout=1m by default), then calculate the average bitrate and packet rate unless given by another two properties? I think this approach would make most sense in the end. (For calculating averages here, use the recursive running average we use in other places I guess: avg = (3 * avg + new_value) / 4)
I think with this, it would be an acceptable solution and we can also get rid of the other bug finally :) Miguel, can you update your patches accordingly for this and attach them here?
I also like more using "time" units for this properties. Sebastian, which patches do you mean?
Didn't you have patches related to this in the other bug?
OK, you are right. The point now is that RTP_MAX_MISORDER is defined in rtpstats.h and used into gstrtpjitterbuffer and rtpsource. So, where do you think that we can add the property?
I guess as usual, properties everywhere :) rtpbin, rtpsession, rtpsource and rtpjitterbuffer, most of them just proxying the values further down. This would then be max-dropout-time max-misorder-time And I think on all but rtpbin you would also have packet-rate (bitrate is already there, it's called bandwidth and automatically calculated by default)
OK, I take note, thanks!!
Created attachment 306390 [details] [review] 0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Created attachment 306391 [details] [review] 0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Created attachment 306393 [details] [review] 0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Created attachment 306394 [details] [review] 0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Review of attachment 306390 [details] [review]: ::: gst/rtpmanager/rtpstats.c @@ +39,3 @@ +guint32 +gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx * ctx, guint16 seqnum, + guint32 ts, gint32 clock_rate) Shouldn't the clock_rate be in some kind of "init" function only? @@ +60,3 @@ + } + + diff_ts = ts - ctx->last_ts; This doesn't handle timestamps wraparounds, or you have to require the caller to provide ext RTP timestamps already (but those should be guint64 then) Also can't the caller just provide nanoseconds time? All callers should already calculate that @@ +63,3 @@ + diff_ts = gst_util_uint64_scale_int (diff_ts, GST_USECOND, clock_rate); + ctx->avg_packet_rate = + ((7 * (ctx->avg_packet_rate + 1)) + (1000 * diff_seqnum / diff_ts)) / 8; Why USECOND? Why not diff_ts = gst_util_uint64_scale_int (diff_ts, GST_SECOND, clock_rate); /* now diff_ts is nanoseconds */ avg_packet_rate = (7 * (avg_packet_rate + 1) + gst_util_uint64_scale (diff_seqnum, GST_SECOND, diff_ts)) / 8 ::: gst/rtpmanager/rtpstats.h @@ +209,3 @@ +void gst_rtp_packet_rate_ctx_destroy (RTPPacketRateCtx *ctx); +guint32 gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx *ctx, guint16 seqnum, guint32 ts, gint32 clock_rate); +guint32 gst_rtp_packet_rate_ctx_get (RTPPacketRateCtx *ctx); Maybe require the caller to allocate that, and then only provide a gst_rtp_packet_rate_ctx_reset() that sets default values to all fields.
Review of attachment 306391 [details] [review]: ::: gst/rtpmanager/rtpstats.h @@ +194,3 @@ */ #define RTP_MAX_MISORDER 100 +#define RTP_MIN_MISORDER 10 I would keep the old values as minimum, and impose no maximum. Your goal here is to use higher numbers than we do now
Review of attachment 306393 [details] [review]: ::: gst/rtpmanager/gstrtpbin.c @@ +2204,3 @@ + g_object_class_install_property (gobject_class, PROP_MAX_MISORDER_TIME, + g_param_spec_int ("max-misorder-time", "Max misorder time", + "The maximum time (milliseconds) of misordered packets tolerated.", Why milliseconds and not nanoseconds? ::: gst/rtpmanager/rtpsource.c @@ +42,3 @@ #define DEFAULT_PROBATION RTP_DEFAULT_PROBATION +#define DEFAULT_MAX_DROPOUT_TIME -1 +#define DEFAULT_MAX_MISORDER_TIME -1 The defaults should probably be like in the RFC: 1 minute and 2 seconds. And we could make -1 mean to never consider something as dropout or outside reordering.
Review of attachment 306394 [details] [review]: Looks good
Review of attachment 306390 [details] [review]: ::: gst/rtpmanager/rtpstats.c @@ +39,3 @@ +guint32 +gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx * ctx, guint16 seqnum, + guint32 ts, gint32 clock_rate) What happens if it changes? @@ +60,3 @@ + } + + diff_ts = ts - ctx->last_ts; How can I manage wraparounds? There is any utility? Do you mean use this values?: rtpsource: RTPPacketInfo.ntpnstime jitterbuffer: dts @@ +63,3 @@ + diff_ts = gst_util_uint64_scale_int (diff_ts, GST_USECOND, clock_rate); + ctx->avg_packet_rate = + ((7 * (ctx->avg_packet_rate + 1)) + (1000 * diff_seqnum / diff_ts)) / 8; This may be more inefficient? Or something insignificant? ::: gst/rtpmanager/rtpstats.h @@ +209,3 @@ +void gst_rtp_packet_rate_ctx_destroy (RTPPacketRateCtx *ctx); +guint32 gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx *ctx, guint16 seqnum, guint32 ts, gint32 clock_rate); +guint32 gst_rtp_packet_rate_ctx_get (RTPPacketRateCtx *ctx); I do not know what do you mean
(In reply to Miguel París Díaz from comment #24) > Review of attachment 306390 [details] [review] [review]: > > ::: gst/rtpmanager/rtpstats.c > @@ +39,3 @@ > +guint32 > +gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx * ctx, guint16 seqnum, > + guint32 ts, gint32 clock_rate) > > What happens if it changes? The caller calls gst_rtp_packet_rate_ctx_reset() with the new clock rate > @@ +60,3 @@ > + } > + > + diff_ts = ts - ctx->last_ts; > > How can I manage wraparounds? > There is any utility? gst_rtp_buffer_ext_timestamp() > Do you mean use this values?: > rtpsource: RTPPacketInfo.ntpnstime > jitterbuffer: dts No, those are different things > @@ +63,3 @@ > + diff_ts = gst_util_uint64_scale_int (diff_ts, GST_USECOND, clock_rate); > + ctx->avg_packet_rate = > + ((7 * (ctx->avg_packet_rate + 1)) + (1000 * diff_seqnum / diff_ts)) / > 8; > > This may be more inefficient? > Or something insignificant? Insignificant, and less confusing :) We already have too many different units in the RTP code, let's not add yet another one. > ::: gst/rtpmanager/rtpstats.h > @@ +209,3 @@ > +void gst_rtp_packet_rate_ctx_destroy (RTPPacketRateCtx *ctx); > +guint32 gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx *ctx, guint16 > seqnum, guint32 ts, gint32 clock_rate); > +guint32 gst_rtp_packet_rate_ctx_get (RTPPacketRateCtx *ctx); > > I do not know what do you mean E.g. GstRTPSource struct would contain struct _GstRTPSource { ... RTPPacketRateCtx packet_rate_ctx; /* no pointer */ ... } and would call things like gst_rtp_packet_rate_ctx_reset(&src->packet_rate_ctx, src->clock_rate).
Created attachment 306416 [details] [review] 0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Created attachment 306417 [details] [review] 0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Created attachment 306418 [details] [review] 0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Created attachment 306419 [details] [review] 0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Review of attachment 306416 [details] [review]: This does not work now, I am reviewing it.
Created attachment 306420 [details] [review] 0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Created attachment 306421 [details] [review] 0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Review of attachment 306421 [details] [review]: ::: gst/rtpmanager/rtpstats.c @@ +78,3 @@ + } + + return MAX (RTP_MIN_DROPOUT, ctx->avg_packet_rate * time_ms / 1000); I still don't like this minimum value (and the one for the misorder). What about setting a minimum value for the packet rate instead? The minimum you have there would allow very very low limits for dropout, which too easily can lead to false positives. > return MAX (RTP_MIN_DROPOUT_OR_MISORDER_PACKET_RATE, ctx->avg_packet_rate) * time_ms / 1000 and RTP_MIN_DROPOUT_OR_MISORDER_PACKET_RATE would be 50 (example from the RFC) ::: gst/rtpmanager/rtpstats.h @@ +192,2 @@ /* + * The default and eht minimum values of the maximum number of misordered packets we tolerate. typo: eht -> the (but in the other comment you omitted the "the" completely, so just remove it here)
Review of attachment 306418 [details] [review]: ::: gst/rtpmanager/rtpsource.c @@ +227,3 @@ + g_param_spec_int ("max-dropout-time", "Max dropout time", + "The maximum time (milliseconds) of missing packets tolerated.", + -1, G_MAXINT, DEFAULT_MAX_DROPOUT_TIME, -1 has no meaning here anymore, maybe set the minimum value to 2s or something? @@ +233,3 @@ + g_param_spec_int ("max-misorder-time", "Max misorder time", + "The maximum time (milliseconds) of misordered packets tolerated.", + -1, G_MAXINT, DEFAULT_MAX_MISORDER_TIME, And same here, maybe just set it to 30s or so
Created attachment 306447 [details] [review] 0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Created attachment 306448 [details] [review] 0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Created attachment 306468 [details] [review] 0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Created attachment 306469 [details] [review] 0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Review of attachment 306447 [details] [review]: ::: gst/rtpmanager/rtpstats.c @@ +48,3 @@ + + new_ts = ctx->last_ts; + new_ts = gst_rtp_buffer_ext_timestamp (&new_ts, ts); Return value is unnecessary here, new_ts is already updated by the function @@ +62,3 @@ + } else { + ctx->avg_packet_rate = (ctx->avg_packet_rate + new_packet_rate + 1) / 2; + } Add a comment here why you do this special averaging. The goal being, that higher packet rates "win". If there's a sudden burst, the average will go up fast, but it will go down again slowly. Because otherwise it would happen that a bursty stream (think of 0.1fps stream... lots of packets every 10 seconds and then nothing) would have a very low packet rate... but during the bursts we have a lot of packets close to each other and should allow a higher reorder/dropout there. Otherwise it might lead to false positives too easily. This part here needs to be tested a lot under all possible workloads to not introduce regression, I would defer this to after 1.6.
Review of attachment 306448 [details] [review]: ::: gst/rtpmanager/rtpstats.c @@ +80,3 @@ +{ + if (time_ms <= 0 || !ctx->probed) { + return RTP_DEF_DROPOUT; So if we don't know anything we will return 3000 or 100 here as safe default values, and otherwise use the calculations below ... which could get lower than 3000 or 100 (300 or 10). We need to make sure because of that, that we never run into the cases where we go below the default (and current value) when it's not allowed. Like during bursts
Review of attachment 306468 [details] [review]: Please merge this patch with the next one
Review of attachment 306469 [details] [review]: Looks good but merge with previous patch :)
Created attachment 306508 [details] [review] 0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Created attachment 306509 [details] [review] 0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Created attachment 306510 [details] [review] 0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Created attachment 306512 [details] [review] 0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Comment on attachment 306509 [details] [review] 0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch Should be merged with the patch that actually uses these properties, will do so when merging.
Created attachment 306533 [details] [review] 0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Created attachment 306534 [details] [review] 0002-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Created attachment 306535 [details] [review] 0003-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Created attachment 306914 [details] [review] 0003-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Can you rebase the patches against latest GIT master?
Review of attachment 306533 [details] [review]: commit bf0e4f65b40fb1ee155cf26131c965c40b8bff38 Author: Miguel París Díaz <mparisdiaz@gmail.com> Date: Tue Jun 30 16:56:19 2015 +0200 rtpstats: add utility for calculating RTP packet rate
Created attachment 312766 [details] [review] rtpmanager: add "max-dropout-time" and "max-misorder-time" props
Created attachment 312768 [details] [review] rtpmanager: take into account packet rate for max-dropout and max-misorder calc
Review of attachment 312766 [details] [review]: ::: gst/rtpmanager/gstrtpbin.c @@ +2,3 @@ * Copyright (C) <2007> Wim Taymans <wim.taymans@gmail.com> + * Copyright (C) 2015 Kurento (http://kurento.org/) + * @author: Miguel París <mparisdiaz@gmail.com> We usually don't do that for trivial patches like this one, not sure if this is even copyright'able at all :) Also note that this isn't really required, you automatically own the copyright of all your code. If we did that for each contribution, we would have a huge list of people for this file alone already, gstrtpbin.c alone would have at least 32 names listed here then. For non-trivial contributions (like the follow-up patch, for the files where you actually do non-trivial changes), this seems ok though.
OK, I agree with you and only add copyright for non-trivial changes. So I should add it only in the follow-up path, let me change it ;)
Created attachment 312812 [details] [review] rtpmanager: add "max-dropout-time" and "max-misorder-time" props
Created attachment 312813 [details] [review] rtpmanager: take into account packet rate for max-dropout and max-misorder calc
Also note that even for non-trivial changes we very often don't do that, it's just noise :) Thanks for updating the patch.
commit f321bfeaf4058202cf5ab923ebbf01cab5304eab Author: Miguel París Díaz <mparisdiaz@gmail.com> Date: Wed Oct 7 13:03:02 2015 +0200 rtpmanager: Take into account packet rate for max-dropout and max-misorder calculations https://bugzilla.gnome.org/show_bug.cgi?id=751311 commit 4c96094fbbe545ae5d679b25e68826d8a84a1335 Author: Miguel París Díaz <mparisdiaz@gmail.com> Date: Wed Oct 7 13:02:12 2015 +0200 rtpmanager: add "max-dropout-time" and "max-misorder-time" props https://bugzilla.gnome.org/show_bug.cgi?id=751311
Created attachment 333070 [details] [review] rtpjitterbuffer: Actually calculate the packet rate for max-dropout and max-misorder calculations. I don't think this feature was actually working because it could have never actually calculated the packet rate, causing the default values to be returned in all cases. There are no calls to gst_rtp_packet_rate_ctx_reset() that would have set the clock rate.
Review of attachment 333070 [details] [review]: Looks generally good but: ::: gst/rtpmanager/rtpstats.c @@ +72,2 @@ } else { + ctx->avg_packet_rate = (ctx->avg_packet_rate + new_packet_rate) / 2; Why? The idea here was to round up.
Oh, I didn't realize that rounding was the purpose. It looked like an odd way of averaging. I'll update the patch.
Created attachment 333076 [details] [review] rtpjitterbuffer: Actually calculate the packet rate for max-dropout and max-misorder calculations. Updated patch.
Comment on attachment 333076 [details] [review] rtpjitterbuffer: Actually calculate the packet rate for max-dropout and max-misorder calculations. Attachment 333076 [details] pushed as 4dff743 - rtpjitterbuffer: Actually calculate the packet rate for max-dropout and max-misorder calculations.
commit a1eefe23deb5407c2fd4fb9a8e4550a483cf4c6e Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Aug 18 09:57:51 2016 +0300 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
@slomo, will these changes be landed in 1.8 branch?
No, nobody tested them during 1.8 (as it was never actually enabled) and the impact of those changes is too big. Too risky to do that for 1.8 now.
I totally agree, thanks!!