860584 - [Buri][ACC] Speaker sound when plug out earphone.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect, P1)

Description

[sync-1]

+++ This bug was initially created as a clone of Bug #424591 +++
 
 DEFECT DESCRIPTION:
 Speaker sound when plug out earphone.
 
 REPRODUCING PROCEDURES:
 1, Plug in earphone and play a song.
 2, Plug out earphone, speaker also sound for a short time.
 
 EXPECTED BEHAVIOUR:
 No sound when plug out earphone.
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 SW114
 
 USER IMPACT:
 
 REPRODUCING RATE:
 
 For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #424591 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This issue comes from stoping music is too slow. We may refine the message system to avoid this sound leakage.

Comment 2

[sync-1]

Maybe android do like this:
 audio route change is been delayed 1000ms, and music pause immediately.
 I want to know is firefox OS do with this issue like Android?
 
 thanks!

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The android first brocast the ACTION_AUDIO_BECOMING_NOISY to let music paused audio in the WiredAccessoryObserver.java and switch patch after 1000ms.

Comment 4

[Daniel Coloma:dcoloma]

This is not a blocker in my opinion.

Comment 5

[David Scravaglieri [:scravag]]

We would track it for consistency

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch 1

Comment 7

[sync-1]

this patch couldn't solve the problem, and I even cannot compile code successfully with this patch.
 
 Are you sure this patch is for solving the PR?

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This patch is generated in mozilla-central trunk. What kind of build break message do you find?

Comment 9

[sync-1]

Created an attachment (id=395833)
 modified patch.
 
 I modify the error from your patch, The attachment is the new modified patch.You can diff it with your patch.
 But this new patch cannot work yet.
 pls confirm it.
 
 thanks

Comment 10

[sync-1]

Attachment: modified patch.

Created an attachment (id=395833)
 modified patch.
 
 I modify the error from your patch, The attachment is the new modified patch.You can diff it with your patch.
 But this new patch cannot work yet.
 pls confirm it.
 
 thanks

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

Can you try this?

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

hg format patch, delay 1s (this value comes from WiredAccessoryObserver.java) to switch path back to speaker.

Comment 13

[sync-1]

ok. this patch works.
 
 thanks.

Comment 14

[Michael Wu [:mwu]]

This patch is a bit of a hack.

What would be best is if we could wait for all listeners of headphone event to run before switching outputs.

Comment 15

[Marco Chen [:mchen]]

Hi Michael,

I think moving NotifyHeadphonesStatus() in front of InternalSetAudioRoutes() is what we should to. But I think only this can't solve this issue except we limit all corresponding actions by content process & app layer MUST response it in the blocking call.

So maybe we can composite your opinion with 1 second time out together for more safety. How about this? Thanks.

Comment 16

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This sound leakage comes from some PCM buffer queue in the audioflinger and media system, So we need some time to wait those buffer flush out to audio codec.

Comment 17

[Michael Wu [:mwu]]

I don't mind adding a small time out if the hardware doesn't respond fast enough to stream pausing. Please check first though - if pausing/stopping the stream is fast enough, we can avoid an arbitrary delay altogether. At least to me, hitting pause in the music app appears to have very low latency.

Comment 18

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This delay magic number comes from 
https://android.googlesource.com/platform/frameworks/base/+/0b285499db739ba50f2f839d633e763c70e67f96/services/java/com/android/server/WiredAccessoryObserver.java 
L208.

Comment 19

[sync-1]

 We have not got the patch in daily version.Any problem with the patch yet?

Comment 20

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2.1

Re-base to current mc.

We refer to android design and use the 1sec delay for switching audio path back to speaker. The short sound I think is the buffer in audio sub system. So we may need short time to wait the pcm audio data flush out and let device switch back to speaker.

Comment 21

[Marco Chen [:mchen]]

I/GonkSwitch(  428): SwitchEventRunnable begin

  I/AudioManager(  428): notify start
    V/AudioFlinger(  115): ThreadBase::setParameters() routing=2
    W/AudioHardwareMSM76XXA(  115): rpc_snd_set_device(6, 1, 1)
  I/AudioManager(  428): notify end

  I/Audiochannelmanager(  542): notify start
    I/mediaelement(  542): pause start ()

I/GonkSwitch(  428): SwitchEventRunnable end

Actions above is ran on main threads of each process.
---------------------------------------------------------
Actions below is ran on non-main thread of content process.

      V/AudioFlinger(  115): pause(4096), calling thread 542
      V/AudioFlinger(  115): ACTIVE/RESUMING => PAUSING (4096) on thread 0x1e6aac0
      E/AudioSystem(  115): AudioSystem::get_audio_policy_service 0x1e6a2c8
    I/mediaelement(  542): stop
  I/Audiochannelmanager(  542): notify end

This log shows that 
  1. switch event is sent to AudioManager on parent process first then routing path is switched to speaker already.
  2. Then switch event is sent to AudioChannelManager on child process then notify the music app about headset missing.
  3. Then music app start to pause the music.
  4. According to decode/mediastatemachine is ran on another thread, so there is a latency there.

Conclusion: After 1, the routing path is changed to speaker already so the latency from 2 ~ 4 will cause the sound output to speaker.

Comment 22

[Marco Chen [:mchen]]

Comment on attachment 775673 [details] [diff] [review]
patch v2.1

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

::: dom/system/gonk/AudioManager.cpp
@@ +117,5 @@
>  
>    return false;
>  }
>  
> +static void ProcessDelayAudioRoutes(SwitchState aState)

ProcessDelayedAudioRoute

@@ +287,2 @@
>      NotifyHeadphonesStatus(aEvent.status());
> +    // Avoid speaker output the short sound when user plug out the headset when music is playing.

Could you re-phrase this comment?

Comment 23

[Marco Chen [:mchen]]

Hi Michael,

Please refer to comment 21 for why do we need to add this delay. Thanks.

Comment 24

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patvh v2.2

Fix nits

Comment 25

[Marco Chen [:mchen]]

Comment on attachment 776930 [details] [diff] [review]
patvh v2.2

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

::: dom/system/gonk/AudioManager.cpp
@@ +282,2 @@
>      NotifyHeadphonesStatus(aEvent.status());
> +    // Avoid users plug out headset and the speaker also output sounds for a short time.

How about the comment as below?

// When user pulled out the headset, a delay of routing here can avoid the leakage of audio from speaker.

Comment 26

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: checl-in patch

Carry reviewer and modify comment.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/27c0318dba85

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27c0318dba85

Comment 29

[sync-1]

resloved