Fix transport-wide CC feedback when simulcast SSRCs are missing

[OxleyS]

When a publisher never sends video frames for a simulcast layer (whether due to its own heuristics or the layer being disabled by the user), the SSRC for that layer will remain unknown. The TWCC feedback mechanism was always picking the first layer's SSRC, causing the report to default to an SSRC of zero if the layer never got any data.

As I understand it, being transport-wide, any SSRC going over the transport will do, so this simply goes through each simulcast layer and picks the first SSRC that exists.

The bug this PR fixes turned out to be the underlying cause of the behavior described here. In short, the lack of feedback reports with known SSRCs caused the publisher to continually downgrade the BWE for the connection, until it got low enough that the VP8 encoder fell over, causing the timestamps on frames to fall out of pace.

[januscla]

Thanks for your contribution, @OxleyS! Please make sure you sign our CLA, as it's a required step before we can merge this.

[OxleyS]

I've just signed the CLA.

[atoppi]

Thanks @OxleyS for the effort.
I did suspect indeed something weird happening on the RTCP but I just focused on the SR/RR ignoring TWCC and BWE related issues.
The proposed changes look good in general.

Did you try what happens with Firefox when different layers SSRCs are being used for twcc media ?

[lminiero]

Did you try what happens with Firefox when different layers SSRCs are being used for twcc media ?

Firefox always advertises SSRCs so it's not an issue I guess. It's Chrome that doesn't when rid-based simulcast is used.

[lminiero]

I added a few notes inline. That said, I think the approach may need to be changed, since you're applying that logic only to a single medium instance, the one we choose initially in janus_ice_outgoing_transport_wide_cc_feedback where we say /* Find inbound video medium */. Under normal circumstances that's ok, but a PeerConnection may contain more than one video stream, so why should only one of them be analysed for this process, especially considering you may end up without a SSRC? (you currently have a warning for that).

You may want to move your SSRC lookup login in the previous while instead:

while (g_hash_table_iter_next(&iter, NULL, &value)) {
	janus_ice_peerconnection_medium *m = value;
	if(m && m->type == JANUS_MEDIA_VIDEO && m->recv) {
		medium = m;
		break;
	}
}

so that you can always move to another medium candidate in case the previous one doesn't contain an SSRC that might be of use.

I'm also wondering if we might extend the selection to audio m-lines as well: I can't remember if we negotiate TWCC for audio streams as well, even though that may be problematic (especially if browsers do not take those into account instead).

[lminiero]

Per code style, we don't define variables inside the for loop, so please define i outside.

[lminiero]

Code style: no space between if and bracket.

[lminiero]

I wouldn't display a warning here: in case it happens, this is going to spam the logs a lot, since TWCC feedback is very frequent (5 to 10 times per second by default). I'd make it a LOG_HUGE instead.

[OxleyS]

Thanks for the review - I'll hopefully be able to work on the points you brought up later today.

[OxleyS]

I'm also wondering if we might extend the selection to audio m-lines as well

As I understand it, we have to take bundling into consideration. If audio/video are being bundled, then we can and should include audio SSRCs in the SSRC selection process. If they're not being bundled, however, then not only must we not consider audio SSRCs for video feedback, but we actually also should be sending multiple sets of feedback reports, one per transport. Does Janus force bundling? Peeking through the code indicates a strong one-to-one relationship between PeerConnections and ICE handles.

One area the spec is unclear on is what happens when two m-lines are bundled, but only one of them negotiates TWCC. Perhaps we can just ignore this case, and treat it as if both specified TWCC? This seems to be current Janus behavior.

I can't remember if we negotiate TWCC for audio streams as well

I peeked around the code for usages of JANUS_RTP_EXTMAP_TRANSPORT_WIDE_CC. It looks like negotiating TWCC on audio is possible in EchoTest and RecordPlay if the client offers it.

VideoRoom, meanwhile, only inserts it into SDP (both offer and answer) for video m-lines. I'm wondering if this could cause issues down the line if, say, a connection first negotiates audio-only, then adds video later.

[lminiero]

As I understand it, we have to take bundling into consideration. If audio/video are being bundled, then we can and should include audio SSRCs in the SSRC selection process

Media is always bundled in Janus. Not all media lines may be part of the TWCC process, though, as it depends whether or not the extension (and RTCP feedback) is negotiated on all of them or not. IIRC that's only true for video streams in browsers, but I may be remembering things wrong.

VideoRoom, meanwhile, only inserts it into SDP (both offer and answer) for video m-lines. I'm wondering if this could cause issues down the line if, say, a connection first negotiates audio-only, then adds video later.

Shouldn't be an issue, as we update our references to RTP extensions any time an update takes place.

[OxleyS]

IIRC that's only true for video streams in browsers, but I may be remembering things wrong

Tested this just now, it seems Chrome negotiates it for both, at least in the latest version. Firefox still does video-only. If we want to use this to gate which SSRCs we consider for the feedback report, then we might want to move the do_transport_wide_cc flag from PeerConnection-level to medium-level.

That's an increase in scope, though, so for this PR maybe we can just expand to all video SSRCs as you suggested, and take care of audio in a followup if so desired.

Shouldn't be an issue, as we update our references to RTP extensions any time an update takes place.

I was more worried about whether browsers would do the right thing, although I tested that too with Chrome/Firefox and it seems they handle it fine.

[atoppi]

That's an increase in scope, though, so for this PR maybe we can just expand to all video SSRCs as you suggested, and take care of audio in a followup if so desired.

From my point of view, even extending the search to all video m-lines is out of scope here (the issue was about missing rid layers in simulcast). The proposed change is still an improvement over the existing code and fixes the issue.

[lminiero]

Sure, we can merge as is (removing the LOG_WARN at least though), but this would need to be addressed in another commit nevertheless.

[OxleyS]

Extending to all video is not a big deal, I'll just do it now (and fix the LOG_WARN thing).

[lminiero]

Thanks! Added a couple of editorial notes (the changes look fine to me otherwise).

[lminiero]

I think this could be simplified as

int i = 0;
for(i=0; i<3; i++) {
	if(m->ssrc_peer[i] > 0) {
		medium = m;
		ssrc_peer = m->ssrc_peer[i];
		break;
	}
}
if(medium && ssrc_peer)
	break;

as I feel the second check is a bit more "awkward" (and possibly error prone should we decide to increase the number of substreams in the future), and the whole block could be made more compact.

[lminiero]

Just as a small nit and personal preference in terms of code style, we typically use if(medium == NULL) for checks like that.

[OxleyS]

I found instances of both styles in the code base and wasn't sure which to use. Is there a coding style document I could refer to in the future?

[lminiero]

You're 100% right, and apologies for the confusion: that's mainly because of the many contributions we've received so far (and of which I'm grateful for of course), where we haven't always enforced a specific code style. Unfortunately we don't have a reference for that, and I'm not really sure we'll prepare one: even @atoppi and I write things a bit differently 😆 These are small nits that can be ascribed to my excessive pedantry on the matter, so no need to concern too much on that!

[lminiero]

Thanks! Incidentally, have you checked if this indeed fixes the stuttering video when the first substream is not sending any data and so the first SSRC is 0? I'll make a test myself later as well.

[OxleyS]

Yeah, I've been checking it using the project where I originally noticed the behavior.

[lminiero]

Just tested and I can't replicate the delayed/stuttering video anymore, so I'll merge. Thanks!