1039943 - [tarako][dolphin][Gallery][mediadb]Gallery should stop scanning and parsing when app goes to background

Status: RESOLVED FIXED

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

Description

[jinchao.wang]

Attachment: Bug332037_gallery_music_stop_scanning_parsing_when_background.patch

Preset Conditions：
many new image files to phone

(1)launch gallery from Homescreen
(2)gallery will scanning and parsing new image file first
(3)when scanning and parsing, gallery goes to background,but scanning and parsing doesn't pause.

When background app scanning and parsing ，switching applications will be not smooth，especially in the low-profile phones.

music and gallery have the same issue。


SPRD provide a patch : music & gallery pause scanning when they go to background,and  music & gallery resume scanning when they resume.

Comment 1

[jinchao.wang]

SPRD Bug 332037

Comment 2

[[:fabrice] Fabrice Desré]

David, does that sound reasonable to you?

Comment 3

[David Flanagan [:djf]]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> David, does that sound reasonable to you?

The patch looks good, though I'd have to study it more carefully before I was willing to land it.

I think, though, that pausing a scan is not the right thing to do here. I'd prefer to abort the scan and restart it from scratch. If a media app scan is paused when the app goes to the background and the user then copies new files to the phone by UMS, the gallery will start a new scan in the background while there is already a paused scan.  That seems like it will cause problems.

And if the user uses the camera to take new pictures while there is a paused gallery scan pending, the gallery will end up updating this metdata database while it has a pending scan, and that also seems like it could cause bad problems.

Given the memory constraints on Tarako, especially, Gallery and Music are likely to be killed while they are in the background and the scans will never resume anyway. So in that case, just aborting the scan won't hurt anything.

Comment 4

[Wayne Chang [:wchang]]

Jingchao,

The concept makes sense but we would prefer to go with David's suggestion per comment 3 - Abort and restart.
Can you change your patch to doing this?

Thanks

Comment 5

[jinchao.wang]

Attachment: galery_music_stop_scannnig_parsing.patch

Comment 6

[jinchao.wang]

(In reply to Wayne Chang [:wchang] from comment #4)
> Jingchao,
> 
> The concept makes sense but we would prefer to go with David's suggestion
> per comment 3 - Abort and restart.
> Can you change your patch to doing this?
> 
> Thanks

According to your suggestion, I updated my patch. Can help evaluating the patch?

Comment 7

[jinchao.wang]

(In reply to David Flanagan [:djf] from comment #3)
> (In reply to Fabrice Desré [:fabrice] from comment #2)
> > David, does that sound reasonable to you?
> 
> The patch looks good, though I'd have to study it more carefully before I
> was willing to land it.
> 
> I think, though, that pausing a scan is not the right thing to do here. I'd
> prefer to abort the scan and restart it from scratch. If a media app scan is
> paused when the app goes to the background and the user then copies new
> files to the phone by UMS, the gallery will start a new scan in the
> background while there is already a paused scan.  That seems like it will
> cause problems.
> 
> And if the user uses the camera to take new pictures while there is a paused
> gallery scan pending, the gallery will end up updating this metdata database
> while it has a pending scan, and that also seems like it could cause bad
> problems.
> 
> Given the memory constraints on Tarako, especially, Gallery and Music are
> likely to be killed while they are in the background and the scans will
> never resume anyway. So in that case, just aborting the scan won't hurt
> anything.

Hi David :
    Although I provide a patch ，I prefer that you be able to give a solution , my patch is only a reference ，or a discussion.
    Can you provide  us with a solution based on my two patch ?

Comment 8

[Hema Koka [:hema]]

David, 

Can you please provide your input?

Thanks
Hema

Comment 9

[David Flanagan [:djf]]

In the long term, I think we can find a good solution to this problem. Because of video hardware limitations, the video app scans files in the background but only creates video thumbnails when it is in the foreground.  Gallery could be modified to do that same thing, and bug 1028751 might be a good place to address that.

But for this 1.4+ bug we need something quicker.

Spreadtrum proposed a patch that would pause the mediadb scan when the gallery goes into the background and then restart it when it comes back to the foreground. For the reasons given in comment #3 I think this could cause data corruption issues.

I proposed instead that we actually fully abort the scan if the gallery goes to the background and then resume it when we come back to the foreground. Even that makes me nervous, though and I worry about the testing required to ensure that it does not break things.  For example, I'm not certain if taking a new photograph while the gallery is in the background would update the most recent known photo date. If it did, then when the app came back to the foreground, the scan would be started to look for photos newer than that most recent photo, and it would not quickly find older photos the way it should.

So my idea now is that if the gallery goes to the background while it is still scanning, maybe it should just exit completely.  On a memory constrained device (Tarako at least), it is likely that it would be killed while in the background anyway. And as bug 1028751 notes, the gallery is hard to use while the scanning process is going on because the thumbnails keep jumping around.  So it is likely that the user will stay in thumbnail view while the scan is happening anyway. So if we kill and restart the app, they won't really even notice, except for the somewhat longer startup time to reload all the existing thumbnails.
Since we don't allow the user to edit images while a scan is going on, so there is no possibility of the user losing an edit-in-progress if we quit during a scan.

I'm taking this bug and will create a simple patch to quit if we go to the background before the scan has completed.

Jingchao: does this seem like an acceptable solution?

Comment 10

[David Flanagan [:djf]]

Attachment: link to master branch patch on github

Punam: what do you think of this approach of just exiting? Can we land this in master and 2.0 and then fix it for real in bug 1046995?

Comment 11

[David Flanagan [:djf]]

Attachment: pull request for v1.4

Jingchao,

Does this patch fix the bug satisfactorily for you?

Comment 12

[jinchao.wang]

(In reply to David Flanagan [:djf] from comment #11)
> Created attachment 8465739 [details] [review]
> pull request for v1.4
> 
> Jingchao,
> 
> Does this patch fix the bug satisfactorily for you?

Hi David:
   Thanks for your patch. I have reviewed the patch and it looks good.

   Besides,Can you do the same modifications for music? Or I submit another bug to trace music issue?

Comment 13

[Marvin Khoo [:Marvin_Khoo]]

Hi David,
with this patch, may i know the expected result on following scenarios?

1. First time boot up
2. Open Music app, and start scanning songs (1st time scanning)
3. user press play once he saw the 1st album / song title.
4. play music and push music app to background.


if systems scanned 3 songs and stop scanning after pushed to background, so user can only listen 3 songs?

Comment 14

[Punam Dahiya [:pdahiya]]

Comment on attachment 8465736 [details] [review]
link to master branch patch on github

Hi David
The patch looks good, I like that we are exiting only for low mem devices and only while scanning, I tested in master and has r+. Thanks

Comment 15

[David Flanagan [:djf]]

(In reply to Marvin Khoo [:Marvin_Khoo] from comment #13)
> Hi David,
> with this patch, may i know the expected result on following scenarios?
> 
> 1. First time boot up
> 2. Open Music app, and start scanning songs (1st time scanning)
> 3. user press play once he saw the 1st album / song title.
> 4. play music and push music app to background.
> 
> 
> if systems scanned 3 songs and stop scanning after pushed to background, so
> user can only listen 3 songs?

Marvin: this patch only affects Gallery.  I only just now noticed that there was any discussion of the Music app in the bug at all. If changes are needed for the Music app, we should do that in a separate bug.

Comment 16

[David Flanagan [:djf]]

(In reply to jingchao.wang from comment #12)
>    Besides,Can you do the same modifications for music? Or I submit another
> bug to trace music issue?

Please file a separate bug for Music. Perhaps you can use the code from this Gallery patch and apply it to the Music app.  But note Marvin's question in comment #13: we can only force the Music app to exit if the user has not yet started playing music, of course.

Comment 17

[David Flanagan [:djf]]

I've updated the PR to activate this hack on any device with less than 512mb rather than on any device that has less than or equal to 256mb, and am carrying the r+ forward.

This change is motivated by the discussion in the dev-gaia thread "low-memory code paths", and also by the desire to be able to have this new behavior manifest on our low-memory Flames which get configured to have memory between 256 and 512.  I don't expect that this change will affect any production devices which are likely to have 256 or 512 and not use values in-between.

Comment 18

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/63a273c6d1bfb2b939718c7262a9e45d92ce9a89

Comment 19

[David Flanagan [:djf]]

Landed in 2.0: https://github.com/mozilla-b2g/gaia/commit/d49bd1cc11274f2bed12ad5311be104a0ce88f59

Comment 20

[David Flanagan [:djf]]

Landed in 1.4: https://github.com/mozilla-b2g/gaia/commit/24d2011dfe2f32e7b5480f0c48b7dc3fd6fad998

Comment 21

[James Zhang (Spreadtrum)]

But what about music background scanning? Jinchao's patch include stopping music background scanning.

Comment 22

[jinchao.wang]

Comment on attachment 8465739 [details] [review]
pull request for v1.4

R+ ,it's looks good to me.

Comment 23

[jinchao.wang]

(In reply to James Zhang (Spreadtrum) from comment #21)
> But what about music background scanning? Jinchao's patch include stopping
> music background scanning.

Hi James:
   I have fired another bug1048085 to trace music issue.

Comment 24

[Brogan Zumwalt [Inactive]]

New test case needs to be written to address bug.

Comment 25

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case has been added to moztrap:

https://moztrap.mozilla.org/manage/case/14373/

Comment 26

[Coler]

This issue CAN be repro on Nexus 5_v2.2&3.0.
Reproduce rate:5/5

N5_3.0(affected):
Build ID               20150317160205
Gaia Revision          63d6639acd771f548a2613f07f3e335921e4ac87
Gaia Date              2015-03-17 16:53:50
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e965a1a534ec
Gecko Version          39.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150317.194723
Firmware Date          Tue Mar 17 19:47:37 EDT 2015
Bootloader             HHZ12d

N5_2.2(affected):
Build ID               20150317162504
Gaia Revision          306772a58335ac4cad285d27c3805090a8cc6886
Gaia Date              2015-03-17 17:12:36
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/83251e534b33
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150317.195033
Firmware Date          Tue Mar 17 19:50:47 EDT 2015
Bootloader             HHZ12d

Comment 27

[Keven Kuo [:kkuo]]

regression?
I checked the code that "doNotScanInBackgroundHack" still in 2.2.

--
Keven

Comment 28

[Hermes Cheng (inactive after July 27, 2015)]

Coler, is this still reproducible on 2.2 and master?

Comment 29

[Coler]

Hi, Hermes
  About Gallery, this bug is still reproducible on latest build of Nexus5_2.2&3.0 by STR as comment 0, but is NOT on Flame2.2&3.0.
  About Music, it is still reproducible on latest build of Nexus5_2.2&3.0, Flame2.2&3.0 by STR as comment 0.


Actual result:
On Nexus 5_2.2&3.0, it will continue scanning and parsing in Gallery and Music when app goes to background.
On Flame 2.2&3.0, Only Music will continue scanning and parsing when app goes to background. 

See attachment:Reproduce_Gallery.3gp, Reproduce_Music.3gp, logcat_Gallery_2044.txt, logcat_Music_2211.txt

Found time:
Music: 22:11
Gallery: 20:44

Reproducing rate:
Nexus 5_2.2&3.0(Gallery&Music): 5/5
Flame 2.2&3.0 Music:5/5
Flame 2.2&3.0 Gallery:0/5

Device:Nexus5_3.0 build(Affected):
Build ID               20150616160206
Gaia Revision          9d42e94446f2dc01b519172b6d75d81d4d435bdc
Gaia Date              2015-06-16 16:16:47
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/27caa5299f9f
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150616.193221
Firmware Date          Tue Jun 16 19:32:40 EDT 2015
Bootloader             HHZ12f

Device:Flame 2.2 build([Music]Affected):
Build ID               20150616002505
Gaia Revision          e7a0c6d5f4df04d45fb3f726efb9e8223600cb79
Gaia Date              2015-06-15 06:12:18
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8045028bf400
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150616.035412
Firmware Date          Tue Jun 16 03:54:24 EDT 2015
Bootloader             L1TC000118D0

Device:Nexus5_2.2 build(Affected):
Build ID               20150616002505
Gaia Revision          e7a0c6d5f4df04d45fb3f726efb9e8223600cb79
Gaia Date              2015-06-15 06:12:18
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8045028bf400
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150616.034849
Firmware Date          Tue Jun 16 03:49:07 EDT 2015
Bootloader             HHZ12f

Device:Flame 3.0 build([Music]Affected):
Build ID               20150616010205
Gaia Revision          62ba52866f4e5ca9120dad5bfe62fc5df981dc39
Gaia Date              2015-06-15 19:09:24
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/ce863f9d8864
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150616.060924
Firmware Date          Tue Jun 16 06:09:36 EDT 2015
Bootloader             L1TC000118D0

Comment 30

[Coler]

Attachment: logcat_Music_2211.txt

Comment 31

[Coler]

Attachment: logcat_Gallery_2044.txt

Comment 32

[Coler]

Attachment: Reproduce_Gallery.3gp

Comment 33

[Coler]

Attachment: Reproduce_Music.3gp

Comment 34

[Hermes Cheng (inactive after July 27, 2015)]

Hi Coler, please clone this bug for following up. Thanks.

Comment 35

[Coler]

(In reply to Hermes Cheng[:hermescheng] from comment #34)
> Hi Coler, please clone this bug for following up. Thanks.

Hi Hermes, 
  I have filed a new bug 1175456.