1036259 - [dolphin] unable to take video after removing and insert SD card again

Status: RESOLVED FIXED

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

Description

[Peipei Cheng (needinfo if you need my action)]

Dolphin's SD card holder is on the side of the device. So user could remove it and insert another one easily.

Steps to reproduce
-----------------------------------------------------------------------
0. Insert a SD card and set default storage to SD card
1. Launch Camera and start recording
2. Remove SD card
   --> “no memory card found” overlay appears. But you can see the camera recording is on going at the backgourd. After a while, camera was killed automatically.
3. Insert SD card
4. Launch Camera again
5. Try to start recording
   --> Recording cannot be started.

According to spreadtrum developer, camera does not handle the case for removing SD card.

Test build
------------------------------------------------------------------------------
Gaia      5c9e1e4131d3ac8915ed88b72bb66dc7d97be6a0
Gecko     a9ee8ae4cca454deb67405fe07de84e713834e05
BuildID   20140707144650
Version   30.0
ro.build.version.incremental=272
ro.build.date=Mon Jul  7 14:44:46 CST 2014

Comment 1

[Vincent Liu[:vliu]]

When I looked into camera app, I found onStorageStateChange handles all responds to storage `statechange` events. When I tried to reproduce this case, onStorageStateChange received |unavailable| event and nothing to do because the below conditions.

CameraController.prototype.onStorageStateChange = function(value) {
  if (value === 'shared' && this.camera.get('recording')) {
         this.camera.stopRecording();
   }
};

I think this part of implementation should also consider removing SD card while camera recording. I also tried to change the detected event to |unavailable| and let it run stopRecording when removing the SD card. When camera got back to preview screen, all buttons were not responsive. I am not familiar with the code for camera app and @Justin may help me to figure out this issue.

Comment 2

[Marcia Knous [:marcia]]

I tried flashing the latest Dolphin build and was having trouble testing this - it seems the camera/video functionality is not very responsive. Will likely need to test with another build.

Comment 3

[Ivan Tsay (:ITsay)]

Hema,

Mind if can help to find a engineer to take a look this 1.4 blocker on camera app?

Comment 4

[Hema Koka [:hema]]

Diego, please help investigate

Thanks
Hema

Comment 5

[Dave Hylands [:dhylands]]

Note: that the flame doesn't have the HW to detect sdcard insertion/removal, but the dolphin does.

So you won't be able to test this on a flame.

Comment 6

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

As Marcia mentioned I'm having problems to get my dolphin to a workable state. Vincent, Does the attached patch solve the issue for you?

Comment 7

[Rachelle Yang [:ryang][[e-mail]]]

Hi Vincent, Could you please kindly check if the patch works ?
Thank you so much !!

Comment 8

[Vincent Liu[:vliu]]

Sorry about late response because I got stuck on other issue. When I finished the test, I will update it.

Comment 9

[James Zhang (Spreadtrum)]

Hi Jinchao, can you help to verify this patch?

Comment 10

[jinchao.wang]

(In reply to James Zhang (Spreadtrum) from comment #9)
> Hi Jinchao, can you help to verify this patch?

Hi James:
    The patch is not work well. It can stop recording but controls button is disabled too.When insert SD card ,controls button states don't update to enable.

Comment 11

[Justin D'Arcangelo [:justindarc]]

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

Looks good! I am able to pull out the SD card while recording and see that recording stops in the background. Then after re-inserting the SD card I can record again. Thanks Diego!

Comment 12

[Diego Marcos [:dmarcos]]

Attachment: Pull Request Master

Comment 13

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8462852 [details] [review]
Pull Request Master

Patch looks good on master. But I don't have a device appropriate for testing it on master.

Comment 14

[Diego Marcos [:dmarcos]]

Landed on 1.4:

https://github.com/mozilla-b2g/gaia/commit/eddef9a416a9f9517dda7209fa05d86ceb76cac9

Comment 15

[Diego Marcos [:dmarcos]]

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/5b9936f9c1531dbfedbef8fda4eb5d968f98270c

This needs to be uplifted to 2.0

Comment 16

[Jason Smith [:jsmith]]

Switching needinfo to rel man, as this is a rel man question.

Comment 17

[Vincent Liu[:vliu]]

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

It seems sprd had verified your patch so I marked it as feedback- since I didn't do anything. Really hope I can help you next time. :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/2764488eb92e120b42a55a81d482c4f44bc3597a

Comment 19

[bhavana bajaj [:bajaj]]

Unless this is reproducible on Flame 2.0 which may not be true per comment #5, I was poked by CAF to back this out. Diego, this seems to be dolphin specific so lets resolve this on master at this time, if we have a need for 2.0 for this in future we'll have to backport it then.

NI, ryan here to help with the backout.

I've also updated https://wiki.mozilla.org/Release_Management/B2G_Landing#Versions_and_Scheduling recently.

Comment 20

[Hema Koka [:hema]]

When the 2.0 uplift request was made, this page: https://wiki.mozilla.org/Release_Management/B2G_Landing#v2.0.0 did not show the restriction for uplifting. It had "1.4+ blocking bugs have auto-approval to land on 2.0 if affected and do not need additional approval". This is updated today (Thanks Bhavana for keeping the landing wiki up-to-date)

Ryan, please help back out the uplift and if it is required for 2.0, we will explicitly request with approval gaia-2.0 flag. Sorry about this.

Thanks
Hema

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Reverted from v2.0. Please be sure to set the status back to affected if you end up wanting to land this on 2.0.

v2.0: https://github.com/mozilla-b2g/gaia/commit/3ed23f27234f9866fa18282a490a6afb46f0386c

Comment 22

[Shine(Leave from Mozilla)]

Attachment: video

Dear Mike,
This bug has been verified to fail on Flame 2.1.
See attachment: Flame2.1_logcat_1712.txt & Verify_1036259.MP4
Reproducing steps: 
1. Insert a SD card and set default storage to SD card
2. Launch Camera and start recording
2. Remove SD card
   -->The video won’t stop recording. (But mount SD card to PC to check the video, the video have been damaged.)
3. Insert SD card
4. Stop recording and go back to Home screen.
5. Launch Camera again
5. Try to start recording
   --> Recording cannot be started.

Flame 2.1 build:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0

Comment 23

[Shine(Leave from Mozilla)]

Attachment: logcat

Comment 24

[Mike Lien[:mlien]]

Refer to comment 5, Flame cannot be taken as the verification device here. I'll find another device to do this.

Comment 25

[Mike Lien[:mlien]]

Verified with Helix v2.1, it cannot be reproduced

Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     helix
FW-Release      4.0.4.0.4.0.4
FW-Incremental  eng.cltbld.20141205.034558
FW-Date         Fri Dec  5 03:46:06 EST 2014

Comment 26

[Dave Hylands [:dhylands]]

(In reply to Shine from comment #22)
> Created attachment 8531541 [details]
> video
> 
> Dear Mike,
> This bug has been verified to fail on Flame 2.1.

Which is irrelevant. On the flame, you're not allowed to remove or insert the sdcard when the device is powered on. The flame doesn't have the hardware to support card insertion/removal. So any bugs which involve inserting/removing an sdcard while the phone is powered on are invalid.

If my memory is correct, the dophin does have the proper card insertion/removal detection (since it's one of those click-in click-out style connectors.

Comment 27

[Dave Hylands [:dhylands]]

Sorry I missed that this was fixed/resolved, you can ignore my last comment.