1214157 - Video is paused, but at video player view it still shows Pause button after starting to play a song with bluetooth speaker/headphone.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect, P2)

Description

[Queena.Wen(Leave from Mozilla)]

Attachment: logcat_1753.txt

[1.Description]:
[Aries KK v2.5][Flame KK v2.5] At video player view, it still shows Pause button on the control bar after starting to play a song with  bluetooth speaker/headphone, but acutally video is paused.
See Attachments: Aries kk v2.5.3gp&logcat_1753.txt
Found Time: 17:53

[2.Testing Steps]: 
1. Enable Bluetooth and connect to a bluetooth speaker/headphone.
2. Launch music app and choose a song to play.
3. Tap home button.
4. Launch video app and play a video.
5. Use bluetooth speaker/headphone to start playing a song.  

[3.Expected Result]: 
5. At video player view, it should show Play button.

[4.Actual Result]: 
5. Video is paused, but at video player view it still shows Pause button on the control bar.

[5.Reproduction build]: 
Device: Flame KK v2.2 (Unaffected)
Build ID               20151012032501
Gaia Revision          885647d92208fb67574ced44004ab2f29d23cb45
Gaia Date              2015-10-07 13:05:24
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ab6c34bfacf7
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151012.072013
Firmware Date          Mon Oct 12 07:20:24 EDT 2015
Fireware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK v2.5 (Affected)
Build ID               20151012150203
Gaia Revision          0b934d06c04adff2cd9bdd0bc204f974a18b710f
Gaia Date              2015-10-12 12:15:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f4215b484d529e01f0f84ff4005e3321ee98b727
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151012.183433
Firmware Date          Mon Oct 12 18:34:45 EDT 2015
Fireware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK v2.5 (Affected)
Build ID               20151012230518
Gaia Revision          0b934d06c04adff2cd9bdd0bc204f974a18b710f
Gaia Date              2015-10-12 12:15:30
Gecko Revision         https://hg.mozilla.org/integration/b2g-inbound/rev/11ff0ccb7d59311df4c190d331c8b58c6e35a0c8
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151012.224830
Firmware Date          Mon Oct 12 22:48:38 UTC 2015
Bootloader             s1


[6.Reproduction Frequency]: 
Always Recurrence,10/10

[7.TCID]: 
Free Test

Comment 1

[Queena.Wen(Leave from Mozilla)]

Attachment: Aries kk v2.5.3gp

Comment 2

[William Hsu [:whsu]]

[Blocking Requested - why for this release]:

NGA related issue?
inconsistent UI. Request for fixing.

Comment 3

[Hema Koka [:hema]]

Any of this behavior changed deliberately for 2.5?

Comment 4

[Justin D'Arcangelo [:justindarc]]

(In reply to Hema Koka [:hema] from comment #3)
> Any of this behavior changed deliberately for 2.5?

Nothing changed in the Music NGA app regarding audio competing policy. However, it seems there are several audio competing policy regressions in Gecko. This sounds similar to some other bugs I've seen.

Comment 5

[Russ Nicoletti [:russn]]

I know of no deliberate changes. This issue seems similar to bug 1167545, which Punam is investigating.

Punam, NI so you can advise after investigating 1167545.

Comment 6

[Punam Dahiya [:pdahiya]]

Hi Russ

Investigated and this issue is different. Bug 1167545 is about play/pause button not working when view activity is called twice without unloading successfully.

I am able to replicate this issue and here is what I see in logs when the video player is paused but the button doesn't change to play.

10-19 17:57:22.268 D/wpa_supplicant( 3419): wlan0: Control interface command 'SIGNAL_POLL'
10-19 17:57:22.278 D/wpa_supplicant( 3419): nl80211: survey data missing!
10-19 17:57:23.118 W/AudioFlinger( 3212): Thread AudioOut_2 cannot connect to the power manager service
10-19 17:57:23.118 E/AudioFlinger( 3212): no wake lock to update!
10-19 17:57:23.298 W/AudioFlinger( 3212): Thread AudioOut_2 cannot connect to the power manager service
10-19 17:57:23.298 E/AudioFlinger( 3212): no wake lock to update!
10-19 17:57:23.728 D/GonkAudioDecoderManager(13834): Got EOS frame!
10-19 17:57:23.728 D/GonkMediaDataDecoder(13834): eos output
10-19 17:57:23.878 W/AudioFlinger( 3212): Thread AudioOut_2 cannot connect to the power manager service
10-19 17:57:23.878 E/AudioFlinger( 3212): no wake lock to update!
10-19 17:57:25.118 W/AudioFlinger( 3212): Thread AudioOut_2 cannot connect to the power manager service
10-19 17:57:25.118 E/AudioFlinger( 3212): no wake lock to update!

I will investigate some more and update bug. Thanks!

Comment 7

[Punam Dahiya [:pdahiya]]

Here are steps used to replicate this issue without bluetooth speaker/headphone.

1. Launch music app and choose a song to play.
2. Pause the playing song in music app. (This will show music controls in notification)
3. Launch video app and play a video.
4. While video is playing, drag notification tray and press play button to play song.

This will pause video but will not change the pause button for video.

Comment 8

[Punam Dahiya [:pdahiya]]

And did a quick test by adding event listeners inside video.js for pause and play.

dom.player.addEventListener('pause', function () {
  console.log('event pause');
});

dom.player.addEventListener('play', function () {
  console.log('event play');
});

When playing the song from notification , it pauses video but doesn't trigger pause event listener for video player indicating this communication is happening at gecko level.

Comment 9

[Punam Dahiya [:pdahiya]]

Got help over IRC from squib, here's the snippet

12:48 pdahiya: try listening for mozinterruptbegin / mozinterruptend
12:48 squib:ok, will try now
12:52 squib: Yay!! it works, thank you!
12:53 pdahiya: no problem! those are the events you need to respond to any kind of interruption from the audiochannel
12:54 squib: cool, this is the regression for 2.5, I wonder how it was working before that
12:54 pdahiya: probably because there was a special case for videos that got removed? it sounds like the audiochannel code is being rewritten
12:56 squib:ok, I will attach patch to handle these event listeners for video app, thanks again!

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug1214157 > mozilla-b2g:master

Comment 11

[Punam Dahiya [:pdahiya]]

Comment on attachment 8675881 [details] [review]
[gaia] punamdahiya:Bug1214157 > mozilla-b2g:master

Hi Russ

Attaching patch that explicitly handle mozinterruptbegin and mozinterruptend events in video.js to set correct play/pause button state. Please review. Thanks!

Comment 12

[Punam Dahiya [:pdahiya]]

IMO even though we have a patch for gaia, this should have been fixed in gecko as it was always done in previous releases. Setting NI flag for Sotaro to help answer if this audio channel interruption case for video can be handled easily inside gecko. Thanks!

Comment 13

[Punam Dahiya [:pdahiya]]

Moving NI flag to Alastor who is helping with audio channel API rewrite. Thanks!

Comment 14

[Russ Nicoletti [:russn]]

Comment on attachment 8675881 [details] [review]
[gaia] punamdahiya:Bug1214157 > mozilla-b2g:master

Looks good to me. I left a couple of nits in the PR.

Comment 15

[Alastor Wu [:alwu]]

Hi, Punam,
Does the reproducing step 5 means that "play the music from the hardware button of the BT heatset and keep video app on the foreground"?

If so, when we resume the music, the video would be stopped by the system app. More precisely, this behavior is "interrupted" , not "paused". The audio channel API can't change the internal behavior of the media element, so the video UI doesn't be changed and keep displaying the play icon.

If you want to change the video UI, you need handle the moz-interrupted events in video app. 

BTW, we'll remove the "moz-interrupted" event from the MediaElement/AudioContext in our future plan [1], and then send the interrupted event from the system app. This implementation is in the bug1206581. You can see the reason in [2]. 

Therefore, the app will need to listen the "moz-interrupted" from the |navigator.mozSetMessageHandler|, instead of the specific MediaElement/AudioContext.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1192748#c23
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1206581#c0

Comment 16

[Punam Dahiya [:pdahiya]]

(In reply to Alastor Wu [:alwu] from comment #15)
> Hi, Punam,
> Does the reproducing step 5 means that "play the music from the hardware
> button of the BT heatset and keep video app on the foreground"?
> 
That's correct, please see comment #7 for STR without using BT headset.

> If so, when we resume the music, the video would be stopped by the system
> app. More precisely, this behavior is "interrupted" , not "paused". The
> audio channel API can't change the internal behavior of the media element,
> so the video UI doesn't be changed and keep displaying the play icon.
> 
> If you want to change the video UI, you need handle the moz-interrupted
> events in video app. 
> 
> BTW, we'll remove the "moz-interrupted" event from the
> MediaElement/AudioContext in our future plan [1], and then send the
> interrupted event from the system app. This implementation is in the
> bug1206581. You can see the reason in [2]. 
> 
> Therefore, the app will need to listen the "moz-interrupted" from the
> |navigator.mozSetMessageHandler|, instead of the specific
> MediaElement/AudioContext.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1192748#c23
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1206581#c0

Comment 17

[Punam Dahiya [:pdahiya]]

(In reply to Russ Nicoletti [:russn] from comment #14)
> Comment on attachment 8675881 [details] [review]
> [gaia] punamdahiya:Bug1214157 > mozilla-b2g:master
> 
> Looks good to me. I left a couple of nits in the PR.

Thanks Russ for review! Similar to bug 1198165 this regression can be seen in video app (both video.js and view.js), gallery and camera app (shared/js/media/video_player.js). I will update the patch to listen to moz-interrupt event in all three apps. 
Also, David recommended that if video playback in these three apps is interrupted than we should pause the playing video and not resume on receiving moz-interrupt-end event. This should save us from maintaining a third state 'interrupted'.

Comment 18

[Punam Dahiya [:pdahiya]]

> Also, David recommended that if video playback in these three apps is
> interrupted than we should pause the playing video and not resume on
> receiving moz-interrupt-end event. This should save us from maintaining a
> third state 'interrupted'.

Debugged and looks like dom.player.play(); throws mozinterruptbegin event and dom.player.pause() throws mozinterruptend event. 

Handling mozinterruptend explicitly to pause video will break the video player main play/pause flow inside video.js. Will investigate if there is an easy way to figure if mozinterruptend is coming from outside the video app (audio channel API).

Comment 19

[Punam Dahiya [:pdahiya]]

Corrections: 

a) dom.player.play() throws play, mozinterruptbegin and mozinterruptend event. 
b) dom.player.pause() throws pause and no interrupt events

Comment 20

[Punam Dahiya [:pdahiya]]

(In reply to Punam Dahiya [:pdahiya] from comment #19)
> Corrections: 
> 
> a) dom.player.play() throws play, mozinterruptbegin and mozinterruptend
> event. 
> b) dom.player.pause() throws pause and no interrupt events

Setting NI flag for Alastor to confirm if above event sequencing is correct for dom.player.play() and why we need to throw mozinterruptend when calling dom.player.play()?

If the above sequencing is correct, it will be difficult to pause the video app on mozinterruptend and will have to wait for Bug1206581. (https://bugzilla.mozilla.org/show_bug.cgi?id=1206581#c0)

Comment 21

[Alastor Wu [:alwu]]

Hi, Punam,
Sorry for the inconvenience.
This behavior is a bug, "moz-interrupt" event should only be sent when the audio competing happens. 
We are solving it in bug 1206581. You can see our plan in [1], we're in the stage 1 now.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1192748#c23

Comment 22

[Punam Dahiya [:pdahiya]]

Alastor: Thanks for clarifying, to implement flow to not resume video app on receiving mozinterruptend event for Gallery,Camera and Video apps, I have created bug 1217139 and marked it dependent on bug 1206581.
Including David to share this finding.

Russ: I have updated patch to fix the issue reported in this bug for Video app. Please review. Thanks!

Comment 23

[Russ Nicoletti [:russn]]

Comment on attachment 8675881 [details] [review]
[gaia] punamdahiya:Bug1214157 > mozilla-b2g:master

Hi Punam, the change looks good with the caveat that bug 1206581 is resolved in the 2.5 timeframe. That's because with this patch, as long as dom.player.play() produces mozinterruptbegin/end, when a video is played the 'play/pause' button is quickly changed from 'play' to 'pause' and to 'play' again. Sometimes it's not noticeable but sometimes it is. I wouldn't want us to ship 2.5 with that behavior.

Alistor, is the plan to address bug 1206581 for the 2.5 release? I have noticed it's not marked as a 2.5 blocker.

Comment 24

[David Flanagan [:djf]]

Punam,

Thanks for letting me know the status of this. What a mess!  I agree with Russ's concern. If we introduce flickering of the play/pause button on every press that is arguably worse than the original bug here, since that is a pretty rare corner case.

Consider introducing a new variable lastPlayTime. Just before you call dom.player.play(), set lastPlayTime = performance.now().  Then, when you get a mozinterruptbegin event, you can check performance.now() - lastPlayTime.  If that is a small number then you can ignore the mozinterruptbegin event.  And actually, if you have that working, then you can try my idea of just calling pause() when you get mozinterruptbegin.

If you do something like that then your patch isn't dependent on Alastor's bug getting fixed in time.  Though you should be able to remove the workaround when bug 1206581 lands...

If you do that, then you can land this bug and not worry about it again until it is time to remove the hack. Otherwise, we just move the 2.5+ here to another bug and have to worry about whether that lands in time.

Comment 25

[Punam Dahiya [:pdahiya]]

Thanks Russ and David for your feedback, I will update patch with the workaround to remove the dependency on bug 1206581. Thanks!

Comment 26

[Alastor Wu [:alwu]]

Hi, all,
The bug 1206581 might be finished in v2.5 (depending on the review progress), but it's just in the stage 1 of the our plan. [1]

The whole plan of new "audio-interrupt" event would not be landed in v2.5. It would be on the next release.

Therefore, I think Punam can use the David's suggestion (comment24), and then remove the hack after finishing the new "audio-interrupt" event.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1192748#c23

Comment 27

[Punam Dahiya [:pdahiya]]

Comment on attachment 8675881 [details] [review]
[gaia] punamdahiya:Bug1214157 > mozilla-b2g:master

Hi Russ
I have updated patch with the workaround in both video.js and view.js. Please review. Thanks!

Comment 28

[Russ Nicoletti [:russn]]

Comment on attachment 8675881 [details] [review]
[gaia] punamdahiya:Bug1214157 > mozilla-b2g:master

Hi Punam, I left a (fairly long) comment in the PR for what is essentially a nit. But I'm curious what you think of my comment.

Comment 29

[Punam Dahiya [:pdahiya]]

Hi Russ

Thanks for helping review the patch, I have updated patch with your feedback and put a fix in view activity for a bug seen when we open view activity for the first time. Please let me know if I can carry r+ on the updated patch.

Including David to share and get his feedback on implementing the workaround for view activity. Thanks

Comment 30

[Russ Nicoletti [:russn]]

I have added a comment in the PR, suggesting yet another alternative. It may be more simple than the current one and I don't believe it is affected by the multiple interrupt events triggered by the 'view activity'.

If this third option turns out not to work, I suggest your second option: don't implement the fix for the view activity. I am wary of a workaround that relies on the timing of events received; we may still see this problem even with the workaround and all the while having ugly code that could remain for a long time.

Comment 31

[Punam Dahiya [:pdahiya]]

(In reply to Russ Nicoletti [:russn] from comment #30)
> I have added a comment in the PR, suggesting yet another alternative. It may
> be more simple than the current one and I don't believe it is affected by
> the multiple interrupt events triggered by the 'view activity'.
> 
> If this third option turns out not to work, I suggest your second option:
> don't implement the fix for the view activity. I am wary of a workaround
> that relies on the timing of events received; we may still see this problem
> even with the workaround and all the while having ugly code that could
> remain for a long time.

Hi Russ, The third option is not going to work for view.js (See github). I agree we should keep the workaround in video.js to unblock on the issue reported in this bug and fix the edge cases as part of bug 1217139 when Audio Channel API dependency is resolved. Thanks!

Comment 32

[Punam Dahiya [:pdahiya]]

Comment on attachment 8675881 [details] [review]
[gaia] punamdahiya:Bug1214157 > mozilla-b2g:master

Hi Russ

Setting review flag on updated patch that fixes the issue in video.js. Please review. Thanks!

Comment 33

[Punam Dahiya [:pdahiya]]

Thanks Russ for review. Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/b22a890d3adef4f8b44e5623916a240bf21502d5

Comment 34

[Norry.L.F(Leave from Mozilla)]

Hi Queena,

Verifyme Please.

Comment 35

[Queena.Wen(Leave from Mozilla)]

This bug has been verified as "pass" on the latest build of Flame KK 2.5 and Aries KK 2.5 by the STR in comment 0.

Actual results: At video player view it shows Play button after starting to play a song with bluetooth speaker/headphone.
See attachment: verified_Aries KK_v2.5.3gp.
Reproduce rate: 0/10.

Device: Aries KK v2.5 (Pass)
Build ID               20151027221526
Gaia Revision          a26eadc5e1133d5112b6cbc10badbb7670a1090f
Gaia Date              2015-10-27 17:36:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151027.213419
Firmware Date          Tue Oct 27 21:34:27 UTC 2015
Bootloader             s1

Device: Flame KK v2.5 (Pass)
Build ID               20151027150240
Gaia Revision          a26eadc5e1133d5112b6cbc10badbb7670a1090f
Gaia Date              2015-10-27 17:36:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151027.184038
Firmware Date          Tue Oct 27 18:40:49 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Comment 36

[Queena.Wen(Leave from Mozilla)]

Attachment: verified_Aries KK_v2.5.3gp