Closed
Bug 1299390
Opened 8 years ago
Closed 7 years ago
clean up audio channel related code
Categories
(Core :: Audio/Video: Playback, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: alwu, Assigned: ben.tian, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
alwu
:
review+
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
alwu
:
review+
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
alwu
:
review+
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
alwu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
alwu
:
review+
|
Details |
We should remove b2g related part in all audio channel code.
Updated•8 years ago
|
Priority: -- → P5
Comment 1•7 years ago
|
||
Alastor, do you have plans to work on this? The mozaudiochannel bits in HTMLMediaElement::ParseAttribute are blocking some improvements to element cloning...
Flags: needinfo?(alwu)
Reporter | ||
Comment 2•7 years ago
|
||
Sure, I can remove them step by step and remove the related codes in HTMLMediaElement first. Filed bug1358061.
Reporter | ||
Comment 3•7 years ago
|
||
Will remove mozaudiochannel related codes in bug1358061.
Flags: needinfo?(alwu)
Reporter | ||
Comment 5•7 years ago
|
||
Need to remove BrowserElementAudioChannel API and these tests [1]. [1] https://goo.gl/KdH4xZ --- If anyone has interest to work on that, feel free to take this bug.
Assignee: alwu → nobody
Reporter | ||
Updated•7 years ago
|
Mentor: alwu
Comment 6•7 years ago
|
||
Setting straight to dev-doc-complete - the only thing not already archived was the mozAudiochannelType property of HTMLMediaElement, as I've archived that now.
Keywords: dev-doc-complete
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8867038 -
Flags: review?(alwu)
Attachment #8867039 -
Flags: review?(alwu)
Attachment #8867040 -
Flags: review?(alwu)
Attachment #8867041 -
Flags: review?(alwu)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8867038 [details] Bug 1299390 - part 1: Remove BrowserElementAudioChannel. https://reviewboard.mozilla.org/r/138636/#review142420 r+ with adding commit message and handling open issues, and this patch still needs to be reviewed by DOM peer because it includes idl-change. ::: dom/html/nsBrowserElement.cpp:16 (Diff revision 3) > -#include "mozilla/dom/BrowserElementAudioChannel.h" > #include "mozilla/dom/DOMRequest.h" > #include "mozilla/dom/ScriptSettings.h" > #include "mozilla/dom/ToJSValue.h" > > #include "AudioChannelService.h" Remove.
Attachment #8867038 -
Flags: review?(alwu) → review+
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8867040 [details] Bug 1299390 - part 3: Remove BrowserElement methods required by BrowserElementAudioChannel only. https://reviewboard.mozilla.org/r/138640/#review142434 This patch still needs to be reviewed by DOM peer because it includes idl-change.
Attachment #8867040 -
Flags: review?(alwu) → review+
Reporter | ||
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8867041 [details] Bug 1299390 - part 4: Clean up BrowserElementAudioChannel related test cases. https://reviewboard.mozilla.org/r/138642/#review142436
Attachment #8867041 -
Flags: review?(alwu) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8867038 -
Flags: review?(amarchesini)
Attachment #8867039 -
Flags: review?(amarchesini)
Attachment #8867040 -
Flags: review?(amarchesini)
Reporter | ||
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8867039 [details] Bug 1299390 - part 2: Remove AudioChannelService methods required by BrowserElementAudioChannel only. https://reviewboard.mozilla.org/r/138638/#review142428 This patch still needs to be reviewed by DOM peer because it includes idl-change. ::: dom/audiochannel/AudioChannelService.h:142 (Diff revision 3) > */ > void AudioAudibleChanged(AudioChannelAgent* aAgent, > AudibleState aAudible, > AudibleChangedReasons aReason); > > - /* Methods for the BrowserElementAudioChannel */ > + void SetAudioChannelVolume(nsPIDOMWindowOuter* aWindow, uint32_t aChannel, Why keep these two functions? ::: dom/audiochannel/AudioChannelService.h:154 (Diff revision 3) > > /** > * Return true if there is a telephony channel active in this process > * or one of its subprocesses. > */ > bool TelephonyChannelIsActive(); Remove, but this should be moved to new patch because it seems not very related with the topic of this patch. ::: dom/audiochannel/AudioChannelService.h:154 (Diff revision 3) > > /** > * Return true if there is a telephony channel active in this process > * or one of its subprocesses. > */ > bool TelephonyChannelIsActive(); Remove, but this should be moved to new patch because it seems not very related with the topic of this patch. ::: dom/audiochannel/AudioChannelService.h:160 (Diff revision 3) > > /** > * Return true if a normal or content channel is active for the given > * process ID. > */ > bool ProcessContentOrNormalChannelIsActive(uint64_t aChildID); Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:168 (Diff revision 3) > * AudioChannelManager calls this function to notify the default channel used > * to adjust volume when there is no any active channel. if aChannel is -1, > * the default audio channel will be used. Otherwise aChannel is casted to > * AudioChannel enum. > */ > virtual void SetDefaultVolumeControlChannel(int32_t aChannel, Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:171 (Diff revision 3) > * AudioChannel enum. > */ > virtual void SetDefaultVolumeControlChannel(int32_t aChannel, > bool aVisible); > > bool AnyAudioChannelIsActive(); Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:314 (Diff revision 3) > GetOrCreateWindowData(nsPIDOMWindowOuter* aWindow); > > AudioChannelWindow* > GetWindowData(uint64_t aWindowID) const; > > struct AudioChannelChildStatus final Remove AudioChannelChildStatus and the related codes. ::: dom/audiochannel/AudioChannelService.h:328 (Diff revision 3) > bool mActiveTelephonyChannel; > bool mActiveContentOrNormalChannel; > }; > > AudioChannelChildStatus* > GetChildStatus(uint64_t aChildID) const; Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:331 (Diff revision 3) > > AudioChannelChildStatus* > GetChildStatus(uint64_t aChildID) const; > > void > RemoveChildStatus(uint64_t aChildID); Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:335 (Diff revision 3) > void > RemoveChildStatus(uint64_t aChildID); > > nsTObserverArray<nsAutoPtr<AudioChannelWindow>> mWindows; > > nsTObserverArray<nsAutoPtr<AudioChannelChildStatus>> mPlayingChildren; Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:338 (Diff revision 3) > nsTObserverArray<nsAutoPtr<AudioChannelWindow>> mWindows; > > nsTObserverArray<nsAutoPtr<AudioChannelChildStatus>> mPlayingChildren; > > // Raw pointers because TabParents must unregister themselves. > nsTArray<TabParent*> mTabParents; Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:340 (Diff revision 3) > nsTObserverArray<nsAutoPtr<AudioChannelChildStatus>> mPlayingChildren; > > // Raw pointers because TabParents must unregister themselves. > nsTArray<TabParent*> mTabParents; > > nsCOMPtr<nsIRunnable> mRunnable; Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:342 (Diff revision 3) > // Raw pointers because TabParents must unregister themselves. > nsTArray<TabParent*> mTabParents; > > nsCOMPtr<nsIRunnable> mRunnable; > > uint64_t mDefChannelChildID; Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:346 (Diff revision 3) > > uint64_t mDefChannelChildID; > > // These boolean are used to know if we have to send an status update to the > // service running in the main process. > bool mTelephonyChannel; Same, remove but in another patch. ::: dom/audiochannel/AudioChannelService.h:352 (Diff revision 3) > bool mContentOrNormalChannel; > bool mAnyChannel; > > // This is needed for IPC comunication between > // AudioChannelServiceChild and this class. > friend class ContentParent; Same, remove but in another patch.
Attachment #8867039 -
Flags: review?(alwu) → review+
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8867039 [details] Bug 1299390 - part 2: Remove AudioChannelService methods required by BrowserElementAudioChannel only. https://reviewboard.mozilla.org/r/138638/#review142868 ::: dom/audiochannel/AudioChannelService.h:142 (Diff revision 3) > */ > void AudioAudibleChanged(AudioChannelAgent* aAgent, > AudibleState aAudible, > AudibleChangedReasons aReason); > > - /* Methods for the BrowserElementAudioChannel */ > + void SetAudioChannelVolume(nsPIDOMWindowOuter* aWindow, uint32_t aChannel, They should be removed. I'll update in the next version. ::: dom/audiochannel/AudioChannelService.cpp:946 (Diff revision 3) > anyChannel); > } > } > > void > AudioChannelService::ChildStatusReceived(uint64_t aChildID, Should we also remove this method since |AudioChannelChildStatus| is removed? If so how about [1]? Since only [2] calls method |ChildStatusReceived|. [1] http://searchfox.org/mozilla-central/source/dom/ipc/PContent.ipdl#857 [2] http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2666
Assignee | ||
Comment 25•7 years ago
|
||
ni? Alastor for comment 24. (In reply to Ben Tian [:btian] from comment #24) > Should we also remove this method since |AudioChannelChildStatus| is > removed? If so how about > [1]? Since only [2] calls method |ChildStatusReceived|. > > [1] http://searchfox.org/mozilla-central/source/dom/ipc/PContent.ipdl#857 > [2] > http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2666
Flags: needinfo?(alwu)
Reporter | ||
Comment 26•7 years ago
|
||
(In reply to Ben Tian [:btian] from comment #24) > ::: dom/audiochannel/AudioChannelService.cpp:946 > (Diff revision 3) > > anyChannel); > > } > > } > > > > void > > AudioChannelService::ChildStatusReceived(uint64_t aChildID, > > Should we also remove this method since |AudioChannelChildStatus| is > removed? If so how about > [1]? Since only [2] calls method |ChildStatusReceived|. > > [1] http://searchfox.org/mozilla-central/source/dom/ipc/PContent.ipdl#857 > [2] > http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2666 Yes, also remove related codes.
Flags: needinfo?(alwu)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8867038 [details] Bug 1299390 - part 1: Remove BrowserElementAudioChannel. https://reviewboard.mozilla.org/r/138636/#review143076
Attachment #8867038 -
Flags: review?(amarchesini) → review+
Comment 38•7 years ago
|
||
mozreview-review |
Comment on attachment 8867039 [details] Bug 1299390 - part 2: Remove AudioChannelService methods required by BrowserElementAudioChannel only. https://reviewboard.mozilla.org/r/138638/#review143078 ::: dom/audiochannel/AudioChannelService.cpp (Diff revision 5) > CreateServiceIfNeeded(); > return sAudioChannelCompeting; > } > > NS_INTERFACE_MAP_BEGIN(AudioChannelService) > - NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAudioChannelService) NS_INTERFACE_MAP_ENTRY(nsISupports) still is needed probably as: NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver)
Attachment #8867039 -
Flags: review?(amarchesini) → review+
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8867040 [details] Bug 1299390 - part 3: Remove BrowserElement methods required by BrowserElementAudioChannel only. https://reviewboard.mozilla.org/r/138640/#review143080 Thanks for these patches.
Attachment #8867040 -
Flags: review?(amarchesini) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8868064 -
Flags: review?(alwu)
Reporter | ||
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8868064 [details] Bug 1299390 - part 5: Remove useless AudioChannelService code. https://reviewboard.mozilla.org/r/139664/#review143412 r+ with handling open issues and ask baku about the code in ProcessPriorityManager.cpp. ::: dom/audiochannel/AudioChannelService.h:151 (Diff revision 4) > static AudioChannel GetAudioChannel(const nsAString& aString); > static AudioChannel GetDefaultAudioChannel(); > - static void GetAudioChannelString(AudioChannel aChannel, nsAString& aString); > - static void GetDefaultAudioChannelString(nsAString& aString); > > void Notify(uint64_t aWindowID); Also remove this function. ::: dom/audiochannel/AudioChannelService.h:172 (Diff revision 4) > - void MaybeSendStatusUpdate(); > - > - bool ContentOrNormalChannelIsActive(); > - > /* Send the default-volume-channel-changed notification */ > void SetDefaultVolumeControlChannelInternal(int32_t aChannel, Remove. ::: dom/audiochannel/AudioChannelService.cpp:39 (Diff revision 4) > static mozilla::LazyLogModule gAudioChannelLog("AudioChannel"); > > namespace { > > // If true, any new AudioChannelAgent will be muted when created. > bool sAudioChannelMutedByDefault = false; Sorry, also need to remove this variable and related functions and pref (dom.audiochannel.mutedByDefault). And change code [1] as following ``` AudioPlaybackConfig(1.0, false, nsISuspendedTypes::NONE_SUSPENDED) ``` [1] https://goo.gl/5EdJQe ::: dom/ipc/ProcessPriorityManager.cpp (Diff revision 4) > } > > - RefPtr<AudioChannelService> service = AudioChannelService::GetOrCreate(); > - if (service && service->ProcessContentOrNormalChannelIsActive(ChildID())) { > - return PROCESS_PRIORITY_BACKGROUND_PERCEIVABLE; > - } I'm not sure what's the purpose of these codes... Maybe ask baku for confirmation.
Attachment #8868064 -
Flags: review?(alwu) → review+
Assignee | ||
Comment 47•7 years ago
|
||
(In reply to Alastor Wu [:alwu][please needinfo? me] from comment #46) > Comment on attachment 8868064 [details] > Bug 1299390 - part 5: Remove useless AudioChannelService code > > https://reviewboard.mozilla.org/r/139664/#review143412 > > ::: dom/ipc/ProcessPriorityManager.cpp > (Diff revision 4) > > } > > > > - RefPtr<AudioChannelService> service = AudioChannelService::GetOrCreate(); > > - if (service && service->ProcessContentOrNormalChannelIsActive(ChildID())) { > > - return PROCESS_PRIORITY_BACKGROUND_PERCEIVABLE; > > - } > > I'm not sure what's the purpose of these codes... > Maybe ask baku for confirmation. baku, is it OK to remove above audiochannel related code from ProcessPriorityManager? Since |AudioChannelService::ProcessContentOrNormalChannelIsActive| is removed as well.
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 53•7 years ago
|
||
ProcessPriorityManager? Since
> |AudioChannelService::ProcessContentOrNormalChannelIsActive| is removed as
> well.
Well, normalChannel is still in used. We change the priority of the processes when audioChannel is active.
I want to have a feedback from someone from the e10s world: Gabor, do we need to change the priority for some reason? Here we do so if some audio component is active.
Flags: needinfo?(amarchesini) → needinfo?(gkrizsanits)
Comment 54•7 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #53) > ProcessPriorityManager? Since > > |AudioChannelService::ProcessContentOrNormalChannelIsActive| is removed as > > well. > > Well, normalChannel is still in used. We change the priority of the > processes when audioChannel is active. > I want to have a feedback from someone from the e10s world: Gabor, do we > need to change the priority for some reason? Here we do so if some audio > component is active. I think process priority is gonk only and no op on all other platforms. Actually I would like to remove this code altogether. Bill, is it OK to remove all the process priority fiddling bits or do we need it for anything? If it's OK I'll file a bug and do it some time in the near future.
Flags: needinfo?(gkrizsanits) → needinfo?(wmccloskey)
I can't find an implementation of anything that actually changes the process priority. So I think that code can be removed.
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 56•7 years ago
|
||
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22cc62127049
Keywords: checkin-needed
Comment 57•7 years ago
|
||
Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8e65b8fad40f part 1: Remove BrowserElementAudioChannel. r=alwu,baku https://hg.mozilla.org/integration/autoland/rev/c0a17591d1d1 part 2: Remove AudioChannelService methods required by BrowserElementAudioChannel only. r=alwu,baku https://hg.mozilla.org/integration/autoland/rev/06f2e3ad6e85 part 3: Remove BrowserElement methods required by BrowserElementAudioChannel only. r=alwu,baku https://hg.mozilla.org/integration/autoland/rev/0f98331fa000 part 4: Clean up BrowserElementAudioChannel related test cases. r=alwu https://hg.mozilla.org/integration/autoland/rev/6cddd88fc085 part 5: Remove useless AudioChannelService code. r=alwu
Keywords: checkin-needed
Comment 58•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e65b8fad40f https://hg.mozilla.org/mozilla-central/rev/c0a17591d1d1 https://hg.mozilla.org/mozilla-central/rev/06f2e3ad6e85 https://hg.mozilla.org/mozilla-central/rev/0f98331fa000 https://hg.mozilla.org/mozilla-central/rev/6cddd88fc085
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•