Closed Bug 1299390 Opened 8 years ago Closed 7 years ago

clean up audio channel related code

Categories

(Core :: Audio/Video: Playback, defect, P5)

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

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

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

Bug 1299390 - part 1: Remove BrowserElementAudioChannel.
59 bytes, text/x-review-board-request
alwu
: review+
baku
: review+
Details
Bug 1299390 - part 2: Remove AudioChannelService methods required by BrowserElementAudioChannel only.
59 bytes, text/x-review-board-request
alwu
: review+
baku
: review+
Details
Bug 1299390 - part 3: Remove BrowserElement methods required by BrowserElementAudioChannel only.
59 bytes, text/x-review-board-request
alwu
: review+
baku
: review+
Details
Bug 1299390 - part 4: Clean up BrowserElementAudioChannel related test cases.
59 bytes, text/x-review-board-request
alwu
: review+
Details
Bug 1299390 - part 5: Remove useless AudioChannelService code.
59 bytes, text/x-review-board-request
alwu
: review+
Details 
We should remove b2g related part in all audio channel code.
Blocks: nukeb2g
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)
Depends on: 1358061
Sure, I can remove them step by step and remove the related codes in HTMLMediaElement first.
Filed bug1358061.
Will remove mozaudiochannel related codes in bug1358061.
Flags: needinfo?(alwu)
No longer blocks: 1357645
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
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
Take this bug.
Assignee: nobody → btian
Attachment #8867038 - Flags: review?(alwu)
Attachment #8867039 - Flags: review?(alwu)
Attachment #8867040 - Flags: review?(alwu)
Attachment #8867041 - Flags: review?(alwu)
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+
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+
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+
Attachment #8867038 - Flags: review?(amarchesini)
Attachment #8867039 - Flags: review?(amarchesini)
Attachment #8867040 - Flags: review?(amarchesini)
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+
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
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)
(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 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+
Attachment #8868064 - Flags: review?(alwu)
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+
(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)
 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)
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
See Also: → 1403060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: