Add substream param to switch in VideoRoom

[brave44]

Ability to specify substream while switching in VideoRoom.

https://github.com/meetecho/janus-gateway/issues/3196

[januscla]

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

[lminiero]

Apologies for this late response, I have been abroad for a few weeks.

I think this is very incomplete at the moment. First of all, you're accessing a new property but not validating it, which is something we do automatically with JANUS_VALIDATE_JSON_OBJECT: check switch_update_parameters which is where the stream properties we support are defined (and which looks like where you're adding the new property). Besides, for some reason you only added a way to specify substreams but not the simulcast temporal layer: any functionality extension that only adds one of them and not both is by definition incomplete.

[brave44]

Apologies for this late response, I have been abroad for a few weeks.

I think this is very incomplete at the moment. First of all, you're accessing a new property but not validating it, which is something we do automatically with JANUS_VALIDATE_JSON_OBJECT: check switch_update_parameters which is where the stream properties we support are defined (and which looks like where you're adding the new property). Besides, for some reason you only added a way to specify substreams but not the simulcast temporal layer: any functionality extension that only adds one of them and not both is by definition incomplete.

Thanks for reply, I'll correct the request according to your comments.

Right now we found out that simulcasting_context_reset in switch causes video freeze right after switch. Picture freezes and we monitor freezeCount and totalFreezesDuration in webRTC stats.

By removing

janus_rtp_simulcasting_context_reset(&stream->sim_context);
janus_vp8_simulcast_context_reset(&stream->vp8_context);
janus_rtp_svc_context_reset(&stream->svc_context);

from switch (along with setting substream param) we have no freezes at all. Plus, if only set substream but not remove _context_reset, I see bitrate spike right after switch. (Spike to default bitrate witch is max bitrate). I think it's understandable and that what context resetting causes - it resets to defaults.

Maybe we don't need context resetting in switch at all? Simulcast context already been set, we working with existed stream, and just need new data in this stream, with ability to set desired substream. Why reset needed? What you think?

[lminiero]

Maybe we don't need context resetting in switch at all? Simulcast context already been set, we working with existed stream, and just need new data in this stream, with ability to set desired substream. Why reset needed? What you think?

The reset is needed because the video publisher is not the same. In fact, we reset and then copy the rid_ext_id property, which may be different (or entirely unavailable), plus the new substream/temporal targets. This way it's as if we're starting fresh with a new stream (which indeed we are).

Are you sure the problem is not in janus_vp8_simulcast_context_reset(&stream->vp8_context) instead? That one may indeed have to be left alone, since just as janus_rtp_switching_context it's more specific to the subscriber stream itself: more precisely, it dictates how to rewrite PictureIDs, when doing simulcast, so resetting that may cause the wrong one to be written in RTP packets, possibly causing freezes.

[brave44]

Are you sure the problem is not in janus_vp8_simulcast_context_reset(&stream->vp8_context) instead? That one may indeed have to be left alone, since just as janus_rtp_switching_context it's more specific to the subscriber stream itself: more precisely, it dictates how to rewrite PictureIDs, when doing simulcast, so resetting that may cause the wrong one to be written in RTP packets, possibly causing freezes.

Thanks, will try and let you know the results. Right now I deleted them both, and have no freezes, so may be it was vp8_simulcast_context_reset indeed.

[brave44]

@lminiero I think you was right, janus_vp8_simulcast_context_reset was causing freezes, everything working good without it. Plus, I added validations and temporal layer param. Hope you'll take a look.

[lminiero]

Thanks for the patch! I'll have a look later this afternoon and merge if it works for me too :+1:

[lminiero]

Looks good, merging :v:

[lminiero]

I guess I'll have to add a similar approach for SVC as well, which at the moment isn't covered either.

[brave44]

Thanks for your great work, man!