1025197 - [B2G][Camera] Camera freezes when switching between video and camera multiple times

Status: VERIFIED FIXED

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

Description

[Anna]

Attachment: logcat

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.73.11 (KHTML, like Gecko) Version/7.0.1 Safari/537.73.11

Steps to reproduce:

Description: 
If a user switches between video recording and camera a couple of times, camera freezes in a loading spinner and the user won't be able to use a camera until restarted. 

Repro Steps:
1) Updated Flame to BuildID: 20140613000203
2) Open Camera 
3) Take a picture and switch to video mode
4) Record video and switch to camera
5) Repeat steps 3&4 multiple times 

Environmental Variables:
Device: Flame 2.0
BuildID: 20140613000203
Gaia: a3a5322692578e0a577fb7fa08e32144b2b05ba3
Gecko: 8897bc43f59b
Version: 32.0a2
Firmware Version: v121-2


Actual results:

Actual:
 Camera freezes with a loading spinner, the user is unable to take pictures 



Expected results:

Expected:
 The camera doesn't freeze when switching from video to camera

Comment 1

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

This sounds a lot like bug 1020560, which is fixed.

Comment 2

[Anna]

http://youtu.be/YrbD5P901-A This is the video from my bug, the issue seems to be a little different from the bug you have mentioned. In this bug the camera goes to a spinner loading mode, where in the other one it makes the screen go black instead of freezing.

Comment 3

[Wilson Page [:wilsonpage]]

Sounds like bug 1007832, which has been fixed. Also bug 1021284 would have resulted in the spinner appearing when it shouldn't have been.

Comment 4

[Wilson Page [:wilsonpage]]

Could you retest on latest 'master'?

Comment 5

[Anna]

I have retested the issue with the latest 'master' but, the issue still reproduces, the spinner appears after switching from camera to video mode. 

Flame master 
BuildID: 20140620040204
Gaia: ccd70903544486bea04e85d8a4aacf63f1de2a72
Gecko: bdac18bd6c74
Version: 33.0a1

Comment 6

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

Attachment: logcat of Camera app hang switching to picture mode

Confirmed that the app hangs (shows a spinner with viewfinder still active in the background) when switching to picture mode.

I reproduced on flame master.

This seems to be the same as bug 1022561. Will try to reproduce with additional logging enabled.

Comment 7

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

Attachment: logcat with extra debugging enabled

Comment 8

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

Looking through attachment 8443570 [details] shows something very suspicious. At line 38245, there is the call into the library's take_picture() method:

06-20 12:33:12.397   284  1846 E QCamera2HWI: [KPI Perf] static int qcamera::QCamera2HardwareInterface::take_picture(camera_device*): E PROFILE_TAKE_PICTURE

Entries like this occur in the log for every other picture taken, but they're always followed by another call to takePicture() (e.g. line 20754):

06-20 12:33:00.397   284  1846 E QCamera2HWI: [KPI Perf] static int qcamera::QCamera2HardwareInterface::take_picture(camera_device*): E PROFILE_TAKE_PICTURE
06-20 12:33:00.397   284  1723 D QCamera2HWI: int qcamera::QCamera2HardwareInterface::takePicture(): E

In the failure case, this second line doesn't appear. Instead, on line 38422, we get the exit entry:

06-20 12:33:12.497   284  1846 D QCamera2HWI: [KPI Perf] static int qcamera::QCamera2HardwareInterface::take_picture(camera_device*): X

...but no takePicture() call happens. This means no picture callback will happen. The app thinks the process is taking a long time and throws up the spinner.

Comment 9

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

Attachment: Only clear the "recording hint" when switching modes, v1

Anna, can you flash a Flame device with a custom Gecko build? If so, can you apply this patch and build? It should fix this problem.

Comment 10

[Natalya Kot [:nkot]]

'qawanted' for comment 9,

Josh, can you have someone to test the patch. Thanks!

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 8443698 [details] [diff] [review]
Only clear the "recording hint" when switching modes, v1

Review of attachment 8443698 [details] [diff] [review]:
-----------------------------------------------------------------

I only noticed one little thing that I made a comment on. So r+ (and I'll defer to you on how or whether you address my comment)

::: dom/camera/GonkCameraControl.cpp
@@ -277,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  
> -  rv = PushParameters();

So - I noticed that you're removing PushParameters, which will be covered by the ICameraControlParameterSetAutoEnter object in SetConfigurationInternal.

Is it possible for SetPictureConfiguration to be called from anywhere else?

I would have expected to either see an ICameraControlParameterSetAutoEnter introduced in this function or perhaps an assert that you're within a deferred push.

Comment 12

[Joshua Mitchell (Inactive)]

Hi Mike - I'm the QA-Contact who will be testing the patch for Anna - Can I get you to put this into your Github repository (our current process requires us to pull it from there).

Comment 13

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

(In reply to Dave Hylands [:dhylands] (PTO - back Mon Jun 23) from comment #11)
> 
> Is it possible for SetPictureConfiguration to be called from anywhere else?
> 
> I would have expected to either see an ICameraControlParameterSetAutoEnter
> introduced in this function or perhaps an assert that you're within a
> deferred push.

Right now, SetPictureConfiguration() isn't called from anywhere else, but there's no reason that couldn't change in the future. Your suggestion will definitely make the code more robust, so I'm happy to take it. Thanks!

Comment 14

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

(In reply to Joshua Mitchell from comment #12)

> Hi Mike - I'm the QA-Contact who will be testing the patch for Anna - Can I
> get you to put this into your Github repository (our current process
> requires us to pull it from there).

Hi Joshua, I should be able to throw something into github--note though that this is a Gecko change, not a Gaia one.

Comment 15

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

(In reply to Mike Habicher [:mikeh] from comment #14)
> (In reply to Joshua Mitchell from comment #12)
> 
> > Hi Mike - I'm the QA-Contact who will be testing the patch for Anna - Can I
> > get you to put this into your Github repository (our current process
> > requires us to pull it from there).
> 
> Hi Joshua, I should be able to throw something into github--note though that
> this is a Gecko change, not a Gaia one.

Having said that, this patch fixes something that is definitely not correct anyway, and it will land shortly. Once it does, it will show up in your builds automatically. You can test it then.

Comment 16

[Hema Koka [:hema]]

QA,

Can you please try to reproduce this on Dolphin?

Thanks
Hema

Comment 17

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

I will also try to reproduce on Dolphin. Regardless, the current implementation is just wrong, so this fix should be applied going forward. If Dolphin is not affected by this issue, it is lucky; if current and future platforms are affected by this fix, then those issues will need to be addressed separately.

Comment 18

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

Attachment: Only clear the "recording hint" when switching modes, v2

Re-request for review due to some fundamental changes resulting from feedback on the preview version of this patch. This change removes most direct calls to 'mParams.Set()' so that a parameter push is implied, unless inside an 'ICameraControlParameterSetAutoEnter' block.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=3c519c0da324

Comment 19

[Dave Hylands [:dhylands]]

Comment on attachment 8445382 [details] [diff] [review]
Only clear the "recording hint" when switching modes, v2

Review of attachment 8445382 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me

Comment 20

[Joshua Mitchell (Inactive)]

(In reply to Hema Koka [:hema] from comment #16)
> QA,
> 
> Can you please try to reproduce this on Dolphin?
> 
> Thanks
> Hema

Hi Hema:

Unfortunately the QA-Wanted team does not have this device.

Comment 21

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

Hema, I tested this patch on Dolphin, and it at least allows the Dolphin camera viewfinder to start in recording mode. Without it, the viewfinder comes up black as in bug 1030296.

In light of Dolphin's other problems, I believe we should:
- land this patch and backport it to v1.4 and v2.0;
- not block on whether or not Dolphin is affected.

Comment 22

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

https://hg.mozilla.org/integration/b2g-inbound/rev/7a84bd4bb3da

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a84bd4bb3da

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2ca5265a5b1

Needs rebasing for b2g30_v1_4 uplift.

Comment 26

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

Attachment: Only clear the "recording hint" when switching modes, v2 [b2g30] r=dhylands

Patch rebased to b2g30.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Thanks! Please include commit information with the patch next time, though :)

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/532de50fa217

Comment 28

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

Attachment: Fix b2g30 backport bustage

b2g30 used MOZ_ASSUME_UNREACHABLE(), which was later morphed into MOZ_ASSERT_UNREACHABLE().

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/236ef7ae3260

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out from v1.4 due to B2G mochitest failures.
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/7bb6ba7f6ef9

https://tbpl.mozilla.org/php/getParsedLog.php?id=42569766&tree=Mozilla-B2g30-v1.4

Comment 32

[William Hsu [:whsu]]

There is something wrong with this patch.
I still can reproduce this bug after I tested the latest central build(PVT)
Attach the demo video (WP_20140630_013.mp4)

* Build:
 - Gaia      de14e61098b742251b34f856e48649db8bed552c
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/b6408c32a170
 - BuildID   20140630040201
 - Version   33.0a1
 ~ Result: I still can reproduce it

Please make a decision to see if we need to back out this patch.
Thanks for your help!

Comment 33

[William Hsu [:whsu]]

Attachment: WP_20140630_013.mp4

Comment 34

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

(In reply to William Hsu [:whsu] from comment #32)
>
> There is something wrong with this patch.
> I still can reproduce this bug after I tested the latest central build(PVT)
> Attach the demo video (WP_20140630_013.mp4)

whsu, your build below at changeset 191421:b6408c32a170 does not include the follow-up fix in bug 1030821, which landed in 191552:e94590e830ff.

Comment 35

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

Attachment: Only clear the "recording hint" when switching modes, v3 [b2g30] r=dhylands, a=1.4+

As per bug 1030821 comment 18, this patch also includes the follow-up fix from that bug. Oh, and it should pass the automated tests too.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Thanks!

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e8270f76c890

Comment 37

[William Hsu [:whsu]]

Needinfo myself to re-test it.
Thanks!

Comment 38

[William Hsu [:whsu]]

Thanks everyone!
Verified it. I cannot reproduce it on the latest V1.4 build (Flame)

* Build information:
 - Gaia      318f8d814d8930b5530bcb0badc1bb5bd0b5ef45
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ef78ebf203e5
 - BuildID   20140708000310
 - Version   30.0

Comment 40

[Elie]

Attachment: VIDEO0079_Compress.MP4

This issue has been successfully verified on Flame 2.0:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141130000204
Version         32.0
Device-Name     flame
FW-Release      4.4.2


This issue has been successfully verified on Flame 2.1:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141130001203
Version         34.0
Device-Name     flame
FW-Release      4.4.2