From c3df64f1027aba05e9aebd43be1ca204c88e251e Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 10 Jul 2018 08:25:17 -0700 Subject: [PATCH] Extend TransferListener with onTransferInitializing and additional parameters. This allows more fine-grained analysis of transfers. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=203950327 --- RELEASENOTES.md | 1 + .../ext/cronet/CronetDataSourceTest.java | 62 ++++++++++++------- .../exoplayer2/upstream/BaseDataSource.java | 20 ++++-- .../upstream/DefaultBandwidthMeter.java | 25 ++++++-- .../exoplayer2/upstream/TransferListener.java | 41 ++++++++++-- .../upstream/BaseDataSourceTest.java | 57 +++++++++++++++-- 6 files changed, 165 insertions(+), 41 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 011deb4987..de76bce9a7 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -29,6 +29,7 @@ directly reading data should implement `BaseDataSource` to handle the registration correctly. Custom `DataSource`'s forwarding to other sources should forward all calls to `addTransferListener`. + * Extend `TransferListener` with additional callback parameters. * Error handling: * Allow configuration of the Loader retry delay ([#3370](https://github.com/google/ExoPlayer/issues/3370)). diff --git a/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java b/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java index 7342b8282a..62e53d3bc1 100644 --- a/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java +++ b/extensions/cronet/src/test/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java @@ -213,7 +213,8 @@ public final class CronetDataSourceTest { public void testRequestOpen() throws HttpDataSourceException { mockResponseStartSuccess(); assertThat(dataSourceUnderTest.open(testDataSpec)).isEqualTo(TEST_CONTENT_LENGTH); - verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); } @Test @@ -225,7 +226,8 @@ public final class CronetDataSourceTest { mockResponseStartSuccess(); assertThat(dataSourceUnderTest.open(testDataSpec)).isEqualTo(5000 /* contentLength */); - verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); } @Test @@ -239,7 +241,8 @@ public final class CronetDataSourceTest { // Check for connection not automatically closed. assertThat(e.getCause() instanceof UnknownHostException).isFalse(); verify(mockUrlRequest, never()).cancel(); - verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener, never()) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); } } @@ -256,7 +259,8 @@ public final class CronetDataSourceTest { // Check for connection not automatically closed. assertThat(e.getCause() instanceof UnknownHostException).isTrue(); verify(mockUrlRequest, never()).cancel(); - verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener, never()) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); } } @@ -272,7 +276,8 @@ public final class CronetDataSourceTest { assertThat(e instanceof HttpDataSource.InvalidResponseCodeException).isTrue(); // Check for connection not automatically closed. verify(mockUrlRequest, never()).cancel(); - verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener, never()) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); } } @@ -298,7 +303,8 @@ public final class CronetDataSourceTest { dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE); assertThat(dataSourceUnderTest.open(testPostDataSpec)).isEqualTo(TEST_CONTENT_LENGTH); - verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testPostDataSpec); + verify(mockTransferListener) + .onTransferStart(dataSourceUnderTest, testPostDataSpec, /* isNetwork= */ true); } @Test @@ -346,7 +352,8 @@ public final class CronetDataSourceTest { // Should have only called read on cronet once. verify(mockUrlRequest, times(1)).read(any(ByteBuffer.class)); - verify(mockTransferListener, times(2)).onBytesTransferred(dataSourceUnderTest, 8); + verify(mockTransferListener, times(2)) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 8); } @Test @@ -386,7 +393,8 @@ public final class CronetDataSourceTest { int bytesRead = dataSourceUnderTest.read(returnedBuffer, 8, 8); assertThat(bytesRead).isEqualTo(8); assertThat(returnedBuffer).isEqualTo(prefixZeros(buildTestDataArray(0, 8), 16)); - verify(mockTransferListener).onBytesTransferred(dataSourceUnderTest, 8); + verify(mockTransferListener) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 8); } @Test @@ -402,7 +410,8 @@ public final class CronetDataSourceTest { int bytesRead = dataSourceUnderTest.read(returnedBuffer, 0, 16); assertThat(bytesRead).isEqualTo(16); assertThat(returnedBuffer).isEqualTo(buildTestDataArray(1000, 16)); - verify(mockTransferListener).onBytesTransferred(dataSourceUnderTest, 16); + verify(mockTransferListener) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 16); } @Test @@ -418,7 +427,8 @@ public final class CronetDataSourceTest { int bytesRead = dataSourceUnderTest.read(returnedBuffer, 0, 16); assertThat(bytesRead).isEqualTo(16); assertThat(returnedBuffer).isEqualTo(buildTestDataArray(1000, 16)); - verify(mockTransferListener).onBytesTransferred(dataSourceUnderTest, 16); + verify(mockTransferListener) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 16); } @Test @@ -433,7 +443,8 @@ public final class CronetDataSourceTest { int bytesRead = dataSourceUnderTest.read(returnedBuffer, 8, 8); assertThat(returnedBuffer).isEqualTo(prefixZeros(buildTestDataArray(0, 8), 16)); assertThat(bytesRead).isEqualTo(8); - verify(mockTransferListener).onBytesTransferred(dataSourceUnderTest, 8); + verify(mockTransferListener) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 8); } @Test @@ -447,7 +458,8 @@ public final class CronetDataSourceTest { int bytesRead = dataSourceUnderTest.read(returnedBuffer, 0, 24); assertThat(returnedBuffer).isEqualTo(suffixZeros(buildTestDataArray(0, 16), 24)); assertThat(bytesRead).isEqualTo(16); - verify(mockTransferListener).onBytesTransferred(dataSourceUnderTest, 16); + verify(mockTransferListener) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 16); } @Test @@ -464,7 +476,8 @@ public final class CronetDataSourceTest { assertThat(bytesRead).isEqualTo(8); dataSourceUnderTest.close(); - verify(mockTransferListener).onTransferEnd(dataSourceUnderTest); + verify(mockTransferListener) + .onTransferEnd(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); try { bytesRead += dataSourceUnderTest.read(returnedBuffer, 0, 8); @@ -505,9 +518,12 @@ public final class CronetDataSourceTest { // Should have only called read on cronet once. verify(mockUrlRequest, times(1)).read(any(ByteBuffer.class)); - verify(mockTransferListener, times(1)).onBytesTransferred(dataSourceUnderTest, 8); - verify(mockTransferListener, times(1)).onBytesTransferred(dataSourceUnderTest, 6); - verify(mockTransferListener, times(1)).onBytesTransferred(dataSourceUnderTest, 2); + verify(mockTransferListener, times(1)) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 8); + verify(mockTransferListener, times(1)) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 6); + verify(mockTransferListener, times(1)) + .onBytesTransferred(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, 2); // Now we already returned the 16 bytes initially asked. // Try to read again even though all requested 16 bytes are already returned. @@ -518,7 +534,8 @@ public final class CronetDataSourceTest { assertThat(returnedBuffer).isEqualTo(new byte[16]); // C.RESULT_END_OF_INPUT should not be reported though the TransferListener. verify(mockTransferListener, never()) - .onBytesTransferred(dataSourceUnderTest, C.RESULT_END_OF_INPUT); + .onBytesTransferred( + dataSourceUnderTest, testDataSpec, /* isNetwork= */ true, C.RESULT_END_OF_INPUT); // There should still be only one call to read on cronet. verify(mockUrlRequest, times(1)).read(any(ByteBuffer.class)); // Check for connection not automatically closed. @@ -559,7 +576,8 @@ public final class CronetDataSourceTest { ShadowSystemClock.setCurrentTimeMillis(startTimeMs + TEST_CONNECT_TIMEOUT_MS + 10); timedOutLatch.await(); - verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener, never()) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); } @Test @@ -597,7 +615,8 @@ public final class CronetDataSourceTest { thread.interrupt(); timedOutLatch.await(); - verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener, never()) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); } @Test @@ -678,7 +697,8 @@ public final class CronetDataSourceTest { ShadowSystemClock.setCurrentTimeMillis(startTimeMs + newTimeoutMs + 10); timedOutLatch.await(); - verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); + verify(mockTransferListener, never()) + .onTransferStart(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); assertThat(openExceptions.get()).isEqualTo(1); } @@ -797,7 +817,7 @@ public final class CronetDataSourceTest { // the subsequent open() call succeeds. doThrow(new NullPointerException()) .when(mockTransferListener) - .onTransferEnd(dataSourceUnderTest); + .onTransferEnd(dataSourceUnderTest, testDataSpec, /* isNetwork= */ true); dataSourceUnderTest.open(testDataSpec); try { dataSourceUnderTest.close(); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/BaseDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/BaseDataSource.java index 804f8c50ac..930cceaa10 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/BaseDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/BaseDataSource.java @@ -15,6 +15,8 @@ */ package com.google.android.exoplayer2.upstream; +import android.support.annotation.Nullable; +import com.google.android.exoplayer2.util.Assertions; import java.util.ArrayList; /** @@ -29,6 +31,8 @@ public abstract class BaseDataSource implements DataSource { private final boolean isNetwork; private final ArrayList> listeners; + private @Nullable DataSpec dataSpec; + /** * Creates base data source. * @@ -50,7 +54,9 @@ public abstract class BaseDataSource implements DataSource { * @param dataSpec {@link DataSpec} describing the data for initializing transfer. */ protected final void transferInitializing(DataSpec dataSpec) { - // TODO: notify listeners. + for (int i = 0; i < listeners.size(); i++) { + listeners.get(i).onTransferInitializing(/* source= */ this, dataSpec, isNetwork); + } } /** @@ -59,8 +65,9 @@ public abstract class BaseDataSource implements DataSource { * @param dataSpec {@link DataSpec} describing the data being transferred. */ protected final void transferStarted(DataSpec dataSpec) { + this.dataSpec = dataSpec; for (int i = 0; i < listeners.size(); i++) { - listeners.get(i).onTransferStart(/* source= */ this, dataSpec); + listeners.get(i).onTransferStart(/* source= */ this, dataSpec, isNetwork); } } @@ -71,15 +78,20 @@ public abstract class BaseDataSource implements DataSource { * (or if the first call, since the transfer was started). */ protected final void bytesTransferred(int bytesTransferred) { + DataSpec dataSpec = Assertions.checkNotNull(this.dataSpec); for (int i = 0; i < listeners.size(); i++) { - listeners.get(i).onBytesTransferred(/* source= */ this, bytesTransferred); + listeners + .get(i) + .onBytesTransferred(/* source= */ this, dataSpec, isNetwork, bytesTransferred); } } /** Notifies listeners that a transfer ended. */ protected final void transferEnded() { + DataSpec dataSpec = Assertions.checkNotNull(this.dataSpec); for (int i = 0; i < listeners.size(); i++) { - listeners.get(i).onTransferEnd(/* source= */ this); + listeners.get(i).onTransferEnd(/* source= */ this, dataSpec, isNetwork); } + this.dataSpec = null; } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/DefaultBandwidthMeter.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/DefaultBandwidthMeter.java index fb084ad3a4..b1bc2c17d2 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/DefaultBandwidthMeter.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/DefaultBandwidthMeter.java @@ -22,8 +22,8 @@ import com.google.android.exoplayer2.util.Clock; import com.google.android.exoplayer2.util.SlidingPercentile; /** - * Estimates bandwidth by listening to data transfers. The bandwidth estimate is calculated using - * a {@link SlidingPercentile} and is updated each time a transfer ends. + * Estimates bandwidth by listening to data transfers. The bandwidth estimate is calculated using a + * {@link SlidingPercentile} and is updated each time a transfer ends. */ public final class DefaultBandwidthMeter implements BandwidthMeter, TransferListener { @@ -177,7 +177,15 @@ public final class DefaultBandwidthMeter implements BandwidthMeter, TransferList } @Override - public synchronized void onTransferStart(Object source, DataSpec dataSpec) { + public void onTransferInitializing(Object source, DataSpec dataSpec, boolean isNetwork) { + // Do nothing. + } + + @Override + public synchronized void onTransferStart(Object source, DataSpec dataSpec, boolean isNetwork) { + if (!isNetwork) { + return; + } if (streamCount == 0) { sampleStartTimeMs = clock.elapsedRealtime(); } @@ -185,12 +193,19 @@ public final class DefaultBandwidthMeter implements BandwidthMeter, TransferList } @Override - public synchronized void onBytesTransferred(Object source, int bytes) { + public synchronized void onBytesTransferred( + Object source, DataSpec dataSpec, boolean isNetwork, int bytes) { + if (!isNetwork) { + return; + } sampleBytesTransferred += bytes; } @Override - public synchronized void onTransferEnd(Object source) { + public synchronized void onTransferEnd(Object source, DataSpec dataSpec, boolean isNetwork) { + if (!isNetwork) { + return; + } Assertions.checkState(streamCount > 0); long nowMs = clock.elapsedRealtime(); int sampleElapsedTimeMs = (int) (nowMs - sampleStartTimeMs); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/TransferListener.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/TransferListener.java index e061f0c7d0..211772e67c 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/TransferListener.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/TransferListener.java @@ -17,31 +17,60 @@ package com.google.android.exoplayer2.upstream; /** * A listener of data transfer events. + * + *

A transfer usually progresses through multiple steps: + * + *

    + *
  1. Initializing the underlying resource (e.g. opening a HTTP connection). {@link + * #onTransferInitializing(Object, DataSpec, boolean)} is called before the initialization + * starts. + *
  2. Starting the transfer after successfully initializing the resource. {@link + * #onTransferStart(Object, DataSpec, boolean)} is called. Note that this only happens if the + * initialization was successful. + *
  3. Transferring data. {@link #onBytesTransferred(Object, DataSpec, boolean, int)} is called + * frequently during the transfer to indicate progress. + *
  4. Closing the transfer and the underlying resource. {@link #onTransferEnd(Object, DataSpec, + * boolean)} is called. Note that each {@link #onTransferStart(Object, DataSpec, boolean)} + * will have exactly one corresponding call to {@link #onTransferEnd(Object, DataSpec, + * boolean)}. + *
*/ public interface TransferListener { + /** + * Called when a transfer is being initialized. + * + * @param source The source performing the transfer. + * @param dataSpec Describes the data for which the transfer is initialized. + * @param isNetwork Whether the data is transferred through a network. + */ + void onTransferInitializing(S source, DataSpec dataSpec, boolean isNetwork); + /** * Called when a transfer starts. * * @param source The source performing the transfer. * @param dataSpec Describes the data being transferred. + * @param isNetwork Whether the data is transferred through a network. */ - void onTransferStart(S source, DataSpec dataSpec); + void onTransferStart(S source, DataSpec dataSpec, boolean isNetwork); /** * Called incrementally during a transfer. * * @param source The source performing the transfer. - * @param bytesTransferred The number of bytes transferred since the previous call to this - * method (or if the first call, since the transfer was started). + * @param dataSpec Describes the data being transferred. + * @param isNetwork Whether the data is transferred through a network. + * @param bytesTransferred The number of bytes transferred since the previous call to this method */ - void onBytesTransferred(S source, int bytesTransferred); + void onBytesTransferred(S source, DataSpec dataSpec, boolean isNetwork, int bytesTransferred); /** * Called when a transfer ends. * * @param source The source performing the transfer. + * @param dataSpec Describes the data being transferred. + * @param isNetwork Whether the data is transferred through a network. */ - void onTransferEnd(S source); - + void onTransferEnd(S source, DataSpec dataSpec, boolean isNetwork); } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/BaseDataSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/BaseDataSourceTest.java index 4d736d20e7..681c55a8b1 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/BaseDataSourceTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/BaseDataSourceTest.java @@ -40,10 +40,21 @@ public class BaseDataSourceTest { testSource.read(/* buffer= */ null, /* offset= */ 0, /* readLength= */ 100); testSource.close(); + assertThat(transferListener.lastTransferInitializingSource).isSameAs(testSource); assertThat(transferListener.lastTransferStartSource).isSameAs(testSource); assertThat(transferListener.lastBytesTransferredSource).isSameAs(testSource); assertThat(transferListener.lastTransferEndSource).isSameAs(testSource); + + assertThat(transferListener.lastTransferInitializingDataSpec).isEqualTo(dataSpec); assertThat(transferListener.lastTransferStartDataSpec).isEqualTo(dataSpec); + assertThat(transferListener.lastBytesTransferredDataSpec).isEqualTo(dataSpec); + assertThat(transferListener.lastTransferEndDataSpec).isEqualTo(dataSpec); + + assertThat(transferListener.lastTransferInitializingIsNetwork).isEqualTo(false); + assertThat(transferListener.lastTransferStartIsNetwork).isEqualTo(false); + assertThat(transferListener.lastBytesTransferredIsNetwork).isEqualTo(false); + assertThat(transferListener.lastTransferEndIsNetwork).isEqualTo(false); + assertThat(transferListener.lastBytesTransferred).isEqualTo(100); } @@ -58,10 +69,21 @@ public class BaseDataSourceTest { testSource.read(/* buffer= */ null, /* offset= */ 0, /* readLength= */ 100); testSource.close(); + assertThat(transferListener.lastTransferInitializingSource).isSameAs(testSource); assertThat(transferListener.lastTransferStartSource).isSameAs(testSource); assertThat(transferListener.lastBytesTransferredSource).isSameAs(testSource); assertThat(transferListener.lastTransferEndSource).isSameAs(testSource); + + assertThat(transferListener.lastTransferInitializingDataSpec).isEqualTo(dataSpec); assertThat(transferListener.lastTransferStartDataSpec).isEqualTo(dataSpec); + assertThat(transferListener.lastBytesTransferredDataSpec).isEqualTo(dataSpec); + assertThat(transferListener.lastTransferEndDataSpec).isEqualTo(dataSpec); + + assertThat(transferListener.lastTransferInitializingIsNetwork).isEqualTo(true); + assertThat(transferListener.lastTransferStartIsNetwork).isEqualTo(true); + assertThat(transferListener.lastBytesTransferredIsNetwork).isEqualTo(true); + assertThat(transferListener.lastTransferEndIsNetwork).isEqualTo(true); + assertThat(transferListener.lastBytesTransferred).isEqualTo(100); } @@ -73,6 +95,7 @@ public class BaseDataSourceTest { @Override public long open(DataSpec dataSpec) throws IOException { + transferInitializing(dataSpec); transferStarted(dataSpec); return C.LENGTH_UNSET; } @@ -96,27 +119,51 @@ public class BaseDataSourceTest { private static final class TestTransferListener implements TransferListener { + public Object lastTransferInitializingSource; + public DataSpec lastTransferInitializingDataSpec; + public boolean lastTransferInitializingIsNetwork; + public Object lastTransferStartSource; public DataSpec lastTransferStartDataSpec; + public boolean lastTransferStartIsNetwork; + public Object lastBytesTransferredSource; + public DataSpec lastBytesTransferredDataSpec; + public boolean lastBytesTransferredIsNetwork; public int lastBytesTransferred; + public Object lastTransferEndSource; + public DataSpec lastTransferEndDataSpec; + public boolean lastTransferEndIsNetwork; @Override - public void onTransferStart(DataSource source, DataSpec dataSpec) { - lastTransferStartSource = source; - lastTransferStartDataSpec = dataSpec; + public void onTransferInitializing(DataSource source, DataSpec dataSpec, boolean isNetwork) { + lastTransferInitializingSource = source; + lastTransferInitializingDataSpec = dataSpec; + lastTransferInitializingIsNetwork = isNetwork; } @Override - public void onBytesTransferred(DataSource source, int bytesTransferred) { + public void onTransferStart(DataSource source, DataSpec dataSpec, boolean isNetwork) { + lastTransferStartSource = source; + lastTransferStartDataSpec = dataSpec; + lastTransferStartIsNetwork = isNetwork; + } + + @Override + public void onBytesTransferred( + DataSource source, DataSpec dataSpec, boolean isNetwork, int bytesTransferred) { lastBytesTransferredSource = source; + lastBytesTransferredDataSpec = dataSpec; + lastBytesTransferredIsNetwork = isNetwork; lastBytesTransferred = bytesTransferred; } @Override - public void onTransferEnd(DataSource source) { + public void onTransferEnd(DataSource source, DataSpec dataSpec, boolean isNetwork) { lastTransferEndSource = source; + lastTransferEndDataSpec = dataSpec; + lastTransferEndIsNetwork = isNetwork; } } }