Can you provide a patch that does not depend on GstHarness? :)

Ping?

Created attachment 306744 [details] [review]
Patch 1/2

The patch reverts a previous commit that introduced a regression. What the reverted patch is trying to solve is fixed more accurately in the next attached patch.

Created attachment 306745 [details] [review]
Patch 2/2

What kind of regression did it introduce? And to make reviewing a bit easier, can you explain how things are supposed to work with your changes? In the commit message you explain how things were before, but getting an idea of the bigger picture of your changes would be useful :)

The test-failure was a wrong timestamp (overflowed?) in the timestamp of the lost-event in the large-gap test.

The patch was to reintroduce the correct behavior on large-gap handling of the lost-event.

The correct behavior for large gaps is to think that when a packet finally arrives (after some time, creating a large gap), you must allow for up to jb-latency ms of buffers still arriving, and not "starting again" from the new packet. Example is that you have received buffers 0 and 1, with 20ms of duration, and then get buffer 500, you must allow for buffers 491-499 still being able to arrive (with default 200ms latency), and hence potentially create lost-events for them if they do not arrive. The test checks that all this is true, and that both the "large-gap" lost-event as well as the following "single" lost-events all have the right timestamp and duration. This was badly broken, and is why I would love for these tests (and the improvements on the existing ones) to make it upstream to avoid more breakage.

Review of attachment 306745 [details] [review]:

Looks good, but the test changes don't apply to latest GIT master.

::: gst/rtpmanager/gstrtpjitterbuffer.c
@@ -3187,3 @@
           "timestamp", G_TYPE_UINT64, timestamp,
           "duration", G_TYPE_UINT64, duration,
-          "late", G_TYPE_BOOLEAN, late,

Why do you remove this? It might be used be something (but it's not used inside GStreamer code AFAICS)

Created attachment 307100 [details] [review]
Updated patch

An updated patch that applies to master (and runs!).

This commit: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=3df0cce65dfbe2bf4bfb4548e2f638d0995bcb74 changes a lot of the tests, so we approached that with a healthy dose of skepticism, but after thinking about it and looking at the tests and logs we think the change is a good one, and updated our tests to take that into account. :)

I also could not resist the temptation of doing some refactoring of the test-buffer generation (making it a lot simpler for the 90% usecase) and changing some of the global "PCMU"-variables to actually make sense.

So this is now all good from our POV, and AFAWK the jitterbuffer should now be in a healthy state. (passes all our internal tests as well)

(In reply to Sebastian Dröge (slomo) from comment #7)
> Review of attachment 306745 [details] [review] [review]:
> 
> Looks good, but the test changes don't apply to latest GIT master.
> 
> ::: gst/rtpmanager/gstrtpjitterbuffer.c
> @@ -3187,3 @@
>            "timestamp", G_TYPE_UINT64, timestamp,
>            "duration", G_TYPE_UINT64, duration,
> -          "late", G_TYPE_BOOLEAN, late,
> 
> Why do you remove this? It might be used be something (but it's not used
> inside GStreamer code AFAICS)

I comment on this in the original commit-msg. It basically does not make any sense anymore.

commit ddd032f56b64316d5a1aa5b267ad34df4e774373
Author: Havard Graff <havard.graff@gmail.com>
Date:   Wed Jul 8 21:08:36 2015 +0200

    rtpjitterbuffer: fix gap-time calculation and remove "late"
    
    The amount of time that is completely expired and not worth waiting for,
    is the duration of the packets in the gap (gap * duration) - the
    latency (size) of the jitterbuffer (priv->latency_ns). This is the duration
    that we make a "multi-lost" packet for.
    
    The "late" concept made some sense in 0.10 as it reflected that a buffer
    coming in had not been waited for at all, but had a timestamp that was
    outside the jitterbuffer to wait for. With the rewrite of the waiting
    (timeout) mechanism in 1.0, this no longer makes any sense, and the
    variable no longer reflects anything meaningful (num > 0 is useless,
    the duration is what matters)
    
    Fixed up the tests that had been slightly modified in 1.0 to allow faulty
    behavior to sneak in, and port some of them to use GstHarness.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=738363

commit 40524e5a493702fa9ff67fde43f94a9da2f16233
Author: Stian Selnes <stian@pexip.com>
Date:   Tue Jun 30 11:21:31 2015 +0200

    Revert "rtpjitterbuffer: Fix expected_dts calc in calculate_expected"
    
    This reverts commit 05bd708fc5e881390fe839803b53144393d95ab0.
    
    The reverted patch is wrong and introduces a regression because there
    may still be time to receive some of the packets included in the gap
    if they are reordered.