864674 - [tarako][dolphin][flame]gallery app show previous pictures from cache when delete pictures from pc.

Status: RESOLVED WONTFIX

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

Description

[renfeng.mei]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.22 (KHTML, like Gecko) Chrome/25.0.1364.172 Safari/537.22

Steps to reproduce:

1. put some pictures in sdcard.
2. open gallery app, view the thumbnails.
3. close gallery app. delete pictures from pc.
4. restart gallery app to view the thumbnails.


Actual results:

see the previous thumbnails in the first eye, it disappered after a while.


Expected results:

should not display the thumbnails which is not existed in sdcard.

Comment 1

[Steven Yang [:styang]]

Hi Ian,

The cache should be cleaned up in this case, could you help to check it? Thanks!

Comment 2

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

I can reproduce the issue in current Gaia master. Will need to investigate the mechanism of refreshing the cache.

Comment 3

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

Hi Steven,
Gallery app will monitor media libraries' event. Once having a modified event from media libraries, it will notify app to refresh thumbnail. According reproduced steps, it looks like a normal scenario to refresh thumbnail. Gallery app just do a process of refreshing thumbnail when it is launching again.

Comment 4

[renfeng.mei]

It will refresh but the first sight will be seen which is the old previews.
Is it possible that when changes found, no old preview?

Comment 5

[Jason Smith [:jsmith]]

Not a regression, so this is not a blocker.

Comment 6

[Jack Liu]

I think it's some thing related to user privacy, please solve it in v1.3.

Comment 7

[Preeti Raghunath(:Preeti)]

What is the user impact? If we've lived with ti for so long why is is a concern now?

Comment 8

[Jason Smith [:jsmith]]

Discussed via Vance - there's agreement now this isn't a blocker.

Comment 9

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

Leave assigned since I am working on other bugs.

Comment 10

[David Flanagan [:djf]]

The only way to fix this would be to check file existence for every thumbnail as we enumerate the database at application startup time. I think (but I have not tried) that this would cause an unacceptable regression in the startup performance of the app.

I'm sympathetic to the idea that there are privacy issues here, but we've lived with this since 1.0 and it doesn't seem like the sort of change that should be made at this stage of the 1.4 release.

If someone wants to investigate the performance implications, the place to do that is in gallery.js inside the photodb.enumerate() call. You'll need to make an async device storage call to get the file and not do the batch.push() stuff until that device storage call is successful.

As I said, I expect the performance implications to be unacceptable. If someone investigates this and proves me wrong, then I'm willing to modify MediaDB to build this check in to the enumeration code. Doing the existence check during first enumeration would allow us to simplify the scanning code in MediaDB.

Ying Xu: since you're the person who renominated this bug, is there anyone at spreadtrum who can try out my suggestion above?

Comment 11

[Preeti Raghunath(:Preeti)]

This has been on for long that I'd like to get this fixed in 2.1 instead

Comment 12

[Punam Dahiya [:pdahiya]]

Taking this to put in fix to check file existence before displaying thumbnail as suggested in #comment 10 and investigate performance implications.

Comment 13

[ying.xu]

(In reply to David Flanagan [:djf] from comment #10)

> Ying Xu: since you're the person who renominated this bug, is there anyone
> at spreadtrum who can try out my suggestion above?

sorry. I didn't see this before.
I would find someone from sprd to follow this bug

Comment 14

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of Bug864674

Hi David

Attaching PR with suggested fix in #comment 10. I have tested the patch on Flame with 306 images.

Time taken
- To scan 306 images without patch ~11 secs
- To scan 306 images with patch ~15 secs
- Delete 111 images and restart app with patch ~16 secs

No significant difference is seen in time to display first batch - "above the fold" content with the patch. 

Please provide your feedback. Thanks

Comment 15

[Punam Dahiya [:pdahiya]]

Hi Ying

It will be great if you can help with the performance testing of the attached patch on Tarako, Dolphin devices. Thanks

Comment 16

[David Flanagan [:djf]]

Punam,

You said "scan" but given the times, I'm guessing what you're describing is the time to enumerate the database and display the thumbnails, right? That should be the time between startup and when we actually call the scan method.  I'd be interested to see more precise measurements based on actually calling Date.now() when enumeration starts and stops.  And also measurements for 1000 photos.

The 11s to 15s increase is about 40% longer. I'm pleasantly surprised that it is not higher than that. As you point out, this doesn't impact the first screen time very much.  We enumerate and display about 30 photos per second without the patch and about 20 per second with the patch, which seems fast enough to keep up with most user's scrolling.

Paying the extra time during enumeration to detect deleted photos also helps us to avoid the jankiness of showing the thumbnails and them removing them afterward. With the introduction of MTP in 2.1, this may be a reasonable tradeoff.

I've left a couple of comments on github about the patch.  It is nice and simple and if Spreatrum wanted to take it into their tree to resolve this issue I would not object. (And I'd be interested in their feedback).

If we're going to take this approach in 2.1, I think I'd want to make the change in MediaDB itself. As a new feature switched on by an option passed to the MediaDB() constructor. The reason to do it that way is that if we detected deleted files during the enumeration pass then we may no longer need to do the fullScan() pass that serves primarily to find deleted files.

Punam: if you'd rather not make those mediadb changes yourself, feel free to reassign this to me.

Comment 17

[ying.xu]

I won't call getDeviceStorage too many times.

according to the url below, there is a memory leak.
https://bugzilla.mozilla.org/show_bug.cgi?id=1007520#c5

Comment 18

[ying.xu]

I test it simply on tarako

modify some code and using make reference-workload-heavy to push 1000 pictures into the phone.

launch the gallery ,wait scanning finished.

then remove sdcard ,reboot and relaunch the gallery. 

It took about 20s to display no photos tips, and before that ,there was a black screen to display.

Comment 19

[Punam Dahiya [:pdahiya]]

(In reply to ying.xu from comment #18)
> I test it simply on tarako
> 
> modify some code and using make reference-workload-heavy to push 1000
> pictures into the phone.
> 
> launch the gallery ,wait scanning finished.
> 
> then remove sdcard ,reboot and relaunch the gallery. 
> 
> It took about 20s to display no photos tips, and before that ,there was a
> black screen to display.

Thanks Ying for helping with performance testing of attached PR on tarako. For comparison, it will help if you can give time taken to display no photo tips without the patch for the above scenario.

Comment 20

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan[OOO until July 14] [:djf] from comment #16)
> Punam,
> 
> You said "scan" but given the times, I'm guessing what you're describing is
> the time to enumerate the database and display the thumbnails, right? 

That's correct, it is time to enumerate and display the thumbnails.

>That
> should be the time between startup and when we actually call the scan
> method.  I'd be interested to see more precise measurements based on
> actually calling Date.now() when enumeration starts and stops.  And also
> measurements for 1000 photos.
> 

I will update code and try to get precise performance numbers with large set of photos

Comment 21

[Punam Dahiya [:pdahiya]]

Here's more detailed performance number taken with and without attached patch for 1000 photos pushed on Flame device using make reference-workload-x-heavy

1. With attached patch
a) For 1011 pics and 4 files deleted - time taken is 10280 ms
b) For 1011 pics and 1000 files deleted - time taken is 6662 ms

In a) deleted files are not seen, however in b) where numbers of file deleted are large, black screen stayed for enumerated time ~ 6 secs :(

2. Without patch on latest master
a) For 1011 pictures and 4 files deleted - time taken is 6438 ms
b) For 1011 pictures and 1000 files deleted - time taken is 6666 ms

In a) deleted files are seen till scanning completes ~ 15 secs


The attached patch is effective for fewer files deletion (IMO more common use case) but for scenarios where we have bulk files deleted/ sdcard removed, black screen is seen which needs to be handled by showing appropriate overlay message/spinner.

Comment 22

[Punam Dahiya [:pdahiya]]

For reference, i have updated attached patch with the log statements used to arrive at performance number in #comment 21

Comment 23

[Punam Dahiya [:pdahiya]]

(In reply to Punam Dahiya from comment #19)
> (In reply to ying.xu from comment #18)
> > I test it simply on tarako
> > 
> > modify some code and using make reference-workload-heavy to push 1000
> > pictures into the phone.
> > 
> > launch the gallery ,wait scanning finished.
> > 
> > then remove sdcard ,reboot and relaunch the gallery. 
> > 
> > It took about 20s to display no photos tips, and before that ,there was a
> > black screen to display.
> 
> Thanks Ying for helping with performance testing of attached PR on tarako.
> For comparison, it will help if you can give time taken to display no photo
> tips without the patch for the above scenario.

Ying: I am clearing the NI request as i am able to replicate the black screen issue you have reported on Flame. Thanks

Comment 24

[Punam Dahiya [:pdahiya]]

With the attached patch, after bulk file deletion (or starting app first time after removing sdcard #comment 18), app shows black screen while enumerating files. Try to fix this by showing scanning overlay message instead of black screen.

Comment 25

[James Zhang (Spreadtrum)]

Yang, please land this WIP patch to verify.

Comment 26

[yang.zhao]

(In reply to James Zhang (Spreadtrum) from comment #25)
> Yang, please land this WIP patch to verify.
Hi,James
   Please see comment 18.gallery shows black screen for 20 seconds.I think it's not the right time to 
land this WIP.

Comment 27

[James Zhang (Spreadtrum)]

(In reply to yang.zhao from comment #26)
> (In reply to James Zhang (Spreadtrum) from comment #25)
> > Yang, please land this WIP patch to verify.
> Hi,James
>    Please see comment 18.gallery shows black screen for 20 seconds.I think
> it's not the right time to 
> land this WIP.

Can we provide a better patch?

Comment 28

[Punam Dahiya [:pdahiya]]

Hi James
In addition to the black screen #comment 18, with the attached patch #21223, I found done (https://github.com/punamdahiya/gaia/blob/Bug864674/apps/gallery/js/gallery.js#L457 ) is getting called immediately after enumeration completes without waiting for all async device storage calls to check for the file and update batch (L441). 

This early exit with done call outside the success callback is resulting in not showing thumbnail view for few files. This issue is replicated easily when there are less files in gallery app.

I have been working on fixing the above issue and the black screen reported in #comment 18. 

Here's the link to WIP patch
https://github.com/mozilla-b2g/gaia/pull/22109

With #22109, since we are now waiting for all async calls to complete before calling done, 
enumeration time for 1000 pics is ~ 27 sec for 1000 pictures versus ~12 sec for 1000 pictures in master. 
The time taken for first batch of image displayed is same as in master.

It will be great, if you can help test this patch on tarako and give your feedback. 

David, setting NI flag for your feedback on #22109, and your inputs to improve the patch to reduce the enumeration time. Thanks

Comment 29

[James Zhang (Spreadtrum)]

Loop Yang.
I think we'd better to land it to 1.4 and verify, there's risk to land 1.3t now.

Comment 30

[David Flanagan [:djf]]

Punam,

Your solution seems mostly technically sound to me but there are probably performance concerns that will prevent it from landing. 

You're using a "scanning..." overlay to deal with the blank screen issue, and don't end the enumeration until all of the device storage checks have completed.  (Good catch by the way: I'm very glad you noticed that before this landed!)

The time to display the first screen does not go up, which is good. But the overall time to enumerate the existing photos does increase substantially. If I recall correctly we don't begin to scan for new files until the db is enumerated, so if there are 1000 photos in the db, and the user takes a new photo with the camera and then launches gallery, I think we'd end up waiting 27s before scanning for new photos, and this seems like it is likely to be an unacceptable performance regression.  (Presumably the fix is to modify the app start up to display the first page of photos from the db and then begin a scan for newer files, but that is a larger, more complicated patch that is not suitable for this 1.4+ bug.) Also, the way that MediaDB defines its enumerate() method, you are constrained to handle each photo synchronously. This means that each time a photo is enumerated you have to check its existence and move on before you get the result.  And therefore we are likely to have hundreds of pending device storage requests at a time. I worry about the memory implications of that.

Also I think there is a problem with how you detect the end of the enumeration...  You test like this:

  filesEnumerated === filesExistCount + filesDeletedCount

But that assumes that the db enumeration will always be ahead of the file existance check. If for some reason the DB slowed down and device storage speeded up, you might find that they were equal before you had actually reached the end of the enumeration. So you'll need to add and test an extra flag that indicates that mediadb.enumerate() is complete.

To sum up: this patch seems close to being landable. But I don't know that I would recommend to Spreadtrum that they land it.

Comment 31

[David Flanagan [:djf]]

Media scanning is a thing that is just plain hard to do correctly and quickly.  I think we should work on addressing this issue more thoroughly along with bug 1039943 and bug 1028751.

Here are a couple of alternative approaches that we could explore for 1.4 here, though I fear it is getting too late to do much of anything...

1) We could go back to the existing code where we put up thumbnails as quickly as we can in batches. But modify it so that each new thumbnail has a CSS 'unverified' class set on it. If that class is set the thumbnail would be invisible.  In the flush() method we would check the existence of each file and remove the unverified class.  In this case no overlay would be necessary because blank squares would be created for each image, even if it was deleted.  Those that still exist would become visible once verified. Those that don't exist would either be deleted when it was determined they don't exist, or would be deleted by the scan process. While doing the existance checks I'd want to be careful to do them sequentially instead of in parallel so that we don't have hundreds of pending device storage calls at once. That is, we should check the existance of the next file from the onsuccess or onerror callback of the previous check.  Probably staring the checks in flush() and checking each batch in parallel would be okay.

This would avoid the underlying bug of displaying thumbnails for deleted files.  It would still have lots of UI flashing during the scan.  But I think it would allow us to start the scan as soon as the enumeration was done without waiting for the existence checks to complete, so I think we would still have acceptable performance.

2) We could attempt to enumerate the entire sdcard when we start the db animation. While we're waiting for that to complete, files enumerated from the db could be handled as above or as in the current patch. But once we have a complete list of pictures, we could do the existence checks synchronously, and this might speed everything up.

3) We can serialize the existence check, either using the approach in the PR or approach #1 above, we might be able to optimize the existence checking process. For example: when we want to check the existence of a file, we first separate the path into directory and filename. Then instead of just checking for that single file, we list the contents of the entire directory and cache all of the files in that directory. Then for all subsequent files in the same directory the existence check becomes synchronous. Since there will be many shared directories, this might be a substantial speed up. 

I hope some of these ideas help.

Yang: please let us know how Spreadtrum wants to proceed with this bug. If the performance implications of Punam's current patch are acceptable, we can land it after she fixes the bug described in comment 30. Or, we could just not land anything for now and try to come up with a proper fix as part of bug 1028751. Or if there is still time, we could continue to experiment with some of the suggestions described here.

Comment 32

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of bug 864674

Comment 33

[Punam Dahiya [:pdahiya]]

Hi David
Thanks for feedback, I have updated the patch (attachment 8465470 [details] [review]) after fixing the bug in #comment 30. 
Your approach 1 (#comment 31) sounds good as we are reducing enumeration time by taking the existence check in flush and not holding up scanning new images. We should definitely experiment this approach and check performance implication and run it by UX (for blank squares) if time permits. Thanks

Comment 34

[David Flanagan [:djf]]

I've filed bug 1046995 to overhaul the gallery app's scanning and startup behavior. I'd like to come up with a more robust fix for this issue as part of the fix for that bug. But that will not be done in time for 1.4, of course.

Comment 35

[Sri Kasetti]

David,
Is there a plan to fix 1046995 in 2.1?
If we are not going to fix 1046995 can we use the fix by Punam in 2.1?

Comment 36

[David Flanagan [:djf]]

Sri,

I plan to work on bug 1046995 for 2.1 and hope to address this issue as part of that. Not something I think we should commit to for the 2.1 release, however.

It is not clear to me how well we can fix this issue without substantially regressing startup performance, however, so the idea of landing Punam's patch for 2.1 is problematic.

Comment 37

[Sri Kasetti]

Moving this to backlog. We will consider this for 2.2

Comment 38

[Glen Flint]

We are experiencing a similar problem with Firefox 31, but not in Firefox 24 or 32.  Does this problem go away in Firefox 24 or 32?

Comment 39

[Michael Wu [:mwu]]

(In reply to Glen Flint from comment #38)
> We are experiencing a similar problem with Firefox 31, but not in Firefox 24
> or 32.  Does this problem go away in Firefox 24 or 32?

Hi Glen. This bug refers to a problem specific to Firefox OS Gallery app. It does not apply to our other products such as Firefox or Firefox for Android. If it's fixed in Firefox 32, you may be able to find the bug that fixed it and request that it be uplifted to Firefox 31.

Comment 40

[Glen Flint]

(In reply to Michael Wu [:mwu] from comment #39)
> (In reply to Glen Flint from comment #38)
> > We are experiencing a similar problem with Firefox 31, but not in Firefox 24
> > or 32.  Does this problem go away in Firefox 24 or 32?
> 
> Hi Glen. This bug refers to a problem specific to Firefox OS Gallery app. It
> does not apply to our other products such as Firefox or Firefox for Android.
> If it's fixed in Firefox 32, you may be able to find the bug that fixed it
> and request that it be uplifted to Firefox 31.

Thanks for the response.  I'll keep searching.

Comment 41

[BMO Automation]

Firefox OS is not being worked on