Skip to content

Commit ea11cd3

Browse files
JonathanLennoxJonathanLennox
authored
Limit keyframe requests based on the source requesting them. (#2367)
1 parent 22868ff commit ea11cd3

File tree

10 files changed

+84
-37
lines changed

10 files changed

+84
-37
lines changed

‎jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/RtpSender.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ abstract class RtpSender :
4343
abstract fun addBandwidthListener(listener: TransportCcEngine.BandwidthListener)
4444
abstract fun removeBandwidthListener(listener: TransportCcEngine.BandwidthListener)
4545
abstract fun getTransportCcEngineStats(): TransportCcEngine.StatisticsSnapshot
46-
abstract fun requestKeyframe(mediaSsrc: Long? = null)
46+
abstract fun requestKeyframe(requesterID: String?, mediaSsrc: Long? = null)
4747
abstract fun addLossListener(lossListener: LossListener)
4848
abstract fun setFeature(feature: Features, enabled: Boolean)
4949
abstract fun isFeatureEnabled(feature: Features): Boolean

‎jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/RtpSenderImpl.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,8 @@ class RtpSenderImpl(
261261
srtcpEncryptWrapper.transformer = srtpTransformers.srtcpEncryptTransformer
262262
}
263263

264-
override fun requestKeyframe(mediaSsrc: Long?) {
265-
keyframeRequester.requestKeyframe(mediaSsrc)
264+
override fun requestKeyframe(requesterID: String?, mediaSsrc: Long?) {
265+
keyframeRequester.requestKeyframe(requesterID, mediaSsrc)
266266
}
267267

268268
override fun addLossListener(lossListener: LossListener) {

‎jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/Transceiver.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,8 @@ class Transceiver(
234234
fun getMediaSources(): Array<MediaSourceDesc> = mediaSources.getMediaSources()
235235

236236
@JvmOverloads
237-
fun requestKeyFrame(mediaSsrc: Long? = null) = rtpSender.requestKeyframe(mediaSsrc)
237+
fun requestKeyFrame(requesterID: String? = null, mediaSsrc: Long? = null) =
238+
rtpSender.requestKeyframe(requesterID, mediaSsrc)
238239

239240
fun addPayloadType(payloadType: PayloadType) {
240241
logger.cdebug { "Payload type added: $payloadType" }

‎jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/rtcp/KeyframeRequester.kt‎

Lines changed: 52 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,9 @@ class KeyframeRequester @JvmOverloads constructor(
5656
) : TransformerNode("Keyframe Requester") {
5757
private val logger = createChildLogger(parentLogger)
5858

59-
// Map a SSRC to the timestamp (represented as an [Instant]) of when we last requested a keyframe for it
60-
private val keyframeLimiter = mutableMapOf<Long, RateLimit>()
59+
// Map the tuple of requester and SSRC to a rate limiter
60+
private val perReceiverKeyframeLimiter = mutableMapOf<String, MutableMap<Long, RateLimit>>()
61+
private val perSourceKeyframeLimiter = mutableMapOf<Long, RateLimit>()
6162
private val keyframeLimiterSyncRoot = Any()
6263
private val firCommandSequenceNumber: AtomicInteger = AtomicInteger(0)
6364
private var localSsrc: Long? = null
@@ -93,14 +94,14 @@ class KeyframeRequester @JvmOverloads constructor(
9394
when (pliOrFirPacket) {
9495
is RtcpFbPliPacket -> {
9596
sourceSsrc = pliOrFirPacket.mediaSourceSsrc
96-
canSend = canSendKeyframeRequest(sourceSsrc, now)
97+
canSend = canSendKeyframeRequest(packetInfo.endpointId, sourceSsrc, now)
9798
forward = canSend && streamInformationStore.supportsPli
9899
if (forward) numPlisForwarded++
99100
if (!canSend) numPlisDropped++
100101
}
101102
is RtcpFbFirPacket -> {
102103
sourceSsrc = pliOrFirPacket.mediaSenderSsrc
103-
canSend = canSendKeyframeRequest(sourceSsrc, now)
104+
canSend = canSendKeyframeRequest(packetInfo.endpointId, sourceSsrc, now)
104105
// When both are supported, we favor generating a PLI rather than forwarding a FIR
105106
forward = canSend && streamInformationStore.supportsFir && !streamInformationStore.supportsPli
106107
if (forward) {
@@ -124,34 +125,59 @@ class KeyframeRequester @JvmOverloads constructor(
124125
}
125126

126127
/**
127-
* Returns 'true' when at least one method is supported, AND we haven't sent a request very recently.
128+
* Returns 'true' when at least one method is supported, AND this requester hasn't sent a request very recently.
128129
*/
129-
private fun canSendKeyframeRequest(mediaSsrc: Long, now: Instant): Boolean {
130+
private fun canSendKeyframeRequest(requesterID: String?, mediaSsrc: Long, now: Instant): Boolean {
130131
if (!streamInformationStore.supportsPli && !streamInformationStore.supportsFir) {
131132
return false
132133
}
134+
if (requesterID == null) {
135+
/* This request is either triggered by a dominant speaker switch, or possibly came over a proxy connection.
136+
* Always allow it. */
137+
return true
138+
}
133139
synchronized(keyframeLimiterSyncRoot) {
134-
val limiter = keyframeLimiter.computeIfAbsent(mediaSsrc) {
135-
RateLimit(defaultMinInterval = minInterval, maxRequests = maxRequests, interval = maxRequestInterval)
140+
val perReceiverLimiter = perReceiverKeyframeLimiter.computeIfAbsent(requesterID) { mutableMapOf() }
141+
.computeIfAbsent(mediaSsrc) {
142+
RateLimit(
143+
defaultMinInterval = minInterval,
144+
maxRequests = maxRequests,
145+
interval = maxRequestInterval
146+
)
147+
}
148+
if (!perReceiverLimiter.accept(now, waitInterval)) {
149+
logger.cdebug {
150+
"Ignoring keyframe request for $mediaSsrc from $requesterID, per-receiver rate limited"
151+
}
152+
return false
136153
}
137-
return if (!limiter.accept(now, waitInterval)) {
138-
logger.cdebug { "Ignoring keyframe request for $mediaSsrc, rate limited" }
139-
false
140-
} else {
141-
logger.cdebug { "Keyframe requester requesting keyframe for $mediaSsrc" }
142-
true
154+
155+
val perSourceLimiter = perSourceKeyframeLimiter.computeIfAbsent(mediaSsrc) {
156+
RateLimit(
157+
defaultMinInterval = sourceWideMinInterval,
158+
maxRequests = sourceWideMaxRequests,
159+
interval = sourceWideMaxRequestInterval
160+
)
161+
}
162+
163+
if (!perSourceLimiter.accept(now, waitInterval)) {
164+
logger.cdebug { "Ignoring keyframe request for $mediaSsrc from $requesterID, per-source rate limited" }
165+
return false
143166
}
167+
168+
logger.cdebug { "Keyframe requester requesting keyframe for $mediaSsrc, requested by $requesterID" }
169+
return true
144170
}
145171
}
146172

147-
fun requestKeyframe(mediaSsrc: Long? = null) {
173+
fun requestKeyframe(requesterID: String?, mediaSsrc: Long? = null) {
148174
val ssrc = mediaSsrc ?: streamInformationStore.primaryMediaSsrcs.firstOrNull() ?: run {
149175
numApiRequestsDropped++
150176
logger.cdebug { "No video SSRC found to request keyframe" }
151177
return
152178
}
153179
numApiRequests++
154-
if (!canSendKeyframeRequest(ssrc, clock.instant())) {
180+
if (!canSendKeyframeRequest(requesterID, ssrc, clock.instant())) {
155181
numApiRequestsDropped++
156182
return
157183
}
@@ -233,6 +259,16 @@ class KeyframeRequester @JvmOverloads constructor(
233259
private val maxRequestInterval: Duration by config {
234260
"jmt.keyframe.max-request-interval".from(JitsiConfig.newConfig)
235261
}
262+
263+
private val sourceWideMinInterval: Duration by config {
264+
"jmt.keyframe.source-wide-min-interval".from(JitsiConfig.newConfig)
265+
}
266+
private val sourceWideMaxRequests: Int by config {
267+
"jmt.keyframe.source-wide-max-requests".from(JitsiConfig.newConfig)
268+
}
269+
private val sourceWideMaxRequestInterval: Duration by config {
270+
"jmt.keyframe.source-wide-max-request-interval".from(JitsiConfig.newConfig)
271+
}
236272
}
237273
}
238274

‎jitsi-media-transform/src/main/resources/reference.conf‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,18 @@ jmt {
6767

6868
}
6969
keyframe {
70-
// The minimum interval between consecutive keyframe requests for a media source.
70+
// The minimum interval between consecutive keyframe requests from a particular receiver for a media source.
7171
// (This interval will also be limited by the current round-trip time to the sender of that source.)
7272
min-interval = 200 ms
73-
// The maximum number of requests for a media source per max-request-interval
73+
// The maximum number of requests from a particular receiver for a media source per max-request-interval
7474
max-requests = 3
7575
// The interval over which to compute max-requests
7676
max-request-interval = 10 s
77+
78+
// The minimum interval between consecutive keyframe requests for any receiver for a media source.
79+
source-wide-min-interval = 200 ms
80+
source-wide-max-requests = 1
81+
source-wide-max-request-interval = 200 ms
7782
}
7883
srtp {
7984
// The maximum number of packets that can be discarded early (without going through the SRTP stack for

‎jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/rtcp/KeyframeRequesterTest.kt‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ class KeyframeRequesterTest : ShouldSpec() {
8383

8484
context("requesting a keyframe") {
8585
context("without a specific SSRC") {
86-
keyframeRequester.requestKeyframe()
86+
keyframeRequester.requestKeyframe("ep1")
8787
should("result in a sent PLI request with the first video SSRC") {
8888
sentKeyframeRequests shouldHaveSize 1
8989
val packet = sentKeyframeRequests.last().packet
@@ -92,7 +92,7 @@ class KeyframeRequesterTest : ShouldSpec() {
9292
}
9393
}
9494
context("when PLI is supported") {
95-
keyframeRequester.requestKeyframe(123L)
95+
keyframeRequester.requestKeyframe("ep1", 123L)
9696
should("result in a sent PLI request") {
9797
sentKeyframeRequests shouldHaveSize 1
9898
val packet = sentKeyframeRequests.last().packet
@@ -104,13 +104,13 @@ class KeyframeRequesterTest : ShouldSpec() {
104104
context("within the wait interval") {
105105
clock.elapse(10.ms)
106106
context("on the same SSRC") {
107-
keyframeRequester.requestKeyframe(123L)
107+
keyframeRequester.requestKeyframe("ep1", 123L)
108108
should("not send anything") {
109109
sentKeyframeRequests.shouldBeEmpty()
110110
}
111111
}
112-
context("on a different SSRC") {
113-
keyframeRequester.requestKeyframe(456L)
112+
context("for a different SSRC") {
113+
keyframeRequester.requestKeyframe("ep1", 456L)
114114
should("result in a sent PLI request") {
115115
sentKeyframeRequests shouldHaveSize 1
116116
val packet = sentKeyframeRequests.last().packet
@@ -121,7 +121,7 @@ class KeyframeRequesterTest : ShouldSpec() {
121121
}
122122
context("after the wait interval has expired") {
123123
clock.elapse(1.secs)
124-
keyframeRequester.requestKeyframe(123L)
124+
keyframeRequester.requestKeyframe("ep1", 123L)
125125
should("result in a sent PLI request") {
126126
sentKeyframeRequests shouldHaveSize 1
127127
val packet = sentKeyframeRequests.last().packet
@@ -133,7 +133,7 @@ class KeyframeRequesterTest : ShouldSpec() {
133133
}
134134
context("when PLI isn't supported") {
135135
streamInformationStore.supportsPli = false
136-
keyframeRequester.requestKeyframe(123L)
136+
keyframeRequester.requestKeyframe("ep1", 123L)
137137
should("result in a sent FIR request") {
138138
sentKeyframeRequests shouldHaveSize 1
139139
val packet = sentKeyframeRequests.last().packet
@@ -144,7 +144,7 @@ class KeyframeRequesterTest : ShouldSpec() {
144144
context("when neither PLI nor FIR is supported") {
145145
streamInformationStore.supportsFir = false
146146
streamInformationStore.supportsPli = false
147-
keyframeRequester.requestKeyframe(123L)
147+
keyframeRequester.requestKeyframe("ep1", 123L)
148148
should("not send anything") {
149149
sentKeyframeRequests.shouldBeEmpty()
150150
}

‎jvb/src/main/java/org/jitsi/videobridge/Conference.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -408,15 +408,17 @@ public void broadcastMessage(BridgeChannelMessage msg)
408408
* Requests a keyframe from the endpoint with the specified id, if the
409409
* endpoint is found in the conference.
410410
*
411+
* @param requesterID the id of the endpoint requesting a keyframe
411412
* @param endpointID the id of the endpoint to request a keyframe from.
413+
* @param mediaSsrc the primary SSRC of the source for which to request a keyframe
412414
*/
413-
public void requestKeyframe(String endpointID, long mediaSsrc)
415+
public void requestKeyframe(String requesterID, String endpointID, long mediaSsrc)
414416
{
415417
AbstractEndpoint remoteEndpoint = getEndpoint(endpointID);
416418

417419
if (remoteEndpoint != null)
418420
{
419-
remoteEndpoint.requestKeyframe(mediaSsrc);
421+
remoteEndpoint.requestKeyframe(requesterID, mediaSsrc);
420422
}
421423
else if (logger.isDebugEnabled())
422424
{

‎jvb/src/main/kotlin/org/jitsi/videobridge/AbstractEndpoint.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ abstract class AbstractEndpoint protected constructor(
224224
*
225225
* @param mediaSsrc the media SSRC to request a keyframe from.
226226
*/
227-
abstract fun requestKeyframe(mediaSsrc: Long)
227+
abstract fun requestKeyframe(requesterID: String, mediaSsrc: Long)
228228

229229
/**
230230
* Requests a keyframe from this endpoint on the first video SSRC

‎jvb/src/main/kotlin/org/jitsi/videobridge/Endpoint.kt‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,8 @@ class Endpoint @JvmOverloads constructor(
223223
newEffectiveConstraints: EffectiveConstraintsMap,
224224
) = this@Endpoint.effectiveVideoConstraintsChanged(oldEffectiveConstraints, newEffectiveConstraints)
225225

226-
override fun keyframeNeeded(endpointId: String?, ssrc: Long) = conference.requestKeyframe(endpointId, ssrc)
226+
override fun keyframeNeeded(endpointId: String?, ssrc: Long) =
227+
conference.requestKeyframe(id, endpointId, ssrc)
227228
},
228229
{ getOrderedEndpoints() },
229230
diagnosticContext,
@@ -802,7 +803,8 @@ class Endpoint @JvmOverloads constructor(
802803

803804
override fun requestKeyframe() = transceiver.requestKeyFrame()
804805

805-
override fun requestKeyframe(mediaSsrc: Long) = transceiver.requestKeyFrame(mediaSsrc)
806+
override fun requestKeyframe(requesterID: String, mediaSsrc: Long) =
807+
transceiver.requestKeyFrame(requesterID, mediaSsrc)
806808

807809
/** Whether we are currently oversending to this endpoint. */
808810
fun isOversending(): Boolean = bitrateController.isOversending()

‎jvb/src/main/kotlin/org/jitsi/videobridge/relay/RelayedEndpoint.kt‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,10 @@ class RelayedEndpoint(
127127
/** Relayed endpoints are not automatically expired. **/
128128
override fun shouldExpire(): Boolean = false
129129

130-
override fun requestKeyframe(mediaSsrc: Long) = relay.transceiver.requestKeyFrame(mediaSsrc)
130+
override fun requestKeyframe(requesterID: String, mediaSsrc: Long) =
131+
relay.transceiver.requestKeyFrame(requesterID, mediaSsrc)
131132

132-
override fun requestKeyframe() = relay.transceiver.requestKeyFrame(mediaSource?.primarySSRC)
133+
override fun requestKeyframe() = relay.transceiver.requestKeyFrame(null, mediaSource?.primarySSRC)
133134

134135
override val isSendingAudio
135136
get() = rtpReceiver.isReceivingAudio()

0 commit comments

Comments
 (0)