1158293 - [Audio Channel] [Bluetooth] [Music] - Can't leave paused state when unplugging wired headset after being connected to bluetooth headphones.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Joshua Mitchell (Inactive)]

Attachment: logcat_20150424_1113.txt

Description:
In the music app, if you are listening to music through a bluetooth headset and then plug in a wired headphones the music will switch over to that set of headphones as appropriate. However, if the music app advances to a new track / song and then you unplug the wired headphones, you will not be able to unpause the track. 
When removing the wired headphones the music app will pause (also as appropriate) but upon hitting the play button music should resume through the prior bluetooth headphones. Instead, when you hit the play button the icon will change from play to pause (to indicate it has started playing again) but the track will not advance.

This state can be remedied by physically dragging the indicator on the track or by hitting track forward/back

This bug does not reproduce if the track has not changed


Repro Steps:
1) Update a Flame to 20150424010200
2) Connect a Bluetooth Headphones to the device
3) Launch Music app and play a track (listening through the BT headset)
4) Connect a wired headphones (Music will play from the new headphones)
5) Advance the track to a new song (or allow it to advance itself)
6) Unplug the wired headphones
7) Press the 'Play' button


Actual:
Track remains in a pause state, music does not play

Expected:
Hitting the Play button will resume play through the bluetooth headphones


Environmental Variables:
Device: Flame 3.0 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150424010200
Gaia: 0c5e2ee1173f3c53379ef3cd10de714836258fe8
Gecko: 22a157f7feb7
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Repro frequency: 8/8
See attached: logcat, video clip: http://youtu.be/ypTo7p9tzQg

Comment 1

[Joshua Mitchell (Inactive)]

This issue does NOT reproduce on Flame 2.2

Actual Results: Upon removing the plugged headset and hitting the pause/play button the track will continue playing through bluetooth headphones.

Device: Flame 2.2 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150424002507
Gaia: b838d0e7c163e66660dcb6e387d8339944a7a30e
Gecko: 5fe76b26e55f
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Functional regression.

Requesting a window.

Comment 3

[Yeojin Chung [:YeojinC]]

Mozilla-inbound Regression Window:

Last Working Environmental Variables:
Device: Flame 3.0
BuildID: 20150331094702
Gaia: 1d14f4a6809de81ebd638117ed4ddd3b1b18f033
Gecko: 83af1139a6d3
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

First Broken Environmental Variables:
Device: Flame 3.0
BuildID: 20150331100302
Gaia: 1d14f4a6809de81ebd638117ed4ddd3b1b18f033
Gecko: 1f5a169f0476
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Last Working Gaia First Broken Gecko: Issue DOES reproduce 
Gaia: 1d14f4a6809de81ebd638117ed4ddd3b1b18f033
Gecko: 1f5a169f0476

First Broken Gaia Last Working Gecko: Issue does NOT reproduce
Gaia: 1d14f4a6809de81ebd638117ed4ddd3b1b18f033
Gecko: 83af1139a6d3

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=83af1139a6d3&tochange=1f5a169f0476

Caused by bug 1139206

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Sotaro, can you take a look at this please? Looks like the landing for Bug 1139206 might have caused this.

Comment 5

[Sotaro Ikeda [:sotaro]]

Yes, I take a look.

Comment 6

[Sotaro Ikeda [:sotaro]]

The problem was in MediaOmxCommonDecoder::ResumeStateMachine(). It set seek as MediaDecoderEventVisibility::Suppressed. It suppressed the MediaDecoder's state change when seek was done. The seek should be MediaDecoderEventVisibility::Observable here.

Comment 7

[Sotaro Ikeda [:sotaro]]

I faced another problem during debugging. In gecko's media, there are 2 AudioSink classes. It caused the naming conflict on local build. It also needs to be addressed.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix AudioOffloadPlayer problem

Comment 9

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=275f4f6d26c1

Comment 10

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8598084 [details] [diff] [review]
patch - Fix AudioOffloadPlayer problem

I am going to update the patch.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix AudioOffloadPlayer problem

Remove crash related fix. It is going to move to bug 1158692.

Comment 12

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8598247 [details] [diff] [review]
patch - Fix AudioOffloadPlayer problem

Current patch has one drawback. A bogus seek event is emitted. Then, I am going to try another way of fix.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix ResumeStateMachine()'s seek handling

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8598657 [details] [diff] [review]
patch - Fix ResumeStateMachine()'s seek handling

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

Why does the MediaDecoderStateMachine::SetDormant(false) call not do the seek for you?

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #14)
> Comment on attachment 8598657 [details] [diff] [review]
> patch - Fix ResumeStateMachine()'s seek handling
> 
> Review of attachment 8598657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why does the MediaDecoderStateMachine::SetDormant(false) call not do the
> seek for you?

It calls seek if MediaDecoder::mRequestedSeekTarget is set. The previous patch attachment 8598084 [details] [diff] [review] uses it. But it needs the seek to be Observable event as in Comment 6.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8598657 [details] [diff] [review]
patch - Fix ResumeStateMachine()'s seek handling

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

::: dom/media/omx/MediaOmxCommonDecoder.cpp
@@ +128,5 @@
>    SecondsToUsecs(mCurrentTime, timeUsecs);
>    mRequestedSeekTarget = SeekTarget(timeUsecs,
>                                      SeekTarget::Accurate,
>                                      MediaDecoderEventVisibility::Suppressed);
> +  // Call Seek of MediaDecoderStateMachine to suppress seek events. 

Nit: trailing whitespace.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix ResumeStateMachine()'s seek handling

Apply the comment. Carry "r=cpearce".

Comment 18

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02ffa1e00567

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02ffa1e00567