1067205 - [Camera] Preview gallery video playback doesn't pause when device is locked

Status: VERIFIED FIXED

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

Description

[hyuna.cho]

Repros.
1) record video content in Camera app
2) click thumbnail image
3) play video file
4) press power key
5) unlock lock screen
Observe video file.

video content is playing after unlock lock screen.
In Gallery app, video file is paused.

Comment 1

[hyuna.cho]

Hi, Diego and Wilson

At this case, video player can't receive 'visibilitychange' event.

I checked it on nexus4 and it's reproduced. I didn't check on flame device.
Plz check it.

Comment 2

[Youngwoo Jo]

I checked camera app manifest. It does not have "audio-

Comment 3

[Youngwoo Jo]

Oh, I'm sorry for the incomplete comments.
It's uploaded while writing my comment.
:-(

Camera app manifest does not have "audio-channel-content" permission.
So it looks like that only "normal" audio channel type is being transferred to gecko-side.
I think it makes the problem.

And I verified that it works fine when "audio-channel-content" permission is added to camera app manifest.

Comment 4

[Wilson Page [:wilsonpage]]

(In reply to hyuna.cho from comment #1)
> Hi, Diego and Wilson
> 
> At this case, video player can't receive 'visibilitychange' event.
> 
> I checked it on nexus4 and it's reproduced. I didn't check on flame device.
> Plz check it.

I just checked and it seems we are receiving no 'visibilitychange' event when the phone is locked. This is a serious issue, as it means that the camera will continue to run in the background when the phone is locked, which will drain the battery very quickly!

Comment 5

[Justin D'Arcangelo [:justindarc]]

(In reply to Wilson Page [:wilsonpage] from comment #4)
> I just checked and it seems we are receiving no 'visibilitychange' event
> when the phone is locked. This is a serious issue, as it means that the
> camera will continue to run in the background when the phone is locked,
> which will drain the battery very quickly!

If that's the case, it sounds like this could be the same root cause as Bug 1066015.

Comment 6

[Wilson Page [:wilsonpage]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #5)
> (In reply to Wilson Page [:wilsonpage] from comment #4)
> If that's the case, it sounds like this could be the same root cause as Bug
> 1066015.

I'm sure we used to have code in camera that dealt with lockscreen events. I can't see them anymore. I know dmarcos did some work with them.

Comment 7

[Wilson Page [:wilsonpage]]

kgrandon: Do you know why we wouldn't be receiving 'visibilitychange' events when the device is locked. Should we be? If not, how can we detect when the device is locked whilst our app is open?

Comment 8

[Wilson Page [:wilsonpage]]

Confirmed in v2.0 we *are* receiving the 'visibilitychange' event once the power button is pressed.

Comment 9

[Wilson Page [:wilsonpage]]

It appears the regression was introduced somewhere between v2.0 - v2.1.

Comment 10

[Wilson Page [:wilsonpage]]

[Blocking Requested - why for this release]: Prevents camera hardware from being released, meaning camera continues to run the background, and battery will deplete rapidly.

Comment 11

[Wilson Page [:wilsonpage]]

This bug only affects camera app; I can confirm that Music app *does* receive the expected events. This suggests some operation Camera is running must be preventing the 'visibilitychange' events from firing.

I believe this to be a Gecko regression as flashing v2.0 Gaia (working with v2.0 Gecko) on latest Gecko still exhibits the bug.

Comment 12

[Wilson Page [:wilsonpage]]

UPDATED STEPS:

1. Open Camera
2. Switch to 'video' mode
3. Record a ~10sec video
4. Stop recording
5. Tap the thumbnail in the bottom right
6. Play the video
7. Press power button to lock device
8. Unlock device

EXPECTED: Preview gallery should have closed
ACTUAL: Preview gallery is open and video is still playing

---

The consequences of this bug are a lot more severe than symptom outlined above. But these steps is the only way I could think of to visually detect the regression.

Comment 13

[Wilson Page [:wilsonpage]]

FWIW we *are* getting the 'visibilitychange' event if the device is locked during video recording.

Comment 14

[Hema Koka [:hema]]

Blocking Reason: regression based on comments 9 and 11

Comment 15

[Kevin Grandon :kgrandon]

Updating flags to reflect currently known branch state.

Comment 16

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

Can we get branch checks and a regression window here?

Comment 17

[hyuna.cho]

(In reply to Youngwoo Jo from comment #3)
> Oh, I'm sorry for the incomplete comments.
> It's uploaded while writing my comment.
> :-(
> 
> Camera app manifest does not have "audio-channel-content" permission.
> So it looks like that only "normal" audio channel type is being transferred
> to gecko-side.
> I think it makes the problem.
> 
> And I verified that it works fine when "audio-channel-content" permission is
> added to camera app manifest.

Hi, Wilson

If we add "audio-channel-content" in manifest.webapp, it's not reproduced.
Our gecko/audio engineers said, Audio object created as 'normal' but changed to ' notification' due to the shutter sound.

plz check it.

Thanks
Hyuna

Comment 18

[Wilson Page [:wilsonpage]]

(In reply to hyuna.cho from comment #17)
> Hi, Wilson
> 
> If we add "audio-channel-content" in manifest.webapp, it's not reproduced.
> Our gecko/audio engineers said, Audio object created as 'normal' but changed
> to ' notification' due to the shutter sound.
> 
> plz check it.
> 
> Thanks
> Hyuna

I'm confused what audio-channel config has to do with the firing or 'visibilitychange'...?

Comment 19

[hyuna.cho]

Gecko engineer said, if audio channel is normal, do change he app to hidden like music.
I will request our gecko engineer to explain more information.

To. Youngwoo
Could you explain relationship between visibility and audio channel.

Comment 20

[Youngwoo Jo]

http://lxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#482
=> if shell.visibleNormalAudioActive is set, it does not set visibility to false('hidden').
That is, it does not change the current visibility state to 'hidden'.
As a result, it does not trigger 'visibilitychange' event, because old value and new value are same as 'shown' state.

http://lxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1025
=> shell.visibleNormalAudioActive is set to true, if visible-audio-channel is 'normal'.

This means that, if 'normal' audio channel is set, 'visibilitychange' event is not triggered.

So we have to check why 'normal' channel is set in the camera-preview-play scene.

Comment 21

[Youngwoo Jo]

Camera app preview player is setting audio channel to 'content'.
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/media/video_player.js#L56
However, camera app does not have 'audio-channel-content' permission.

So I expect that gecko changes the audio channel of the video element to the default value('normal').

Anyway, camera app is using 'content' audio channel, but it does not have the permission.
So we have to add 'audio-channel-content' permission to camera app manifest.

Comment 22

[Justin D'Arcangelo [:justindarc]]

This seems like a serious mix of concerns. Audio channels shouldn't be affecting page visibility events. I can't think of any cases where this would be useful. If an app wanted to be able to continue playing in the background (I'm guessing that's what the intention is with this mix-up), then the app could simply ignore the visibility events.

Comment 23

[Adam Alldredge [:AdamA][Inactive]]

This issue is occurring on 2.2 Flame, 2.2 Open_C, 2.1 Flame, 2.0 Flame, and 2.0 Flame Base.

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20140918150023
Gaia: d37950eb09e28aa18d0e01df9ff90574bd4337e0
Gecko: d6f78faaefcc
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Device: Open_C 2.2 Master
BuildID: 20140919052449
Gaia: d170091ba1b5597b05f37fb259f6b8eb02568798
Gecko: 3475e6a1665a
Version: 35.0a1 (2.2 Master)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Device: Flame 2.1
BuildID: 20140918145021
Gaia: 9f3588e8fdd37f6f354b5518dbeab7299b118a9d
Gecko: 6f26d4fce19c
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0
BuildID: 20140918082321
Gaia: 31434a3949556171f3565ca47ac2b44e810e95e6
Gecko: 5cf783171d5c
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.0
BuildID: 20140904160718 (Base)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Result:
Video plays while device is locked.

Comment 24

[Wilson Page [:wilsonpage]]

(In reply to Youngwoo Jo from comment #20)
> http://lxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#482
> => if shell.visibleNormalAudioActive is set, it does not set visibility to
> false('hidden').
> That is, it does not change the current visibility state to 'hidden'.
> As a result, it does not trigger 'visibilitychange' event, because old value
> and new value are same as 'shown' state.
> 
> http://lxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.
> js#1025
> => shell.visibleNormalAudioActive is set to true, if visible-audio-channel
> is 'normal'.
> 
> This means that, if 'normal' audio channel is set, 'visibilitychange' event
> is not triggered.
> 
> So we have to check why 'normal' channel is set in the camera-preview-play
> scene.

I agree completely with Justin, 'visibilitychange' should always fire when the app is no longer visible on the screen aside from what audio-channel is in use.

Youngwoo: Can you please advise a solution and arrange the necessary Gaia/Gecko patches to fix?

Comment 25

[Joshua Mitchell (Inactive)]

Just to be sure - I checked the EARLIEST 2.0 we had as well and this bug repro'd there - this and the prior comments' branch check indicate that this in not a regression

striking regression window wanted flag.



Device: Flame 2.0
BuildID: 20140904114640
Gaia: 13978cf2230652274969536322378d448fd142a4
Gecko: 2537ab191112
Version: 32.0 (2.0)
Firmware: 
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 26

[Youngwoo Jo]

I agree the coupling of "visibilitychange" event and audio channel is not a good design.
But the cause of this issue is from the wrong camera app permission.
So now Hyuna is attaching the patch.

To Hyuna,
Can you upload your patch?

And I'm not a proper person to comment about modifying the coupling issue,
because I don't know history of the audio channel and the visibility issues and I'm not a person of medai/audio/video part.

Comment 27

[hyuna.cho]

Attachment: PR-1067205.html

Hi, Wilson

We can fix it if we add permissions for content audio channel in camera app.
Plz check it.

Thanks

Comment 28

[Justin D'Arcangelo [:justindarc]]

(In reply to hyuna.cho from comment #27)
> Created attachment 8492959 [details]
> PR-1067205.html
> 
> Hi, Wilson
> 
> We can fix it if we add permissions for content audio channel in camera app.
> Plz check it.
> 
> Thanks

This patch is not fixing the issue for me on Flame. I still am not receiving the `visibilitychange` event with the patch applied.

Comment 29

[Diego Marcos [:dmarcos]]

Tim, we need visibilitychange to be always fired when the app is not visible. It should not depend on the audio channel permissions. Is anybody looking into this or any other bug tracking the issue?

Comment 30

[Justin D'Arcangelo [:justindarc]]

On Flame (JB v123), the `visibilitychange` event is not firing on master (v2.2) when the lock-button is pressed. All other branches appear to be unaffected. Here's the regression window I've identified:

Last Working:
Gaia-Rev        8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/6b3948d3413a
Build-ID        20140909160207
Version         35.0a1

First Broken:
Gaia-Rev        8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/152ef25e89ae
Build-ID        20140910040203
Version         35.0a1

NOTE: I've noticed that Comment 0 appears to be referring to an issue regarding the playback of recorded video content. However, the title of this bug is simply referring to the absence of the `visibilitychange` event when pressing the 'lock' button. So, I'm not sure if these are two separate issues or not. My concern is with the missing `visibilitychange` events because this has serious negative consequences with the Camera app. The easiest way to tell if we're getting the `visibilitychange` events is to have the Flash set to "Auto" or "On" and point the camera in a dim/dark area which will cause the auto-focus to continuously trigger the flash to turn on. When you press the 'lock' button, the camera should stop focusing and you should no longer see the flash turning on and off. In Build 20140910040203 and onward, the camera continues to try and focus and trigger the flash even after the 'lock' button is pressed.

Comment 31

[Justin D'Arcangelo [:justindarc]]

Re-naming this bug to clarify. There are two separate issues here.

Comment 32

[Justin D'Arcangelo [:justindarc]]

Moving discussion specifically surrounding the absence of the `visibilitychange` event upon pressing the 'lock' button to Bug 1071198. This bug will continue to track the specific issue in Comment 0 regarding the preview gallery video playback.

Comment 33

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8492959 [details]
PR-1067205.html

Taking this review from Wilson :-)

I'll check and see if this permission change fixes the specific issue from Comment 0 on the v2.1 branch. If it does, we can land and uplift this change to v2.1.

Comment 34

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8492959 [details]
PR-1067205.html

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): n/a
[User impact] if declined: Video playback within the Camera's preview gallery would continue playing after the 'lock' button is pressed.
[Testing completed]: Tested on Flame (JB-v123)
[Risk to taking this patch] (and alternatives if risky): none; just adds a permission for 'audio-channel-content'
[String changes made]: none

Hyuna,
This definitely causes video playback in the Camera preview gallery to stop when the 'lock' button is pressed. Thank you for your patch!

Tim,
Can you explain why the `visibilitychange` event is conditional depending on the type of audio-channel? It seems that something should be done to address the separation of these concerns.

Comment 35

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/3c898380b47f298cd3b7a0dacb3a6529e94322d4

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Diego Marcos [:dmarcos] from comment #29)
> Tim, we need visibilitychange to be always fired when the app is not
> visible. It should not depend on the audio channel permissions. Is anybody
> looking into this or any other bug tracking the issue?

Diego, the bug to look is bug 1015073, specifically the last comment. Again, unfortunately, this is nothing Gaia System can do at this point. I plan to make sure we have proper Gecko help address this soon.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/acebdf90a836c27ab7a280381e9e0a4966205e9d

Comment 38

[Roland Kunkel [:RolandK]]

This issue is verified fixed on the latest 2.1 and 2.2 Flame builds:

Environmental Variables:
----------------------------------------
Device: Flame 2.1
BuildID: 20141012001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
----------------------------------------
Device: Flame 2.2 Master
BuildID: 20141012040203
Gaia: 717ad4e8b7fc10ab8248500d00ba5ba0977fa8ab
Gecko: 44168a7af20d
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

The video is paused when user presses the power button to lock device and replays at correct location when resumed.