AudioChannelService refactoring

RESOLVED WONTFIX

Status

()

Core
DOM
RESOLVED WONTFIX
5 years ago
3 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 13 obsolete attachments)

61.01 KB, patch
Details | Diff | Splinter Review
17.10 KB, patch
mchen
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 754741 [details] [diff] [review]
patch
Attachment #754741 - Flags: review?(mchen)
(Assignee)

Updated

5 years ago
Blocks: 853101

Comment 2

5 years ago
Hi Andrea,

It seems that AudioChannelAgent in FMRadio.cpp is not updated for new version.
(Assignee)

Comment 3

5 years ago
b2g18?

Comment 4

5 years ago
(In reply to Andrea Marchesini (:baku) from comment #3)
> b2g18?

Yes, I applied your patch into b2g18. Or I need to apply it into v1.0.1?

More info: FMRadio.cpp used AudioChannelAgent and call setVisibilityState() but this API is replaced by setVisible(). Also the arguments for initWithWeakCallback() is different too, there is no window can be used to put in new version of initWithWeakCallback().
(Assignee)

Comment 5

5 years ago
I updated the patch for 862899. This is needed too.

Comment 6

5 years ago
Comment on attachment 754741 [details] [diff] [review]
patch

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

Hi Andrea,

I didn't complete the review but there are some defect found.
So post the first round comments to you. Please help to correct them. Thanks.

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +12,5 @@
> +
> +void
> +AudioChannelAgentParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mChildDestroyed = true;

I think we need to take care the case of child process crashed then un-register this agent from AudioChannelService.

@@ +41,5 @@
> +void
> +AudioChannelAgentParent::NotifyAudioChannelStateChanged()
> +{
> +  AudioChannelService* service = AudioChannelService::GetAudioChannelService();
> +  bool muted = service->CanPlay(this);

1. CanPlay() returns true if this agent is allowed to play.
2. But muted variable means if true then this agent should not be allowed to play.
3. They are opposite meaning here. So after calling SendNotify(!muted) agentClient will become "not to play" even agentParent is allowed.

::: dom/audiochannel/AudioChannelService.cpp
@@ +153,5 @@
> +      // If nothing is visible, the list has to been frozen.
> +      // Or if there is still any one with other ChildID in foreground then
> +      // it should be removed from list and left other ChildIDs in the foreground
> +      // to keep playing. Finally only last one childID which go to background
> +      // will be in list.

In this comment we used childID to identify an app but in the code it seems we used groupID to identify a window or a tab. So we may need to modify the comment.

@@ +161,5 @@
> +        if (mAgents[i]->mAgent->Type() != AUDIO_CHANNEL_CONTENT) {
> +           continue;
> +        }
> +
> +        ++countContentAgents;

I think this variable is used to record any "visible" apps in the foreground and to set frozen to true if it is 0.
Based on this assumption, do we need to check whether this agent is hidden or not?

@@ +172,5 @@
> +
> +      if (!countContentAgents) {
> +        mActiveContentGroupIdsFrozen = true;
> +      } else if (!countVisibleContentAgentsInThisGroup) {
> +        mActiveContentGroupIds.RemoveElement(aAgent->GroupId());

maybe in comment we need to add something like this
  "The GroupID will be removed from list only when last agent moved from visible to hidden in case of multiple agents in the same GroupID"

@@ +213,5 @@
>  
> +  // This channel is supposed to play.
> +  if (mCurrentHighestChannel <= aAgent->Type()) {
> +    return true;
> +  }

In the case of current highest channel is content and aAgent is content too, then here will always return true.
But actually we need additional check in the next code section.

@@ +252,5 @@
>  
> +  for (int i = 0, len = mAgents.Length(); i < len; ++i) {
> +    bool canPlay = CanPlayInternal(mAgents[i]->mAgent, mAgents[i]);
> +    if (!canPlay) {
> +      continue;

To image the use case:
  1. Music app is on playing then alarm is fired.
  2. Alarm is end then calling UnregisterAudioChannelAgent() -> SendAudioChannelChangedNotification() to here.
  3. Agent with content channel can't be allowed to play because the highest channel is still alarm.
  4. Finally highest channel is remained to initial value - AUDIO_CHANNEL_LAST. (Issue here.)
Attachment #754741 - Flags: review?(mchen)

Comment 7

5 years ago
Also, there is a crash issue on IPC part so you would need to fix it before landing.
Please see the back trace as below. It is happened when
  1. Play a audio from music app.
  2. Crash will be happened when I perform pause or seek action.

(gdb) bt
#0  0x4147e38e in mozilla::dom::PContentChild::OnMessageReceived (this=0x42e1b618, 
    __msg=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/objdir-gonk-noopt/ipc/ipdl/PContentChild.cpp:2208
#1  0x413628ca in mozilla::ipc::AsyncChannel::OnDispatchMessage (this=0x42e1b624, 
    msg=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/AsyncChannel.cpp:471
#2  0x4136cb10 in mozilla::ipc::RPCChannel::OnMaybeDequeueOne (this=0x42e1b624)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/RPCChannel.cpp:402
#3  0x41334ade in DispatchToMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)()> (obj=0x42e1b624, 
    method=0x4136c959 <mozilla::ipc::RPCChannel::OnMaybeDequeueOne()>, arg=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/tuple.h:383
#4  0x413348b8 in RunnableMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)(), Tuple0>::Run (this=0x42e01ba0)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/task.h:307
#5  0x4136b770 in mozilla::ipc::RPCChannel::RefCountedTask::Run (this=0x42e0a0b8)
    at ../../dist/include/mozilla/ipc/RPCChannel.h:425
#6  0x4136b854 in mozilla::ipc::RPCChannel::DequeueTask::Run (this=0x45ffd250)
---Type <return> to continue, or q <return> to quit---
    at ../../dist/include/mozilla/ipc/RPCChannel.h:448
#7  0x41631ade in MessageLoop::RunTask (this=0xbeffb8bc, task=0x45ffd250)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:337
#8  0x41631b34 in MessageLoop::DeferOrRunPendingTask (this=0xbeffb8bc, 
    pending_task=...)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:345
#9  0x41631e62 in MessageLoop::DoWork (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:445
#10 0x4136a330 in mozilla::ipc::DoWorkRunnable::Run (this=0x42e033e0)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:42
#11 0x415eaddc in nsThread::ProcessNextEvent (this=0x42e068e0, mayWait=false, 
    result=0xbeffaf47)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/xpcom/threads/nsThread.cpp:620
#12 0x415a4a7a in NS_ProcessNextEvent_P (thread=0x42e068e0, mayWait=false)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/objdir-gonk-noopt/xpcom/build/nsThreadUtils.cpp:237
#13 0x4136a458 in mozilla::ipc::MessagePump::Run (this=0x42e02340, 
---Type <return> to continue, or q <return> to quit---
    aDelegate=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:82
#14 0x4136a7c4 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x42e02340, 
    aDelegate=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:231
#15 0x41631770 in MessageLoop::RunInternal (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:219
#16 0x41631742 in MessageLoop::RunHandler (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:212
#17 0x416316ea in MessageLoop::Run (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:186
#18 0x41260f0e in nsBaseAppShell::Run (this=0x448430a0)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/widget/xpwidgets/nsBaseAppShell.cpp:163
#19 0x40465416 in XRE_RunAppShell ()
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/toolkit/xre/nsEmbedFunctions.cpp:646
#20 0x4136a772 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x42e02340, 
---Type <return> to continue, or q <return> to quit---
    aDelegate=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/glue/MessagePump.cpp:198
#21 0x41631770 in MessageLoop::RunInternal (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:219
#22 0x41631742 in MessageLoop::RunHandler (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:212
#23 0x416316ea in MessageLoop::Run (this=0xbeffb8bc)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/chromium/src/base/message_loop.cc:186
#24 0x4046504c in XRE_InitChildProcess (aArgc=3, aArgv=0xbeffba44, 
    aProcess=GeckoProcessType_Content)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/toolkit/xre/nsEmbedFunctions.cpp:485
#25 0x00008620 in main (argc=5, argv=0xbeffba44)
    at /home/marco/workspaces/B2G/mozilla-central/b2g18/ipc/app/MozillaRuntimeMain.cpp:60
(Assignee)

Comment 8

5 years ago
> ::: dom/audiochannel/AudioChannelAgentParent.cpp
> @@ +12,5 @@
> > +
> > +void
> > +AudioChannelAgentParent::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  mChildDestroyed = true;
> 
> I think we need to take care the case of child process crashed then
> un-register this agent from AudioChannelService.

When the child destroys the parent, the parent unregister it self in the destructor.
This part is tested. But I can check it better :)

> 1. CanPlay() returns true if this agent is allowed to play.
> 2. But muted variable means if true then this agent should not be allowed to
> play.
> 3. They are opposite meaning here. So after calling SendNotify(!muted)
> agentClient will become "not to play" even agentParent is allowed.

Yeah... I have to rename muted in canPlay and change SendNotify(!muted) to SendNotify(canPlay);

> To image the use case:
>   1. Music app is on playing then alarm is fired.
>   2. Alarm is end then calling UnregisterAudioChannelAgent() ->
> SendAudioChannelChangedNotification() to here.
>   3. Agent with content channel can't be allowed to play because the highest
> channel is still alarm.
>   4. Finally highest channel is remained to initial value -
> AUDIO_CHANNEL_LAST. (Issue here.)

good point.

I'm going to submit a new patch. With the crash fixed!
(Assignee)

Comment 9

5 years ago
Created attachment 757880 [details] [diff] [review]
patch

It took a while. Can you review this code? The crash is fixed and all your comments should be applied.
Attachment #754741 - Attachment is obsolete: true
Attachment #757880 - Flags: review?(mchen)
(Assignee)

Updated

5 years ago
Attachment #757880 - Flags: review?(bent.mozilla)

Updated

5 years ago
Depends on: 862899

Comment 10

5 years ago
Some issues are listed here after doing a simple test.

 Issue 1:
    a. To play an audio from music app.
    b. To play an video from video app.
    c. video and audio are playing concurrently. (NG)
    d. Stop the video then audio is stop too. (wired.)

 Issue 2:
    a. To play an audio from music app.
    b. Set an alarm and wait for it to fire.
    c. Alarm is fired and audio from music app is stopped.
    d. After alarm is end, the audio from music app doesn't be resumed automatically. (NG)

Could you please do the simple tests and make sure the functions are all correct then ask to the review? Thanks.

Updated

5 years ago
Attachment #757880 - Flags: review?(mchen)
Comment on attachment 757880 [details] [diff] [review]
patch

Andrea, are you ready for me to review this? Marco had asked for some additional testing before continuing, is that done?
(Assignee)

Comment 12

5 years ago
Yes, absolutely. Marco and I are checking each feature, but what I would like to see from you is a review of the IPDL part. In particular the last patch fixes the problem where:

1. clientAgent->StopPlaying(); clientAgent = nullptr;

2. then the parent receives the StopPlaying call and destroys itself. The client has to be kept alive until the communication from the parent is received.
Comment on attachment 757880 [details] [diff] [review]
patch

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

I don't think this is quite ready, we should make some simplifications to the IPDL stuff.

::: dom/audiochannel/AudioChannelAgentChild.h
@@ +14,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class AudioChannelAgentChild : public nsIAudioChannelAgent,
> +                               public AudioChannelAgent,

You should make AudioChannelAgent your first base. Also mark as MOZ_FINAL.

@@ +15,5 @@
> +namespace dom {
> +
> +class AudioChannelAgentChild : public nsIAudioChannelAgent,
> +                               public AudioChannelAgent,
> +                               public PAudioChannelAgentChild

It's really weird that this class is used in both the child and the parent but is called "Child". Can we instead just merge AudioChannelAgentChild into AudioChannelAgent? Then have AudioChannelAgentParent inherit that with protected inheritance or something? That would mean we only have two classes, AudioChannelAgent and AudioChannelAgentParent.

The other alternative is to have AudioChannelAgent be the only class that implements nsISupports and have AudioChannelAgentParent and AudioChannelAgentChild pointers, only one of which is ever used depending on which process you're in.

This current setup is pretty confusing though.

@@ +68,5 @@
> +                        nsIAudioChannelAgentCallback* aCallback,
> +                        bool aUseWeakRef);
> +
> +  virtual void
> +  ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;

Nit: your style is inconsistent here

@@ +74,5 @@
> +  virtual bool StartPlayingInternal() MOZ_OVERRIDE;
> +  virtual void StopPlayingInternal() MOZ_OVERRIDE;
> +  virtual void SetVisibleInternal() MOZ_OVERRIDE;
> +
> +  bool RecvNotify(const bool& aCanPlay) MOZ_OVERRIDE;

Nit: virtual

@@ +80,5 @@
> +  nsWeakPtr mWeakWindow;
> +  nsCOMPtr<nsIAudioChannelAgentCallback> mCallback;
> +  nsWeakPtr mWeakCallback;
> +
> +  nsRefPtr<AudioChannelAgentChild> mKungFu;

This I really don't understand. I think it would be much better to let IPDL "own" this object rather than trying to manage a self-reference in certain circumstances.

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +12,5 @@
> +
> +void
> +AudioChannelAgentParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mChildDestroyed = true;

I think rather than tracking this you should just immediately unregister yourself from the service. That way you should die for real and you won't need to check any state below.

@@ +25,5 @@
> +  return service->CanPlay(this);
> +}
> +
> +void
> +AudioChannelAgentParent::StopPlayingInternal(void)

Nit: no need for void

@@ +52,5 @@
> +AudioChannelAgentParent::RecvStopPlaying()
> +{
> +  nsRefPtr<AudioChannelAgentParent> kungFuDeathGrip = this;
> +
> +  if (!mChildDestroyed) {

It shouldn't be possible to get here with mChildDestroyed being true (unless the child sends multiple messages at once, which would be bizarre).

@@ +54,5 @@
> +  nsRefPtr<AudioChannelAgentParent> kungFuDeathGrip = this;
> +
> +  if (!mChildDestroyed) {
> +    Send__delete__(this);
> +    mChildDestroyed = true;

ActorDestroy should have done this already.

::: dom/audiochannel/AudioChannelService.h
@@ +73,5 @@
> +    : mAgent(aAgent)
> +    , mElementHidden(aElementHidden)
> +    {}
> +
> +    virtual ~AudioChannelAgentData()

refcounted classes should have non-public destructors.

::: dom/audiochannel/PAudioChannelAgent.ipdl
@@ +18,5 @@
> +
> +parent:
> +  // When the visibility changes, the AudioChannelService can decide to mute
> +  // this AudioChannelAgent.
> +  sync SetVisible(bool aVisible)

Nit: s/aVisible/visible/

@@ +21,5 @@
> +  // this AudioChannelAgent.
> +  sync SetVisible(bool aVisible)
> +    returns (bool canPlay);
> +
> +  // StopPlaying unregisters the AudioChannelAgentParent and deletes it.

This comment is sorta misleading. It's up to the parent to send the __delete__ message, so you could just say that calling this method triggers a sequence of events that eventually causes the parent to delete the actor?

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +70,3 @@
>     */
> +  void init(in nsIDOMWindow window, in long channelType,
> +            in nsIAudioChannelAgentCallback callback);

Don't forget to bump your iids.

::: dom/ipc/ContentParent.cpp
@@ +88,5 @@
>  #include "StructuredCloneUtils.h"
>  #include "TabParent.h"
>  #include "URIUtils.h"
>  #include "nsGeolocation.h"
> +#include "AudioChannelService.h"

Nit: Alphabetize! (whoever added nsGeolocation gets two demerits).

::: dom/ipc/TabChild.cpp
@@ +1630,5 @@
> +bool
> +TabChild::DeallocPAudioChannelAgent(PAudioChannelAgentChild* actor)
> +{
> +    // Nothing to do here. The child doesn't have to be released when the parent
> +    // is deleted.

Nit: This comment doesn't really make sense, just say "Nothing to do here"

::: dom/ipc/TabParent.cpp
@@ +417,5 @@
> +                                             const AudioChannelType& aType,
> +                                             const bool& aVisible,
> +                                             bool* aCanPlay)
> +{
> +    nsRefPtr<AudioChannelAgentParent> agent =

Nit: No need for the refptr here, you just gave it a ref in Alloc

::: dom/ipc/TabParent.h
@@ +308,5 @@
>      bool mShown;
>      bool mUpdatedDimensions;
>  
>  private:
> +    static uint64_t sTabParentId;

Let's just have this be a global in TabParent.cpp

@@ +337,5 @@
>      // anymore.
>      bool mIsDestroyed;
> +
> +    // A unique ID for this TabParent object
> +    uint64_t mId;

Nit: I'd move this up somewhere before the final bool params.
Attachment #757880 - Flags: review?(bent.mozilla)

Updated

5 years ago
Blocks: 823273

Updated

5 years ago
Blocks: 855655

Updated

5 years ago
No longer blocks: 823273

Updated

5 years ago
No longer blocks: 855655
(Assignee)

Comment 14

5 years ago
Created attachment 826784 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

On the plane, flying back to europe I had time to play a bit with this patch.
This patch introduces the window in any Init() method of nsIAudioChannelAgent.

This is the first patch of 3.
Attachment #757880 - Attachment is obsolete: true
Attachment #826784 - Flags: review?(mchen)
(Assignee)

Comment 15

5 years ago
Created attachment 826788 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

I decided to work on this issue just to see in practice what I and bent discussed during the IPDL "work week".
This patch does this:

1. PAudioChannelAgent.ipdl: a simple protocol

2. AudioChannelAgent is also a PAudioChannelAgentParent.

3. When AudioChannelAgent runs on a child process, it creates a AudioChannelAgentChild and sends all the request through the PAudioChannelAgent protocol.

I wrote a test to check if this works.
Attachment #826788 - Flags: review?(mchen)
Attachment #826788 - Flags: review?(bent.mozilla)
(Assignee)

Comment 16

5 years ago
Comment on attachment 826784 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +72,3 @@
>  }
>  
> +/* void initWithVideo(in nsIDOMWindow window, in long channelType,

this is moved before line 66.
(Assignee)

Comment 17

5 years ago
The last patch should clean a bit the AudioChannelService code but I don't have time to work on it.
(Assignee)

Comment 18

5 years ago
Created attachment 827055 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

Small issues fixed.
Attachment #826788 - Attachment is obsolete: true
Attachment #826788 - Flags: review?(mchen)
Attachment #826788 - Flags: review?(bent.mozilla)
Attachment #827055 - Flags: review?(mchen)
Attachment #827055 - Flags: review?(bent.mozilla)

Comment 19

5 years ago
Comment on attachment 827055 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

Hi Andrea,

I tested your patch on master branch but the behavior of audio competing became not normal.

  1. lunch music player and play it.
  2. lunch video or fm and play it.
  3. both music and video/fm are playing
  4. stop video/fm then music is stopped too.

May I know your testing result or I missed to apply something?

::: content/html/content/src/HTMLAudioElement.cpp
@@ +272,5 @@
>      HTMLMediaElement::UpdateAudioChannelPlayingState();
>      return;
>    }
> +
> +  if (UseAudioChannelService() &&

We can move "the check of UseAudioChannelService()" before here then we can save one level inside if-else.

if (!UseAudioChannelService()) {
  return;
}
Attachment #827055 - Flags: review?(mchen)
(Assignee)

Comment 20

5 years ago
> I didn't complete the review but there are some defect found.

Yes. It's normal! These are the first 2 patches of 3. With the 3rd one the behaviour will be the same.
I hope to have time to work on the last patch.

Thanks for the comments, I'll check them soon.
(Assignee)

Comment 21

5 years ago
Created attachment 829701 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent
Attachment #826784 - Attachment is obsolete: true
Attachment #826784 - Flags: review?(mchen)
Attachment #829701 - Flags: review?(mchen)
(Assignee)

Comment 22

5 years ago
Created attachment 829702 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent
Attachment #827055 - Attachment is obsolete: true
Attachment #827055 - Flags: review?(bent.mozilla)
Attachment #829702 - Flags: review?(bent.mozilla)
(Assignee)

Comment 23

5 years ago
Created attachment 829703 [details] [diff] [review]
patch 3 - AudioChannelService refactoring
Attachment #829703 - Flags: review?(mchen)
(Assignee)

Comment 24

5 years ago
These 3 patches are green on try and they work on a real device. Of course some regression can be found.
Marco, from you I would like to know if you like this code more than the previous one. I think it's easier to understand and to be changed. Then reviews are welcome!

Something nice of this code is the groupID object. With this we group per window/tab/app. I think this code can be reused in the SpeakerManager too and probably somewhere else.
(Assignee)

Comment 25

5 years ago
A follow up, when this code is landed, is to add a permission check in the AudioChannelAgentParent so that we can see if the app has the right permission to use a particular AudioChannelType.

Comment 26

5 years ago
(In reply to Andrea Marchesini (:baku) from comment #25)
> A follow up, when this code is landed, is to add a permission check in the
> AudioChannelAgentParent so that we can see if the app has the right
> permission to use a particular AudioChannelType.

About this improvement, do we need to check permission in AudioChannelAgentParent?
The only way now for assigning audio channel explicitly is by argument/property from video/audio element.
And we already did permission check there.
(Assignee)

Comment 27

5 years ago
> About this improvement, do we need to check permission in
> AudioChannelAgentParent?
> The only way now for assigning audio channel explicitly is by
> argument/property from video/audio element.
> And we already did permission check there.

What about if the child process has been hacked? A MediaElement can skip that check and play without additional permission checks. One more is simple and nice to have. Of course... if all the rest of these patches are good enough :)

Comment 28

5 years ago
(In reply to Andrea Marchesini (:baku) from comment #27)
> What about if the child process has been hacked? 
This is not a normal case and we should fixed the weakness been attacked not build a second wall there.

And I am going to review and do some simple test.

Comment 29

5 years ago
Comment on attachment 829701 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

It looks good except the one question about window in DOMCameraControl.

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +5,5 @@
>  #include "AudioChannelAgent.h"
>  #include "AudioChannelCommon.h"
>  #include "AudioChannelService.h"
> +#include "nsIDOMWindow.h"
> +#include "nsXULAppAPI.h"

Do we need this header?

@@ +71,3 @@
>                                   bool aUseWeakRef)
>  {
> +  return InitInternal(aWindow, aChannelType, aCallback, aUseWeakRef, true);

Could you help to add "/* useWeakRef = */ true"?
Thanks.

::: dom/camera/DOMCameraControl.cpp
@@ +374,5 @@
>    if (!mAudioChannelAgent) {
>      mAudioChannelAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");
>      if (mAudioChannelAgent) {
>        // Camera app will stop recording when it falls to the background, so no callback is necessary.
> +      mAudioChannelAgent->Init(nullptr, AUDIO_CHANNEL_CONTENT, nullptr);

This object is running on child process so it needs a window (mWindow).
Or it will hit the assertion in AudioChannelAgent.cpp
Attachment #829701 - Flags: review?(mchen)
Comment on attachment 829702 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

This mostly looks fine, but r- for the protocol error in PAudioChannelAgent.ipdl

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +75,5 @@
>                                   nsIAudioChannelAgentCallback *aCallback,
>                                   bool aUseWeakRef)
>  {
> +  return InitInternal(aWindow, aChannelType, aCallback, aUseWeakRef,
> +                      /* useWeakRef = */ true);

Wait, there are two args that specify whether to use weak refs?

@@ +126,5 @@
>  }
>  
>  /* boolean startPlaying (); */
> +NS_IMETHODIMP
> +AudioChannelAgent::StartPlaying(int32_t *aCanPlay)

Nit: * are sometimes on the left, sometimes on the right...

@@ +148,5 @@
> +
> +    nsCOMPtr<nsPIDOMWindow> window = static_cast<nsPIDOMWindow*>(mWindow.get());
> +    MOZ_ASSERT(window);
> +
> +    TabChild* tabChild =  dom::TabChild::GetFrom(window->GetDocShell());

Nit: No need for the dom:: here.

@@ +158,5 @@
> +             static_cast<AudioChannelType>(mAudioChannelType),
> +             mVisible, aCanPlay);
> +    MOZ_ASSERT(mIPC);
> +
> +    static_cast<AudioChannelAgentChild*>(mIPC)->SetParent(this);

This would be better with the other constructor:

  AudioChannelAgentChild* actor = new AudioChannelAgentChild(this);
  tabChild->SendPAudioChannelAgentConstructor(actor, ...);

It would mean adding a non-default constructor to AudioChannelAgentChild but that's ok.

@@ +182,5 @@
>    mIsRegToService = false;
> +
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    if (!mIPC) {
> +      return NS_OK;

This can be cleaned up as:

  if (XRE_GetProcessType() != GeckoProcessType_Default && mIPC) {

There's another spot below too.

@@ +194,5 @@
>  }
>  
>  /* void setVisibilityState (in boolean visible); */
> +NS_IMETHODIMP
> +AudioChannelAgent::SetVisibilityState(bool visible)

Nit: aVisible

::: dom/audiochannel/AudioChannelAgent.h
@@ +8,5 @@
>  #define mozilla_dom_audio_channel_agent_h__
>  
>  #include "nsIAudioChannelAgent.h"
>  #include "nsCycleCollectionParticipant.h"
> +#include "mozilla/dom/PAudioChannelAgentChild.h"

No need to include this here, just forward-declare.

@@ +28,1 @@
>  /* Header file */

Nit: remove

@@ +50,5 @@
>                          nsIAudioChannelAgentCallback* aCallback,
>                          bool aUseWeakRef, bool aWithVideo=false);
>  
>    nsCOMPtr<nsIDOMWindow> mWindow;
> +  PAudioChannelAgentChild* mIPC;

Nit: mActor is much more readable

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +7,5 @@
> +#include "mozilla/dom/TabParent.h"
> +
> +using namespace mozilla::dom;
> +
> +NS_IMPL_ADDREF_INHERITED(AudioChannelAgentParent, AudioChannelAgent)

All of this macro goop can be replaces with:

  NS_IMPL_ISUPPORTS_INHERITED0(AudioChannelAgentParent, AudioChannelAgent)

::: dom/audiochannel/AudioChannelAgentParent.h
@@ +14,5 @@
> +namespace dom {
> +
> +class TabParent;
> +
> +/* Header file */

Nit: remove

@@ +21,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AudioChannelAgentParent,
> +                                           AudioChannelAgent)

I don't think you need to add anything to the CC map, no need for this macro.

@@ +23,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AudioChannelAgentParent,
> +                                           AudioChannelAgent)
> +
> +  AudioChannelAgentParent()

This and the destructor should be moved to the cpp file, otherwise the compiler can't figure out what to do with the forward-declared TabParent.

@@ +37,5 @@
> +  RecvSetVisibleState(const bool& aVisible,
> +                      int32_t* aCanPlay) MOZ_OVERRIDE;
> +
> +  virtual bool
> +  Recv__delete__();

Nit: MOZ_OVERRIDE

::: dom/audiochannel/AudioChannelService.cpp
@@ +151,4 @@
>  
> +  // In order to avoid race conditions, it's safer to notify any existing
> +  // agent any time a new one is registered.
> +  SendAudioChannelChangedNotification(aChildID);

Wait, you removed this message from PContent... What does this do now?

::: dom/audiochannel/PAudioChannelAgent.ipdl
@@ +26,5 @@
> +child:
> +  // Notify is called when the AudioChannelService wants to mute this
> +  // AudioChannelAgent. Read nsIAudioChannelAgent.idl for more info about
> +  // canPlay values.
> +  Notify(int32_t aCanPlay);

There's nothing here to prevent sending a Notify() from parent->child at the same time as sending the __delete__ from child->parent. You have to prevent that or the parent could crash.

::: dom/ipc/TabParent.cpp
@@ +1336,5 @@
> +                                             const bool& aVisible,
> +                                             int32_t* aCanPlay)
> +{
> +  nsRefPtr<AudioChannelAgentParent> agent =
> +    static_cast<AudioChannelAgentParent*>(aActor);

Nit: You know there's a ref here, no need to add another

@@ +1339,5 @@
> +  nsRefPtr<AudioChannelAgentParent> agent =
> +    static_cast<AudioChannelAgentParent*>(aActor);
> +
> +  nsresult rv = agent->InitFromTabParent(this, aType);
> +  NS_ENSURE_SUCCESS(rv, nullptr);

Eek! This method returns bool :)

@@ +1345,5 @@
> +  rv = agent->SetVisibilityState(aVisible);
> +  NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +  agent->StartPlaying(aCanPlay);
> +  NS_ENSURE_SUCCESS(rv, nullptr);

Shouldn't you be setting rv from StartPlaying()?

::: ipc/glue/IPCMessageUtils.h
@@ +19,5 @@
>  #include <stdint.h>
>  
>  #include "nsID.h"
>  #include "nsMemory.h"
> +#include "nsStringGlue.h"

Did you need to do this? It's rather unlikely that IPCMessageUtils can work outside of libxul
Attachment #829702 - Flags: review?(bent.mozilla) → review-
(Assignee)

Comment 31

5 years ago
Created attachment 8335337 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

I cannot remove nsXULAppAPI.h because it's needed for XRE_GetProcessType()
Attachment #829701 - Attachment is obsolete: true
Attachment #8335337 - Flags: review?(mchen)
(Assignee)

Comment 32

5 years ago
> @@ +23,5 @@
> > +  NS_DECL_ISUPPORTS_INHERITED
> > +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AudioChannelAgentParent,
> > +                                           AudioChannelAgent)
> > +
> > +  AudioChannelAgentParent()
> 
> This and the destructor should be moved to the cpp file, otherwise the
> compiler can't figure out what to do with the forward-declared TabParent.

it actually works. why do you say this?

> Wait, you removed this message from PContent... What does this do now?

Now we use the RecvNotify() in the AudioChannelAgentChild.

> There's nothing here to prevent sending a Notify() from parent->child at the
> same time as sending the __delete__ from child->parent. You have to prevent
> that or the parent could crash.

Still working on this.

> Did you need to do this? It's rather unlikely that IPCMessageUtils can work
> outside of libxul

This is needed for a CPP Unit Test that runs outside of libxul.

Comment 33

5 years ago
Comment on attachment 829702 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +140,5 @@
>    }
>  
>    service->RegisterAudioChannelAgent(this,
>      static_cast<AudioChannelType>(mAudioChannelType), mWithVideo);
> +  mIsRegToService = true;

Line 167 also assigned mIsRegToService to true.
Maybe we need to move this line before line 163.

::: dom/ipc/PBrowser.ipdl
@@ +319,5 @@
>  
> +    /**
> +     * Create a new AudioChannelAgent.
> +     */
> +    sync PAudioChannelAgent(AudioChannelType aType, bool aVisible)

There is a new attribute called AudioChannelAgent::mWithVideo.
This is used to notify AudioChannelService whether this audio is a from a video or not.
We need to send mWithVideo to parent side too.

Comment 34

5 years ago
Comment on attachment 829703 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

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

Hi,

This change looks good to me and actually improve the readable of the codes. Great Job.

But there are some bugs and questions in this patch, please help to address them.
Thanks.

::: content/html/content/src/HTMLAudioElement.cpp
@@ +283,5 @@
>      if (!mAudioChannelAgent) {
> +      nsCOMPtr<nsIDOMWindow> window = OwnerDoc()->GetWindow();
> +      if (!window) {
> +        return;
> +      }

NS_ENSURE_TRUE_VOID(window);

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3857,5 @@
>      if (!mAudioChannelAgent) {
> +      nsCOMPtr<nsIDOMWindow> window = OwnerDoc()->GetWindow();
> +      if (!window) {
> +        return;
> +      }

NS_ENSURE_TRUE_VOID(window);

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +185,5 @@
>    if (!service) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  service->RegisterAudioChannelAgent(this);

Since there is a representative (AudioChannelAgentParent) in parent process of each AudioChannelAgent in child process. In child process, do we still need to register each AudioChannelAgent into AudioChannelServiceClient.

It seems there is no necessary of AudioChannelServiceClient to know and own AudioChannelAgents.

::: dom/audiochannel/AudioChannelCommon.h
@@ +44,5 @@
> +    Unknown = 0,
> +    App,
> +    BrowserTab,
> +    Window
> +  } mType;

Does mType need to be public or just private?

@@ +96,5 @@
> +  }
> +
> +private:
> +  uint64_t mId;
> +  uint64_t mProcessId;

I only saw one line to do assignment into mProcessId and the value is CONTENT_PROCESS_ID_MAIN.
And it seems to be no meaning of comparing mProcessId in operator == or !=.

If it is useful then could we call it mChildId (I am confused between mProcessId and pid of a process)?

::: dom/audiochannel/AudioChannelService.cpp
@@ +259,5 @@
> +      // updated properly as hidden, mPlayableHiddenContentChildID is used to
> +      // check whether this background process is playable or not.
> +      CountAgents(AUDIO_CHANNEL_CONTENT, Visible,
> +                  aAgent->GroupId().ProcessId() == 0 &&
> +      mPlayableHiddenContentGroupId != aAgent->GroupId())) {

The right parenthesis of CountAgents is wrong.

@@ +382,5 @@
> +    if (aAgent->Type() >= AUDIO_CHANNEL_NOTIFICATION) {
> +      data->mHigherChannel = aAgent->Type();
> +    }
> +    else if (aAgent->Type() == AUDIO_CHANNEL_CONTENT) {
> +      if (gAudioChannelService->mPlayableHiddenContentGroupId.IsUnknown()) {

Consider the case as below.
  1. An App is starting to play a music and be put into background.
  2. Now the mPlayableHiddenContentGroupId will not be an unknown ID so mHigherChannel didn't be updated.
This will let HW volume keys not control content channel.

Maybe we can 
  1. change mPlayableHiddenContentGroupId to mPlayableContentGroupId.
  2. at line 141/228, assign agent's groupId into mPlayableContentGroupId.
  3. then we can avoid to count in muted agent too.

@@ +493,5 @@
>    }
>  
> +  return PL_DHASH_NEXT;
> +}
> +  

remove spaces.
Attachment #829703 - Flags: review?(mchen)

Comment 35

5 years ago
Comment on attachment 8335337 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

Hi Andrea,

It looks good to me for all the changes here, but how about the AudioChannelAgent in FMRadio.
Could you help to add that part? Or this patch can't be landed.
Attachment #8335337 - Flags: review?(mchen)
(Assignee)

Comment 36

5 years ago
> > +  uint64_t mProcessId;
> 
> I only saw one line to do assignment into mProcessId and the value is
> CONTENT_PROCESS_ID_MAIN.
> And it seems to be no meaning of comparing mProcessId in operator == or !=.

Actually mProcessId (now renamed mChildID) is setwhen  AudioChannelGroupId is created from a TabParent.
This is the reason why I need to compare it too.

> @@ +382,5 @@
> > +    if (aAgent->Type() >= AUDIO_CHANNEL_NOTIFICATION) {
> > +      data->mHigherChannel = aAgent->Type();
> > +    }
> > +    else if (aAgent->Type() == AUDIO_CHANNEL_CONTENT) {
> > +      if (gAudioChannelService->mPlayableHiddenContentGroupId.IsUnknown()) {
> 
> Consider the case as below.
>   1. An App is starting to play a music and be put into background.
>   2. Now the mPlayableHiddenContentGroupId will not be an unknown ID so
> mHigherChannel didn't be updated.
> This will let HW volume keys not control content channel.

I'll work on this.
(Assignee)

Comment 37

5 years ago
> Consider the case as below.
>   1. An App is starting to play a music and be put into background.
>   2. Now the mPlayableHiddenContentGroupId will not be an unknown ID so
> mHigherChannel didn't be updated.
> This will let HW volume keys not control content channel.

I don't follow. Actually I think that the code can be written in this way:

PLDHashOperator
AudioChannelService::HigherChannelsEnumerator(AudioChannelAgent* aAgent,                                  
                                              AudioChannelAgentData* aData,
                                              void *aPtr)                                                 
{   
  HigherChannelsData* data = static_cast<HigherChannelsData*>(aPtr);                                      
  
  // Visible agents                                                                                       
  if (data->mVisibleHigherChannel < aAgent->Type() && aAgent->VisibleState()) {                           
    data->mVisibleHigherChannel = aAgent->Type(); 
  }                                                                                                       
  
  // Any agents
  if (data->mHigherChannel < aAgent->Type()) {                                                            
    data->mHigherChannel = aAgent->Type();                                                                
  } 
      
  return PL_DHASH_NEXT;                                                                                   
}     
    

what do you think?
(Assignee)

Updated

5 years ago
Flags: needinfo?(mchen)

Comment 38

5 years ago
(In reply to Andrea Marchesini (:baku) from comment #37)
> what do you think?

Consider the use case as below:

  1. Play a audio from music app.
  2. Put music app into background.
  3. Play then stop video from video app in the foreground.

Then in this moment, the mHigherChannel will be "content" channel but actually that audio from music app is paused by AudioChannel already. Then in homescreen, HW key will control normal/content not notification/ringer channel.
Flags: needinfo?(mchen)
(Assignee)

Comment 39

4 years ago
>   1. Play a audio from music app.
>   2. Put music app into background.
>   3. Play then stop video from video app in the foreground.

In this case we don't want to restore the audio of the music app. So probably we should set mPlayableHiddenContentGroupId to unknown. Sort of:

if (aAgent->WithVideo() && aAgent->VisibleState()) {
  mPlayableHiddenContentGroupId = AudioChannelGroupId::UnknownGroupId();
}

Comment 40

4 years ago
> else if (aAgent->Type() == AUDIO_CHANNEL_CONTENT && aAgent->VisibleState()) {
> mPlayableHiddenContentGroupId = AudioChannelGroupId::UnknownGroupId();
> }

In patch 3's AudioChannelService::RegisterAudioChannelAgent(...), codes above will clear mPlayableHiddenContentGroupId already. So in AudioChannelService::HigherChannelsEnumerator() we should do check for "mHigherChannel"

  if (data->mHigherChannel < aAgent->Type()) {
    // If this agent is a content channel in the background and it was paused by policy then we should not count it in.
    if (!aAgent->VisibleState() && aAgent->Type() == AUDIO_CHANNEL_CONTENT && gAudioChannelService->mPlayableHiddenContentGroupId.IsUnknown()) {
      return PL_DHASH_NEXT;
    }
    data->mHigherChannel = aAgent->Type();                                                                
  }
(Assignee)

Comment 41

4 years ago
Created attachment 8343053 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

Marco, thanks for your help.
Attachment #829703 - Attachment is obsolete: true
Attachment #8343053 - Flags: review?(mchen)
(Assignee)

Comment 42

4 years ago
Created attachment 8343230 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent
Attachment #829702 - Attachment is obsolete: true
Attachment #8343230 - Flags: review?(bent.mozilla)

Comment 43

4 years ago
Comment on attachment 8343053 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

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

The other parts are great to me and expect to see it's landing. Thanks.

::: dom/audiochannel/AudioChannelService.cpp
@@ +469,2 @@
>    mDeferTelChannelTimer = nullptr;
> +  mDeferTelChannelAgent = nullptr;

Currently dialer app has a telephony channel for sending key tone during voice_call. When incoming call is coming (dialer is not launched yet) and remote side close the call, the sequences as below are happened

1. dialer app is killed because incoming call is dropped by remote side.
2. GC deletes AudioChannelAgentParent for telephony channel from dialer app.
3. The deconstructor of AudioChannelAgentParent calls stopPlaying() and finally go into the code at line 168 (mDeferTelChannelAgent = aAgent).
4. After timer is expired, the function here is called and assign mDeferTelChannelAgent to null.

According to this agent is already freed during step 2, release() operation performed by smart pointer will be crashed.

I didn't figure out the solution and the first idea is 
  1. add/remove reference actively for audiochannelagent when put/remove it into/from mAgent.
  2. In processing of ipc-shutdown from ::observer(), before remove agent from mAgent we can help to call ::UnregisterAudioChannelAgent() first.
Attachment #8343053 - Flags: review?(mchen)
(Assignee)

Comment 44

4 years ago
Created attachment 8355216 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

the hashtable can take care of the refcounting object if we use nsRefPtrHashKey.
Is this enough? I also added a security check if the child is destroyed before the parent.
Attachment #8343053 - Attachment is obsolete: true
Attachment #8355216 - Flags: review?(mchen)

Comment 45

4 years ago
Comment on attachment 8355216 [details] [diff] [review]
patch 3 - AudioChannelService refactoring

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

I think it is really close to be landed.
And how about the patch for AudioChannelAgent in FMRadio Client?

Thanks.

::: dom/audiochannel/AudioChannelAgentParent.cpp
@@ +41,5 @@
>  bool
>  AudioChannelAgentParent::RecvSetVisibleState(const bool& aVisible,
>                                               int32_t* aCanPlay)
>  {
> +  MOZ_ASSERT(!mDestroyed);

Since "ActorDestroy()" was called already, does it still have chance to receive "SetVisibleState" and "StopPlaying"?
I think these two are redundant.

Updated

4 years ago
Attachment #8355216 - Flags: review?(mchen)
(Assignee)

Comment 46

4 years ago
> I think it is really close to be landed.
> And how about the patch for AudioChannelAgent in FMRadio Client?

That landed. did it?

> Since "ActorDestroy()" was called already, does it still have chance to
> receive "SetVisibleState" and "StopPlaying"?
> I think these two are redundant.

I agree with you but it's just for debugging build and it's a nice check to have.

Comment 47

4 years ago
Hi Andrea,

I mean there is a new attribute - window which should be added into each initialization of AudioChannelAgent (child side).

http://hg.mozilla.org/mozilla-central/file/49d2fce9a86c/dom/fmradio/FMRadio.cpp#l152
(Assignee)

Comment 48

4 years ago
Created attachment 8355499 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

patch updated.
Attachment #8335337 - Attachment is obsolete: true
Attachment #8355499 - Flags: review?(mchen)

Comment 49

4 years ago
Comment on attachment 8355499 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

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

And please move the modification of window attribute in HTMLAudioElement from patch 2 to here. 

Thanks.

::: dom/fmradio/FMRadio.cpp
@@ +149,5 @@
>      do_CreateInstance("@mozilla.org/audiochannelagent;1");
>    NS_ENSURE_TRUE_VOID(audioChannelAgent);
>  
>    audioChannelAgent->InitWithWeakCallback(
> +    static_cast<nsIDOMWindow*>(GetOwner()),

Why do you need to add explicitly casting here but didn't apply in other places?
Attachment #8355499 - Flags: review?(mchen)
(Assignee)

Comment 50

4 years ago
Created attachment 8358352 [details] [diff] [review]
patch 1 - window in the AudioChannelAgent

I just changed what you said in your comments.
Attachment #8355499 - Attachment is obsolete: true
Attachment #8358352 - Flags: review?(mchen)
(Assignee)

Comment 51

4 years ago
Created attachment 8358353 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent
Attachment #8343230 - Attachment is obsolete: true
Attachment #8343230 - Flags: review?(bent.mozilla)
Attachment #8358353 - Flags: review?(bent.mozilla)
(Assignee)

Comment 52

4 years ago
So, it seems that we are almost there. We need just an additional review for the IPDL part.
mchen, what do you think about landing this piece of code soon? Do we have to wait for a particular release?
Flags: needinfo?(mchen)

Updated

4 years ago
Attachment #8358352 - Flags: review?(mchen) → review+

Comment 53

4 years ago
(In reply to Andrea Marchesini (:baku) from comment #52)
> So, it seems that we are almost there. We need just an additional review for
> the IPDL part.
> mchen, what do you think about landing this piece of code soon? Do we have
> to wait for a particular release?

I think now is a right timing because V1.3 already is branched into aurora and V1.4 on master didn't start the sprint 1 yet. So we should not break any mile stone if there is a regression there unfortunately.
Flags: needinfo?(mchen)

Comment 54

4 years ago
Comment on attachment 8358353 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

Blake, do you mind stealing this from Ben please?
Attachment #8358353 - Flags: review?(mrbkap)
(Assignee)

Comment 55

4 years ago
I think it's impossible to have these patches landed before bug 1016277 is fixed. Right?
I'm going to get feedback here by the end of the week.
Requesting this to be prioritized into 2.2 since this blocks bug 876631.
See bug 1015073 for user-facing issue.
feature-b2g: --- → 2.2?
Comment on attachment 8358353 [details] [diff] [review]
patch 2 - IPDL for AudioChannelAgent

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

I think bent needs to review this. I have a couple of questions, though.

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +31,5 @@
>    , mWithVideo(false)
>  {
>  }
>  
>  AudioChannelAgent::~AudioChannelAgent()

Do we have to worry about the shutdown case here, where the connection to the parent is lost and we get dealloc'd by the manager?

@@ +203,3 @@
>    nsCOMPtr<nsIAudioChannelAgentCallback> callback = GetCallback();
>  
> +  if (XRE_GetProcessType() != GeckoProcessType_Default && mActor) {

Why does it make sense to call into the service if we're in a child process but mActor is null?

::: dom/ipc/ContentChild.cpp
@@ +119,5 @@
>  #include "nsContentUtils.h"
>  #include "nsIPrincipal.h"
>  #include "nsDeviceStorage.h"
>  #include "AudioChannelService.h"
> +#include "AudioChannelService.h"

Duplicated.

::: dom/ipc/TabParent.cpp
@@ +1432,5 @@
> +                                         const bool& aWithVideo,
> +                                         int32_t* aCanPlay)
> +{
> +  nsRefPtr<AudioChannelAgentParent> agent = new AudioChannelAgentParent();
> +  return agent.forget().get();

Doesn't this have to be |take()| instead of |get()| now?
Attachment #8358353 - Flags: review?(mrbkap)
(Assignee)

Comment 59

4 years ago
> ::: dom/audiochannel/AudioChannelAgent.cpp
> @@ +31,5 @@
> >    , mWithVideo(false)
> >  {
> >  }
> >  
> >  AudioChannelAgent::~AudioChannelAgent()
> 
> Do we have to worry about the shutdown case here, where the connection to
> the parent is lost and we get dealloc'd by the manager?

In this case the parent should receive an ActorDestoy() call and magically that should be handled.


> > +  if (XRE_GetProcessType() != GeckoProcessType_Default && mActor) {
> 
> Why does it make sense to call into the service if we're in a child process
> but mActor is null?

I don't remember exactly all this code but I know that, the AudioChannelService in the child process is used to know if there is something active in the current app.

> > +  nsRefPtr<AudioChannelAgentParent> agent = new AudioChannelAgentParent();
> > +  return agent.forget().get();
> 
> Doesn't this have to be |take()| instead of |get()| now?

At that time it was take() :)
I have to rewrit^H^H^Hrebase this patch and I guess I'll find a lot of these issue.

Comment 60

3 years ago
Bobby, please follow up this topic.
feature-b2g: 2.2? → ---
Flags: needinfo?(bchien)
(Assignee)

Comment 61

3 years ago
I'm working on the new AudioChannel API.
I guess we can close this bug and file a new one for the new API.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX

Updated

3 years ago
No longer blocks: 853101

Comment 62

3 years ago
clear ni. Already link to AudioChannel meta.
Flags: needinfo?(bchien)
(Assignee)

Updated

3 years ago
Attachment #8358353 - Attachment is obsolete: true
Attachment #8358353 - Flags: review?(bent.mozilla)
You need to log in before you can comment on or make changes to this bug.