Closed Bug 1192748 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-, tracking-b2g:+)

RESOLVED DUPLICATE of bug 1217711
FxOS-S21 (01Apr)
blocking-b2g -
tracking-b2g +

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(1 file)

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
Duplicate of this bug: 1186157
[Blocking Requested - why for this release]:
Carry the flag status from the bug1186157.
blocking-b2g: --- → 2.5?
Bug 1192748 - only send moz-interrupt when audio competing happens
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
Found some errors, still need to modify the code of the AudioDestinationNode.
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 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
Still have test case errors, need to fix them.
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
Duplicate of this bug: 1186572
Summary: [B2G] Only send the moz-interrupt-begin when the audio is really be interrupted → [B2G] Only send the moz-interrupt event when audio competing happens
blocking-b2g: 2.5? → 2.5+
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
Attachment #8645668 - Flags: review?(amarchesini)
Duplicate of this bug: 1193626
Duplicate of this bug: 1193647
Duplicate of this bug: 1194637
See Also: → 1186677
Blocks: 1183033
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.
Blocks: 1130350
Attachment #8645668 - Flags: review?(amarchesini)
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?
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 :)
Flags: needinfo?(amarchesini)
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.
Attachment #8645668 - Flags: review?(amarchesini)
No longer blocks: 1186677
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.
Depends on: 1206581
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?
Flags: needinfo?(alwu)
Priority: -- → P2
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.
Flags: needinfo?(alwu)
Attachment #8645668 - Flags: review?(amarchesini)
No longer blocks: 1183033
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.
Flags: needinfo?(bwu)
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.
tracking-b2g: --- → +
Target Milestone: --- → FxOS-S21 (01Apr)
Per comment 26 and comment 27, make this bug not a blocker.
blocking-b2g: 2.5+ → -
Flags: needinfo?(bwu)
Priority: P2 → --
See Also: → 1217711
See Also: 1217711
Since this bug has some redundant information, I'll open an bug to track this issue.
Status: NEW → RESOLVED
Closed: 5 years ago
No longer depends on: 1206581
Resolution: --- → DUPLICATE
Duplicate of bug: 1217711
You need to log in before you can comment on or make changes to this bug.