971414 - [Sora][streaming]video streaming is still playing after lock the screen

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: AudioChannel, defect, P2)

Description

[sync-1]

Mozilla build ID: 20131208040203
 
 DEFECT DESCRIPTION:
 streaming is still playing after lock the screen
 
  REPRODUCING PROCEDURES:
 1.Enable wifi and connected wifi;
 2.Launch browser and go to:http://imps.tcl-ta.com/liuqiong/media/content_e5.html;
 3.Select video media;
 4.Open any one streaming to play;
 5.Press power key to lock screen during playing;Streaming is still playing.->KO
 
  EXPECTED BEHAVIOUR:
 Streaming should be paused after lock screen.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:5/5
 
  For FT PR, Please list reference mobile's behavior:Beetle lite FF has same phenomenon,but this is a bug.
 tel:7035

Comment 1

[Jason Smith [:jsmith]]

Not a regression, so this is not a blocker.

Comment 2

[sync-1]

(In reply to comment #1)
 > Comment from Mozilla:Not a regression, so this is not a blocker.
 > 
 
 Hi,
   Please give me some advice about how can I trace related code.

Comment 3

[buri.blff]

It's block.

Comment 4

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

I know this issue is not a regression issue, but considering the bad user experience it has caused, I believe it should be nominated as a blocker...

Here is a video to this issue:

http://www.youtube.com/watch?v=OVFj0FgY030&feature=youtu.be

As you can see, the stream video not only keeps playing after user press the power key, but it keeps playing intermittently, which is really annoying..So can we reconsider to nominate this as blocker?

Thanks

Comment 5

[Jason Smith [:jsmith]]

Hmm...something seems odd here. I don't recall the audio gaps happening on Buri in the 1.1 timeframe. Are we absolutely sure Buri isn't seeing the same issue with the audio gaps?

Comment 6

[Jason Smith [:jsmith]]

Adding qawanted to get a video for comparison on 1.1 Buri.

Comment 7

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Jason Smith [:jsmith] from comment #5)
> Hmm...something seems odd here. I don't recall the audio gaps happening on
> Buri in the 1.1 timeframe. Are we absolutely sure Buri isn't seeing the same
> issue with the audio gaps?

Buri 1.1 doesn't has this problem. 

http://www.youtube.com/watch?v=1m_rHheouwY&feature=youtu.be

Also I test with 1.3 phone from other partner, also has the audio gap problem.

Comment 8

[Jason Smith [:jsmith]]

So here's what I think we should do:

This bug as filed isn't a regression, as it's talking about the case where streaming is playing while the phone is sleeping. However, the audio gaps is a regression & should be nominated to block. So let's open a new bug for the audio gaps problem.

TCL - Can you open a separate bug for the audio gaps problem & nominate that?

Comment 9

[buri.blff]

We expect streaming will be paused after lock screen.

Comment 10

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hello tester -

In fact, in the current FFOS, the design is when you press the power key, the phone typically won't close the current operations, for example, the FM radio, the voice call will all keep working once you press the power key, hence to me, it is our desired behavior, not a bug. So for 1.3 release, we will only fix the audio gap problem. 

Thanks

Vance

Comment 11

[Jason Smith [:jsmith]]

Just saw bug 975955 get filed - thanks.

Comment 12

[buri.blff]

I think the video should be paused after lock screen when playing it on browser. Such as: the video app on FFOS.

Comment 13

[sync-1]

I can't receive "visibilitychange" when only play video on browser.

Comment 14

[sync-1]

After press power key, I can't receive "visibilitychange" when only play video on browser. But, if I press home key, I can receive this message.

Comment 15

[sync-1]

After press power key, I can receive "visibilitychange" message in browser app when I open the tab without playing video, including pause video-playing.
 If I playing a video in browser, there is not "visibilitychange" message.
 
 What's the reason?

Comment 16

[Blake Wu [:bwu][:blakewu]]

(In reply to buri.blff from comment #12)
> I think the video should be paused after lock screen when playing it on
> browser. Such as: the video app on FFOS.
Yes. For power consumption concern, normally video playback should be stopped after locking screen. 
Besides, user cannot see the video either. It is a resource waste to play a video.

Comment 18

[Preeti Raghunath(:Preeti)]

Inder,

Please provide next steps

Comment 19

[Inder]

Preeti -- next steps needs to come from Mozilla to fix the issue.
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1019949#c27

Comment 21

[bhavana bajaj [:bajaj]]

Discussed offline with Inder, given the amount of work here its going to be hard to get this fixed in 2.0. We are still confirming this is will be fixed in 2.1 or 2.1.

Comment 23

[Juan Perez-Bedmar [:juanpbf]]

For your information, this issue was reported during certification. 
We need a fix in the branches. We understand it is late for 2.0 but please, at least, include it in 2.1 scope. 

With current behaviour, the users will waste their data tariff (and money), thats why it has some user impact.

BR

Comment 24

[Salvador de la Puente González [:salva]]

(In reply to sync-1 from comment #15)
> After press power key, I can receive "visibilitychange" message in browser
> app when I open the tab without playing video, including pause video-playing.
>  If I playing a video in browser, there is not "visibilitychange" message.
>  
>  What's the reason?

Could it be browser is running not OOP? Let's take a look.

Comment 25

[Salvador de la Puente González [:salva]]

The problem is system application never set the browser's visibility to false after pressing lock button despite lock screen is active or not. This is why you don't receive the visibility change event. But I don't know why system is not setting browser's visibility to false why playing. It seems to be intended.

Anyway, setting the browser visibility to false will force **all the streams** to be paused and other kind of services not requiring to be observed to work (as audio playback applications) will cease to play. I can prepare a patch for setting browser visibility to false but are we sure we want to stop playing **all** the streams?

Comment 26

[Salvador de la Puente González [:salva]]

Ok, confirmed. It's intended (bug 828283 [1]) to not "hide" applications playing something through the audio channel [2]. For that reason the browser app (and other applications using that channel) won't be hidden.

As it is not possible to distinguish a browser application from other, I think this is not feasible without affecting other behaviors.

With the help of Gecko, aa solution could be to hide not prevent any application to be hidden but allowing applications to keep themselves unmuted while not visible.

[1] https://github.com/mozilla-b2g/gaia/commit/bd65308e5a9fa045e9a0cc83368df0e0ab7b9c3e#diff-fd6daebc0fb827952f791e6ace77107aR1636
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/visibility_manager.js#L97

Comment 27

[Salvador de la Puente González [:salva]]

A final comment, we could solve the bug for the specific case of Gaia's browser but it won't solve the problem for YouTube app or any other application using the audio-channel.

Comment 28

[ChengLin.Wu]

(In reply to Salvador de la Puente González [:salva] from comment #25)
> The problem is system application never set the browser's visibility to
> false after pressing lock button despite lock screen is active or not. This
> is why you don't receive the visibility change event. But I don't know why
> system is not setting browser's visibility to false why playing. It seems to
> be intended.
> 
> Anyway, setting the browser visibility to false will force **all the
> streams** to be paused and other kind of services not requiring to be
> observed to work (as audio playback applications) will cease to play. I can
> prepare a patch for setting browser visibility to false but are we sure we
> want to stop playing **all** the streams?

No, The music should be playing after lock screen. But, the video should be pause.

Comment 29

[Salvador de la Puente González [:salva]]

(In reply to ChengLin.Wu from comment #28)
> (In reply to Salvador de la Puente González [:salva] from comment #25)
> > The problem is system application never set the browser's visibility to
> > false after pressing lock button despite lock screen is active or not. This
> > is why you don't receive the visibility change event. But I don't know why
> > system is not setting browser's visibility to false why playing. It seems to
> > be intended.
> > 
> > Anyway, setting the browser visibility to false will force **all the
> > streams** to be paused and other kind of services not requiring to be
> > observed to work (as audio playback applications) will cease to play. I can
> > prepare a patch for setting browser visibility to false but are we sure we
> > want to stop playing **all** the streams?
> 
> No, The music should be playing after lock screen. But, the video should be
> pause.

AFAIK, it is impossible without Gecko collaboration. Waiting for ni.

Comment 30

[ChengLin.Wu]

Yes, It's need to research gecko to fix this.  It seems that only can distinguish music and video in gecko.

Comment 31

[Tony Mechelynck [:tonymec]]

Note: There is work going on in bug 1022669 to allow the screen to sleep while audio intentionally goes on. Aren't this bug and that one antagonistic?

Comment 32

[bhavana bajaj [:bajaj]]

Eric, can you help priortize this for 2.1 given the feedback here? We atleast want a interim solution for 2.1 given the number of times we punted this.

Comment 33

[Bobby Chien]

As previous discussion, either on or off the video playing make sense to some use cases. Technical perspective, keep current implementation is less effort. However, implementing video off mechanism required efforts on core and gaia area. 

Basically, I would recommend to fix/implement this with product spec. 

Bruce, could you have some comment from product perspective.

Comment 34

[Bruce Huang [:bhuang] (inactive)]

Flagging UX for this one.  For reference, Firefox for Android seems to keep playing video in this case (while other browsers pause).

Comment 35

[Bruce Huang [:bhuang] (inactive)]

Moving ni to Francis for browser

Comment 36

[Wilfred Mathanaraj [:WDM]]

i find that putting the device into lock means i am not watching the video - so I don't want to use my data (which I pay for). We should pause the video or stop the video when the screen is locked.

Comment 37

[Francis Djabri [:djabber]]

(In reply to Wilfred Mathanaraj [:WDM] from comment #36)
> i find that putting the device into lock means i am not watching the video -
> so I don't want to use my data (which I pay for). We should pause the video
> or stop the video when the screen is locked.

One issue is that we know that people rely on YouTube A LOT to listen to music. I'd actually prefer if we keep the current behavior, as otherwise we'd force users to keep the screen on to listen to music on YouTube, which would drain their valuable battery. What do you think, Wilfred?

Comment 38

[Dietrich Ayala (:dietrich)]

Triage group decided blocking+ due to CAF priority and partner concerns. Need to at least workaround for 2.1.

Comment 39

[Bobby Chien]

Hi Ben, could you help to assign case for evaluation?

Comment 40

[Star Cheng [:scheng]]

Attachment: bug-971414-wip.patch

I give a wip patch. It suspends the video streaming while pressing power key, but doest *not* affect audio streaming.

Comment 41

[Albert [:albert]]

As Star provided a WIP patch I assign to nobody to let him to take it.

Comment 42

[Star Cheng [:scheng]]

Comment on attachment 8502306 [details] [diff] [review]
bug-971414-wip.patch


Hi, Marco 

I try to distinguish the visibility audio channel event for audio and video of "normal" channel type with "withvideo" parameter. If the withvideo is true, to use another event instead "visibility-audio-channel". Do you have any suggestion about this wip in term of Audio channel service? 

Hi, JW

Do you have any suggestion for this wip about HTMLMediaElement?

Comment 43

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8502306 [details] [diff] [review]
bug-971414-wip.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3940,5 @@
>        if (!mAudioChannelAgent) {
>          return;
>        }
>        // Use a weak ref so the audio channel agent can't leak |this|.
> +      if (AudioChannel::Normal == mAudioChannel && IsVideo() && GetVideoFrameContainer()) {

1. GetVideoFrameContainer() checks IsVideo(), so you can save IsVideo() here.
2. It would be wrong if it happens before metadata is loaded for GetVideoFrameContainer() always returns non-null if |readyState < HAVE_METADATA|.

Comment 44

[Star Cheng [:scheng]]

Attachment: bug-971414-wip.patch

Because GetVideoFrameContainer() will call IsVideo(), remove the IsVideo() check.

Comment 45

[Star Cheng [:scheng]]

Comment on attachment 8502368 [details] [diff] [review]
bug-971414-wip.patch

Hi, Marco 

I try to distinguish the visibility audio channel event for audio and video of "normal" channel type with "withvideo" parameter. If the withvideo is true, to use another event instead "visibility-audio-channel". Do you have any suggestion about this wip in term of Audio channel service?

Comment 46

[Marco Chen [:mchen]]

Comment on attachment 8502368 [details] [diff] [review]
bug-971414-wip.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3940,5 @@
>        if (!mAudioChannelAgent) {
>          return;
>        }
>        // Use a weak ref so the audio channel agent can't leak |this|.
> +      if (AudioChannel::Normal == mAudioChannel && GetVideoFrameContainer()) {

What is your rule for whether this media element is video or not?

The original code (IsVideo()) check whether this is a video or audio tag. If web page used video tag then it is a video even there is no video track on it (only audio track). Or do you want to treat this situation as none-video case?

::: dom/audiochannel/AudioChannelService.cpp
@@ +647,5 @@
>      if (obs) {
> +      if (mWithVideoChildIDs.Contains(aChildID)) {
> +        obs->NotifyObservers(nullptr, "visible-audio-channel-changed-withvideo", channelName.get());
> +      } else {
> +        obs->NotifyObservers(nullptr, "visible-audio-channel-changed", channelName.get());

How about the corresponding change on the shell.js?

And if you leverage the same topic but append video status to channelName (ex: normal-video) then you don't need to add a new anonymous function in shell.js to listen observer. How about it?

Comment 47

[Eric Chou [:ericchou] [:echou]]

Since we are having a solution for this issue, I would like to confirm that if we've already had a consensus of how we are going to use the solution for 2.1 and 2.2.

According to comment 38, I assume that CAF and partner would like to take it at least for their own 2.1 branch. So the question would become if we're going to merge the solution into Mozilla-B2g34-v2.1. Star, please remember to ni release managers after your patch got r+ so that they can decide if the patch should be picked.

As to the plan for 2.2 or later, first off, we may need our UX and developers to revisit https://wiki.mozilla.org/WebAPI/AudioChannels to make sure all of us understand what will happen for each case. Next, we've been talking about moving the whole audio channel mechanism to Gaia for a long time so that it would be easier to control the power consumption from app side. Last week Star gave a presentation of how to start working on this in Taipei's media group meeting. In that meeting, we reached a decision that Steven Lee's team will take over the implementation later on. Randy Lin should be the owner of it.

Comment 48

[Star Cheng [:scheng]]

Attachment: bug-971414-fix.patch

Hi, Marco

Could you help to review the audio channl service part?

Hi, Matthew

Could you help to review the HTMLMediaElement part?

Comment 49

[Marco Chen [:mchen]]

Comment on attachment 8503895 [details] [diff] [review]
bug-971414-fix.patch

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

I would like to defer the review of audiochannelservice to :baku because he made this solution for keeping web page playing during screen off.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3926,5 @@
>          !(mSrcStream && !(mSrcStream->GetTrackTypesAvailable() &
>                            DOMMediaStream::HINT_CONTENTS_AUDIO))) ||
>         mPlayingThroughTheAudioChannelBeforeSeek));
> +
> +  // playingThroughTheAudioChannel is ok but mReadyState is not ready

You may need to give more comments for why you need to wait mReadyState to higher then HAVE_METADATA.
ex: in order to recognize whether this media contains video track or not.

::: dom/audiochannel/AudioChannelService.cpp
@@ +645,5 @@
>      }
>  
>      if (obs) {
> +      if (mWithVideoChildIDs.Contains(aChildID)) {
> +        channelName.AppendLiteral("video");

Could we append the string like "-video"?

And I still suggest shell.js should be changed as well for splitting the combination info here to different attributes.
Without modifying shell.js, the gaia will receive channel name in mozChromeEvent of "visible-audio-channel-changed" like "normal" or "normal-video".
  1. The additional information is mixed into channel name.
  2. That might affect gaia's logic which checks for exact channel name.
So I suggest to modify shell.js and split "normal-video" to two attributes.

Comment 50

[Star Cheng [:scheng]]

Attachment: bug-971414-fix_v2.patch

1.HTMLMediaElement : to add more comments 
2.Audio Channel Service : to append the string with "-video" instead of "video"

Comment 51

[Star Cheng [:scheng]]

> ::: dom/audiochannel/AudioChannelService.cpp
> @@ +645,5 @@
> >      }
> >  
> >      if (obs) {
> > +      if (mWithVideoChildIDs.Contains(aChildID)) {
> > +        channelName.AppendLiteral("video");
> 
> Could we append the string like "-video"?
> 
> And I still suggest shell.js should be changed as well for splitting the
> combination info here to different attributes.
> Without modifying shell.js, the gaia will receive channel name in
> mozChromeEvent of "visible-audio-channel-changed" like "normal" or
> "normal-video".
>   1. The additional information is mixed into channel name.
>   2. That might affect gaia's logic which checks for exact channel name.
> So I suggest to modify shell.js and split "normal-video" to two attributes.

Hi, Alive

I try to suspend the video streaming while pressing power key. And I add new another channel name -> "normal-video" for video streaming in browser app. Could you help to modify code in order to split "normal-video" to two attributes.

Comment 52

[Star Cheng [:scheng]]

(In reply to Star Cheng [:scheng] from comment #51)
> 
> Hi, Alive
> 
> I try to suspend the video streaming while pressing power key. And I add new
> another channel name -> "normal-video" for video streaming in browser app.
> Could you help to modify code in order to split "normal-video" to two
> attributes.

The related code should be in gaia\apps\system\js\visibility_manager.js

Comment 53

[(inactive after 6/18) Alive Kuo [:alive]]

Yes, I know it well. (Next time could you try to lemme know you need my help before code freeze?)

(In reply to Star Cheng [:scheng] from comment #52)
> (In reply to Star Cheng [:scheng] from comment #51)
> > 
> > Hi, Alive
> > 
> > I try to suspend the video streaming while pressing power key. And I add new
> > another channel name -> "normal-video" for video streaming in browser app.
> > Could you help to modify code in order to split "normal-video" to two
> > attributes.
> 
> The related code should be in gaia\apps\system\js\visibility_manager.js

Comment 54

[(inactive after 6/18) Alive Kuo [:alive]]

Are we still firing 'normal' for audio channel? If so we don't need to change anything in gaia side.

Comment 55

[Marco Chen [:mchen]]

Comment on attachment 8503961 [details] [diff] [review]
bug-971414-fix_v2.patch

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

As comment 49 mentioned I defer the review to :baku who made this solution for hacking visibility.

Comment 56

[Ken Chang[:kenkai]]

According to comment 55.

Comment 57

[Andrea Marchesini [:baku]]

Comment on attachment 8503961 [details] [diff] [review]
bug-971414-fix_v2.patch

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

This patch looks good but I need a confirm that :UpdateAudioChannel* method is called again once the kind of stream is discovered.
Probably roc can help to define a better comment or to see if this workflow is fully correct.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8503961 [details] [diff] [review]
bug-971414-fix_v2.patch

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

This makes sense to me.

Comment 59

[Star Cheng [:scheng]]

Attachment: bug-971414-fix_v3.patch

Hi, roc

The GetVideoFrameContainer() go to check whether mReadyState is in HAVE_CURRENT_DATA status or not. I miss that. So I remove the check in HTMLMediaElement::UpdateAudioChannelPlayingState() function.

Comment 60

[Star Cheng [:scheng]]

Attachment: bug-971414-fix_v3.patch

Hi, roc

The GetVideoFrameContainer() go to check whether mReadyState is in HAVE_CURRENT_DATA status or not. I miss that. So I remove the check in HTMLMediaElement::UpdateAudioChannelPlayingState() function.


ps. The above bug-971414-fix_v3.patch is wrong, and update it.

Comment 61

[Star Cheng [:scheng]]

Thank you for your feedback, roc. 
Thanks for your review, baku.

Comment 62

[Star Cheng [:scheng]]

The TPBL result : https://tbpl.mozilla.org/?tree=Try&rev=1cf800225698.

Other people also encount the failure of Mnw test case of "B2G ICS Emulator Opt".

Comment 63

[Star Cheng [:scheng]]

Hi, Bhavana

According to comment 47, I provide the patch (attachment 8507812 [details] [diff] [review]) for 2.1 branch.

Comment 64

[Bobby Chien]

Comment on attachment 8507812 [details] [diff] [review]
bug-971414-fix_v3.patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 65

[Wilfred Mathanaraj [:WDM]]

(In reply to Francis Djabri [:djabber] from comment #37)
> (In reply to Wilfred Mathanaraj [:WDM] from comment #36)
> > i find that putting the device into lock means i am not watching the video -
> > so I don't want to use my data (which I pay for). We should pause the video
> > or stop the video when the screen is locked.
> 
> One issue is that we know that people rely on YouTube A LOT to listen to
> music. I'd actually prefer if we keep the current behavior, as otherwise
> we'd force users to keep the screen on to listen to music on YouTube, which
> would drain their valuable battery. What do you think, Wilfred?

Perhaps we should give an option to the user at some point to let them choose what they want to do. both sides have a good point where we dont want to keep on playing and wasting the battery something a user may not may want and at the same time user may want to listen to music while screen is locked.

Comment 66

[bhavana bajaj [:bajaj]]

(In reply to Bobby Chien [:bchien] from comment #64)
> Comment on attachment 8507812 [details] [diff] [review]
> bug-971414-fix_v3.patch
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
> [User impact] if declined:
> [Testing completed]:
> [Risk to taking this patch] (and alternatives if risky):
> [String changes made]:

Bobby for any approval request to be considered, this form needs to be filled. Just a friendly reminder :) I am NI star who has written the patch and should have the answers here to help fill this after which we can consider approval on 2.1. Also, I do not see any central landing yet so waiting for that as well to happen .

Comment 67

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #66)
> (In reply to Bobby Chien [:bchien] from comment #64)
> > Comment on attachment 8507812 [details] [diff] [review]
> > bug-971414-fix_v3.patch
> > 
> > [Approval Request Comment]
> > [Bug caused by] (feature/regressing bug #):
> > [User impact] if declined:
> > [Testing completed]:
> > [Risk to taking this patch] (and alternatives if risky):
> > [String changes made]:
> 
> Bobby for any approval request to be considered, this form needs to be
> filled. Just a friendly reminder :) I am NI star who has written the patch
> and should have the answers here to help fill this after which we can
> consider approval on 2.1. Also, I do not see any central landing yet so
> waiting for that as well to happen .

Also since this is a gecko change the correct approval flag here will be approval-mozilla-b2g34? instead of the gaia one.

Comment 68

[Star Cheng [:scheng]]

(In reply to bhavana bajaj [:bajaj] from comment #66)
> (In reply to Bobby Chien [:bchien] from comment #64)
> > Comment on attachment 8507812 [details] [diff] [review]
> > bug-971414-fix_v3.patch
> > 
> > [Approval Request Comment]
> > [Bug caused by] (feature/regressing bug #):
> > [User impact] if declined:
> > [Testing completed]:
> > [Risk to taking this patch] (and alternatives if risky):
> > [String changes made]:
> 
> Bobby for any approval request to be considered, this form needs to be
> filled. Just a friendly reminder :) I am NI star who has written the patch
> and should have the answers here to help fill this after which we can
> consider approval on 2.1. Also, I do not see any central landing yet so
> waiting for that as well to happen .

Hi, Bhavana

According to comment 47, we provide a patch for CAF and partner for their own 2.1 branch. If we plan for v2.2 or later, we need to discuss with UX and developer. So this patch is only for CAF & partner, I consider we don't need to land it into center branch. 

And do you think we need to land it to v2.1 branch? If yes, do we need get agreement form UX?

Comment 69

[bhavana bajaj [:bajaj]]

(In reply to Star Cheng [:scheng] from comment #68)
> (In reply to bhavana bajaj [:bajaj] from comment #66)
> > (In reply to Bobby Chien [:bchien] from comment #64)
> > > Comment on attachment 8507812 [details] [diff] [review]
> > > bug-971414-fix_v3.patch
> > > 
> > > [Approval Request Comment]
> > > [Bug caused by] (feature/regressing bug #):
> > > [User impact] if declined:
> > > [Testing completed]:
> > > [Risk to taking this patch] (and alternatives if risky):
> > > [String changes made]:
> > 
> > Bobby for any approval request to be considered, this form needs to be
> > filled. Just a friendly reminder :) I am NI star who has written the patch
> > and should have the answers here to help fill this after which we can
> > consider approval on 2.1. Also, I do not see any central landing yet so
> > waiting for that as well to happen .
> 
> Hi, Bhavana
> 
> According to comment 47, we provide a patch for CAF and partner for their
> own 2.1 branch. If we plan for v2.2 or later, we need to discuss with UX and
> developer. So this patch is only for CAF & partner, I consider we don't need
> to land it into center branch. 
> 
> And do you think we need to land it to v2.1 branch? If yes, do we need get
> agreement form UX?

can you help fill the form so we get a risk assessment here? Why are we hesitating to land the workaround in our 2.1 branch ? Is the workaround reviewed by UX team at all ?

" [Approval Request Comment]
> > [Bug caused by] (feature/regressing bug #):
> > [User impact] if declined:
> > [Testing completed]:
> > [Risk to taking this patch] (and alternatives if risky):
> > [String changes made]:"

Comment 70

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #69)
> (In reply to Star Cheng [:scheng] from comment #68)
> > (In reply to bhavana bajaj [:bajaj] from comment #66)
> > > (In reply to Bobby Chien [:bchien] from comment #64)
> > > > Comment on attachment 8507812 [details] [diff] [review]
> > > > bug-971414-fix_v3.patch
> > > > 
> > > > [Approval Request Comment]
> > > > [Bug caused by] (feature/regressing bug #):
> > > > [User impact] if declined:
> > > > [Testing completed]:
> > > > [Risk to taking this patch] (and alternatives if risky):
> > > > [String changes made]:
> > > 
> > > Bobby for any approval request to be considered, this form needs to be
> > > filled. Just a friendly reminder :) I am NI star who has written the patch
> > > and should have the answers here to help fill this after which we can
> > > consider approval on 2.1. Also, I do not see any central landing yet so
> > > waiting for that as well to happen .
> > 
> > Hi, Bhavana
> > 
> > According to comment 47, we provide a patch for CAF and partner for their
> > own 2.1 branch. If we plan for v2.2 or later, we need to discuss with UX and
> > developer. So this patch is only for CAF & partner, I consider we don't need
> > to land it into center branch. 
> > 
> > And do you think we need to land it to v2.1 branch? If yes, do we need get
> > agreement form UX?
> 
> can you help fill the form so we get a risk assessment here? Why are we
> hesitating to land the workaround in our 2.1 branch ? Is the workaround
> reviewed by UX team at all ?
> 
> " [Approval Request Comment]
> > > [Bug caused by] (feature/regressing bug #):
> > > [User impact] if declined:
> > > [Testing completed]:
> > > [Risk to taking this patch] (and alternatives if risky):
> > > [String changes made]:"

Also, why is it not possible to land the current workaround on master/central today so we have this consistent across all branches and replace it when we have the full solution in future by randy ?

Comment 71

[Star Cheng [:scheng]]

Hi, Jonas

Do you agree we land the patch into center branch? According to comment 32, Bhavana requested us to provide a interim solution for CAF. It's UX behavior change, and it is *NOT* a bug. What do you think?

Comment 72

[StevenLee[:slee]]

(In reply to bhavana bajaj [:bajaj] from comment #69)
> can you help fill the form so we get a risk assessment here? Why are we
> hesitating to land the workaround in our 2.1 branch ? Is the workaround
> reviewed by UX team at all ?
According to comment 32, we just provide a "interim solution for 2.1". We already have plan to refactor audiochannel. We also need to discuss with UX and gaia team about the spec and implementation. So that we don't want to put the patch into m-c.

Comment 73

[[:fabrice] Fabrice Desré]

In any case, the patch has only feeback+ and not r+ from roc. Please nominate once the situation is clear.

Comment 74

[Star Cheng [:scheng]]

(In reply to Fabrice Desré [:fabrice] from comment #73)
> In any case, the patch has only feeback+ and not r+ from roc. Please
> nominate once the situation is clear.

Hi, Fabrice

In the Comment 57, the patch had r+ from baku. Does this patch also need roc's r+ ? If yes, it will do it.

Comment 75

[Star Cheng [:scheng]]

(In reply to StevenLee[:slee] from comment #72)
> (In reply to bhavana bajaj [:bajaj] from comment #69)
> > can you help fill the form so we get a risk assessment here? Why are we
> > hesitating to land the workaround in our 2.1 branch ? Is the workaround
> > reviewed by UX team at all ?
> According to comment 32, we just provide a "interim solution for 2.1". We
> already have plan to refactor audiochannel. We also need to discuss with UX
> and gaia team about the spec and implementation. So that we don't want to
> put the patch into m-c.

Hi, bhavana

Any feedback? Thanks.

Comment 76

[[:fabrice] Fabrice Desré]

(In reply to Star Cheng [:scheng] from comment #74)
> (In reply to Fabrice Desré [:fabrice] from comment #73)
> > In any case, the patch has only feeback+ and not r+ from roc. Please
> > nominate once the situation is clear.
> 
> Hi, Fabrice
> 
> In the Comment 57, the patch had r+ from baku. Does this patch also need
> roc's r+ ? If yes, it will do it.

Alright, thanks.

Comment 77

[Ken Chang[:kenkai]]

Bhavana, according to comment 72, this fix is an interim solution and CAF make this bug as priority 2. Can we discuss with CAF to remove this bug from CAF list.

Comment 78

[bhavana bajaj [:bajaj]]

(In reply to Ken Chang[:ken] from comment #77)
> Bhavana, according to comment 72, this fix is an interim solution and CAF
> make this bug as priority 2. Can we discuss with CAF to remove this bug from
> CAF list.

Yes, I spoke offline with CAF and given the option in hand, Inder from CAF was fine for this one to ride the trains and get fixed in 2.2

Comment 79

[Bobby Chien]

Move to next release.

Additional information:
- Video App will suspend video playback if power button off the screen. 

-----
Gaia-Rev        c2c55870de22d596de4f41612c0b44090f90ebad
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0b81c10a9074
Build-ID        20141102160202
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  39
FW-Date         Thu Oct 16 18:19:14 CST 2014
Bootloader      L1TC00011880

Comment 80

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Bobby,
Is this being actively worked on?
Just want to confirm if the blocking-b2g status still valid.

Comment 81

[Bobby Chien]

Since new AudioChannel design is in-progress, dev team decide to correct this behavior once gaia adopt new AudioChannel. ETA is after v2.2.

Comment 82

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, this is a long bug so I haven't read though it all, but it seems to cover many different topics.

My understanding was that it was that the following is desired behavior:

1. User visits YouTube.
2. User starts playing a music video.
3. User press power button to turn off the screen.
4. User is still able to listen to music.

I.e. that we want to enable using YouTube as a music player and that music players should keep playing in the background.

We've had this debate a million times and this is so far the conclusion of what behavior that we want.


Or is this bug about something else? Such as stuttery audio or the fact that we don't set .visibility to false? If so, please let me know what problem we are trying to solve here and flag me for needinfo again.

Comment 83

[Beatriz Rodríguez [:brg]]

The desired behaviour here is to pause the video player when the screen is off.

Comment 84

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That doesn't seem to match Telefonica's previous requests that YouTube should keep playing when the power button is pushed?

Is that no longer desired? Or is the request here to have different behavior on YouTube and on http://imps.tcl-ta.com?

Comment 85

[Beatriz Rodríguez [:brg]]

(In reply to Jonas Sicking (:sicking) from comment #84)
> That doesn't seem to match Telefonica's previous requests that YouTube
> should keep playing when the power button is pushed?
Could you please explain where TEF requirement said that YT should play with the screen off? It could be a mistake. 
> Is that no longer desired? Or is the request here to have different behavior
> on YouTube and on http://imps.tcl-ta.com?
The desired behaviour here is to pause the video player when the screen is off (TEF reason is to avoid overcharging the user).

Comment 86

[Andrea Marchesini [:baku]]

I'm removing my NI. Soon the AudioChannel policy will be managed by the system app.
We can re-think what we want for the visibility of normal-audiochannel websistes.

Comment 87

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Beatriz Rodríguez [:brg] from comment #85)
> Could you please explain where TEF requirement said that YT should play with
> the screen off? It could be a mistake. 

This was discussed in many meetings and bugs. I remember that it was a hotly debated topic at the Berlin work-week.

I think much of the implementation, and some of the discussion happened in bug 828283

Comment 88

[Beatriz Rodríguez [:brg]]

(In reply to Jonas Sicking (:sicking) from comment #87)
> (In reply to Beatriz Rodríguez [:brg] from comment #85)
> > Could you please explain where TEF requirement said that YT should play with
> > the screen off? It could be a mistake. 
> 
> This was discussed in many meetings and bugs. I remember that it was a hotly
> debated topic at the Berlin work-week.
> 
> I think much of the implementation, and some of the discussion happened in
> bug 828283

Bug 828283 is about sound players (music) while this bug is about video player. TEF requirement is to pause the video player when the screen is off.

IIRC, music and video behaviour was not different in the past(gecko 18 or so) thats why same behaviour was implemented for both kind of apps however if video app and music app are used in different way by customers, they can have different sound/sleep policies.

Comment 89

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I thought that YouTube was something that we specifically wanted to support since people commonly use it as a music player?

But we could definitely treat audio and video different. Though that will need some platform work first.

Comment 90

[Blake Wu [:bwu][:blakewu]]

(In reply to Jonas Sicking (:sicking) from comment #89)
> I thought that YouTube was something that we specifically wanted to support
> since people commonly use it as a music player?
IMO, more people use it to watch videos only (not very sure). So those people expect it would be stopped when screen is off. Besides,  the behavior of youtube on iphone and android phones are also to stop video playing when screen off.

Comment 91

[BMO Automation]

Firefox OS is not being worked on