Closed Bug 1317326 Opened 8 years ago Closed 8 years ago

When headset is unplugged, media playback should be paused.

Categories

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect

Tracking

(firefox53 verified)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: bwu, Assigned: maliu)

References

Details

Attachments

(1 file)

Bug 1317326 - Pause media playing when headset unplugged,
58 bytes, text/x-review-board-request
alwu
: review+
sebastian
: review+
Details 
As Alastor mentioned in our team meeting in Taipei, media playback on Fennec doesn't pause when headset is unplugged. 

I tried it on the latest nightly. Media playback was not paused.
Most users should expect media playback should be paused.
Summary: When headset is unplugged, media playback will not be paused. → When headset is unplugged, media playback should be paused.
Assignee: nobody → max
Max, 
Thank you to take this bug!
Without figuring out the priority of this feature first, Max and I talked and we have a little technical discussion.

The topmost question here would be, is there something exists in the media platform that allow the front-end code in chrome process to monitor and control *all* tabs on *all processes*? If there is, it should not be hard to hook platform specific code onto it.

If these isn't such infrastructure, we would have to do that in the front-end JavaScript world by hook up frame scripts to monitor every tab ourselves.

(For this feature we would need to monitor (a) audio start of any tab and (b) audio stop of all tabs, and we would need (c) an interface to stop audio in all tabs)
Flags: needinfo?(bwu)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> If these isn't such infrastructure, we would have to do that in the
> front-end JavaScript world by hook up frame scripts to monitor every tab
> ourselves.

Some plumbing made in bug 486262 deep in the media service should be reusable for building this on top of it.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> (For this feature we would need to monitor (a) audio start of any tab and
> (b) audio stop of all tabs, and we would need (c) an interface to stop audio
> in all tabs)

(a) listen the "AudioPlayback:Start" event [1] 
(b) listen the "AudioPlayback:Stop" event [2]
(c) pauseMedia(true /* disposable */) [3]

[1] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/toolkit/content/widgets/browser.xml#1034
[2] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/toolkit/content/widgets/browser.xml#1037
[3] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/toolkit/content/widgets/browser.xml#755
FYI, pauseMedia() is to pause specific tab, not all tabs. On Fennec, we don't allow multiple tab playing at the same time (using pref "dom.audiochannel.audioCompeting").
(Alastor also pointed me to 
http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/toolkit/content/browser-content.js#1021 over IRC)

So I guess if we want to go with this route, this is the part we could reuse ...

(Alastor however point me to bug 1240423 and that maybe the better route)
My naive approach would be to register a BroadcastReceiver in MediaControlService that listens to Intent.ACTION_HEADSET_PLUG / AudioManager.ACTION_HEADSET_PLUG events and then calls pause() on the MediaController/TransportControls. It's only a couple of lines - wouldn't this be enough here? MediaControlService already does the heavy lifting of keeping track of which tab plays etc.
That's the code implemented in bug 1240423 right? Yeah I wasn't aware of that because I don't work on Fennec :P.
Flags: needinfo?(bwu)
Comment on attachment 8814675 [details]
Bug 1317326 - Pause media playing when headset unplugged,

https://reviewboard.mozilla.org/r/95812/#review96004

Good call to use ACTION_AUDIO_BECOMING_NOISY here.

This looks good (with some nits), but somehow it is not working on my phone. When plugging headsets in/out it continues to play after a short break. Maybe Alastor knows why.I  added him as second reviewer as he has been working on MediaControlService mostly.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:166
(Diff revision 2)
>          if (!mInitialize) {
>              return;
>          }
>  
>          Log.d(LOGTAG, "shutdown");
> +        mHeadSetStateReceiver.unregisterReceiver(getApplicationContext());

I'd remove registering and unregistering the receiver to onCreate() / onDestroy(). That's where it should happen - the lifecycle of all other methods can change.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:441
(Diff revision 2)
> +        HeadSetStateReceiver registerReceiver(Context context) {
> +            IntentFilter intentFilter = new IntentFilter(AudioManager.ACTION_AUDIO_BECOMING_NOISY);
> +            context.registerReceiver(this, intentFilter);
> +            return this;
> +        }

Interesting pattern. Haven't seen this before. Not really sure if it is more readable, but let's add the  @CheckResult annotation.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:462
(Diff revision 2)
> +                Intent pauseIntent = new Intent(getApplicationContext(), MediaControlService.class);
> +                pauseIntent.setAction(ACTION_PAUSE);
> +                handleIntent(pauseIntent);

This part is a bit weird. If we build an Intent then I'd expect it to be used with startService() (and then the broadcast receiver class could be static) - Or: We build no Intent and call a shared method to pause.
Attachment #8814675 - Flags: review?(s.kaspari)
Comment on attachment 8814675 [details]
Bug 1317326 - Pause media playing when headset unplugged,

I'm unable to add alastor as reviewer because of the following error. So I'll just add him here on bugzilla instead:
> Unable to update reviewers as the review request has pending changes (the patch author has a draft)
Attachment #8814675 - Flags: review?(alwu)
Comment on attachment 8814675 [details]
Bug 1317326 - Pause media playing when headset unplugged,

https://reviewboard.mozilla.org/r/95812/#review96046

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:461
(Diff revision 2)
> +
> +            if (!AudioManager.ACTION_AUDIO_BECOMING_NOISY.equals(intent.getAction())) {
> +                return;
> +            }
> +
> +            if (isMediaPlaying()) {

Without this check it seems to work though.
Comment on attachment 8814675 [details]
Bug 1317326 - Pause media playing when headset unplugged,

https://reviewboard.mozilla.org/r/95812/#review96004

> This part is a bit weird. If we build an Intent then I'd expect it to be used with startService() (and then the broadcast receiver class could be static) - Or: We build no Intent and call a shared method to pause.

It could be static if we also pass-in some reference to check if isMediaPlaying(). If we pause through startService(), there is possible to be a burst gap of sound caused by binder queue busy. Though the shared method is handleMessage with mis-understanding Intent parameter, it wouldn't be better if we directly invoke this:"mController.getTransportControls().pause();" right?
Comment on attachment 8814675 [details]
Bug 1317326 - Pause media playing when headset unplugged,

https://reviewboard.mozilla.org/r/95812/#review96308

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:444
(Diff revision 3)
> +
> +    private class HeadSetStateReceiver extends BroadcastReceiver {
> +
> +        @CheckResult(suggest = "new HeadSetStateReceiver().registerReceiver(Context)")
> +        HeadSetStateReceiver registerReceiver(Context context) {
> +            IntentFilter intentFilter = new IntentFilter(AudioManager.ACTION_AUDIO_BECOMING_NOISY);

In [1], it said that the 'ACTION_AUDIO_BECOMING_NOISY' would also happen when switch audio route to speaker.

If so, I think we shouldn't pause music when change music from headset to speaker. If it's not the same as I think, please ingore this comment :)

[1] https://developer.android.com/reference/android/media/AudioManager.html
Attachment #8814675 - Flags: review?(alwu) → review+
Comment on attachment 8814675 [details]
Bug 1317326 - Pause media playing when headset unplugged,

https://reviewboard.mozilla.org/r/95812/#review96308

> In [1], it said that the 'ACTION_AUDIO_BECOMING_NOISY' would also happen when switch audio route to speaker.
> 
> If so, I think we shouldn't pause music when change music from headset to speaker. If it's not the same as I think, please ingore this comment :)
> 
> [1] https://developer.android.com/reference/android/media/AudioManager.html

Pause playback when audio route switched to speaker is this bug requested enhancement.
Comment on attachment 8814675 [details]
Bug 1317326 - Pause media playing when headset unplugged,

https://reviewboard.mozilla.org/r/95812/#review96488
Attachment #8814675 - Flags: review?(s.kaspari) → review+
Do we want to this feature on Android KK? (MediaControlService is only available on LL or above) 
If we want to pause media on KK, need to send the "mediaControlPaused" [1] event directly to Gecko.

[1] http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java#267
(In reply to Alastor Wu [:alwu] from comment #19)
Good point, seems we missed out the supported deploy scope in this patch.

Hi Blake, is it acceptable to reduce this function deploy scope to kk above?
Flags: needinfo?(bwu)
I think that's acceptable. The solution to that is back-porting MediaControlService by using MediaSession/MediaController etc. from the support library - with that we get the whole media notification etc. on older Android versions. I think I filed a bug for that in the past but I can't find it right now.
There it is: bug 1278624.
See Also: → 1278624
Agree with Sebastian!
Flags: needinfo?(bwu)
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/73cbeef4f940
Pause media playing when headset unplugged, r=alwu,sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/73cbeef4f940
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Verified as fixed in build 53.0a1 (2016-12-09);
Device: HTC Desire 820 (Android 6.0.1).
status-firefox53: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: