890482 - [Gallery] Images from only internal memory is displayed, when media storage is made active-inactive

Status: RESOLVED WONTFIX

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

Description

[Leo]

1. Title: Only images from internal memory are dislplayed in Gallery, when storage resumes back from USB sharing
2. Precondition: Have a couple of images in Internal and External Memory
3. Tester's Action: 
	1. Place images in sd card and internal memory
	2. check if gallery displays all images
	3. goto settings->enable usb mass storage.
	4. goto media storage->enable all the options["enable USB storage" , internal storage -share using USB,SD storage-share using USB]
	5. Open gallery->"unplug phone to use the app" is displayed .
	6. Now, goto setting and only disable "internal storage - share using USB" and "SD storage - share using USB"
	7. Only images on internal storage are displayed
4. Detailed Symptom (ENG.): Only internal memory images are displayed, when USB sharing is made from active to inactive state.
5. Expected : All images stored should be displayed
6. Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 0a8b1d7c70c5ec2b48e52c2cd7e463f47fa0b9cb

Comment 1

[Leo]

Hi djf, 

I have added the below mentioned code in mediabd.js file,

########## ORIGINAL CODE ###################
function getState(availability) {
....
....
        if (s > 0)
          return MediaDB.UNMOUNTED;

        // If all volumes are unavailable, then MediaDB is unavailable
        if (u === n)
          return MediaDB.NOCARD;
....
....
		
##############################

########## Modified code ###################
function getState(availability) {
....
....
        if (s > 0)
          return MediaDB.UNMOUNTED;

        // If all volumes are unavailable, then MediaDB is unavailable
        if (u === n)
          return MediaDB.NOCARD;
		
	if (a < 2) {
          return MediaDB.UNMOUNTED;
         }
...
...

##############################


After adding this piece of code, images from internal and External memory are displayed.

I need your opinion and suggestions on this defect and code added.

Thanks,

Comment 2

[Dominic Kuo [:dkuo]]

Leo, this issue looks like the same as bug 884688, can you help to investigate if they have the same root cause? or does your patch also solve bug 884688? thanks.

Comment 3

[Leo]

We dont think this is similar to bug 884688. Here the issue is that scanning/available event is not happening for SD card storage. In bug 884688 we are getting two ready event and thus two scanning with some delay.

Comment 4

[Leo]

This issue is happening for videos in videoplayer also.

Comment 5

[Leo]

David, Please let me know if the code snippet mentioned in Comment 2 is fine enought for this issue.

Comment 7

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

Leo, 

Please find the comment at bug 885132 comment 6 which was wrote by Mark Shiao. That's the reason Dominic thought this one is related to bug 884688. The second scan is canceled by the first one. This is the design defect in mediadb which uses single flag to handle multiple volume states.

I think your patch in comment 2 can solve this bug and introduce some regressions. Mark Shiao had discussed this kind of patch with me. The patch may change the original design of mediadb, please find that at bug 884688 comment 23. The original state looks like:

Internal State       External State    Media State
shared               shared            unmounted
unavailable          shared            unmounted
available            shared            unmounted
available            unavailable       ready <===  Key is in this one state.
available            available         ready 

The patch changes the state of available/unavailable to unmounted state, like:

Internal State       External State    Media State
shared               shared            unmounted
unavailable          shared            unmounted
available            shared            unmounted
available            unavailable       unmounted <===  Key is in this one state.
available            available         ready

That's the root cause of two ready state dispatched to client apps which causes music app scan twice and this one.

But, if we apply the patch, the device can't access mediadb while no sdcard inserted. This is the key issue of this patch.

As I said at first part, this bug is related to the flag issue. If we don't care about the state problem, we can also uses different flags to monitor the full scan of different volumes which solves this bug. But we should use more complete way to solve it, like your patch which is trying to solve the real root cause, but not totally. It's a complex bug.

I leave the need info flag of David to wait for his comments on this one.

Comment 8

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

Forgot one thing, there is a video of this bug which is at the duplicated bug 885132. URL: https://bugzilla.mozilla.org/attachment.cgi?id=765090

Comment 9

[David Flanagan [:djf]]

Dave,

I haven't made the time to look deeply at this yet, but could you take a look at the state change event issues mentioned here and in bug 884688?  This might be something that is best fixed in Gecko rather than trying to workaround in Gaia.

Comment 10

[Dave Hylands [:dhylands]]

So, I was going to fix 884688 by changing mediadb to do its scanning by volume.

It would have a queue of scan requests. Initial scanning would queue up a request per volume, and then volume chnaged events would queue up requests just for the volume that changed.

Comment 11

[David Flanagan [:djf]]

So the issue here is with the following code at mediadb.js:775

        else if (newState === MediaDB.READY) {
          // In this case, the state did not change. But we may still need to
          // send out an event. If both states are READY, then the user
          // just inserted or removed an sdcard. If the user just added a
          // card, then we want to send another available event and start a
          // scan.  If the user just removed a card then we need to immediately
          // tell the client that happened so the music app (for example) can
          // stop playing. If it is playing a file that just disappeared it is
          // in danger of crashing. Also, in this case we must delete the
          // records (and send events) for all of the files on that card.
          if (e.reason === 'available') {
            // An SD card was just inserted, so send another ready event.
            dispatchEvent(media, 'ready');

            // And if we're automatically scanning, start the scan now.
            // It would be more efficient if the scan() function could scan
            // just one storage area at a time. But SD card insertion should
            // be rare enough that efficiency is not so important.
            if (media.autoscan)
              scan(media);
          }

This is the code that handles sdcard insertion when the mediadb is already available. It sees that a new volume has been added and rescans.  Its inefficient to do a complete scan, but I stand by my initial judgement that it is rare enough that we don't really have to do a per-volume scan.

The bug here, is that the gallery has autoscan disabled. It wants to control scanning on its own, and doesn't want the scan to start until after it has gotten the known photos displayed.

Without autoscan on, the gallery app isn't receiving any notification that this sdcard has been inserted.  See the comments around line 302. I added a new 'cardremoved' event to handle the card removed but still available case. Maybe I should have added a 'cardadded' event to use here. Gallery could listen for it and use that to trigger a new scan.

Or maybe gallery can be modified so that when it recieves a 'ready' event from a mediadb that is already ready, it triggers a new scan.

Or, maybe gallery could be modified to just use autoscanning. But we'd have to lookout for regressions in startup time (where startup time includes the time required to display the first page of thumbnails).

Comment 12

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

The gallery already had the code to trigger a new scan when it receives a ready event[1]. In the onready handler of gallery, it calls initThumbnails method. initThumbnails method calls photodb.scan when visibilityMonitor is not null or enumerating is done.

https://github.com/mozilla-b2g/gaia/blob/8b54efdd03caf458b5a5503660389056c474aa4a/apps/gallery/js/gallery.js#L271

Comment 13

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

Make this bug depend on bug 884688. Once the gecko patch of that bug is done, we will test both bug 884688 and this one.

Comment 14

[Alex Keybl [:akeybl]]

Leo - when do you need this resolved by to make it into 1.1.0?

Comment 15

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

I never found Dave already have a bug for removing intermediate unavailable events. I will change this bug to depend on bug 878310.

Comment 16

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

Attachment: device behavior after patch 878310 applied

This video is the validation result of this bug after the patch of bug 878310 is applied. The first three images are stored in sdcard, and others are in internal storage. The behavior of mediadb runs as expected:
1. keep all items when usb is connected. Before the patch, mediadb removed all of them because of unavailable events.
2. scan once when usb is disconnected. Before the patch, mediadb dispatched two ready events to client app which causes overriding of flag. I will open a follow-up bug to scan volume separately.
3. sdcard insertion/removal with usb connected can be handled by the scanning of mediadb. They are checked by the scan function call when USB is disconnected.
4. sdcard insertion/removal without usb connected still work.

I will set this bug as won't fix when bug 878310 is landed. Thanks, Dave.

Comment 17

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

This bug is already fixed by the patch of bug 878310.

Comment 18

[Leo]

We are not considering major changes at this point of time.