From 5803b2538d4f6a8ef45e95ec73b6c42cc974dcd9 Mon Sep 17 00:00:00 2001 From: olly Date: Thu, 6 Oct 2016 10:13:27 -0700 Subject: [PATCH 01/13] No-op reorder of CronetDataSource methods This change is a no-op reodering, as a precursor to further cleanup. The public methods are grouped by the class/interface they implement. The private methods are ordered with things that will become static in a subsequent change at the bottom. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135372629 --- .../ext/cronet/CronetDataSource.java | 427 +++++++++--------- 1 file changed, 213 insertions(+), 214 deletions(-) diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index cae28731ea..04b468291d 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -169,6 +169,8 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou connectionState = IDLE_CONNECTION; } + // HttpDataSource implementation. + @Override public void setRequestProperty(String name, String value) { synchronized (requestProperties) { @@ -195,6 +197,11 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return responseInfo == null ? null : responseInfo.getAllHeaders(); } + @Override + public Uri getUri() { + return Uri.parse(currentUrl); + } + @Override public long open(DataSpec dataSpec) throws HttpDataSourceException { Assertions.checkNotNull(dataSpec); @@ -224,186 +231,6 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return contentLength; } - private void startRequest(DataSpec dataSpec) throws HttpDataSourceException { - currentUrl = dataSpec.uri.toString(); - currentDataSpec = dataSpec; - UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(currentUrl, this, executor, - cronetEngine); - fillCurrentRequestHeader(urlRequestBuilder); - fillCurrentRequestPostBody(urlRequestBuilder, dataSpec); - currentUrlRequest = urlRequestBuilder.build(); - currentUrlRequest.start(); - } - - private void fillCurrentRequestHeader(UrlRequest.Builder urlRequestBuilder) { - synchronized (requestProperties) { - for (Entry headerEntry : requestProperties.entrySet()) { - urlRequestBuilder.addHeader(headerEntry.getKey(), headerEntry.getValue()); - } - } - if (currentDataSpec.position == 0 && currentDataSpec.length == C.LENGTH_UNSET) { - // Not required. - return; - } - StringBuilder rangeValue = new StringBuilder(); - rangeValue.append("bytes="); - rangeValue.append(currentDataSpec.position); - rangeValue.append("-"); - if (currentDataSpec.length != C.LENGTH_UNSET) { - rangeValue.append(currentDataSpec.position + currentDataSpec.length - 1); - } - urlRequestBuilder.addHeader("Range", rangeValue.toString()); - } - - private void fillCurrentRequestPostBody(UrlRequest.Builder urlRequestBuilder, DataSpec dataSpec) - throws HttpDataSourceException { - if (dataSpec.postBody != null) { - if (!requestProperties.containsKey("Content-Type")) { - throw new OpenException("POST requests must set a Content-Type header", dataSpec, - getCurrentRequestStatus()); - } - urlRequestBuilder.setUploadDataProvider( - new ByteArrayUploadDataProvider(dataSpec.postBody), executor); - } - } - - @Override - public synchronized void onFailed(UrlRequest request, UrlResponseInfo info, - UrlRequestException error) { - if (request != currentUrlRequest) { - return; - } - if (connectionState == OPENING_CONNECTION) { - IOException cause = error.getErrorCode() == UrlRequestException.ERROR_HOSTNAME_NOT_RESOLVED - ? new UnknownHostException() : error; - exception = new OpenException(cause, currentDataSpec, getCurrentRequestStatus()); - } else if (connectionState == OPEN_CONNECTION) { - exception = new HttpDataSourceException(error, currentDataSpec, - HttpDataSourceException.TYPE_READ); - } - operation.open(); - } - - @Override - public synchronized void onResponseStarted(UrlRequest request, UrlResponseInfo info) { - if (request != currentUrlRequest) { - return; - } - try { - validateResponse(info); - responseInfo = info; - - if (isCompressed(info)) { - contentLength = currentDataSpec.length; - } else { - // Check content length. - contentLength = getContentLength(info.getAllHeaders()); - // If a specific length is requested and a specific length is returned but the 2 don't match - // it's an error. - if (currentDataSpec.length != C.LENGTH_UNSET - && contentLength != C.LENGTH_UNSET - && currentDataSpec.length != contentLength) { - throw new OpenException("Content length did not match requested length", currentDataSpec, - getCurrentRequestStatus()); - } - } - - if (contentLength > 0) { - expectedBytesRemainingToRead = new AtomicLong(contentLength); - } - - // Keep track of redirects. - currentUrl = responseInfo.getUrl(); - connectionState = CONNECTED_CONNECTION; - } catch (HttpDataSourceException e) { - exception = e; - } finally { - operation.open(); - } - } - - /** - * Returns {@code true} iff the content is compressed. - * - *

If {@code true}, clients cannot use the value of content length from the request headers to - * read the data, since Cronet returns the uncompressed data and this content length reflects the - * compressed content length. - */ - private boolean isCompressed(UrlResponseInfo info) { - for (Map.Entry entry : info.getAllHeadersAsList()) { - if (entry.getKey().equalsIgnoreCase("Content-Encoding")) { - return !entry.getValue().equalsIgnoreCase("identity"); - } - } - - return false; - } - - private void validateResponse(UrlResponseInfo info) throws HttpDataSourceException { - // Check for a valid response code. - int responseCode = info.getHttpStatusCode(); - if (responseCode < 200 || responseCode > 299) { - InvalidResponseCodeException exception = new InvalidResponseCodeException( - responseCode, info.getAllHeaders(), currentDataSpec); - if (responseCode == 416) { - exception.initCause(new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE)); - } - throw exception; - } - // Check for a valid content type. - try { - String contentType = info.getAllHeaders().get("Content-Type").get(0); - if (contentTypePredicate != null && !contentTypePredicate.evaluate(contentType)) { - throw new InvalidContentTypeException(contentType, currentDataSpec); - } - } catch (IndexOutOfBoundsException e) { - throw new InvalidContentTypeException(null, currentDataSpec); - } - } - - private long getContentLength(Map> headers) { - // Logic copied from {@code DefaultHttpDataSource} - long contentLength = C.LENGTH_UNSET; - List contentLengthHeader = headers.get("Content-Length"); - if (contentLengthHeader != null - && !contentLengthHeader.isEmpty() - && !TextUtils.isEmpty(contentLengthHeader.get(0))) { - try { - contentLength = Long.parseLong(contentLengthHeader.get(0)); - } catch (NumberFormatException e) { - log(Log.ERROR, "Unexpected Content-Length [" + contentLengthHeader + "]"); - } - } - List contentRangeHeader = headers.get("Content-Range"); - if (contentRangeHeader != null - && !contentRangeHeader.isEmpty() - && !TextUtils.isEmpty(contentRangeHeader.get(0))) { - Matcher matcher = CONTENT_RANGE_HEADER_PATTERN.matcher(contentRangeHeader.get(0)); - if (matcher.find()) { - try { - long contentLengthFromRange = - Long.parseLong(matcher.group(2)) - Long.parseLong(matcher.group(1)) + 1; - if (contentLength < 0) { - // Some proxy servers strip the Content-Length header. Fall back to the length - // calculated here in this case. - contentLength = contentLengthFromRange; - } else if (contentLength != contentLengthFromRange) { - // If there is a discrepancy between the Content-Length and Content-Range headers, - // assume the one with the larger value is correct. We have seen cases where carrier - // change one of them to reduce the size of a request, but it is unlikely anybody - // would increase it. - log(Log.WARN, "Inconsistent headers [" + contentLengthHeader + "] [" - + contentRangeHeader + "]"); - contentLength = Math.max(contentLength, contentLengthFromRange); - } - } catch (NumberFormatException e) { - log(Log.ERROR, "Unexpected Content-Range [" + contentRangeHeader + "]"); - } - } - } - return contentLength; - } - @Override public int read(byte[] buffer, int offset, int readLength) throws HttpDataSourceException { synchronized (this) { @@ -451,6 +278,31 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return bytesRead; } + @Override + public synchronized void close() { + if (currentUrlRequest != null) { + currentUrlRequest.cancel(); + currentUrlRequest = null; + } + currentDataSpec = null; + currentUrl = null; + exception = null; + contentLength = 0; + hasData = false; + responseInfo = null; + expectedBytesRemainingToRead = null; + responseFinished = false; + try { + if (listener != null && connectionState == OPEN_CONNECTION) { + listener.onTransferEnd(this); + } + } finally { + connectionState = IDLE_CONNECTION; + } + } + + // UrlRequest.Callback implementation + @Override public void onRedirectReceived(UrlRequest request, UrlResponseInfo info, String newLocationUrl) { if (request != currentUrlRequest) { @@ -474,6 +326,44 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou request.followRedirect(); } + @Override + public synchronized void onResponseStarted(UrlRequest request, UrlResponseInfo info) { + if (request != currentUrlRequest) { + return; + } + try { + validateResponse(info); + responseInfo = info; + + if (isCompressed(info)) { + contentLength = currentDataSpec.length; + } else { + // Check content length. + contentLength = getContentLength(info.getAllHeaders()); + // If a specific length is requested and a specific length is returned but the 2 don't match + // it's an error. + if (currentDataSpec.length != C.LENGTH_UNSET + && contentLength != C.LENGTH_UNSET + && currentDataSpec.length != contentLength) { + throw new OpenException("Content length did not match requested length", currentDataSpec, + getCurrentRequestStatus()); + } + } + + if (contentLength > 0) { + expectedBytesRemainingToRead = new AtomicLong(contentLength); + } + + // Keep track of redirects. + currentUrl = responseInfo.getUrl(); + connectionState = CONNECTED_CONNECTION; + } catch (HttpDataSourceException e) { + exception = e; + } finally { + operation.open(); + } + } + @Override public synchronized void onReadCompleted(UrlRequest request, UrlResponseInfo info, ByteBuffer buffer) { @@ -495,37 +385,154 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou } @Override - public synchronized void close() { - if (currentUrlRequest != null) { - currentUrlRequest.cancel(); - currentUrlRequest = null; + public synchronized void onFailed(UrlRequest request, UrlResponseInfo info, + UrlRequestException error) { + if (request != currentUrlRequest) { + return; } - currentDataSpec = null; - currentUrl = null; - exception = null; - contentLength = 0; - hasData = false; - responseInfo = null; - expectedBytesRemainingToRead = null; - responseFinished = false; - try { - if (listener != null && connectionState == OPEN_CONNECTION) { - listener.onTransferEnd(this); + if (connectionState == OPENING_CONNECTION) { + IOException cause = error.getErrorCode() == UrlRequestException.ERROR_HOSTNAME_NOT_RESOLVED + ? new UnknownHostException() : error; + exception = new OpenException(cause, currentDataSpec, getCurrentRequestStatus()); + } else if (connectionState == OPEN_CONNECTION) { + exception = new HttpDataSourceException(error, currentDataSpec, + HttpDataSourceException.TYPE_READ); + } + operation.open(); + } + + // Internal methods. + + private void fillCurrentRequestHeader(UrlRequest.Builder urlRequestBuilder) { + synchronized (requestProperties) { + for (Entry headerEntry : requestProperties.entrySet()) { + urlRequestBuilder.addHeader(headerEntry.getKey(), headerEntry.getValue()); } - } finally { - connectionState = IDLE_CONNECTION; + } + if (currentDataSpec.position == 0 && currentDataSpec.length == C.LENGTH_UNSET) { + // Not required. + return; + } + StringBuilder rangeValue = new StringBuilder(); + rangeValue.append("bytes="); + rangeValue.append(currentDataSpec.position); + rangeValue.append("-"); + if (currentDataSpec.length != C.LENGTH_UNSET) { + rangeValue.append(currentDataSpec.position + currentDataSpec.length - 1); + } + urlRequestBuilder.addHeader("Range", rangeValue.toString()); + } + + private void fillCurrentRequestPostBody(UrlRequest.Builder urlRequestBuilder, DataSpec dataSpec) + throws HttpDataSourceException { + if (dataSpec.postBody != null) { + if (!requestProperties.containsKey("Content-Type")) { + throw new OpenException("POST requests must set a Content-Type header", dataSpec, + getCurrentRequestStatus()); + } + urlRequestBuilder.setUploadDataProvider( + new ByteArrayUploadDataProvider(dataSpec.postBody), executor); } } - @Override - public Uri getUri() { - return Uri.parse(currentUrl); + private void startRequest(DataSpec dataSpec) throws HttpDataSourceException { + currentUrl = dataSpec.uri.toString(); + currentDataSpec = dataSpec; + UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(currentUrl, this, executor, + cronetEngine); + fillCurrentRequestHeader(urlRequestBuilder); + fillCurrentRequestPostBody(urlRequestBuilder, dataSpec); + currentUrlRequest = urlRequestBuilder.build(); + currentUrlRequest.start(); } - private void log(int priority, String message) { - if (Log.isLoggable(TAG, priority)) { - Log.println(priority, TAG, message); + private void validateResponse(UrlResponseInfo info) throws HttpDataSourceException { + // Check for a valid response code. + int responseCode = info.getHttpStatusCode(); + if (responseCode < 200 || responseCode > 299) { + InvalidResponseCodeException exception = new InvalidResponseCodeException( + responseCode, info.getAllHeaders(), currentDataSpec); + if (responseCode == 416) { + exception.initCause(new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE)); + } + throw exception; } + // Check for a valid content type. + try { + String contentType = info.getAllHeaders().get("Content-Type").get(0); + if (contentTypePredicate != null && !contentTypePredicate.evaluate(contentType)) { + throw new InvalidContentTypeException(contentType, currentDataSpec); + } + } catch (IndexOutOfBoundsException e) { + throw new InvalidContentTypeException(null, currentDataSpec); + } + } + + private boolean blockUntilConnectTimeout() { + long now = clock.elapsedRealtime(); + boolean opened = false; + while (!opened && now < currentConnectTimeoutMs) { + opened = operation.block(currentConnectTimeoutMs - now + 5 /* fudge factor */); + now = clock.elapsedRealtime(); + } + return opened; + } + + private void resetConnectTimeout() { + currentConnectTimeoutMs = clock.elapsedRealtime() + connectTimeoutMs; + } + + private boolean isCompressed(UrlResponseInfo info) { + for (Map.Entry entry : info.getAllHeadersAsList()) { + if (entry.getKey().equalsIgnoreCase("Content-Encoding")) { + return !entry.getValue().equalsIgnoreCase("identity"); + } + } + + return false; + } + + private long getContentLength(Map> headers) { + // Logic copied from {@code DefaultHttpDataSource} + long contentLength = C.LENGTH_UNSET; + List contentLengthHeader = headers.get("Content-Length"); + if (contentLengthHeader != null + && !contentLengthHeader.isEmpty() + && !TextUtils.isEmpty(contentLengthHeader.get(0))) { + try { + contentLength = Long.parseLong(contentLengthHeader.get(0)); + } catch (NumberFormatException e) { + log(Log.ERROR, "Unexpected Content-Length [" + contentLengthHeader + "]"); + } + } + List contentRangeHeader = headers.get("Content-Range"); + if (contentRangeHeader != null + && !contentRangeHeader.isEmpty() + && !TextUtils.isEmpty(contentRangeHeader.get(0))) { + Matcher matcher = CONTENT_RANGE_HEADER_PATTERN.matcher(contentRangeHeader.get(0)); + if (matcher.find()) { + try { + long contentLengthFromRange = + Long.parseLong(matcher.group(2)) - Long.parseLong(matcher.group(1)) + 1; + if (contentLength < 0) { + // Some proxy servers strip the Content-Length header. Fall back to the length + // calculated here in this case. + contentLength = contentLengthFromRange; + } else if (contentLength != contentLengthFromRange) { + // If there is a discrepancy between the Content-Length and Content-Range headers, + // assume the one with the larger value is correct. We have seen cases where carrier + // change one of them to reduce the size of a request, but it is unlikely anybody + // would increase it. + log(Log.WARN, "Inconsistent headers [" + contentLengthHeader + "] [" + + contentRangeHeader + "]"); + contentLength = Math.max(contentLength, contentLengthFromRange); + } + } catch (NumberFormatException e) { + log(Log.ERROR, "Unexpected Content-Range [" + contentRangeHeader + "]"); + } + } + } + return contentLength; } private int getCurrentRequestStatus() { @@ -544,18 +551,10 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return result.get(); } - private boolean blockUntilConnectTimeout() { - long now = clock.elapsedRealtime(); - boolean opened = false; - while (!opened && now < currentConnectTimeoutMs) { - opened = operation.block(currentConnectTimeoutMs - now + 5 /* fudge factor */); - now = clock.elapsedRealtime(); + private void log(int priority, String message) { + if (Log.isLoggable(TAG, priority)) { + Log.println(priority, TAG, message); } - return opened; - } - - private void resetConnectTimeout() { - currentConnectTimeoutMs = clock.elapsedRealtime() + connectTimeoutMs; } } From ba56f9165cd7feae9a6039779de0bc311d8736d7 Mon Sep 17 00:00:00 2001 From: olly Date: Thu, 6 Oct 2016 10:20:55 -0700 Subject: [PATCH 02/13] Partial cleanup of CronetDataSource - Fix bug in getCurrentRequestStatus where we weren't blocking on the condition variable. - Make methods static where possible. - Clean up getUri implementation. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135373586 --- .../ext/cronet/CronetDataSource.java | 76 +++++++------------ 1 file changed, 29 insertions(+), 47 deletions(-) diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index 04b468291d..266feca6d6 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -37,12 +37,12 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.Executor; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.chromium.net.CronetEngine; import org.chromium.net.UrlRequest; +import org.chromium.net.UrlRequest.Status; import org.chromium.net.UrlRequestException; import org.chromium.net.UrlResponseInfo; @@ -112,7 +112,6 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou private UrlResponseInfo responseInfo; /* package */ volatile int connectionState; - private volatile String currentUrl; private volatile long currentConnectTimeoutMs; private volatile HttpDataSourceException exception; private volatile long contentLength; @@ -199,7 +198,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou @Override public Uri getUri() { - return Uri.parse(currentUrl); + return responseInfo == null ? null : Uri.parse(responseInfo.getUrl()); } @Override @@ -212,7 +211,14 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou operation.close(); resetConnectTimeout(); - startRequest(dataSpec); + + currentDataSpec = dataSpec; + UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(dataSpec.uri.toString(), this, + executor, cronetEngine); + fillCurrentRequestHeader(urlRequestBuilder); + fillCurrentRequestPostBody(urlRequestBuilder, dataSpec); + currentUrlRequest = urlRequestBuilder.build(); + currentUrlRequest.start(); boolean requestStarted = blockUntilConnectTimeout(); if (exception != null) { @@ -220,7 +226,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou throw exception; } else if (!requestStarted) { // The timeout was reached before the connection was opened. - throw new OpenException(new SocketTimeoutException(), dataSpec, getCurrentRequestStatus()); + throw new OpenException(new SocketTimeoutException(), dataSpec, getStatus(currentUrlRequest)); } // Connection was opened. @@ -285,7 +291,6 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou currentUrlRequest = null; } currentDataSpec = null; - currentUrl = null; exception = null; contentLength = 0; hasData = false; @@ -315,7 +320,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou // redirect is followed. if (responseCode == 307 || responseCode == 308) { exception = new OpenException("POST request redirected with 307 or 308 response code", - currentDataSpec, getCurrentRequestStatus()); + currentDataSpec, getStatus(request)); operation.open(); return; } @@ -333,9 +338,9 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou } try { validateResponse(info); - responseInfo = info; - if (isCompressed(info)) { + responseInfo = info; + if (getIsCompressed(info)) { contentLength = currentDataSpec.length; } else { // Check content length. @@ -346,16 +351,13 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou && contentLength != C.LENGTH_UNSET && currentDataSpec.length != contentLength) { throw new OpenException("Content length did not match requested length", currentDataSpec, - getCurrentRequestStatus()); + getStatus(request)); } } - if (contentLength > 0) { expectedBytesRemainingToRead = new AtomicLong(contentLength); } - // Keep track of redirects. - currentUrl = responseInfo.getUrl(); connectionState = CONNECTED_CONNECTION; } catch (HttpDataSourceException e) { exception = e; @@ -393,7 +395,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou if (connectionState == OPENING_CONNECTION) { IOException cause = error.getErrorCode() == UrlRequestException.ERROR_HOSTNAME_NOT_RESOLVED ? new UnknownHostException() : error; - exception = new OpenException(cause, currentDataSpec, getCurrentRequestStatus()); + exception = new OpenException(cause, currentDataSpec, getStatus(request)); } else if (connectionState == OPEN_CONNECTION) { exception = new HttpDataSourceException(error, currentDataSpec, HttpDataSourceException.TYPE_READ); @@ -428,24 +430,13 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou if (dataSpec.postBody != null) { if (!requestProperties.containsKey("Content-Type")) { throw new OpenException("POST requests must set a Content-Type header", dataSpec, - getCurrentRequestStatus()); + Status.IDLE); } urlRequestBuilder.setUploadDataProvider( new ByteArrayUploadDataProvider(dataSpec.postBody), executor); } } - private void startRequest(DataSpec dataSpec) throws HttpDataSourceException { - currentUrl = dataSpec.uri.toString(); - currentDataSpec = dataSpec; - UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(currentUrl, this, executor, - cronetEngine); - fillCurrentRequestHeader(urlRequestBuilder); - fillCurrentRequestPostBody(urlRequestBuilder, dataSpec); - currentUrlRequest = urlRequestBuilder.build(); - currentUrlRequest.start(); - } - private void validateResponse(UrlResponseInfo info) throws HttpDataSourceException { // Check for a valid response code. int responseCode = info.getHttpStatusCode(); @@ -482,17 +473,16 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou currentConnectTimeoutMs = clock.elapsedRealtime() + connectTimeoutMs; } - private boolean isCompressed(UrlResponseInfo info) { + private static boolean getIsCompressed(UrlResponseInfo info) { for (Map.Entry entry : info.getAllHeadersAsList()) { if (entry.getKey().equalsIgnoreCase("Content-Encoding")) { return !entry.getValue().equalsIgnoreCase("identity"); } } - return false; } - private long getContentLength(Map> headers) { + private static long getContentLength(Map> headers) { // Logic copied from {@code DefaultHttpDataSource} long contentLength = C.LENGTH_UNSET; List contentLengthHeader = headers.get("Content-Length"); @@ -502,7 +492,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou try { contentLength = Long.parseLong(contentLengthHeader.get(0)); } catch (NumberFormatException e) { - log(Log.ERROR, "Unexpected Content-Length [" + contentLengthHeader + "]"); + Log.e(TAG, "Unexpected Content-Length [" + contentLengthHeader + "]"); } } List contentRangeHeader = headers.get("Content-Range"); @@ -523,38 +513,30 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou // assume the one with the larger value is correct. We have seen cases where carrier // change one of them to reduce the size of a request, but it is unlikely anybody // would increase it. - log(Log.WARN, "Inconsistent headers [" + contentLengthHeader + "] [" - + contentRangeHeader + "]"); + Log.w(TAG, "Inconsistent headers [" + contentLengthHeader + "] [" + contentRangeHeader + + "]"); contentLength = Math.max(contentLength, contentLengthFromRange); } } catch (NumberFormatException e) { - log(Log.ERROR, "Unexpected Content-Range [" + contentRangeHeader + "]"); + Log.e(TAG, "Unexpected Content-Range [" + contentRangeHeader + "]"); } } } return contentLength; } - private int getCurrentRequestStatus() { - if (currentUrlRequest == null) { - return UrlRequest.Status.IDLE; - } + private static int getStatus(UrlRequest request) { final ConditionVariable conditionVariable = new ConditionVariable(); - final AtomicInteger result = new AtomicInteger(); - currentUrlRequest.getStatus(new UrlRequest.StatusListener() { + final int[] statusHolder = new int[1]; + request.getStatus(new UrlRequest.StatusListener() { @Override public void onStatus(int status) { - result.set(status); + statusHolder[0] = status; conditionVariable.open(); } }); - return result.get(); - } - - private void log(int priority, String message) { - if (Log.isLoggable(TAG, priority)) { - Log.println(priority, TAG, message); - } + conditionVariable.block(); + return statusHolder[0]; } } From 661b14020e02210f1bff436bc145c2c38663f115 Mon Sep 17 00:00:00 2001 From: olly Date: Thu, 6 Oct 2016 10:32:54 -0700 Subject: [PATCH 03/13] Partial cleanup of CronetDataSource II - Allow null Content-Type in response headers. - Inline validateResponse, just because it makes it clearer what thread it's being executed on when inlined. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135375063 --- .../ext/cronet/CronetDataSource.java | 59 +++++++++---------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index 266feca6d6..b563b70419 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -85,6 +85,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou public static final int DEFAULT_READ_TIMEOUT_MILLIS = 8 * 1000; private static final String TAG = "CronetDataSource"; + private static final String CONTENT_TYPE = "Content-Type"; private static final Pattern CONTENT_RANGE_HEADER_PATTERN = Pattern.compile("^bytes (\\d+)-(\\d+)/(\\d+)$"); // The size of read buffer passed to cronet UrlRequest.read(). @@ -337,7 +338,25 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return; } try { - validateResponse(info); + // Check for a valid response code. + int responseCode = info.getHttpStatusCode(); + if (responseCode < 200 || responseCode > 299) { + InvalidResponseCodeException exception = new InvalidResponseCodeException( + responseCode, info.getAllHeaders(), currentDataSpec); + if (responseCode == 416) { + exception.initCause(new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE)); + } + throw exception; + } + // Check for a valid content type. + if (contentTypePredicate != null) { + List contentTypeHeaders = info.getAllHeaders().get(CONTENT_TYPE); + String contentType = contentTypeHeaders == null || contentTypeHeaders.isEmpty() ? null + : contentTypeHeaders.get(0); + if (!contentTypePredicate.evaluate(contentType)) { + throw new InvalidContentTypeException(contentType, currentDataSpec); + } + } responseInfo = info; if (getIsCompressed(info)) { @@ -347,8 +366,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou contentLength = getContentLength(info.getAllHeaders()); // If a specific length is requested and a specific length is returned but the 2 don't match // it's an error. - if (currentDataSpec.length != C.LENGTH_UNSET - && contentLength != C.LENGTH_UNSET + if (currentDataSpec.length != C.LENGTH_UNSET && contentLength != C.LENGTH_UNSET && currentDataSpec.length != contentLength) { throw new OpenException("Content length did not match requested length", currentDataSpec, getStatus(request)); @@ -427,36 +445,14 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou private void fillCurrentRequestPostBody(UrlRequest.Builder urlRequestBuilder, DataSpec dataSpec) throws HttpDataSourceException { - if (dataSpec.postBody != null) { - if (!requestProperties.containsKey("Content-Type")) { - throw new OpenException("POST requests must set a Content-Type header", dataSpec, - Status.IDLE); - } - urlRequestBuilder.setUploadDataProvider( - new ByteArrayUploadDataProvider(dataSpec.postBody), executor); + if (dataSpec.postBody == null) { + return; } - } - - private void validateResponse(UrlResponseInfo info) throws HttpDataSourceException { - // Check for a valid response code. - int responseCode = info.getHttpStatusCode(); - if (responseCode < 200 || responseCode > 299) { - InvalidResponseCodeException exception = new InvalidResponseCodeException( - responseCode, info.getAllHeaders(), currentDataSpec); - if (responseCode == 416) { - exception.initCause(new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE)); - } - throw exception; - } - // Check for a valid content type. - try { - String contentType = info.getAllHeaders().get("Content-Type").get(0); - if (contentTypePredicate != null && !contentTypePredicate.evaluate(contentType)) { - throw new InvalidContentTypeException(contentType, currentDataSpec); - } - } catch (IndexOutOfBoundsException e) { - throw new InvalidContentTypeException(null, currentDataSpec); + if (!requestProperties.containsKey(CONTENT_TYPE)) { + throw new OpenException("POST request must set Content-Type", dataSpec, Status.IDLE); } + urlRequestBuilder.setUploadDataProvider(new ByteArrayUploadDataProvider(dataSpec.postBody), + executor); } private boolean blockUntilConnectTimeout() { @@ -483,7 +479,6 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou } private static long getContentLength(Map> headers) { - // Logic copied from {@code DefaultHttpDataSource} long contentLength = C.LENGTH_UNSET; List contentLengthHeader = headers.get("Content-Length"); if (contentLengthHeader != null From d95baa3ee95121b227497d0fe63a0481b7b8b0e8 Mon Sep 17 00:00:00 2001 From: olly Date: Thu, 6 Oct 2016 10:46:38 -0700 Subject: [PATCH 04/13] CronetDataSource: Fix thread safety issue with requestProperties The access in fillCurrentRequestPostBody wasn't protected with synchronization. Furthermore, just synchronizing it wouldn't be sufficient, since what we really need to check is whether the Content-Type header has been added to the UrlRequest.Builder. The contents of requestProperties may have changed between the headers being added to UrlRequest.Builder and the call to fillCurrentRequestPostBody. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135376904 --- .../ext/cronet/CronetDataSource.java | 54 +++++++++---------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index b563b70419..594ea8cb17 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -212,13 +212,8 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou operation.close(); resetConnectTimeout(); - currentDataSpec = dataSpec; - UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(dataSpec.uri.toString(), this, - executor, cronetEngine); - fillCurrentRequestHeader(urlRequestBuilder); - fillCurrentRequestPostBody(urlRequestBuilder, dataSpec); - currentUrlRequest = urlRequestBuilder.build(); + currentUrlRequest = buildRequest(dataSpec); currentUrlRequest.start(); boolean requestStarted = blockUntilConnectTimeout(); @@ -423,36 +418,35 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou // Internal methods. - private void fillCurrentRequestHeader(UrlRequest.Builder urlRequestBuilder) { + private UrlRequest buildRequest(DataSpec dataSpec) throws OpenException { + UrlRequest.Builder requestBuilder = new UrlRequest.Builder(dataSpec.uri.toString(), this, + executor, cronetEngine); + // Set the headers. synchronized (requestProperties) { + if (dataSpec.postBody != null && !requestProperties.containsKey(CONTENT_TYPE)) { + throw new OpenException("POST request must set Content-Type", dataSpec, Status.IDLE); + } for (Entry headerEntry : requestProperties.entrySet()) { - urlRequestBuilder.addHeader(headerEntry.getKey(), headerEntry.getValue()); + requestBuilder.addHeader(headerEntry.getKey(), headerEntry.getValue()); } } - if (currentDataSpec.position == 0 && currentDataSpec.length == C.LENGTH_UNSET) { - // Not required. - return; + // Set the Range header. + if (currentDataSpec.position != 0 || currentDataSpec.length != C.LENGTH_UNSET) { + StringBuilder rangeValue = new StringBuilder(); + rangeValue.append("bytes="); + rangeValue.append(currentDataSpec.position); + rangeValue.append("-"); + if (currentDataSpec.length != C.LENGTH_UNSET) { + rangeValue.append(currentDataSpec.position + currentDataSpec.length - 1); + } + requestBuilder.addHeader("Range", rangeValue.toString()); } - StringBuilder rangeValue = new StringBuilder(); - rangeValue.append("bytes="); - rangeValue.append(currentDataSpec.position); - rangeValue.append("-"); - if (currentDataSpec.length != C.LENGTH_UNSET) { - rangeValue.append(currentDataSpec.position + currentDataSpec.length - 1); + // Set the body. + if (dataSpec.postBody != null) { + requestBuilder.setUploadDataProvider(new ByteArrayUploadDataProvider(dataSpec.postBody), + executor); } - urlRequestBuilder.addHeader("Range", rangeValue.toString()); - } - - private void fillCurrentRequestPostBody(UrlRequest.Builder urlRequestBuilder, DataSpec dataSpec) - throws HttpDataSourceException { - if (dataSpec.postBody == null) { - return; - } - if (!requestProperties.containsKey(CONTENT_TYPE)) { - throw new OpenException("POST request must set Content-Type", dataSpec, Status.IDLE); - } - urlRequestBuilder.setUploadDataProvider(new ByteArrayUploadDataProvider(dataSpec.postBody), - executor); + return requestBuilder.build(); } private boolean blockUntilConnectTimeout() { From 97020e0bab87d8f7321c3f29ba4041904e227426 Mon Sep 17 00:00:00 2001 From: olly Date: Thu, 6 Oct 2016 10:55:09 -0700 Subject: [PATCH 05/13] CronetDataSource: Fix getContentLength logging This is a minor cleanup. The main thing it fixes is that the "Inconsistent headers" and "Unexpected Content-Range" log messages were printing List objects, rather than the actual headers they are supposed to print. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135378074 --- .../ext/cronet/CronetDataSource.java | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index 594ea8cb17..8e53e32331 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -358,7 +358,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou contentLength = currentDataSpec.length; } else { // Check content length. - contentLength = getContentLength(info.getAllHeaders()); + contentLength = getContentLength(info); // If a specific length is requested and a specific length is returned but the 2 don't match // it's an error. if (currentDataSpec.length != C.LENGTH_UNSET && contentLength != C.LENGTH_UNSET @@ -472,23 +472,25 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return false; } - private static long getContentLength(Map> headers) { + private static long getContentLength(UrlResponseInfo info) { long contentLength = C.LENGTH_UNSET; - List contentLengthHeader = headers.get("Content-Length"); - if (contentLengthHeader != null - && !contentLengthHeader.isEmpty() - && !TextUtils.isEmpty(contentLengthHeader.get(0))) { - try { - contentLength = Long.parseLong(contentLengthHeader.get(0)); - } catch (NumberFormatException e) { - Log.e(TAG, "Unexpected Content-Length [" + contentLengthHeader + "]"); + Map> headers = info.getAllHeaders(); + List contentLengthHeaders = headers.get("Content-Length"); + String contentLengthHeader = null; + if (contentLengthHeaders != null && !contentLengthHeaders.isEmpty()) { + contentLengthHeader = contentLengthHeaders.get(0); + if (!TextUtils.isEmpty(contentLengthHeader)) { + try { + contentLength = Long.parseLong(contentLengthHeader); + } catch (NumberFormatException e) { + Log.e(TAG, "Unexpected Content-Length [" + contentLengthHeader + "]"); + } } } - List contentRangeHeader = headers.get("Content-Range"); - if (contentRangeHeader != null - && !contentRangeHeader.isEmpty() - && !TextUtils.isEmpty(contentRangeHeader.get(0))) { - Matcher matcher = CONTENT_RANGE_HEADER_PATTERN.matcher(contentRangeHeader.get(0)); + List contentRangeHeaders = headers.get("Content-Range"); + if (contentRangeHeaders != null && !contentRangeHeaders.isEmpty()) { + String contentRangeHeader = contentRangeHeaders.get(0); + Matcher matcher = CONTENT_RANGE_HEADER_PATTERN.matcher(contentRangeHeader); if (matcher.find()) { try { long contentLengthFromRange = From 91f8328a5f1675a25d6546d4c79bcfdc175a535c Mon Sep 17 00:00:00 2001 From: olly Date: Thu, 28 Jul 2016 08:48:37 +0100 Subject: [PATCH 06/13] UI component improvements - Make sure no events are posted on PlaybackControlView if it's not attached to a window. This can cause leaks. The target hide time is recorded if necessary and processed when the view is re-attached. - Deduplicated PlaybackControlView.VisibilityListener invocations. - Fixed timeouts to be more intuitive (I think). - Fixed initial visibility of PlaybackControlView when used as part of SimpleExoPlayerView. - Made some more attributes configurable from layout xml. Issue: #1908 Issue: #1919 Issue: #1923 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135679988 --- .../exoplayer2/ui/PlaybackControlView.java | 144 +++++++++++++----- .../exoplayer2/ui/SimpleExoPlayerView.java | 119 ++++++++++----- library/src/main/res/values/attrs.xml | 12 ++ 3 files changed, 199 insertions(+), 76 deletions(-) diff --git a/library/src/main/java/com/google/android/exoplayer2/ui/PlaybackControlView.java b/library/src/main/java/com/google/android/exoplayer2/ui/PlaybackControlView.java index 13c5d14df0..3823f1760e 100644 --- a/library/src/main/java/com/google/android/exoplayer2/ui/PlaybackControlView.java +++ b/library/src/main/java/com/google/android/exoplayer2/ui/PlaybackControlView.java @@ -16,6 +16,8 @@ package com.google.android.exoplayer2.ui; import android.content.Context; +import android.content.res.TypedArray; +import android.os.SystemClock; import android.util.AttributeSet; import android.view.KeyEvent; import android.view.LayoutInflater; @@ -52,7 +54,7 @@ public class PlaybackControlView extends FrameLayout { public static final int DEFAULT_FAST_FORWARD_MS = 15000; public static final int DEFAULT_REWIND_MS = 5000; - public static final int DEFAULT_SHOW_DURATION_MS = 5000; + public static final int DEFAULT_SHOW_TIMEOUT_MS = 5000; private static final int PROGRESS_BAR_MAX = 1000; private static final long MAX_POSITION_FOR_SEEK_TO_PREVIOUS = 3000; @@ -74,9 +76,10 @@ public class PlaybackControlView extends FrameLayout { private VisibilityListener visibilityListener; private boolean dragging; - private int rewindMs = DEFAULT_REWIND_MS; - private int fastForwardMs = DEFAULT_FAST_FORWARD_MS; - private int showDurationMs = DEFAULT_SHOW_DURATION_MS; + private int rewindMs; + private int fastForwardMs; + private int showTimeoutMs; + private long hideAtMs; private final Runnable updateProgressAction = new Runnable() { @Override @@ -103,6 +106,22 @@ public class PlaybackControlView extends FrameLayout { public PlaybackControlView(Context context, AttributeSet attrs, int defStyleAttr) { super(context, attrs, defStyleAttr); + rewindMs = DEFAULT_REWIND_MS; + fastForwardMs = DEFAULT_FAST_FORWARD_MS; + showTimeoutMs = DEFAULT_SHOW_TIMEOUT_MS; + if (attrs != null) { + TypedArray a = context.getTheme().obtainStyledAttributes(attrs, + R.styleable.PlaybackControlView, 0, 0); + try { + rewindMs = a.getInt(R.styleable.PlaybackControlView_rewind_increment, rewindMs); + fastForwardMs = a.getInt(R.styleable.PlaybackControlView_fastforward_increment, + fastForwardMs); + showTimeoutMs = a.getInt(R.styleable.PlaybackControlView_show_timeout, showTimeoutMs); + } finally { + a.recycle(); + } + } + currentWindow = new Timeline.Window(); formatBuilder = new StringBuilder(); formatter = new Formatter(formatBuilder, Locale.getDefault()); @@ -124,7 +143,6 @@ public class PlaybackControlView extends FrameLayout { rewindButton.setOnClickListener(componentListener); fastForwardButton = findViewById(R.id.ffwd); fastForwardButton.setOnClickListener(componentListener); - updateAll(); } /** @@ -169,6 +187,7 @@ public class PlaybackControlView extends FrameLayout { */ public void setRewindIncrementMs(int rewindMs) { this.rewindMs = rewindMs; + updateNavigation(); } /** @@ -178,51 +197,60 @@ public class PlaybackControlView extends FrameLayout { */ public void setFastForwardIncrementMs(int fastForwardMs) { this.fastForwardMs = fastForwardMs; + updateNavigation(); } /** - * Sets the duration to show the playback control in milliseconds. + * Returns the playback controls timeout. The playback controls are automatically hidden after + * this duration of time has elapsed without user input. * - * @param showDurationMs The duration in milliseconds. + * @return The duration in milliseconds. A non-positive value indicates that the controls will + * remain visible indefinitely. */ - public void setShowDurationMs(int showDurationMs) { - this.showDurationMs = showDurationMs; + public int getShowTimeoutMs() { + return showTimeoutMs; } /** - * Shows the controller for the duration last passed to {@link #setShowDurationMs(int)}, or for - * {@link #DEFAULT_SHOW_DURATION_MS} if {@link #setShowDurationMs(int)} has not been called. + * Sets the playback controls timeout. The playback controls are automatically hidden after this + * duration of time has elapsed without user input. + * + * @param showTimeoutMs The duration in milliseconds. A non-positive value will cause the controls + * to remain visible indefinitely. + */ + public void setShowTimeoutMs(int showTimeoutMs) { + this.showTimeoutMs = showTimeoutMs; + } + + /** + * Shows the playback controls. If {@link #getShowTimeoutMs()} is positive then the controls will + * be automatically hidden after this duration of time has elapsed without user input. */ public void show() { - show(showDurationMs); - } - - /** - * Shows the controller for the {@code durationMs}. If {@code durationMs} is 0 the controller is - * shown until {@link #hide()} is called. - * - * @param durationMs The duration in milliseconds. - */ - public void show(int durationMs) { - setVisibility(VISIBLE); - if (visibilityListener != null) { - visibilityListener.onVisibilityChange(getVisibility()); + if (!isVisible()) { + setVisibility(VISIBLE); + if (visibilityListener != null) { + visibilityListener.onVisibilityChange(getVisibility()); + } + updateAll(); } - updateAll(); - showDurationMs = durationMs; - hideDeferred(); + // Call hideAfterTimeout even if already visible to reset the timeout. + hideAfterTimeout(); } /** * Hides the controller. */ public void hide() { - setVisibility(GONE); - if (visibilityListener != null) { - visibilityListener.onVisibilityChange(getVisibility()); + if (isVisible()) { + setVisibility(GONE); + if (visibilityListener != null) { + visibilityListener.onVisibilityChange(getVisibility()); + } + removeCallbacks(updateProgressAction); + removeCallbacks(hideAction); + hideAtMs = C.TIME_UNSET; } - removeCallbacks(updateProgressAction); - removeCallbacks(hideAction); } /** @@ -232,10 +260,15 @@ public class PlaybackControlView extends FrameLayout { return getVisibility() == VISIBLE; } - private void hideDeferred() { + private void hideAfterTimeout() { removeCallbacks(hideAction); - if (showDurationMs > 0) { - postDelayed(hideAction, showDurationMs); + if (showTimeoutMs > 0) { + hideAtMs = SystemClock.uptimeMillis() + showTimeoutMs; + if (isAttachedToWindow()) { + postDelayed(hideAction, showTimeoutMs); + } + } else { + hideAtMs = C.TIME_UNSET; } } @@ -246,7 +279,7 @@ public class PlaybackControlView extends FrameLayout { } private void updatePlayPauseButton() { - if (!isVisible()) { + if (!isVisible() || !isAttachedToWindow()) { return; } boolean playing = player != null && player.getPlayWhenReady(); @@ -258,7 +291,7 @@ public class PlaybackControlView extends FrameLayout { } private void updateNavigation() { - if (!isVisible()) { + if (!isVisible() || !isAttachedToWindow()) { return; } Timeline currentTimeline = player != null ? player.getCurrentTimeline() : null; @@ -276,13 +309,13 @@ public class PlaybackControlView extends FrameLayout { } setButtonEnabled(enablePrevious , previousButton); setButtonEnabled(enableNext, nextButton); - setButtonEnabled(isSeekable, fastForwardButton); - setButtonEnabled(isSeekable, rewindButton); + setButtonEnabled(fastForwardMs > 0 && isSeekable, fastForwardButton); + setButtonEnabled(rewindMs > 0 && isSeekable, rewindButton); progressBar.setEnabled(isSeekable); } private void updateProgress() { - if (!isVisible()) { + if (!isVisible() || !isAttachedToWindow()) { return; } long duration = player == null ? 0 : player.getDuration(); @@ -377,13 +410,40 @@ public class PlaybackControlView extends FrameLayout { } private void rewind() { + if (rewindMs <= 0) { + return; + } player.seekTo(Math.max(player.getCurrentPosition() - rewindMs, 0)); } private void fastForward() { + if (fastForwardMs <= 0) { + return; + } player.seekTo(Math.min(player.getCurrentPosition() + fastForwardMs, player.getDuration())); } + @Override + public void onAttachedToWindow() { + super.onAttachedToWindow(); + if (hideAtMs != C.TIME_UNSET) { + long delayMs = hideAtMs - SystemClock.uptimeMillis(); + if (delayMs <= 0) { + hide(); + } else { + postDelayed(hideAction, delayMs); + } + } + updateAll(); + } + + @Override + public void onDetachedFromWindow() { + super.onDetachedFromWindow(); + removeCallbacks(updateProgressAction); + removeCallbacks(hideAction); + } + @Override public boolean dispatchKeyEvent(KeyEvent event) { if (player == null || event.getAction() != KeyEvent.ACTION_DOWN) { @@ -440,7 +500,7 @@ public class PlaybackControlView extends FrameLayout { public void onStopTrackingTouch(SeekBar seekBar) { dragging = false; player.seekTo(positionValue(seekBar.getProgress())); - hideDeferred(); + hideAfterTimeout(); } @Override @@ -485,7 +545,7 @@ public class PlaybackControlView extends FrameLayout { } else if (playButton == view) { player.setPlayWhenReady(!player.getPlayWhenReady()); } - hideDeferred(); + hideAfterTimeout(); } } diff --git a/library/src/main/java/com/google/android/exoplayer2/ui/SimpleExoPlayerView.java b/library/src/main/java/com/google/android/exoplayer2/ui/SimpleExoPlayerView.java index cd0acb77fa..51955ccef3 100644 --- a/library/src/main/java/com/google/android/exoplayer2/ui/SimpleExoPlayerView.java +++ b/library/src/main/java/com/google/android/exoplayer2/ui/SimpleExoPlayerView.java @@ -48,8 +48,10 @@ public final class SimpleExoPlayerView extends FrameLayout { private final AspectRatioFrameLayout layout; private final PlaybackControlView controller; private final ComponentListener componentListener; + private SimpleExoPlayer player; private boolean useController = true; + private int controllerShowTimeoutMs; public SimpleExoPlayerView(Context context) { this(context, null); @@ -64,6 +66,9 @@ public final class SimpleExoPlayerView extends FrameLayout { boolean useTextureView = false; int resizeMode = AspectRatioFrameLayout.RESIZE_MODE_FIT; + int rewindMs = PlaybackControlView.DEFAULT_REWIND_MS; + int fastForwardMs = PlaybackControlView.DEFAULT_FAST_FORWARD_MS; + int controllerShowTimeoutMs = PlaybackControlView.DEFAULT_SHOW_TIMEOUT_MS; if (attrs != null) { TypedArray a = context.getTheme().obtainStyledAttributes(attrs, R.styleable.SimpleExoPlayerView, 0, 0); @@ -73,6 +78,11 @@ public final class SimpleExoPlayerView extends FrameLayout { useTextureView); resizeMode = a.getInt(R.styleable.SimpleExoPlayerView_resize_mode, AspectRatioFrameLayout.RESIZE_MODE_FIT); + rewindMs = a.getInt(R.styleable.SimpleExoPlayerView_rewind_increment, rewindMs); + fastForwardMs = a.getInt(R.styleable.SimpleExoPlayerView_fastforward_increment, + fastForwardMs); + controllerShowTimeoutMs = a.getInt(R.styleable.SimpleExoPlayerView_show_timeout, + controllerShowTimeoutMs); } finally { a.recycle(); } @@ -82,12 +92,17 @@ public final class SimpleExoPlayerView extends FrameLayout { componentListener = new ComponentListener(); layout = (AspectRatioFrameLayout) findViewById(R.id.video_frame); layout.setResizeMode(resizeMode); - controller = (PlaybackControlView) findViewById(R.id.control); shutterView = findViewById(R.id.shutter); subtitleLayout = (SubtitleView) findViewById(R.id.subtitles); subtitleLayout.setUserDefaultStyle(); subtitleLayout.setUserDefaultTextSize(); + controller = (PlaybackControlView) findViewById(R.id.control); + controller.hide(); + controller.setRewindIncrementMs(rewindMs); + controller.setFastForwardIncrementMs(fastForwardMs); + this.controllerShowTimeoutMs = controllerShowTimeoutMs; + View view = useTextureView ? new TextureView(context) : new SurfaceView(context); ViewGroup.LayoutParams params = new ViewGroup.LayoutParams( ViewGroup.LayoutParams.MATCH_PARENT, @@ -122,6 +137,9 @@ public final class SimpleExoPlayerView extends FrameLayout { this.player.setVideoSurface(null); } this.player = player; + if (useController) { + controller.setPlayer(player); + } if (player != null) { if (surfaceView instanceof TextureView) { player.setVideoTextureView((TextureView) surfaceView); @@ -131,20 +149,36 @@ public final class SimpleExoPlayerView extends FrameLayout { player.setVideoListener(componentListener); player.addListener(componentListener); player.setTextOutput(componentListener); + maybeShowController(false); } else { shutterView.setVisibility(VISIBLE); - } - if (useController) { - controller.setPlayer(player); + controller.hide(); } } /** - * Set the {@code useController} flag which indicates whether the playback control view should - * be used or not. If set to {@code false} the controller is never visible and is disconnected - * from the player. + * Sets the resize mode which can be of value {@link AspectRatioFrameLayout#RESIZE_MODE_FIT}, + * {@link AspectRatioFrameLayout#RESIZE_MODE_FIXED_HEIGHT} or + * {@link AspectRatioFrameLayout#RESIZE_MODE_FIXED_WIDTH}. * - * @param useController If {@code false} the playback control is never used. + * @param resizeMode The resize mode. + */ + public void setResizeMode(int resizeMode) { + layout.setResizeMode(resizeMode); + } + + /** + * Returns whether the playback controls are enabled. + */ + public boolean getUseController() { + return useController; + } + + /** + * Sets whether playback controls are enabled. If set to {@code false} the playback controls are + * never visible and are disconnected from the player. + * + * @param useController Whether playback controls should be enabled. */ public void setUseController(boolean useController) { if (this.useController == useController) { @@ -160,14 +194,26 @@ public final class SimpleExoPlayerView extends FrameLayout { } /** - * Sets the resize mode which can be of value {@link AspectRatioFrameLayout#RESIZE_MODE_FIT}, - * {@link AspectRatioFrameLayout#RESIZE_MODE_FIXED_HEIGHT} or - * {@link AspectRatioFrameLayout#RESIZE_MODE_FIXED_WIDTH}. + * Returns the playback controls timeout. The playback controls are automatically hidden after + * this duration of time has elapsed without user input and with playback or buffering in + * progress. * - * @param resizeMode The resize mode. + * @return The timeout in milliseconds. A non-positive value will cause the controller to remain + * visible indefinitely. */ - public void setResizeMode(int resizeMode) { - layout.setResizeMode(resizeMode); + public int getControllerShowTimeoutMs() { + return controllerShowTimeoutMs; + } + + /** + * Sets the playback controls timeout. The playback controls are automatically hidden after this + * duration of time has elapsed without user input and with playback or buffering in progress. + * + * @param controllerShowTimeoutMs The timeout in milliseconds. A non-positive value will cause + * the controller to remain visible indefinitely. + */ + public void setControllerShowTimeoutMs(int controllerShowTimeoutMs) { + this.controllerShowTimeoutMs = controllerShowTimeoutMs; } /** @@ -197,15 +243,6 @@ public final class SimpleExoPlayerView extends FrameLayout { controller.setFastForwardIncrementMs(fastForwardMs); } - /** - * Sets the duration to show the playback control in milliseconds. - * - * @param showDurationMs The duration in milliseconds. - */ - public void setControlShowDurationMs(int showDurationMs) { - controller.setShowDurationMs(showDurationMs); - } - /** * Get the view onto which video is rendered. This is either a {@link SurfaceView} (default) * or a {@link TextureView} if the {@code use_texture_view} view attribute has been set to true. @@ -218,21 +255,23 @@ public final class SimpleExoPlayerView extends FrameLayout { @Override public boolean onTouchEvent(MotionEvent ev) { - if (useController && ev.getActionMasked() == MotionEvent.ACTION_DOWN) { - if (controller.isVisible()) { - controller.hide(); - } else { - controller.show(); - } + if (!useController || player == null || ev.getActionMasked() != MotionEvent.ACTION_DOWN) { + return false; + } + if (controller.isVisible()) { + controller.hide(); + } else { + maybeShowController(true); } return true; } + @Override public boolean onTrackballEvent(MotionEvent ev) { - if (!useController) { + if (!useController || player == null) { return false; } - controller.show(); + maybeShowController(true); return true; } @@ -241,6 +280,20 @@ public final class SimpleExoPlayerView extends FrameLayout { return useController ? controller.dispatchKeyEvent(event) : super.dispatchKeyEvent(event); } + private void maybeShowController(boolean isForced) { + if (!useController || player == null) { + return; + } + int playbackState = player.getPlaybackState(); + boolean showIndefinitely = playbackState == ExoPlayer.STATE_IDLE + || playbackState == ExoPlayer.STATE_ENDED || !player.getPlayWhenReady(); + boolean wasShowingIndefinitely = controller.isVisible() && controller.getShowTimeoutMs() <= 0; + controller.setShowTimeoutMs(showIndefinitely ? 0 : controllerShowTimeoutMs); + if (isForced || showIndefinitely || wasShowingIndefinitely) { + controller.show(); + } + } + private final class ComponentListener implements SimpleExoPlayer.VideoListener, TextRenderer.Output, ExoPlayer.EventListener { @@ -278,9 +331,7 @@ public final class SimpleExoPlayerView extends FrameLayout { @Override public void onPlayerStateChanged(boolean playWhenReady, int playbackState) { - if (useController && playbackState == ExoPlayer.STATE_ENDED) { - controller.show(0); - } + maybeShowController(false); } @Override diff --git a/library/src/main/res/values/attrs.xml b/library/src/main/res/values/attrs.xml index 5f39dcb6c4..d58882c0aa 100644 --- a/library/src/main/res/values/attrs.xml +++ b/library/src/main/res/values/attrs.xml @@ -20,10 +20,16 @@ + + + + + + @@ -31,4 +37,10 @@ + + + + + + From 1b81cfdb56b4ca1a56d8ca0427e6be6ed7068a62 Mon Sep 17 00:00:00 2001 From: eguven Date: Mon, 10 Oct 2016 09:09:47 -0700 Subject: [PATCH 07/13] Rename build flavors and remove pre hashing of DataSpec key values. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135682130 --- demo/build.gradle | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/demo/build.gradle b/demo/build.gradle index 593796bda5..bfbcd1aa4c 100644 --- a/demo/build.gradle +++ b/demo/build.gradle @@ -38,15 +38,15 @@ android { } productFlavors { - demo - demoExt + noExtensions + withExtensions } } dependencies { compile project(':library') - demoExtCompile project(path: ':extension-ffmpeg') - demoExtCompile project(path: ':extension-flac') - demoExtCompile project(path: ':extension-opus') - demoExtCompile project(path: ':extension-vp9') + withExtensionsCompile project(path: ':extension-ffmpeg') + withExtensionsCompile project(path: ':extension-flac') + withExtensionsCompile project(path: ':extension-opus') + withExtensionsCompile project(path: ':extension-vp9') } From d922a21160ebd8edd351e1d15e1b727e1768e0b2 Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 10 Oct 2016 10:44:13 -0700 Subject: [PATCH 08/13] Select the cenc sinf atom explicitly ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135692709 --- .../exoplayer2/extractor/mp4/AtomParsers.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/library/src/main/java/com/google/android/exoplayer2/extractor/mp4/AtomParsers.java b/library/src/main/java/com/google/android/exoplayer2/extractor/mp4/AtomParsers.java index 0ca0f216c6..0b2d5ec330 100644 --- a/library/src/main/java/com/google/android/exoplayer2/extractor/mp4/AtomParsers.java +++ b/library/src/main/java/com/google/android/exoplayer2/extractor/mp4/AtomParsers.java @@ -47,6 +47,7 @@ import java.util.List; private static final int TYPE_sbtl = Util.getIntegerCodeForString("sbtl"); private static final int TYPE_subt = Util.getIntegerCodeForString("subt"); private static final int TYPE_clcp = Util.getIntegerCodeForString("clcp"); + private static final int TYPE_cenc = Util.getIntegerCodeForString("cenc"); /** * Parses a trak atom (defined in 14496-12). @@ -1004,7 +1005,7 @@ import java.util.List; /** * Parses encryption data from an audio/video sample entry, populating {@code out} and returning - * the unencrypted atom type, or 0 if no sinf atom was present. + * the unencrypted atom type, or 0 if no common encryption sinf atom was present. */ private static int parseSampleEntryEncryptionData(ParsableByteArray parent, int position, int size, StsdData out, int entryIndex) { @@ -1017,10 +1018,10 @@ import java.util.List; if (childAtomType == Atom.TYPE_sinf) { Pair result = parseSinfFromParent(parent, childPosition, childAtomSize); - Integer dataFormat = result.first; - Assertions.checkArgument(dataFormat != null, "frma atom is mandatory"); - out.trackEncryptionBoxes[entryIndex] = result.second; - return dataFormat; + if (result != null) { + out.trackEncryptionBoxes[entryIndex] = result.second; + return result.first; + } } childPosition += childAtomSize; } @@ -1032,6 +1033,7 @@ import java.util.List; int position, int size) { int childPosition = position + Atom.HEADER_SIZE; + boolean isCencScheme = false; TrackEncryptionBox trackEncryptionBox = null; Integer dataFormat = null; while (childPosition - position < size) { @@ -1042,15 +1044,20 @@ import java.util.List; dataFormat = parent.readInt(); } else if (childAtomType == Atom.TYPE_schm) { parent.skipBytes(4); - parent.readInt(); // schemeType. Expect cenc - parent.readInt(); // schemeVersion. Expect 0x00010000 + isCencScheme = parent.readInt() == TYPE_cenc; } else if (childAtomType == Atom.TYPE_schi) { trackEncryptionBox = parseSchiFromParent(parent, childPosition, childAtomSize); } childPosition += childAtomSize; } - return Pair.create(dataFormat, trackEncryptionBox); + if (isCencScheme) { + Assertions.checkArgument(dataFormat != null, "frma atom is mandatory"); + Assertions.checkArgument(trackEncryptionBox != null, "schi->tenc atom is mandatory"); + return Pair.create(dataFormat, trackEncryptionBox); + } else { + return null; + } } private static TrackEncryptionBox parseSchiFromParent(ParsableByteArray parent, int position, From 4fab402274c8bc9e8f495493868842cce05d0eff Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 10 Oct 2016 11:40:08 -0700 Subject: [PATCH 09/13] Fix missing generics ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135700280 --- .../android/exoplayer2/ExoPlayerFactory.java | 19 ++++++++++--------- .../android/exoplayer2/ExoPlayerImpl.java | 9 +++++---- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerFactory.java b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerFactory.java index d8bbd81dd8..91ab56805a 100644 --- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerFactory.java +++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerFactory.java @@ -18,6 +18,7 @@ package com.google.android.exoplayer2; import android.content.Context; import android.os.Looper; import com.google.android.exoplayer2.drm.DrmSessionManager; +import com.google.android.exoplayer2.drm.FrameworkMediaCrypto; import com.google.android.exoplayer2.trackselection.TrackSelector; /** @@ -41,7 +42,7 @@ public final class ExoPlayerFactory { * @param trackSelector The {@link TrackSelector} that will be used by the instance. * @param loadControl The {@link LoadControl} that will be used by the instance. */ - public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, + public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, LoadControl loadControl) { return newSimpleInstance(context, trackSelector, loadControl, null); } @@ -56,8 +57,8 @@ public final class ExoPlayerFactory { * @param drmSessionManager An optional {@link DrmSessionManager}. May be null if the instance * will not be used for DRM protected playbacks. */ - public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, - LoadControl loadControl, DrmSessionManager drmSessionManager) { + public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, + LoadControl loadControl, DrmSessionManager drmSessionManager) { return newSimpleInstance(context, trackSelector, loadControl, drmSessionManager, false); } @@ -74,8 +75,8 @@ public final class ExoPlayerFactory { * available extensions over those defined in the core library. Note that extensions must be * included in the application build for setting this flag to have any effect. */ - public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, - LoadControl loadControl, DrmSessionManager drmSessionManager, + public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, + LoadControl loadControl, DrmSessionManager drmSessionManager, boolean preferExtensionDecoders) { return newSimpleInstance(context, trackSelector, loadControl, drmSessionManager, preferExtensionDecoders, DEFAULT_ALLOWED_VIDEO_JOINING_TIME_MS); @@ -96,8 +97,8 @@ public final class ExoPlayerFactory { * @param allowedVideoJoiningTimeMs The maximum duration for which a video renderer can attempt to * seamlessly join an ongoing playback. */ - public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, - LoadControl loadControl, DrmSessionManager drmSessionManager, + public static SimpleExoPlayer newSimpleInstance(Context context, TrackSelector trackSelector, + LoadControl loadControl, DrmSessionManager drmSessionManager, boolean preferExtensionDecoders, long allowedVideoJoiningTimeMs) { return new SimpleExoPlayer(context, trackSelector, loadControl, drmSessionManager, preferExtensionDecoders, allowedVideoJoiningTimeMs); @@ -110,7 +111,7 @@ public final class ExoPlayerFactory { * @param renderers The {@link Renderer}s that will be used by the instance. * @param trackSelector The {@link TrackSelector} that will be used by the instance. */ - public static ExoPlayer newInstance(Renderer[] renderers, TrackSelector trackSelector) { + public static ExoPlayer newInstance(Renderer[] renderers, TrackSelector trackSelector) { return newInstance(renderers, trackSelector, new DefaultLoadControl()); } @@ -122,7 +123,7 @@ public final class ExoPlayerFactory { * @param trackSelector The {@link TrackSelector} that will be used by the instance. * @param loadControl The {@link LoadControl} that will be used by the instance. */ - public static ExoPlayer newInstance(Renderer[] renderers, TrackSelector trackSelector, + public static ExoPlayer newInstance(Renderer[] renderers, TrackSelector trackSelector, LoadControl loadControl) { return new ExoPlayerImpl(renderers, trackSelector, loadControl); } diff --git a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java index d1f72ed059..a79ad1dba0 100644 --- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java +++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java @@ -35,7 +35,7 @@ import java.util.concurrent.CopyOnWriteArraySet; private static final String TAG = "ExoPlayerImpl"; private final Handler eventHandler; - private final ExoPlayerImplInternal internalPlayer; + private final ExoPlayerImplInternal internalPlayer; private final CopyOnWriteArraySet listeners; private final Timeline.Window window; private final Timeline.Period period; @@ -63,7 +63,8 @@ import java.util.concurrent.CopyOnWriteArraySet; * @param loadControl The {@link LoadControl} that will be used by the instance. */ @SuppressLint("HandlerLeak") - public ExoPlayerImpl(Renderer[] renderers, TrackSelector trackSelector, LoadControl loadControl) { + public ExoPlayerImpl(Renderer[] renderers, TrackSelector trackSelector, + LoadControl loadControl) { Log.i(TAG, "Init " + ExoPlayerLibraryInfo.VERSION); Assertions.checkNotNull(renderers); Assertions.checkState(renderers.length > 0); @@ -79,8 +80,8 @@ import java.util.concurrent.CopyOnWriteArraySet; } }; playbackInfo = new ExoPlayerImplInternal.PlaybackInfo(0, 0); - internalPlayer = new ExoPlayerImplInternal(renderers, trackSelector, loadControl, playWhenReady, - eventHandler, playbackInfo); + internalPlayer = new ExoPlayerImplInternal<>(renderers, trackSelector, loadControl, + playWhenReady, eventHandler, playbackInfo); } @Override From 907b9bf4f65aef81ee12cb06840fd3fac555ed77 Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 10 Oct 2016 11:55:09 -0700 Subject: [PATCH 10/13] Sanitize threading in CronetDataSource - Move nearly all logic onto the calling thread (i.e. the thread calling open/read/close), to make threading correctness more obvious. - Document which variables are read/written from which thread, and why the call sequences are safe. - Fix thread safety issue that I think could probably cause data corruption in the case of a read timeout followed by another request into the DataSource. Also: - Relaxed content length checking to be consistent with the other http DataSource implementations, and avoided parsing the headers where they're not used. - Fixed missing generics in CronetDataSourceFactory. - Added TODO to work with servers that don't support partial range requests. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135702217 --- .../ext/cronet/CronetDataSourceTest.java | 52 +---- .../ext/cronet/CronetDataSource.java | 219 +++++++++--------- .../ext/cronet/CronetDataSourceFactory.java | 2 +- .../ext/okhttp/OkHttpDataSource.java | 9 +- .../upstream/DefaultHttpDataSource.java | 11 +- 5 files changed, 120 insertions(+), 173 deletions(-) diff --git a/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java b/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java index ccd4dec191..dcc5bc9b97 100644 --- a/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java +++ b/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java @@ -174,10 +174,7 @@ public final class CronetDataSourceTest { @Test(expected = IllegalStateException.class) public void testOpeningTwiceThrows() throws HttpDataSourceException { mockResponseStartSuccess(); - - assertConnectionState(CronetDataSource.IDLE_CONNECTION); dataSourceUnderTest.open(testDataSpec); - assertConnectionState(CronetDataSource.OPEN_CONNECTION); dataSourceUnderTest.open(testDataSpec); } @@ -205,7 +202,7 @@ public final class CronetDataSourceTest { dataSourceUnderTest.onFailed( mockUrlRequest, testUrlResponseInfo, - null); + mockUrlRequestException); dataSourceUnderTest.onResponseStarted( mockUrlRequest2, testUrlResponseInfo); @@ -253,13 +250,10 @@ public final class CronetDataSourceTest { @Test public void testRequestOpen() throws HttpDataSourceException { mockResponseStartSuccess(); - assertEquals(TEST_CONTENT_LENGTH, dataSourceUnderTest.open(testDataSpec)); - assertConnectionState(CronetDataSource.OPEN_CONNECTION); verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec); } - @Test public void testRequestOpenGzippedCompressedReturnsDataSpecLength() throws HttpDataSourceException { @@ -271,7 +265,6 @@ public final class CronetDataSourceTest { testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null); assertEquals(5000 /* contentLength */, dataSourceUnderTest.open(testDataSpec)); - assertConnectionState(CronetDataSource.OPEN_CONNECTION); verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec); } @@ -286,7 +279,6 @@ public final class CronetDataSourceTest { // Check for connection not automatically closed. assertFalse(e.getCause() instanceof UnknownHostException); verify(mockUrlRequest, never()).cancel(); - assertConnectionState(CronetDataSource.OPENING_CONNECTION); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); } } @@ -304,7 +296,6 @@ public final class CronetDataSourceTest { // Check for connection not automatically closed. assertTrue(e.getCause() instanceof UnknownHostException); verify(mockUrlRequest, never()).cancel(); - assertConnectionState(CronetDataSource.OPENING_CONNECTION); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); } } @@ -321,7 +312,6 @@ public final class CronetDataSourceTest { assertTrue(e instanceof HttpDataSource.InvalidResponseCodeException); // Check for connection not automatically closed. verify(mockUrlRequest, never()).cancel(); - assertConnectionState(CronetDataSource.OPENING_CONNECTION); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); } } @@ -338,37 +328,16 @@ public final class CronetDataSourceTest { assertTrue(e instanceof HttpDataSource.InvalidContentTypeException); // Check for connection not automatically closed. verify(mockUrlRequest, never()).cancel(); - assertConnectionState(CronetDataSource.OPENING_CONNECTION); verify(mockContentTypePredicate).evaluate(TEST_CONTENT_TYPE); } } - @Test - public void testRequestOpenValidatesContentLength() { - mockResponseStartSuccess(); - - // Data spec's requested length, 5000. Test response's length, 16,000. - testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null); - - try { - dataSourceUnderTest.open(testDataSpec); - fail("HttpDataSource.HttpDataSourceException expected"); - } catch (HttpDataSourceException e) { - verify(mockUrlRequest).addHeader("Range", "bytes=1000-5999"); - // Check for connection not automatically closed. - verify(mockUrlRequest, never()).cancel(); - assertConnectionState(CronetDataSource.OPENING_CONNECTION); - verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testPostDataSpec); - } - } - @Test public void testPostRequestOpen() throws HttpDataSourceException { mockResponseStartSuccess(); dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE); assertEquals(TEST_CONTENT_LENGTH, dataSourceUnderTest.open(testPostDataSpec)); - assertConnectionState(CronetDataSource.OPEN_CONNECTION); verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testPostDataSpec); } @@ -510,7 +479,6 @@ public final class CronetDataSourceTest { dataSourceUnderTest.close(); verify(mockTransferListener).onTransferEnd(dataSourceUnderTest); - assertConnectionState(CronetDataSource.IDLE_CONNECTION); try { bytesRead += dataSourceUnderTest.read(returnedBuffer, 0, 8); @@ -572,7 +540,6 @@ public final class CronetDataSourceTest { verify(mockUrlRequest, times(1)).read(any(ByteBuffer.class)); // Check for connection not automatically closed. verify(mockUrlRequest, never()).cancel(); - assertConnectionState(CronetDataSource.OPEN_CONNECTION); assertEquals(16, bytesRead); } @@ -603,15 +570,12 @@ public final class CronetDataSourceTest { // We should still be trying to open. assertFalse(timedOutCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // We should still be trying to open as we approach the timeout. when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1); assertFalse(timedOutCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // Now we timeout. when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS); timedOutCondition.block(); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); } @@ -637,15 +601,12 @@ public final class CronetDataSourceTest { // We should still be trying to open. assertFalse(openCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // We should still be trying to open as we approach the timeout. when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1); assertFalse(openCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // The response arrives just in time. dataSourceUnderTest.onResponseStarted(mockUrlRequest, testUrlResponseInfo); openCondition.block(); - assertEquals(CronetDataSource.OPEN_CONNECTION, dataSourceUnderTest.connectionState); } @Test @@ -674,11 +635,9 @@ public final class CronetDataSourceTest { // We should still be trying to open. assertFalse(timedOutCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // We should still be trying to open as we approach the timeout. when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1); assertFalse(timedOutCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // A redirect arrives just in time. dataSourceUnderTest.onRedirectReceived(mockUrlRequest, testUrlResponseInfo, "RandomRedirectedUrl1"); @@ -689,7 +648,6 @@ public final class CronetDataSourceTest { assertFalse(timedOutCondition.block(newTimeoutMs)); // We should still be trying to open as we approach the new timeout. assertFalse(timedOutCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // A redirect arrives just in time. dataSourceUnderTest.onRedirectReceived(mockUrlRequest, testUrlResponseInfo, "RandomRedirectedUrl2"); @@ -700,11 +658,9 @@ public final class CronetDataSourceTest { assertFalse(timedOutCondition.block(newTimeoutMs)); // We should still be trying to open as we approach the new timeout. assertFalse(timedOutCondition.block(50)); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); // Now we timeout. when(mockClock.elapsedRealtime()).thenReturn(newTimeoutMs); timedOutCondition.block(); - assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); assertEquals(1, openExceptions.get()); @@ -818,7 +774,7 @@ public final class CronetDataSourceTest { dataSourceUnderTest.onFailed( mockUrlRequest, createUrlResponseInfo(500), // statusCode - null); + mockUrlRequestException); return null; } }).when(mockUrlRequest).read(any(ByteBuffer.class)); @@ -869,8 +825,4 @@ public final class CronetDataSourceTest { return testBuffer; } - private void assertConnectionState(int state) { - assertEquals(state, dataSourceUnderTest.connectionState); - } - } diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index 8e53e32331..a758f71f45 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -37,7 +37,6 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.Executor; -import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.chromium.net.CronetEngine; @@ -91,11 +90,6 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou // The size of read buffer passed to cronet UrlRequest.read(). private static final int READ_BUFFER_SIZE_BYTES = 32 * 1024; - /* package */ static final int IDLE_CONNECTION = 5; - /* package */ static final int OPENING_CONNECTION = 2; - /* package */ static final int CONNECTED_CONNECTION = 3; - /* package */ static final int OPEN_CONNECTION = 4; - private final CronetEngine cronetEngine; private final Executor executor; private final Predicate contentTypePredicate; @@ -105,20 +99,29 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou private final boolean resetTimeoutOnRedirects; private final Map requestProperties; private final ConditionVariable operation; - private final ByteBuffer readBuffer; private final Clock clock; + // Accessed by the calling thread only. + private boolean opened; + private long bytesRemaining; + + // Written from the calling thread only. currentUrlRequest.start() calls ensure writes are visible + // to reads made by the Cronet thread. private UrlRequest currentUrlRequest; private DataSpec currentDataSpec; - private UrlResponseInfo responseInfo; - /* package */ volatile int connectionState; + // Reference written and read by calling thread only. Passed to Cronet thread as a local variable. + // operation.open() calls ensure writes into the buffer are visible to reads made by the calling + // thread. + private ByteBuffer readBuffer; + + // Written from the Cronet thread only. operation.open() calls ensure writes are visible to reads + // made by the calling thread. + private UrlResponseInfo responseInfo; + private IOException exception; + private boolean finished; + private volatile long currentConnectTimeoutMs; - private volatile HttpDataSourceException exception; - private volatile long contentLength; - private volatile AtomicLong expectedBytesRemainingToRead; - private volatile boolean hasData; - private volatile boolean responseFinished; /** * @param cronetEngine A CronetEngine. @@ -163,10 +166,8 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou this.readTimeoutMs = readTimeoutMs; this.resetTimeoutOnRedirects = resetTimeoutOnRedirects; this.clock = Assertions.checkNotNull(clock); - readBuffer = ByteBuffer.allocateDirect(READ_BUFFER_SIZE_BYTES); requestProperties = new HashMap<>(); operation = new ConditionVariable(); - connectionState = IDLE_CONNECTION; } // HttpDataSource implementation. @@ -205,10 +206,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou @Override public long open(DataSpec dataSpec) throws HttpDataSourceException { Assertions.checkNotNull(dataSpec); - synchronized (this) { - Assertions.checkState(connectionState == IDLE_CONNECTION, "Connection already open"); - connectionState = OPENING_CONNECTION; - } + Assertions.checkState(!opened); operation.close(); resetConnectTimeout(); @@ -218,61 +216,99 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou boolean requestStarted = blockUntilConnectTimeout(); if (exception != null) { - // An error occurred opening the connection. - throw exception; + throw new OpenException(exception, currentDataSpec, getStatus(currentUrlRequest)); } else if (!requestStarted) { // The timeout was reached before the connection was opened. throw new OpenException(new SocketTimeoutException(), dataSpec, getStatus(currentUrlRequest)); } - // Connection was opened. + // Check for a valid response code. + int responseCode = responseInfo.getHttpStatusCode(); + if (responseCode < 200 || responseCode > 299) { + InvalidResponseCodeException exception = new InvalidResponseCodeException(responseCode, + responseInfo.getAllHeaders(), currentDataSpec); + if (responseCode == 416) { + exception.initCause(new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE)); + } + throw exception; + } + + // Check for a valid content type. + if (contentTypePredicate != null) { + List contentTypeHeaders = responseInfo.getAllHeaders().get(CONTENT_TYPE); + String contentType = isEmpty(contentTypeHeaders) ? null : contentTypeHeaders.get(0); + if (!contentTypePredicate.evaluate(contentType)) { + throw new InvalidContentTypeException(contentType, currentDataSpec); + } + } + + // TODO: Handle the case where we requested a range starting from a non-zero position and + // received a 200 rather than a 206. This occurs if the server does not support partial + // requests, and requires that the source skips to the requested position. + + // Calculate the content length. + if (!getIsCompressed(responseInfo)) { + if (dataSpec.length != C.LENGTH_UNSET) { + bytesRemaining = dataSpec.length; + } else { + bytesRemaining = getContentLength(responseInfo); + } + } else { + // If the response is compressed then the content length will be that of the compressed data + // which isn't what we want. Always use the dataSpec length in this case. + bytesRemaining = currentDataSpec.length; + } + + opened = true; if (listener != null) { listener.onTransferStart(this, dataSpec); } - connectionState = OPEN_CONNECTION; - return contentLength; + + return bytesRemaining; } @Override public int read(byte[] buffer, int offset, int readLength) throws HttpDataSourceException { - synchronized (this) { - Assertions.checkState(connectionState == OPEN_CONNECTION); - } + Assertions.checkState(opened); if (readLength == 0) { return 0; - } - if (expectedBytesRemainingToRead != null && expectedBytesRemainingToRead.get() == 0) { + } else if (bytesRemaining == 0) { return C.RESULT_END_OF_INPUT; } - if (!hasData) { - // Read more data from cronet. + if (readBuffer == null) { + readBuffer = ByteBuffer.allocateDirect(READ_BUFFER_SIZE_BYTES); + readBuffer.limit(0); + } + if (!readBuffer.hasRemaining()) { + // Fill readBuffer with more data from Cronet. operation.close(); readBuffer.clear(); currentUrlRequest.read(readBuffer); if (!operation.block(readTimeoutMs)) { + // We're timing out, but since the operation is still ongoing we'll need to replace + // readBuffer to avoid the possibility of it being written to by this operation during a + // subsequent request. + readBuffer = null; throw new HttpDataSourceException( new SocketTimeoutException(), currentDataSpec, HttpDataSourceException.TYPE_READ); - } - if (exception != null) { - throw exception; - } - // The expected response length is unknown, but cronet has indicated that the request - // already finished successfully. - if (responseFinished) { + } else if (exception != null) { + throw new HttpDataSourceException(exception, currentDataSpec, + HttpDataSourceException.TYPE_READ); + } else if (finished) { return C.RESULT_END_OF_INPUT; + } else { + // The operation didn't time out, fail or finish, and therefore data must have been read. + readBuffer.flip(); } } int bytesRead = Math.min(readBuffer.remaining(), readLength); readBuffer.get(buffer, offset, bytesRead); - if (!readBuffer.hasRemaining()) { - hasData = false; - } - if (expectedBytesRemainingToRead != null) { - expectedBytesRemainingToRead.addAndGet(-bytesRead); + if (bytesRemaining != C.LENGTH_UNSET) { + bytesRemaining -= bytesRead; } if (listener != null) { listener.onBytesTransferred(this, bytesRead); @@ -286,26 +322,26 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou currentUrlRequest.cancel(); currentUrlRequest = null; } + if (readBuffer != null) { + readBuffer.limit(0); + } currentDataSpec = null; - exception = null; - contentLength = 0; - hasData = false; responseInfo = null; - expectedBytesRemainingToRead = null; - responseFinished = false; - try { - if (listener != null && connectionState == OPEN_CONNECTION) { + exception = null; + finished = false; + if (opened) { + opened = false; + if (listener != null) { listener.onTransferEnd(this); } - } finally { - connectionState = IDLE_CONNECTION; } } // UrlRequest.Callback implementation @Override - public void onRedirectReceived(UrlRequest request, UrlResponseInfo info, String newLocationUrl) { + public synchronized void onRedirectReceived(UrlRequest request, UrlResponseInfo info, + String newLocationUrl) { if (request != currentUrlRequest) { return; } @@ -315,8 +351,8 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou // For other redirect response codes the POST request is converted to a GET request and the // redirect is followed. if (responseCode == 307 || responseCode == 308) { - exception = new OpenException("POST request redirected with 307 or 308 response code", - currentDataSpec, getStatus(request)); + exception = new InvalidResponseCodeException(responseCode, info.getAllHeaders(), + currentDataSpec); operation.open(); return; } @@ -332,51 +368,8 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou if (request != currentUrlRequest) { return; } - try { - // Check for a valid response code. - int responseCode = info.getHttpStatusCode(); - if (responseCode < 200 || responseCode > 299) { - InvalidResponseCodeException exception = new InvalidResponseCodeException( - responseCode, info.getAllHeaders(), currentDataSpec); - if (responseCode == 416) { - exception.initCause(new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE)); - } - throw exception; - } - // Check for a valid content type. - if (contentTypePredicate != null) { - List contentTypeHeaders = info.getAllHeaders().get(CONTENT_TYPE); - String contentType = contentTypeHeaders == null || contentTypeHeaders.isEmpty() ? null - : contentTypeHeaders.get(0); - if (!contentTypePredicate.evaluate(contentType)) { - throw new InvalidContentTypeException(contentType, currentDataSpec); - } - } - - responseInfo = info; - if (getIsCompressed(info)) { - contentLength = currentDataSpec.length; - } else { - // Check content length. - contentLength = getContentLength(info); - // If a specific length is requested and a specific length is returned but the 2 don't match - // it's an error. - if (currentDataSpec.length != C.LENGTH_UNSET && contentLength != C.LENGTH_UNSET - && currentDataSpec.length != contentLength) { - throw new OpenException("Content length did not match requested length", currentDataSpec, - getStatus(request)); - } - } - if (contentLength > 0) { - expectedBytesRemainingToRead = new AtomicLong(contentLength); - } - - connectionState = CONNECTED_CONNECTION; - } catch (HttpDataSourceException e) { - exception = e; - } finally { - operation.open(); - } + responseInfo = info; + operation.open(); } @Override @@ -385,17 +378,15 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou if (request != currentUrlRequest) { return; } - readBuffer.flip(); - hasData = true; operation.open(); } @Override - public void onSucceeded(UrlRequest request, UrlResponseInfo info) { + public synchronized void onSucceeded(UrlRequest request, UrlResponseInfo info) { if (request != currentUrlRequest) { return; } - responseFinished = true; + finished = true; operation.open(); } @@ -405,14 +396,8 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou if (request != currentUrlRequest) { return; } - if (connectionState == OPENING_CONNECTION) { - IOException cause = error.getErrorCode() == UrlRequestException.ERROR_HOSTNAME_NOT_RESOLVED - ? new UnknownHostException() : error; - exception = new OpenException(cause, currentDataSpec, getStatus(request)); - } else if (connectionState == OPEN_CONNECTION) { - exception = new HttpDataSourceException(error, currentDataSpec, - HttpDataSourceException.TYPE_READ); - } + exception = error.getErrorCode() == UrlRequestException.ERROR_HOSTNAME_NOT_RESOLVED + ? new UnknownHostException() : error; operation.open(); } @@ -477,7 +462,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou Map> headers = info.getAllHeaders(); List contentLengthHeaders = headers.get("Content-Length"); String contentLengthHeader = null; - if (contentLengthHeaders != null && !contentLengthHeaders.isEmpty()) { + if (!isEmpty(contentLengthHeaders)) { contentLengthHeader = contentLengthHeaders.get(0); if (!TextUtils.isEmpty(contentLengthHeader)) { try { @@ -488,7 +473,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou } } List contentRangeHeaders = headers.get("Content-Range"); - if (contentRangeHeaders != null && !contentRangeHeaders.isEmpty()) { + if (!isEmpty(contentRangeHeaders)) { String contentRangeHeader = contentRangeHeaders.get(0); Matcher matcher = CONTENT_RANGE_HEADER_PATTERN.matcher(contentRangeHeader); if (matcher.find()) { @@ -530,4 +515,8 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return statusHolder[0]; } + private static boolean isEmpty(List list) { + return list == null || list.isEmpty(); + } + } diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceFactory.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceFactory.java index 1cffee8188..0f94dad158 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceFactory.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceFactory.java @@ -41,7 +41,7 @@ public final class CronetDataSourceFactory implements Factory { private final CronetEngine cronetEngine; private final Executor executor; private final Predicate contentTypePredicate; - private final TransferListener transferListener; + private final TransferListener transferListener; private final int connectTimeoutMs; private final int readTimeoutMs; private final boolean resetTimeoutOnRedirects; diff --git a/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java b/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java index 7a8f3bca3f..2b6eaa736d 100644 --- a/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java +++ b/extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java @@ -185,9 +185,12 @@ public class OkHttpDataSource implements HttpDataSource { bytesToSkip = responseCode == 200 && dataSpec.position != 0 ? dataSpec.position : 0; // Determine the length of the data to be read, after skipping. - long contentLength = response.body().contentLength(); - bytesToRead = dataSpec.length != C.LENGTH_UNSET ? dataSpec.length - : (contentLength != -1 ? (contentLength - bytesToSkip) : C.LENGTH_UNSET); + if (dataSpec.length != C.LENGTH_UNSET) { + bytesToRead = dataSpec.length; + } else { + long contentLength = response.body().contentLength(); + bytesToRead = contentLength != -1 ? (contentLength - bytesToSkip) : C.LENGTH_UNSET; + } opened = true; if (listener != null) { diff --git a/library/src/main/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java b/library/src/main/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java index b193b446c2..34ec76b673 100644 --- a/library/src/main/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java +++ b/library/src/main/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java @@ -231,10 +231,13 @@ public class DefaultHttpDataSource implements HttpDataSource { // Determine the length of the data to be read, after skipping. if ((dataSpec.flags & DataSpec.FLAG_ALLOW_GZIP) == 0) { - long contentLength = getContentLength(connection); - bytesToRead = dataSpec.length != C.LENGTH_UNSET ? dataSpec.length - : contentLength != C.LENGTH_UNSET ? contentLength - bytesToSkip - : C.LENGTH_UNSET; + if (dataSpec.length != C.LENGTH_UNSET) { + bytesToRead = dataSpec.length; + } else { + long contentLength = getContentLength(connection); + bytesToRead = contentLength != C.LENGTH_UNSET ? (contentLength - bytesToSkip) + : C.LENGTH_UNSET; + } } else { // Gzip is enabled. If the server opts to use gzip then the content length in the response // will be that of the compressed data, which isn't what we want. Furthermore, there isn't a From 83107cc25d402a5b405d67cdf4b3687d472d4f53 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 11 Oct 2016 07:20:44 -0700 Subject: [PATCH 11/13] Fix missing release calls on loadingPeriodHolder playingPeriodHolder can be null in the case that the first period is still being prepared. We need to make sure we release the period that's being prepared in such cases, which is loadingPeriodHolder. Issue: #1914 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135793472 --- .../exoplayer2/ExoPlayerImplInternal.java | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java index 85f365e51e..56b862073a 100644 --- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java +++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java @@ -538,16 +538,23 @@ import java.io.IOException; periodIndex = C.INDEX_UNSET; } - // Clear the timeline, but keep the requested period if it is already prepared. - MediaPeriodHolder periodHolder = playingPeriodHolder; MediaPeriodHolder newPlayingPeriodHolder = null; - while (periodHolder != null) { - if (periodHolder.index == periodIndex && periodHolder.prepared) { - newPlayingPeriodHolder = periodHolder; - } else { - periodHolder.release(); + if (playingPeriodHolder == null) { + // We're still waiting for the first period to be prepared. + if (loadingPeriodHolder != null) { + loadingPeriodHolder.release(); + } + } else { + // Clear the timeline, but keep the requested period if it is already prepared. + MediaPeriodHolder periodHolder = playingPeriodHolder; + while (periodHolder != null) { + if (periodHolder.index == periodIndex && periodHolder.prepared) { + newPlayingPeriodHolder = periodHolder; + } else { + periodHolder.release(); + } + periodHolder = periodHolder.next; } - periodHolder = periodHolder.next; } // Disable all the renderers if the period is changing. @@ -892,7 +899,8 @@ import java.io.IOException; } // Release all loaded periods. - releasePeriodHoldersFrom(playingPeriodHolder); + releasePeriodHoldersFrom(playingPeriodHolder != null ? playingPeriodHolder + : loadingPeriodHolder); bufferAheadPeriodCount = 0; playingPeriodHolder = null; readingPeriodHolder = null; From 29f3eb5e5a0f720d55f73992c0817ac4f7c3b9b7 Mon Sep 17 00:00:00 2001 From: olly Date: Sat, 7 May 2016 08:13:17 +0100 Subject: [PATCH 12/13] Fixes for retries - Fix issue in ExoPlayerImpl where the timeline was null'd but onTimelineChanged was not fired. - Add the ability to not reset the timeline. This is useful for retries where you know the timeline will be the same as it was previously. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135797577 --- .../android/exoplayer2/demo/EventLogger.java | 3 +++ .../exoplayer2/demo/PlayerActivity.java | 27 +++++++++---------- .../google/android/exoplayer2/ExoPlayer.java | 11 +++++--- .../android/exoplayer2/ExoPlayerImpl.java | 12 ++++++--- .../android/exoplayer2/SimpleExoPlayer.java | 4 +-- 5 files changed, 34 insertions(+), 23 deletions(-) diff --git a/demo/src/main/java/com/google/android/exoplayer2/demo/EventLogger.java b/demo/src/main/java/com/google/android/exoplayer2/demo/EventLogger.java index 8c12290d35..d79de04657 100644 --- a/demo/src/main/java/com/google/android/exoplayer2/demo/EventLogger.java +++ b/demo/src/main/java/com/google/android/exoplayer2/demo/EventLogger.java @@ -97,6 +97,9 @@ import java.util.Locale; @Override public void onTimelineChanged(Timeline timeline, Object manifest) { + if (timeline == null) { + return; + } int periodCount = timeline.getPeriodCount(); int windowCount = timeline.getWindowCount(); Log.d(TAG, "sourceInfo [periodCount=" + periodCount + ", windowCount=" + windowCount); diff --git a/demo/src/main/java/com/google/android/exoplayer2/demo/PlayerActivity.java b/demo/src/main/java/com/google/android/exoplayer2/demo/PlayerActivity.java index 667ccd5bab..e9aa46f85f 100644 --- a/demo/src/main/java/com/google/android/exoplayer2/demo/PlayerActivity.java +++ b/demo/src/main/java/com/google/android/exoplayer2/demo/PlayerActivity.java @@ -101,6 +101,7 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay } private Handler mainHandler; + private Timeline.Window window; private EventLogger eventLogger; private SimpleExoPlayerView simpleExoPlayerView; private LinearLayout debugRootView; @@ -115,7 +116,7 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay private boolean playerNeedsSource; private boolean shouldAutoPlay; - private boolean shouldRestorePosition; + private boolean isTimelineStatic; private int playerWindow; private long playerPosition; @@ -127,6 +128,7 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay shouldAutoPlay = true; mediaDataSourceFactory = buildDataSourceFactory(true); mainHandler = new Handler(); + window = new Timeline.Window(); if (CookieHandler.getDefault() != DEFAULT_COOKIE_MANAGER) { CookieHandler.setDefault(DEFAULT_COOKIE_MANAGER); } @@ -147,7 +149,7 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay @Override public void onNewIntent(Intent intent) { releasePlayer(); - shouldRestorePosition = false; + isTimelineStatic = false; setIntent(intent); } @@ -262,7 +264,7 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay player.setVideoDebugListener(eventLogger); player.setId3Output(eventLogger); simpleExoPlayerView.setPlayer(player); - if (shouldRestorePosition) { + if (isTimelineStatic) { if (playerPosition == C.TIME_UNSET) { player.seekToDefaultPosition(playerWindow); } else { @@ -305,7 +307,7 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay } MediaSource mediaSource = mediaSources.length == 1 ? mediaSources[0] : new ConcatenatingMediaSource(mediaSources); - player.prepare(mediaSource, !shouldRestorePosition); + player.prepare(mediaSource, !isTimelineStatic, !isTimelineStatic); playerNeedsSource = false; updateButtonVisibilities(); } @@ -348,15 +350,11 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay debugViewHelper.stop(); debugViewHelper = null; shouldAutoPlay = player.getPlayWhenReady(); - shouldRestorePosition = false; + playerWindow = player.getCurrentWindowIndex(); + playerPosition = C.TIME_UNSET; Timeline timeline = player.getCurrentTimeline(); - if (timeline != null) { - playerWindow = player.getCurrentWindowIndex(); - Timeline.Window window = timeline.getWindow(playerWindow, new Timeline.Window()); - if (!window.isDynamic) { - shouldRestorePosition = true; - playerPosition = window.isSeekable ? player.getCurrentPosition() : C.TIME_UNSET; - } + if (timeline != null && timeline.getWindow(playerWindow, window).isSeekable) { + playerPosition = player.getCurrentPosition(); } player.release(); player = null; @@ -412,7 +410,8 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay @Override public void onTimelineChanged(Timeline timeline, Object manifest) { - // Do nothing. + isTimelineStatic = timeline != null && timeline.getWindowCount() > 0 + && !timeline.getWindow(timeline.getWindowCount() - 1, window).isDynamic; } @Override @@ -501,7 +500,7 @@ public class PlayerActivity extends Activity implements OnClickListener, ExoPlay button.setText(label); button.setTag(i); button.setOnClickListener(this); - debugRootView.addView(button); + debugRootView.addView(button, debugRootView.getChildCount() - 1); } } } diff --git a/library/src/main/java/com/google/android/exoplayer2/ExoPlayer.java b/library/src/main/java/com/google/android/exoplayer2/ExoPlayer.java index 4d53455151..e3c9b6e114 100644 --- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayer.java +++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayer.java @@ -130,8 +130,8 @@ public interface ExoPlayer { /** * Called when timeline and/or manifest has been refreshed. * - * @param timeline The latest timeline. - * @param manifest The latest manifest. + * @param timeline The latest timeline, or null if the timeline is being cleared. + * @param manifest The latest manifest, or null if the manifest is being cleared. */ void onTimelineChanged(Timeline timeline, Object manifest); @@ -247,7 +247,7 @@ public interface ExoPlayer { /** * Prepares the player to play the provided {@link MediaSource}. Equivalent to - * {@code prepare(mediaSource, true)}. + * {@code prepare(mediaSource, true, true)}. */ void prepare(MediaSource mediaSource); @@ -259,8 +259,11 @@ public interface ExoPlayer { * @param resetPosition Whether the playback position should be reset to the default position in * the first {@link Timeline.Window}. If false, playback will start from the position defined * by {@link #getCurrentWindowIndex()} and {@link #getCurrentPosition()}. + * @param resetTimeline Whether the timeline and manifest should be reset. Should be true unless + * the player is being prepared to play the same media as it was playing previously (e.g. if + * playback failed and is being retried). */ - void prepare(MediaSource mediaSource, boolean resetPosition); + void prepare(MediaSource mediaSource, boolean resetPosition, boolean resetTimeline); /** * Sets whether playback should proceed when {@link #getPlaybackState()} == {@link #STATE_READY}. diff --git a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java index a79ad1dba0..3eb2ceb38b 100644 --- a/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java +++ b/library/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java @@ -101,12 +101,18 @@ import java.util.concurrent.CopyOnWriteArraySet; @Override public void prepare(MediaSource mediaSource) { - prepare(mediaSource, true); + prepare(mediaSource, true, true); } @Override - public void prepare(MediaSource mediaSource, boolean resetPosition) { - timeline = null; + public void prepare(MediaSource mediaSource, boolean resetPosition, boolean resetTimeline) { + if (resetTimeline && (timeline != null || manifest != null)) { + timeline = null; + manifest = null; + for (EventListener listener : listeners) { + listener.onTimelineChanged(null, null); + } + } internalPlayer.prepare(mediaSource, resetPosition); } diff --git a/library/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java b/library/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java index a8f04e5113..f2c26b9495 100644 --- a/library/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java +++ b/library/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java @@ -420,8 +420,8 @@ public final class SimpleExoPlayer implements ExoPlayer { } @Override - public void prepare(MediaSource mediaSource, boolean resetPosition) { - player.prepare(mediaSource, resetPosition); + public void prepare(MediaSource mediaSource, boolean resetPosition, boolean resetTimeline) { + player.prepare(mediaSource, resetPosition, resetTimeline); } @Override From 996fe47f8cbafd3a9e6a0b2025a036414bd70a04 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 11 Oct 2016 08:29:55 -0700 Subject: [PATCH 13/13] Fix NPE releasing HlsMediaPeriod Issue: #1907 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135798950 --- .../android/exoplayer2/source/hls/HlsMediaPeriod.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/src/main/java/com/google/android/exoplayer2/source/hls/HlsMediaPeriod.java b/library/src/main/java/com/google/android/exoplayer2/source/hls/HlsMediaPeriod.java index 7ed979c902..57925ed67a 100644 --- a/library/src/main/java/com/google/android/exoplayer2/source/hls/HlsMediaPeriod.java +++ b/library/src/main/java/com/google/android/exoplayer2/source/hls/HlsMediaPeriod.java @@ -103,8 +103,10 @@ import java.util.List; public void release() { continueLoadingHandler.removeCallbacksAndMessages(null); manifestFetcher.release(); - for (HlsSampleStreamWrapper sampleStreamWrapper : sampleStreamWrappers) { - sampleStreamWrapper.release(); + if (sampleStreamWrappers != null) { + for (HlsSampleStreamWrapper sampleStreamWrapper : sampleStreamWrappers) { + sampleStreamWrapper.release(); + } } }