GNOME Bugzilla – Bug 795162
Various patches on rtpmux
Last modified: 2018-10-10 19:41:21 UTC
Created attachment 370803 [details] [review] rtpmux: property should overrule both upstream and downstream https://bugzilla.gnome.org/show_bug.cgi?id=762213
Created attachment 370804 [details] [review] rtpmux: protect against NULL caps Due to state-changes deactivating the pad from another thread, this can happen.
Created attachment 370805 [details] [review] rtpmux: cleanup ssrc-handling code a bit And add some better logging.
Created attachment 370806 [details] [review] rtpmux: respect downstream "timestamp-offset" in caps.
Review of attachment 370803 [details] [review]: I think overruling downstream make break the ssrc change on collision detection, or did we have a custom event for this?
Review of attachment 370804 [details] [review]: Looks good, you don't happen to have a clever test for this?
Review of attachment 370803 [details] [review]: Oh yes, we have an event, so this looks ok
Review of attachment 370805 [details] [review]: That sounds like a lot of INFO instead of DEBUG? ::: gst/rtpmanager/gstrtpmux.c @@ +231,2 @@ new_ssrc = rtp_mux->current_ssrc; + GST_INFO_OBJECT (rtp_mux, "Chose new ssrc %x", new_ssrc); Maybe "New ssrc chosen after collision: %x" @@ +878,3 @@ rtp_mux->current_ssrc = rtp_mux->ssrc; rtp_mux->have_ssrc = TRUE; + GST_INFO_OBJECT (rtp_mux, "ssrc set to %x", rtp_mux->ssrc); Maybe add that this is a property "SSRC property set to %x" ?
Review of attachment 370806 [details] [review]: ::: gst/rtpmanager/gstrtpmux.c @@ +626,3 @@ + if (gst_structure_get_uint (structure, + "timestamp-offset", &rtp_mux->ts_base)) { + GST_INFO_OBJECT (pad, "Use downstream timestamp-offset: %u", Sounds like a GST_DEBUG_OBJECT to me
Ping?
*** Bug 762213 has been marked as a duplicate of this bug. ***
@hgr Any chance you have a unit test for this ? and maybe should downgrade some of the GST_INFO to debug.
(In reply to Olivier Crête from comment #11) > @hgr Any chance you have a unit test for this ? and maybe should downgrade > some of the GST_INFO to debug. You mean in addition to the 5 tests in this patch-set? ;) As for INFO vs. DEBUG, I find that I want to have an "overview" on the internal functionality of an element when using INFO, and DEBUG is more for digging in. I just saw setting of SSRC as such a central component to what this element does, so I wanted to keep them high up. They are also not per-buffer so it should not be spammy.
Oh I was looking in the wrong patch.. I merged it as-is with a couple improvements to the debug logs (to make the fact that ssrc change is due to a collision