Send PLI when starting a paused stream #2645

xadhoom · 2021-04-21T15:01:44Z

We have noticed that sometimes resuming a paused stream takes long (even 10 seconds or more) before getting displayed on the subscriber. This depends on framerate / frame size (eg. on a nHD stream from a webcam with a somewhat high fps takes less than a desktop stream shared in FHD with maybe 3/5 fps), but also on the rtcp feedback from the browser that may not be immediate.

By sending a pli when starting the stream, the restart is more consistent.

This is a wip for the following reasons:

makes sense to always send a pli request or is better to add a parameter to the start request?
I'm not sure about the guard for the janus_videoroom_reqpli call, the one here is taken from the recording portion and seems ok
maybe is better to call it only if the subscriber is paused, so get's triggered only on stream resume?

januscla · 2021-04-21T15:01:46Z

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

lminiero · 2021-04-21T15:09:30Z

I think you're modifying the wrong part of the code, if you want to resume a paused stream. The start request is used to send your SDP answer, as a subscriber, to the offer the plugin sent you, and so the negotiation is still ongoing. For startup, we already have a PLI when the PeerConnection goes up, in janus_videoroom_setup_media:

	janus_videoroom_reqpli(p, "New subscriber available");

There already is a PLI also for video subscriptions that had been paused via configure requests:

	janus_videoroom_reqpli(publisher, "Restoring video for subscriber");

lminiero · 2021-04-21T15:11:29Z

Ok, I think I understand the point now: this is a start in response to a previous pause. Then yes, to answer your question above:

maybe is better to call it only if the subscriber is paused, so get's triggered only on stream resume?

I think this should only be done if the PC was up and previously paused, not always (otherwise it will try to always send a PLI during negotiation, before the PC even exists).

xadhoom · 2021-04-21T15:17:15Z

I think this should only be done if the PC was up and previously paused, not always (otherwise it will try to always send a PLI during negotiation, before the PC even exists).

Ok, moved inside the "paused" block, in order to be triggered only if the stream was previously paused.

plugins/janus_videoroom.c

lminiero · 2021-04-21T15:25:31Z

plugins/janus_videoroom.c

+				if(feed && g_atomic_int_get(&feed->started, 1)) {
+						/* Send a FIR */
+						janus_videoroom_reqpli(feed, "Subscriber start");
+				}


This should be in the if(subscriber->paused above, though. Indentation should be fixed too.
As a side note, I'd move the *feed initialization right before the check, since it's unused if we're not in subscriber->paused.

ok, moved inside the paused if, along with the needed initialization and indentation

something is wrong into the g_atomic_int_get(&feed->started, 1), investigating on it

something is wrong into the g_atomic_int_get(&feed->started, 1), investigating on it

ok, it was if(feed && feed->session && g_atomic_int_get(&feed->session->started)) {

lminiero · 2021-04-26T10:31:44Z

Merging, thanks! 👍

* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c

* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c (cherry picked from commit 1827315)

* meetecho_master: (345 commits) Remove support for framemarking RTP extension (meetecho#2640) Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599) Fixed overflow runtime error Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637) Send PLI when starting a paused stream (meetecho#2645) Prevent too high shift exponent Fixed type of seq/ts in file-based Streaming mountpoint threads Fix missing g_thread_unref when a streaming helper thread quits. Added custom headers for SIP INFO request (meetecho#2644) Free participant->user_id_str in case of opus enc/decoder error. Reject flexfec when offered, as still unsupported (see meetecho#2639) Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639) Added NULL checks for json_dumps (see meetecho#2629) Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636) Timestamp correction for janus-pp-rec (meetecho#2573) Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569) Add missing library link for WS event handler (fixes meetecho#2628) Fixed broken switch in Streaming plugin when using helper threads Resolves meetecho#2624 jansson double referencing (meetecho#2634) Unlock mountpoints mutex after the spawning of helper threads. ... # Conflicts: # transports/janus_mqtt.c (cherry picked from commit 1827315) (cherry picked from commit b9318b7)

xadhoom force-pushed the master branch 2 times, most recently from acaa528 to 0befda2 Compare April 21, 2021 15:15

lminiero reviewed Apr 21, 2021

View reviewed changes

plugins/janus_videoroom.c Show resolved Hide resolved

xadhoom force-pushed the master branch 2 times, most recently from 2cba85f to e28ce0f Compare April 21, 2021 15:23

lminiero reviewed Apr 21, 2021

View reviewed changes

xadhoom force-pushed the master branch from e28ce0f to ab3ef74 Compare April 21, 2021 15:29

Send PLI when starting a paused stream

f55526b

xadhoom force-pushed the master branch from ab3ef74 to f55526b Compare April 21, 2021 15:39

xadhoom changed the title ~~WIP: Send PLI when starting a paused stream~~ Send PLI when starting a paused stream Apr 21, 2021

lminiero merged commit 541c00e into meetecho:master Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send PLI when starting a paused stream #2645

Send PLI when starting a paused stream #2645

xadhoom commented Apr 21, 2021

januscla commented Apr 21, 2021

lminiero commented Apr 21, 2021

lminiero commented Apr 21, 2021

xadhoom commented Apr 21, 2021

lminiero Apr 21, 2021

xadhoom Apr 21, 2021

xadhoom Apr 21, 2021

xadhoom Apr 21, 2021

xadhoom Apr 21, 2021

lminiero commented Apr 26, 2021

Send PLI when starting a paused stream #2645

Send PLI when starting a paused stream #2645

Conversation

xadhoom commented Apr 21, 2021

januscla commented Apr 21, 2021

lminiero commented Apr 21, 2021

lminiero commented Apr 21, 2021

xadhoom commented Apr 21, 2021

lminiero Apr 21, 2021

Choose a reason for hiding this comment

xadhoom Apr 21, 2021

Choose a reason for hiding this comment

xadhoom Apr 21, 2021

Choose a reason for hiding this comment

xadhoom Apr 21, 2021

Choose a reason for hiding this comment

xadhoom Apr 21, 2021

Choose a reason for hiding this comment

lminiero commented Apr 26, 2021