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

RESOLVED WORKSFORME

Status

defect
RESOLVED WORKSFORME
5 years ago
3 years ago

People

(Reporter: justindarc, Assigned: justindarc)

Tracking

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 affected)

Details

Attachments

(2 attachments)

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
(Assignee)

Comment 1

5 years ago
[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.
Blocks: 1067205
blocking-b2g: --- → 2.1?
Flags: needinfo?(timdream)
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).
Flags: needinfo?(timdream)

Comment 3

5 years ago
[Blocking Requested - why for this release]:

Triage: regression, this happens in 2.2 master, not 2.1. blocking as 2.2+.
Assignee: nobody → alive
blocking-b2g: 2.1? → 2.2+
(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.
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...
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 6

5 years ago
(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/
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 7

5 years ago
Tim: Here's a list of the commits to mozilla-central between the two builds in question.
Flags: needinfo?(timdream)
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)
Assignee: alive → jdarcangelo
Component: Gaia::System::Window Mgmt → Gaia::Camera
Flags: needinfo?(timdream)
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
Attachment #8494750 - Flags: review?(dmarcos)
Flags: needinfo?(mhabicher)
(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.
Flags: needinfo?(mhabicher) → needinfo?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Updated

5 years ago
See Also: → 1058452
(Assignee)

Updated

5 years ago
Depends on: 1015073
(Assignee)

Comment 13

5 years ago
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.
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?
Flags: needinfo?(timdream)
(Assignee)

Comment 15

5 years ago
Changing the NI? to sotaro since after speaking with timdream this week, this is *NOT* a Window Mgmt bug.
Flags: needinfo?(timdream) → needinfo?(sotaro.ikeda.g)
Depends on: 1069109
Flags: needinfo?(sotaro.ikeda.g)
(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.

Updated

5 years ago
Target Milestone: --- → 2.1 S6 (10oct)
(Assignee)

Comment 17

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
(Assignee)

Updated

3 years ago
Attachment #8494750 - Flags: review?(dmarcos)
You need to log in before you can comment on or make changes to this bug.