Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1180253002:
Chromecast: unmute audio stream on Cast session finish on pre-M devices. (Closed)
DescriptionChromecast: unmute audio stream on Cast session finish on pre-M devices.
R=halliwell@chromium.org,byungchul@chromium.org
BUG=internal b/19964892
Committed: https://crrev.com/7bfe4daab14676db218d3a4acfb523d164b7e98d
Cr-Commit-Position: refs/heads/master@{#334442}
Patch Set 1 #
Total comments: 4
Patch Set 2 : always get AudioManager from application context #
Total comments: 2
Patch Set 3 : CastShellActivity: use "this" instead of getApplicationContext #
Messages
Total messages: 22 (6 generated)
One note: it took a lot of experimentation to find the "right place" to invoke the unmute. I chose CastShellActivity.onStop because it's the most reliable way to know that a Cast session has actually ended, even if that session was across multiple Cast apps---that is, not unmuting when going from one Cast app to the next. I had also tried unmuting when we get an APP_STATE_STARTING from our "idle screen" app, but it turns out there's a bit of lag between getting APP_STATE_STOPPED and the render process having actually died, so if I stopped casting YouTube while playback was occurring (muted), you'd end up hearing about ~200ms of audio content after unmuting. Plus this has awkward edge cases at startup time, particularly since the TV (in Amai case, at least) shows a dialog when the unmute occurs.
https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java (right): https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java:13: */ I don't understand the need for this class. I don't see a change to use this API to mute things in the first place?
https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java (right): https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java:13: */ On 2015/06/13 00:54:04, halliwell wrote: > I don't understand the need for this class. I don't see a change to use this > API to mute things in the first place? Oops, I forgot to mail the corresponding internal CL. AVSettingsAndroid has its own AudioManager instance, and muting/unmuting usually goes through that. I have AVSettingsAndroid now get its AudioManager instance through this class so that we can unmute on Cast session teardown.
On 2015/06/13 18:32:10, gunsch wrote: > https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... > File > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java > (right): > > https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java:13: > */ > On 2015/06/13 00:54:04, halliwell wrote: > > I don't understand the need for this class. I don't see a change to use this > > API to mute things in the first place? > > Oops, I forgot to mail the corresponding internal CL. > > AVSettingsAndroid has its own AudioManager instance, and muting/unmuting usually > goes through that. I have AVSettingsAndroid now get its AudioManager instance > through this class so that we can unmute on Cast session teardown. lgtm
gunsch@chromium.org changed reviewers: + lcwu@google.com
[+lcwu for committer stamp]
https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java (right): https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java:20: sAudioManager = (AudioManager) context.getSystemService(Context.AUDIO_SERVICE); Could audio manager be different according to context in same application? Or, need to call like context.getApplicationContext().getSystemService()?
https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java (right): https://codereview.chromium.org/1180253002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastAudioManager.java:20: sAudioManager = (AudioManager) context.getSystemService(Context.AUDIO_SERVICE); On 2015/06/15 17:30:17, byungchul wrote: > Could audio manager be different according to context in same application? Or, > need to call like context.getApplicationContext().getSystemService()? Good suggestion, updated to getApplicationContext(). Docs are explicit: "In general, do not share the service objects between various different contexts (Activities, Applications, Services, Providers, etc.)"
lgtm A nit. https://codereview.chromium.org/1180253002/diff/20001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java (right): https://codereview.chromium.org/1180253002/diff/20001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:180: CastAudioManager.getAudioManager(getApplicationContext()) CastAudioManager.getAudioManager(this)
The CQ bit was checked by gunsch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org, byungchul@chromium.org Link to the patchset: https://codereview.chromium.org/1180253002/#ps40001 (title: "CastShellActivity: use "this" instead of getApplicationContext")
https://codereview.chromium.org/1180253002/diff/20001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java (right): https://codereview.chromium.org/1180253002/diff/20001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:180: CastAudioManager.getAudioManager(getApplicationContext()) On 2015/06/15 18:27:58, byungchul wrote: > CastAudioManager.getAudioManager(this) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180253002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lcwu@chromium.org changed reviewers: + lcwu@chromium.org
lgtm
The CQ bit was checked by gunsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180253002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7bfe4daab14676db218d3a4acfb523d164b7e98d Cr-Commit-Position: refs/heads/master@{#334442} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
