Skip to content

Commit

Permalink
Fix BitrateControllerTest2 (#1766)
Browse files Browse the repository at this point in the history
* ref: Improve reading resource file.

* fix: Rename BitrateControllerTest2 to BitrateControllerTraceTest (correct test name).

* fix: Do not use static fields for bandwidth allocation config.

This makes testing harder because it's not possible to switch the
configuration. In particular, BitrateControllerTest and BitrateControllerTraceTest
need to run with different configuration and break when ran in the same
suite.
  • Loading branch information
bgrozev authored Nov 22, 2021
1 parent b5f3b42 commit 7fb44c1
Show file tree
Hide file tree
Showing 9 changed files with 261 additions and 271 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@
public class BandwidthAllocator<T extends MediaSourceContainer>
{
/**
* Returns a boolean that indicates whether or not the current bandwidth estimation (in bps) has changed above the
* Returns a boolean that indicates whether the current bandwidth estimation (in bps) has changed above the
* configured threshold with respect to the previous bandwidth estimation.
*
* @param previousBwe the previous bandwidth estimation (in bps).
* @param currentBwe the current bandwidth estimation (in bps).
* @return true if the bandwidth has changed above the configured threshold, * false otherwise.
*/
private static boolean bweChangeIsLargerThanThreshold(long previousBwe, long currentBwe)
private boolean bweChangeIsLargerThanThreshold(long previousBwe, long currentBwe)
{
if (previousBwe == -1 || currentBwe == -1)
{
Expand All @@ -65,7 +65,7 @@ private static boolean bweChangeIsLargerThanThreshold(long previousBwe, long cur
// In any case, there are other triggers for re-allocation, so any suppression we do here will only last up to
// a few seconds.
long deltaBwe = Math.abs(currentBwe - previousBwe);
return deltaBwe > previousBwe * BitrateControllerConfig.bweChangeThreshold();
return deltaBwe > previousBwe * config.bweChangeThreshold();

// If, on the other hand, the bwe has decreased, we require at least a 15% drop in order to update the bitrate
// allocation. This is an ugly hack to prevent too many resolution/UI changes in case the bridge produces too
Expand Down Expand Up @@ -107,10 +107,13 @@ private static boolean bweChangeIsLargerThanThreshold(long previousBwe, long cur
*/
private final Supplier<Boolean> trustBwe;

private final BitrateControllerConfig config = new BitrateControllerConfig();

/**
* The allocations settings signalled by the receiver.
*/
private AllocationSettings allocationSettings = new AllocationSettings();
private AllocationSettings allocationSettings
= new AllocationSettings(new VideoConstraints(config.thumbnailMaxHeightPx()));

/**
* The last time {@link BandwidthAllocator#update()} was called.
Expand Down Expand Up @@ -384,7 +387,8 @@ public boolean hasNonZeroEffectiveConstraints(String endpointId)
effectiveConstraints.get(endpoint.getId()),
allocationSettings.getOnStageEndpoints().contains(endpoint.getId()),
diagnosticContext,
clock));
clock,
config));
}
}

Expand All @@ -396,8 +400,7 @@ public boolean hasNonZeroEffectiveConstraints(String endpointId)
*/
void maybeUpdate()
{
if (Duration.between(lastUpdateTime, clock.instant())
.compareTo(BitrateControllerConfig.maxTimeBetweenCalculations()) > 0)
if (Duration.between(lastUpdateTime, clock.instant()).compareTo(config.maxTimeBetweenCalculations()) > 0)
{
logger.debug("Forcing an update");
TaskPools.CPU_POOL.execute(this::update);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,20 @@
package org.jitsi.videobridge.cc.allocation

import org.jitsi.utils.OrderedJsonObject
import org.jitsi.videobridge.cc.config.BitrateControllerConfig
import org.jitsi.videobridge.message.ReceiverVideoConstraintsMessage
import java.util.stream.Collectors
import kotlin.math.min
import org.jitsi.videobridge.cc.config.BitrateControllerConfig as config

/**
* This class encapsulates all of the client-controlled settings for bandwidth allocation.
*/
data class AllocationSettings(
data class AllocationSettings @JvmOverloads constructor(
val onStageEndpoints: List<String> = emptyList(),
val selectedEndpoints: List<String> = emptyList(),
val videoConstraints: Map<String, VideoConstraints> = emptyMap(),
val lastN: Int = -1,
val defaultConstraints: VideoConstraints = VideoConstraints(config.thumbnailMaxHeightPx())
val defaultConstraints: VideoConstraints
) {

fun toJson() = OrderedJsonObject().apply {
Expand Down Expand Up @@ -64,6 +64,8 @@ internal class AllocationSettingsWrapper {

private var videoConstraints: Map<String, VideoConstraints> = emptyMap()

private val config = BitrateControllerConfig()

private var defaultConstraints: VideoConstraints = VideoConstraints(config.thumbnailMaxHeightPx())

private var onStageEndpoints: List<String> = emptyList()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class BitrateController<T : MediaSourceContainer> @JvmOverloads constructor(
*/
private var forwardedEndpoints: Set<String> = emptySet()

private val config = BitrateControllerConfig()

/**
* Keep track of how much time we spend knowingly oversending (due to enableOnstageVideoSuspend being false)
*/
Expand Down Expand Up @@ -104,7 +106,7 @@ class BitrateController<T : MediaSourceContainer> @JvmOverloads constructor(
* TODO: Is this comment still accurate?
*/
private val trustBwe: Boolean
get() = BitrateControllerConfig.trustBwe() && supportsRtx && packetHandler.timeSinceFirstMedia() >= 10000
get() = config.trustBwe() && supportsRtx && packetHandler.timeSinceFirstMedia() >= 10000

// Proxy to the allocator
fun endpointOrderingChanged() = bandwidthAllocator.update()
Expand Down
Loading

0 comments on commit 7fb44c1

Please sign in to comment.