1192748 - [B2G] Only send the moz-interrupt event when audio competing happens

Status: RESOLVED DUPLICATE of bug 1217711

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Alastor Wu [:alwu]]

We send the event "moz-interrupt-begin" when the muted status is changed.

However, in b2g, the initial muted state is true. After the system app open the audio, the muted state is changed to false. We don't want to send the event at that time. 

This issue is seperated from the Bug1129882 [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1129882#c86

Comment 2

[Alastor Wu [:alwu]]

[Blocking Requested - why for this release]:
Carry the flag status from the bug1186157.

Comment 3

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Bug 1192748 - only send moz-interrupt when audio competing happens

Comment 4

[Alastor Wu [:alwu]]

The "moz-interrupt" event should only be sent when the audio competing happens.

In present codebase, if the |aMuted| is different with the |mMuted|, we would send the event. However, we forgot to consider some cases, so we send the event at wrong time. (ex.bug1186157)

We should not send the event in following situations,
(1) In audio initialization (in b2g)
The audio would be muted in default, and then wait for the system app to turn it on. Even if the muted status would be changed, we shouldn't send the event. 

(2) Paused & Re-play
When we pause the audio, we should reset its muted status. The bug 1186157 is caused by that.

  Step0. Before start playing music : muted = MUTED_BY_AUDIO_CHANNEL
  Step1. Play music : muted = 0
  Step2. Pause music : muted = 0
  Step3. Replay music : muted = 0 <---- wrong, it should be reset to MUTED_BY_AUDIO_CHANNEL

Because we forgot to reset the muted status, in step3, we would send the "moz-interrupt-begin". It cause the music widget flashing.

---

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b36fbd3d444f

Comment 5

[Alastor Wu [:alwu]]

Found some errors, still need to modify the code of the AudioDestinationNode.

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Bug 1192748 - only send moz-interrupt when audio competing happens

Comment 7

[Alastor Wu [:alwu]]

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93347dabe2c7

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Bug 1192748 - only send moz-interrupt when audio competing happens

Comment 9

[Alastor Wu [:alwu]]

New try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=151c32db8fc7

Comment 10

[Alastor Wu [:alwu]]

Still have test case errors, need to fix them.

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Bug 1192748 - only send moz-interrupt when audio competing happens

Comment 12

[Alastor Wu [:alwu]]

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=558f4f1cd576

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Hi, Baku,
Could you help me review this patch?
The reason why we need this patch is in the comment4.
Very appreciate :)

---

Here are some modifications need to notice,
* AudioChannelService
 - Sometime we would call IsAudioChannelMutedByDefault() before the ctor of the AudioChannelService, so that we get the wrong value. Therefore, I change the get preference value from ctor to IsAudioChannelMutedByDefault().
 - When there is no audio playing, we should reset the mMuted of the window data.

* HTMLMediaElement
 - If we want to suspend element from audio channel, be sure that we have already start the audio channel agent. 
   Here is a wrong example,
    (1) set |mMuted| = MUTED_BY_AUDIOCHANNEL in ctor
    (2) suspend the element at wrong time (after the decoder setup, the NotifyOwnerDocumentActivityChangedInternal() would check |mMute| to decide whether need to be suspended)

 - Reset |mMuted| when the audio stop playing

Comment 18

[Alastor Wu [:alwu]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Bug 1192748 - only send moz-interrupt when audio competing happens

Move the modification of the AudioDestinationNode to bug1183033.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

https://reviewboard.mozilla.org/r/15543/#review15109

::: dom/audiochannel/AudioChannelService.cpp
(Diff revision 5)
> -bool sAudioChannelMutedByDefault = false;

why have you removed this?

::: dom/audiochannel/AudioChannelService.cpp
(Diff revision 5)
> -  Preferences::AddBoolVarCache(&sAudioChannelMutedByDefault,

same here.

::: dom/audiochannel/AudioChannelService.cpp:917
(Diff revision 5)
> -  return sAudioChannelMutedByDefault;
> +  // If true, any new AudioChannelAgent will be muted when created.

I don't see why you wanted to remove this static variable.

::: dom/html/HTMLMediaElement.cpp:4469
(Diff revision 5)
> +  if (aMuted != ComputedMuted()) {

This additional check is not needed. We have both conditions in the next if() and elseif()

::: dom/html/HTMLMediaElement.cpp:4481
(Diff revision 5)
> +        mHaveDispatchedInterruptBeginEvent = false;

This means that on B2G, by default an app is muted and when it's unmuted it doesn't receive any notification.
Why we don't want the 'mozinterruptend' in this case? If the app is muted by default we will receive a mozinterruptend without a begin. It seems to me that the app should take care of this.

::: dom/html/HTMLMediaElement.cpp:4553
(Diff revision 5)
> +    mMuted |= AudioChannelService::IsAudioChannelMutedByDefault() ?

why this?

Comment 20

[Alastor Wu [:alwu]]

Hi, Baku,
Here are some detailed in the comment14.

(In reply to Andrea Marchesini (:baku) from comment #19)
> Comment on attachment 8645668 [details]
> MozReview Request: Bug 1192748 - only send moz-interrupt when audio
> competing happens
> 
> https://reviewboard.mozilla.org/r/15543/#review15109
> 
> ::: dom/audiochannel/AudioChannelService.cpp
> (Diff revision 5)
> > -bool sAudioChannelMutedByDefault = false;
> 
> why have you removed this?
> 
> ::: dom/audiochannel/AudioChannelService.cpp
> (Diff revision 5)
> > -  Preferences::AddBoolVarCache(&sAudioChannelMutedByDefault,
> 
> same here.
> 
> ::: dom/audiochannel/AudioChannelService.cpp:917
> (Diff revision 5)
> > -  return sAudioChannelMutedByDefault;
> > +  // If true, any new AudioChannelAgent will be muted when created.
> 
> I don't see why you wanted to remove this static variable.

The beginning is that, we used the IsAudioChannelMutedByDefault() before the AudioChannelService's ctor, so that we got the wrong value of initial muted state.

The first thing I tried is to move Preferences::AddBoolVarCache() into the IsAudioChannelMutedByDefault(). However, It would get the crash like the bug1196084. It seems that in some cases we would attempt to add a bool pref repeatedly. 

Therefore, I think we can replace AddBoolVarCache() with GetBool, let would ensure two things. (1) get the correct muted state (2) to avoid the crash. 


> ::: dom/html/HTMLMediaElement.cpp:4469
> (Diff revision 5)
> > +  if (aMuted != ComputedMuted()) {
> 
> This additional check is not needed. We have both conditions in the next
> if() and elseif()

Without this check, we would send the "mozinterruptend" in the beginning.

> ::: dom/html/HTMLMediaElement.cpp:4481
> (Diff revision 5)
> > +        mHaveDispatchedInterruptBeginEvent = false;
> 
> This means that on B2G, by default an app is muted and when it's unmuted it
> doesn't receive any notification.
> Why we don't want the 'mozinterruptend' in this case? If the app is muted by
> default we will receive a mozinterruptend without a begin. It seems to me
> that the app should take care of this.

I think we should only dispatch these events when the audio competing happens, because some reasons.
(1) That might make us debug more efficiently in the future. 
(2) If some apps handle the mozinterruptend and do something, it would cause unexpected result.

Actually, the bug1186157 is caused by sending moz-interrupt at wrong time. The music app would change the playback widget when receive the moz-interrupt events.

> ::: dom/html/HTMLMediaElement.cpp:4553
> (Diff revision 5)
> > +    mMuted |= AudioChannelService::IsAudioChannelMutedByDefault() ?
> 
> why this?

The muted state should be reset when the music stop playing or start a new load. I want to make ensure that we would not send the wrong moz-interrupt event.

---

Very appreciate :)

Comment 21

[Alastor Wu [:alwu]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Bug 1192748 - only send moz-interrupt when audio competing happens

Move the modification of the AudioDestinationNode to bug1183033.

Comment 22

[Alastor Wu [:alwu]]

After offline discussion with Baku and Evan, we decided to move out the "moz-interrupted-event".

We'll go through with three stages,

(1) Implement the new methods in AudioChannelAPI, maybe like "SendResume/SendInterruptedEnd".
The purpose of that is to split the interrupted event and resume behavior. The interrupted events would change from per element to per app. The system app would notify the app if the interrupted ended.

(2) Modification in system app 
System app can the navigator.mozSetMessageHandler to send the notification to other apps.

(3) Change all apps which are using the moz-interrupted event
Change them to adopt new method.

Comment 23

[Alastor Wu [:alwu]]

After offline discussion with Baku and Evan, we decided to move out the "moz-interrupted-event".

We'll go through with three stages,

(1) [Gecko] Implement the new methods in AudioChannelAPI, maybe like "SendResume/SendInterruptedEnd".
The purpose of that is to split the interrupted event and resume behavior. The interrupted events would change from per element to per app. The system app would notify the app if the interrupted ended.

(2) [Gaia] Modification in system app 
System app can the navigator.mozSetMessageHandler to send the notification to other apps.

(3) [Gaia] IChange all apps which are using the moz-interrupted event
Change them to adopt new method.

Comment 24

[Andrea Marchesini [:baku]]

Alastor, do we still need this patch? During the workweek we decided to go for a different path and remove the moz-interrupt events. Do you still want me to review this patch?

Comment 25

[Alastor Wu [:alwu]]

Comment on attachment 8645668 [details]
MozReview Request: Bug 1192748 - only send moz-interrupt when audio competing happens

Hi, Baku,
Following our conclusion, yes, we'll remove the "moz-interrupt" event.
Cancel review.

Comment 26

[Alastor Wu [:alwu]]

Hi, all,
I propose to remove this bug out from the 2.5 blocker, that is because some following reasons,

---

(1) Bug situation is not serious
Although this bug has lots of duplicate bugs, but most of them have already been solved.
In addition, these bugs are not related with the "moz-interrupt" event, these bugs were set the duplicate just because my previous patch can solve them.

The only bug related with the "moz-interrupt" event is the bug1186157, the music widget would flash one time when we press the play/pause button. 

I think this issue is not serious as the blocker.

---

(2) Lots works need to do
The best solution is that we'll remove the "moz-interrupt" event from the MediaElement/AudioContext, and move them to the system message. System app can use the new AudioChannel API method to notify apps about the audio interruption.

We can see the solving plan in comment 23, there are different stages.

Now, we're in the stage 1, implementing the new method of the AudioChannel API. (Bug 1206581) After that, we still need to "implement new logic in system app" and then "need to modify all app to adopt new method".

I think these works might be not finished before v2.5 branch out.

Comment 27

[Bobby Chien]

As comment #26, this issue doesn't impact user's daily phone usage. It's good to be rescheduled to next version (v2.6). According to blocking-b2g:2.6 didn't be created now, I just noted milestone as FxOS-S21 and leave it here.

Comment 28

[Blake Wu [:bwu][:blakewu]]

Per comment 26 and comment 27, make this bug not a blocker.

Comment 29

[Alastor Wu [:alwu]]

Since this bug has some redundant information, I'll open an bug to track this issue.