GNOME Bugzilla – Bug 790661
rtpsession: fix allow_early flag for RTCP early handling
Last modified: 2018-11-03 15:23:46 UTC
Created attachment 364114 [details] [review] rtpsession: fix allow_early flag for RTCP early handling Fix bug introduced by 647eefea67bf1ec4858b1108784e73e9216a7c13 https://bugzilla.gnome.org/show_bug.cgi?id=746543) causing no Regular RTCP to be sent up to minutes. Here you have one of the a lot of occurrences I come across: rtp_session_on_timeout: Time since last regular RTCP: 71:47:54.246928902 - 71:46:14.195306724 = 0:01:40.051622178
Comment on attachment 364114 [details] [review] rtpsession: fix allow_early flag for RTCP early handling Please explain in more detail what was wrong and what/how this fixes it, in the commit message :)
Also a unit test would be nice
Created attachment 364134 [details] [review] rtpsession: fix allow_early flag for RTCP early handling Patch description updated. Regarding the unit test, is there a way of build a deterministic one?
Yes, see the existing unit tests for rtpsession around GstHarness.
Created attachment 364273 [details] [review] rtpsession: fix allow_early flag for RTCP early handling Patch updated with a unit test. It cannot be deterministic as from GstRtpSession API the thread for RTCP management cannot be controlled. Anyway, it is enough to: - Demonstrate the bug and how the proposed solution fixes it. - Avoid future regressions.
Created attachment 364280 [details] [review] rtpsession: fix allow_early flag for RTCP early handling Simplify test.
Created attachment 364281 [details] [review] rtpsession: fix allow_early flag for RTCP early handling
Review of attachment 364281 [details] [review]: Various typos in the commit message. "... where all RTCP packet *are* early and *r*egular RTCP packets are sent again after minutes" "making this complex algorithm" ::: gst/rtpmanager/rtpsession.c @@ -4284,3 @@ - * max_delay */ - if (sess->last_rtcp_check_time + T_rr > current_time) - offset = (sess->last_rtcp_check_time + T_rr) - current_time; After reading all the code again, I'm not sure why this code existed at all. If last time was a regular RTCP packet, we should've just reset allow_early. The other case is already covered by the code below, and a regular RTCP packet would be sent in time that covers this early request. And this case here would actually prevent a regular RTCP packet to be sent while that's exactly what should happen. Your changes simplify the code and seem more correct. ::: tests/check/elements/rtpsession.c @@ +738,3 @@ + + GST_DEBUG ("Wait for RTCP..."); + g_usleep (2000000); Check the gst_test_clock_advance_time() / crank_rtcp_thread() code in the other tests. You can do this without a sleep and deterministically.
OK, I will do it. Thanks for the feedback ;).
Hello again, after trying to develop a deterministic test and thinking about it, I think that it is not possible to do it because the RTCP cannot be control with the fine grain level that this test needs. Specifically, NACKs could be handled or not depending on the state of RTCP thread and.
Created attachment 364339 [details] [review] rtpsession: allow early if current time is less or equals Fixing issue detected while trying to develop a deterministic test.
Havard/others, do you have any idea how this test could be improved and be made deterministically? :) I really don't want to merge an undeterministic test...
The rtpsession tests were rewritten to use GstHarness now, which should help also making your test deterministic. Take a look at the existing, new tests please :)
Yes, I have taken a look to revision [1] but I think it will need some extra work to make it totally deterministic due to the next function could cause generating more than num_rtcp_packets static void session_harness_produce_rtcp (SessionHarness * h, gint num_rtcp_packets) { /* due to randomness in rescheduling of RTCP timeout, we need to keep cranking until we have the desired amount of packets */ while (gst_harness_buffers_in_queue (h->rtcp_h) < num_rtcp_packets) session_harness_crank_clock (h); } The ideal would be crank and wait until having the desired RTCP packets. I will work on it when I have a time slot ;) Refs [1] https://bugzilla.gnome.org/review?bug=791070&attachment=364744
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/415.