Closed Bug 1198165 Opened 9 years ago Closed 9 years ago

[AudioChannel]Play a song, then play a video, and drap the progress bar, the music will continue playing.

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+, b2g-v2.2 unaffected, b2g-v2.5 verified, b2g-master verified)

VERIFIED FIXED
FxOS-S10 (30Oct)
blocking-b2g 2.5+
Tracking Status
b2g-v2.2 --- unaffected
b2g-v2.5 --- verified
b2g-master --- verified

People

(Reporter: qiutian, Assigned: djf)

References

Details

(Keywords: regression, Whiteboard: [2.5-aries-test-run-1] [landing eta 10/27])

Attachments

(7 files)

Attached video AriesKK_v2.5_video.3gp
[1.Description]:
[Aries KK v2.5][Flame KK v2.5][AudioChannel] When playing a song, back to homescreen. Then play a video, and drap its progress bar, the music will continue playing.
Found time:22:50
See attachements:logcat_video.txt, AriesKK_v2.5_video.3gp

[2.Testing Steps]: 
1.Launch Music,play a song.
2.Back to homescreen.
3.Launch Video, play a video.
4.Drap the progress bar.

[3.Expected Result]: 
4.There is no sound.

[4.Actual Result]: 
4.The sound of Music has been resumed.

[5.Reproduction build]: 
Device: Arise KK 2.5 (Affected)
Build ID               20150824124704

Gaia Revision          d7fb5717d3e0153ac64af2c0d5c11079846d81c3

Gaia Date              2015-08-24 10:07:41

Gecko Revision https://hg.mozilla.org/mozillacentral/rev/ba43a48d3c528cc956335793e02504e5ca2c149f

Gecko Version          43.0a1

Device Name            aries

Firmware(Release)      4.4.2

Firmware(Incremental)  eng.worker.20150824.122007

Firmware Date          Mon Aug 24 12:20:15 UTC 2015

Bootloader             s1

Device: Flame KK 2.2 (Unaffected)
Build ID               20150824032503
Gaia Revision          335cd8e79c20f8d8e93a6efc9b97cc0ec17b5a46
Gaia Date              2015-08-14 19:06:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1effc4cb6414
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150824.065639
Firmware Date          Mon Aug 24 06:56:50 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK 2.5 (Affected)
Build ID               20150824150208
Gaia Revision          d7fb5717d3e0153ac64af2c0d5c11079846d81c3
Gaia Date              2015-08-24 10:07:41
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/ba43a48d3c528cc956335793e02504e5ca2c149f
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150824.182403
Firmware Date          Mon Aug 24 18:24:15 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

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

[7.TCID]: 
Free Test
Attached file logcat_video.txt
[Blocking Requested - why for this release]:
The audio should to play from the background app when video is playing.

Adding qawanted to repro this on the latest build.
blocking-b2g: --- → 2.5?
Keywords: qawanted
This issue is still reproducible on latest Aries and Flame. With song playing in Music app in background, pausing a video in Video app in foreground resumes song playing in Music app.

Device: Flame 2.5
BuildID: 20150911030227
Gaia: 6280500a6cb8d1b178cdd163450e36d22846fbed
Gecko: c0abc2a6e11f52761366e029eb1bae4c9864a8a3
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 43.0a1 (2.5) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Device: Aries 2.5
BuildID: 20150911152752
Gaia: 758c75ee087ea3722213ea2c185cca1d952c8a29
Gecko: 12b6c9a7fe539c9da25a5682a6b8f2dac0678bad
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 43.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
mozilla-inbound Regression Window:

Last Working Environmental Variables:
Device: Flame KK 2.5
Build ID               20150710163851
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gaia Date              2015-07-10 14:29:23
Gecko Revision         https://hg.mozilla.org/integration/mozilla-inbound/rev/07bcf36f5ab2
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150709.042047
Firmware Date          Thu Jul  9 04:20:56 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

First Broken Environmental Variables:
Device: Flame KK 2.5
Build ID               20150710170452
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gaia Date              2015-07-10 14:29:23
Gecko Revision         https://hg.mozilla.org/integration/mozilla-inbound/rev/675ea719b91c
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150709.042047
Firmware Date          Thu Jul  9 04:20:56 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

First Broken Gaia & Last Working Gecko - issue DOES NOT repro
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gecko Revision         https://hg.mozilla.org/integration/mozilla-inbound/rev/07bcf36f5ab2

First Broken Gecko & Last Working Gaia - issue DOES repro
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gecko Revision         https://hg.mozilla.org/integration/mozilla-inbound/rev/675ea719b91c

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=07bcf36f5ab2&tochange=675ea719b91c
Hi  Andrea,
This bug is probably caused by Bug 1113086, could you help to check?  
Thank you.
Flags: needinfo?(amarchesini)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Flags: needinfo?(amarchesini) → needinfo?(alwu)
Assignee: nobody → alwu
Flags: needinfo?(alwu)
This issue is because that "the audio competing behavior is incorrect. Content channel shouldn't be resumed after interruption ended". [1] You can test this behavior easily by [2].

---

Here I would explain why this issue happens.

*advance preparation* Playing music from music app first, then play the video from video app.

Every time we drag the process bar, the video is being seeking.
In normal situation, when we want to seek the video, we only need to call seek() once.

However, the video app does some extra behaviors during the seeking process, it calls pause() -> play() -> seek(). When we call pause(), the audio channel agent would be unregistered. Then the system app would follow the audio competing policy to decide whether we need to resume/no-resume the music.

In this case, the music should not be resumed when the video is paused (extra called by seeking behavior).
System app should modify its competing logic.

[1] UX Sound Spec : https://bug1068219.bmoattachments.org/attachment.cgi?id=8579177#12
[2] Testing steps.
      1. Play music from music app
      2. Play video from video app
      3. Pause video
      4. Music is resumed (Wrong) 

---

Hi, Evan,
Could you help with this issue?
It seems that the audio competing logic is incorrect.
Very appreciate :)
Assignee: alwu → nobody
Component: AudioChannel → Gaia::System::Audio Mgmt
Flags: needinfo?(evan)
Blocks 2.5 with a P2 priority
blocking-b2g: 2.5? → 2.5+
Priority: -- → P2
As Alastor said in Comment 6 "the video app does some extra behaviors during the seeking process, it calls pause() -> play() -> seek()", that is root cause of this bug.

Video App cannot call pause() when it want to seek a video, or the music will continue playing.
Component: Gaia::System::Audio Mgmt → Gaia::Video
Flags: needinfo?(evan)
(In reply to Alastor Wu [:alwu] from comment #6)
> This issue is because that "the audio competing behavior is incorrect.
> Content channel shouldn't be resumed after interruption ended". [1] You can
> test this behavior easily by [2].
> 
> ---
> 
> Here I would explain why this issue happens.
> 
> *advance preparation* Playing music from music app first, then play the
> video from video app.
> 
> Every time we drag the process bar, the video is being seeking.
> In normal situation, when we want to seek the video, we only need to call
> seek() once.
> 
> However, the video app does some extra behaviors during the seeking process,
> it calls pause() -> play() -> seek(). When we call pause(), the audio
> channel agent would be unregistered. Then the system app would follow the
> audio competing policy to decide whether we need to resume/no-resume the
> music.
> 
> In this case, the music should not be resumed when the video is paused
> (extra called by seeking behavior).
> System app should modify its competing logic.
> 
In spec[1], it said the music should be resumed when video is paused/end, just like the case 2 "PAUSE the current channel, RESUME the current channel after the new channel ends"

[1]: https://bug1068219.bmoattachments.org/attachment.cgi?id=8579177#12
Flags: needinfo?(rnicoletti)
I believe the reason the video app pauses the video before seeking in this situation is that if the video is playing when the slider is being dragged and the user stops dragging the slider and keeps their finger on the slider bar, the video will begin playing. This would cause problems if the user was trying to seek to a particular location in the video -- they find the location they want in the video but don't remove their finger quickly enough or they're looking at the frame deciding if they want to re-position the slider and then ... the video starts playing -- that would be frustrating. Also, my guess is the spec regarding this behavior says the video should pause when the progress bar is first moved and then play when the user releases the progress bar.

jsavory, can you comment on this issue?
Flags: needinfo?(rnicoletti) → needinfo?(jsavory)
Typically whether it is playing or paused when the slider is released depends on the behaviour before it is dragged. In this case if the video is paused and then dragged and released, the expected behaviour would be that it is still paused. If it had been playing beforehand, it would begin playing again.
Flags: needinfo?(jsavory)
Sorry to NI you again. I didn't represent the situation or my question clearly enough. This issue is about when the slider is dragged when the video is playing. In that situation, the video app pauses the video before changing the position and then the video app resumes playing the video when the dragging is released.

When the video app is opened after a song had been playing in the music app, the music is paused. When the video app pauses the video when starting to drag the slider, the music begins to play. That is what this bug is about. The suggestion is to not pause the video when the slider is first being dragged (comment 8).

My concern with not pausing the video when first starting to drag the slider is: if the user stops moving the slider but does not release it (continues contact with it), the video will begin playing. That seems undesirable to me because the user has not released the slider and would not expect the video to begin playing until the slider has been released.

Do you think my concern is justified? 

Thanks!
Flags: needinfo?(jsavory)
Oh sorry for misunderstanding! I agree that your concern is valid but I do think we need to pause the video before seeking, since as you mentioned, it would be awkward to have it playing while dragging. However, I tried out the seeking behaviour with the music playing while holding the slider, and that experience is not ideal either. 

It might be best to not resume music playback after the video is paused or stopped in general. Since we don't even know how long the video might be, the music doesn't need to play again after the video has ended. So in this case, if the user is playing music and then plays a video, the music should not play again until the user initiates it. 

Hopefully this answers your question, but feel free to NI me again if I missed it :)
Flags: needinfo?(jsavory)
David,

Could you help with this bug? Russ has a couple of bugs already. 

Thanks
Hema
Flags: needinfo?(dflanagan)
I think the right solution to this bug is that when the video app is in the foreground, we should not let background music play even if the user is not currently playing a video.  It seems silly to have music start and stop when the users pauses and resumes a video.  Let's just stop the music when the video app is lanuched, and keep the music stopped until the user switches to the homescreen or to another app.

Jacqueline: do you agree with the above plan?

Jim: I seem to recall that in Music or Ringtones you did something with Web Audio in order to hold on to an audio channel and not allow music to play in the background. Does that sound familiar? Can you remind me what you did?
Assignee: nobody → dflanagan
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(jsavory)
Flags: needinfo?(dflanagan)
Here's the relevant code: https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L143
Flags: needinfo?(squibblyflabbetydoo)
Jim's code is not working for me to stop music playback in the video app. Jim's ringtones app actually plays a competing sound, and I'm trying to get the blocking effect without actually playing anything, so that could be the difference, I suppose.

Also, with the ringtones code, the music doesn't resume once the user is done setting a ringtone and returns to the settings app. That is probably a gecko regression, but maybe Jim's technique is too successful.
I can get the AudioContext code to block the music if I modify it to use an OscillatorNode as the source. But then there is an annoying tone playing whenever the app is in the foreground.  I can use a Gain node to reduce the volume almost to 0.  And I can even do context.suspend() to pause the oscillator and still block the music. That might be the solution I need. I do worry, however, that a fix like that might turn on unnecessary hardaware or start threads running that don't need to be running.  I'll want to make sure that CPU usage does not go up and that video frame rate does not go down.  It is probably also important to check on power consumption while playing a video if I'm going to use this.
It turns out that this is not specific to the Video app. It also affects video playback in Gallery and in Camera.

So we either have to fix the regression caused by bug 1113086, or I need to patch three different Gaia apps to work around that regression.

Note that this bug also means that if you have music playing, then play a video, then pause the video the music resumes instantly.  As Jacqueline notes in comment #13, that is bad UX. And I believe that that is also a regression.

I'd argue that if there is a video element on the content channel in the foreground app, then that video element should hold on to the content channel even when it is paused.  Maybe it shouldn't take the channel until playback begins, but once the background music is interrupted, the video element should hold on to the channel (and not allow background music to resume) until the video element is unloaded or the element is removed from the tree or something like that.  

In the past, music did not resume when the user paused the video directly with the pause button or indirectly for the seek UI. Now the music does resume, and I think that is a bug best fixed in the audio channel API.

Andrea and Alastor: is this something that is possible to fix in Gecko before the 11/2 FC deadline?  Or do I need to figure out a workaround? Can you suggest a good workaround that the media apps could use to block background music playing in the content channel? Does what I describe in comment #18 sound reasonable, or do you think it is likely to impair performance?
Flags: needinfo?(amarchesini)
Flags: needinfo?(alwu)
To be specific, here is the blocking code I've found that can work to block background music. I worry about the performance impact of adding this to Camera, Gallery and Video, however:

      this.context = new AudioContext('content');
      this.source = this.context.createOscillator();
      this.gain = this.context.createGain();
      this.source.connect(this.gain);
      this.gain.connect(this.context.destination);
      this.source.type = 'square';
      this.source.frequency.value = 1;
      this.gain.gain.value = 0.000001;
      this.source.start();
      this.context.suspend();

I run the code above when the app comes to the foreground, then run the following when it goes to the background so that the music app can resume the background music:

      this.source.stop();
      this.source.disconnect();
      this.context.close();
I've just tried investigating to see what happens if I stop the explicit pause/play when the user drags to seek within the movie. For a longish (multi-minute) movie, the UI seems fine... it isn't moving too much for me to find my spot.  But... the music starts playing again during the seek, even though I have commented out the pause() call.

This regression is in Camera, Gallery and Video apps. Seeking within a playing video obviously should not cause the background content channel to resume. So that, at least, is a bug that must be fixed in gecko.  I would hope that it could be fixed in a way so that just pausing a video does not cause a music resume either.  That is, I still think that this entire bug should be reassigned to the audio channel compoent and be fixed in gecko.

But if need be, I can continue to try to develop a workaround based on the code in comment #20.
If you have to do the stuff in comment 20, I'd really recommend checking battery usage. Gecko devs put a fair amount of pressure on the Ringtones app to stop using AudioContext since it used a lot of battery (I think it might be better now, though).
Hi, David,
From comment 9, the content type audio needs to be resumed after the interruption ended.
In addition, the UX spec also mentioned that "An inactive app can decide whether or not to resume when the channel is no longer in use".
It means that even if the system app doesn't directly resume the Music app, the Music app can also listen the interruption event to resume itself.

Therefore, I think we have two solutions here,
1. Do not pause the video when we want to seek. [1]
2. Use workaround that play the silence sound before seeking, and stop that sound when the seeking is finished. (like comment20.)

IMMO, I prefer the solution 1. 
It's very resonable to resume the music when the video is paused. 
Even if we don't resume music, the music app also can resume itself because there is no active audio channel.

However, if we choose the solution 1, there are some following Gecko issues [1].
I think we can file another two bugs to handle them.

How do you think?
Thanks :)

---

[1] (In reply to David Flanagan [:djf] from comment #21)
> I've just tried investigating to see what happens if I stop the explicit
> pause/play when the user drags to seek within the movie. For a longish
> (multi-minute) movie, the UI seems fine... it isn't moving too much for me
> to find my spot.  But... the music starts playing again during the seek,
> even though I have commented out the pause() call.

After removing the pause() from the video app, the background music still can be resumed. It's a gecko bug, and I also found another bug.

If we remove the pause from video app, there are two gecko bugs we need to fix,
(1) During seeking, the audio channel agent would still be unregistered.
(2) After seeking, the video frame would be freezed, we can only hear the audio.

The bug (1) is the reason why the music can still be resumed, I'll investigate it.
Flags: needinfo?(alwu) → needinfo?(dflanagan)
See Also: → 1215447
(In reply to Alastor Wu [:alwu] from comment #23)

> Therefore, I think we have two solutions here,
> 1. Do not pause the video when we want to seek. [1]
> 2. Use workaround that play the silence sound before seeking, and stop that
> sound when the seeking is finished. (like comment20.)
> 
> IMMO, I prefer the solution 1. 
> It's very resonable to resume the music when the video is paused. 

I don't actually think it is reasonable, but that is a matter for UX, not for us programmers. My argument is that if the user was listening to music but then decided to watch a video, they have probably lost interest in the music and will be surprised if it resumes automatically, especially if it resumes when they have only temporarily paused the video. Obviously the music should resume after an interruption like a phone call. But in this case, the user has decided to direct their attention elsewhere, and I don't think they want the music to come back.

> Even if we don't resume music, the music app also can resume itself because
> there is no active audio channel.

Are you saying that even though our music app doesn't do that some other app might? So therefore all of our video playing apps need to guard against it? I guess it is hard to argue against that.

> If we remove the pause from video app, there are two gecko bugs we need to
> fix,
> (1) During seeking, the audio channel agent would still be unregistered.

Thanks for filing the bug for that.

> (2) After seeking, the video frame would be freezed, we can only hear the
> audio.

I have not noticed that one. I assume that the video resumes eventually but that the audio starts playing before the video starts moving, right? I'd guess that might be codec dependent and happens if it is faster to seek audio than video, maybe? If that is what you are describing, I suspect it would be hard to fix and we will just have to live with it.
 
If you are able to fix bug 1215447, I'm willing to remove the pauses when the user seeks. That is probably the fastest way to resolve this blocker.

I'd prefer, however, if we could just fix the regression at the source.  Do you understand what the regression is? Has the audio competing policy actually changed? Are we now explicitly and intentionally resuming music in situations where we did not resume it before? Evan cites the spec from bug 1068219, but maybe that spec needs to be reconsidered.
Flags: needinfo?(dflanagan) → needinfo?(alwu)
Depends on: 1215447
(In reply to David Flanagan [:djf] from comment #24)
> I don't actually think it is reasonable, but that is a matter for UX, not
> for us programmers. My argument is that if the user was listening to music
> but then decided to watch a video, they have probably lost interest in the
> music and will be surprised if it resumes automatically, especially if it
> resumes when they have only temporarily paused the video. Obviously the
> music should resume after an interruption like a phone call. But in this
> case, the user has decided to direct their attention elsewhere, and I don't
> think they want the music to come back.

We can ask UX to reconsider the audio competing table.
Actually, it still has lots of thing need to be improved.

> Are you saying that even though our music app doesn't do that some other app
> might? So therefore all of our video playing apps need to guard against it?
> I guess it is hard to argue against that.

I mean the previous interrupted app should have a right to resume its audio, when there are no other active audio channel.
But in this case, it exactly shouldn't be resumed, because we're just doing seeking, not pause.

> I have not noticed that one. I assume that the video resumes eventually but
> that the audio starts playing before the video starts moving, right? I'd
> guess that might be codec dependent and happens if it is faster to seek
> audio than video, maybe? If that is what you are describing, I suspect it
> would be hard to fix and we will just have to live with it.

Since I don't know lots about video codec, maybe we can solve this one first and then open another bug to deal with it.

> If you are able to fix bug 1215447, I'm willing to remove the pauses when
> the user seeks. That is probably the fastest way to resolve this blocker.

The bug1215447 was solved now, you can remove the pause() during the seeking.

> I'd prefer, however, if we could just fix the regression at the source.  Do
> you understand what the regression is? Has the audio competing policy
> actually changed? Are we now explicitly and intentionally resuming music in
> situations where we did not resume it before? Evan cites the spec from bug
> 1068219, but maybe that spec needs to be reconsidered.

Yes, we'll ask UX to update this spec.
Flags: needinfo?(alwu)
NI UX Team. Need your input here. 

Thanks
Flags: needinfo?(firefoxos-ux-bugzilla)
Whiteboard: [2.5-aries-test-run-1] → [2.5-aries-test-run-1] [landing eta 10/27]
Target Milestone: --- → FxOS-S10 (30Oct)
Comment on attachment 8679170 [details] [review]
[gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master

Russ,

This is a pretty straightforward patch that just removes the pause on drag code from video.js, view.js and video_player.js.  The only other thing I had to do was to call updateSlider() when the drag ends. This was because video.js and vid.js  don't update the thumb while the user's finger is on it. But since we don't pause, it can be at the wrong time when the finger is lifted. And if the video has reached the end and paused while our finger is down then we don't get a timeupdate event to move the thumb.  video_player.js moves the thumb even during drags so doesn't have that problem.

I don't like fighting the audio competing policy like this, but this patch does seem to solve the problem so we should probably just get it landed for 2.5.

Note that there is a semi-related patch coming in 1159343 which will prevent calling fastSeek() while we are already seeking. That should improve responsiveness, I hope. And, if you try this patch out, you might want to try it out with that patch in as well to see how they work together.
Attachment #8679170 - Flags: review?(rnicoletti)
Oops. Looks like there are unit tests I broke with this patch.
Okay, pull request updated with a fix for the broken tests
Comment on attachment 8679170 [details] [review]
[gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master

David, I'm concerned that we're trading one undesirable behavior (music starts playing when user uses the slider to move the position of the video) for another (video begins to play while user is moving the position of the video -- see comment 12 and comment 13 -- one particularly undesirable aspect is when the slider is no longer being moved (and the finger is still on the slider) the video plays starting from where it was when the slider was first touched, not from where the slider has been moved to). And it's not obvious to me which undesirable situation will ocurr more often.

Regarding the interaction of this patch and the bug 1159343 patch, I tested the bug 1159343 in isolation and it works very nicely. However, when I tested it as part of this patch it did not work well; it appears seeking when the video is playing is problematic. I'm not seeing a 'seeked' event when 'fastSeek' is invoked while the video is playing. The result is the seeking stops immediately after the slider is first moved.

If the behavior of the two patches could be combined I would be much more comfortable with change in behavior introduced by this patch because at least when the video began to play -- while the slider was still in the process of being adjusted -- it would play from the current position of the video. But instead, as I described, there is an issue seeking.

I am giving r- because based on my testing the negative interaction of this patch and that of bug 1159343 is unacceptable.
Attachment #8679170 - Flags: review?(rnicoletti) → review-
(In reply to Russ Nicoletti [:russn] from comment #31)
> Comment on attachment 8679170 [details] [review]
> [gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master
> 
> David, I'm concerned that we're trading one undesirable behavior (music
> starts playing when user uses the slider to move the position of the video)
> for another (video begins to play while user is moving the position of the
> video -- see comment 12 and comment 13 -- one particularly undesirable
> aspect is when the slider is no longer being moved (and the finger is still
> on the slider) the video plays starting from where it was when the slider
> was first touched, not from where the slider has been moved to). 

That's very weird. Like the seek calls were not working at all? Were you seeing this when you tested this patch alone, or when you applied it on top of the one in the other bug?  (If fastSeek is failing to send 'seeked' then I would expect what you describe here)  

And it's
> not obvious to me which undesirable situation will ocurr more often.
> 
> Regarding the interaction of this patch and the bug 1159343 patch, I tested
> the bug 1159343 in isolation and it works very nicely. However, when I
> tested it as part of this patch it did not work well; it appears seeking
> when the video is playing is problematic. I'm not seeing a 'seeked' event
> when 'fastSeek' is invoked while the video is playing. The result is the
> seeking stops immediately after the slider is first moved.
> 
> If the behavior of the two patches could be combined I would be much more
> comfortable with change in behavior introduced by this patch because at
> least when the video began to play -- while the slider was still in the
> process of being adjusted -- it would play from the current position of the
> video. But instead, as I described, there is an issue seeking.
> 
> I am giving r- because based on my testing the negative interaction of this
> patch and that of bug 1159343 is unacceptable.

Okay. Maybe we need to use a setTimeout in addition to the 'seeked' event in that other bug. I've already suggested that there.

I agree with the points in comment #12 and #13 and think that the audio competing policy should be fixed. But I think the quickest way to resolve this blocker for 2.5 is to remove the pause() calls when seeking, if we can make that work.
Comment on attachment 8679170 [details] [review]
[gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master

I've tested this again after re-flashing my device. I re-flashed because even when testing with master I was seeing unexpected behavior while dragging the slider (similar to what I described in my original review of this patch). After re-applying this patch and the bug 1159343 patch, the behavior I'm seeing looks good. As I mentioned in my review comments, even though the video will play while the user has not finished changing the position of the video (when they stop moving the slider but don't remove their finger), it is playing at the position the slider was moved to (thanks to the bug 1159343 patch).

I am convinced the behavior I saw while doing the original review was due to an unrelated issue with my device. I am therefore approving this patch with the caveat that it be landed after bug 1159343 lands.
Attachment #8679170 - Flags: review- → review+
Sorry for the delay here, yes I think we can change from the audio competing policy spec on this one as it doesn't seem to work in this case of music and video. We can also work towards updating the spec to a more recent version.

In terms of the UX, I still believe that it would be best if the music pauses when a video is started and not resume later. Sorry, I'm just a bit unclear if that is possible or not in terms of implementation from the comments. 

David, I'm not too sure about pausing when the video app is opened since it would then act differently for gallery and possibly browser? It would be better if we could keep it consistent throughout all video players. And if we paused for each of these apps it might result in a lot of unwanted pausing.
Flags: needinfo?(jsavory)
Flags: needinfo?(firefoxos-ux-bugzilla)
David, I've just landed bug 1210860. I believe you'll need to rebase your patch.
Flags: needinfo?(dflanagan)
Comment on attachment 8679170 [details] [review]
[gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master

Russ, do you have time to review this again? I've incorporated the fix for bug 1159343 into this one, and have modified that fix to be more sophisticated about when it defers seek requests. I think the performance is better this way, and since these two bugs are interrelated, I like having the fixes in the same patch so we can test them together.

I have not tested the view activity yet, but will test it before this lands. If you've got time to try out the regular video player and see if you think that seeking performance is improved, I'd love to know if this is making a difference or if it is all in my head...
Flags: needinfo?(dflanagan)
Attachment #8679170 - Flags: review+ → review?(rnicoletti)
Comment on attachment 8679170 [details] [review]
[gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master

John: if you have a chance to try this patch out, I'd appreciate your opinion on whether it adequately fixes bug 1159343.
Attachment #8679170 - Flags: feedback?(jolin)
Still more test failures. Updated patch coming soon.
Comment on attachment 8679170 [details] [review]
[gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master

I find there is a difference in performance with the latest patch. Mostly when dragging quickly where I would think what's happening is the intermediate seeks are deferred because the request are made within the seek interval. Therefore, I am giving this r+ as the performance is better than the previous patch.

I left a couple of comments (questions) in the PR.
Attachment #8679170 - Flags: review?(rnicoletti) → review+
Thanks Russ. I've answered the questions on github. That code is still needed.  I'll land this once the (newly updated) tests are green.
Taskcluster is busted right now, so the test runs will have to wait
Tests finally ran.

Landed to master: https://github.com/mozilla-b2g/gaia/commit/5ed2d4ebf03ccc1ab44e95a9eb57936231b1e036

This should also resolve bug 1159343.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to David Flanagan [:djf] from comment #37)
> Comment on attachment 8679170 [details] [review]
> [gaia] davidflanagan:bug1198165-2 > mozilla-b2g:master
> 
> John: if you have a chance to try this patch out, I'd appreciate your
> opinion on whether it adequately fixes bug 1159343.

 It does. Thanks a lot!
Attachment #8679170 - Flags: feedback?(jolin) → feedback+
Hi David, 

   I has verified the bug as "fail" on the latest build of Flame KK 2.5&2.6 and Aries KK v2.6 by the STR in comment 0.

Actual results: The sound of Music has been resumed in step 4.
See attachment: verified_logcat_1724.txt, verified_Aries KK_v2.6.3gp.
Reproduce rate: 10/10.


Device: Aries KK 2.6(affected)
Build ID               20151101012023
Gaia Revision          91cac94948094cfdcd00cba5c6483e27e80cb3b0
Gaia Date              2015-10-28 20:32:15
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/96377bdbcdf3e444a22aeaa677da696243b00d98
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151101.004008
Firmware Date          Sun Nov  1 00:40:17 UTC 2015
Bootloader             s1

Device: Flame KK 2.6(affected)
Build ID               20151101150206
Gaia Revision          91cac94948094cfdcd00cba5c6483e27e80cb3b0
Gaia Date              2015-10-28 20:32:15
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/96377bdbcdf3e444a22aeaa677da696243b00d98
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151101.183234
Firmware Date          Sun Nov  1 18:32:48 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK 2.5(affected)
Build ID               20151101004501
Gaia Revision          91cac94948094cfdcd00cba5c6483e27e80cb3b0
Gaia Date              2015-10-28 20:32:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/36d8fc192b45d26eb4d32bbeae19f79cb2aebb65
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151101.031103
Firmware Date          Sun Nov  1 03:11:16 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0
Flags: needinfo?(dflanagan)
Tony,

Note that in comment #44 you say that you are using Gaia from 2015-10-28.

But from comment #42 you can see that this patch did not land until 2015-10-29.

Maybe there was a build failure or something, but it looks like you were doing verification with the wrong version of gaia.

Please check that and verify again.
Flags: needinfo?(dflanagan) → needinfo?(tianxu)
This bug has been verified as "pass" on the latest build of Aries KK v2.6 and Flame KK v2.6 512mb by the STR in comment 0.

Actual results: Play a video, drop the progress bar, the sound of Music does not resumed.
See attachment: Verified_Aries_KK_v2.6_vid.3gp

Reproduce rate: 0/10

Device: Aries KK 2.6 (master)(Pass)
Build ID               20151104004249
Gaia Revision          61918ddd9ccce104c009e873e34a0791e125753a
Gaia Date              2015-11-03 17:22:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151104.000116
Firmware Date          Wed Nov  4 00:01:24 UTC 2015
Bootloader             s1


Device: Flame KK 2.6 512mb (master)(Pass)
Build ID               20151103150203
Gaia Revision          61918ddd9ccce104c009e873e34a0791e125753a
Gaia Date              2015-11-03 17:22:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151103.182550
Firmware Date          Tue Nov  3 18:26:03 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0
Flags: needinfo?(tianxu)
This bug has been verified as "pass" on the latest build of Aries KK v2.5 and Flame KK v2.5 512mb by the STR in comment 0.

Actual results: Play a video, drop the progress bar, the sound of Music does not resumed.

See attachment: Verified_Aries_KK_v2.5_vid.3gp
Reproduce rate: 0/10

Device: Aries KK v2.5(Pass)
Build ID               20151109233837
Gaia Revision          07baf613699fa6225359c7f04825c5caeb71d424
Gaia Date              2015-11-09 21:32:50
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/772f6c235fc48d91efd6346f5b3d8b216eb2dcb1
Gecko Version          44.0a2
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151109.225902
Firmware Date          Mon Nov  9 22:59:11 UTC 2015
Bootloader             s1

Device: Flame KK v2.5 512MB(Pass)
Build ID               20151109004552
Gaia Revision          cf646c52bb947af28329b0a100df91d1b1f2a907
Gaia Date              2015-11-09 02:55:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4eafef5b80f8985c94c4a067f130d37513e1a581
Gecko Version          44.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151109.041411
Firmware Date          Mon Nov  9 04:14:26 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0
Status: RESOLVED → VERIFIED
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: