clean up audio channel related code

RESOLVED FIXED in Firefox 55

Status

()

P5
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: alwu, Assigned: ben.tian, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Other Branch
mozilla55
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
We should remove b2g related part in all audio channel code.
Priority: -- → P5
(Reporter)

Updated

2 years ago
Blocks: 1306391
See Also: → bug 1307238
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)

Updated

a year ago
Depends on: 1358061
(Reporter)

Comment 2

a year ago
Sure, I can remove them step by step and remove the related codes in HTMLMediaElement first.
Filed bug1358061.
(Reporter)

Comment 3

a year ago
Will remove mozaudiochannel related codes in bug1358061.
Flags: needinfo?(alwu)
(Reporter)

Updated

a year ago
Duplicate of this bug: 1338133
(Reporter)

Comment 5

a year 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

a year ago
Mentor: alwu
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
(Assignee)

Comment 7

a year ago
Take this bug.
Assignee: nobody → btian
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

a year 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

a year 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

a year 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

a year 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

a year ago
Attachment #8867038 - Flags: review?(amarchesini)
Attachment #8867039 - Flags: review?(amarchesini)
Attachment #8867040 - Flags: review?(amarchesini)
(Reporter)

Comment 23

a year 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

a year 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

a year 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

a year 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 on attachment 8867038 [details]
Bug 1299390 - part 1: Remove BrowserElementAudioChannel.

https://reviewboard.mozilla.org/r/138636/#review143076
Attachment #8867038 - Flags: review?(amarchesini) → 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 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

a year ago
Attachment #8868064 - Flags: review?(alwu)
(Reporter)

Comment 46

a year 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

a year 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)
 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)
(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

a year ago
Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22cc62127049
Keywords: checkin-needed

Comment 57

a year 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
No longer blocks: 1369194

Updated

11 months ago
See Also: → bug 1403060
You need to log in before you can comment on or make changes to this bug.