From dd2a3736368b2b02b64f42bb646ca8059591d933 Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 8 Sep 2023 02:24:53 -0700 Subject: [PATCH] Add missing synchronization in ExperimentalBandwidthMeter Issue: androidx/media#612 PiperOrigin-RevId: 563690229 --- RELEASENOTES.md | 2 + .../upstream/DefaultBandwidthMeter.java | 20 +++++- .../ExperimentalBandwidthMeter.java | 18 +++-- .../upstream/DefaultBandwidthMeterTest.java | 46 ++++++++++-- .../ExperimentalBandwidthMeterTest.java | 70 +++++++++++++++++-- 5 files changed, 134 insertions(+), 22 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 363ddda6c0..668b2212ef 100644 --- 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/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeter.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeter.java index 65f89e3fa7..8907b03c2e 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeter.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeter.java @@ -17,6 +17,7 @@ package androidx.media3.exoplayer.upstream; 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 final class DefaultBandwidthMeter implements BandwidthMeter, TransferList private final ImmutableMap 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 @@ public final class DefaultBandwidthMeter implements BandwidthMeter, TransferList slidingPercentile.reset(); } + @GuardedBy("this") private void maybeNotifyBandwidthSample( int elapsedMs, long bytesTransferred, long bitrateEstimate) { if (elapsedMs == 0 && bytesTransferred == 0 && bitrateEstimate == lastReportedBitrateEstimate) { diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeter.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeter.java index e7e62179e7..7ed2233079 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeter.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeter.java @@ -19,6 +19,7 @@ import static androidx.media3.common.util.Assertions.checkNotNull; 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,10 +276,14 @@ public final class ExperimentalBandwidthMeter implements BandwidthMeter, Transfe } private final ImmutableMap initialBitrateEstimates; - private final TimeToFirstByteEstimator timeToFirstByteEstimator; - private final BandwidthEstimator bandwidthEstimator; 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 @C.NetworkType int networkType; private long initialBitrateEstimate; private boolean networkTypeOverrideSet; @@ -323,7 +328,7 @@ public final class ExperimentalBandwidthMeter implements BandwidthMeter, Transfe } @Override - public long getTimeToFirstByteEstimateUs() { + public synchronized long getTimeToFirstByteEstimateUs() { return timeToFirstByteEstimator.getTimeToFirstByteEstimateUs(); } @@ -333,19 +338,20 @@ public final class ExperimentalBandwidthMeter implements BandwidthMeter, Transfe } @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/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeterTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeterTest.java index 87aea55c92..9ad273c6aa 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeterTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/DefaultBandwidthMeterTest.java @@ -52,7 +52,6 @@ import org.robolectric.shadows.ShadowTelephonyManager; @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,7 +667,8 @@ public final class DefaultBandwidthMeterTest { 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); @@ -676,11 +676,12 @@ public final class DefaultBandwidthMeterTest { 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 final class DefaultBandwidthMeterTest { 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 @@ public final class DefaultBandwidthMeterTest { 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/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeterTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeterTest.java index 292a52f25f..e00382dc1c 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeterTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/upstream/experimental/ExperimentalBandwidthMeterTest.java @@ -53,7 +53,6 @@ import org.robolectric.shadows.ShadowTelephonyManager; @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 final class ExperimentalBandwidthMeterTest { 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 @@ public final class ExperimentalBandwidthMeterTest { 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);