From 49a6dac8c63d6eb7d9141dd3db9ad0d7f2308498 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Tue, 8 Dec 2015 14:00:17 -0600 Subject: [PATCH 01/13] Splits the last-n related code form VideoChannel into LastNController (breaks adaptive last-n and adaptive simulcast). --- .../java/org/jitsi/videobridge/Content.java | 30 + .../jitsi/videobridge/LastNController.java | 405 ++++++++++ .../org/jitsi/videobridge/RtpChannel.java | 22 +- .../org/jitsi/videobridge/VideoChannel.java | 734 ++---------------- .../ratecontrol/BitrateController.java | 15 +- .../ratecontrol/VideoChannelLastNAdaptor.java | 4 + 6 files changed, 517 insertions(+), 693 deletions(-) create mode 100644 src/main/java/org/jitsi/videobridge/LastNController.java diff --git a/src/main/java/org/jitsi/videobridge/Content.java b/src/main/java/org/jitsi/videobridge/Content.java index f325f59447..bfef97670a 100644 --- a/src/main/java/org/jitsi/videobridge/Content.java +++ b/src/main/java/org/jitsi/videobridge/Content.java @@ -194,6 +194,36 @@ public boolean accept( return accept; } + /** + * Sends keyframe requests for all SSRCs in all video channels of the + * endpoints specified by ID in {@code endpointIds}. + * @param endpointIds the list of IDs of endpoints to send keyframe + * requests to. + */ + public void askForKeyframesById(Collection endpointIds) + { + List endpoints = new LinkedList<>(); + Conference conference = getConference(); + for (String endpointId : endpointIds) + { + Endpoint endpoint = conference.getEndpoint(endpointId); + if (endpoint != null) + { + endpoints.add(endpoint); + } + } + + if (!endpoints.isEmpty()) + { + askForKeyframes(endpoints); + } + } + + /** + * Sends keyframe requests for all SSRCs in all video channels of the + * endpoints in {@code endpoints}. + * @param endpoints the list of endpoints to send keyframe requests to. + */ void askForKeyframes(Collection endpoints) { for (Endpoint endpoint : endpoints) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java new file mode 100644 index 0000000000..2ec23d0362 --- /dev/null +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -0,0 +1,405 @@ +package org.jitsi.videobridge; + +import org.jitsi.util.*; + +import java.util.*; + +/** + * Manages the set of Endpoints whose video streams are being + * forwarded to a specific VideoChannel (i.e. the + * VideoChannel's LastN set). + * + * @author Lyubomir Marinov + * @author George Politis + * @author Boris Grozev + */ +public class LastNController +{ + /** + * The Logger used by the VideoChannel class and its + * instances to print debug information. + */ + private static final Logger logger = Logger.getLogger(VideoChannel.class); + + /** + * The set of Endpoints whose video streams are currently being + * forwarded. + */ + private List forwardedEndpoints + = Collections.unmodifiableList(new LinkedList()); + + /** + * The list of all Endpoints in the conference, ordered by the + * last time they were elected dominant speaker. + */ + private List conferenceSpeechActivityEndpoints = new LinkedList<>(); + + /** + * The list of endpoints which have been explicitly marked as 'pinned' + * and whose video streams should always be forwarded. + */ + private List pinnedEndpoints + = Collections.unmodifiableList(new LinkedList()); + + /** + * The maximum number of endpoints whose video streams will be forwarded + * to the endpoint. A value of {@code -1} means that there is no limit, and + * all endpoints' video streams will be forwarded. + */ + private int lastN = -1; + + /** + * Whether or not adaptive lastN is in use. + */ + private boolean adaptiveLastN = false; + + /** + * Whether or not adaptive simulcast is in use. + */ + private boolean adaptiveSimulcast = false; + + /** + * The {@link VideoChannel} which owns this {@link LastNController}. + */ + private final VideoChannel channel; + + /** + * The ID of the endpoint of {@link #channel}. + */ + private String endpointId; + + /** + * Initializes a new {@link LastNController} instance which is to belong + * to a particular {@link VideoChannel}. + * @param channel the owning {@link VideoChannel}. + */ + public LastNController(VideoChannel channel) + { + this.channel = channel; + } + + /** + * @return the maximum number of endpoints whose video streams will be + * forwarded to the endpoint. A value of {@code -1} means that there is no + * limit. + */ + public int getLastN() + { + return lastN; + } + + /** + * @return the set of Endpoints whose video streams are currently + * being forwarded. + */ + public List getForwardedEndpoints() + { + return forwardedEndpoints; + } + + /** + * Sets the value of {@code lastN}, that is, the maximum number of endpoints + * whose video streams will be forwarded to the endpoint. A value of + * {@code -1} means that there is no limit. + * @param lastN the value to set. + */ + public void setLastN(int lastN) + { + List endpointsToAskForKeyframe; + synchronized (this) + { + // Since we have the lock anyway, call update() inside, so it + // doesn't have to obtain it again. But keep the call to + // askForKeyframes() outside. + + this.lastN = lastN; + + endpointsToAskForKeyframe = update(); + } + + askForKeyframes(endpointsToAskForKeyframe); + } + + /** + * Sets the list of "pinned" endpoints (i.e. endpoints for which video + * should always be forwarded, regardless of {@code lastN}). + * @param newPinnedEndpoints the list of endpoints to set. + */ + public void setPinnedEndpoints( + Collection newPinnedEndpoints) + { + List newPinnedEndpointIds = new LinkedList<>(); + for (Endpoint endpoint : newPinnedEndpoints) + { + newPinnedEndpointIds.add(endpoint.getID()); + } + + List endpointsToAskForKeyframe; + synchronized (this) + { + // Since we have the lock anyway, call update() inside, so it + // doesn't have to obtain it again. But keep the call to + // askForKeyframes() outside. + this.pinnedEndpoints + = Collections.unmodifiableList(newPinnedEndpointIds); + + endpointsToAskForKeyframe = update(); + } + + askForKeyframes(endpointsToAskForKeyframe); + } + + /** + * Checks whether RTP packets from {@code sourceChannel} should be forwarded + * to {@link #channel}. + * @param sourceChannel the channel. + * @return {@code true} iff RTP packets from {@code sourceChannel} should + * be forwarded to {@link #channel}. + */ + public boolean isForwarded(Channel sourceChannel) + { + if (lastN < 0) + { + // If Last-N is disabled, we forward everything. + return true; + } + + if (sourceChannel == null) + { + logger.warn("Invalid sourceChannel: null."); + return false; + } + + Endpoint channelEndpoint = sourceChannel.getEndpoint(); + if (channelEndpoint == null) + { + return false; + } + + return forwardedEndpoints.contains(channelEndpoint.getID()); + } + + /** + * @return the number of streams currently being forwarded. + */ + public int getN() + { + return forwardedEndpoints.size(); + } + + /** + * @return the list of "pinned" endpoints. + */ + public List getPinnedEndpoints() + { + return pinnedEndpoints; + } + + /** + * Notifies this instance that the ordered list of endpoints in the + * conference has changed. + * + * @param endpoints the new ordered list of endpoints in the conference. + * @return the list of endpoints which were added to the list of forwarded + * endpoints as a result of the call. + */ + public List speechActivityEndpointsChanged( + List endpoints) + { + List newEndpointIdList = new LinkedList<>(); + for (Endpoint endpoint : endpoints) + { + newEndpointIdList.add(endpoint.getID()); + } + + List enteringEndpointIds + = speechActivityEndpointIdsChanged(newEndpointIdList); + + List ret = new LinkedList<>(); + if (enteringEndpointIds != null) + { + for (Endpoint endpoint : endpoints) + { + if (enteringEndpointIds.contains(endpoint.getID())) + { + ret.add(endpoint); + } + } + } + + return ret; + } + + /** + * Notifies this instance that the ordered list of endpoints (specified + * as a list of endpoint IDs) in the conference has changed. + * + * @param endpointIds the new ordered list of endpoints (specified as a list + * of endpoint IDs) in the conference. + * @return the list of IDs of endpoints which were added to the list of + * forwarded endpoints as a result of the call. + */ + private synchronized List speechActivityEndpointIdsChanged( + List endpointIds) + { + boolean change = false; + if (endpointIds.size() != conferenceSpeechActivityEndpoints.size()) + { + change = true; + } + else + { + for (int i = 0; i < endpointIds.size(); i++) + { + if (!(conferenceSpeechActivityEndpoints.get(i) + .equals(endpointIds.get(i)))) + { + change = true; + break; + } + } + } + + if (!change) + { + return null; + } + + conferenceSpeechActivityEndpoints = endpointIds; + + return update(); + } + + /** + * Enables or disables the "adaptive last-n" mode, depending on the value of + * {@code adaptiveLastN}. + * @param adaptiveLastN {@code true} to enable, {@code false} to disable + */ + public void setAdaptiveLastN(boolean adaptiveLastN) + { + this.adaptiveLastN = adaptiveLastN; + // TODO: actually enable/disable + } + + /** + * Enables or disables the "adaptive simulcast" mod, depending on the value + * of {@code adaptiveLastN}. + * @param adaptiveSimulcast {@code true} to enable, {@code false} to + * disable. + */ + public void setAdaptiveSimulcast(boolean adaptiveSimulcast) + { + this.adaptiveSimulcast = adaptiveSimulcast; + // TODO: actually enable/disable + } + + /** + * @return {@code true} iff the "adaptive last-n" mode is enabled. + */ + public boolean getAdaptiveLastN() + { + return adaptiveLastN; + } + + /** + * @return {@code true} iff the "adaptive simulcast" mode is enabled. + */ + public boolean getAdaptiveSimulcast() + { + return adaptiveSimulcast; + } + + /** + * Recalculates the list of forwarded endpoints based on the current values + * of the various parameters of this instance ({@link #lastN}, + * {@link #conferenceSpeechActivityEndpoints}, {@link #pinnedEndpoints}). + * + * @return the list of IDs of endpoints which were added to + * {@link #forwardedEndpoints} (i.e. of endpoints * "entering last-n") as a + * result of this call. + */ + private synchronized List update() + { + List newForwardedEndpoints = new LinkedList<>(); + String ourEndpointId = getEndpointId(); + + if (lastN < 0) + { + // Last-N is disabled, we forward everything. + newForwardedEndpoints.addAll(conferenceSpeechActivityEndpoints); + if (ourEndpointId != null) + { + newForwardedEndpoints.remove(ourEndpointId); + } + } + else + { + // Pinned endpoints are always forwarded. + newForwardedEndpoints.addAll(getPinnedEndpoints()); + // As long as they are still endpoints in the conference. + newForwardedEndpoints.retainAll(conferenceSpeechActivityEndpoints); + + if (newForwardedEndpoints.size() > lastN) + { + // What do we want in this case? It looks like a contradictory + // request from the client, but maybe it makes for a good API + // on the client to allow the pinned to override last-n. + // Unfortunately, this will not play well with Adaptive-Last-N + // or changes to Last-N for other reasons. + } + else if (newForwardedEndpoints.size() < lastN) + { + for (String endpointId : conferenceSpeechActivityEndpoints) + { + if (newForwardedEndpoints.size() < lastN) + { + if (!endpointId.equals(ourEndpointId) + && !newForwardedEndpoints.contains(endpointId)) + { + newForwardedEndpoints.add(endpointId); + } + } + else + { + break; + } + } + } + } + + List enteringEndpoints = new ArrayList<>(newForwardedEndpoints); + enteringEndpoints.removeAll(forwardedEndpoints); + + forwardedEndpoints + = Collections.unmodifiableList(newForwardedEndpoints); + + return enteringEndpoints; + } + + /** + * Sends a keyframe request to the endpoints specified in + * {@code endpointIds} + * @param endpointIds the list of IDs of endpoints to which to send a + * request for a keyframe. + */ + private void askForKeyframes(List endpointIds) + { + channel.getContent().askForKeyframesById(endpointIds); + } + + /** + * @return the ID of the endpoint of our channel. + */ + private String getEndpointId() + { + if (endpointId == null) + { + Endpoint endpoint = channel.getEndpoint(); + if (endpoint != null) + { + endpointId = endpoint.getID(); + } + } + return endpointId; + } +} diff --git a/src/main/java/org/jitsi/videobridge/RtpChannel.java b/src/main/java/org/jitsi/videobridge/RtpChannel.java index 1ba3c0dd33..24b26c8b8a 100644 --- a/src/main/java/org/jitsi/videobridge/RtpChannel.java +++ b/src/main/java/org/jitsi/videobridge/RtpChannel.java @@ -975,26 +975,6 @@ public void initialize() } } - /** - * Determines whether a specific Channel is within the set of - * Channels limited by lastN i.e. whether the RTP video - * streams of the specified channel are to be sent to the remote endpoint of - * this Channel. - * - * @param channel the Channel to be checked whether it is within - * the set of Channels limited by lastN i.e. whether its - * RTP streams are to be sent to the remote endpoint of this - * Channel - * @return true if the RTP streams of channel are to be - * sent to the remote endpoint of this Channel; otherwise, - * false. The implementation of the RtpChannel class - * always returns true. - */ - public boolean isInLastN(Channel channel) - { - return true; - } - /** * Starts {@link #stream} if it has not been started yet and if the state of * this Channel meets the prerequisites to invoke @@ -1178,6 +1158,8 @@ public void propertyChange(PropertyChangeEvent ev) { Object source = ev.getSource(); + // At the time of writing this doesn't do anything (dominantSpeakerChanged + // is empty in all Channel implementations). Should we remove it if it is unused? if ((conferenceSpeechActivity == source) && (conferenceSpeechActivity != null)) { diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index dd437b864d..a0e0083070 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -17,11 +17,9 @@ import java.beans.*; import java.io.*; -import java.lang.ref.*; import java.net.*; import java.util.*; import java.util.concurrent.atomic.*; -import java.util.concurrent.locks.*; import javax.media.rtp.*; @@ -87,11 +85,6 @@ public class VideoChannel */ private static final Logger logger = Logger.getLogger(VideoChannel.class); - /** - * The SimulcastMode for this VideoChannel. - */ - private SimulcastMode simulcastMode; - /** * Updates the values of the property inLastN of all * VideoChannels in the Content of a specific @@ -126,14 +119,9 @@ else if (t instanceof ThreadDeath) } /** - * Whether or not to use adaptive lastN. - */ - private boolean adaptiveLastN = false; - - /** - * Whether or not to use adaptive simulcast. + * The SimulcastMode for this VideoChannel. */ - private boolean adaptiveSimulcast = false; + private SimulcastMode simulcastMode; /** * The BitrateController which will be controlling the @@ -141,6 +129,13 @@ else if (t instanceof ThreadDeath) */ private BitrateController bitrateController; + /** + * The instance which controls which endpoints' video streams are to be + * forwarded on this {@link VideoChannel} (i.e. implements last-n and its + * extensions (pinned endpoints, adaptation). + */ + private final LastNController lastNController = new LastNController(this); + /** * The instance which will be computing the incoming bitrate for this * VideoChannel. @@ -154,26 +149,6 @@ else if (t instanceof ThreadDeath) */ private final AtomicBoolean inLastN = new AtomicBoolean(true); - /** - * The maximum number of video RTP stream to be sent from Jitsi Videobridge - * to the endpoint associated with this video Channel. - */ - private Integer lastN; - - /** - * The Endpoints in the multipoint conference in which this - * Channel is participating ordered by - * {@link #conferenceSpeechActivity} and used by this Channel for - * the support of {@link #lastN}. - */ - private List> lastNEndpoints; - - /** - * The Object which synchronizes the access to - * {@link #lastNEndpoints} and {@link #lastN}. - */ - private final ReadWriteLock lastNSyncRoot = new ReentrantReadWriteLock(); - /** * Whether the bridge should request retransmissions for missing packets * on this channel. @@ -297,6 +272,9 @@ public void initialize() stream.getMediaStreamStats().addNackListener(this); } + + // FIXME: this shouldn't depend on cfg, and shouldn't happen here. + getBitrateController(); } /** @@ -363,34 +341,14 @@ public void describe(ColibriConferenceIQ.ChannelCommon commonIq) super.describe(iq); - iq.setLastN(lastN); + iq.setLastN(lastNController.getLastN()); iq.setSimulcastMode(getSimulcastMode()); } - /** - * Gets a boolean value indicating whether or not to use adaptive lastN. - * - * @return a boolean value indicating whether or not to use adaptive lastN. - */ - public boolean getAdaptiveLastN() - { - return this.adaptiveLastN; - } - - /** - * Gets a boolean value indicating whether or not to use adaptive simulcast. - * - * @return a boolean value indicating whether or not to use adaptive - * simulcast. - */ - public boolean getAdaptiveSimulcast() - { - return this.adaptiveSimulcast; - } - /** * Returns the BitrateController for this VideoChannel, * creating it if necessary. + * TODO: move to LastNController * * @return the VideoChannelLastNAdaptor for this * VideoChannel, creating it if necessary. @@ -425,136 +383,7 @@ public long getIncomingBitrate() */ public int getLastN() { - Integer lastNInteger = this.lastN; - - return (lastNInteger == null) ? -1 : lastNInteger.intValue(); - } - - /** - * Returns the list of Endpoints for the purposes of - * "last N". - * - * @return the list of Endpoints for the purposes of - * "last N" - */ - public List getLastNEndpoints() - { - Lock readLock = lastNSyncRoot.readLock(); - List endpoints; - - readLock.lock(); - try - { - if (lastNEndpoints == null || lastNEndpoints.isEmpty()) - { - endpoints = Collections.emptyList(); - } - else - { - endpoints = new ArrayList<>(lastNEndpoints.size()); - for (WeakReference wr : lastNEndpoints) - { - Endpoint endpoint = wr.get(); - - if (endpoint != null) - endpoints.add(endpoint); - } - } - } - finally - { - readLock.unlock(); - } - - return endpoints; - } - - private Endpoint getEffectivePinnedEndpoint() - { - // For the purposes of LastN, we consider that the user has no pinned - // participant if he/she has pinned himself/herself. - Endpoint thisEndpoint = getEndpoint(); - - if (thisEndpoint == null) - return null; - - Endpoint pinnedEndpoint = thisEndpoint.getPinnedEndpoint(); - - return thisEndpoint.equals(pinnedEndpoint) ? null : pinnedEndpoint; - } - - public int getReceivingEndpointCount() - { - int receivingEndpointCount; - - if (getLastN() == -1) - { - // LastN is disabled. Consequently, this endpoint receives all the - // other participants. - receivingEndpointCount - = getContent().getConference().getEndpointCount(); - } - else - { - // LastN is enabled. Get the last N endpoints that this endpoint is - // receiving. - receivingEndpointCount = getLastNEndpoints().size(); - } - return receivingEndpointCount; - } - - /** - * Creates and returns an iterator of the endpoints that are currently - * being received by this channel. - * - * @return an iterator of the endpoints that are currently being received - * by this channel. - */ - public Iterator getReceivingEndpoints() - { - final List endpoints; - - if (getLastN() == -1) - { - // LastN is disabled. Consequently, this endpoint receives all the - // other participants. - endpoints = getContent().getConference().getEndpoints(); - } - else - { - // LastN is enabled. Get the last N endpoints that this endpoint is - // receiving. - endpoints = getLastNEndpoints(); - } - - final int lastIx = endpoints.size() - 1; - - return - new Iterator() - { - private int ix = 0; - - @Override - public boolean hasNext() - { - return ix <= lastIx; - } - - @Override - public Endpoint next() - { - if (hasNext()) - return endpoints.get(ix++); - else - throw new NoSuchElementException(); - } - - @Override - public void remove() - { - throw new UnsupportedOperationException(); - } - }; + return lastNController.getLastN(); } /** @@ -599,151 +428,23 @@ public boolean isInLastN() } /** - * {@inheritDoc} - */ - @Override - public boolean isInLastN(Channel channel) - { - int lastN = getLastN(); - - if (lastN < 0) - return true; - - Endpoint channelEndpoint = channel.getEndpoint(); - - if (channelEndpoint == null) - return true; - - ConferenceSpeechActivity conferenceSpeechActivity - = this.conferenceSpeechActivity; - - if (conferenceSpeechActivity == null) - return true; - if (lastN == 0) - return false; - - // We do not hold any lock on lastNSyncRoot here because it should be OK - // for multiple threads to check whether lastNEndpoints is null and - // invoke the method to populate it because (1) the method to populate - // lastNEndpoints will acquire the necessary locks to ensure preserving - // the correctness of the state of this instance under the conditions of - // concurrent access and (2) we do not want to hold a write lock on - // lastNSyncRoot while invoking the method to populate lastNEndpoints - // because the latter might fire an event. - if (lastNEndpoints == null) - { - // Pretend that the ordered list of Endpoints maintained by - // conferenceSpeechActivity has changed in order to populate - // lastNEndpoints. - speechActivityEndpointsChanged(null); - } - - Lock readLock = lastNSyncRoot.readLock(); - boolean inLastN = false; - - readLock.lock(); - try - { - if (lastNEndpoints != null) - { - int n = 0; - // The pinned endpoint is always in the last N set, if - // last N > 0. - Endpoint pinnedEndpoint = getEffectivePinnedEndpoint(); - // Keep one empty slot for the pinned endpoint. - int nMax = (pinnedEndpoint == null) ? lastN : (lastN - 1); - Endpoint thisEndpoint = getEndpoint(); - - for (WeakReference wr : lastNEndpoints) - { - if (n >= nMax) - break; - - Endpoint e = wr.get(); - - if (e != null) - { - if (e.equals(thisEndpoint)) - { - continue; - } - else if (e.equals(channelEndpoint)) - { - inLastN = true; - break; - } - } - - ++n; - } - - // FIXME(gp) move this if before the for loop (to avoid an - // unnecessary loop) - if (!inLastN && pinnedEndpoint != null) - inLastN = channelEndpoint == pinnedEndpoint; - } - } - finally - { - readLock.unlock(); - } - return inLastN; - } - - /** - * Notifies this instance that the list of Endpoints defined by - * {@link #lastN} has changed. + * Determines whether a specific Channel is within the set of + * Channels limited by lastN i.e. whether the RTP video + * streams of the specified channel are to be sent to the remote endpoint of + * this Channel. * - * @param endpointsEnteringLastN the Endpoints which are entering - * the list of Endpoints defined by lastN + * @param channel the Channel to be checked whether it is within + * the set of Channels limited by lastN i.e. whether its + * RTP streams are to be sent to the remote endpoint of this + * Channel + * @return true if the RTP streams of channel are to be + * sent to the remote endpoint of this Channel; otherwise, + * false. The implementation of the RtpChannel class + * always returns true. */ - private void lastNEndpointsChanged(List endpointsEnteringLastN) - { - try - { - sendLastNEndpointsChangeEventOnDataChannel(endpointsEnteringLastN); - } - finally - { - updateInLastN(this); - } - } - - /** - * Gets the index of a specific Endpoint in a specific list of - * lastN Endpoints. - * - * @param endpoints the list of Endpoints into which to look for - * endpoint - * @param lastN the number of Endpoints in endpoints to - * look through - * @param endpoint the Endpoint to find within lastN - * elements of endpoints - * @return the lastN index of endpoint in - * endpoints or -1 if endpoint is not within the - * lastN elements of endpoints - */ - private int lastNIndexOf( - List endpoints, - int lastN, - Endpoint endpoint) + public boolean isInLastN(Channel channel) { - Endpoint thisEndpoint = getEndpoint(); - int n = 0; - - for (Endpoint e : endpoints) - { - if (n >= lastN) - break; - - if (e.equals(thisEndpoint)) - continue; - else if (e.equals(endpoint)) - return n; - - ++n; - } - return -1; + return lastNController.isForwarded(channel); } @Override @@ -755,25 +456,8 @@ public void propertyChange(PropertyChangeEvent ev) if (Endpoint.PINNED_ENDPOINT_PROPERTY_NAME.equals(propertyName)) { - // The pinned endpoint is always in the last N set, if last N > 0. - // So, it (the pinned endpoint) has changed, the lastN has changed. - if (this.getLastN() < 1) - { - return; - } - - // Pretend that the ordered list of Endpoints maintained by - // conferenceSpeechActivity has changed in order to populate - // lastNEndpoints and get the channel endpoints to ask for key - // frames. - List channelEndpointsToAskForKeyframes - = speechActivityEndpointsChanged(null, true); - - if ((channelEndpointsToAskForKeyframes != null) - && !channelEndpointsToAskForKeyframes.isEmpty()) - { - getContent().askForKeyframes(channelEndpointsToAskForKeyframes); - } + lastNController.setPinnedEndpoints( + Collections.singleton(getEndpoint().getPinnedEndpoint())); } else if (Content.CHANNEL_MODIFIED_PROPERTY_NAME.equals(propertyName)) { @@ -787,21 +471,6 @@ else if (Content.CHANNEL_MODIFIED_PROPERTY_NAME.equals(propertyName)) } } - /** - * Notifies this VideoChannel that an RTCP REMB packet with a - * bitrate value of bitrateBps bits per second was received. - * - * @param bitrateBps the bitrate of the received REMB packet in bits per - * second. - */ - public void handleREMB(long bitrateBps) - { - BitrateController bc = getBitrateController(); - - if (bc != null) - bc.receivedREMB(bitrateBps); - } - /** * {@inheritDoc} */ @@ -816,7 +485,7 @@ boolean rtpTranslatorWillWrite( if (data && (source != null)) { // XXX(gp) we could potentially move this into a TransformEngine. - accept = isInLastN(source); + accept = lastNController.isForwarded(source); } return accept; @@ -836,7 +505,11 @@ void sctpConnectionReady(Endpoint endpoint) super.sctpConnectionReady(endpoint); if (endpoint.equals(getEndpoint())) - lastNEndpointsChanged(null); + { + // TODO: + // 1. Send list of endpoints to the channel + // 2. updateInLastN(this) + } } /** @@ -849,126 +522,40 @@ void sctpConnectionReady(Endpoint endpoint) * the list of Endpoints defined by lastN */ private void sendLastNEndpointsChangeEventOnDataChannel( - List endpointsEnteringLastN) + List endpointsEnteringLastN) { - int lastN = getLastN(); - - if (lastN < 0) - return; - Endpoint thisEndpoint = getEndpoint(); if (thisEndpoint == null) return; - // Represent the list of Endpoints defined by lastN in JSON format. - Lock readLock = lastNSyncRoot.readLock(); - StringBuilder lastNEndpointsStr = new StringBuilder(); + List forwardedEndpoints + = lastNController.getForwardedEndpoints(); + // We want endpointsEnteringLastN to always to reported. Consequently, // we will pretend that all lastNEndpoints are entering if no explicit // endpointsEnteringLastN is specified. - List effectiveEndpointsEnteringLastN = endpointsEnteringLastN; - - if (effectiveEndpointsEnteringLastN == null) - effectiveEndpointsEnteringLastN = new ArrayList<>(lastN); - - // The pinned endpoint is always in the last N set, if last N > 0. - Endpoint pinnedEndpoint = getEffectivePinnedEndpoint(); - - readLock.lock(); - try - { - if ((lastNEndpoints != null) && !lastNEndpoints.isEmpty()) - { - int n = 0; - boolean foundPinnedEndpoint = pinnedEndpoint == null; - - for (WeakReference wr : lastNEndpoints) - { - if (n >= lastN) - break; - Endpoint e = wr.get(); - - // The pinned endpoint is always in the last N set, if - // last N > 0. - if (!foundPinnedEndpoint) - { - if (n == lastN - 1) - e = pinnedEndpoint; - else - foundPinnedEndpoint = e == pinnedEndpoint; - } - - if (e != null) - { - if (e.equals(thisEndpoint)) - { - continue; - } - else - { - if (lastNEndpointsStr.length() != 0) - lastNEndpointsStr.append(','); - lastNEndpointsStr.append('"'); - lastNEndpointsStr.append( - JSONValue.escape(e.getID())); - lastNEndpointsStr.append('"'); - - if (effectiveEndpointsEnteringLastN - != endpointsEnteringLastN) - { - effectiveEndpointsEnteringLastN.add(e); - } - } - } - - ++n; - } - } - } - finally - { - readLock.unlock(); - } + // XXX do we really want that? + if (endpointsEnteringLastN == null) + endpointsEnteringLastN = forwardedEndpoints; + // XXX Should we just build JSON here? // colibriClass StringBuilder msg = new StringBuilder( "{\"colibriClass\":\"LastNEndpointsChangeEvent\""); - // lastNEndpoints - msg.append(",\"lastNEndpoints\":["); - msg.append(lastNEndpointsStr); - msg.append(']'); - - // endpointsEnteringLastN - - // We want endpointsEnteringLastN to always to reported. Consequently, - // we will pretend that all lastNEndpoints are entering if no explicit - // endpointsEnteringLastN is specified. - endpointsEnteringLastN = effectiveEndpointsEnteringLastN; - if (!endpointsEnteringLastN.isEmpty()) { - StringBuilder endpointsEnteringLastNStr = new StringBuilder(); + // lastNEndpoints + msg.append(",\"lastNEndpoints\":"); + msg.append(getJsonString(forwardedEndpoints)); - for (Endpoint e : endpointsEnteringLastN) - { - if (endpointsEnteringLastNStr.length() != 0) - endpointsEnteringLastNStr.append(','); - endpointsEnteringLastNStr.append('"'); - endpointsEnteringLastNStr.append( - JSONValue.escape(e.getID())); - endpointsEnteringLastNStr.append('"'); - } - if (endpointsEnteringLastNStr.length() != 0) - { - msg.append(",\"endpointsEnteringLastN\":["); - msg.append(endpointsEnteringLastNStr); - msg.append(']'); - } + // endpointsEnteringLastN + msg.append(",\"endpointsEnteringLastN\":"); + msg.append(getJsonString(endpointsEnteringLastN)); } - msg.append('}'); + try { thisEndpoint.sendMessageOnDataChannel(msg.toString()); @@ -979,22 +566,17 @@ private void sendLastNEndpointsChangeEventOnDataChannel( } } - /** - * {@inheritDoc} - */ - @Override - public void setAdaptiveLastN(boolean adaptiveLastN) + private String getJsonString(List strings) { - this.adaptiveLastN = adaptiveLastN; - } - - /** - * {@inheritDoc} - */ - @Override - public void setAdaptiveSimulcast(boolean adaptiveSimulcast) - { - this.adaptiveSimulcast = adaptiveSimulcast; + JSONArray array = new JSONArray(); + if (strings != null && !strings.isEmpty()) + { + for (String s : strings) + { + array.add(s); + } + } + return array.toString(); } /** @@ -1003,100 +585,7 @@ public void setAdaptiveSimulcast(boolean adaptiveSimulcast) @Override public void setLastN(Integer lastN) { - // XXX Comparing Integer references may not be enough to short-circuit - // so a more complex comaprison for equality is implemented bellow. - if (this.lastN == null) - { - if (lastN == null) - return; - } - else if (lastN != null && this.lastN.intValue() == lastN.intValue()) - { - return; - } - - Lock writeLock = lastNSyncRoot.writeLock(); - List endpointsEnteringLastN = new LinkedList<>(); - // If the old value was null, even though we may detect endpoints - // "entering" lastN, they are already being received and so no keyframes - // are necessary. - boolean askForKeyframes = this.lastN == null; - - writeLock.lock(); - try - { - // XXX(gp) question to the lastN guru : if this.lastN == null or - // this.lastN < 0, do we really want to call lastNEndpointsChanged - // with an empty (but not null!!) list of endpoints? - if (this.lastN != null && this.lastN >= 0 && lastN > this.lastN) - { - Endpoint pinnedEndpoint = getEffectivePinnedEndpoint(); - // The pinned endpoint is always in the last N set, if - // last N > 0; Count it here. - int n = (pinnedEndpoint != null) ? 1 : 0; - Endpoint thisEndpoint = getEndpoint(); - - // We do not hold any lock on lastNSyncRoot here because it - // should be OK for multiple threads to check whether - // lastNEndpoints is null and invoke the method to populate it - // because (1) the method to populate lastNEndpoints will - // acquire the necessary locks to ensure preserving the - // correctness of the state of this instance under the - // conditions of concurrent access and (2) we do not want to - // hold a write lock on lastNSyncRoot while invoking the method - // to populate lastNEndpoints because the latter might fire an - // event. - if (lastNEndpoints == null) - { - // Pretend that the ordered list of Endpoints maintained by - // conferenceSpeechActivity has changed in order to populate - // lastNEndpoints. - speechActivityEndpointsChanged(null); - } - - if (lastNEndpoints != null) - { - for (WeakReference wr : lastNEndpoints) - { - if (n >= lastN) - break; - - Endpoint endpoint = wr.get(); - - if (endpoint != null) - { - if (endpoint.equals(thisEndpoint)) - continue; - - // We've already signaled to the client the fact - // that the pinned endpoint has entered the lastN - // set when we handled the - // PINNED_ENDPOINT_PROPERTY_NAME property change - // event. Also, we've already counted it above. So, - // we don't want to either add it in the - // endpointsEnteringLastN or count it here. - if (endpoint.equals(pinnedEndpoint)) - continue; - } - - ++n; - if (n > this.lastN && endpoint != null) - endpointsEnteringLastN.add(endpoint); - } - } - } - - this.lastN = lastN; - } - finally - { - writeLock.unlock(); - } - - lastNEndpointsChanged(endpointsEnteringLastN); - - if (askForKeyframes && !endpointsEnteringLastN.isEmpty()) - getContent().askForKeyframes(new HashSet<>(endpointsEnteringLastN)); + lastNController.setLastN(lastN); touch(); // It seems this Channel is still active. } @@ -1107,102 +596,7 @@ else if (lastN != null && this.lastN.intValue() == lastN.intValue()) @Override List speechActivityEndpointsChanged(List endpoints) { - return speechActivityEndpointsChanged(endpoints, false); - } - - private List speechActivityEndpointsChanged( - List endpoints, boolean pinnedEndpointChanged) - { - Lock writeLock = lastNSyncRoot.writeLock(); - List endpointsEnteringLastN = null; - boolean lastNEndpointsChanged = pinnedEndpointChanged; - - writeLock.lock(); - try - { - // Determine which Endpoints are entering the list of lastN. - int lastN = getLastN(); - - if (endpoints == null) - { - endpoints = conferenceSpeechActivity.getEndpoints(); - } - if (lastN >= 0) - { - Endpoint thisEndpoint = getEndpoint(); - - // At most the first lastN are entering the list of lastN. - endpointsEnteringLastN = new ArrayList<>(lastN); - - // The pinned endpoint is always in the last N set, if - // last N > 0. - Endpoint pinnedEndpoint = getEffectivePinnedEndpoint(); - - if (pinnedEndpoint != null && lastN > 0) - endpointsEnteringLastN.add(pinnedEndpoint); - - for (Endpoint e : endpoints) - { - if (endpointsEnteringLastN.size() >= lastN) - break; - if (!e.equals(thisEndpoint) && !e.equals(pinnedEndpoint)) - endpointsEnteringLastN.add(e); - } - - if (lastNEndpoints != null && !lastNEndpoints.isEmpty()) - { - // Some of these first lastN are already in the list of - // lastN. - int n = 0; - - for (WeakReference wr : lastNEndpoints) - { - if (n >= lastN) - break; - - Endpoint e = wr.get(); - - if (e != null) - { - if (e.equals(thisEndpoint)) - { - continue; - } - else - { - endpointsEnteringLastN.remove(e); - if (lastNIndexOf(endpoints, lastN, e) < 0) - lastNEndpointsChanged = true; - } - } - - ++n; - } - } - } - - // Remember the Endpoints for the purposes of lastN. - lastNEndpoints = new ArrayList<>(endpoints.size()); - for (Endpoint endpoint : endpoints) - lastNEndpoints.add(new WeakReference<>(endpoint)); - } - finally - { - writeLock.unlock(); - } - - if (endpointsEnteringLastN != null - && !endpointsEnteringLastN.isEmpty()) - { - lastNEndpointsChanged = true; - } - - // Notify about changes in the list of lastN. - if (lastNEndpointsChanged) - lastNEndpointsChanged(endpointsEnteringLastN); - - // Request keyframes from the Endpoints entering the list of lastN. - return endpointsEnteringLastN; + return lastNController.speechActivityEndpointsChanged(endpoints); } /** diff --git a/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java b/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java index fa4a9591f4..6fc0e83baf 100644 --- a/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java +++ b/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java @@ -43,6 +43,7 @@ * @author George Politis */ public class BitrateController + //implements BandwidthEstimator.Listener { /** * Whether the values for the constants have been initialized or not. @@ -187,6 +188,9 @@ public class BitrateController public BitrateController(VideoChannel channel) { this.channel = channel; + // FIXME + //BandwidthEstimator be = ((VideoMediaStream) channel.getStream()).getOrCreateBandwidthEstimator(); + //be.addListener(this); initializeConfiguration(); } @@ -210,7 +214,7 @@ public int calcNumEndpointsThatFitIn() long remainingBandwidth = availableBandwidth; int numEndpointsThatFitIn = 0; - final Iterator it = channel.getReceivingEndpoints(); + final Iterator it = null; //channel.getReceivingEndpoints(); final Endpoint thisEndpoint = channel.getEndpoint(); while (it.hasNext()) @@ -296,6 +300,7 @@ private BitrateAdaptor getOrCreateBitrateAdaptor() { bitrateAdaptorSet = true; + /* if (channel.getAdaptiveLastN()) { bitrateAdaptor = new VideoChannelLastNAdaptor(this); @@ -304,6 +309,7 @@ else if (channel.getAdaptiveSimulcast()) { bitrateAdaptor = new SimulcastAdaptor(this); } + */ } return bitrateAdaptor; @@ -370,19 +376,22 @@ private void initializeConfiguration() * * @param remb the bitrate of the REMB packet received. */ - public void receivedREMB(long remb) + //@Override + public void bandwidthEstimationChanged(long remb) { + logger.warn("XXX new bw estimate: " + remb); BitrateAdaptor bitrateAdaptor = getOrCreateBitrateAdaptor(); if (bitrateAdaptor == null) { // A bitrate adaptor is not set. It makes no sense to continue. return; } + logger.warn(hashCode()+" YYY new bw estimate: " + remb); long now = System.currentTimeMillis(); // The number of endpoints this channel is currently receiving - int receivingEndpointCount = channel.getReceivingEndpointCount(); + int receivingEndpointCount = 0;//channel.getReceivingEndpointCount(); if (firstRemb == -1) firstRemb = now; diff --git a/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java b/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java index c13b09dad9..61d0a028f7 100644 --- a/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java +++ b/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java @@ -102,6 +102,7 @@ public VideoChannelLastNAdaptor(BitrateController bitrateController) { this.bitrateController = bitrateController; + /* if (bitrateController.getChannel().getAdaptiveSimulcast()) { this.slaveSimulcastAdaptor @@ -109,6 +110,7 @@ public VideoChannelLastNAdaptor(BitrateController bitrateController) } this.initializeConfiguration(); + */ } @Override @@ -261,11 +263,13 @@ private int setInitialLastN(int lastN) Endpoint thisEndpoint = channel.getEndpoint(); int endpointCount = 0; + /* for (Endpoint endpoint : channel.getLastNEndpoints()) { if (endpoint != null && !endpoint.equals(thisEndpoint)) endpointCount += 1; } + */ /* * We update lastN if either: From 11a280216dbadb121406f1fcedc42052b59bc738 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Tue, 15 Dec 2015 13:52:02 -0600 Subject: [PATCH 02/13] Handles initialization properly. --- .../jitsi/videobridge/LastNController.java | 33 ++++++++++++++++--- .../org/jitsi/videobridge/RtpChannel.java | 8 +++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index 2ec23d0362..30962ed029 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -21,25 +21,29 @@ public class LastNController */ private static final Logger logger = Logger.getLogger(VideoChannel.class); + /** + * An empty list instance. + */ + private static final List INITIAL_EMPTY_LIST + = Collections.unmodifiableList(new LinkedList()); + /** * The set of Endpoints whose video streams are currently being * forwarded. */ - private List forwardedEndpoints - = Collections.unmodifiableList(new LinkedList()); + private List forwardedEndpoints = INITIAL_EMPTY_LIST; /** * The list of all Endpoints in the conference, ordered by the * last time they were elected dominant speaker. */ - private List conferenceSpeechActivityEndpoints = new LinkedList<>(); + private List conferenceSpeechActivityEndpoints = INITIAL_EMPTY_LIST; /** * The list of endpoints which have been explicitly marked as 'pinned' * and whose video streams should always be forwarded. */ - private List pinnedEndpoints - = Collections.unmodifiableList(new LinkedList()); + private List pinnedEndpoints = INITIAL_EMPTY_LIST; /** * The maximum number of endpoints whose video streams will be forwarded @@ -173,9 +177,17 @@ public boolean isForwarded(Channel sourceChannel) Endpoint channelEndpoint = sourceChannel.getEndpoint(); if (channelEndpoint == null) { + logger.warn("sourceChannel has no endpoint."); return false; } + if (forwardedEndpoints == INITIAL_EMPTY_LIST) + { + // LastN is enabled, but we haven't yet initialized the list of + // endpoints in the conference. + initializeConferenceEndpoints(); + } + return forwardedEndpoints.contains(channelEndpoint.getID()); } @@ -402,4 +414,15 @@ private String getEndpointId() } return endpointId; } + + /** + * Initializes the local list of endpoints + * ({@link #speechActivityEndpointsChanged(List)}) with the current + * endpoints from the conference. + */ + private synchronized void initializeConferenceEndpoints() + { + speechActivityEndpointsChanged( + channel.getConferenceSpeechActivity().getEndpoints()); + } } diff --git a/src/main/java/org/jitsi/videobridge/RtpChannel.java b/src/main/java/org/jitsi/videobridge/RtpChannel.java index 24b26c8b8a..3dfe9136b0 100644 --- a/src/main/java/org/jitsi/videobridge/RtpChannel.java +++ b/src/main/java/org/jitsi/videobridge/RtpChannel.java @@ -1890,4 +1890,12 @@ public long getFidPairedSsrc(long ssrc) return -1; } + + /** + * @return the {@link ConferenceSpeechActivity} for this channel. + */ + public ConferenceSpeechActivity getConferenceSpeechActivity() + { + return conferenceSpeechActivity; + } } From 73c54d51acfde221a9a630f9bee906ba9a042e59 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Tue, 15 Dec 2015 14:47:19 -0600 Subject: [PATCH 03/13] Adds log messages. --- .../jitsi/videobridge/LastNController.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index 30962ed029..0cbf840017 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -109,6 +109,11 @@ public List getForwardedEndpoints() */ public void setLastN(int lastN) { + if (logger.isDebugEnabled()) + { + logger.debug("Setting lastN=" + lastN); + } + List endpointsToAskForKeyframe; synchronized (this) { @@ -138,6 +143,12 @@ public void setPinnedEndpoints( newPinnedEndpointIds.add(endpoint.getID()); } + if (logger.isDebugEnabled()) + { + logger.debug("Setting pinned endpoints: " + + newPinnedEndpointIds.toString()); + } + List endpointsToAskForKeyframe; synchronized (this) { @@ -188,6 +199,10 @@ public boolean isForwarded(Channel sourceChannel) initializeConferenceEndpoints(); } + // This may look like a place to optimize, because we query an unordered + // list (in O(n)) and it executes on each video packet if lastN is + // enabled. However, the size of forwardedEndpoints is restricted to + // lastN and so small enough that it is not worth optimizing. return forwardedEndpoints.contains(channelEndpoint.getID()); } @@ -227,6 +242,15 @@ public List speechActivityEndpointsChanged( List enteringEndpointIds = speechActivityEndpointIdsChanged(newEndpointIdList); + if (logger.isDebugEnabled()) + { + logger.debug( + "New list of conference endpoints: " + + newEndpointIdList.toString() + "; entering endpoints: " + + (enteringEndpointIds == null ? "none" : + enteringEndpointIds.toString())); + } + List ret = new LinkedList<>(); if (enteringEndpointIds != null) { @@ -274,6 +298,10 @@ private synchronized List speechActivityEndpointIdsChanged( if (!change) { + if (logger.isDebugEnabled()) + { + logger.debug("Conference endpoints have not changed."); + } return null; } @@ -382,6 +410,15 @@ else if (newForwardedEndpoints.size() < lastN) List enteringEndpoints = new ArrayList<>(newForwardedEndpoints); enteringEndpoints.removeAll(forwardedEndpoints); + if (logger.isDebugEnabled()) + { + logger.debug( + "Forwarded endpoints (maybe) changed: " + + forwardedEndpoints.toString() + " -> " + + newForwardedEndpoints.toString() + + ". Entering: " + enteringEndpoints.toString()); + } + forwardedEndpoints = Collections.unmodifiableList(newForwardedEndpoints); @@ -396,6 +433,7 @@ else if (newForwardedEndpoints.size() < lastN) */ private void askForKeyframes(List endpointIds) { + // TODO: Execute asynchronously. channel.getContent().askForKeyframesById(endpointIds); } @@ -424,5 +462,11 @@ private synchronized void initializeConferenceEndpoints() { speechActivityEndpointsChanged( channel.getConferenceSpeechActivity().getEndpoints()); + + if (logger.isDebugEnabled()) + { + logger.debug("Initialized the list of endpoints: " + + conferenceSpeechActivityEndpoints.toString()); + } } } From 3d8c709ab51c5e0c24fbf3785e565076cb34af40 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Wed, 16 Dec 2015 11:08:04 -0600 Subject: [PATCH 04/13] Supports a list of pinned endpoints. --- .../java/org/jitsi/videobridge/Endpoint.java | 205 +++++++++++------- .../jitsi/videobridge/LastNController.java | 12 +- .../org/jitsi/videobridge/VideoChannel.java | 5 +- .../simulcast/SimulcastSender.java | 111 ++++++---- 4 files changed, 201 insertions(+), 132 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/Endpoint.java b/src/main/java/org/jitsi/videobridge/Endpoint.java index 1b6d8aab94..253a69a140 100644 --- a/src/main/java/org/jitsi/videobridge/Endpoint.java +++ b/src/main/java/org/jitsi/videobridge/Endpoint.java @@ -55,8 +55,8 @@ public class Endpoint * specifies the JID of the currently pinned Endpoint of this * Endpoint. */ - public static final String PINNED_ENDPOINT_PROPERTY_NAME - = Endpoint.class.getName() + ".pinnedEndpoint"; + public static final String PINNED_ENDPOINTS_PROPERTY_NAME + = Endpoint.class.getName() + ".pinnedEndpoints"; /** * The name of the Endpoint property sctpConnection which @@ -74,6 +74,41 @@ public class Endpoint public static final String SELECTED_ENDPOINT_PROPERTY_NAME = Endpoint.class.getName() + ".selectedEndpoint"; + /** + * The {@link Videobridge#COLIBRI_CLASS} value indicating a + * {@code SelectedEndpointChangedEvent}. + */ + private static final String COLIBRI_CLASS_SELECTED_ENDPOINT_CHANGED + = "SelectedEndpointChangedEvent"; + + /** + * The {@link Videobridge#COLIBRI_CLASS} value indicating a + * {@code PinnedEndpointChangedEvent}. + */ + private static final String COLIBRI_CLASS_PINNED_ENDPOINT_CHANGED + = "PinnedEndpointChangedEvent"; + + /** + * The {@link Videobridge#COLIBRI_CLASS} value indicating a + * {@code PinnedEndpointsChangedEvent}. + */ + private static final String COLIBRI_CLASS_PINNED_ENDPOINTS_CHANGED + = "PinnedEndpointsChangedEvent"; + + /** + * The {@link Videobridge#COLIBRI_CLASS} value indicating a + * {@code ClientHello} message. + */ + private static final String COLIBRI_CLASS_CLIENT_HELLO + = "ClientHello"; + + /** + * The {@link Videobridge#COLIBRI_CLASS} value indicating a + * {@code EndpointMessage}. + */ + private static final String COLIBRI_CLASS_ENDPOINT_MESSAGE + = "EndpointMessage"; + /** * The list of Channels associated with this Endpoint. */ @@ -119,10 +154,9 @@ public class Endpoint private final WeakReference weakConference; /** - * A weak reference to the currently pinned Endpoint at this - * Endpoint. + * The list of IDs of the pinned endpoints of this {@code endpoint}. */ - private WeakReference weakPinnedEndpoint; + private List pinnedEndpoints = new LinkedList<>(); /** * A weak reference to the currently selected Endpoint at this @@ -310,29 +344,6 @@ public SctpConnection getSctpConnection() return sctpConnection.get(); } - /** - * Gets the currently effectively selected Endpoint at this - * Endpoint. - * - * @return the currently effectively selected Endpoint at this - * Endpoint. - */ - public Endpoint getEffectivelySelectedEndpoint() - { - Endpoint pinned = getPinnedEndpoint(); - if (pinned == null) - { - WeakReference wr = this.weakSelectedEndpoint; - Endpoint e = wr == null ? null : wr.get(); - - return e == null || e.expired ? null : e; - } - else - { - return pinned; - } - } - /** * Gets the currently selected Endpoint at this Endpoint. * @@ -348,16 +359,12 @@ private Endpoint getSelectedEndpoint() } /** - * Gets the currently pinned Endpoint at this Endpoint. - * - * @return the currently pinned Endpoint at this Endpoint. + * @return the list of pinned endpoints, represented as a list of endpoint + * IDs. */ - public Endpoint getPinnedEndpoint() + public List getPinnedEndpoints() { - WeakReference wr = weakPinnedEndpoint; - Endpoint e = (wr == null) ? null : wr.get(); - - return e == null || e.expired ? null : e; + return pinnedEndpoints; } /** @@ -425,22 +432,24 @@ private void onJSONData( JSONObject jsonObject, Object colibriClass) { - if ("SelectedEndpointChangedEvent".equals(colibriClass)) + if (COLIBRI_CLASS_SELECTED_ENDPOINT_CHANGED.equals(colibriClass)) onSelectedEndpointChangedEvent(src, jsonObject); - else if ("PinnedEndpointChangedEvent".equals(colibriClass)) + else if (COLIBRI_CLASS_PINNED_ENDPOINT_CHANGED.equals(colibriClass)) onPinnedEndpointChangedEvent(src, jsonObject); - else if ("ClientHello".equals(colibriClass)) + else if (COLIBRI_CLASS_PINNED_ENDPOINTS_CHANGED.equals(colibriClass)) + onPinnedEndpointsChangedEvent(src, jsonObject); + else if (COLIBRI_CLASS_CLIENT_HELLO.equals(colibriClass)) onClientHello(src, jsonObject); - else if ("EndpointMessage".equals(colibriClass)) + else if (COLIBRI_CLASS_ENDPOINT_MESSAGE.equals(colibriClass)) onClientEndpointMessage(src, jsonObject); } /** - * Handles an opaque essage from this {@code Endpoint} that should be + * Handles an opaque message from this {@code Endpoint} that should be * forwarded to either: a) another client in this conference (1:1 * message) or b) all other clients in this conference (broadcast message) * - * @param src the {@WebRtcDataStream) by which {@code jsonObject} has + * @param src the {@link WebRtcDataStream) by which {@code jsonObject} has * been received * @param jsonObject the JSON object with * {@link Videobridge#COLIBRI_CLASS} EndpointMessage which has been @@ -470,13 +479,14 @@ private void onClientEndpointMessage( JSONObject jsonObject) { String to = (String)jsonObject.get("to"); - String msgPayload = ((JSONObject)jsonObject.get("msgPayload")).toString(); + String msgPayload = jsonObject.get("msgPayload").toString(); Conference conf = getConference(); if ("".equals(to)) { // Broadcast message List endpointSubset = new ArrayList<>(); - for (Endpoint endpoint : conf.getEndpoints()) { + for (Endpoint endpoint : conf.getEndpoints()) + { if (!endpoint.getID().equalsIgnoreCase(getID())) { endpointSubset.add(endpoint); @@ -496,7 +506,9 @@ private void onClientEndpointMessage( } else { - logger.warn("Unable to find endpoint " + to + " to send EndpointMessage"); + logger.warn( + "Unable to find endpoint " + to + + " to send EndpointMessage"); } } } @@ -525,59 +537,92 @@ private void onPinnedEndpointChangedEvent( sc.bind("this", this); logger.debug(sc.c( "Endpoint {this.id} notified us that it has pinned" - + " {pinnedId}.")); + + " {pinnedId}.")); } - Conference conference = weakConference.get(); + pinnedEndpointsChanged(Collections.singletonList(newPinnedEndpointID)); + } - Endpoint newPinnedEndpoint; - if (!StringUtils.isNullOrEmpty(newPinnedEndpointID) - && conference != null) + /** + * Notifies this {@code Endpoint} that a {@code PinnedEndpointsChangedEvent} + * has been received by the associated {@code SctpConnection}. + * + * @param src the {@code WebRtcDataStream} by which {@code jsonObject} has + * been received + * @param jsonObject the JSON object with {@link Videobridge#COLIBRI_CLASS} + * {@code PinnedEndpointChangedEvent} which has been received by the + * associated {@code SctpConnection} + */ + private void onPinnedEndpointsChangedEvent( + WebRtcDataStream src, + JSONObject jsonObject) + { + // Find the new pinned endpoint. + Object o = jsonObject.get("pinnedEndpoints"); + if (!(o instanceof JSONArray)) { - newPinnedEndpoint = conference.getEndpoint(newPinnedEndpointID); + logger.warn("Received invalid or unexpected JSON: " + jsonObject); + return; } - else + + JSONArray jsonArray = (JSONArray) o; + List newPinnedEndpoints = new LinkedList<>(); + for (Object endpointId : jsonArray) + { + if (endpointId != null && endpointId instanceof String) + { + newPinnedEndpoints.add((String)endpointId); + } + } + + if (logger.isDebugEnabled()) { - newPinnedEndpoint = null; + StringCompiler sc = new StringCompiler(); + sc.bind("pinned", newPinnedEndpoints); + sc.bind("this", this); + logger.debug(sc.c( + "Endpoint {this.id} notified us that it has pinned" + + " {pinned}.")); } - // Check if that's different to what we think the pinned endpoint is. + pinnedEndpointsChanged(newPinnedEndpoints); + } + + private void pinnedEndpointsChanged(List pinnedEndpoints) + { + // Check if that's different to what we think the pinned endpoints are. boolean changed; - Endpoint oldPinnedEndpoint = this.getPinnedEndpoint(); synchronized (pinnedEndpointSyncRoot) { - changed = newPinnedEndpoint != oldPinnedEndpoint; - if (changed) + changed = pinnedEndpoints.size() != this.pinnedEndpoints.size(); + if (!changed) { - if (newPinnedEndpoint == null) - { - this.weakPinnedEndpoint = null; - } - else + for (int i = 0; i < pinnedEndpoints.size(); i++) { - this.weakPinnedEndpoint - = new WeakReference<>(newPinnedEndpoint); + if (!pinnedEndpoints.get(i). + equals(this.pinnedEndpoints.get(i))) + { + changed = true; + break; + } } } - } - // NOTE(gp) This won't guarantee that property change events are fired - // in the correct order. We should probably call the - // firePropertyChange() method from inside the synchronized _and_ the - // underlying PropertyChangeNotifier should have a dedicated events - // queue and a thread for firing PropertyChangeEvents from the queue. - - if (changed) - { - if (logger.isDebugEnabled()) + if (changed) { - StringCompiler sc = new StringCompiler(); - sc.bind("pinned", newPinnedEndpoint); - sc.bind("this", this); - logger.debug(sc.c("Endpoint {this.id} pinned {pinned.id}.")); + List oldPinnedEndpoints = this.pinnedEndpoints; + this.pinnedEndpoints = pinnedEndpoints; + + if (logger.isDebugEnabled()) + { + StringCompiler sc = new StringCompiler(); + sc.bind("pinned", pinnedEndpoints); + sc.bind("this", this); + logger.debug(sc.c("Endpoint {this.id} pinned {pinned}.")); + } + firePropertyChange(PINNED_ENDPOINTS_PROPERTY_NAME, + oldPinnedEndpoints, pinnedEndpoints); } - firePropertyChange(PINNED_ENDPOINT_PROPERTY_NAME, - oldPinnedEndpoint, newPinnedEndpoint); } } diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index 0cbf840017..ad565e322e 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -132,23 +132,15 @@ public void setLastN(int lastN) /** * Sets the list of "pinned" endpoints (i.e. endpoints for which video * should always be forwarded, regardless of {@code lastN}). - * @param newPinnedEndpoints the list of endpoints to set. + * @param newPinnedEndpointIds the list of endpoint IDs to set. */ - public void setPinnedEndpoints( - Collection newPinnedEndpoints) + public void setPinnedEndpointIds(List newPinnedEndpointIds) { - List newPinnedEndpointIds = new LinkedList<>(); - for (Endpoint endpoint : newPinnedEndpoints) - { - newPinnedEndpointIds.add(endpoint.getID()); - } - if (logger.isDebugEnabled()) { logger.debug("Setting pinned endpoints: " + newPinnedEndpointIds.toString()); } - List endpointsToAskForKeyframe; synchronized (this) { diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index a0e0083070..5b8b56d602 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -454,10 +454,9 @@ public void propertyChange(PropertyChangeEvent ev) String propertyName = ev.getPropertyName(); - if (Endpoint.PINNED_ENDPOINT_PROPERTY_NAME.equals(propertyName)) + if (Endpoint.PINNED_ENDPOINTS_PROPERTY_NAME.equals(propertyName)) { - lastNController.setPinnedEndpoints( - Collections.singleton(getEndpoint().getPinnedEndpoint())); + lastNController.setPinnedEndpointIds((List)ev.getNewValue()); } else if (Content.CHANNEL_MODIFIED_PROPERTY_NAME.equals(propertyName)) { diff --git a/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java b/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java index ca18980bff..60c16bff2e 100644 --- a/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java +++ b/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java @@ -246,53 +246,36 @@ else if (SimulcastReceiver.SIMULCAST_LAYERS_PNAME.equals(propertyName)) // The simulcast streams of the peer have changed, (re)attach. receiveStreamsChanged(); } - else if (Endpoint.SELECTED_ENDPOINT_PROPERTY_NAME.equals(propertyName) - || Endpoint.PINNED_ENDPOINT_PROPERTY_NAME.equals(propertyName)) + else if (Endpoint.SELECTED_ENDPOINT_PROPERTY_NAME.equals(propertyName)) { - // Here we update the targetOrder value. - Endpoint oldEndpoint = (Endpoint) ev.getOldValue(); Endpoint newEndpoint = (Endpoint) ev.getNewValue(); + selectedEndpointChanged(oldEndpoint, newEndpoint); + } + else if(Endpoint.PINNED_ENDPOINTS_PROPERTY_NAME.equals(propertyName)) + { + // We handle the pinned endpoint in the same way as the selected + // endpoint. We assume that "the" pinned endpoint is the first + // endpoint in the list of endpoints. - if (newEndpoint == null) - { - logDebug("Now I'm not watching anybody. What?!"); - } - else - { - logDebug("Now I'm watching " + newEndpoint.getID()); - } + List oldEndpointIds = (List) ev.getOldValue(); + List newEndpointIds = (List) ev.getNewValue(); + String oldEndpointId + = (oldEndpointIds != null && !oldEndpointIds.isEmpty()) ? + oldEndpointIds.get(0) : null; - SimulcastReceiver simulcastReceiver = getSimulcastReceiver(); - if (simulcastReceiver == null) - { - logWarn("The simulcastReceiver has been garbage collected. " + - "This simulcastSender is now defunkt."); - return; - } + String newEndpointId + = (newEndpointIds != null && !newEndpointIds.isEmpty()) ? + newEndpointIds.get(0) : null; - SimulcastStream[] simStreams = simulcastReceiver.getSimulcastStreams(); - if (simStreams == null || simStreams.length == 0) - { - logWarn("The remote endpoint hasn't signaled simulcast. " + - "This simulcastSender is now disabled."); - return; - } + Conference conference + = simulcastSenderManager.getSimulcastEngine() + .getVideoChannel().getContent().getConference(); - int hqOrder = simStreams.length - 1; - if (newEndpoint == getSendEndpoint() && targetOrder != hqOrder) - { - targetOrder = hqOrder; - react(false); - } + selectedEndpointChanged( + conference.getEndpoint(oldEndpointId), + conference.getEndpoint(newEndpointId)); - // Send LQ stream for the previously selected endpoint. - if (oldEndpoint == getSendEndpoint() - && targetOrder != SimulcastStream.SIMULCAST_LAYER_ORDER_BASE) - { - targetOrder = SimulcastStream.SIMULCAST_LAYER_ORDER_BASE; - react(false); - } } else if (VideoChannel.SIMULCAST_MODE_PNAME.equals(propertyName)) { @@ -315,6 +298,56 @@ else if (VideoChannel.ENDPOINT_PROPERTY_NAME.equals(propertyName)) } } + /** + * Handles a change in the selected endpoint. + * @param oldEndpoint the old selected endpoint. + * @param newEndpoint the new selected endpoint. + */ + private void selectedEndpointChanged( + Endpoint oldEndpoint, Endpoint newEndpoint) + { + // Here we update the targetOrder value. + if (newEndpoint == null) + { + logDebug("Now I'm not watching anybody. What?!"); + } + else + { + logDebug("Now I'm watching " + newEndpoint.getID()); + } + + SimulcastReceiver simulcastReceiver = getSimulcastReceiver(); + if (simulcastReceiver == null) + { + logWarn("The simulcastReceiver has been garbage collected. " + + "This simulcastSender is now defunkt."); + return; + } + + SimulcastStream[] simStreams = simulcastReceiver.getSimulcastStreams(); + if (simStreams == null || simStreams.length == 0) + { + logWarn("The remote endpoint hasn't signaled simulcast. " + + "This simulcastSender is now disabled."); + return; + } + + int hqOrder = simStreams.length - 1; + if (newEndpoint == getSendEndpoint() && targetOrder != hqOrder) + { + targetOrder = hqOrder; + react(false); + } + + // Send LQ stream for the previously selected endpoint. + if (oldEndpoint == getSendEndpoint() + && targetOrder != SimulcastStream.SIMULCAST_LAYER_ORDER_BASE) + { + targetOrder = SimulcastStream.SIMULCAST_LAYER_ORDER_BASE; + react(false); + } + } + /** * Returns a boolean indicating whether the caller must drop, or accept, the * packet passed in as a parameter. From 248b713bf3b92b7e10d4776b1c1449860303b481 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Wed, 16 Dec 2015 13:37:17 -0600 Subject: [PATCH 05/13] Does not handle pinned endpoints in simulcast. --- .../simulcast/SimulcastSender.java | 25 ------------------- 1 file changed, 25 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java b/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java index 60c16bff2e..61e2cbe227 100644 --- a/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java +++ b/src/main/java/org/jitsi/videobridge/simulcast/SimulcastSender.java @@ -252,31 +252,6 @@ else if (Endpoint.SELECTED_ENDPOINT_PROPERTY_NAME.equals(propertyName)) Endpoint newEndpoint = (Endpoint) ev.getNewValue(); selectedEndpointChanged(oldEndpoint, newEndpoint); } - else if(Endpoint.PINNED_ENDPOINTS_PROPERTY_NAME.equals(propertyName)) - { - // We handle the pinned endpoint in the same way as the selected - // endpoint. We assume that "the" pinned endpoint is the first - // endpoint in the list of endpoints. - - List oldEndpointIds = (List) ev.getOldValue(); - List newEndpointIds = (List) ev.getNewValue(); - String oldEndpointId - = (oldEndpointIds != null && !oldEndpointIds.isEmpty()) ? - oldEndpointIds.get(0) : null; - - String newEndpointId - = (newEndpointIds != null && !newEndpointIds.isEmpty()) ? - newEndpointIds.get(0) : null; - - Conference conference - = simulcastSenderManager.getSimulcastEngine() - .getVideoChannel().getContent().getConference(); - - selectedEndpointChanged( - conference.getEndpoint(oldEndpointId), - conference.getEndpoint(newEndpointId)); - - } else if (VideoChannel.SIMULCAST_MODE_PNAME.equals(propertyName)) { logDebug("The simulcast mode has changed."); From b76e9221d63c412b836aee27de095cf3c5cae8fe Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Wed, 16 Dec 2015 14:50:02 -0600 Subject: [PATCH 06/13] Fixes the LastNController logger. --- src/main/java/org/jitsi/videobridge/LastNController.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index ad565e322e..a4bec4ece6 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -19,7 +19,8 @@ public class LastNController * The Logger used by the VideoChannel class and its * instances to print debug information. */ - private static final Logger logger = Logger.getLogger(VideoChannel.class); + private static final Logger logger + = Logger.getLogger(LastNController.class); /** * An empty list instance. From e931b8bc8b8d64fad55fdc5afd0b17a10e045d0f Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Wed, 16 Dec 2015 16:28:41 -0600 Subject: [PATCH 07/13] Sends last-N-related events via the DataChannel, avoids unnecessary updates, cleans up. --- .../jitsi/videobridge/LastNController.java | 92 ++++++++++--------- .../org/jitsi/videobridge/RtpChannel.java | 3 +- .../org/jitsi/videobridge/VideoChannel.java | 6 +- 3 files changed, 55 insertions(+), 46 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index a4bec4ece6..f7413be60c 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -115,16 +115,19 @@ public void setLastN(int lastN) logger.debug("Setting lastN=" + lastN); } - List endpointsToAskForKeyframe; + List endpointsToAskForKeyframe = null; synchronized (this) { // Since we have the lock anyway, call update() inside, so it // doesn't have to obtain it again. But keep the call to // askForKeyframes() outside. - this.lastN = lastN; + if (this.lastN != lastN) + { + this.lastN = lastN; - endpointsToAskForKeyframe = update(); + endpointsToAskForKeyframe = update(); + } } askForKeyframes(endpointsToAskForKeyframe); @@ -142,16 +145,19 @@ public void setPinnedEndpointIds(List newPinnedEndpointIds) logger.debug("Setting pinned endpoints: " + newPinnedEndpointIds.toString()); } - List endpointsToAskForKeyframe; + List endpointsToAskForKeyframe = null; synchronized (this) { // Since we have the lock anyway, call update() inside, so it // doesn't have to obtain it again. But keep the call to // askForKeyframes() outside. - this.pinnedEndpoints - = Collections.unmodifiableList(newPinnedEndpointIds); + if (!pinnedEndpoints.equals(newPinnedEndpointIds)) + { + pinnedEndpoints + = Collections.unmodifiableList(newPinnedEndpointIds); - endpointsToAskForKeyframe = update(); + endpointsToAskForKeyframe = update(); + } } askForKeyframes(endpointsToAskForKeyframe); @@ -221,7 +227,7 @@ public List getPinnedEndpoints() * * @param endpoints the new ordered list of endpoints in the conference. * @return the list of endpoints which were added to the list of forwarded - * endpoints as a result of the call. + * endpoints as a result of the call, or {@code null} if none were added. */ public List speechActivityEndpointsChanged( List endpoints) @@ -271,25 +277,7 @@ public List speechActivityEndpointsChanged( private synchronized List speechActivityEndpointIdsChanged( List endpointIds) { - boolean change = false; - if (endpointIds.size() != conferenceSpeechActivityEndpoints.size()) - { - change = true; - } - else - { - for (int i = 0; i < endpointIds.size(); i++) - { - if (!(conferenceSpeechActivityEndpoints.get(i) - .equals(endpointIds.get(i)))) - { - change = true; - break; - } - } - } - - if (!change) + if (conferenceSpeechActivityEndpoints.equals(endpointIds)) { if (logger.isDebugEnabled()) { @@ -297,10 +285,13 @@ private synchronized List speechActivityEndpointIdsChanged( } return null; } + else + { - conferenceSpeechActivityEndpoints = endpointIds; + conferenceSpeechActivityEndpoints = endpointIds; - return update(); + return update(); + } } /** @@ -349,7 +340,7 @@ public boolean getAdaptiveSimulcast() * * @return the list of IDs of endpoints which were added to * {@link #forwardedEndpoints} (i.e. of endpoints * "entering last-n") as a - * result of this call. + * result of this call. Returns {@code null} if no endpoints were added. */ private synchronized List update() { @@ -400,20 +391,36 @@ else if (newForwardedEndpoints.size() < lastN) } } - List enteringEndpoints = new ArrayList<>(newForwardedEndpoints); - enteringEndpoints.removeAll(forwardedEndpoints); + List enteringEndpoints; + if (forwardedEndpoints.equals(newForwardedEndpoints)) + { + // We want forwardedEndpoints != INITIAL_EMPTY_LIST + forwardedEndpoints = newForwardedEndpoints; - if (logger.isDebugEnabled()) + enteringEndpoints = null; + } + else { - logger.debug( - "Forwarded endpoints (maybe) changed: " - + forwardedEndpoints.toString() + " -> " - + newForwardedEndpoints.toString() - + ". Entering: " + enteringEndpoints.toString()); + enteringEndpoints = new ArrayList<>(newForwardedEndpoints); + enteringEndpoints.removeAll(forwardedEndpoints); + + if (logger.isDebugEnabled()) + { + logger.debug( + "Forwarded endpoints changed: " + + forwardedEndpoints.toString() + " -> " + + newForwardedEndpoints.toString() + + ". Entering: " + enteringEndpoints.toString()); + } + + forwardedEndpoints + = Collections.unmodifiableList(newForwardedEndpoints); + + // TODO: we may want to do this asynchronously. + channel.sendLastNEndpointsChangeEventOnDataChannel( + forwardedEndpoints, enteringEndpoints); } - forwardedEndpoints - = Collections.unmodifiableList(newForwardedEndpoints); return enteringEndpoints; } @@ -427,7 +434,10 @@ else if (newForwardedEndpoints.size() < lastN) private void askForKeyframes(List endpointIds) { // TODO: Execute asynchronously. - channel.getContent().askForKeyframesById(endpointIds); + if (endpointIds != null && !endpointIds.isEmpty()) + { + channel.getContent().askForKeyframesById(endpointIds); + } } /** diff --git a/src/main/java/org/jitsi/videobridge/RtpChannel.java b/src/main/java/org/jitsi/videobridge/RtpChannel.java index 3dfe9136b0..334f04d05e 100644 --- a/src/main/java/org/jitsi/videobridge/RtpChannel.java +++ b/src/main/java/org/jitsi/videobridge/RtpChannel.java @@ -1578,7 +1578,8 @@ else if (!this.rtpLevelRelayType.equals(rtpLevelRelayType)) * conferenceSpeechActivity * @return a list of the Endpoints which should be asked for * (video) keyframes because, for example, they are entering the set of - * lastN Endpoints of this Channel + * lastN Endpoints of this Channel, or + * {@code null} if there are no such endpoints. */ List speechActivityEndpointsChanged(List endpoints) { diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index 5b8b56d602..ecbb4b4640 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -520,7 +520,8 @@ void sctpConnectionReady(Endpoint endpoint) * @param endpointsEnteringLastN the Endpoints which are entering * the list of Endpoints defined by lastN */ - private void sendLastNEndpointsChangeEventOnDataChannel( + public void sendLastNEndpointsChangeEventOnDataChannel( + List forwardedEndpoints, List endpointsEnteringLastN) { Endpoint thisEndpoint = getEndpoint(); @@ -528,9 +529,6 @@ private void sendLastNEndpointsChangeEventOnDataChannel( if (thisEndpoint == null) return; - List forwardedEndpoints - = lastNController.getForwardedEndpoints(); - // We want endpointsEnteringLastN to always to reported. Consequently, // we will pretend that all lastNEndpoints are entering if no explicit // endpointsEnteringLastN is specified. From a8e9d52237c5e9d138890484aea2c4afe49aa65a Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Thu, 17 Dec 2015 13:33:26 -0600 Subject: [PATCH 08/13] Avoids unnecessary keyframe requests. Initializes the conference endpoint list. --- .../jitsi/videobridge/LastNController.java | 43 ++++++++++++++++--- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index f7413be60c..a31b1c3840 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -124,9 +124,16 @@ public void setLastN(int lastN) if (this.lastN != lastN) { + // If we're just now enabling lastN, we don't need to ask for + // keyframes as all streams were being forwarded already. + boolean update = this.lastN != -1; + this.lastN = lastN; - endpointsToAskForKeyframe = update(); + if (update) + { + endpointsToAskForKeyframe = update(); + } } } @@ -232,12 +239,7 @@ public List getPinnedEndpoints() public List speechActivityEndpointsChanged( List endpoints) { - List newEndpointIdList = new LinkedList<>(); - for (Endpoint endpoint : endpoints) - { - newEndpointIdList.add(endpoint.getID()); - } - + List newEndpointIdList = getIDs(endpoints); List enteringEndpointIds = speechActivityEndpointIdsChanged(newEndpointIdList); @@ -347,6 +349,12 @@ private synchronized List update() List newForwardedEndpoints = new LinkedList<>(); String ourEndpointId = getEndpointId(); + if (conferenceSpeechActivityEndpoints == INITIAL_EMPTY_LIST) + { + conferenceSpeechActivityEndpoints + = getIDs(channel.getConferenceSpeechActivity().getEndpoints()); + } + if (lastN < 0) { // Last-N is disabled, we forward everything. @@ -421,6 +429,12 @@ else if (newForwardedEndpoints.size() < lastN) forwardedEndpoints, enteringEndpoints); } + // If lastN is disabled, the endpoints entering forwardedEndpoints were + // never filtered, so they don't need to be asked for keyframes. + if (lastN < 0) + { + enteringEndpoints = null; + } return enteringEndpoints; } @@ -472,4 +486,19 @@ private synchronized void initializeConferenceEndpoints() + conferenceSpeechActivityEndpoints.toString()); } } + + private List getIDs(List endpoints) + { + if (endpoints != null && !endpoints.isEmpty()) + { + List endpointIds = new LinkedList<>(); + for (Endpoint endpoint : endpoints) + { + endpointIds.add(endpoint.getID()); + } + return endpointIds; + } + + return null; + } } From fb7935a52ed89c4881621cf5128bae8cbf7a457e Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Thu, 17 Dec 2015 13:42:45 -0600 Subject: [PATCH 09/13] Avoids sending unnecessary keyframe requests. --- .../jitsi/videobridge/LastNController.java | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index a31b1c3840..f1659724c5 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -289,10 +289,12 @@ private synchronized List speechActivityEndpointIdsChanged( } else { + List newEndpoints = new LinkedList<>(endpointIds); + newEndpoints.removeAll(conferenceSpeechActivityEndpoints); conferenceSpeechActivityEndpoints = endpointIds; - return update(); + return update(newEndpoints); } } @@ -345,6 +347,25 @@ public boolean getAdaptiveSimulcast() * result of this call. Returns {@code null} if no endpoints were added. */ private synchronized List update() + { + return update(null); + } + + /** + * Recalculates the list of forwarded endpoints based on the current values + * of the various parameters of this instance ({@link #lastN}, + * {@link #conferenceSpeechActivityEndpoints}, {@link #pinnedEndpoints}). + * + * @param newConferenceEndpoints A list of endpoints which entered the + * conference since the last call to this method. They need not be asked + * for keyframes, because they were never filtered by this + * {@link #LastNController(VideoChannel)}. + * + * @return the list of IDs of endpoints which were added to + * {@link #forwardedEndpoints} (i.e. of endpoints * "entering last-n") as a + * result of this call. Returns {@code null} if no endpoints were added. + */ + private synchronized List update(List newConferenceEndpoints) { List newForwardedEndpoints = new LinkedList<>(); String ourEndpointId = getEndpointId(); @@ -353,6 +374,7 @@ private synchronized List update() { conferenceSpeechActivityEndpoints = getIDs(channel.getConferenceSpeechActivity().getEndpoints()); + newConferenceEndpoints = conferenceSpeechActivityEndpoints; } if (lastN < 0) @@ -436,6 +458,13 @@ else if (newForwardedEndpoints.size() < lastN) enteringEndpoints = null; } + if (enteringEndpoints != null && newConferenceEndpoints != null) + { + // Endpoints just entering the conference need not be asked for + // keyframes. + enteringEndpoints.removeAll(newConferenceEndpoints); + } + return enteringEndpoints; } @@ -487,6 +516,11 @@ private synchronized void initializeConferenceEndpoints() } } + /** + * Extracts a list of endpoint IDs from a list of {@link Endpoint}s. + * @param endpoints the list of {@link Endpoint}s. + * @return the list of IDs of endpoints in {@code endpoints}. + */ private List getIDs(List endpoints) { if (endpoints != null && !endpoints.isEmpty()) From 2113727ea1dc4696ab833b92ec6b01b4355aca5a Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Thu, 7 Jan 2016 21:18:14 +0200 Subject: [PATCH 10/13] Sends the list of lastNEndpoints when the data channel is opened. --- .../java/org/jitsi/videobridge/LastNController.java | 2 +- src/main/java/org/jitsi/videobridge/VideoChannel.java | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index f1659724c5..b9bace81c7 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -504,7 +504,7 @@ private String getEndpointId() * ({@link #speechActivityEndpointsChanged(List)}) with the current * endpoints from the conference. */ - private synchronized void initializeConferenceEndpoints() + public synchronized void initializeConferenceEndpoints() { speechActivityEndpointsChanged( channel.getConferenceSpeechActivity().getEndpoints()); diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index c9dad95131..1c425b49e8 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -512,9 +512,14 @@ void sctpConnectionReady(Endpoint endpoint) if (endpoint.equals(getEndpoint())) { - // TODO: - // 1. Send list of endpoints to the channel - // 2. updateInLastN(this) + if (lastNController.getLastN() >= 0) + { + lastNController.initializeConferenceEndpoints(); + sendLastNEndpointsChangeEventOnDataChannel( + lastNController.getForwardedEndpoints(), null); + } + + updateInLastN(this); } } From f0a3d4dfdbb164c5027ae5ce6a5e73ef39787555 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Thu, 7 Jan 2016 23:17:06 +0200 Subject: [PATCH 11/13] Hooks up adaptive last-n and simulcast to the new lastN implementation. --- .../jitsi/videobridge/LastNController.java | 44 ++++++++++++++-- .../org/jitsi/videobridge/VideoChannel.java | 43 +++++++--------- .../ratecontrol/BitrateController.java | 51 +++++++++++-------- .../ratecontrol/VideoChannelLastNAdaptor.java | 4 +- 4 files changed, 90 insertions(+), 52 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/LastNController.java b/src/main/java/org/jitsi/videobridge/LastNController.java index b9bace81c7..fc1a0c8245 100644 --- a/src/main/java/org/jitsi/videobridge/LastNController.java +++ b/src/main/java/org/jitsi/videobridge/LastNController.java @@ -1,6 +1,22 @@ +/* + * Copyright @ 2015 Atlassian Pty Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.jitsi.videobridge; import org.jitsi.util.*; +import org.jitsi.videobridge.ratecontrol.*; import java.util.*; @@ -63,6 +79,12 @@ public class LastNController */ private boolean adaptiveSimulcast = false; + /** + * The instance which implements Adaptive LastN or + * Adaptive Simulcast on our behalf. + */ + private BitrateController bitrateController = null; + /** * The {@link VideoChannel} which owns this {@link LastNController}. */ @@ -305,8 +327,15 @@ private synchronized List speechActivityEndpointIdsChanged( */ public void setAdaptiveLastN(boolean adaptiveLastN) { - this.adaptiveLastN = adaptiveLastN; - // TODO: actually enable/disable + if (this.adaptiveLastN != adaptiveLastN) + { + if (adaptiveLastN && bitrateController == null) + { + bitrateController = new BitrateController(this, channel); + } + + this.adaptiveLastN = adaptiveLastN; + } } /** @@ -317,8 +346,15 @@ public void setAdaptiveLastN(boolean adaptiveLastN) */ public void setAdaptiveSimulcast(boolean adaptiveSimulcast) { - this.adaptiveSimulcast = adaptiveSimulcast; - // TODO: actually enable/disable + if (this.adaptiveSimulcast != adaptiveSimulcast) + { + if (adaptiveSimulcast && bitrateController == null) + { + bitrateController = new BitrateController(this, channel); + } + + this.adaptiveSimulcast = adaptiveSimulcast; + } } /** diff --git a/src/main/java/org/jitsi/videobridge/VideoChannel.java b/src/main/java/org/jitsi/videobridge/VideoChannel.java index 1c425b49e8..3207d97df6 100644 --- a/src/main/java/org/jitsi/videobridge/VideoChannel.java +++ b/src/main/java/org/jitsi/videobridge/VideoChannel.java @@ -37,7 +37,6 @@ import org.jitsi.service.neomedia.format.*; import org.jitsi.service.neomedia.rtp.*; import org.jitsi.util.*; -import org.jitsi.videobridge.ratecontrol.*; import org.jitsi.videobridge.rtcp.*; import org.jitsi.videobridge.simulcast.*; import org.jitsi.videobridge.transform.*; @@ -130,12 +129,6 @@ else if (t instanceof ThreadDeath) */ private SimulcastMode simulcastMode; - /** - * The BitrateController which will be controlling the - * value of bitrate for this VideoChannel. - */ - private BitrateController bitrateController; - /** * The instance which controls which endpoints' video streams are to be * forwarded on this {@link VideoChannel} (i.e. implements last-n and its @@ -279,9 +272,6 @@ public void initialize() stream.getMediaStreamStats().addNackListener(this); } - - // FIXME: this shouldn't depend on cfg, and shouldn't happen here. - getBitrateController(); } /** @@ -352,21 +342,6 @@ public void describe(ColibriConferenceIQ.ChannelCommon commonIq) iq.setSimulcastMode(getSimulcastMode()); } - /** - * Returns the BitrateController for this VideoChannel, - * creating it if necessary. - * TODO: move to LastNController - * - * @return the VideoChannelLastNAdaptor for this - * VideoChannel, creating it if necessary. - */ - private BitrateController getBitrateController() - { - if (bitrateController == null) - bitrateController = new BitrateController(this); - return bitrateController; - } - /** * Returns the current incoming bitrate in bits per second for this * VideoChannel (computed as the average bitrate over the last @@ -1152,4 +1127,22 @@ public void updateTranslatedVideoChannel(VideoChannel peerVideoChannel) // FIXME Force NACK termination. Postponing because this will require a // few changes here and there, and it's enabled by default anyway. } + + /** + * {@inheritDoc} + */ + @Override + public void setAdaptiveLastN(boolean adaptiveLastN) + { + lastNController.setAdaptiveLastN(adaptiveLastN); + } + + /** + * {@inheritDoc} + */ + @Override + public void setAdaptiveSimulcast(boolean adaptiveSimulcast) + { + lastNController.setAdaptiveSimulcast(adaptiveSimulcast); + } } diff --git a/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java b/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java index 6fc0e83baf..84ed34e79f 100644 --- a/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java +++ b/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java @@ -21,29 +21,28 @@ import org.jitsi.service.configuration.*; import org.jitsi.service.neomedia.*; +import org.jitsi.service.neomedia.rtp.*; import org.jitsi.videobridge.*; import org.jitsi.videobridge.simulcast.*; /** - * Controls the bitrate of a specific VideoChannel. *

- * Gets notified of received RTCP REMB packets through - * {@link #receivedREMB(long)}. Based on this information (the estimation of - * the available bandwidth to the endpoint of the VideoChannel) and on - * the recent average bitrates coming from the other endpoints in the conference - * decides whether the bitrate of the channel should be changed. A specialized - * bitrate adaptor performs the change. + * Gets notified of changes to the estimation of the available bandwidth towards + * the remote endpoint through {@link #bandwidthEstimationChanged(long)}. Based + * on this information, and on the current average bitrates coming from the + * other endpoints in the conference decides whether the configuration of the + * channel (i.e. the set of forwarded endpoints) should be changed. *

*

* The specific logic used to make the decision is implemented and documented in - * {@link #receivedREMB(long)}. + * {@link #bandwidthEstimationChanged(long)}. *

* * @author Boris Grozev * @author George Politis */ public class BitrateController - //implements BandwidthEstimator.Listener + implements BandwidthEstimator.Listener { /** * Whether the values for the constants have been initialized or not. @@ -179,23 +178,32 @@ public class BitrateController private final ReceivedRembList receivedRembs = new ReceivedRembList(REMB_AVERAGE_INTERVAL_MS); + private final LastNController lastNController; + /** * Initializes a new BitrateController instance. * * @param channel the VideoChannel for which the new instance is to * serve. */ - public BitrateController(VideoChannel channel) + public BitrateController(LastNController lastNController, + VideoChannel channel) { this.channel = channel; - // FIXME - //BandwidthEstimator be = ((VideoMediaStream) channel.getStream()).getOrCreateBandwidthEstimator(); - //be.addListener(this); + this.lastNController = lastNController; initializeConfiguration(); + + // Create a bandwidth estimator and hook us up to changes to the + // estimation. + BandwidthEstimator be + = ((VideoMediaStream) channel.getStream()) + .getOrCreateBandwidthEstimator(); + be.addListener(this); + } - public int calcNumEndpointsThatFitIn() + int calcNumEndpointsThatFitIn() { final long now = System.currentTimeMillis(); @@ -245,7 +253,7 @@ public int calcNumEndpointsThatFitIn() * * @return the VideoChannel of this BitrateController. */ - public VideoChannel getChannel() + VideoChannel getChannel() { return channel; } @@ -300,16 +308,14 @@ private BitrateAdaptor getOrCreateBitrateAdaptor() { bitrateAdaptorSet = true; - /* - if (channel.getAdaptiveLastN()) + if (lastNController.getAdaptiveLastN()) { bitrateAdaptor = new VideoChannelLastNAdaptor(this); } - else if (channel.getAdaptiveSimulcast()) + else if (lastNController.getAdaptiveSimulcast()) { bitrateAdaptor = new SimulcastAdaptor(this); } - */ } return bitrateAdaptor; @@ -376,7 +382,7 @@ private void initializeConfiguration() * * @param remb the bitrate of the REMB packet received. */ - //@Override + @Override public void bandwidthEstimationChanged(long remb) { logger.warn("XXX new bw estimate: " + remb); @@ -449,6 +455,11 @@ else if (numEndpointsThatFitIn > receivingEndpointCount) } } + LastNController getLastNController() + { + return lastNController; + } + /** * Saves the received REMB values along with their time of reception and * allows getting the average value over a certain period. diff --git a/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java b/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java index 61d0a028f7..569fbae20b 100644 --- a/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java +++ b/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java @@ -102,15 +102,13 @@ public VideoChannelLastNAdaptor(BitrateController bitrateController) { this.bitrateController = bitrateController; - /* - if (bitrateController.getChannel().getAdaptiveSimulcast()) + if (bitrateController.getLastNController().getAdaptiveSimulcast()) { this.slaveSimulcastAdaptor = new SimulcastAdaptor(bitrateController); } this.initializeConfiguration(); - */ } @Override From 329ef7e02f3a165cda6cf397bd78a1a1fb08a1b8 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Tue, 12 Jan 2016 12:42:39 +0200 Subject: [PATCH 12/13] Finishes porting the adaptive last-n code (fixes issues reported by George Politis). --- .../ratecontrol/BitrateController.java | 155 +++++++++--------- .../ratecontrol/VideoChannelLastNAdaptor.java | 40 ++--- 2 files changed, 87 insertions(+), 108 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java b/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java index 84ed34e79f..497553ef9a 100644 --- a/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java +++ b/src/main/java/org/jitsi/videobridge/ratecontrol/BitrateController.java @@ -138,6 +138,57 @@ public class BitrateController private static final String REMB_MULT_CONSTANT_PNAME = BitrateController.class.getName() + ".REMB_MULT_CONSTANT"; + /** + * Initializes the constants used by this class from the configuration. + */ + private static void initializeConfiguration(ConfigurationService cfg) + { + synchronized (BitrateController.class) + { + if (configurationInitialized) + return; + configurationInitialized = true; + + + if (cfg != null) + { + INCREASE_LAG_MS + = cfg.getInt(INCREASE_LAG_MS_PNAME, INCREASE_LAG_MS); + INCREASE_LAG_MS + = cfg.getInt(DECREASE_LAG_MS_PNAME, DECREASE_LAG_MS); + INITIAL_INTERVAL_MS + = cfg.getInt( + INITIAL_INTERVAL_MS_PNAME, + INITIAL_INTERVAL_MS); + + String rembMultConstantStr + = cfg.getString(REMB_MULT_CONSTANT_PNAME, null); + + if (rembMultConstantStr != null) + { + try + { + REMB_MULT_CONSTANT + = Double.parseDouble(rembMultConstantStr); + } + catch (Exception e) + { + // Whatever, use the default + } + } + + REMB_AVERAGE_INTERVAL_MS + = cfg.getInt( + REMB_AVERAGE_INTERVAL_MS_PNAME, + REMB_AVERAGE_INTERVAL_MS); + MIN_ASSUMED_ENDPOINT_BITRATE_BPS + = cfg.getInt( + MIN_ASSUMED_ENDPOINT_BITRATE_BPS_PNAME, + MIN_ASSUMED_ENDPOINT_BITRATE_BPS); + } + } + } + /** * The BitrateAdaptor to use to adapt the bandwidth. */ @@ -192,7 +243,10 @@ public BitrateController(LastNController lastNController, this.channel = channel; this.lastNController = lastNController; - initializeConfiguration(); + initializeConfiguration( + ServiceUtils.getService( + channel.getBundleContext(), + ConfigurationService.class)); // Create a bandwidth estimator and hook us up to changes to the // estimation. @@ -205,31 +259,20 @@ public BitrateController(LastNController lastNController, int calcNumEndpointsThatFitIn() { - final long now = System.currentTimeMillis(); - - // Our estimate of the available bandwidth is the average of all - // REMBs received in the last REMB_AVERAGE_INTERVAL_MS milliseconds. We - // do this in order to reduce the fluctuations, because REMBs often - // change very rapidly and we want to avoid changing lastN often. - // Multiplying with a constant is an experimental option. - final long availableBandwidth - = (long) (receivedRembs.getAverage(now) * REMB_MULT_CONSTANT); + final long availableBandwidth = receivedRembs.getLast(); + //= (long) (receivedRembs.getAverage(now) * REMB_MULT_CONSTANT); + long remainingBandwidth = availableBandwidth; + int numEndpointsThatFitIn = 0; + Conference conference = channel.getContent().getConference(); // Calculate the biggest number K, such that there are at least K other // endpoints in the conference, and the cumulative bitrate of the first // K endpoints does not exceed the available bandwidth estimate. - - long remainingBandwidth = availableBandwidth; - int numEndpointsThatFitIn = 0; - - final Iterator it = null; //channel.getReceivingEndpoints(); - final Endpoint thisEndpoint = channel.getEndpoint(); - - while (it.hasNext()) + for (String endpointId : lastNController.getForwardedEndpoints()) { - Endpoint endpoint = it.next(); + Endpoint endpoint = conference.getEndpoint(endpointId); - if (endpoint != null && !endpoint.equals(thisEndpoint)) + if (endpoint != null) { long endpointBitrate = getEndpointBitrate(endpoint); @@ -261,8 +304,8 @@ VideoChannel getChannel() /** * Returns the incoming bitrate in bits per second from all * VideoChannels of the endpoint endpoint or - * {@link #MIN_ASSUMED_ENDPOINT_BITRATE_BPS} if the actual bitrate is that - * limit. + * {@link #MIN_ASSUMED_ENDPOINT_BITRATE_BPS} if the actual bitrate is below + * that limit. * * @param endpoint the endpoint. * @return the incoming bitrate in bits per second from endpoint, @@ -320,61 +363,6 @@ else if (lastNController.getAdaptiveSimulcast()) return bitrateAdaptor; } - /** - * Initializes the constants used by this class from the configuration. - */ - private void initializeConfiguration() - { - synchronized (BitrateController.class) - { - if (configurationInitialized) - return; - configurationInitialized = true; - - ConfigurationService cfg - = ServiceUtils.getService( - channel.getBundleContext(), - ConfigurationService.class); - - if (cfg != null) - { - INCREASE_LAG_MS - = cfg.getInt(INCREASE_LAG_MS_PNAME, INCREASE_LAG_MS); - INCREASE_LAG_MS - = cfg.getInt(DECREASE_LAG_MS_PNAME, DECREASE_LAG_MS); - INITIAL_INTERVAL_MS - = cfg.getInt( - INITIAL_INTERVAL_MS_PNAME, - INITIAL_INTERVAL_MS); - - String rembMultConstantStr - = cfg.getString(REMB_MULT_CONSTANT_PNAME, null); - - if (rembMultConstantStr != null) - { - try - { - REMB_MULT_CONSTANT - = Double.parseDouble(rembMultConstantStr); - } - catch (Exception e) - { - // Whatever, use the default - } - } - - REMB_AVERAGE_INTERVAL_MS - = cfg.getInt( - REMB_AVERAGE_INTERVAL_MS_PNAME, - REMB_AVERAGE_INTERVAL_MS); - MIN_ASSUMED_ENDPOINT_BITRATE_BPS - = cfg.getInt( - MIN_ASSUMED_ENDPOINT_BITRATE_BPS_PNAME, - MIN_ASSUMED_ENDPOINT_BITRATE_BPS); - } - } - } - /** * Notifies this instance that an RTCP REMB packet with a bitrate value of @@ -385,19 +373,18 @@ private void initializeConfiguration() @Override public void bandwidthEstimationChanged(long remb) { - logger.warn("XXX new bw estimate: " + remb); BitrateAdaptor bitrateAdaptor = getOrCreateBitrateAdaptor(); if (bitrateAdaptor == null) { // A bitrate adaptor is not set. It makes no sense to continue. return; } - logger.warn(hashCode()+" YYY new bw estimate: " + remb); long now = System.currentTimeMillis(); // The number of endpoints this channel is currently receiving - int receivingEndpointCount = 0;//channel.getReceivingEndpointCount(); + int receivingEndpointCount + = lastNController.getForwardedEndpoints().size(); if (firstRemb == -1) firstRemb = now; @@ -485,6 +472,8 @@ private static class ReceivedRembList */ private long sum = 0; + private long last = -1; + /** * Used in {@link #clean(long)}. */ @@ -511,6 +500,7 @@ private void add(long time, long rate) { sum += rate; receivedRembs.put(time, rate); + last = rate; clean(time); } @@ -550,5 +540,10 @@ private long getAverage(long time) return (size == 0) ? 0 : (sum / size); } + + private long getLast() + { + return last; + } } } diff --git a/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java b/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java index 569fbae20b..ba50ba4955 100644 --- a/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java +++ b/src/main/java/org/jitsi/videobridge/ratecontrol/VideoChannelLastNAdaptor.java @@ -258,27 +258,18 @@ private void initializeConfiguration() private int setInitialLastN(int lastN) { VideoChannel channel = bitrateController.getChannel(); - Endpoint thisEndpoint = channel.getEndpoint(); - int endpointCount = 0; - - /* - for (Endpoint endpoint : channel.getLastNEndpoints()) - { - if (endpoint != null && !endpoint.equals(thisEndpoint)) - endpointCount += 1; - } - */ - - /* - * We update lastN if either: - * 1. It is currently disabled (-1) - * 2. It is currently more than the number of endpoints (because - * otherwise we detect this as a drop in the number of endpoint the - * channel can receive and we drop it aggressively) - * - * In the other cases (0 <= lastN <= endpointCount) we leave it as it is - * because it is a reasonable start point. - */ + int endpointCount + = bitrateController + .getLastNController().getForwardedEndpoints().size(); + + // We update lastN if either: + // 1. It is currently disabled (-1) + // 2. It is currently more than the number of endpoints (because + // otherwise we detect this as a drop in the number of endpoint the + // channel can receive and we drop it aggressively) + // + // In the other cases (0 <= lastN <= endpointCount) we leave it as it is + // because it is a reasonable start point. if (lastN < 0 || lastN > endpointCount) { lastN = endpointCount; @@ -301,13 +292,6 @@ public boolean touch() if (lastN > 0) lastNonZeroLastN = now; - // The ordered (by speech activity) list of endpoints currently in the - // conference. - // XXX Lyubomir Marinov: The method VideoChannel.getLastNEndpoints() - // never returns null. -// if (channel.getLastNEndpoints() == null) -// return false; - if (!initialLastNSet) { lastN = setInitialLastN(lastN); From b79f6848a32cb7d0486c030afef7647f5778929a Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Thu, 21 Jan 2016 09:49:07 -0600 Subject: [PATCH 13/13] Fixes un-pinning an endpoint (based on a fix by @lsevans). --- src/main/java/org/jitsi/videobridge/Endpoint.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jitsi/videobridge/Endpoint.java b/src/main/java/org/jitsi/videobridge/Endpoint.java index 253a69a140..fdf3cd38ca 100644 --- a/src/main/java/org/jitsi/videobridge/Endpoint.java +++ b/src/main/java/org/jitsi/videobridge/Endpoint.java @@ -540,7 +540,13 @@ private void onPinnedEndpointChangedEvent( + " {pinnedId}.")); } - pinnedEndpointsChanged(Collections.singletonList(newPinnedEndpointID)); + List newPinnedIDList = Collections.EMPTY_LIST; + if (newPinnedEndpointID == null || "".equals(newPinnedEndpointID)) + { + newPinnedIDList = Collections.singletonList(newPinnedEndpointID); + } + + pinnedEndpointsChanged(newPinnedIDList); } /**