On a reconfigure, it should do a downstream getcaps to get the ssrc again if it ignores the ssrc from upstream. so that ... ! payloader ! rtpmux ! capsfilter ! ... works if you change the ssrc on the capsfilter?  It shoudl also react to the collision custom event just like rtpbasepayload.

(In reply to Olivier Crête from comment #1)
> On a reconfigure, it should do a downstream getcaps to get the ssrc again if
> it ignores the ssrc from upstream. so that ... ! payloader ! rtpmux !
> capsfilter ! ... works if you change the ssrc on the capsfilter?  It shoudl
> also react to the collision custom event just like rtpbasepayload.

Sure, but my patch does not change any of that, and I don't think this is/was working either. I can add another patch to fix this as well, but I think the first patch should be applied first :)

Here is a patch to respect downstream ssrc as well as a disabled test for the downstream ssrc changing dynamically. (not implemented)

https://github.com/pexip/gst-plugins-good/commit/d709198fe691d8be85248efe504603b5c74951ff.patch

Here is a patch with both of the patches:
https://github.com/pexip/gst-plugins-good/compare/d5195b83493bca88c3d5a95e3b8f56264a563e5c...d709198fe691d8be85248efe504603b5c74951ff.patch

I should probably also mention that without these patches, rtpmux does not actually use the same ssrc on the srcpad at all, meaning two different inputs with different ssrc will have different ssrc on the output as well, making the whole point of a muxer void.

Setting something in getcaps is wrong. And the current code breaks the Farstream unit tests.

I think what you want to do is, in setcaps, get the downstream caps, if they are set, then take the ssrc from there, is not, try the property and as a last resort, generate a random one. I'd also drop everything that is not ssrc/timestamp-offset/seqnum-offset/clock-rate/media-type from the caps. In any subsequent setcaps, if the clock-rate/media-type match the current ones, just return TRUE without pushing a different event. This will reduce the amount of churn. Also, we need to support the GstRTPCollision event to deal with SSRC collisions correctly.

Yeah, as I mentioned, when trying to make the dynamic caps test pass I quickly realized this would need a bigger refactoring. As long as you agree that the tests in the patch should pass, I don't really care how it is inplemented, but currently the rtpmux outouts different ssrcs which is very bad.

Olivier, so is farstream just depending on completely broken behaviour or does it make sense in some way? Do you agree that hgr's tests are making sense, or are there bugs in them? If his tests make sense, let's fix rtpmux to make them pass ;)

I agree with hgr's tests, and Farstraem should work with them.

The underlying problem is that around 1.2-1.4, wtay changed how ssrc are specified, before (in 0.10 and 1.0), you specified it inside rtpbin and it would propagate out, now wtay changed it so that you specify it at the payloader. I had tried to cheat out by just having the muxer just be a passthrough and let the rtpbin and payloaders talk to each other, but it seems that it's not enough. So we need rtpmux to implement the same logic as the payloader to prefer downstream suggestions, but fallback on internal choice. hgr's patch are in the right direction, but are not complete.

If no one else has time to fix it, I'll get to it.

Created attachment 310389 [details] [review]
gstrtpmux: allow the ssrc-property to decide ssrc on outgoing buffers

By not doing this, the muxer is not effectively a rtpmuxer, rather a
funnel, since it should be a single stream that exists the muxer.

If not specified, take the first ssrc seen on a sinkpad, allowing upstream
to decide ssrc in "passthrough" with only one sinkpad.

Also, let downstream ssrc overrule internal configured one

We hence has the following order for determining the ssrc used by
rtpmux:

0. Suggestion from GstRTPCollision event
1. Downstream caps
2. ssrc-Property
3. (First) upstream caps containing ssrc
4. Randomly generated

Created attachment 310390 [details] [review]
rtpmux: As 0xFFFFFFFF is a valid ssrc, check if it has been set

Created attachment 310391 [details] [review]
rtpmux: Use default upstream event handling

@hgr: Let me know if these patches fix your problem.

@(In reply to Olivier Crête from comment #13)
> @hgr: Let me know if these patches fix your problem.

If they pass the added tests (including the dynamic one) I am very happy, thanks!

Review of attachment 310389 [details] [review]:

Looks mostly good and the approach makes sense IMHO

::: gst/rtpmanager/gstrtpmux.c
@@ +231,3 @@
+
+          while (ssrc == rtp_mux->current_ssrc)
+            rtp_mux->current_ssrc = g_random_int ();

This can probably use a mutex somewhere. The upstream event is not necessarily coming from our streaming thread, and we might put some transient ssrc on some buffers that is not the same as the one in the caps.

Olivier?

Thanks for finding this, added a couple more GST_OBJECT_LOCKs and pushed the patches:

commit 58073eaa7ae5d7c7b93da89faea05e7f01ee58cd
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Mon Aug 31 21:10:16 2015 -0400

    rtpmux: Use default upstream event handling
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752694

commit 43c213fc5db75dcf570ef1c224122307a0899911
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Mon Aug 31 21:05:03 2015 -0400

    rtpmux: As 0xFFFFFFFF is a valid ssrc, check if it has been set
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752694

commit d5e26ab9099e5a46eda7e9b59f01c1efe463344d
Author: Havard Graff <havard.graff@gmail.com>
Date:   Wed Jul 22 09:47:22 2015 +0200

    gstrtpmux: allow the ssrc-property to decide ssrc on outgoing buffers
    
    By not doing this, the muxer is not effectively a rtpmuxer, rather a
    funnel, since it should be a single stream that exists the muxer.
    
    If not specified, take the first ssrc seen on a sinkpad, allowing upstream
    to decide ssrc in "passthrough" with only one sinkpad.
    
    Also, let downstream ssrc overrule internal configured one
    
    We hence has the following order for determining the ssrc used by
    rtpmux:
    
    0. Suggestion from GstRTPCollision event
    1. Downstream caps
    2. ssrc-Property
    3. (First) upstream caps containing ssrc
    4. Randomly generated
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752694