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 746445 - rtpaux: Unit test is racy and producing warnings
rtpaux: Unit test is racy and producing warnings
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 747965
Blocks:
 
 
Reported: 2015-03-19 11:36 UTC by Sebastian Dröge (slomo)
Modified: 2015-08-16 13:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: rtpaux: fix to set valid payload type to pt-map (1.07 KB, patch)
2015-08-05 01:56 UTC, Hyunjun Ko
rejected Details | Review
rtprtxsend: print valid type where guint32 is expected (1.15 KB, patch)
2015-08-05 01:57 UTC, Hyunjun Ko
committed Details | Review
tests: rtpaux: use a dynamic pt in the test (1.60 KB, patch)
2015-08-05 14:23 UTC, Thiago Sousa Santos
committed Details | Review

Description Sebastian Dröge (slomo) 2015-03-19 11:36:41 UTC
Same as bug #728501. It works more often now it seems, but still deadlocks every now and then. Also it produces warnings now:

Unexpected critical/warning: gstpad.c:4798:store_sticky_event:<rtpsession1:sync_src> Sticky event misordering, got 'segment' before 'caps'
0%: Checks: 1, Failures: 1, Errors: 0
gstcheck.c:79:F:general:test_simple_rtpbin_aux:0: Unexpected critical/warning: gstpad.c:4798:store_sticky_event:<rtpsession1:sync_src> Sticky event misordering, got 'segment' before 'caps'
Comment 1 Sebastian Dröge (slomo) 2015-03-21 18:29:22 UTC
The warning is gone but the test is still racy and sometimes fails

commit 0e3609a6e12cc44a4d83cabed910422c2e91a5b8
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Mar 21 17:50:40 2015 +0100

    rtpsession: Fix another instance of sticky event misordering warnings
    
    Make sure that the sync_src pad has caps before the segment event.
    Otherwise we might get a segment event before caps from the receive
    RTCP pad, and then later when receiving RTCP packets will set caps.
    This will results in a sticky event misordering warning
    
    This fixes warnings in the rtpaux unit test but also in the
    rtpaux and rtx examples in tests/examples/rtp
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746445
Comment 2 Tim-Philipp Müller 2015-05-10 22:26:42 UTC
For me it fails reliable, every single time, with:

elements/rtpaux.c:388:F:general:test_simple_rtpbin_aux:0: Failure 'nb_rtx_send_packets < 1' occurred
Comment 3 Thiago Sousa Santos 2015-08-04 21:26:05 UTC
commit 5f9e5bf385d3851ce1b84fc560f9362559c2c154
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Tue Aug 4 18:07:35 2015 -0300

    tests: rtpaux: fix test failure
    
    The RTP PT for alaw is 8.
    Less than 50 packets are received in the length of this test so it
    would never drop a buffer or would drop only the last buffer and
    it would fail sometimes when the received wouldn't receive the
    retransmission packet in time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746445
Comment 4 Hyunjun Ko 2015-08-05 01:56:06 UTC
Created attachment 308757 [details] [review]
tests: rtpaux: fix to set valid payload type to pt-map

Thiago's patch makes this unit test successful.
I think setting payload type to pt-map needs to be changed also even though it doesn't affect this unit test.
Comment 5 Hyunjun Ko 2015-08-05 01:57:36 UTC
Created attachment 308758 [details] [review]
rtprtxsend: print valid type where guint32 is expected

When I'm investigating, I found something to print wrong value, which is mis-type printing.
Comment 6 Thiago Sousa Santos 2015-08-05 02:23:22 UTC
Ah, it seems like I overlooked that the pt type was being explicitly set. Will revisit this tomorrow.
Comment 7 Thiago Sousa Santos 2015-08-05 14:23:12 UTC
Created attachment 308794 [details] [review]
tests: rtpaux: use a dynamic pt in the test

1) Tests that using dynamic PT instead of the default ones work
2) If we ever decide to change the codec here we don't need to
   worry about change the PT for the default one of the new codec
   in the test
Comment 8 Thiago Sousa Santos 2015-08-05 14:25:52 UTC
The real blocker is #747965,

after that is fixed we can push the new update to the tests to make them use pt=96 again and this should be all solved.
Comment 9 Hyunjun Ko 2015-08-06 02:52:36 UTC
Indeed. 
I tested again with #747965's patch and your second patch. Seems to be sloved.

Anyway, my second patch to print out ssrc value properly can be merged? :)
Comment 10 Thiago Sousa Santos 2015-08-06 04:41:48 UTC
commit dac431ef3fee58870db21005b89650b892524cdd
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Wed Aug 5 11:13:09 2015 -0300

    tests: rtpaux: use a dynamic pt in the test
    
    1) Tests that using dynamic PT instead of the default ones work
    2) If we ever decide to change the codec here we don't need to
       worry about change the PT for the default one of the new codec
       in the test
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746445

commit b0d602086251c7ab435b6845884e4b0be277a218
Author: Hyunjun Ko <zzoon.ko@samsung.com>
Date:   Wed Aug 5 10:53:15 2015 +0900

    rtprtxsend: print valid type where guint32 is expected
    
    https://bugzilla.gnome.org/show_bug.cgi?id=746445
Comment 11 Tim-Philipp Müller 2015-08-06 11:17:20 UTC
Comment on attachment 308758 [details] [review]
rtprtxsend: print valid type where guint32 is expected

>From: Hyunjun Ko <zzoon.ko@samsung.com>
>Subject: [PATCH] rtprtxsend: print valid type where guint32 is expected

>       GST_DEBUG_OBJECT (rtx,
>-          "got caps for payload: %d->%d, ssrc: %u->%d: %" GST_PTR_FORMAT,
>-          payload, GPOINTER_TO_INT (rtx_payload), ssrc, data->rtx_ssrc, caps);
>+          "got caps for payload: %d->%d, ssrc: %u->%" G_GUINT32_FORMAT ": %"
>+          GST_PTR_FORMAT, payload, GPOINTER_TO_INT (rtx_payload), ssrc,
>+          data->rtx_ssrc, caps);

For what it's worth, I think %u would probably have been just fine, integer types <sizeof(int) should get promoted to integer size when being passed through vararg functions, so I suspect it was just a signed/unsigned mix-up here?
Comment 12 Hyunjun Ko 2015-08-06 15:31:48 UTC
> 
> For what it's worth, I think %u would probably have been just fine, integer
> types <sizeof(int) should get promoted to integer size when being passed
> through vararg functions, so I suspect it was just a signed/unsigned mix-up
> here?

Yes. It printed negative value when I'm testing