Closed Bug 1194442 Opened 4 years ago Closed 4 years ago

Code clean up of AudioManager

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
FxOS-S6 (04Sep)
Tracking Status
firefox43 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 1 obsolete file)

It seems better to do some code clean up before Bug 1194249
Blocks: 1194249
Assignee: nobody → sotaro.ikeda.g
Removed global variables and some static functions.
Attachment #8650084 - Flags: review?(alwu)
Comment on attachment 8650084 [details] [diff] [review]
patch - Code clean up of AudioManager

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

Review minus because there are some codes can be clean up better. 

There are lots of similar codes about AudioSystem::setDeviceConnectionState(), we can collect them into AudioManager::UpdateDeviceConnectionState(). 
In addition, we can use |mDevicesState| to store all devices, instead of just storing the headset information (mHeadsetState).

If you have any better ideas or my suggestion is wrong, please discuss it with me!
Hope to see another review.

::: dom/system/gonk/AudioManager.cpp
@@ +189,5 @@
> +  } else if (mHeadsetState & AUDIO_DEVICE_OUT_WIRED_HEADPHONE) {
> +    UpdateHeadsetConnectionState(SWITCH_STATE_HEADPHONE);
> +  } else {
> +    UpdateHeadsetConnectionState(SWITCH_STATE_OFF);
> +  }

We can call
UpdateDeviceConnectionState(mDevicesState, AUDIO_POLICY_DEVICE_STATE_AVAILABLE);

@@ +307,5 @@
>  
>  NS_IMPL_ISUPPORTS(AudioManager, nsIAudioManager, nsIObserver)
>  
> +void
> +AudioManager::UpdateHeadsetConnectionState(hal::SwitchState aState)

Change it to UpdateDeviceConnectionState(device, available_state, address), only use this function to do AudioSystem::setDeviceConnectionState().

@@ +373,5 @@
>        SwitchProfileData(DEVICE_BLUETOOTH, false);
>      }
>    } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
> +    if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE && mA2dpSwitchDone) {
> +

Empty line.

@@ +382,5 @@
> +            return;
> +          }
> +          AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_A2DP,
> +                                                audioState,
> +                                                aAddress.get());

UpdateDeviceConnectionState(...);

@@ +547,5 @@
> +      NS_NewRunnableFunction([self]() {
> +        if (self->mSwitchDone) {
> +          return;
> +        }
> +        self->UpdateHeadsetConnectionState(SWITCH_STATE_OFF);

UpdateDeviceConnectionState(...);

@@ +555,5 @@
> +
> +    SwitchProfileData(DEVICE_HEADSET, false);
> +    mSwitchDone = false;
> +  } else if (aEvent.status() != SWITCH_STATE_OFF) {
> +    UpdateHeadsetConnectionState(aEvent.status());

UpdateDeviceConnectionState(...);

@@ +575,3 @@
>  AudioManager::AudioManager()
>    : mPhoneState(PHONE_STATE_CURRENT)
> +  , mHeadsetState(0)

mDevicesState

@@ +588,5 @@
>  #endif
>  {
>    RegisterSwitchObserver(SWITCH_HEADPHONES, mObserver);
>  
> +  UpdateHeadsetConnectionState(GetCurrentSwitchState(SWITCH_HEADPHONES));

UpdateDeviceConnectionState(...);

@@ +793,5 @@
>  {
>    AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_FM,
>      aFmRadioAudioEnabled ? AUDIO_POLICY_DEVICE_STATE_AVAILABLE :
>      AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE, "");
> +  UpdateHeadsetConnectionState(GetCurrentSwitchState(SWITCH_HEADPHONES));

UpdateDeviceConnectionState(...);

::: dom/system/gonk/AudioManager.h
@@ +107,5 @@
>  protected:
>    int32_t mPhoneState;
> +
> +  // A bitwise variable for recording what kind of headset/headphone is attached.
> +  int32_t mHeadsetState;

Not just remember the headset state, we can use bitwise the store all active devices.

Use |mDevicesState|.

@@ +110,5 @@
> +  // A bitwise variable for recording what kind of headset/headphone is attached.
> +  int32_t mHeadsetState;
> +
> +  bool mSwitchDone;
> +

I'm think about whether we can remove mSwitchDone and mA2dpSwitchDone. Because these attributes are showing the alive status of the present device, maybe we can use |mDevicesState| to achieve that?

@@ +173,5 @@
>    // Promise functions.
>    void InitProfileVolumeSucceeded();
>    void InitProfileVolumeFailed(const char* aError);
>  
> +  void UpdateHeadsetConnectionState(hal::SwitchState aState);

void UpdateDeviceConnectionState(audio_devices_t,
                                 audio_policy_dev_state_t,
                                 device_address);
Attachment #8650084 - Flags: review?(alwu) → review-
(In reply to Alastor Wu [:alwu] from comment #3)
> Comment on attachment 8650084 [details] [diff] [review]
> patch - Code clean up of AudioManager
> 
> Review of attachment 8650084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Review minus because there are some codes can be clean up better. 
> 
> There are lots of similar codes about
> AudioSystem::setDeviceConnectionState(), we can collect them into
> AudioManager::UpdateDeviceConnectionState(). 
> In addition, we can use |mDevicesState| to store all devices, instead of
> just storing the headset information (mHeadsetState).

I wanted to make a change incrementally. I am thinking to do it in another bug. This bug is clean up without changing how to control audio.
(In reply to Alastor Wu [:alwu] from comment #3)
> Comment on attachment 8650084 [details] [diff] [review]
> patch - Code clean up of AudioManager
> 
> Review of attachment 8650084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Review minus because there are some codes can be clean up better. 
> 
> There are lots of similar codes about
> AudioSystem::setDeviceConnectionState(), we can collect them into
> AudioManager::UpdateDeviceConnectionState(). 
> In addition, we can use |mDevicesState| to store all devices, instead of
> just storing the headset information (mHeadsetState).
> 
> If you have any better ideas or my suggestion is wrong, please discuss it
> with me!
> Hope to see another review.
> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +189,5 @@
> > +  } else if (mHeadsetState & AUDIO_DEVICE_OUT_WIRED_HEADPHONE) {
> > +    UpdateHeadsetConnectionState(SWITCH_STATE_HEADPHONE);
> > +  } else {
> > +    UpdateHeadsetConnectionState(SWITCH_STATE_OFF);
> > +  }
> 
> We can call
> UpdateDeviceConnectionState(mDevicesState,
> AUDIO_POLICY_DEVICE_STATE_AVAILABLE);

Current implementation is doing only about headset/headphone state. Therefore I changed the name to say that. I also thinking to change the function name to UpdateDeviceConnectionState(), but it need some changed to audio controlling. I am going to do it in another bug.

> 
> @@ +307,5 @@
> >  
> >  NS_IMPL_ISUPPORTS(AudioManager, nsIAudioManager, nsIObserver)
> >  
> > +void
> > +AudioManager::UpdateHeadsetConnectionState(hal::SwitchState aState)
> 
> Change it to UpdateDeviceConnectionState(device, available_state, address),
> only use this function to do AudioSystem::setDeviceConnectionState().

same as the above.

> 
> @@ +373,5 @@
> >        SwitchProfileData(DEVICE_BLUETOOTH, false);
> >      }
> >    } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
> > +    if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE && mA2dpSwitchDone) {
> > +
> 
> Empty line.

I am going to update in a next patch.

> 
> @@ +382,5 @@
> > +            return;
> > +          }
> > +          AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_A2DP,
> > +                                                audioState,
> > +                                                aAddress.get());
> 
> UpdateDeviceConnectionState(...);

same as the above.

> 
> @@ +547,5 @@
> > +      NS_NewRunnableFunction([self]() {
> > +        if (self->mSwitchDone) {
> > +          return;
> > +        }
> > +        self->UpdateHeadsetConnectionState(SWITCH_STATE_OFF);
> 
> UpdateDeviceConnectionState(...);

same as the above.


> 
> @@ +555,5 @@
> > +
> > +    SwitchProfileData(DEVICE_HEADSET, false);
> > +    mSwitchDone = false;
> > +  } else if (aEvent.status() != SWITCH_STATE_OFF) {
> > +    UpdateHeadsetConnectionState(aEvent.status());
> 
> UpdateDeviceConnectionState(...);

Same as the above.

> 
> @@ +575,3 @@
> >  AudioManager::AudioManager()
> >    : mPhoneState(PHONE_STATE_CURRENT)
> > +  , mHeadsetState(0)
> 
> mDevicesState

It is same as the above. Current implementation is handling only headset/headphone sate. Therefore, the name is kept. I also going to change it as mDevicesStates, but in another bug, because it require audio control change.

> 
> @@ +588,5 @@
> >  #endif
> >  {
> >    RegisterSwitchObserver(SWITCH_HEADPHONES, mObserver);
> >  
> > +  UpdateHeadsetConnectionState(GetCurrentSwitchState(SWITCH_HEADPHONES));
> 
> UpdateDeviceConnectionState(...);

Same as the above.

> 
> @@ +793,5 @@
> >  {
> >    AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_FM,
> >      aFmRadioAudioEnabled ? AUDIO_POLICY_DEVICE_STATE_AVAILABLE :
> >      AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE, "");
> > +  UpdateHeadsetConnectionState(GetCurrentSwitchState(SWITCH_HEADPHONES));
> 
> UpdateDeviceConnectionState(...);

Same as the above.

> 
> ::: dom/system/gonk/AudioManager.h
> @@ +107,5 @@
> >  protected:
> >    int32_t mPhoneState;
> > +
> > +  // A bitwise variable for recording what kind of headset/headphone is attached.
> > +  int32_t mHeadsetState;
> 
> Not just remember the headset state, we can use bitwise the store all active
> devices.

Current bug, I do not want to change how to control audio. I am going to change it another bug.

> 
> Use |mDevicesState|.
> 
> @@ +110,5 @@
> > +  // A bitwise variable for recording what kind of headset/headphone is attached.
> > +  int32_t mHeadsetState;
> > +
> > +  bool mSwitchDone;
> > +
> 
> I'm think about whether we can remove mSwitchDone and mA2dpSwitchDone.
> Because these attributes are showing the alive status of the present device,
> maybe we can use |mDevicesState| to achieve that?

I do not think it is a good idea to do it in this bug. It is better to do it in another bug.

> 
> @@ +173,5 @@
> >    // Promise functions.
> >    void InitProfileVolumeSucceeded();
> >    void InitProfileVolumeFailed(const char* aError);
> >  
> > +  void UpdateHeadsetConnectionState(hal::SwitchState aState);
> 
> void UpdateDeviceConnectionState(audio_devices_t,
>                                  audio_policy_dev_state_t,
>                                  device_address);
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> (In reply to Alastor Wu [:alwu] from comment #3)
> > Comment on attachment 8650084 [details] [diff] [review]
> > patch - Code clean up of AudioManager
> > 
> > Review of attachment 8650084 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Review minus because there are some codes can be clean up better. 
> > 
> > There are lots of similar codes about
> > AudioSystem::setDeviceConnectionState(), we can collect them into
> > AudioManager::UpdateDeviceConnectionState(). 
> > In addition, we can use |mDevicesState| to store all devices, instead of
> > just storing the headset information (mHeadsetState).
> 
> I wanted to make a change incrementally. I am thinking to do it in another
> bug. This bug is clean up without changing how to control audio.

I want to make clear the focus of the bug. I am not fun of doing many things in one bug. This bug is about cleaning up the code without audio control change.
Blocks: 1196724
I applied only the comment about blank line. For another comments, I am going to do it in Bug 1196724 to make clear the bug's focus.
Attachment #8650084 - Attachment is obsolete: true
Attachment #8650445 - Flags: review?(alwu)
> 
> ::: dom/system/gonk/AudioManager.h
> @@ +107,5 @@
> >  protected:
> >    int32_t mPhoneState;
> > +
> > +  // A bitwise variable for recording what kind of headset/headphone is attached.
> > +  int32_t mHeadsetState;
> 
> Not just remember the headset state, we can use bitwise the store all active
> devices.

We can not store all devices as bitwise. There are conflicts between 'out devices' and 'in devices'.
Comment on attachment 8650445 [details] [diff] [review]
patch - Code clean up of AudioManager

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

Thanks for the work!
Attachment #8650445 - Flags: review?(alwu) → review+
Thanks. The refactoring is going to be done in Bug 1196724.
https://hg.mozilla.org/mozilla-central/rev/625b155671eb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
Blocks: 1218629
No longer blocks: 1218629
You need to log in before you can comment on or make changes to this bug.