Closed Bug 1087475 Opened 5 years ago Closed 5 years ago

[Flame][Camera]The viewfinder will fail to function when the user opens and closes the camera app multiple times via the home button

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 wontfix, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S8 (7Nov)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: njpark, Assigned: dmarcos)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1080912 +++

The fix to Bug 1080912 uncovered a different bug.

Repro Steps:
1) Update a Flame device to BuildID: 20141008000201
2) Open the camera app and quickly close it via the home button mutliple times.
3) Open the camera app for the third or fourth time.
4) Press the picture and or the video button.
5) Notice the viewfinder will show a spinning circle and fails to function.
  
Actual:
The picture and video buttons will fail to function when the user quickly opens and closes the camera app via the home button.
  
Expected: 
The picture and video buttons always function.

Repro frequency: 80%
This is both reproducible in 2.1 and 2.2
Summary: [Camera]The camera button and video button will fail to function when the user opens and closes the camera app multiple times via the home button → [Camera]The viewfinder will fail to function when the user opens and closes the camera app multiple times via the home button
Whiteboard: [2.1-flame-test-run-3]
Attached file camera.log
logcat during the process
[Blocking Requested - why for this release]:
Same reason as Bug 1080912.  The camera app becomes unusable since the viewfinder does not become active.
blocking-b2g: --- → 2.1?
This seems like a base image issue.  This bug is reproducible only on v188.  on v184, this is not reproducible.

Changing the component to vendcom
Component: Gaia::Camera → Vendcom
wesly, francis, can you help communicate this to T2M?  also ni? mike lien to check again on the v188 base to confirm.
Flags: needinfo?(wehuang)
Flags: needinfo?(mlien)
Flags: needinfo?(frlee)
Qawanted on v188 only to see if it repros.
Keywords: qawanted
I flashed v184, then did a full flash of 2.1, and this problem did not appear, while v188 with a full flash of 2.1 showed this issue.  I check that base image versions weren't changed.

This is also reproducible right after flashing v188 base image.
Keywords: qawanted
triaged to block 2.0+.  ni? bhavana to follow up with vendor
blocking-b2g: 2.1? → 2.0+
Flags: needinfo?(bbajaj)
Whiteboard: [NPOTB]
Summary: [Camera]The viewfinder will fail to function when the user opens and closes the camera app multiple times via the home button → [Flame][Camera]The viewfinder will fail to function when the user opens and closes the camera app multiple times via the home button
(In reply to Tony Chung [:tchung] from comment #7)
> triaged to block 2.0+.  ni? bhavana to follow up with vendor

wesly, is the right person to escalate this, lets wait for him to come back here once he has touch-based with T2M.
Flags: needinfo?(bbajaj)
verify with v180, v184, and v188 base images from partner
All base images have this problem

Clarify the easiest way to reproduce it:
1. Launch Camera app -> Press home button before viewfinder appearing to back to homescreen
2. Repeat step 1 three times
3. Launch Camera app again to check viewfinder
4. Kill camera app process
5. Repeat step 1~step4

Follow above STR, the reproduce rate is 100%
Flags: needinfo?(mlien)
Is this line obsolete?

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/camera/camera.js#L449

Introduced by bug 1008039 to prevent a black preview when button bashing the switch button

I suspect that it could be preventing the ready event to be emitted when open/closing the camera rapidly
Flags: needinfo?(wilsonpage)
(In reply to Mike Lien[:mlien] from comment #9)
> verify with v180, v184, and v188 base images from partner
> All base images have this problem
> 
> Clarify the easiest way to reproduce it:
> 1. Launch Camera app -> Press home button before viewfinder appearing to
> back to homescreen
> 2. Repeat step 1 three times
> 3. Launch Camera app again to check viewfinder
> 4. Kill camera app process
> 5. Repeat step 1~step4
> 
> Follow above STR, the reproduce rate is 100%

What do you mean by killing the app process? Long press home button and close?
Flags: needinfo?(mlien)
(In reply to Diego Marcos [:dmarcos] from comment #11)
> (In reply to Mike Lien[:mlien] from comment #9)
> > verify with v180, v184, and v188 base images from partner
> > All base images have this problem
> > 
> > Clarify the easiest way to reproduce it:
> > 1. Launch Camera app -> Press home button before viewfinder appearing to
> > back to homescreen
> > 2. Repeat step 1 three times
> > 3. Launch Camera app again to check viewfinder
> > 4. Kill camera app process
> > 5. Repeat step 1~step4
> > 
> > Follow above STR, the reproduce rate is 100%
> 
> What do you mean by killing the app process? Long press home button and
> close?

yes, step 4's purpose is let next iteration to start with cold launch
Flags: needinfo?(mlien)
I may lost some information from previous comments

Update STR
1. Launch Camera app -> Press home button before viewfinder appearing to back to homescreen
2. Repeat step 1 three times
3. Launch Camera app again to check viewfinder
    -> base image will get black viewfinder and switch picture/video mode button cannot function
    -> after shallow flash on top of base image can see the viewfinder but switch picture/video mode button cannot function

(If cannot reproduce it you can follow step 4 and step 5)
4. Kill camera app process
5. Repeat step 1~step4

Also, all base images with latest v2.1 gaia/gecko have this problem
(In reply to Diego Marcos [:dmarcos] from comment #10)
> Is this line obsolete?
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/lib/camera/
> camera.js#L449
> 
> Introduced by bug 1008039 to prevent a black preview when button bashing the
> switch button
> 
> I suspect that it could be preventing the ready event to be emitted when
> open/closing the camera rapidly

That line is there to make sure in the time period between camera configuration start->complete, the camera wasn't released. TBH the success callback *shouldn't* fire if the camera was released, but I believe in some cases it does. If so, this should be solved on the Gecko side.
Flags: needinfo?(wilsonpage)
Hi Youlong, pls help check this issue, I've added it into our tracking list to follow with other issues together. Thanks.
Flags: needinfo?(youlong.jiang)
Flags: needinfo?(wehuang)
Flags: needinfo?(frlee)
Btw Youlong since this is reported after v180 pls also check if this is related to something changed while migrating to CS release.
(In reply to Mike Lien[:mlien] from comment #13)
> I may lost some information from previous comments
> 
> Update STR
> 1. Launch Camera app -> Press home button before viewfinder appearing to
> back to homescreen
> 2. Repeat step 1 three times
> 3. Launch Camera app again to check viewfinder
>     -> base image will get black viewfinder and switch picture/video mode
> button cannot function
>     -> after shallow flash on top of base image can see the viewfinder but
> switch picture/video mode button cannot function
> 
> (If cannot reproduce it you can follow step 4 and step 5)
> 4. Kill camera app process
> 5. Repeat step 1~step4
> 
> Also, all base images with latest v2.1 gaia/gecko have this problem

Can you also reproduce on master?
Flags: needinfo?(mlien)
Duplicate of this bug: 1087484
What is supposed to happen if we call mozCamera.release before mozCamera.setConfiguration has finished? Is the success callback called or the error one? I think none of them should be called. Release should cancel any pending operation on camera.
Flags: needinfo?(mhabicher)
Flags: needinfo?(aosmond)
follow STR, master will stuck on camera app initial status, there has a circle rotating and cannot function everything
Gaia-Rev        f46d56d812480bff7f3b35e8cacbedfa4d49edc5
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/d8de0d7e52e0
Build-ID        20141023160203
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
Flags: needinfo?(mlien)
(In reply to Diego Marcos [:dmarcos] from comment #19)
> What is supposed to happen if we call mozCamera.release before
> mozCamera.setConfiguration has finished? Is the success callback called or
> the error one? I think none of them should be called. Release should cancel
> any pending operation on camera.

We don't define the behaviour for this scenario (release interrupting pending operations) so it would be update to the driver. We should change this and I will make a patch for whatever we agree on.

There is a related scenario; for example, if DOMCameraControl::Shutdown is called due to a window switch, then it drops all success/error callbacks, drops any persistent state callbacks, rejects any pending promises and requests the driver to shutdown. I believe you would still receive a close event (not the callback) but I will need to confirm.

While with the callbacks we have flexibility, I think the promises should be rejected if they cannot be fulfilled (albeit perhaps with an error code specific to this case). That would be consistent with their definition and feedback from our DOM peer when I added support for them in the first place.
Flags: needinfo?(youlong.jiang)
Flags: needinfo?(aosmond)
Err cleared wrong ni ;).
Flags: needinfo?(mhabicher) → needinfo?(youlong.jiang)
I'm fine with release() clearing any pending callbacks, but if any of them have their 'onError' versions pending, those should be fired with a suitable error (e.g. "HardwareClosed").
Yeah let's do that. How long would it take to have a patch to cancel any previous hardware requests if release is invoked? Thanks guys
Flags: needinfo?(mhabicher)
Flags: needinfo?(aosmond)
(In reply to Diego Marcos [:dmarcos] from comment #24)
> Yeah let's do that. How long would it take to have a patch to cancel any
> previous hardware requests if release is invoked? Thanks guys

Not very long. I'll have a patch up in the next hour or two.
Flags: needinfo?(mhabicher)
Flags: needinfo?(aosmond)
This seems to resolve the issue for me; please let me know if that isn't the case for anyone. I haven't seen anything in my testing that would suggest this is a vendor issue, because we never received the release call for camera from the app. Without the release call, we never triggered the release success callback, which would have fixed the application state. The reason it never called release seems to be that focus.stopFaceDetection threw an exception and stopped execution (I guess those are not shown to the user?).

This should be reproducible on v184. The most important part when reproducing is that the getCamera call needs to have finished before you open the app again. That's why the STR needs three tries to do it (waste some time). I consistently do it with 2 attempts to open the app:

1) Open the app and press home button quickly as normal.
2) Wait 30 seconds (or watch logcat output for the camera preview to have started).
3) Open the app again, now stuck on spinner.
Attachment #8511396 - Flags: review?(dmarcos)
Assignee: nobody → aosmond
Component: Vendcom → Gaia::Camera
Flags: needinfo?(youlong.jiang)
Whiteboard: [NPOTB]
Ok. With the direction Andrew hinted I think I understand what's going on. 

We call stopFaceDetection in the release call. When leaving the app quickly the focus module has never been properly configured, the call to stopFaceDetection fails that makes the release call fail subsequently and mozCamera.release is never invoked. 

The call to stopFaceDetection doesn't seem necessary and it looks like a left over of the first implementation of face detection from bug 966828.
Attached file Pull Request
Andrew, No-Jun, Does this patch solve the issue for you?

I'm going to make sure that it doesn't have any side effects.
Attachment #8511434 - Flags: feedback?(npark)
Attachment #8511434 - Flags: feedback?(aosmond)
Comment on attachment 8511434 [details] [review]
Pull Request

Yes because it works on the same principle as my patch :).
Attachment #8511434 - Flags: feedback?(aosmond) → feedback+
Attachment #8511434 - Flags: review?(jdarcangelo)
I just tested this on master, and while it doesn't show the bug when repeating the STR, now it doesn't recognize the shutter button afterwards.  when the shutter is pressed, it activates tap to focus instead.  is this caused by the separate issue?  If the patch is not the cause of it, I can f+ it and create a new bug.

Version tested: (applied the patch to)
Gaia-Rev        e91d99e4d96954f06383c00bb9d79598a697e310
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8230834302c9
Build-ID        20141027040237
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141027.074236
FW-Date         Mon Oct 27 07:42:47 EDT 2014
Bootloader      L1TC00011880
(In reply to No-Jun Park [:njpark] from comment #30)
> I just tested this on master, and while it doesn't show the bug when
> repeating the STR, now it doesn't recognize the shutter button afterwards. 
> when the shutter is pressed, it activates tap to focus instead.  is this
> caused by the separate issue?  If the patch is not the cause of it, I can f+
> it and create a new bug.
> 
> Version tested: (applied the patch to)
> Gaia-Rev        e91d99e4d96954f06383c00bb9d79598a697e310
> Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8230834302c9
> Build-ID        20141027040237
> Version         36.0a1
> Device-Name     flame
> FW-Release      4.4.2
> FW-Incremental  eng.cltbld.20141027.074236
> FW-Date         Mon Oct 27 07:42:47 EDT 2014
> Bootloader      L1TC00011880

Oh, I should mention, it can't change the mode either, so the original bug is back it seems.
Comment on attachment 8511396 [details] [review]
Fix stop face detection during early camera release, v1

Superseded by attachment 8511434 [details] [review]
Attachment #8511396 - Attachment is obsolete: true
Attachment #8511396 - Flags: review?(dmarcos)
(In reply to No-Jun Park [:njpark] from comment #30)
> I just tested this on master, and while it doesn't show the bug when
> repeating the STR, now it doesn't recognize the shutter button afterwards. 
> when the shutter is pressed, it activates tap to focus instead.  is this
> caused by the separate issue?  If the patch is not the cause of it, I can f+
> it and create a new bug.
> 
> Version tested: (applied the patch to)
> Gaia-Rev        e91d99e4d96954f06383c00bb9d79598a697e310
> Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8230834302c9
> Build-ID        20141027040237
> Version         36.0a1
> Device-Name     flame
> FW-Release      4.4.2
> FW-Incremental  eng.cltbld.20141027.074236
> FW-Date         Mon Oct 27 07:42:47 EDT 2014
> Bootloader      L1TC00011880

What STR are you referring to? I cannot see the problem you mentioned with the patch attached applied and this STR:

https://bugzilla.mozilla.org/show_bug.cgi?id=1087475#c20
Flags: needinfo?(npark)
I was doing the same STR as the Comment 0, 

What i did was:
- flashed latest base image (v188)
- did full image flash on today's latest nightly mentioned in comment 31.
- get latest gaia build, and apply patch, and do make reset-gaia on it.

Also did:
- flashed latest base image (v188)
- flashed gaia and gecko image on top of it, no full image flash
- get latest gaia build, and apply patch, and do make reset-gaia on it.

In both instances, the viewfinder correctly displays, but buttons are not accessible.
Flags: needinfo?(npark)
Does it happen with v184 too?
Flags: needinfo?(npark)
No-Jun, I made a couple of changes to the patch. Can you verify if you can still reproduce the issue?
(In reply to Diego Marcos [:dmarcos] from comment #35)
> Does it happen with v184 too?

Yes, I flashed v184, shallow flashed gaia and gecko and just tried..  the buttons are also not accessible in this case as well.
Flags: needinfo?(npark)
(In reply to Diego Marcos [:dmarcos] from comment #36)
> No-Jun, I made a couple of changes to the patch. Can you verify if you can
> still reproduce the issue?

The latest patch looks good, it starts up fine, and now i can access all UIs as well.
Attachment #8511434 - Flags: feedback?(npark) → feedback+
Comment on attachment 8511434 [details] [review]
Pull Request

LGTM
Attachment #8511434 - Flags: review?(jdarcangelo) → review+
Ok, this is ready to go. Before landing: No-Jun, Can you please confirm one more time that all the problems when quickly opening and closing the camera are gone with the attached patch? Thank you!
Flags: needinfo?(npark)
(In reply to Diego Marcos [:dmarcos] from comment #40)
> Ok, this is ready to go. Before landing: No-Jun, Can you please confirm one
> more time that all the problems when quickly opening and closing the camera
> are gone with the attached patch? Thank you!

Hi Diego, I just retested on the latest master, and it still looks good, all buttons are working, and no viewfinder issues.
Flags: needinfo?(npark)
I rebased the PR. I'll merge it after getting green.
Assigning to Diego since he is doing the actual work.
Assignee: aosmond → dmarcos
Status: NEW → ASSIGNED
Landed on master:

https://github.com/mozilla-b2g/gaia/commit/2968d7d045cd1c4d338767ac6c17df1852b90e9f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8511434 [details] [review]
Pull Request

[Bug caused by]

1. Zoom bar retrieves the zoom value from camera hardware before being available 

bug 1045914 - https://github.com/mozilla-b2g/gaia/blob/8ae6598f3ab7b0c34ac42a73083ddb74266affba/apps/camera/js/controllers/zoom-bar.js#L35

2. When leaving the app quickly the focus module has never been properly configured, the call to stopFaceDetection fails which makes the release call fail subsequently and mozCamera.release is never invoked

bug 966828 - https://github.com/dmarcos/gaia/blob/99dfafa3f456e7d801d6e1ca157ebaaef959d485/apps/camera/js/lib/camera/camera.js#L637 

[User impact] if declined:

When quickly opening and closing the camera, the app might not boot properly ending up on a dark preview with no controls available.

[Testing completed]: 

Added unit tests to cover the regressions.

[Risk to taking this patch] (and alternatives if risky):

Low. It's a 10 lines change covering both causes of the bug. The rest of the 100 lines of the patch are additional unit tests to cover the regressions.

[String changes made]:

None
Attachment #8511434 - Flags: approval-gaia-v2.1?(fabrice)
Target Milestone: --- → 2.1 S8 (7Nov)
This is marked as blocking 2.0 as well, so presumably it needs a v2.0 nomination as well? :)
Flags: needinfo?(dmarcos)
Keywords: verifyme
Comment on attachment 8511434 [details] [review]
Pull Request

Thanks for the tests approving for uplift on 2.1, but we will need to make sure QA has this verified post landing.
Attachment #8511434 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
Verified the issue is fixed on 2.2 and 2.1

Viewfinder is functional after the user closes and opens the camera application a few times

Device: Flame 2.2 Master KK
BuildID: 20141104040207
Gaia: 3c50520982560ccba301474d1ac43706138fc851
Gecko: 54d05732f29b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1 KK
BuildID: 20141104001202
Gaia: 8b0cf889ae0d48a9eb7ecdcb9b67590de45cc5e5
Gecko: 388b03efe92d
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
  
Leaving "verifyme" until the patch will be uplifted for 2.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: needinfo?(dmarcos)
Still waiting on a v2.0 approval request here, AFAICT?
Flags: needinfo?(dmarcos)
Hi, Diego,

Could we uplift this patch to 2.0 branch?
Thanks.
Calling this wontfix for v2.0 so it stops showing up on various bug queries as something needing attention still. Feel free to set it back and push this bug forward if that's not the case.
Thanks Ryan. Remove NI.
Flags: needinfo?(dmarcos)
Per comment 49 and comment 52, clear "verifyme" keyword.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.