1024384 - [Flame] When changing default media storage from Internal to SD Card, the change is not effective till a power off cycle

Status: VERIFIED FIXED

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

Description

[isabelrios]

Seen on Flame with 2.0 build:
Gecko-a39cee4
Gaia-26d8fca

STR
Use a Flame device with sd card inserted.
-Go to Settings -> Media storage -> Check that the default media location is: Internal memory.
-Take some pictures
-Check that the pictures are stored there, in the Internal memory
-Then go to Settings -> Media storage -> and change the default media location to: SD Card
-Take some more pictures
-Check where those pictures are stored

EXPECTED
The pictures are stored in the memory selected as Default media location, Internal memory or SD Card.

ACTUAL
If the Default media location is Internal memory and user takes some pictures, then changes the default media to SD Card memory, then the new pictures are not stored in the SD Card but in the Internal memory. 
The change of the default memory to be used is not effective. In order to get the change done and so the pictures stored in the SD Card, it is necessary to power the device off and on.

MORE INFO:
If the change from Internal memory to SD Card is done without taking any picture first, when Internal memory is the default one, then the problem is NOT reproduced.

This was reported in the past, see bug 914461, although it was closed as WFM this is still happening.

Comment 1

[Juan Perez-Bedmar [:juanpbf]]

Although this bug has not been marked as a blocker during the certification, the Operator has increased the priority and has warned us that it will become a showstopper once 2.0 version comes. 

Therefore I'm nominating it.

Comment 2

[Jason Smith [:jsmith]]

QA Wanted for branch checks.

Comment 3

[Duane Dixon [:ddixon]]

I was unable to reproduce this bug in the latest 2.1, 2.0, 1.4 Flame builds. 

Flame 2.1 

Device: Flame Master
Build ID: 20140703045555
Gaia: d7a517f0bde32072f1799e4a47ea34c6d786c287
Gecko: ac6960197eb6
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
---------------------------------------------------------------------------------------------------------------------

Flame 2.0 

Device: Flame 2.0
Build ID: 20140703100253
Gaia: 5725321dd1aef29077b6fc5c4c49b43dccf208dc
Gecko: 9146671aac33
Version: 32.0a2 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
---------------------------------------------------------------------------------------------------------------------

Flame 1.4 

Device: Flame 1.4
Build ID: 20140703063016
Gaia: 71aa8a3697e8daacdaee3d447a38ee10f13d5b54
Gecko: 1bae550358a6
Version: 30.0 (1.4)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Comment 4

[Joshua Mitchell (Inactive)]

Hi Isabel - because this issue was already written and closed WFM, I do not want to do that to this bug prematurely. My testers have been unable to repro this in the latest. Based on the length of time that has passed since this bug was written we are probably dealing with new firmware and the like. Could I get you to attempt a repro with the latest Master and Base?

Comment 5

[Beatriz Rodríguez [:brg]]

Isabel had just started her maternity leave. Changing the ni to Junjo so he can provide some feedback here.

Comment 6

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

Triage: waiting for needinfo.

Comment 7

[Juanjo Iglesias]

I tested using  Flame 2.0 (build identifier 20140703200131 with Firmware Version: v122)

I have done the follow steps to repeat the bug:
1 Go to Settings -> Media storage -> Check that the default media location is: Internal memory.  (Look screenshot_init.png)
2 Take some pictures, for example three. 
3 Check that the pictures are stored there, in the Internal memory
 (Look screenshot_ChangeInternalToSD.png)
4 Then go to Settings -> Media storage -> and change the default media location to: SD Card
5 Take some more pictures
6 Check where those pictures are stored in Internal memory, not in SD Card. (Look screenshot_SD_end.png)

All pictures are stored in the internal memory while the latest must be in SD card.

In order to reproduce this issue first you need to take some pictures first with Internal memory like Default media location, and then change this to sd card value.

Comment 8

[Juanjo Iglesias]

Attachment: screenshot_ChangeInternalToSD.png

Comment 9

[Juanjo Iglesias]

Attachment: screenshot_init.png

Comment 10

[Juanjo Iglesias]

Attachment: screenshot_SD_end.png

Comment 11

[Jason Smith [:jsmith]]

QA Wanted to try branch checks again following comment 7's suggestion.

Comment 12

[Duane Dixon [:ddixon]]

I was unable to reproduce the bug on 2.1, 2.0, 1.4 builds (tested both engineering and non-engineering builds).

DOES NOT occur on Open_C 2.1 build. 

Bug does not apply to Buri device because user cannot store media files on Internal Memory. 

I used the most recent STR in comment 7.  
--------------------------------------------------------------------------------------------
Flame 2.1 DOES NOT repro 
 
Device: Flame Master
Build ID: 20140707060819
Gaia: 99f56d9db3cd37c684b01de6fed786421f47e2b7
Gecko: 085eea991bb9
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
--------------------------------------------------------------------------------------------
Flame 2.0 DOES NOT repro

Device: Flame 2.0
Build ID: 20140707064631
Gaia: 9b0f757efaf9fbd60dd932da0ff9b9182c8b8ed9
Gecko: c87c07dc23b3
Version: 32.0a2 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
----------------------------------------------------------------------------------------------
Flame 1.4  DOES NOT repro

Device: Flame 1.4
Build ID: 20140706160201
Gaia: 5c9e1e4131d3ac8915ed88b72bb66dc7d97be6a0
Gecko: 2d0c15450488
Version: 30.0 (1.4)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0
---------------------------------------------------------------------------------------------------
Open_C 2.1 DOES NOT repro

Device: Open_C Master
Build ID: 20140707060819
Gaia: 99f56d9db3cd37c684b01de6fed786421f47e2b7
Gecko: 085eea991bb9
Version: 33.0a1 (Master)
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Comment 13

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

Triage: this is a WFM bug because it does not reproduce.

Comment 14

[Beatriz Rodríguez [:brg]]

We can easily reproduce this bug with different HW and versions. We will post a video to show it later (please Juanjo).

Can the QA-tester write down the STR followed and/or post a video?

Comment 15

[Juanjo Iglesias]

I have recorded the video and repeat the test using: 

Flame 2.0
Gecko: a39cee4
Gaia: 26d8fca
Platform Version: 30.0a1
Build Id. : 20140708054110

With this video, I show the steps to reproduce the bug. 

VIDEO: 
https://www.dropbox.com/s/itdxme2hh04l45k/MOV_0060.mp4

Download video: https://db.tt/NeHE0tlP

Comment 16

[Duane Dixon [:ddixon]]

Branch Check: 

After further testing, I found that Settings App and Camera App MUST NOT be closed during the repro of this issue.  

If user closes Settings and Camera Apps while switching between them, the issue DOES NOT reproduce which determines my previous findings in comment 3 and comment 12.  (Sorry for the confusion.)

STR used (same STR as comment 7 with apps kept open): 

1. Go to Settings>Media Storage>Default media location=Internal
2. Tap home>open Camera>take a photo>tap home>Go to Settings
3. Change Default media location to "SD Card" 
4. Go to Camera>take 3 photos>tap home>Settings>Media Storage
5. Observe that photos do not save in proper location.  


Issue DOES reproduce on Flame 2.1, 2.0, 1.4 builds and Open_C 2.1
Issue DOES NOT reproduce on Flame 1.3 base image
Issue is not applicable for Buri device. 
-----------------------------------------------------------------------------------------------
Flame 2.1 Repro: Yes

Device: Flame Master
Build ID: 20140708061447
Gaia: 740faa5d0060fb218b407cf224330654ddf833a5
Gecko: 8bfe3372f848
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
-----------------------------------------------------------------------------------------------
Flame 2.0  Repro: Yes

Device: Flame 2.0
Build ID: 20140708013713
Gaia: d96dffda5a72906acd44840716d11bae6b2c7988
Gecko: 872c1c947b13
Version: 32.0a2 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
-----------------------------------------------------------------------------------------------
Flame 1.4 Repro:  Yes

Device: Flame 1.4
Build ID: 20140706160201
Gaia: 5c9e1e4131d3ac8915ed88b72bb66dc7d97be6a0
Gecko: 2d0c15450488
Version: 30.0 (1.4)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0
-----------------------------------------------------------------------------------------------
Open_C 2.1 Repro: Yes

Device: Open_C Master
Build ID: 20140707060819
Gaia: 99f56d9db3cd37c684b01de6fed786421f47e2b7
Gecko: 085eea991bb9
Version: 33.0a1 (Master)
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
-----------------------------------------------------------------------------------------------
-----------------------------------------------------------------------------------------------
DOES NOT reproduce on Flame 1.3 (Base Image)

Flame 1.3 Base Image 

Device: Flame 1.3
Build ID: 20140616171114
Gaia: e1b7152715072d27e0880cdc6b637f82fa42bf4e
Gecko: e181a36ebafaa24e5390db9f597313406edfc794
Version: 28.0 (1.3)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:28.0) Gecko/28.0 Firefox/28.0

Comment 17

[Beatriz Rodríguez [:brg]]

Thanks for the effort to reproduce the bug. Renominating per comment 1.

Comment 18

[Duane Dixon [:ddixon]]

Unable to provide Regression Window. 

Issue occurs in earliest Master Flame build we have access to. 

Environmental Variables:
Device: Flame Master
Build ID: 20140417000006
Gaia: 7591e9dc782ac2e97d63a96f9deb71c7b3588328
Gecko: e71ed4135461
Version: 31.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:31.0) Gecko/31.0 Firefox/31.0

Comment 19

[Joshua Mitchell (Inactive)]

Regression-window not available, not able to do in Buri either (bug does not repro there)

Comment 20

[howie [:howie]]

Hi Ian, mind taking a look? thanks

Comment 21

[[e-mail], [e-mail]]

Will take a looks first.

Comment 22

[[e-mail], [e-mail]]

Just have a test on Helix with following build version. The symptom is same with flame. Camera have to re-launch after default storage is changed. Then, the new picture we just take is saved in the default storage. I guess Gecko::deviceStorage might not monitor the change event for settings key 'device.storage.writable.name'. Or something is broken for changed key to set default storage immediately from Gecko. I will need some help in part of Gecko side.

Gaia      3d4b4b06475d2376bad23ac46da185cd48a68d17
Gecko     https://hg.mozilla.org/mozilla-central/rev/dd50745d7f35
BuildID   20140416040204
Version   31.0a1
ro.build.version.incremental=eng.root.20140214.225714
ro.build.date=Fri Feb 14 23:00:24 CST 2014

Comment 23

[[e-mail], [e-mail]]

Alphan, could you please help to take a look in Gecko side? Thanks.

Comment 24

[[e-mail], [e-mail]]

Since Gaia settings key is working fine as before(no other relative bug recently), I deassign to leave investigation.

Comment 25

[Eric Chou [:ericchou] [:echou]]

Hi Dave,

Would you mind taking a look? Please feel free to ping us if there is anything we could help.

Comment 26

[Dave Hylands [:dhylands]]

I have a feeling that the camera app is grabbing navigator.getDeviceStorage once, and not updating it ever.

There are 2 storage areas. navigator.getDeviceStorage returns the one that is marked as the default.

If the user changes the default, then you need to call navigator.getDeviceStorage again to get the new default storage area.

So navigator.getDeviceStorage is behaving like a file open, and changing the default is like changing the current directory.

So I think that camera app should listen for changes of the default storage area setting and call navigator.getDeviceStorage again to update its internal copy.

Comment 27

[howie [:howie]]

Hi Hema, could camera app make the change as Comment 26 mentioned? Thanks.

Comment 28

[[e-mail], [e-mail]]

Per comment 26, it looks like an incomplete feature, might not regression. Did Camera app have some code base change relative navigator.getDeviceStorage(keep one instance only) after v1.3(per comment 16, test on flame v1.3)? The default device storage settings is existed from v1.1.0hd. Otherwise, there is no reason to broken the feature for a long time. Will need Camera app devs for investigation. Remove ni from Alphan.

Comment 29

[Dave Hylands [:dhylands]]

The behaviour of getDeviceStorage hasn't changed since it was implemented to support multiple volume (i.e. 1.1 timeframe).

However, the users of deviceStorage may have been tweaked to not call navigator.getDeviceStorage for every picture, which would have changed the perceived behaviour.

Comment 30

[Hema Koka [:hema]]

Wilson/David, Can one of you take a look please?

Thanks
Hema

Comment 31

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

Sounds to me like a Camera app bug from comment 26.

Comment 32

[Aus Lacroix [:aus]]

I actually have some spare cycles right now so I can pick this up.

Comment 33

[Aus Lacroix [:aus]]

Attachment: Pull Request - Listen to default storage location change, update our device storage instances when needed.

Comment 34

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8456120 [details] [review]
Pull Request - Listen to default storage location change, update our device storage instances when needed.

Switching reviewer to Diego because he has a similar patch as well.

Comment 36

[Diego Marcos [:dmarcos]]

Comment on attachment 8456120 [details] [review]
Pull Request - Listen to default storage location change, update our device storage instances when needed.

Looks good! Thanks!

Comment 37

[Diego Marcos [:dmarcos]]

Comment on attachment 8456120 [details] [review]
Pull Request - Listen to default storage location change, update our device storage instances when needed.

Ooops! Unit tests are failing. Once they pass we can ship it

Comment 38

[Diego Marcos [:dmarcos]]

Attachment: Pull request with unit test fixed

Comment 39

[hyuna.cho]

Hi, Diego
Don't you change the storage location in lib\camera\camera.js?
When start the video recording, video storage passed to mozCamera.startRecording in this file.

I'm checking this issue in our device.
If we change storage.js only, the video file stored in the previous storage location and only the video thumbnail image is stored in the changed location.

Comment 40

[hyuna.cho]

(In reply to Diego Marcos [:dmarcos] from comment #38)
> Created attachment 8456524 [details] [review]
> Pull request with unit test fixed

I tried to do this way.
But the device storage name doesn't update using observing 'device.storage.writable.name'.
It update the setting DB not update DeviceStorage.storageName.
You can find my means adding the below debug code.

Storage.prototype.updateStorage = function() {
...
  this.video = navigator.getDeviceStorage('videos');
  this.picture = navigator.getDeviceStorage('pictures');
  debug('video storage volume: %s', this.video.storageName); // Do NOT update to changed stoage
  debug('picture storage volume: %s', this.picture.storageName); // Do NOT update to changed stoage
...
};

I heard from gecko engineer that there is no event for changing the default storage.
So call navigator.getDeviceStorage() whenever get the device storage information.
I think it's not good way. Do you have any idea to handle this?

In v1.1 version, it handled in gecko side.

Comment 41

[Aus Lacroix [:aus]]

(In reply to hyuna.cho from comment #40)
> (In reply to Diego Marcos [:dmarcos] from comment #38)
> > Created attachment 8456524 [details] [review]
> > Pull request with unit test fixed
> 
> I tried to do this way.
> But the device storage name doesn't update using observing
> 'device.storage.writable.name'.
> It update the setting DB not update DeviceStorage.storageName.
> You can find my means adding the below debug code.
> 
> Storage.prototype.updateStorage = function() {
> ...
>   this.video = navigator.getDeviceStorage('videos');
>   this.picture = navigator.getDeviceStorage('pictures');
>   debug('video storage volume: %s', this.video.storageName); // Do NOT
> update to changed stoage
>   debug('picture storage volume: %s', this.picture.storageName); // Do NOT
> update to changed stoage
> ...
> };
> 
> I heard from gecko engineer that there is no event for changing the default
> storage.
> So call navigator.getDeviceStorage() whenever get the device storage
> information.
> I think it's not good way. Do you have any idea to handle this?
> 
> In v1.1 version, it handled in gecko side.

True, doesn't look like this works for videos. I'll try and see what's going on.

Comment 42

[Aus Lacroix [:aus]]

Comment on attachment 8456120 [details] [review]
Pull Request - Listen to default storage location change, update our device storage instances when needed.

Updated, fixes all remaining issues. Please read the code comments!

Comment 43

[Diego Marcos [:dmarcos]]

Comment on attachment 8456120 [details] [review]
Pull Request - Listen to default storage location change, update our device storage instances when needed.

Thanks for the patch!

r-ed because:

1. lib/camera/camera.js should not be coupled with the Storage module.
2. We purposefully avoid setters and getters because they make code difficult to understand. Side effects when assigning or retrieving a variable are going to always surpr

I made a new PR that addresses the issues above.

Comment 44

[Diego Marcos [:dmarcos]]

Attachment: PR that addresses issues that hyuna pointed out

Comment 45

[Diego Marcos [:dmarcos]]

Comment on attachment 8457506 [details] [review]
PR that addresses issues that hyuna pointed out

Does this solve the issue you pointed out when recording video after changing the storage volume?

Comment 46

[Wilson Page [:wilsonpage]]

Comment on attachment 8457506 [details] [review]
PR that addresses issues that hyuna pointed out

Conditional r+, depending on discussed points.

Comment 47

[Diego Marcos [:dmarcos]]

I updated the PR to address some remaining issues during camera initialization. Hyuna, Can you confirm it this works on your side?

Comment 48

[hyuna.cho]

(In reply to Diego Marcos [:dmarcos] from comment #47)
> I updated the PR to address some remaining issues during camera
> initialization. Hyuna, Can you confirm it this works on your side?

I checked on flame and our device.
It works well.
Thanks!

Comment 49

[Diego Marcos [:dmarcos]]

Landed on master:

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

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

Comment 51

[Juanjo Iglesias]

I have tested using today builds with: 

- Flame master: Gecko c431020, Gaia 1c9eb3d.
- Flame 2.0: Gecko 5287d8d, Gaia 8cb1a94.

In both cases, the pictures are stored in the right places, which is indicated by default media location value without having to power off/on the device 

Now, if we change the internal memory to SD card in the default media location option... the new pictures taken will be stored in SD card.

Comment 52

[Elie]

Attachment: VIDEO0072_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        20141127000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2


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