819852 - Background music intermittently plays while the phone is ringing because looped <audio> doesn't occupied the channel the whole time

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

STR:
1. Open Music app
2. Play a song.
3. Call in by another phone.

Expected:
1. The music is totally muted or paused.

Actual:
1. The music plays intermittently when the ringing sound stops.(Before next ring)

Note:
This is not dupe of bug 819849.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Wrong flag.

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

BB+, P2 - severe usability issue

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bug 815569 won't actually help here. The only thing that will help is to use a looping audio rather than repeatedly calling .play().

Bug 815569 would just mean that we'd play a different set of snippets of music between the rings than we do now.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Bug 815569 won't actually help here. The only thing that will help is to use
> a looping audio rather than repeatedly calling .play().
> 
> Bug 815569 would just mean that we'd play a different set of snippets of
> music between the rings than we do now.

Dominic, would you give some updates on your proposed fix?

Comment 5

[Dominic Kuo [:dkuo]]

I just took a look at the code of Dialer, and found out the audio element does use loop = true to play a ringtone.

I guess it might be an issue of the implementation on looping an audio in gecko, looks like while we are looping an audio in Dialer, it does not always occupy the mozAudioChannel, it releases mozAudioChannel after every cycle when the ringtone ends, so Music in background will play intermittently in the interval of end and start of that ringtone.

If my guessing is right, it should be possible to fix this bug, but a workaround actually.

Here is my proposal:
1. Listen to timeupdate event of audio element.

2. Check if audio.currentTime is smaller then audio.duration,
like (this.audio.currentTime >= this.audio.duration - 1), cannot be equal cause we don't want the audio to reach to the end, then trigger mozAudioChannel change.

3. Seek audio.currentTime to 0 so it will just like a restart of the ringtone.

And I think Music app cannot do anything to help this bug, since Music shouldn't do anything to audio element when mozAudioChannel changes, except the UI works.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If the music app is already using looping, then I think we should fix this in the platform. I.e. no changes should be needed in the app, instead the platform should ensure to occupy the "ringer" channel as long as the element is looping.

Matthew, is this something you could take? Or Dominic, would you be able to?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This is now a platform audio bug, according to comment 6.

Marco, can you help? Thanks.

Comment 8

[Marco Chen [:mchen]]

yep, I take this.

Comment 9

[Andrew Overholt [:overholt]]

Andrea can take this.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, why don't we give this to someone that doesn't have a big pile of B2G bugs already :)

Comment 11

[Marco Chen [:mchen]]

Hi kinetik,

I want to help here because there is no Christmas Eve here.

Comment 12

[Marco Chen [:mchen]]

Attachment: Patch v1

To modify the UpdateAudioChannelPlayingState() as below.

If media element is not in pause state and attribute of Loop is true, then it means this media element will be continued to play again when it is in end of the stream.

So we keep audio channel state as playing in case above in order to prevent other sound fires in that gap.

Comment 13

[Marco Chen [:mchen]]

Attachment: Patch v2

Comment 14

[Joe Cheng [:jcheng] (please needinfo)]

is :kinetik available to review this bug? if not, will someone else be able to?

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Would this patch change the timing to fire audio-channel-changed event?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Which event is that?

It only changes behavior for looping elements, at least.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

it's an event that the AudioChannelService fires on Gaia whenever the currently active audio channel changes. I.e. when we switch from "normal" to "alarm" or to "none".

Comment 18

[Marco Chen [:mchen]]

Since media element with loop property doesn't release the audio channel until it is paused, the audio-channel-changed event will be held to this media element's channel (if it is highest one now).

Comment 19

[Marco Chen [:mchen]]

Wait for green lights from try server then check in patch.
https://tbpl.mozilla.org/?tree=Try&rev=72960020c8ec

Comment 20

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version

1. Pass the try server and ask for checkin-needed.
2. Add reviewer and a=blocking-basecamp.

Comment 21

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f42175818fe

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f42175818fe

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/fa7539fba95f

Comment 24

[Askeing Yen[:askeing][:fyen]]

Gaia:
  Rev     7fdd1dd731ccdcf09e0c51aeefc361adc4bfc11a
BuildID   20130124114510
Version   18.0

Verified.