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

Status: VERIFIED FIXED

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

Description

[No-Jun Park [:njpark]]

+++ 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

Comment 1

[No-Jun Park [:njpark]]

Attachment: camera.log

logcat during the process

Comment 2

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
Same reason as Bug 1080912.  The camera app becomes unusable since the viewfinder does not become active.

Comment 3

[No-Jun Park [:njpark]]

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

Comment 4

[Tony Chung [:tchung]]

wesly, francis, can you help communicate this to T2M?  also ni? mike lien to check again on the v188 base to confirm.

Comment 5

[Tony Chung [:tchung]]

Qawanted on v188 only to see if it repros.

Comment 6

[No-Jun Park [:njpark]]

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.

Comment 7

[Tony Chung [:tchung]]

triaged to block 2.0+.  ni? bhavana to follow up with vendor

Comment 8

[bhavana bajaj [:bajaj]]

(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.

Comment 9

[Mike Lien[:mlien]]

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%

Comment 10

[Diego Marcos [:dmarcos]]

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

Comment 11

[Diego Marcos [:dmarcos]]

(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?

Comment 12

[Mike Lien[: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

Comment 13

[Mike Lien[: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

Comment 14

[Wilson Page [:wilsonpage]]

(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.

Comment 15

[Wesly Huang (EPM)]

Hi Youlong, pls help check this issue, I've added it into our tracking list to follow with other issues together. Thanks.

Comment 16

[Wesly Huang (EPM)]

Btw Youlong since this is reported after v180 pls also check if this is related to something changed while migrating to CS release.

Comment 17

[Diego Marcos [:dmarcos]]

(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?

Comment 19

[Diego Marcos [:dmarcos]]

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.

Comment 20

[Mike Lien[:mlien]]

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

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Err cleared wrong ni ;).

Comment 23

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

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").

Comment 24

[Diego Marcos [:dmarcos]]

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

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Fix stop face detection during early camera release, v1

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.

Comment 27

[Diego Marcos [:dmarcos]]

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.

Comment 28

[Diego Marcos [:dmarcos]]

Attachment: 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.

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8511434 [details] [review]
Pull Request

Yes because it works on the same principle as my patch :).

Comment 30

[No-Jun Park [:njpark]]

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

Comment 31

[No-Jun Park [:njpark]]

(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 32

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8511396 [details] [review]
Fix stop face detection during early camera release, v1

Superseded by attachment 8511434 [details] [review]

Comment 33

[Diego Marcos [: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

Comment 34

[No-Jun Park [:njpark]]

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.

Comment 35

[Diego Marcos [:dmarcos]]

Does it happen with v184 too?

Comment 36

[Diego Marcos [:dmarcos]]

No-Jun, I made a couple of changes to the patch. Can you verify if you can still reproduce the issue?

Comment 37

[No-Jun Park [:njpark]]

(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.

Comment 38

[No-Jun Park [:njpark]]

(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.

Comment 39

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8511434 [details] [review]
Pull Request

LGTM

Comment 40

[Diego Marcos [:dmarcos]]

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!

Comment 41

[No-Jun Park [:njpark]]

(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.

Comment 42

[Diego Marcos [:dmarcos]]

I rebased the PR. I'll merge it after getting green.

Comment 43

[Andrew Osmond [:aosmond] (he/him)]

Assigning to Diego since he is doing the actual work.

Comment 44

[Diego Marcos [:dmarcos]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/2968d7d045cd1c4d338767ac6c17df1852b90e9f

Comment 45

[Diego Marcos [:dmarcos]]

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

Comment 46

[Ryan VanderMeulen [:RyanVM]]

This is marked as blocking 2.0 as well, so presumably it needs a v2.0 nomination as well? :)

Comment 47

[bhavana bajaj [:bajaj]]

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.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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

Comment 49

[sarsenyev]

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Still waiting on a v2.0 approval request here, AFAICT?

Comment 51

[William Hsu [:whsu]]

Hi, Diego,

Could we uplift this patch to 2.0 branch?
Thanks.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 53

[William Hsu [:whsu]]

Thanks Ryan. Remove NI.

Comment 54

[Shally Li（leave from Mozilla）]

Per comment 49 and comment 52, clear "verifyme" keyword.