From 36b6741489e8d329b936688c25f3a13b8806d9c7 Mon Sep 17 00:00:00 2001 From: mdaneshi Date: Tue, 10 May 2016 16:17:11 -0700 Subject: [PATCH 1/3] [Jisti-Bridge] Move the RoC logic to videoChannel. Summary: SRTP logic to detecting ROC doesn't quiet work when streams are paused (due to speech activity) and once a stream resumes, it may fail to guess the correct ROC, causing rtp packet drops. Jitsi already had a logic for handing this (sending a few rtp packets every once in a while) but not general enough for our usecase. In this CR, I moved that logic to videoChannel to take advantage of LastNController Test Plan: tested locally Reviewers: brian Subscribers: eng-team-list Differential Revision: https://phab.fatline.io/D3263 --- .../org/jitsi/videobridge/VideoChannel.java | 101 ++++++++++++++++-- .../sendmodes/SwitchingSendMode.java | 93 ---------------- 2 files changed, 95 insertions(+), 99 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index b4c7b20099..eb5d582f0e 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -19,7 +19,10 @@ import java.io.*; import java.net.*; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.*; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import javax.media.rtp.*; @@ -109,6 +112,13 @@ public class VideoChannel = SimulcastStream.SIMULCAST_LAYER_ORDER_BASE; // Integer.MAX_VALUE; /** + * A cyclic counters multitone that counts how many packets we've dropped + * per source end point. + */ + private final CyclicCounters dropped = new CyclicCounters(); + + + /** * Updates the values of the property inLastN of all * VideoChannels in the Content of a specific * VideoChannel. @@ -498,14 +508,31 @@ boolean rtpTranslatorWillWrite( byte[] buffer, int offset, int length, Channel source) { - boolean accept = true; - - if (data && (source != null)) + if (!data || source == null) { - // XXX(gp) we could potentially move this into a TransformEngine. - accept = lastNController.isForwarded(source); + return true; + } + // XXX(gp) we could potentially move this into a TransformEngine. + boolean accept = lastNController.isForwarded(source); + if (!accept && !source.getID().equals(this.getID())) + { + // For SRTP replay protection the webrtc.org implementation uses a + // replay database with extended range, using a rollover counter + // (ROC) which counts the number of times the RTP sequence number + // carried in the RTP packet has rolled over. + // + // In this way, the ROC extends the 16-bit RTP sequence number to a + // 48-bit "SRTP packet index". The ROC is not be explicitly + // exchanged between the SRTP endpoints because in all practical + // situations a rollover of the RTP sequence number can be detected + // unless 2^15 consecutive RTP packets are lost. + // + // If this variable is set to true, then for every 0x800 (2048) + // dropped packets (at most), we send 8 packets so that the + // receiving endpoint can update its ROC. + CyclicCounter counter = dropped.getOrCreate(source.getEndpoint().getID(), 0x800); + accept = counter.cyclicallyIncrementAndGet() < 8; } - return accept; } @@ -1248,4 +1275,66 @@ public void setAdaptiveSimulcast(boolean adaptiveSimulcast) { lastNController.setAdaptiveSimulcast(adaptiveSimulcast); } + + /** + * A thread safe cyclic counter. + */ + static class CyclicCounter + { + private final AtomicInteger ai = new AtomicInteger(0); + + private final int maxVal; + + public CyclicCounter(int maxVal) + { + this.maxVal = maxVal; + } + + public int cyclicallyIncrementAndGet() + { + int curVal, newVal; + do + { + curVal = this.ai.get(); + newVal = (curVal + 1) % this.maxVal; + // note that this doesn't guarantee fairness + } + while (!this.ai.compareAndSet(curVal, newVal)); + return newVal; + } + } + + /** + * Multitone pattern with Lazy Initialization. + */ + static class CyclicCounters + { + private final Map instances + = new ConcurrentHashMap<>(); + + private final Lock createLock = new ReentrantLock(); + + CyclicCounter getOrCreate(String key, int maxVal) + { + CyclicCounter instance = instances.get(key); + + if (instance == null) + { + createLock.lock(); + try + { + if ((instance = instances.get(key)) == null) + { + instance = new CyclicCounter(maxVal); + instances.put(key, instance); + } + } + finally + { + createLock.unlock(); + } + } + return instance; + } + } } diff --git a/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java b/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java index e904ad1d4d..96cbe149de 100644 --- a/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java +++ b/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java @@ -71,12 +71,6 @@ public class SwitchingSendMode private static final SimulcastMessagesMapper mapper = new SimulcastMessagesMapper(); - /** - * A cyclic counters multitone that counts how many packets we've dropped - * per SSRC. - */ - private final CyclicCounters dropped = new CyclicCounters(); - /** * The sync root object protecting the access to the simulcast streams. */ @@ -194,31 +188,6 @@ public boolean accept(RawPacket pkt) if (override != null) accept = override.matches(pkt); - if (!accept) - { - // For SRTP replay protection the webrtc.org implementation uses a - // replay database with extended range, using a rollover counter - // (ROC) which counts the number of times the RTP sequence number - // carried in the RTP packet has rolled over. - // - // In this way, the ROC extends the 16-bit RTP sequence number to a - // 48-bit "SRTP packet index". The ROC is not be explicitly - // exchanged between the SRTP endpoints because in all practical - // situations a rollover of the RTP sequence number can be detected - // unless 2^15 consecutive RTP packets are lost. - // - // If this variable is set to true, then for every 0x800 (2048) - // dropped packets (at most), we send 8 packets so that the - // receiving endpoint can update its ROC. - // - // TODO(gp) We may want to move this code somewhere more centralized - // to take into account last-n etc. - - Integer key = pkt.getSSRC(); - CyclicCounter counter = dropped.getOrCreate(key, 0x800); - accept = counter.cyclicallyIncrementAndGet() < 8; - } - if (logger.isDebugEnabled()) { logger.debug("Accepting packet " @@ -678,68 +647,6 @@ private void maybeConfigureOverride(SwitchingModeOptions options) } } - /** - * A thread safe cyclic counter. - */ - static class CyclicCounter - { - private final AtomicInteger ai = new AtomicInteger(0); - - private final int maxVal; - - public CyclicCounter(int maxVal) - { - this.maxVal = maxVal; - } - - public int cyclicallyIncrementAndGet() - { - int curVal, newVal; - do - { - curVal = this.ai.get(); - newVal = (curVal + 1) % this.maxVal; - // note that this doesn't guarantee fairness - } - while (!this.ai.compareAndSet(curVal, newVal)); - return newVal; - } - } - - /** - * Multitone pattern with Lazy Initialization. - */ - static class CyclicCounters - { - private final Map instances - = new ConcurrentHashMap<>(); - - private final Lock createLock = new ReentrantLock(); - - CyclicCounter getOrCreate(Integer key, int maxVal) - { - CyclicCounter instance = instances.get(key); - - if (instance == null) - { - createLock.lock(); - try - { - if ((instance = instances.get(key)) == null) - { - instance = new CyclicCounter(maxVal); - instances.put(key, instance); - } - } - finally - { - createLock.unlock(); - } - } - return instance; - } - } - /** * Holds the configuration options for the SwitchingSimulcastSender. * From ab0207f8688ee9f1e66b101b99df6d32c296afa0 Mon Sep 17 00:00:00 2001 From: mdaneshi Date: Wed, 11 May 2016 13:36:08 -0700 Subject: [PATCH 2/3] addree CR --- src/main/java/org/jitsi/videobridge/VideoChannel.java | 6 +++--- .../videobridge/simulcast/sendmodes/SwitchingSendMode.java | 4 ---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index eb5d582f0e..bb3e2df838 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -19,10 +19,10 @@ import java.io.*; import java.net.*; import java.util.*; -import java.util.concurrent.ConcurrentHashMap; +import java.util.*; +import java.util.concurrent.*; import java.util.concurrent.atomic.*; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.locks.*; import javax.media.rtp.*; diff --git a/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java b/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java index 96cbe149de..7af1a03c20 100644 --- a/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java +++ b/src/main/java/org/jitsi/videobridge/simulcast/sendmodes/SwitchingSendMode.java @@ -25,10 +25,6 @@ import java.io.*; import java.lang.ref.*; -import java.util.*; -import java.util.concurrent.*; -import java.util.concurrent.atomic.*; -import java.util.concurrent.locks.*; /** * The SwitchingSendMode implements the switching simulcast streams From dc28f281f5085bc195a698f30d57655b1d018a6e Mon Sep 17 00:00:00 2001 From: mdaneshi Date: Wed, 11 May 2016 13:38:31 -0700 Subject: [PATCH 3/3] remove extra import line --- src/main/java/org/jitsi/videobridge/VideoChannel.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index bb3e2df838..e20f523b1b 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -19,7 +19,6 @@ import java.io.*; import java.net.*; import java.util.*; -import java.util.*; import java.util.concurrent.*; import java.util.concurrent.atomic.*; import java.util.concurrent.locks.*;