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 738363 - jitterbuffer: lost-events are broken
jitterbuffer: lost-events are broken
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other AIX
: Normal blocker
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 728368 751916
Blocks:
 
 
Reported: 2014-10-11 18:11 UTC by Håvard Graff (hgr)
Modified: 2015-08-16 13:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch 1/2 (1.65 KB, patch)
2015-07-03 16:55 UTC, Stian Selnes (stianse)
committed Details | Review
Patch 2/2 (38.87 KB, patch)
2015-07-03 16:56 UTC, Stian Selnes (stianse)
needs-work Details | Review
Updated patch (46.78 KB, patch)
2015-07-08 19:20 UTC, Håvard Graff (hgr)
committed Details | Review

Comment 1 Sebastian Dröge (slomo) 2014-10-13 06:28:56 UTC
Can you provide a patch that does not depend on GstHarness? :)
Comment 2 Sebastian Dröge (slomo) 2015-03-23 09:11:20 UTC
Ping?
Comment 3 Stian Selnes (stianse) 2015-07-03 16:55:13 UTC
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.
Comment 4 Stian Selnes (stianse) 2015-07-03 16:56:09 UTC
Created attachment 306745 [details] [review]
Patch 2/2
Comment 5 Sebastian Dröge (slomo) 2015-07-06 08:00:09 UTC
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 :)
Comment 6 Håvard Graff (hgr) 2015-07-06 08:25:51 UTC
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.
Comment 7 Sebastian Dröge (slomo) 2015-07-08 12:32:54 UTC
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)
Comment 8 Håvard Graff (hgr) 2015-07-08 19:20:13 UTC
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)
Comment 9 Håvard Graff (hgr) 2015-07-08 19:20:50 UTC
(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.
Comment 10 Sebastian Dröge (slomo) 2015-07-08 20:19:20 UTC
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.