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 751311 - rtp: Dynamic dropout / reorder limits
rtp: Dynamic dropout / reorder limits
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-06-22 09:30 UTC by Jose Antonio Santos Cadenas
Modified: 2016-08-29 20:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtpsource: Do not try to push NULL buffers (1.08 KB, patch)
2015-06-22 09:30 UTC, Jose Antonio Santos Cadenas
committed Details | Review
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch (2.77 KB, patch)
2015-06-30 12:07 UTC, Miguel París Díaz
none Details | Review
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch (2.33 KB, patch)
2015-06-30 12:07 UTC, Miguel París Díaz
none Details | Review
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch (19.79 KB, patch)
2015-06-30 12:08 UTC, Miguel París Díaz
none Details | Review
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch (9.26 KB, patch)
2015-06-30 12:08 UTC, Miguel París Díaz
none Details | Review
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch (2.74 KB, patch)
2015-06-30 15:15 UTC, Miguel París Díaz
none Details | Review
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch (2.87 KB, patch)
2015-06-30 15:15 UTC, Miguel París Díaz
none Details | Review
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch (19.81 KB, patch)
2015-06-30 15:16 UTC, Miguel París Díaz
none Details | Review
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch (8.78 KB, patch)
2015-06-30 15:16 UTC, Miguel París Díaz
none Details | Review
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch (2.73 KB, patch)
2015-06-30 15:37 UTC, Miguel París Díaz
none Details | Review
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch (2.90 KB, patch)
2015-06-30 15:37 UTC, Miguel París Díaz
none Details | Review
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch (2.92 KB, patch)
2015-06-30 22:45 UTC, Miguel París Díaz
none Details | Review
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch (2.90 KB, patch)
2015-06-30 22:45 UTC, Miguel París Díaz
none Details | Review
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch (19.85 KB, patch)
2015-07-01 08:06 UTC, Miguel París Díaz
none Details | Review
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch (8.79 KB, patch)
2015-07-01 08:07 UTC, Miguel París Díaz
none Details | Review
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch (2.90 KB, patch)
2015-07-01 13:57 UTC, Miguel París Díaz
none Details | Review
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch (19.85 KB, patch)
2015-07-01 13:57 UTC, Miguel París Díaz
none Details | Review
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch (8.79 KB, patch)
2015-07-01 13:58 UTC, Miguel París Díaz
none Details | Review
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch (2.84 KB, patch)
2015-07-01 13:58 UTC, Miguel París Díaz
none Details | Review
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch (3.20 KB, patch)
2015-07-01 15:08 UTC, Miguel París Díaz
committed Details | Review
0002-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch (19.85 KB, patch)
2015-07-01 15:09 UTC, Miguel París Díaz
none Details | Review
0003-rtpmanager-take-into-account-packet-rate-for-max-dro.patch (13.82 KB, patch)
2015-07-01 15:09 UTC, Miguel París Díaz
none Details | Review
0003-rtpmanager-take-into-account-packet-rate-for-max-dro.patch (15.65 KB, patch)
2015-07-06 11:59 UTC, Miguel París Díaz
none Details | Review
rtpmanager: add "max-dropout-time" and "max-misorder-time" props (22.78 KB, patch)
2015-10-06 21:00 UTC, Miguel París Díaz
none Details | Review
rtpmanager: take into account packet rate for max-dropout and max-misorder calc (13.24 KB, patch)
2015-10-06 21:01 UTC, Miguel París Díaz
none Details | Review
rtpmanager: add "max-dropout-time" and "max-misorder-time" props (20.11 KB, patch)
2015-10-07 11:04 UTC, Miguel París Díaz
committed Details | Review
rtpmanager: take into account packet rate for max-dropout and max-misorder calc (14.29 KB, patch)
2015-10-07 11:04 UTC, Miguel París Díaz
committed Details | Review
rtpjitterbuffer: Actually calculate the packet rate for max-dropout and max-misorder calculations. (2.89 KB, patch)
2016-08-10 16:54 UTC, GstBlub
none Details | Review
rtpjitterbuffer: Actually calculate the packet rate for max-dropout and max-misorder calculations. (2.75 KB, patch)
2016-08-10 17:46 UTC, GstBlub
committed Details | Review

Description Jose Antonio Santos Cadenas 2015-06-22 09:30:46 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.
Comment 1 Sebastian Dröge (slomo) 2015-06-22 09:50:29 UTC
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.
Comment 2 Jose Antonio Santos Cadenas 2015-06-22 09:53:58 UTC
From chrome when there are reception losses
Comment 3 Sebastian Dröge (slomo) 2015-06-22 10:02:24 UTC
But that's on your sender side when packets are sent. Why are you sending packets with broken seqnums? :)
Comment 4 Jose Antonio Santos Cadenas 2015-06-22 10:09:43 UTC
That's true, no idea, payload is directly connected to rtpbin, so nothing special here that could change sequence numbers.
Comment 5 Sebastian Dröge (slomo) 2015-06-22 10:11:46 UTC
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.
Comment 6 Miguel París Díaz 2015-06-22 10:15:40 UTC
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).
Comment 7 Sebastian Dröge (slomo) 2015-06-22 10:21:07 UTC
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.
Comment 8 Miguel París Díaz 2015-06-22 10:33:00 UTC
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?
Comment 9 Sebastian Dröge (slomo) 2015-06-22 11:04:02 UTC
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)
Comment 10 Sebastian Dröge (slomo) 2015-06-22 12:19:13 UTC
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?
Comment 11 Miguel París Díaz 2015-06-22 16:36:06 UTC
I also like more using "time" units for this properties.

Sebastian, which patches do you mean?
Comment 12 Sebastian Dröge (slomo) 2015-06-22 20:02:26 UTC
Didn't you have patches related to this in the other bug?
Comment 13 Miguel París Díaz 2015-06-23 08:26:43 UTC
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?
Comment 14 Sebastian Dröge (slomo) 2015-06-23 08:43:58 UTC
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)
Comment 15 Miguel París Díaz 2015-06-23 09:03:08 UTC
OK, I take note, thanks!!
Comment 16 Miguel París Díaz 2015-06-30 12:07:19 UTC
Created attachment 306390 [details] [review]
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Comment 17 Miguel París Díaz 2015-06-30 12:07:42 UTC
Created attachment 306391 [details] [review]
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Comment 18 Miguel París Díaz 2015-06-30 12:08:02 UTC
Created attachment 306393 [details] [review]
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Comment 19 Miguel París Díaz 2015-06-30 12:08:19 UTC
Created attachment 306394 [details] [review]
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Comment 20 Sebastian Dröge (slomo) 2015-06-30 14:00:38 UTC
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.
Comment 21 Sebastian Dröge (slomo) 2015-06-30 14:03:16 UTC
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
Comment 22 Sebastian Dröge (slomo) 2015-06-30 14:08:29 UTC
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.
Comment 23 Sebastian Dröge (slomo) 2015-06-30 14:11:42 UTC
Review of attachment 306394 [details] [review]:

Looks good
Comment 24 Miguel París Díaz 2015-06-30 14:15:04 UTC
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
Comment 25 Sebastian Dröge (slomo) 2015-06-30 14:18:35 UTC
(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).
Comment 26 Miguel París Díaz 2015-06-30 15:15:35 UTC
Created attachment 306416 [details] [review]
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Comment 27 Miguel París Díaz 2015-06-30 15:15:58 UTC
Created attachment 306417 [details] [review]
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Comment 28 Miguel París Díaz 2015-06-30 15:16:19 UTC
Created attachment 306418 [details] [review]
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Comment 29 Miguel París Díaz 2015-06-30 15:16:39 UTC
Created attachment 306419 [details] [review]
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Comment 30 Miguel París Díaz 2015-06-30 15:19:11 UTC
Review of attachment 306416 [details] [review]:

This does not work now, I am reviewing it.
Comment 31 Miguel París Díaz 2015-06-30 15:37:35 UTC
Created attachment 306420 [details] [review]
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Comment 32 Miguel París Díaz 2015-06-30 15:37:54 UTC
Created attachment 306421 [details] [review]
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Comment 33 Sebastian Dröge (slomo) 2015-06-30 16:25:50 UTC
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)
Comment 34 Sebastian Dröge (slomo) 2015-06-30 16:27:54 UTC
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
Comment 35 Miguel París Díaz 2015-06-30 22:45:28 UTC
Created attachment 306447 [details] [review]
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Comment 36 Miguel París Díaz 2015-06-30 22:45:46 UTC
Created attachment 306448 [details] [review]
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Comment 37 Miguel París Díaz 2015-07-01 08:06:47 UTC
Created attachment 306468 [details] [review]
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Comment 38 Miguel París Díaz 2015-07-01 08:07:09 UTC
Created attachment 306469 [details] [review]
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Comment 39 Sebastian Dröge (slomo) 2015-07-01 11:56:53 UTC
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.
Comment 40 Sebastian Dröge (slomo) 2015-07-01 11:59:24 UTC
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
Comment 41 Sebastian Dröge (slomo) 2015-07-01 12:02:55 UTC
Review of attachment 306468 [details] [review]:

Please merge this patch with the next one
Comment 42 Sebastian Dröge (slomo) 2015-07-01 12:05:07 UTC
Review of attachment 306469 [details] [review]:

Looks good but merge with previous patch :)
Comment 43 Miguel París Díaz 2015-07-01 13:57:33 UTC
Created attachment 306508 [details] [review]
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Comment 44 Miguel París Díaz 2015-07-01 13:57:52 UTC
Created attachment 306509 [details] [review]
0002-rtpstats-add-utility-for-calculating-max_dropout-and.patch
Comment 45 Miguel París Díaz 2015-07-01 13:58:29 UTC
Created attachment 306510 [details] [review]
0003-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Comment 46 Miguel París Díaz 2015-07-01 13:58:50 UTC
Created attachment 306512 [details] [review]
0004-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Comment 47 Sebastian Dröge (slomo) 2015-07-01 15:00:50 UTC
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.
Comment 48 Miguel París Díaz 2015-07-01 15:08:30 UTC
Created attachment 306533 [details] [review]
0001-rtpstats-add-utility-for-calculating-RTP-packet-rate.patch
Comment 49 Miguel París Díaz 2015-07-01 15:09:01 UTC
Created attachment 306534 [details] [review]
0002-rtpmanager-add-max-dropout-time-and-max-misorder-tim.patch
Comment 50 Miguel París Díaz 2015-07-01 15:09:37 UTC
Created attachment 306535 [details] [review]
0003-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Comment 51 Miguel París Díaz 2015-07-06 11:59:48 UTC
Created attachment 306914 [details] [review]
0003-rtpmanager-take-into-account-packet-rate-for-max-dro.patch
Comment 52 Sebastian Dröge (slomo) 2015-10-02 14:05:59 UTC
Can you rebase the patches against latest GIT master?
Comment 53 Luis de Bethencourt 2015-10-02 18:26:05 UTC
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
Comment 54 Miguel París Díaz 2015-10-06 21:00:48 UTC
Created attachment 312766 [details] [review]
rtpmanager: add "max-dropout-time" and "max-misorder-time" props
Comment 55 Miguel París Díaz 2015-10-06 21:01:15 UTC
Created attachment 312768 [details] [review]
rtpmanager: take into account packet rate for max-dropout and max-misorder calc
Comment 56 Sebastian Dröge (slomo) 2015-10-07 10:48:30 UTC
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.
Comment 57 Miguel París Díaz 2015-10-07 11:01:20 UTC
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 ;)
Comment 58 Miguel París Díaz 2015-10-07 11:04:00 UTC
Created attachment 312812 [details] [review]
rtpmanager: add "max-dropout-time" and "max-misorder-time" props
Comment 59 Miguel París Díaz 2015-10-07 11:04:37 UTC
Created attachment 312813 [details] [review]
rtpmanager: take into account packet rate for max-dropout and max-misorder calc
Comment 60 Sebastian Dröge (slomo) 2015-10-07 11:05:48 UTC
Also note that even for non-trivial changes we very often don't do that, it's just noise :) Thanks for updating the patch.
Comment 61 Sebastian Dröge (slomo) 2015-10-07 11:09:49 UTC
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
Comment 62 GstBlub 2016-08-10 16:54:10 UTC
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.
Comment 63 Sebastian Dröge (slomo) 2016-08-10 17:39:36 UTC
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.
Comment 64 GstBlub 2016-08-10 17:43:10 UTC
Oh, I didn't realize that rounding was the purpose.  It looked like an odd way of averaging.  I'll update the patch.
Comment 65 GstBlub 2016-08-10 17:46:31 UTC
Created attachment 333076 [details] [review]
rtpjitterbuffer: Actually calculate the packet rate for max-dropout and max-misorder calculations.

Updated patch.
Comment 66 Sebastian Dröge (slomo) 2016-08-10 17:50:10 UTC
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.
Comment 67 Sebastian Dröge (slomo) 2016-08-18 06:59:12 UTC
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
Comment 68 Miguel París Díaz 2016-08-29 10:33:18 UTC
@slomo, will these changes be landed in 1.8 branch?
Comment 69 Sebastian Dröge (slomo) 2016-08-29 10:52:55 UTC
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.
Comment 70 Miguel París Díaz 2016-08-29 20:20:48 UTC
I totally agree, thanks!!