890427 - [Camera] repetitive touches makes camera app hang

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title :  repetitive touches makes camera app hang
2. Precondition : Camera app should be working
3. Tester's Action : 1. Open camera app -> touch fast and repetitively on toggle and capture buttons randomly
                   
 
4. Detailed Symptom (ENG.) : camera app hangs with focusing ring shown and UI is disabled
5. Expected : camera app should not hang.

6.Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 65e861f5941cab66815a2f79b0bc0a6a0fc8f6fe
9.Personal email id: [e-mail]

Comment 1

[Leo]

Attachment: Video attachment

Attached video shows camera app got hanged after random repetitive touches on toggle and capture buttons.

Comment 2

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

Confirmed--I have been able to reproduce on Inari:
- gecko: b2g18:45507bc2bea1
- gaia: v1-train:7c40bdaeaffae708342fc773926dcfac5389348e

It took a few attempts to reproduce, but the hang (symptoms: black viewfinder, gallery button enabled, shutter and video-mode switch buttons disabled, tapping the viewfinder causes the filmstrip to come and go) _seems_ to happen after switching to video mode, hitting the record button twice in rapid succession to start and stop recording, and then switching back to photo mode.

Comment 3

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

This is definitely blocking: once the camera gets into this state, it is stuck. Closing and reopening the camera does not clear the hang. The lockscreen camera is also hosed.

Comment 4

[Leo]

Target date for this issue in leo device: July 12, 2013.

Comment 5

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

The CameraThread is blocked here:

Thread 14 (Thread 459.476):
#0  __ioctl () at bionic/libc/arch-arm/syscalls/__ioctl.S:9
#1  0x40053b5c in ioctl (fd=<value optimized out>, request=8) at bionic/libc/bionic/ioctl.c:41
#2  0x42257a6e in android::IPCThreadState::talkWithDriver (this=0x425ffd40, doReceive=true)
    at frameworks/base/libs/binder/IPCThreadState.cpp:804
#3  0x42257e0c in android::IPCThreadState::waitForResponse (this=0x425ffd40, reply=0x44cffd68, acquireResult=0x0)
    at frameworks/base/libs/binder/IPCThreadState.cpp:668
#4  0x4225846c in android::IPCThreadState::transact (this=0x425ffd40, handle=2, code=5, data=..., reply=0x44cffd68, flags=16)
    at frameworks/base/libs/binder/IPCThreadState.cpp:560
#5  0x4225549e in android::BpBinder::transact (this=0x437c9580, code=5, data=..., reply=0x44cffd68, flags=0)
    at frameworks/base/libs/binder/BpBinder.cpp:165
#6  0x42892422 in android::BpCamera::unlock (this=0x42255479) at frameworks/base/libs/camera/ICamera.cpp:264
#7  0x4288fe9e in android::Camera::setPreviewTexture (this=<value optimized out>, surfaceTexture=<value optimized out>)
    at frameworks/base/libs/camera/Camera.cpp:196
#8  0x41ec6a0c in ?? () from /home/mikeh/dev/mozilla/btg024/objdir-gecko-b2g18-debug/dist/bin/libxul.so
#9  0x41ec6a0c in ?? () from /home/mikeh/dev/mozilla/btg024/objdir-gecko-b2g18-debug/dist/bin/libxul.so

...which is called from here: http://mxr.mozilla.org/mozilla-b2g18/source/dom/camera/GonkCameraHwMgr.cpp#186

This launches a binder call into:
http://androidxref.com/4.0.4/xref/frameworks/base/libs/camera/Camera.cpp#187

...and into the Camera service here:
http://androidxref.com/4.0.4/xref/frameworks/base/services/camera/libcameraservice/CameraService.cpp#570

...then to ::setPreviewWindow():
http://androidxref.com/4.0.4/xref/frameworks/base/services/camera/libcameraservice/CameraService.cpp#515

Hardware calls are here:
http://androidxref.com/4.0.4/xref/frameworks/base/services/camera/libcameraservice/CameraService.cpp#536

...and here:
http://androidxref.com/4.0.4/xref/frameworks/base/services/camera/libcameraservice/CameraService.cpp#541

Comment 6

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

Okay, got it. This bug is a deadlock between the main mediaserver thread and the camera snapshot thread.

1. call takePicture(), which sets a snapshot mode flag in the camera library, and return
2. call startPreview(), which sees the snapshot mode flag is set, and so blocks on a condvar waiting for the snapshot mode flag to get cleared
3. the camera library invokes the takePicture callback

Normally, the snapshot mode flag is cleared in (3).  In order to do this, the callback needs to get the CameraService::Client::mLock, but the mLock is still held by the blocked takePicture() call in (1).

Comment 7

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

The solution is (probably) to disable all of the non-preview messages when starting the preview. Then, once the preview is started, enable messages like shutter and compressed-image.

Comment 8

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

Ugh, the CameraService API doesn't allow us to specify which messages are enabled (aside from takePicture(), for some reason).  Nor does it expose an API where we can cancel picture-taking (though we can cancel auto-focus--again, for some reason).

Sotaro, any thoughts on the best approach to tackling this? I'm thinking of maintaining a counter to keep track of outstanding API calls that invoke callbacks, and deferring any API calls until we receive all of those callbacks.

Comment 9

[Sotaro Ikeda [:sotaro]]

I also regenerate the freeze and got following stack.

(gdb) info stack
#0  __ioctl () at bionic/libc/arch-arm/syscalls/__ioctl.S:9
#1  0x4008e280 in ioctl (fd=<value optimized out>, request=8) at bionic/libc/bionic/ioctl.c:41
#2  0x41781a6e in android::IPCThreadState::talkWithDriver (this=0x4385fe20, doReceive=true)
    at frameworks/base/libs/binder/IPCThreadState.cpp:804
#3  0x41781e0c in android::IPCThreadState::waitForResponse (this=0x4385fe20, reply=0x44333dd0, acquireResult=0x0)
    at frameworks/base/libs/binder/IPCThreadState.cpp:668
#4  0x4178246c in android::IPCThreadState::transact (this=0x4385fe20, handle=2, code=5, data=..., reply=0x44333dd0, 
    flags=16) at frameworks/base/libs/binder/IPCThreadState.cpp:560
#5  0x4177f49e in android::BpBinder::transact (this=0x43f0f800, code=5, data=..., reply=0x44333dd0, flags=0)
    at frameworks/base/libs/binder/BpBinder.cpp:165
#6  0x41dc4486 in android::BpCamera::startPreview (this=<value optimized out>)
    at frameworks/base/libs/camera/ICamera.cpp:109
#7  0x41dc1f02 in android::Camera::startPreview (this=<value optimized out>) at frameworks/base/libs/camera/Camera.cpp:206
#8  0x40855cd0 in android::GonkCameraHardware::CancelAutoFocus (this=0x43fb84c0)
    at /home/sotaro/b2g_11_leo/B2G/gecko/dom/camera/GonkCameraHwMgr.cpp:267
#9  0x40854ae0 in mozilla::nsGonkCameraControl::SetParameter (this=0x43860d40, aKey=<value optimized out>, aValue=1)
    at /home/sotaro/b2g_11_leo/B2G/gecko/dom/camera/GonkCameraControl.cpp:629
#10 0x40be252e in nsCOMPtr<nsIThreadObserver>::operator= (this=0x42c9fe80, mayWait=<value optimized out>, 
    result=0x42c9fe80) at ../../dist/include/nsCOMPtr.h:614
#11 nsThread::ProcessNextEvent (this=0x42c9fe80, mayWait=<value optimized out>, result=0x42c9fe80)
    at /home/sotaro/b2g_11_leo/B2G/gecko/xpcom/threads/nsThread.cpp:630
#12 0x40bc2906 in NS_ProcessPendingEvents_P (thread=0x43fb84c0, timeout=1140033984)
    at /home/sotaro/b2g_11_leo/B2G/objdir-gecko/xpcom/build/nsThreadUtils.cpp:171

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #8)
> Ugh, the CameraService API doesn't allow us to specify which messages are
> enabled (aside from takePicture(), for some reason).  Nor does it expose an
> API where we can cancel picture-taking (though we can cancel
> auto-focus--again, for some reason).
> 
> Sotaro, any thoughts on the best approach to tackling this? I'm thinking of
> maintaining a counter to keep track of outstanding API calls that invoke
> callbacks, and deferring any API calls until we receive all of those
> callbacks.

I feel it is better to make clear about the camera hw's limitation. We do not have information about it. We do not have enough src/symbol around camera module. We cannot debug around it. It is better to get the info from partners.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #8)
> Sotaro, any thoughts on the best approach to tackling this? I'm thinking of
> maintaining a counter to keep track of outstanding API calls that invoke
> callbacks, and deferring any API calls until we receive all of those
> callbacks.

I am not sure that the counter is a good choice to solve this problem. But a similar thing seems necessary. It seems necessary to wait a snapshot completion. If camera is in SNAPSHOT_IN_PROGRESS, Camera API need to reject camera control. In android, the camera app checks if the camera hw is idle or not. And the app decide how to control the camera hw.
http://androidxref.com/4.0.4/xref/packages/apps/Camera/src/com/android/camera/Camera.java#isCameraIdle

Comment 12

[Sotaro Ikeda [:sotaro]]

I do not have enough symbol in mediaserver side. Though following seems the place of the deadlock in my case.

 (gdb) info stack
#0  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
#1  0x400fe544 in _normal_lock (mutex=0xab0dbc) at bionic/libc/bionic/pthread.c:951
#2  pthread_mutex_lock (mutex=0xab0dbc) at bionic/libc/bionic/pthread.c:1041
#3  0x40d44062 in android::QCameraStream_preview::initDisplayBuffers() ()
   from /home/sotaro/b2g_11_leo/B2G/out/target/product/leo/system/lib/hw/camera.msm7627a.so
#4  0x40d44062 in android::QCameraStream_preview::initDisplayBuffers() ()
   from /home/sotaro/b2g_11_leo/B2G/out/target/product/leo/system/lib/hw/camera.msm7627a.so
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Comment 13

[Leo]

Hi Mike,

Do you have updates on this issue?

Comment 14

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

I am working on this issue. I should have a patch ready for testing by EoD today/early tomorrow.

Comment 15

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

Incidentally, I've refined the STR for this bug:
1. open the Camera app
2. press the Mode switch button to switch to Video mode
3. press the Mode switch button again and IMMEDIATELY start pressing the shutter button rapidly to try to take a picture.

Expected behaviour: the camera should take a picture.

Observed results: the camera service deadlocks as described in comment 6.

Comment 16

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

From an API perspective, I think the problem is this:
1. switching to the picture mode disables the camera UI
2. the Camera app calls getPreviewStream() which returns right away
3. the onsuccess callback passed to getPreviewStream() is invoked and connects the stream to the viewfinder <video> DOM object, then enables camera UI

Connecting the preview stream object to the <video> tag triggers the call to startPreview() on the camera hardware, and this happens asynchronously. If this happens after the UI is enabled, a number of problems can occur:
a. we run the risk of deadlocking the camera service as in comment 6
b. the call to takePicture() will fail

I can protect the CameraControl API against deadlocking the camera service, but the Camera app will need to prevent enabling the UI until the preview stream has started.

daleharvey, is there a property on the video tag the Camera app can listen for? I see 'onreadystatechange', but not sure if that's the right way to go.

Comment 17

[Leo]

HI MIke and Dale,

Addition of minimal delay when preview switching (in changeMode()) prevents this scenario. Also, addition of delay will not cause any issues in performance.

setTimeout(function() {
      if (self._captureMode === self.CAMERA) {
        self._cameraObj.getPreviewStream(self._previewConfig,
                 gotPreviewStream.bind(self));
      } else {
        self._previewConfigVideo.rotation = self._phoneOrientation;
        self._cameraObj.getPreviewStreamVideoMode(self._previewConfigVideo,
                 gotPreviewStream.bind(self));
      }
    }, 50);

Mike, I have tried disabling UI till next preview loads, but it didn't work few times, though I need to verify my implementaion

Comment 18

[Dale Harvey (:daleharvey)]

There are no events fired on the video object when mozSrcObject is set, this means we cant disable UI until its properly loaded, the setTimeout as a hack can work intermittently, but its asking for further bugs

Can we get the video tag (or the camera api) to fire and event when the UI is ready to be enabled?

Comment 19

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

(In reply to Dale Harvey (:daleharvey) from comment #18)
>
> There are no events fired on the video object when mozSrcObject is set, this
> means we cant disable UI until its properly loaded, the setTimeout as a hack
> can work intermittently, but its asking for further bugs

The setTimeout() hack proves the solution, but I agree it's too non-deterministic.

> Can we get the video tag (or the camera api) to fire and event when the UI
> is ready to be enabled?

Yes, I can add something like an onPreviewStarted callback to CameraControl.

Actually, since you'll probably want to disable the UI when the preview stops, I'll make it onPreviewStateChanged(bool running) (or something like that), and camera.js can react appropriately.

(I'm a little miffed at how rigid the AOSP API is; but this exercise has given me some things to think about in terms of the future migration to the getUserMedia() API.)

Comment 20

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

Attachment: [v1][b2g18] Let the Camera app know when the preview has actually started/stopped

sotaro, this patch adds a |onPreviewStateChange| attribute to CameraControl that invokes a callback function with an argument of either "started" or "stopped", so that the Camera app can enable/disable its UI as needed.

I'm on PTO as of this evening, no laptop (but should be able to check email), so I'm hoping you can look this over and ask someone (jst?--it's almost completely boilerplate) to review it, then land it. (Please.)

I've tested the patch and it works as expected. As follow-up, daleharvey will need to make the corresponding gaia change to hook into the new callback.

Comment 21

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

Attachment: [v1][m-c] Let the Camera app know when the preview has actually started/stopped

This is the m-c version of the patch; it uses a different macro to indicate unreachable code.

Comment 22

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

Attachment: HOWTO

And this is how to use it (kind of).

Comment 23

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

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

Comment 24

[Sotaro Ikeda [:sotaro]]

mikeh, okey. I am going to handle the patches. Have a nice PTO!

Comment 25

[Dale Harvey (:daleharvey)]

That looks good to me, sotoro do you want to take this bug, I will get the gaia side of it done today or tomorrow so they can be tested together

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Dale Harvey (:daleharvey) from comment #25)
> That looks good to me, sotoro do you want to take this bug, I will get the
> gaia side of it done today or tomorrow so they can be tested together

I take this bug. Thanks!

Comment 27

[Sotaro Ikeda [:sotaro]]

Comment on attachment 777368 [details] [diff] [review]
[v1][b2g18] Let the Camera app know when the preview has actually started/stopped

Looks good!

Comment 28

[Sotaro Ikeda [:sotaro]]

Comment on attachment 777424 [details] [diff] [review]
[v1][m-c] Let the Camera app know when the preview has actually started/stopped

lgtm.

Comment 29

[Leo]

Mike,

Camera app hangs with focus ring during the following scenario also:
1. touch gallery -> immediately touch capture -> camera app hangs with focus ring
2. touch capture button -> immediately press home button -> camera app hangs with focus ring.

Comment 30

[Sotaro Ikeda [:sotaro]]

Hi Leo, can you get a stack trace of where the deadlock happen by using GDB?

It seems like deadlock happens somewhere by the contention between taking picture and camera. Bug 836427 seems similar bug, but different result.

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> It seems like deadlock happens somewhere by the contention between taking
> picture and camera. Bug 836427 seems similar bug, but different result.

If it is correct, it is better to handle as another bug. FIX should be different.

Comment 32

[Sotaro Ikeda [:sotaro]]

(In reply to Leo from comment #29)
> Camera app hangs with focus ring during the following scenario also:
> 1. touch gallery -> immediately touch capture -> camera app hangs with focus
> ring
> 2. touch capture button -> immediately press home button -> camera app hangs
> with focus ring.

Is it Bug 895857? If it is, discuss there :-)

Comment 33

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> Hi Leo, can you get a stack trace of where the deadlock happen by using GDB?

I checked that on my leo, dead lock did not happend in camera app process, but happened only on camera service side. I do not have a symbol around camera hw. I can not investigate more about the place of the deadlock.

We should move the discussion of Comment 29 case to Bug 895857.

Comment 34

[Leo]

(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> (In reply to Sotaro Ikeda [:sotaro] from comment #30)
> > Hi Leo, can you get a stack trace of where the deadlock happen by using GDB?
> 
> I checked that on my leo, dead lock did not happend in camera app process,
> but happened only on camera service side. I do not have a symbol around
> camera hw. I can not investigate more about the place of the deadlock.
> 
> We should move the discussion of Comment 29 case to Bug 895857.

Yes, we can discuss on this issue issue in Bug 895857

Comment 35

[Sotaro Ikeda [:sotaro]]

Comment on attachment 777368 [details] [diff] [review]
[v1][b2g18] Let the Camera app know when the preview has actually started/stopped

jst, can you review a patch?

Comment 36

[Sotaro Ikeda [:sotaro]]

Dale, about when can we get the gaia side?

Comment 37

[David Flanagan [:djf]]

Dale,

Feel free to resassign to me if you're busy with other things.

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 777368 [details] [diff] [review]
[v1][b2g18] Let the Camera app know when the preview has actually started/stopped

Looks good, r=jst

Comment 39

[Johnny Stenback (:jst)]

Any chance we could get an automated test for this fix?

Comment 40

[Sotaro Ikeda [:sotaro]]

mikeh, do you know about current status of camera's automated test?

Comment 41

[Dale Harvey (:daleharvey)]

Cheers djf, yeh given I am not involved in planning and that it impacts everyone estimations etc I think in general its best to start sticking to the teams by default, of course if your needing some more resources then feel free to assign me.

Comment 42

[David Flanagan [:djf]]

I'm not able to reproduce this bug on my unagi, and my Leo device has such old firmware that it thinks it has no device storage, so it won't run the camera at all...

I can try making a Gaia patch, but it looks like I'll need someone else to test it.

Comment 43

[John Hu [:johnhu][:johu][:醬糊小弟]]

David, 

I can help to do the test. I had tested with my Inari which can reproduce this bug. But I may need more information on how to do it.

Comment 44

[Sotaro Ikeda [:sotaro]]

I also can help the test. I can reproduce it on leo device.

Following is easier to reproduce the problem for me.
STR
- [1] Start camera app
- [2] Change to video mode
- [3] When you tap a camera mode icon, you also tap camera shutter button slightly later than the camera mode icon.

Frequency Rate: 1/20

When doing [3], I tap shutter button multiple times not to fail taking a photo.

Comment 45

[David Flanagan [:djf]]

Attachment: Gaia portion of the patch

Sotaro,

If I'm understanding the bug correctly, this patch should be the fix that is required in Gaia. But since I can't reproduce it reliably, I need you to test it.  Note that I've only attempted to use the new onPreviewStateChange callback in the case where the user switches from photos to videos or vice versa.  If you know of other cases where deadlock can arise from the same underlying bug, let me know.

John: your testing is also welcome, but I am also asking you to review the patch.

Comment 46

[Sotaro Ikeda [:sotaro]]

Djf, thanks for the patch. I will check the patch on leo device soon.

Comment 47

[Sotaro Ikeda [:sotaro]]

When I applied attachment 784600 [details], camera app does not work correctly. When I changed the camera app to video mode, camera app's all buttons stay disabled and never enabled again.

Comment 48

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #47)
> When I applied attachment 784600 [details], camera app does not work
> correctly. When I changed the camera app to video mode, camera app's all
> buttons stay disabled and never enabled again.

Corection: shutter button and mode button stay disabled.

Comment 49

[John Hu [:johnhu][:johu][:醬糊小弟]]

To help make this bug easy to happen, I add the following code to the end of enableButtons at camera.js:

    window.setTimeout(function(){
      this.capturePressed({'target': this.captureButton});  
    }.bind(this));

That help me press the button once it is enabled. After the patch, the STR of this bug changes to:
1. open camera app
2. once shutter sound heard, press home
3. do 1 and 2 repeatedly.

The reason to press home and reopen is that the camera app stops preview while invisible and starts preview while visible. The program above helps press shutter button ASAP. This is made according to the comment 6 and comment 16. We may have the halt screen about 2 or 3 times. After the halt screen, we may need to kill camera app and reopen it to double confirm that.

Comment 50

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 784600 [details]
Gaia portion of the patch

This patch looks good. But we may need to add the similar code to the function of loadCameraPreview. It's about line 843. There are two functions calling getPreviewStream. One is at the place this patch is. Another is at loadCameraPreview. With the testing code I propose, it is still reproducible. I think the reason is startPreview function uses loadCameraPreview.

Comment 51

[David Flanagan [:djf]]

(In reply to Sotaro Ikeda [:sotaro] from comment #48)
> (In reply to Sotaro Ikeda [:sotaro] from comment #47)
> > When I applied attachment 784600 [details], camera app does not work
> > correctly. When I changed the camera app to video mode, camera app's all
> > buttons stay disabled and never enabled again.
> 
> Corection: shutter button and mode button stay disabled.

Sotaro: did you have Mike's gecko patch applied when you tested? The behavior you describe is what I'd expect if you tested the gaia patch against an unpatched gecko.
This is one of those bugs where we'll have to be sure that that gecko patch has landed before we apply the gaia patch.

Comment 52

[David Flanagan [:djf]]

(In reply to John Hu [:johnhu] from comment #49)
> To help make this bug easy to happen, I add the following code to the end of
> enableButtons at camera.js:
> 
>     window.setTimeout(function(){
>       this.capturePressed({'target': this.captureButton});  
>     }.bind(this));
> 

Brilliant!  Thanks for that idea.  I'll patch loadCameraPreview too.  Were you able to verify that the patch did at least fix the deadlock when switching from video to photo and then pressing the shutter button right away?

Comment 53

[John Hu [:johnhu][:johu][:醬糊小弟]]

I had tried but it was too hard to reproduce. The reproducing rate is pretty low. I had reproduced twice yesterday. But I never reproduce it today, no matter the patch applied or not.

But, I had patched loadCameraPreview. The automatic testing code can't reproduce it. I think the code and logic is correct.

Comment 54

[John Hu [:johnhu][:johu][:醬糊小弟]]

I found another testing code to test switch mode:

  enableButtons: function camera_enableButtons() {
    this.captureButton.removeAttribute('disabled');
    this.switchButton.removeAttribute('disabled');

    if (this._pendingPick) {
      this.cancelPickButton.removeAttribute('disabled', 'disabled');
    }
    // add here
    window.setTimeout(function(){
      if (this._captureMode === this.CAMERA) {
        this.capturePressed({'target': this.captureButton});
      }
      this.toggleModePressed({'target': this.switchButton})
    }.bind(this));
    // end of testing code
  },


This code help us switch mode and press shutter button when buttons are enabled. The STR with this code is:
1. open camera app
2. wait shutter sound
3a. when no more shutter sound heard, kill and reopen camera app for double confirm. If it only shows the shutter button, it is affected. If it shows 3 buttons, and we will need to redo the test.
3b. If we heard more than 10 times, kill camera app and redo the test.


The reproducing ratio is about 1/5. If we dump the logcat lively, we may found the crash message of media server when no more shutter sound heard.

BTW, I had tested the patch made by David. The camera app works. It will take picture until SD card is full.

Comment 55

[John Hu [:johnhu][:johu][:醬糊小弟]]

Another thing: we should be able to take pictures forever with the above testing code. But it doesn't. Camera app takes about 50 pictures and stops. That's why we need to restart the camera app for double confirm.

After the patch, it still can't take picture forever. And the symptom looks the same 30 pictures and stopped. But we can't find crash message in the logcat in this time. I think there may be another race condition inside Camera app and mostly likely in gaia part.

Comment 56

[David Flanagan [:djf]]

Comment on attachment 784600 [details]
Gaia portion of the patch

John,

I've updated the pull request to use onPreviewStateChange in loadCameraPreview(). Thanks for pointing that out.

As part of this change, I modified the _camera property of the Camera object to be called _cameraNumber. This makes the code a lot clearer, I think.

Does this fix the bug for you?

Comment 57

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 784600 [details]
Gaia portion of the patch

Thanks, David.

This version works good for both cases. And the change to cameraNumber makes sense. I was also confused about it before. r=me, 9eadf36.

Comment 58

[Sotaro Ikeda [:sotaro]]

(In reply to David Flanagan [:djf] from comment #51)
> Sotaro: did you have Mike's gecko patch applied when you tested? The
> behavior you describe is what I'd expect if you tested the gaia patch
> against an unpatched gecko.
> This is one of those bugs where we'll have to be sure that that gecko patch
> has landed before we apply the gaia patch.

Sorry, I totally forgotten about it. I re-confirm by new gaia patch soon.

Comment 59

[Sotaro Ikeda [:sotaro]]

I applied attachment 784600 [details] to master leo. This time I applied also gecko side patch. Mode change works correctly, but camera preview does not resume after taking a photo.

Comment 60

[Sotaro Ikeda [:sotaro]]

I am going to check more about Comment 59. Though I am now recreating the built from scrash :-(

Comment 61

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #59)
> I applied attachment 784600 [details] to master leo. This time I applied
> also gecko side patch. Mode change works correctly, but camera preview does
> not resume after taking a photo.

It is not related to the bug. Default Master LEO have the problem might be affected by Bug 858914, Bug 900012.

Comment 62

[Sotaro Ikeda [:sotaro]]

master leo can not used for testing from Comment 61. I checked the patches on master hamachi. STR in Comment 44 seems OK. But I find a problem in following STR. I am not sure it happened by same problem on this bug and if it is master dependent problem.

STR
-[1] Start Camera app
-[2] Change to video mode
-[3] start video recording and stop recording some seconds after the start.
-[4] soon change to camera mode and take a picture.

Comment 63

[Sotaro Ikeda [:sotaro]]

gecko's master have a problem around camera. So I checked STR in Comment 44 by gecko(b2g18) + gaia(master) on leo and hamachi. No problem for the STR.

Comment 64

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #62)
> STR
> -[1] Start Camera app
> -[2] Change to video mode
> -[3] start video recording and stop recording some seconds after the start.
> -[4] soon change to camera mode and take a picture.

The above STR did not happened on b2g18 gecko on leo and hamachi. I am going to create a bug for the STR.

Comment 65

[David Flanagan [:djf]]

Sotaro,

Do you think we should land the gecko and gaia patches, since they seem to fix the bug for 1.1, or do you think we need to investigate further and figure out what is happening on master?

Either way, I have re-assigned the bug back to you:

1) If you think we should land the patch, I don't know how to land gecko patches, and I don't have commit rights on m-c. If you land the gecko patch today, I'll land the gaia patch on monday to avoid people pulling gaia with an out of date gecko.

2) If you think we should keep investigating, it sounds like there are gecko issues that I won't be able to help with. In this case maybe we should re-assign it back to Mike, but since he is still on vacation, you get it for now.

Comment 66

[Sotaro Ikeda [:sotaro]]

Master has a lot of problems around graphics, camera and video. They can not be fixed until Bug 858914 is fixed. I am going to create a new bug and going to set blocked by the bug.

I always ask to check-in by using keyword. I am going to do it after passed try server test.

Comment 67

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=ca0b6e9a936c

Comment 68

[Sotaro Ikeda [:sotaro]]

Attachment: [v2][b2g18] Let the Camera app know when the preview has actually started/stopped

Update the patch's header. Carry "jst: review+".

Comment 69

[Sotaro Ikeda [:sotaro]]

Attachment: [v2][m-c] Let the Camera app know when the preview has actually

Update the patch's header. Carry "jst: review+".

Comment 70

[David Flanagan [:djf]]

Adding "KEEP OPEN" to the white board. Please don't close the bug when the gecko patch is checked in. There is a corresponding gaia patch that needs to land after the gecko patch has landed and made it to people's nightly builds.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4eef02979648

[leave open] is the magic incantation you're looking for :)

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4eef02979648

Comment 73

[Sotaro Ikeda [:sotaro]]

Comment on attachment 785317 [details] [diff] [review]
[v2][m-c] Let the Camera app know when the preview has actually

Carry "jst: review+".

Comment 74

[Sotaro Ikeda [:sotaro]]

Can the patch be uplifted to gecko v1-train?

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1433d653b701

Please resolve the bug and set the appropriate status flags when landing the Gaia patch on the appropriate branches.

Comment 76

[Sotaro Ikeda [:sotaro]]

djf, gecko side was committed. I assign you for remaining gaia side.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/1433d653b701

Comment 78

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/376d45c7afdbf97f75d55803e15fbb87931f3a50

Comment 79

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  376d45c7afdbf97f75d55803e15fbb87931f3a50
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 80

[David Flanagan [:djf]]

Weird. The needinfo above does not show up on my dashboard. Maybe because the bug is closed?

Comment 81

[David Flanagan [:djf]]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/3335e3ba4e8bb6965112514f00e8ba4e312ab540

I don't know if anyone is still using the v1-train branch, but even if not, this revised patch will be easier to cherry-pick to the hd branch.

needinfo jhford in case he needs to know that this can now be uplifted for the hd branch.

Comment 82

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

v1.1.0hd: 3335e3ba4e8bb6965112514f00e8ba4e312ab540
v1.1.0hd: 9df4b6b2e06b0c72611e20b43c321ee89742fd70

Comment 83

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to David Flanagan [:djf] from comment #81)
> needinfo jhford in case he needs to know that this can now be uplifted for
> the hd branch.

The HD Branch team is taking care of the v1-train to v1.1.0hd branch merges, and it was completed :)