Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1679923005:
Re-enable volume control for Android Cast (Closed)
|
Created:
4 years, 10 months ago by aberent Modified:
4 years, 10 months ago Reviewers:
whywhat CC:
chromium-reviews, dgn, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, mlamouri (slow - plz ping), posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable volume control for Android Cast
Previously this was enabled through the (lock screen)
RemoteControlClient, but this was removed in
https://codereview.chromium.org/1652163002/. Reenable
it through the notification's MediaSessionCompat.
BUG=585393
Committed: https://crrev.com/d265521868584caae4c6db60f5103f2d602c5e65
Cr-Commit-Position: refs/heads/master@{#374729}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Respond to comments #Messages
Total messages: 20 (7 generated)
aberent@chromium.org changed reviewers: + avayvod@chromium.org
The CQ bit was checked by aberent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679923005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679923005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:599: if (mMediaSession == null) mMediaSession = createMediaSession(); So the side effect of that is that we will show the notification on the watch and also receive events from the lock screen and from the headsets, etc. When play/pause is not supported, we avoided doing that before. Not sure if that change of functionality is required. Maybe only do that when supportsPlayPause is true? It would become true for both Presentation and VideoFling notifications if the media is playing. https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:600: MediaRouter.getInstance(mContext).setMediaSessionCompat(mMediaSession); As we know, this could cause a crash on some versions of JB. We should either make sure the crash was fixed (e.g. in the support library) or that we handle the exception here as we do in other places. Time to write that MediaRouterUtil class? https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:612: // TODO(avayvod) either the comment or the code is wrong! This will only be reached I think the comment is wrong, just remove it.
Leaving the other comments until we have agreed what the behaviour should be. https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:599: if (mMediaSession == null) mMediaSession = createMediaSession(); On 2016/02/09 20:05:46, whywhat wrote: > So the side effect of that is that we will show the notification on the watch > and also receive events from the lock screen and from the headsets, etc. When > play/pause is not supported, we avoided doing that before. Not sure if that > change of functionality is required. Maybe only do that when supportsPlayPause > is true? It would become true for both Presentation and VideoFling notifications > if the media is playing. I have just tested the behaviour of this code; on JB the lock screen doesn't appear until the video actually starts playing. On M the notification appears on the lock screen when the video starts loading (with only a stop button) but the background doesn't change until the video starts playing. I see the notification on the phone when it starts loading, but don't see the notification on my watch until it starts playing. Note that, on my watch, swiping left on the notification gives volume controls. I haven't compared this with what happens with the current master code, but all this behaviour seems reasonable (although I would like the background to change sooner on M). On cast we probably want the notification while it is loading, so that it can be cancelled (particularly if, as sometimes happens, it gets stuck loading or loads very slowly). Why, anyway, do we want to avoid remote control (of volume, for example) when play/pause isn't available?
https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:599: if (mMediaSession == null) mMediaSession = createMediaSession(); On 2016/02/10 at 11:47:18, aberent wrote: > On 2016/02/09 20:05:46, whywhat wrote: > > So the side effect of that is that we will show the notification on the watch > > and also receive events from the lock screen and from the headsets, etc. When > > play/pause is not supported, we avoided doing that before. Not sure if that > > change of functionality is required. Maybe only do that when supportsPlayPause > > is true? It would become true for both Presentation and VideoFling notifications > > if the media is playing. > > I have just tested the behaviour of this code; on JB the lock screen doesn't appear until the video actually starts playing. On M the notification appears on the lock screen when the video starts loading (with only a stop button) but the background doesn't change until the video starts playing. > > I see the notification on the phone when it starts loading, but don't see the notification on my watch until it starts playing. > > Note that, on my watch, swiping left on the notification gives volume controls. > > I haven't compared this with what happens with the current master code, but all this behaviour seems reasonable (although I would like the background to change sooner on M). > > On cast we probably want the notification while it is loading, so that it can be cancelled (particularly if, as sometimes happens, it gets stuck loading or loads very slowly). > > Why, anyway, do we want to avoid remote control (of volume, for example) when play/pause isn't available? I'm not against volume control when play/pause isn't available (makes sense for games using the Cast SDK, for example), I want to avoid unreasonable behavior like having the notification on the watch / lock screen before anything starts playng or side effects like receiving play/pause commands via headset before the playback even started (if we receive those, we need to handle them more carefully now). I tested the patch locally with M and the watch and headset. It seems like the volume controls don't work until something is playing (so even if it's paused, the hardware buttons on the phone don't work for Cast/VideoFling while the watch allows to change volume). Perhaps, we need to set some media session state to be able to use those -- but then the watch notification might appear earlier.
https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:599: if (mMediaSession == null) mMediaSession = createMediaSession(); On 2016/02/10 14:51:28, whywhat wrote: > On 2016/02/10 at 11:47:18, aberent wrote: > > On 2016/02/09 20:05:46, whywhat wrote: > > > So the side effect of that is that we will show the notification on the > watch > > > and also receive events from the lock screen and from the headsets, etc. > When > > > play/pause is not supported, we avoided doing that before. Not sure if that > > > change of functionality is required. Maybe only do that when > supportsPlayPause > > > is true? It would become true for both Presentation and VideoFling > notifications > > > if the media is playing. > > > > I have just tested the behaviour of this code; on JB the lock screen doesn't > appear until the video actually starts playing. On M the notification appears on > the lock screen when the video starts loading (with only a stop button) but the > background doesn't change until the video starts playing. > > > > I see the notification on the phone when it starts loading, but don't see the > notification on my watch until it starts playing. > > > > Note that, on my watch, swiping left on the notification gives volume > controls. > > > > I haven't compared this with what happens with the current master code, but > all this behaviour seems reasonable (although I would like the background to > change sooner on M). > > > > On cast we probably want the notification while it is loading, so that it can > be cancelled (particularly if, as sometimes happens, it gets stuck loading or > loads very slowly). > > > > Why, anyway, do we want to avoid remote control (of volume, for example) when > play/pause isn't available? > > I'm not against volume control when play/pause isn't available (makes sense for > games using the Cast SDK, for example), I want to avoid unreasonable behavior > like having the notification on the watch / lock screen before anything starts > playng or side effects like receiving play/pause commands via headset before the > playback even started (if we receive those, we need to handle them more > carefully now). > > I tested the patch locally with M and the watch and headset. It seems like the > volume controls don't work until something is playing (so even if it's paused, > the hardware buttons on the phone don't work for Cast/VideoFling while the watch > allows to change volume). Perhaps, we need to set some media session state to be > able to use those -- but then the watch notification might appear earlier. Moved inside the if block, since this makes the current behavior explicit. https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:600: MediaRouter.getInstance(mContext).setMediaSessionCompat(mMediaSession); On 2016/02/09 20:05:46, whywhat wrote: > As we know, this could cause a crash on some versions of JB. We should either > make sure the crash was fixed (e.g. in the support library) or that we handle > the exception here as we do in other places. > Time to write that MediaRouterUtil class? Not sure a class is worthwhile, since we may want to handle the exception differently in each case; however I am now catching the exception. https://codereview.chromium.org/1679923005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:612: // TODO(avayvod) either the comment or the code is wrong! This will only be reached On 2016/02/09 20:05:46, whywhat wrote: > I think the comment is wrong, just remove it. Done.
The CQ bit was checked by aberent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679923005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679923005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avayvod@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679923005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679923005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Re-enable volume control for Android Cast Previously this was enabled through the (lock screen) RemoteControlClient, but this was removed in https://codereview.chromium.org/1652163002/. Reenable it through the notification's MediaSessionCompat. BUG=585393 ========== to ========== Re-enable volume control for Android Cast Previously this was enabled through the (lock screen) RemoteControlClient, but this was removed in https://codereview.chromium.org/1652163002/. Reenable it through the notification's MediaSessionCompat. BUG=585393 Committed: https://crrev.com/d265521868584caae4c6db60f5103f2d602c5e65 Cr-Commit-Position: refs/heads/master@{#374729} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d265521868584caae4c6db60f5103f2d602c5e65 Cr-Commit-Position: refs/heads/master@{#374729} |
