Review of attachment 364282 [details] [review]:

::: gst/rtpmanager/rtpsession.c
@@ +3420,3 @@
 
+  if (source->internal) {
+    GST_DEBUG ("source %08x not internal", source->ssrc);

There's a "not" too much here :)

The internal sources are the ones that we produce, right? So for a sender these would be the ssrcs with which data is sent, and those should surely be included here? And for a receiver they're the sources we use for sending RTCP?
Just trying to confirm that my memory is correct first.

Hello Sebastian,
I understand better (I hope for you too) the internal meaning considering:
  - internal sources are related to LOCAL RTP SSRCs
  - non-internal sources are related to REMOTE RTP SSRCs

Taking this into account:
  - internal sources are created to send RTP packets or to be used as (sender-SSRC) in RR reports [1].
  - non-internal sources are created to receive RTP packets.

Hence, following [2], the Reception Report Blocks MUST be only use non-internal sources (remote RTP SSRCs).

Refs
[1] https://tools.ietf.org/search/rfc3550#section-6.4.1
[2] https://tools.ietf.org/search/rfc3550#section-6.4

Created attachment 364311 [details] [review]
rtpsession: do not report internal sources

Fix wrong log.

(In reply to Miguel París Díaz from comment #2)
> Hello Sebastian,
> I understand better (I hope for you too) the internal meaning considering:
>   - internal sources are related to LOCAL RTP SSRCs
>   - non-internal sources are related to REMOTE RTP SSRCs
> 
> Taking this into account:
>   - internal sources are created to send RTP packets or to be used as
> (sender-SSRC) in RR reports [1].
>   - non-internal sources are created to receive RTP packets.

And what's a "sender" (the field on the RTP source) in this context? Something that actually sends RTP packets, and not just an RTCP sender? Is every "sender" always an internal source?
I guess this should be documented in a comment in rtpsource.h next to those fields. Would you mind making another commit for that?


I agree that RBs must only contain SSRCs from which packets were actually received. It doesn't make any sense otherwise.

Miguel?

Hello Sebastian,
sorry for the delayed response, but I am focus on testing in our system this issue once I have fixed it (together [1] and [2]) and it seems that the problems we experienced have disappeared.

They provoked a real impact in our system and I am sure that for other users too, so seen that closing them implies long tasks like testing and doc I would prioritize them:
  (1) - Fix them (it seems that it is already done by the patches I proposed).
  (2) - Perform unit tests to avoid future regressions.
  (3) - Update doc in order to improve the understanding of the code.

In relation to (2) I will try to develop the best tests I can in relation with these 3 issues.

Regarding (3), I totally agree with you and I will document it once (1) and (2) are finished. Answering your questions:
  - A SENDER RtpSource represents a SSRC used to send RTP packets.
  - It could be remote (non-internal) or local (internal).

Refs
[1] https://bugzilla.gnome.org/show_bug.cgi?id=790661
[2] https://bugzilla.gnome.org/show_bug.cgi?id=790795

Created attachment 364670 [details] [review]
0001 rtpsession: do not report internal sources

Split fix from test to ease management.

Created attachment 364671 [details] [review]
0002 (test)rtpsession: make test harness always sender and receiver

Created attachment 364672 [details] [review]
0003 (test)rtpsession: fix test_multiple_senders_roundrobin_rbs

Created attachment 364673 [details] [review]
0004 (test)rtpsession: do not report internal sources

Hi!

First, good work Miguel and team, this looks like real improvements to the RTP stack!

What I really really would like is for the following patch to be merged first, and then have the test-changes (and potentially the new tests) use the new SessionHarness instead:
https://bugzilla.gnome.org/show_bug.cgi?id=791070

As an example, the whole crank_harness_rtcp block would just be a matter of calling the provided session_harness_produce_rtcp.

The patch "make test harness always sender and receiver" would conflict horribly with the SessionHarness changes, and I think it achieves exactly the same thing but with much less code to worry about. (SessionHarness does set up both rtcp and rtp send/recv)

Would this be acceptable to you Miguel? I am happy to help out converting your test to the new SessionHarness, and I think you will find it only makes things clearer and easier?

Also, it turns out we have the same fix internally, so maybe we could discuss what the *best* approach is:
https://github.com/pexip/gst-plugins-good/commit/5661107a93c062612b8d087429d855487e8cbbc5

Miguel, what do you think? Also the tests need to be updated now

Hello Håvard and Sebastian,
first of all thanks for helping with this RtpSession issues.

Regarding using GstHarness, I totally agree with Håvard, using it makes tests quite easier. Actually, I used it in the new tests I did for RtpSession.
Hence, I will rebase my changes onto the test refactoring and I will use the SessionHarness abstraction.

Apart of that, I am happy that we both have found and fixed the issue independently, which demonstrates that there was a really bug.
In relation to your fix, I am not sure if removing the next check could cause an unexpected behavior, we should check it:

-  /* only report about other sender */
-  if (source == data->source)

Anyway, independently your solution or mine is merged I would add both tests to have a wider coverage.
Could you please attach your patches here?

-- 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/416.