Closed Bug 1580659 Opened 1 year ago Closed 5 months ago

do not run audio channel agent's audio capturing changed function during registering the agent

Categories

(Core :: Audio/Video: Playback, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(9 files)

We call audio channel agent's audio capturing callback during registering the agent [1], it makes the audio capturing callback happen before finishing registration.

There are two drawbacks, (1) it's hard to be aware of that the callback would be called before finishing AudioChannelAgent::NotifyStartedPlaying() [2], which causes unclear call flow (2) If someone checks AudioChannelAgent::IsPlayingStarted() [3] inside audio capturing callback, then we would find that the mIsRegToService is false even if we have registered the agent to AudioChannelService because mIsRegToService is updated in the last line in the AudioChannelAgent::NotifyStartedPlaying() but the audio capturing callback could be executed before that.

[1] https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/dom/audiochannel/AudioChannelService.cpp#723
[2] https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/dom/audiochannel/AudioChannelAgent.cpp#144
[3] https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/dom/audiochannel/AudioChannelAgent.cpp#285

See Also: → 1580662

nsIAudioChannelAgent was created a common interface for a usage of in both js and C++ before, now we have no any JS code would use nsIAudioChannelAgent, it's only used in C++ code.

Therefore, in a coming refactoring (bug1580662), we will remove nsIAudioChannelAgent and use AudioChannelAgent as the only interface. Here we can make these classes start to reference AudioChannelAgent, instead of nsIAudioChannelAgent.

We have already had the same checking in AudioCaptureStreamChange(), so we can remove the checking in AudioCaptureStreamChangeIfNeeded().

Now AudioChannelService calls WindowAudioCaptureChanged() implicitly whenever we add the agent to the service [1], it makes the audio capturing callback happen before finishing registration.

There are two drawbacks,
(1) it's hard to be aware of that the audio capturing callback would be called before finishing AudioChannelAgent::NotifyStartedPlaying() [2], which causes unclear call flow.

(2) If someone checks AudioChannelAgent::IsPlayingStarted() [3] inside audio capturing callback, then we would find that the mIsRegToService is false even if we have registered the agent to AudioChannelService because mIsRegToService is updated in the last line in the AudioChannelAgent::NotifyStartedPlaying(), but the audio capturing callback could be executed before that.

[1] https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/dom/audiochannel/AudioChannelService.cpp#723
[2] https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/dom/audiochannel/AudioChannelAgent.cpp#144
[3] https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/dom/audiochannel/AudioChannelAgent.cpp#285

Instead of calling those callback functions seperately, we could provide a function to pull those changes at once after starting the agent.

In addition, WindowXXXChanged are callback functions of nsIAudioChannelAgentCallback, so they should only be called by AudioChannelAgent, to indicate receiving the change from AudioChannelService. We should not call them directly.

As we start audio capturing explicitly, we should also take responsibility to stop audio capturing when we don't need it.

We were hiding too many details on AudioChannelAgent before, which allow us hard to know who and where we handle audio capturing.

After applying patch4, now we would pull the change from AudioChannelService everytime after starting the agent, so we don't need to rely on letting NotifyStartedPlaying() to modify the config and use it to update our state.

Now other WindowXXXChanged() would receive the change directly from their input parameter, we should make WindowVolumeChanged() consistent with them, instead of asking AudioChannelService again.

To hide some functions from AudioChannelAgent to avoid an access from AudioChannelAgentCallback (eg. media element, audio destination node...), and only allow AudioChannelService to use those functions.

I've been distracted by meetings today, sorry.
The situation may be similar tomorrow, but I'll see if I can find some time to look.

The suspend state and muting state are two different things, so changing suspend state should not affect on muting state.

Attachment #9093652 - Attachment description: Bug 1580659 - part3.5 : suspend nsNPAPIPluginInstance should not affect its muting state. → Bug 1580659 - part3.5 : suspend nsNPAPIPluginInstance should not affect its muted state.
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e92ff4ac7f43
part1 : use AudioChannelAgent directly. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/20cfcd96a40e
part2 : remove redundant checking. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/c25bfc8b31b1
part3 : call 'WindowAudioCaptureChanged()' explicitly. r=chunmin,karlt
https://hg.mozilla.org/integration/autoland/rev/b712f15af619
part3.5 : suspend nsNPAPIPluginInstance should not affect its muted state. r=karlt
https://hg.mozilla.org/integration/autoland/rev/ecb2bf9fb452
part4 : pulling the intial update after starting AudioChannelAgent. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/276827f54c71
part5 : stop audio capturing explicitly. r=chunmin,karlt
https://hg.mozilla.org/integration/autoland/rev/8b9f61694270
part6 : remove input config parameter from 'NotifyStartedPlaying()'. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/fd31977767d5
part7 : make AudioChannelAgent's 'WindowVolumeChanged()' consistent with other 'WindowXXXChanged()'. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/b8c46aaec410
part8 : trimming public methods of AudioChannelAgent. r=chunmin
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ab67517a95b
Backed out 9 changesets for causing linting failure in /builds/worker/checkouts/gecko/dom/plugins/base/nsNPAPIPluginInstance.cpp CLOSED TREE
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c56f14e9421
part1 : use AudioChannelAgent directly. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/c5afcdf73898
part2 : remove redundant checking. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/b52d87d498b8
part3 : call 'WindowAudioCaptureChanged()' explicitly. r=chunmin,karlt
https://hg.mozilla.org/integration/autoland/rev/91381001afa5
part3.5 : suspend nsNPAPIPluginInstance should not affect its muted state. r=karlt
https://hg.mozilla.org/integration/autoland/rev/46e39dfd51d2
part4 : pulling the intial update after starting AudioChannelAgent. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/c4bc95efde9a
part5 : stop audio capturing explicitly. r=chunmin,karlt
https://hg.mozilla.org/integration/autoland/rev/12f46e16c72c
part6 : remove input config parameter from 'NotifyStartedPlaying()'. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/99500f4bb146
part7 : make AudioChannelAgent's 'WindowVolumeChanged()' consistent with other 'WindowXXXChanged()'. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c2bbb774843e
part8 : trimming public methods of AudioChannelAgent. r=chunmin
Flags: needinfo?(alwu)
Regressions: 1590579
No longer regressions: 1590579
You need to log in before you can comment on or make changes to this bug.