From 7afdf84a9d36bbd151fff7bf9afb38da672d822e Mon Sep 17 00:00:00 2001 From: olly Date: Sun, 6 May 2018 12:06:09 -0700 Subject: [PATCH] Fix DownloadService notification flicker This type of flicker was visible (at least on my S8) whenever starting the demo app process (from being properly killed) when other notifications (e.g. completed download notification) was present. This change passes whether the service was started as foreground through the intent, and only applies the show-notification-on-stop hack if it was. It's only necessary to start as foreground if your app is not already in the foreground, so it's not necessary to do this from activity/ui components. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=195594930 --- .../exoplayer2/demo/DownloadTracker.java | 8 +- .../demo/SampleChooserActivity.java | 11 +- .../exoplayer2/offline/DownloadService.java | 179 ++++++++++-------- .../dash/offline/DownloadServiceDashTest.java | 3 +- 4 files changed, 107 insertions(+), 94 deletions(-) diff --git a/demos/main/src/main/java/com/google/android/exoplayer2/demo/DownloadTracker.java b/demos/main/src/main/java/com/google/android/exoplayer2/demo/DownloadTracker.java index fcdf52a153..b4bce01c7a 100644 --- a/demos/main/src/main/java/com/google/android/exoplayer2/demo/DownloadTracker.java +++ b/demos/main/src/main/java/com/google/android/exoplayer2/demo/DownloadTracker.java @@ -126,7 +126,7 @@ public class DownloadTracker implements DownloadManager.Listener { if (isDownloaded(uri)) { DownloadAction removeAction = getDownloadHelper(uri, extension).getRemoveAction(Util.getUtf8Bytes(name)); - addDownloadAction(removeAction); + startServiceWithAction(removeAction); } else { StartDownloadDialogHelper helper = new StartDownloadDialogHelper(activity, getDownloadHelper(uri, extension), name); @@ -197,11 +197,11 @@ public class DownloadTracker implements DownloadManager.Listener { } trackedDownloadStates.put(action.uri, action); handleTrackedDownloadStatesChanged(); - addDownloadAction(action); + startServiceWithAction(action); } - private void addDownloadAction(DownloadAction action) { - DownloadService.addDownloadAction(context, DemoDownloadService.class, action); + private void startServiceWithAction(DownloadAction action) { + DownloadService.startWithAction(context, DemoDownloadService.class, action, false); } private DownloadHelper getDownloadHelper(Uri uri, String extension) { diff --git a/demos/main/src/main/java/com/google/android/exoplayer2/demo/SampleChooserActivity.java b/demos/main/src/main/java/com/google/android/exoplayer2/demo/SampleChooserActivity.java index fb0b20f0e4..324c5ea4cb 100644 --- a/demos/main/src/main/java/com/google/android/exoplayer2/demo/SampleChooserActivity.java +++ b/demos/main/src/main/java/com/google/android/exoplayer2/demo/SampleChooserActivity.java @@ -91,10 +91,12 @@ public class SampleChooserActivity extends Activity } downloadTracker = ((DemoApplication) getApplication()).getDownloadTracker(); - startDownloadServiceForeground(); - SampleListLoader loaderTask = new SampleListLoader(); loaderTask.execute(uris); + + // Ping the download service in case it's not running (but should be). + startService( + new Intent(this, DemoDownloadService.class).setAction(DownloadService.ACTION_INIT)); } @Override @@ -115,11 +117,6 @@ public class SampleChooserActivity extends Activity sampleAdapter.notifyDataSetChanged(); } - private void startDownloadServiceForeground() { - Intent intent = new Intent(DownloadService.ACTION_INIT).setPackage(getPackageName()); - Util.startForegroundService(this, intent); - } - private void onSampleGroups(final List groups, boolean sawError) { if (sawError) { Toast.makeText(getApplicationContext(), R.string.sample_list_load_error, Toast.LENGTH_LONG) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java index 40a50c53a6..091b215280 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java @@ -34,32 +34,38 @@ import com.google.android.exoplayer2.util.Util; import java.io.IOException; import java.util.HashMap; -/** - * A {@link Service} that downloads streams in the background. - * - *

To start the service, create an instance of one of the subclasses of {@link DownloadAction} - * and call {@link #addDownloadAction(Context, Class, DownloadAction)} with it. - */ +/** A {@link Service} for downloading media. */ public abstract class DownloadService extends Service { - /** Use this action to initialize {@link DownloadManager}. */ + /** Starts a download service without adding a new {@link DownloadAction}. */ public static final String ACTION_INIT = "com.google.android.exoplayer.downloadService.action.INIT"; - /** Use this action to add a {@link DownloadAction} to {@link DownloadManager} action queue. */ + /** Starts a download service, adding a new {@link DownloadAction} to be executed. */ public static final String ACTION_ADD = "com.google.android.exoplayer.downloadService.action.ADD"; - /** Use this action to make {@link DownloadManager} stop download tasks. */ - private static final String ACTION_STOP = - "com.google.android.exoplayer.downloadService.action.STOP"; + /** Like {@link #ACTION_INIT}, but with {@link #KEY_FOREGROUND} implicitly set to true. */ + private static final String ACTION_RESTART = + "com.google.android.exoplayer.downloadService.action.RESTART"; - /** Use this action to make {@link DownloadManager} start download tasks. */ - private static final String ACTION_START = - "com.google.android.exoplayer.downloadService.action.START"; + /** Starts download tasks. */ + private static final String ACTION_START_DOWNLOADS = + "com.google.android.exoplayer.downloadService.action.START_DOWNLOADS"; + + /** Stops download tasks. */ + private static final String ACTION_STOP_DOWNLOADS = + "com.google.android.exoplayer.downloadService.action.STOP_DOWNLOADS"; /** Key for the {@link DownloadAction} in an {@link #ACTION_ADD} intent. */ public static final String KEY_DOWNLOAD_ACTION = "download_action"; + /** + * Key for a boolean flag in any intent to indicate whether the service was started in the + * foreground. If set, the service is guaranteed to call {@link #startForeground(int, + * Notification)}. + */ + public static final String KEY_FOREGROUND = "foreground"; + /** Default foreground notification update interval in milliseconds. */ public static final long DEFAULT_FOREGROUND_NOTIFICATION_UPDATE_INTERVAL = 1000; @@ -79,6 +85,7 @@ public abstract class DownloadService extends Service { private DownloadManager downloadManager; private DownloadManagerListener downloadManagerListener; private int lastStartId; + private boolean startedInForeground; /** * Creates a DownloadService with {@link #DEFAULT_FOREGROUND_NOTIFICATION_UPDATE_INTERVAL}. @@ -91,8 +98,6 @@ public abstract class DownloadService extends Service { } /** - * Creates a DownloadService. - * * @param foregroundNotificationId The notification id for the foreground notification, must not * be 0. * @param foregroundNotificationUpdateInterval The maximum interval to update foreground @@ -108,8 +113,6 @@ public abstract class DownloadService extends Service { } /** - * Creates a DownloadService. - * * @param foregroundNotificationId The notification id for the foreground notification. Must not * be 0. * @param foregroundNotificationUpdateInterval The maximum interval between updates to the @@ -134,33 +137,44 @@ public abstract class DownloadService extends Service { } /** - * Creates an {@link Intent} to be used to start this service and adds the {@link DownloadAction} - * to the {@link DownloadManager}. + * Builds an {@link Intent} for adding an action to be executed by the service. * - * @param context A {@link Context} of the application calling this service. - * @param clazz Class object of DownloadService or subclass. - * @param downloadAction A {@link DownloadAction} to be executed. + * @param context A {@link Context}. + * @param clazz The concrete download service being targeted by the intent. + * @param downloadAction The action to be executed. + * @param foreground Whether this intent will be used to start the service in the foreground. * @return Created Intent. */ - public static Intent createAddDownloadActionIntent( - Context context, Class clazz, DownloadAction downloadAction) { + public static Intent buildAddActionIntent( + Context context, + Class clazz, + DownloadAction downloadAction, + boolean foreground) { return new Intent(context, clazz) .setAction(ACTION_ADD) - .putExtra(KEY_DOWNLOAD_ACTION, downloadAction.toByteArray()); + .putExtra(KEY_DOWNLOAD_ACTION, downloadAction.toByteArray()) + .putExtra(KEY_FOREGROUND, foreground); } /** - * Adds a {@link DownloadAction} to the {@link DownloadManager}. This will start the download - * service if it was not running. + * Starts the service, adding an action to be executed. * - * @param context A {@link Context} of the application calling this service. - * @param clazz Class object of DownloadService or subclass. - * @param downloadAction A {@link DownloadAction} to be executed. - * @see #createAddDownloadActionIntent(Context, Class, DownloadAction) + * @param context A {@link Context}. + * @param clazz The concrete download service being targeted by the intent. + * @param downloadAction The action to be executed. + * @param foreground Whether this intent will be used to start the service in the foreground. */ - public static void addDownloadAction( - Context context, Class clazz, DownloadAction downloadAction) { - context.startService(createAddDownloadActionIntent(context, clazz, downloadAction)); + public static void startWithAction( + Context context, + Class clazz, + DownloadAction downloadAction, + boolean foreground) { + Intent intent = buildAddActionIntent(context, clazz, downloadAction, foreground); + if (foreground) { + Util.startForegroundService(context, intent); + } else { + context.startService(intent); + } } @Override @@ -186,6 +200,48 @@ public abstract class DownloadService extends Service { requirementsHelper.start(); } + @Override + public int onStartCommand(Intent intent, int flags, int startId) { + String intentAction = null; + if (intent != null) { + intentAction = intent.getAction(); + startedInForeground |= + intent.getBooleanExtra(KEY_FOREGROUND, false) || ACTION_RESTART.equals(intentAction); + } + logd("onStartCommand action: " + intentAction + " startId: " + startId); + switch (intentAction) { + case ACTION_INIT: + case ACTION_RESTART: + // Do nothing. The RequirementsWatcher will start downloads when possible. + break; + case ACTION_ADD: + byte[] actionData = intent.getByteArrayExtra(KEY_DOWNLOAD_ACTION); + if (actionData == null) { + Log.e(TAG, "Ignoring ADD action with no action data"); + } else { + try { + downloadManager.handleAction(actionData); + } catch (IOException e) { + Log.e(TAG, "Failed to handle ADD action", e); + } + } + break; + case ACTION_STOP_DOWNLOADS: + downloadManager.stopDownloads(); + break; + case ACTION_START_DOWNLOADS: + downloadManager.startDownloads(); + break; + default: + Log.e(TAG, "Ignoring unrecognized action: " + intentAction); + break; + } + if (downloadManager.isIdle()) { + stop(); + } + return START_STICKY; + } + @Override public void onDestroy() { logd("onDestroy"); @@ -207,47 +263,6 @@ public abstract class DownloadService extends Service { return null; } - @Override - public int onStartCommand(Intent intent, int flags, int startId) { - this.lastStartId = startId; - String intentAction = intent != null ? intent.getAction() : null; - if (intentAction == null) { - intentAction = ACTION_INIT; - } - logd("onStartCommand action: " + intentAction + " startId: " + startId); - switch (intentAction) { - case ACTION_INIT: - // Do nothing. DownloadManager and RequirementsWatcher is initialized. If there are download - // or remove tasks loaded from file, they will start if the requirements are met. - break; - case ACTION_ADD: - byte[] actionData = intent.getByteArrayExtra(KEY_DOWNLOAD_ACTION); - if (actionData == null) { - Log.e(TAG, "Ignoring ADD action with no action data"); - } else { - try { - downloadManager.handleAction(actionData); - } catch (IOException e) { - Log.e(TAG, "Failed to handle ADD action", e); - } - } - break; - case ACTION_STOP: - downloadManager.stopDownloads(); - break; - case ACTION_START: - downloadManager.startDownloads(); - break; - default: - Log.e(TAG, "Ignoring unrecognized action: " + intentAction); - break; - } - if (downloadManager.isIdle()) { - stop(); - } - return START_STICKY; - } - /** * Returns a {@link DownloadManager} to be used to downloaded content. Called only once in the * life cycle of the service. The service will call {@link DownloadManager#startDownloads()} and @@ -299,7 +314,7 @@ public abstract class DownloadService extends Service { private void stop() { foregroundNotificationUpdater.stopPeriodicUpdates(); // Make sure startForeground is called before stopping. Workaround for [Internal: b/69424260]. - if (Util.SDK_INT >= 26) { + if (startedInForeground && Util.SDK_INT >= 26) { foregroundNotificationUpdater.showNotificationIfNotAlready(); } boolean stopSelfResult = stopSelfResult(lastStartId); @@ -414,7 +429,7 @@ public abstract class DownloadService extends Service { @Override public void requirementsMet(RequirementsWatcher requirementsWatcher) { - startServiceWithAction(DownloadService.ACTION_START); + startServiceWithAction(DownloadService.ACTION_START_DOWNLOADS); if (scheduler != null) { scheduler.cancel(); } @@ -422,9 +437,10 @@ public abstract class DownloadService extends Service { @Override public void requirementsNotMet(RequirementsWatcher requirementsWatcher) { - startServiceWithAction(DownloadService.ACTION_STOP); + startServiceWithAction(DownloadService.ACTION_STOP_DOWNLOADS); if (scheduler != null) { - boolean success = scheduler.schedule(requirements, context.getPackageName(), ACTION_INIT); + String servicePackage = context.getPackageName(); + boolean success = scheduler.schedule(requirements, servicePackage, ACTION_RESTART); if (!success) { Log.e(TAG, "Scheduling downloads failed."); } @@ -432,7 +448,8 @@ public abstract class DownloadService extends Service { } private void startServiceWithAction(String action) { - Intent intent = new Intent(context, serviceClass).setAction(action); + Intent intent = + new Intent(context, serviceClass).setAction(action).putExtra(KEY_FOREGROUND, true); Util.startForegroundService(context, intent); } } diff --git a/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadServiceDashTest.java b/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadServiceDashTest.java index e256e4850d..745acd9bbf 100644 --- a/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadServiceDashTest.java +++ b/library/dash/src/test/java/com/google/android/exoplayer2/source/dash/offline/DownloadServiceDashTest.java @@ -229,8 +229,7 @@ public class DownloadServiceDashTest { @Override public void run() { Intent startIntent = - DownloadService.createAddDownloadActionIntent( - context, DownloadService.class, action); + DownloadService.buildAddActionIntent(context, DownloadService.class, action, false); dashDownloadService.onStartCommand(startIntent, 0, 0); } });