Closed Bug 1183033 Opened 9 years ago Closed 9 years ago

[B2G] Keyboard doesn't have click sound

Categories

(Firefox OS Graveyard :: AudioChannel, defect, P1)

Product:
Firefox OS Graveyard
Component:
AudioChannel
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:2.5+, firefox44 fixed, b2g-master verified)

Status:
VERIFIED FIXED
Milestone:
FxOS-S10 (30Oct)
Project Flags:
blocking-b2g 2.5+
Tracking Flags:
Tracking Status
firefox44 --- fixed
b2g-master --- verified

People

(Reporter: alwu, Assigned: alwu)

References

Details

(Whiteboard: permafail)

Attachments

(1 file, 2 obsolete files)

MozReview Request: Bug 1183033 - Don't mute the system channel type.
40 bytes, text/x-review-board-request
baku
: review+
Details 
After using the new audio channel architecture, the keyboard click sound can't work normally.
[Bug root cause]
In present, when the input audio chunk is null or muted, we would stop the audio channel agent of the Audio Destination Node.

However, the agent might not been activated yet by the system app.

So, we would get this situation,
1. agent call StartPlaying()
   - add agent to window data
2. send notification to system app
   - default is muted, need to wait for the response of the system app
3. agent call StopPlaying()
   - remove agent to window data
4. system app call SetAudioChannelMute()
   - there is no agent in window data, that is a useless operation

[Solution]
Therefore, I postpone the StopPlaying() until the agent really gets the response from the system app. 

---

Hi, Baku,
Could you help me review this patch?
Thanks :)
Attachment #8632706 - Flags: review?(amarchesini)
Blocks: 1130350
No longer depends on: 1130350
Different parts but related, this bug is depend on bug1180614.
No longer blocks: 1130350
Depends on: 1180614
Blocks: 1130350
Comment on attachment 8632706 [details] [diff] [review]
Stop agent after the audio channel is activated by system app

Review of attachment 8632706 [details] [diff] [review]:
-----------------------------------------------------------------

I want to propose a different approach, can you tell me if this makes sense?

0. Create a Mutedtype like this: enum { MutedByAudioChannel = 1, MutedByStream = 2 }
1. Let's be 'muted' uint32_t and use it as a bitmap.
2. by default this muted is equal to: AudioChannelService::MutedByDefault()
3. When you receive WindowVolumeChanged(volume, muted == true) then:
 3a. send the notification and set the muted to |= MutedByAudioChannel
4. When the stream is muted because no inputs (InputMuted(true/false)) then do muted |= MutedByStream
 4a. and call NotifyStoppedPlaying.

This should fix the problem, correct?
It's a bit more work, but it's similar to what we do in HTMLMediaElement.

::: dom/media/webaudio/AudioDestinationNode.cpp
@@ +670,5 @@
>      return;
>    }
>  
>    if (aMuted) {
> +    // Sometime we would stop the agent before the system app opens it, because

I don't like all of this. It's too b2g specific.
Attachment #8632706 - Flags: review?(amarchesini) → review-
Carrying blocking+ from bug 1189115.
blocking-b2g: --- → 2.5+
(In reply to Andrea Marchesini (:baku) from comment #4)
> 
> I want to propose a different approach, can you tell me if this makes sense?
> 
> 0. Create a Mutedtype like this: enum { MutedByAudioChannel = 1,
> MutedByStream = 2 }
> 1. Let's be 'muted' uint32_t and use it as a bitmap.
> 2. by default this muted is equal to: AudioChannelService::MutedByDefault()
> 3. When you receive WindowVolumeChanged(volume, muted == true) then:
>  3a. send the notification and set the muted to |= MutedByAudioChannel
> 4. When the stream is muted because no inputs (InputMuted(true/false)) then
> do muted |= MutedByStream
>  4a. and call NotifyStoppedPlaying.
> 
> This should fix the problem, correct?
> It's a bit more work, but it's similar to what we do in HTMLMediaElement.
> 

Hi, Baku,
Change |mMuted| to a Mutedtype is a good idea, but I have some questions.

Do you means that we still call NotifyStoppedPlaying() in InputMuted()?
However, it doesn't solve the problem that the AudioDestinationNode might be not started before we stop it, right?
Or I have forgot something?

Very appreciate :)
Flags: needinfo?(amarchesini)
> Do you means that we still call NotifyStoppedPlaying() in InputMuted()?

Yes. We should call it because the stream is muted.

> However, it doesn't solve the problem that the AudioDestinationNode might be
> not started before we stop it, right?

Well... in this case the AudioDestinationNode will be muted by the AudioChannelService. Correct?
Then you can unmute it if this node is still playing audio.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #8)
> Well... in this case the AudioDestinationNode will be muted by the
> AudioChannelService. Correct?
> Then you can unmute it if this node is still playing audio.

Hi, Baku,
The AudioDestinationNode is muted by InputMuted() before the system app open it.
We need to postpone to unregister audio agent. Otherwise, we can't get WindowVolumeChanged(aMute=false).

---
And the bug situation is we can only heard the first two sound, let me explain why,
(1) First click sound
In the beginning, we would set the InputMuted(false) in AudioDestinationNode::CreateAudioChannelAgent.
That ensure the first sound can be heard.

After the first click sound ended, the AudioChannelAgent would be set to nullptr, so the WindowVolumeChanged(aMute=true) can't be called. (the agent is unregistered)

(2) Second click sound
Because we didn't call WindowVolumeChanged(aMute=true) before, the sound can be heard.

Notice that, although the audio should be muted by default in b2g, I found that the InputMuted(false) (called by having audio chunk) seems that be called after the sound really be heard. In this step, we set WindowVolumeChanged(aMute=true).

And then the audio chunk is null, we set the InputMuted(true). Therefore, we unregister the agent and then we can't receive the open command from the system app. We still stay at the muted state.

(3) Third sound sound
Because we are in the muted state, any sound can't be heard.
Flags: needinfo?(amarchesini)
> And the bug situation is we can only heard the first two sound, let me
> explain why,
> (1) First click sound
> In the beginning, we would set the InputMuted(false) in
> AudioDestinationNode::CreateAudioChannelAgent.
> That ensure the first sound can be heard.

By default, in b2g, any AudioDestinationNode is muted. If the system app doesn't unmute the app, I think it's correct that the AudioDestinatioNode stays muted. If this first sound is active, with the MutedType, this should be fixed.

> After the first click sound ended, the AudioChannelAgent would be set to
> nullptr, so the WindowVolumeChanged(aMute=true) can't be called. (the agent
> is unregistered)

Correct. No audio, no audioChannelAgent.

> (2) Second click sound
> Because we didn't call WindowVolumeChanged(aMute=true) before, the sound can
> be heard.

This seems wrong. The second click should be muted as the first one if the system app didn't change the audio state.
This can be fixed with MutedType. Right?

> (3) Third sound sound
> Because we are in the muted state, any sound can't be heard.

Correct. Tell me more where the issue is.
Flags: needinfo?(amarchesini) → needinfo?(alwu)
(In reply to Andrea Marchesini (:baku) from comment #10)
> By default, in b2g, any AudioDestinationNode is muted. If the system app
> doesn't unmute the app, I think it's correct that the AudioDestinatioNode
> stays muted. If this first sound is active, with the MutedType, this should
> be fixed.

We create a new AudioContext when we open the keyboard app, and then call InputMuted(false) in CreateAudioChannelAgent(). Therefore, before the audio actually be playback, we have already started agent and get the open command from the system app. 

Flow like that,
> - Open app
> - Create AudioContext & Destination node
> - Start agent in CreateAudioChannelAgent()
> - Receive unMuted from the system app, any sound of this window can be playback
> - (Idle)
> - Touch keyboard and the sound can be playback

---

> This seems wrong. The second click should be muted as the first one if the
> system app didn't change the audio state.
> This can be fixed with MutedType. Right?

Here is the strange point. The flow is like that,

> - Touch keyboard (second sound)
> - InputMuted(false)
> - Start agent and call WindowVolumeChanged(aMute=true) 
> - (Heard the sound)
> - InputMuted(true), release agent.
> - System app open the sound (agent has been released, this step is no meaning)

Even if the above steps have set the audio muted, the sound still can be heard. This WindowVolumeChanged(aMute=true) would affect next sound(third click), so we can't hear the third click sound. It seems that the sound has already been playback before we call the InputMuted(false).
Flags: needinfo?(alwu)
The problems we face are,

(1) Stop agent before the system app un-mutes it 
In DestinationNodeEngine, we call InputMuted(false) first, and then call InputMuted(true) very soon. Therefore, we can't open the AudioDestinationNode.

That is why I want to postpone the NotifyStoppedPlaying(), and I don't understand how to solve it with the enum mute type.

(2) InputMuted is called after the sound actually be heard ()
This is only my assumption, and I can't make sure of that. The description is in comment11 (about why the sound click sound can be heard).
Flags: needinfo?(amarchesini)
Depends on: 1192748
Bug 1183033 - Muted node only after the node is initialized by AudioChannelAPI.

In this patch, we would check whether the AudioDestinationNode is initialized by AudioChannelAPI. We need to make sure not to close the AudioDestinationNode before its initialization.

This patch is depended on the bug1192748, because that bug has the fix for IsAudioChannelMutedByDefault().
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

Hi, Baku,
Could you help me review this patch?
Very appreciate :)
Flags: needinfo?(amarchesini)
Attachment #8649152 - Flags: review?(amarchesini)
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

Bug 1183033 - Muted node only after the node is initialized by AudioChannelAPI.

In this patch, we would check whether the AudioDestinationNode is initialized by AudioChannelAPI. We need to make sure not to close the AudioDestinationNode before its initialization.

This patch is depended on the bug1192748, because that bug has the fix for IsAudioChannelMutedByDefault().
Attachment #8649152 - Flags: review?(amarchesini)
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

Rebase the patch.
Attachment #8649152 - Flags: review?(amarchesini)
Whiteboard: [2.5-aries-test-run-2]
After offline discussion with Baku and Evan, we decided to use the Gaia solution for this issue.

Now the Gaia would close the audio channel after the sound ended, and we think that is no necessary. If the app is still in the foreground, we should not close its audio channel if it have already asked for the audio channel permission before.

---

Evan will help to this issue.
Assignee: alwu → nobody
Component: AudioChannel → Gaia::System::Audio Mgmt
Flags: needinfo?(evan)
Any update?
Assignee: nobody → evan
Status: NEW → ASSIGNED
Flags: needinfo?(evan)
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

Should this be obsolete as we decided to fix it in Gaia?

(I don't know the detail and it was not recorded here...)
Attachment #8649152 - Attachment is obsolete: true
Attachment #8649152 - Flags: review?(amarchesini)
Yes, I'll fix this in Gaia.
After thought deeply, if we fix this in Gaia, we might need to do big change and we might have more regression bugs. I think doing bug change is not suitable for a blocker bug.

We can do the refactor in another bug, no in this blocker bug. I think we can fix this bug in gecko first. Then we can refactor this in another bug.

How do you think, Alastor? Let's discuss this in person tomorrow.
Assignee: evan → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(alwu)
Component: Gaia::System::Audio Mgmt → AudioChannel
And we will do the refactor in Bug 1206691.
Priority: -- → P1
Depends on: 1206691
Hi Bobby,

It doesn't mean this bug depends Bug 1206691.

The Comment 23 means we can fix this in gecko first. Then we can refactor Audio Channel Service in next release or other release for performance things.

I don't think we should do big change in a blocker bug.
No longer depends on: 1206691
Hi Alastor,

If we implement Media Focus in gaia, we need to do big changes for refactoring the Audio Channel Service[1].

Currently, we assume all audio channel can not be played be default in Audio Channel Service. But Media Focus assumes audio channel in foreground can be play be default. They're totally different assumptions. So it will be a big change if we implement Media Focus. And big change might cause regression bugs.

My suggestion is that we fix this bug in gecko first, and we implement Media Focus to refactor Audio Channel Service in another bug(Bug 1206691).

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/audio_channel_service.js
Attachment #8649152 - Attachment is obsolete: false
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

Bug 1183033 - Muted node only after the node is initialized by AudioChannelAPI.

In this patch, we would check whether the AudioDestinationNode is initialized by AudioChannelAPI. We need to make sure not to close the AudioDestinationNode before its initialization.
Assignee: nobody → alwu
Flags: needinfo?(alwu)
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

Hi, Baku,
According to the comment26, it seems that we need to have a Gecko solution.
Could you help me feedback this patch?
Very appreciate!

---

FYI, in DestinationNodeEngine::ProcessBlock(),
Because the audio click is too short, it causes that the mute/unmute would be send almost the same time. 

> 09-22 16:42:11.688  7611  7987 I Gecko   : DD | newInputMuted = 0 
> 09-22 16:42:11.688  7611  7987 I Gecko   : DD | newInputMuted = 1
Attachment #8649152 - Flags: feedback?(amarchesini)
Hi, Baku,
Because of comment26, we can't solve this issue in Gaia now.
Could you help me feedback this patch?
Very appreciate :)
Flags: needinfo?(amarchesini)
Whiteboard: [2.5-aries-test-run-2] → permafail
Hi, Baku,
Could you help me to feedback this bug?
According comment26, we need to solve it in Gecko now, but the final goal is still to solve it in Gaia.
However, since it's a P1 blocker, we need to solve it ASAP.
Very appreciate :)
Blocks: 1168609
https://reviewboard.mozilla.org/r/16363/#review15969

::: dom/media/webaudio/AudioDestinationNode.cpp:518
(Diff revision 1)
> -    if (UseAudioChannelAPI()) {
> +  if (aMuted && !ComputeMuted()) {

if (aMuted) {
  MOZ_ASSERT(!ComputeMuted());

::: dom/media/webaudio/AudioDestinationNode.cpp:524
(Diff revision 1)
> +  } else if (!aMuted && ComputeMuted()) {

} else {
  MOZ_ASSERT(ComputeMuted());

::: dom/media/webaudio/AudioDestinationNode.cpp:747
(Diff revision 1)
> +  return (mAudioChannelAgent);

return !!mAudioChannelAgent;

::: dom/media/webaudio/AudioDestinationNode.cpp:228
(Diff revision 4)
> +    , mDestinationNode(aNode)

You don't need this. Just use: NodeMainThread().

::: dom/media/webaudio/AudioDestinationNode.cpp:258
(Diff revision 4)
> +      DispatchMutedRunnable(aStream, mPendingMuted);

I don't understand this part:
newInputMuted can be true or false here, but you are ignoring it. Why?

::: dom/media/webaudio/AudioDestinationNode.cpp:299
(Diff revision 4)
> +  AudioDestinationNode* mDestinationNode;

remove this.

::: dom/media/webaudio/AudioDestinationNode.cpp:727
(Diff revision 4)
> -    mAudioChannelAgent->NotifyStoppedPlaying(nsIAudioChannelAgent::AUDIO_AGENT_NOTIFY);
> +    DestroyAudioChannelAgent();

why do you destroy the audioAgent when this is muted?
I propose to have a meeting about this issue. What about your Tuesday 8 UTC? It should be 4pm in Taipei.
Flags: needinfo?(amarchesini)
Flags: needinfo?(alwu)
Sure, but Evan have meeting in Tuesday 4pm, we need to wait for his response.
I'll send the invitation after checking his free time.
Flags: needinfo?(alwu)
Hi, Baku,
Here I propose another solution and it's not just a workaround. 
This solution is equal to what we want to do in Gaia, and I think that is a better solution for you.

However, I must to said, this solution CAN'T solve this problem 100%, we still have a little problem.
Even if we use the Gaia soluion, this problem will still exist.
That is we'll still get FAIL on the FIRST short audio.

Let's me explain why..

---

We have known that if we have a short audio, it might be unregistered before the system app opens it, and that is the FIRST error reason why the keyboard can't be playback. The SECOND error reason is that the system app doesn't need to mute window after that window doesn't have any audible sound.

If we use my new solution or use Gaia solution, then we can solve the second error. BUT, we still have to solve the first error.

If we only solve the second error (using this new patch or Gaia solution), we would find that
> The first sound can't be heard, and the following sound can be heard correctly.

Therefore, I think we still need to solve the first bug, and that is what I did in the previous patch. 
I know that my previous patch might not be a good solution, but if you agree with that we need to solve both errors as well, we can try to figure out a better solution.

How do you think?
Attachment #8675286 - Flags: feedback?(amarchesini)
Why the first sound can't be heard?
In the present codebase, the AudioDesitinationNode would be muted/unmuted by the DestinationNodeEngine [1]. The DestinationNodeEngine would mute/unmute the AudioDesitinationNode , that depends on whether the audio chunk has data. (also other conditions, but it's not the point)

When the first sound starts to play,
(1) Input audio chunk has data.
    => newInputMuted = true, mLastInputMute = false 
    => call InputMuted(false), assign mLastInputMute to true

(2) In InputMuted(false), starts the AudioChannelAgent and gets the playable status from AudioChannelService
    => on b2g default is muted, so the AudioDesitinationNode can't playback.
    => it needs to wait for open permission sent from the system app 

(3) Since it's the short sound, audio chunk has already no data in very short period.
    => newInputMuted = false, mLastInputMute = true 
    => call InputMuted(true), assign mLastInputMute to false

(4) In InputMuted(true), unregister AudioChannelAgent and stop the AudioDesitinationNode  
 
(5) AudioChannelService receives the open permission, and wants to notify the AudioDesitinationNode
    => since the AudioChannelAgent have been unregistered, the AudioDesitinationNode can't receive the notification
    => the sound can't be heard.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/AudioDestinationNode.cpp#238
Blocks: 1206621
Depends on: 1216495
Attachment #8675286 - Attachment is obsolete: true
Attachment #8675286 - Flags: feedback?(amarchesini)
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

Bug 1183033 - Don't mute the system channel type.
Attachment #8649152 - Attachment description: MozReview Request: Bug 1183033 - Muted node only after the node is initialized by AudioChannelAPI. → MozReview Request: Bug 1183033 - Don't mute the system channel type.
Attachment #8649152 - Flags: feedback?(amarchesini) → review?(amarchesini)
Comment on attachment 8649152 [details]
MozReview Request: Bug 1183033 - Don't mute the system channel type.

https://reviewboard.mozilla.org/r/16363/#review20097
Attachment #8649152 - Flags: review?(amarchesini) → review+
No longer depends on: 1192748
After applying my patch, now the keyboard sound can be heard normally.
However, here still is a little problem, that is the first sound can't be heard when we start the keyboard app. But, if you press the cancel button in the search bar, and then click the keyboard button again, the first sound can be heard.

I think that is another bug related with AudioDestinationNode, we can file it and solve it later.
Keywords: checkin-needed
See Also: → 1217321
https://hg.mozilla.org/mozilla-central/rev/4084fa04400b
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S10 (30Oct)
This issue is verified fixed on the latest Flame and Aries 2.5 Master build.
Keyboard click sounds are audible, and play properly. 

Environmental Variables:
Device: Aries 2.5
BuildID: 20151026111709
Gaia: a677ddd3aa3a81058775938bd56008d96dbc78b0
Gecko: 5ca03a00d26823ce91ee0eaa2937bed605bd53c1
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 44.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Environmental Variables:
Device: Flame 2.5
BuildID: 20151026030217
Gaia: a677ddd3aa3a81058775938bd56008d96dbc78b0
Gecko: 5ca03a00d26823ce91ee0eaa2937bed605bd53c1
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 44.0a1 (2.5) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-master: --- → verified
Flags: needinfo?(jmercado)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: