1071198 - [Camera] Camera is not receiving 'visibilitychange' event when 'lock' button is pressed

Status: RESOLVED WORKSFORME

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

Description

[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

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.

This bug was discovered while investigating Bug 1067205

Comment 1

[Justin D'Arcangelo [:justindarc]]

[Blocking Requested - why for this release]:

Blocking Bug 1067205 which is a 2.1+ blocker.

Setting NI? for timdream to help identify what may have caused this regression.

Comment 2

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

Bug 1067205 is already fixed. 

Can you see if this is essentially the same bug as bug 1015073? We currently do not change visibility if it is has audio tag with "normal" audio channel. I quickly grep'd the code and I only found an "notification" channel audio tag.

If bug 1015073 is not the cause we need to look into System app (please needinfo Alive).

Comment 3

[howie [:howie]]

[Blocking Requested - why for this release]:

Triage: regression, this happens in 2.2 master, not 2.1. blocking as 2.2+.

Comment 4

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

(In reply to Tim Guan-tin Chien [:timdream] (OOO) (MoCo-TPE) (please ni?) from comment #2)
> Bug 1067205 is already fixed. 
> 
> Can you see if this is essentially the same bug as bug 1015073? We currently
> do not change visibility if it is has audio tag with "normal" audio channel.
> I quickly grep'd the code and I only found an "notification" channel audio
> tag.
> 
> If bug 1015073 is not the cause we need to look into System app (please
> needinfo Alive).

This comment is incorrect, since bug 1015073 happens since 1.1 but according to comment 0 this happens around Sep. 9.

Comment 5

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

Justin,

The Gaia commits listed in comment 0 are both 8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7. How did you find it? If the Gaia commits is indeed the same between two builds we are looking at Gecko regression here...

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Tim Guan-tin Chien [:timdream] (OOO) (MoCo-TPE) (please ni?) from comment #5)
> Justin,
> 
> The Gaia commits listed in comment 0 are both
> 8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7. How did you find it? If the Gaia
> commits is indeed the same between two builds we are looking at Gecko
> regression here...

Tim,
If the Camera's flash is "On" or "Auto" and you are in a dimly-lit area, you'll notice that the flash on the back of the device will periodically turn on and off as the camera auto-focuses. When the Camera app receives the `visibilitychange` event, we check `document.hidden`. If we're hidden, we shutdown and release the camera hardware which would stop auto-focusing and the flash should stop turning on and off. Therefore, when the user presses the "lock" button, the flash LED should stop blinking until the device is unlocked again.

To identify which build we regressed, I worked backwards through the PVT builds here until I identified one where it worked as expected:

https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-central-flame-eng/2014/09/

Comment 7

[Justin D'Arcangelo [:justindarc]]

Attachment: mozilla-central diff (6b3948d3413a:152ef25e89ae)

Tim: Here's a list of the commits to mozilla-central between the two builds in question.

Comment 8

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

Thank for posting the log. Window mgmt can't help at this point.

Per conversation on irc, Justin will confirm if this is a mozCamera API regression and put the bug to DOM: Device interface accordingly if confirmed.

(Temporary put the bug to Gaia::Camera and assign to Justin)

Comment 9

[Justin D'Arcangelo [:justindarc]]

It appears as though the only Camera-specific Gecko patch that landed within the range is Bug 1058452. Will first investigate if this is a MozCamera regression.

Comment 10

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Diego,
This patch isn't going to make any sense. However, I've recently become aware that the `visibilitychange` event is dependent upon audio channels. By default, the <video> tag uses mozaudiochannel="normal". But, to ensure we receive the event, we need to change it to "content" since we have the "audio-channel-content" permission in the manifest.webapp.

Anyhow, this patch fixes this bug. I still do not know why we do not have this issue on other branches (only master/v2.2 is affected). Flagging mikeh for info to see if Bug 1058452 would have any effect on this since it landed in the regression window.

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #10)

> Flagging mikeh for info to see if Bug 1058452 would have any effect
> on this since it landed in the regression window.

I doubt this. Bug 1058452 just provides a clean-up for some class plumbing, but I've ni?-ed sotaro just in case.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #11)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #10)
> 
> > Flagging mikeh for info to see if Bug 1058452 would have any effect
> > on this since it landed in the regression window.
> 
> I doubt this. Bug 1058452 just provides a clean-up for some class plumbing,
> but I've ni?-ed sotaro just in case.

I did not know "`visibilitychange` event is dependent upon audio channels". Then Bug 1058452 affect to the bug. Until Bug 1058452 fix, camera preview's MediaStream event is not delivered to nsHTMLMediaElement. Then Bug 1058452 fixed the event delivery problem.

Comment 13

[Justin D'Arcangelo [:justindarc]]

Making this blocked by Bug 1015073 -- Although I have a patch here that works around the issue, we need to fix the root cause which is Bug 1015073.

Comment 14

[Diego Marcos [:dmarcos]]

Justin's patch looks good to me but it's a hack. I would prefer not to land it. I would like to see the root cause fixed: Visibility change and audio channel are coupled (bug 1015073). What's the ETA?

Comment 15

[Justin D'Arcangelo [:justindarc]]

Changing the NI? to sotaro since after speaking with timdream this week, this is *NOT* a Window Mgmt bug.

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #15)
> Changing the NI? to sotaro since after speaking with timdream this week,
> this is *NOT* a Window Mgmt bug.

If bug 1069109 is fixed, this bug is also fixed. How to solve it at HTMLMediaElement is already clear. The remaining thing is that which change is acceptable for a reviewer.

Comment 17

[Justin D'Arcangelo [:justindarc]]

This appears to now be fixed on master as a result of the patch in Bug 1069109 landing as Sotaro mentioned in Comment 16. Closing.