Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

send a PLI when a subscriber is resuming a previously paused stream #2922

Merged

Conversation

flaviogrossi
Copy link
Contributor

Hi, i think i found a small issue since the multistream merge:

in the current codebase, a PLI is sent when a subscriber issues a configure request to start a previously paused stream, see here.

However this is only done when using the deprecated video parameter, and not when using send, causing a delay to the receiver (the stream is "frozen" until the next complete frame).

This PR is to add the PLI request also in latter case.

currently the PLI is only sent when using the deprecated "video"
parameter of the configure message. This also sends it when using the
new parameter "send"
@lminiero
Copy link
Member

Good catch! Shouldn't it be in the if before that, though?

if(!oldsend && newsend) {
	/* Medium just resumed, reset the RTP sequence numbers */
	stream->context.seq_reset = TRUE;
}

@lminiero
Copy link
Member

Good catch! Shouldn't it be in the if before that, though?

Ah no, it's outside of the if in the previous check too, so that makes sense. Merging then!

@lminiero lminiero merged commit c684d43 into meetecho:master Mar 14, 2022
@flaviogrossi
Copy link
Contributor Author

Great, thanks!

Fiddling with the sources i noticed too that the mid parameter is ignored when using the old parameter video, causing all of the streams to be paused.

Out of curiosity, is this intentional? I guess it's to be absolutely retrocompatible, because the mid could be used anyway, if found.

@lminiero
Copy link
Member

Yes, if you pass video you implicitly ask for them all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants