Add missing synchronization in ExperimentalBandwidthMeter

Issue: #612 PiperOrigin-RevId: 563690229
androidx · Sep 8, 2023 · dd2a373 · dd2a373
1 parent f2b1d0c
commit dd2a373
Show file tree

Hide file tree

Showing 5 changed files with 133 additions and 21 deletions.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -14,6 +14,8 @@
         ([#8699](https://github.com/google/ExoPlayer/issues/8699)).
     *   Add functionality to transmit Common Media Client Data (CMCD) data using
         query parameters ([#553](https://github.com/androidx/media/issues/553)).
+    *   Fix `ConcurrentModificationException` in `ExperimentalBandwidthMeter`
+        ([#612](https://github.com/androidx/media/issues/612)).
 *   Transformer:
     *   Changed `frameRate` and `durationUs` parameters of
         `SampleConsumer.queueInputBitmap` to `TimestampIterator`.

diff --git a/...ies/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeter.java b/...ies/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeter.java
@@ -17,6 +17,7 @@
 
 import android.content.Context;
 import android.os.Handler;
+import androidx.annotation.GuardedBy;
 import androidx.annotation.Nullable;
 import androidx.media3.common.C;
 import androidx.media3.common.util.Assertions;
@@ -285,20 +286,34 @@ public static synchronized DefaultBandwidthMeter getSingletonInstance(Context co
 
   private final ImmutableMap<Integer, Long> initialBitrateEstimates;
   private final EventDispatcher eventDispatcher;
-  private final SlidingPercentile slidingPercentile;
   private final Clock clock;
   private final boolean resetOnNetworkTypeChange;
 
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
+  private final SlidingPercentile slidingPercentile;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private int streamCount;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private long sampleStartTimeMs;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private long sampleBytesTransferred;
 
-  private @C.NetworkType int networkType;
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private long totalElapsedTimeMs;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private long totalBytesTransferred;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private long bitrateEstimate;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private long lastReportedBitrateEstimate;
 
+  private @C.NetworkType int networkType;
   private boolean networkTypeOverrideSet;
   private @C.NetworkType int networkTypeOverride;
 
@@ -445,6 +460,7 @@ private synchronized void onNetworkTypeChanged(@C.NetworkType int networkType) {
     slidingPercentile.reset();
   }
 
+  @GuardedBy("this")
   private void maybeNotifyBandwidthSample(
       int elapsedMs, long bytesTransferred, long bitrateEstimate) {
     if (elapsedMs == 0 && bytesTransferred == 0 && bitrateEstimate == lastReportedBitrateEstimate) {

diff --git a/...main/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeter.java b/...main/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeter.java
@@ -19,6 +19,7 @@
 
 import android.content.Context;
 import android.os.Handler;
+import androidx.annotation.GuardedBy;
 import androidx.annotation.Nullable;
 import androidx.media3.common.C;
 import androidx.media3.common.util.NetworkTypeObserver;
@@ -275,9 +276,13 @@ private static Map<Integer, Long> getInitialBitrateEstimatesForCountry(String co
   }
 
   private final ImmutableMap<Integer, Long> initialBitrateEstimates;
+  private final boolean resetOnNetworkTypeChange;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private final TimeToFirstByteEstimator timeToFirstByteEstimator;
+
+  @GuardedBy("this") // Used in TransferListener methods that are called on a background thread.
   private final BandwidthEstimator bandwidthEstimator;
-  private final boolean resetOnNetworkTypeChange;
 
   private @C.NetworkType int networkType;
   private long initialBitrateEstimate;
@@ -323,7 +328,7 @@ public synchronized long getBitrateEstimate() {
   }
 
   @Override
-  public long getTimeToFirstByteEstimateUs() {
+  public synchronized long getTimeToFirstByteEstimateUs() {
     return timeToFirstByteEstimator.getTimeToFirstByteEstimateUs();
   }
 
@@ -333,19 +338,20 @@ public TransferListener getTransferListener() {
   }
 
   @Override
-  public void addEventListener(Handler eventHandler, EventListener eventListener) {
+  public synchronized void addEventListener(Handler eventHandler, EventListener eventListener) {
     checkNotNull(eventHandler);
     checkNotNull(eventListener);
     bandwidthEstimator.addEventListener(eventHandler, eventListener);
   }
 
   @Override
-  public void removeEventListener(EventListener eventListener) {
+  public synchronized void removeEventListener(EventListener eventListener) {
     bandwidthEstimator.removeEventListener(eventListener);
   }
 
   @Override
-  public void onTransferInitializing(DataSource source, DataSpec dataSpec, boolean isNetwork) {
+  public synchronized void onTransferInitializing(
+      DataSource source, DataSpec dataSpec, boolean isNetwork) {
     if (!isTransferAtFullNetworkSpeed(dataSpec, isNetwork)) {
       return;
     }

diff --git a/...exoplayer/src/test/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeterTest.java b/...exoplayer/src/test/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeterTest.java
@@ -52,7 +52,6 @@
 @Config(sdk = Config.ALL_SDKS) // Test all SDKs because network detection logic changed over time.
 public final class DefaultBandwidthMeterTest {
 
-  private static final int SIMULATED_TRANSFER_COUNT = 100;
   private static final String FAST_COUNTRY_ISO = "TW";
   private static final String SLOW_COUNTRY_ISO = "PG";
 
@@ -668,19 +667,21 @@ public void networkTypeOverride_doesFullReset() {
         new DefaultBandwidthMeter.Builder(ApplicationProvider.getApplicationContext())
             .setClock(clock)
             .build();
-    long[] bitrateEstimatesWithNewInstance = simulateTransfers(bandwidthMeter, clock);
+    long[] bitrateEstimatesWithNewInstance =
+        simulateTransfers(bandwidthMeter, clock, /* simulatedTransferCount= */ 100);
 
     // Create a new instance and seed with some transfers.
     setActiveNetworkInfo(networkInfo2g);
     bandwidthMeter =
         new DefaultBandwidthMeter.Builder(ApplicationProvider.getApplicationContext())
             .setClock(clock)
             .build();
-    simulateTransfers(bandwidthMeter, clock);
+    simulateTransfers(bandwidthMeter, clock, /* simulatedTransferCount= */ 100);
 
     // Override the network type to ethernet and simulate transfers again.
     bandwidthMeter.setNetworkTypeOverride(C.NETWORK_TYPE_ETHERNET);
-    long[] bitrateEstimatesAfterReset = simulateTransfers(bandwidthMeter, clock);
+    long[] bitrateEstimatesAfterReset =
+        simulateTransfers(bandwidthMeter, clock, /* simulatedTransferCount= */ 100);
 
     // If overriding the network type fully reset the bandwidth meter, we expect the bitrate
     // estimates generated during simulation to be the same.
@@ -697,6 +698,36 @@ public void defaultInitialBitrateEstimate_withoutContext_isReasonable() {
     assertThat(initialEstimate).isLessThan(50_000_000L);
   }
 
+  @Test
+  public void getBitrateEstimate_withSimultaneousTransferEvents_receivesUpdatedValues() {
+    FakeClock clock = new FakeClock(/* initialTimeMs= */ 0);
+    DefaultBandwidthMeter bandwidthMeter =
+        new DefaultBandwidthMeter.Builder(ApplicationProvider.getApplicationContext())
+            .setClock(clock)
+            .build();
+
+    Thread thread =
+        new Thread("backgroundTransfers") {
+          @Override
+          public void run() {
+            simulateTransfers(bandwidthMeter, clock, /* simulatedTransferCount= */ 10000);
+          }
+        };
+    thread.start();
+
+    long currentBitrateEstimate = bandwidthMeter.getBitrateEstimate();
+    boolean bitrateEstimateUpdated = false;
+    while (thread.isAlive()) {
+      long newBitrateEstimate = bandwidthMeter.getBitrateEstimate();
+      if (newBitrateEstimate != currentBitrateEstimate) {
+        currentBitrateEstimate = newBitrateEstimate;
+        bitrateEstimateUpdated = true;
+      }
+    }
+
+    assertThat(bitrateEstimateUpdated).isTrue();
+  }
+
   private void setActiveNetworkInfo(NetworkInfo networkInfo) {
     setActiveNetworkInfo(networkInfo, TelephonyDisplayInfo.OVERRIDE_NETWORK_TYPE_NONE);
   }
@@ -725,12 +756,13 @@ private void setNetworkCountryIso(String countryIso) {
     Shadows.shadowOf(telephonyManager).setNetworkCountryIso(countryIso);
   }
 
-  private static long[] simulateTransfers(DefaultBandwidthMeter bandwidthMeter, FakeClock clock) {
-    long[] bitrateEstimates = new long[SIMULATED_TRANSFER_COUNT];
+  private static long[] simulateTransfers(
+      DefaultBandwidthMeter bandwidthMeter, FakeClock clock, int simulatedTransferCount) {
+    long[] bitrateEstimates = new long[simulatedTransferCount];
     Random random = new Random(/* seed= */ 0);
     DataSource dataSource = new FakeDataSource();
     DataSpec dataSpec = new DataSpec(Uri.parse("https://test.com"));
-    for (int i = 0; i < SIMULATED_TRANSFER_COUNT; i++) {
+    for (int i = 0; i < simulatedTransferCount; i++) {
       bandwidthMeter.onTransferStart(dataSource, dataSpec, /* isNetwork= */ true);
       clock.advanceTime(random.nextInt(/* bound= */ 5000));
       bandwidthMeter.onBytesTransferred(

diff --git a/.../java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeterTest.java b/.../java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeterTest.java
@@ -53,7 +53,6 @@
 @Config(sdk = Config.ALL_SDKS) // Test all SDKs because network detection logic changed over time.
 public final class ExperimentalBandwidthMeterTest {
 
-  private static final int SIMULATED_TRANSFER_COUNT = 100;
   private static final String FAST_COUNTRY_ISO = "TW";
   private static final String SLOW_COUNTRY_ISO = "PG";
 
@@ -666,23 +665,79 @@ public void networkTypeOverride_doesFullReset() {
     setActiveNetworkInfo(networkInfoEthernet);
     ExperimentalBandwidthMeter bandwidthMeter =
         new ExperimentalBandwidthMeter.Builder(ApplicationProvider.getApplicationContext()).build();
-    long[] bitrateEstimatesWithNewInstance = simulateTransfers(bandwidthMeter);
+    long[] bitrateEstimatesWithNewInstance =
+        simulateTransfers(bandwidthMeter, /* simulatedTransferCount= */ 100);
 
     // Create a new instance and seed with some transfers.
     setActiveNetworkInfo(networkInfo2g);
     bandwidthMeter =
         new ExperimentalBandwidthMeter.Builder(ApplicationProvider.getApplicationContext()).build();
-    simulateTransfers(bandwidthMeter);
+    simulateTransfers(bandwidthMeter, /* simulatedTransferCount= */ 100);
 
     // Override the network type to ethernet and simulate transfers again.
     bandwidthMeter.setNetworkTypeOverride(C.NETWORK_TYPE_ETHERNET);
-    long[] bitrateEstimatesAfterReset = simulateTransfers(bandwidthMeter);
+    long[] bitrateEstimatesAfterReset =
+        simulateTransfers(bandwidthMeter, /* simulatedTransferCount= */ 100);
 
     // If overriding the network type fully reset the bandwidth meter, we expect the bitrate
     // estimates generated during simulation to be the same.
     assertThat(bitrateEstimatesAfterReset).isEqualTo(bitrateEstimatesWithNewInstance);
   }
 
+  @Test
+  public void getTimeToFirstByteEstimateUs_withSimultaneousTransferEvents_receivesUpdatedValues() {
+    ExperimentalBandwidthMeter bandwidthMeter =
+        new ExperimentalBandwidthMeter.Builder(ApplicationProvider.getApplicationContext()).build();
+
+    Thread thread =
+        new Thread("backgroundTransfers") {
+          @Override
+          public void run() {
+            simulateTransfers(bandwidthMeter, /* simulatedTransferCount= */ 10000);
+          }
+        };
+    thread.start();
+
+    long currentTimeToFirstByteEstimateUs = bandwidthMeter.getTimeToFirstByteEstimateUs();
+    boolean timeToFirstByteEstimateUpdated = false;
+    while (thread.isAlive()) {
+      long newTimeToFirstByteEstimateUs = bandwidthMeter.getTimeToFirstByteEstimateUs();
+      if (newTimeToFirstByteEstimateUs != currentTimeToFirstByteEstimateUs) {
+        currentTimeToFirstByteEstimateUs = newTimeToFirstByteEstimateUs;
+        timeToFirstByteEstimateUpdated = true;
+      }
+    }
+
+    assertThat(timeToFirstByteEstimateUpdated).isTrue();
+  }
+
+  @Test
+  public void getBitrateEstimate_withSimultaneousTransferEvents_receivesUpdatedValues() {
+    ExperimentalBandwidthMeter bandwidthMeter =
+        new ExperimentalBandwidthMeter.Builder(ApplicationProvider.getApplicationContext()).build();
+
+    Thread thread =
+        new Thread("backgroundTransfers") {
+          @Override
+          public void run() {
+            simulateTransfers(bandwidthMeter, /* simulatedTransferCount= */ 10000);
+          }
+        };
+    thread.start();
+
+    long currentBitrateEstimate = bandwidthMeter.getBitrateEstimate();
+    boolean bitrateEstimateUpdated = false;
+    while (thread.isAlive()) {
+      long newBitrateEstimate = bandwidthMeter.getBitrateEstimate();
+      if (newBitrateEstimate != currentBitrateEstimate) {
+        currentBitrateEstimate = newBitrateEstimate;
+        bitrateEstimateUpdated = true;
+      }
+    }
+
+    assertThat(bitrateEstimateUpdated).isTrue();
+  }
+
   private void setActiveNetworkInfo(NetworkInfo networkInfo) {
     setActiveNetworkInfo(networkInfo, TelephonyDisplayInfo.OVERRIDE_NETWORK_TYPE_NONE);
   }
@@ -711,12 +766,13 @@ private void setNetworkCountryIso(String countryIso) {
     Shadows.shadowOf(telephonyManager).setNetworkCountryIso(countryIso);
   }
 
-  private static long[] simulateTransfers(ExperimentalBandwidthMeter bandwidthMeter) {
-    long[] bitrateEstimates = new long[SIMULATED_TRANSFER_COUNT];
+  private static long[] simulateTransfers(
+      ExperimentalBandwidthMeter bandwidthMeter, int simulatedTransferCount) {
+    long[] bitrateEstimates = new long[simulatedTransferCount];
     Random random = new Random(/* seed= */ 0);
     DataSource dataSource = new FakeDataSource();
     DataSpec dataSpec = new DataSpec(Uri.parse("https://test.com"));
-    for (int i = 0; i < SIMULATED_TRANSFER_COUNT; i++) {
+    for (int i = 0; i < simulatedTransferCount; i++) {
       bandwidthMeter.onTransferInitializing(dataSource, dataSpec, /* isNetwork= */ true);
       ShadowSystemClock.advanceBy(Duration.ofMillis(random.nextInt(50)));
       bandwidthMeter.onTransferStart(dataSource, dataSpec, /* isNetwork= */ true);