901421 - [Music] After enabling Shuffle , user is not able to select next(>>) song

Status: VERIFIED FIXED

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

Description

[Leo]

1. Title: After user enabling shuffle, tapping on next (>>) song becomes inactive.
2. Precondition: Have some mp3 and ogg songs in internal memory and external SD card 
3. Tester's Action:
        1. Open music app -> play any song from main music window -> enable shuffle
        2. while playing the song press back button on the top -> go to all songs (last tab) -> play any song -> press next(>>),no response..
        
4. Detailed Symptom (ENG.): After Enabling shuffle, next (>> icon) becomes non responsive
5. Expected :  User should be able to select next song , irrespective of shuffle and repeat option
6. Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced on v1-train

Comment 1

[Leo]

Hi Dkuo,

I would like to add one more point here,

I was able to see this problem, only when shuffle was enabled.When Shuffle is inactive i dint come across this problem atleast till now.

Comment 2

[:gerard-majax]

I'm looking into this.

Comment 3

[:gerard-majax]

I can't reproduce on v1-train. Please provide better STR, I can't enable shuffle as you explain.

Comment 4

[Leo]

Hi Alex,

Copy a couple of ogg and mp3 files on to the device.
I had some 100 songs(combination of ogg and mp3) on the device.

STR:

1. Copy a few songs with the same album(such that when you launch music app, in the main screen you get unknown album which has more songs).
2. Now tap on the unknown album,music starts playing , here you can enable shuffle.
3. press back button , and goto "songs tab"
4. play any song and press next(>>) .

Comment 5

[:gerard-majax]

Okay, I know get your steps. I can't reproduce this on Inari with Gaia master, I'm checking with v1-train.

Comment 6

[:gerard-majax]

It's working well on v1-train.

Comment 7

[:gerard-majax]

I'm checking if I can reproduce with the x-heavy reference workload, since you specified "100 songs".

Comment 8

[:gerard-majax]

Still not able to reproduce this, using a 250 songs workload.

Comment 9

[:gerard-majax]

And confirmed I cannot reproduce after testing with more than 1.1GB of Ogg files, too (previous tests were with MP3).

Which buildid is this against ? Is it the plain gaia repo or a fork ?

Comment 10

[Leo]

Hi Alex,

Please refer the Gaia and Gecko version below,

Gaia : 0d5a9a7577f16b6a72a982148c6f509ee1714ea2
Gecko :  499c1f8ea7ad0cdaa7086214278e2944b8a2ea33

Also, i would like to add one more point here.

1. Make sure you atleast have one album in which you atleast have more than 5-6 songs....
2. Along with the above copy some more songs .So that in the music thumbnail view  you can enable shuffle(album of 6 songs) .
3. Now Goto songs tab and tap any song and play..

Hope you will be able to reproduce it now.

Comment 11

[Leo]

Attachment: test_1.rar

test file_1

Comment 12

[Leo]

Attachment: justin_bieber_ft_ludacris_-_ba.mp3

Test File_2

Comment 13

[Leo]

Attachment: Kaadal Endral - Yuvan Shankar Raja.mp3

Test File_3

Comment 14

[Leo]

Attachment: ninne kanaan.mp3

Test File_4

Comment 15

[Leo]

Attachment: vaa vaa en dhevadaye.mp3

Test File_4

Comment 16

[Leo]

Attachment: Vida_Chollan_Happydays.mp3

Test File_5

Comment 17

[Leo]

Hi Alex,

I have uploaded the test files .please test with this files.

STR:

1. Make sure device has only these files.
2. Launch music app(stay in thumbnail view , main menu of music app).
3. Click on unknown artist (this might have some 3 songs only)
4. Enable Shuffle.start playing music
5. Goto songs tab while music is still paying.
6. Tap on any song to play
7. Press next button

repeat step#7

Observation:

Next button may work only for 2-3 times( based on the availability of the number of tracks present in the unknown artist described in step#3).

After the count exceeds , further pressing the next button doesnt play the next song.

Please let me know, if you any any more info.

Thanks,

Comment 18

[:gerard-majax]

It seems I'm reproducing this, now. The behavior that I'm seeing is that after a while, pressing |Next| brings me back to the song list. Is it consistent with your bug ?

Comment 19

[Dominic Kuo [:dkuo]]

I am in Vancouver for Media team meet-up this week and apologizes for the late reply, I try to reproduce yesterday night and found it's reproducible, but I haven't found the root cause yet, and I also saw what Alex said in comment 18.

Comment 20

[Leo]

Hi Alex,

As you said this was consistent , repeatedly pressing NEXT button gets me back to the song list.

Comment 21

[:gerard-majax]

(In reply to Leo from comment #20)
> Hi Alex,
> 
> As you said this was consistent , repeatedly pressing NEXT button gets me
> back to the song list.

It's not clear, does the application becomes unresponsive or the next button takes you back to the song list, or both depending on some cosmic ray touching the device ?

Comment 22

[:gerard-majax]

Okay, I've added some debug inside Player.js and I think I have a lead:

E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:408 in pv_next: Music: pv_next: IN
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:409 in pv_next: Music: pv_next: isAutomatic=undefined
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:416 in pv_next: Music: pv_next: passed sourceType
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:424 in pv_next: Music: pv_next: passed repeatOption
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:428 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:429 in pv_next: Music: pv_next: playingIndex=1
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:464 in pv_next: Music: pv_next: computing realIndex
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:468 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:469 in pv_next: Music: pv_next: realIndex=3
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:471 in pv_next: Music: pv_next: OUT

E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:408 in pv_next: Music: pv_next: IN
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:409 in pv_next: Music: pv_next: isAutomatic=undefined
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:416 in pv_next: Music: pv_next: passed sourceType
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:424 in pv_next: Music: pv_next: passed repeatOption
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:428 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:429 in pv_next: Music: pv_next: playingIndex=2
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:464 in pv_next: Music: pv_next: computing realIndex
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:468 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:469 in pv_next: Music: pv_next: realIndex=2
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:471 in pv_next: Music: pv_next: OUT

E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:408 in pv_next: Music: pv_next: IN
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:409 in pv_next: Music: pv_next: isAutomatic=undefined
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:416 in pv_next: Music: pv_next: passed sourceType
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:424 in pv_next: Music: pv_next: passed repeatOption
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:428 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:429 in pv_next: Music: pv_next: playingIndex=3
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:464 in pv_next: Music: pv_next: computing realIndex
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:468 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:469 in pv_next: Music: pv_next: realIndex=undefined

E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:408 in pv_next: Music: pv_next: IN
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:409 in pv_next: Music: pv_next: isAutomatic=undefined
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:416 in pv_next: Music: pv_next: passed sourceType
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:424 in pv_next: Music: pv_next: passed repeatOption
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:428 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:429 in pv_next: Music: pv_next: playingIndex=4
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:464 in pv_next: Music: pv_next: computing realIndex
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:468 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:469 in pv_next: Music: pv_next: realIndex=undefined

E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:408 in pv_next: Music: pv_next: IN
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:409 in pv_next: Music: pv_next: isAutomatic=undefined
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:416 in pv_next: Music: pv_next: passed sourceType
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:424 in pv_next: Music: pv_next: passed repeatOption
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:428 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:429 in pv_next: Music: pv_next: playingIndex=5
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:464 in pv_next: Music: pv_next: computing realIndex
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:468 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:469 in pv_next: Music: pv_next: realIndex=undefined

E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:408 in pv_next: Music: pv_next: IN
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:409 in pv_next: Music: pv_next: isAutomatic=undefined
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:416 in pv_next: Music: pv_next: passed sourceType
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:424 in pv_next: Music: pv_next: passed repeatOption
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:428 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:429 in pv_next: Music: pv_next: playingIndex=6
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:464 in pv_next: Music: pv_next: computing realIndex
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:468 in pv_next: Music: pv_next: this.shuffleOption=true
E/GeckoConsole(  505): Content JS WARN at app://music.gaiamobile.org/js/Player.js:469 in pv_next: Music: pv_next: realIndex=undefined

Comment 23

[:gerard-majax]

So, as far as I could debug in the train, it seems that this.dataSource and this.shuffledList are somehow not in sync: the first one contains 8 elements while the second one only contains four.

When the list is shuffled for the first time, this.dataSource contains 4 elements, which is consistent with the shuffled list obtained.

Comment 24

[:gerard-majax]

As far as I could dig up for now, it turns out that in apps/music/js/music.js, in the handleEvent method of the ListView, when we follow the code path for the option value "title" (around lines 1200-1210 in v1-train), we reset PlayerView.dataSource with the actual value from ListView.dataSource.

However, the shuffledList might not be in sync anymore regarding the dataSource: this what happens in the current case.

Comment 25

[:gerard-majax]

Calling PlayerView.setShuffle(true) fixes the issue. The fact that tapping 'next' when you are at the end of the list brings you back to the song list seems to be the correct behavior, but I'll let dominic review and check all of this to ensure I did not break anything.

Comment 26

[:gerard-majax]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11451

Pointer to Github pull-request

Comment 27

[:gerard-majax]

Comment on attachment 788028 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11451

Please find attached a link to the github pull request https://github.com/mozilla-b2g/gaia/pull/11451 that fixes this behavior.

Comment 28

[Leo]

Hi Alex, 

I need one more help.

What should be the expected behavior of shuffle in Music in different tab and do you have any doc for the shuffle behavior.

Thanks,

Comment 29

[:gerard-majax]

(In reply to Leo from comment #28)
> Hi Alex, 
> 
> I need one more help.
> 
> What should be the expected behavior of shuffle in Music in different tab
> and do you have any doc for the shuffle behavior.
> 
> Thanks,

I don't know, we'll have to wait for dominic's feedback on this :(

Comment 30

[Leo]

Hi Alex, 

The reason for me asking you any reference document was, 

I tried with your patch,

I found one issue, but not sure whether it is a expected behaviour of shuffle or an issue.
Eg:
Out of 8 songs, one song was missed out in songs list while playing(but the count of next was still 8 with some other song been played twice).

I tried out with some random collections of music files.

Need to know, what is the correct scenario here.

Thanks,

Comment 31

[:gerard-majax]

It does not change the fact that I can't help you on this and that we need feedback from dominic. He should be back from his workweek, hopefully he will be able to help us soon !

Comment 33

[Dominic Kuo [:dkuo]]

Comment on attachment 788028 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11451

Alex, really appreciated you for working on this, I took a quick look on your patch and realized what's going on here.

Actually the lengths of this.dataSource and this.shuffledList should be sync, but because it takes some time to complete enumerating all the songs from MediaDB and push to dataSource, before the enumeration finishes, if we try to create a shuffledList with an uncompleted dataSource, the shuffledList will be also uncompleted, but at that time the enumeration ends and the dataSource will get all of the songs, so that's why they are not sync.

Base on the above reason, there seems more works to fix this issue, so I am cancelling the review request first. I have started to work on this issue and tomorrow the patch should be ready for review, thanks!

Comment 34

[Dominic Kuo [:dkuo]]

Attachment: use MediaDB.getAll() and sort the song array to display all songs list

Update:

Here is the patch I have got now and should solve this issue, but because the patch used MediaDB.getAll() to get all the songs at one time, then draw them all at once, a performance issue is accompanied so I need to deal with it as well.

Comment 35

[Ray REN]

Hi Dominic,

Any updates?

Comment 36

[Dominic Kuo [:dkuo]]

Comment on attachment 789497 [details]
use MediaDB.getAll() and sort the song array to display all songs list

Jim,

The root cause of this bug is music app used MediaDB.enumerate() to get all songs and render them on the ui, but before the enumeration ends, if the user tap one of the songs with the shuffle enabled, then music app will try generate a shuffled list with the retrieved records, which is a incomplete array so that the shuffled list will be wrong and shorter than the correct one, then it will cause a wrong logic on switching the tracks as well.

First, the fix is actually simple, just used MediaDB.getAll() to retrieve all the songs at once and create the shuffled list, but what I have encountered was, if we render them all at once, it takes a while and the UX is real bad, so I try to write a logic to batch updating the list elements. With this, users are able to see the first paint very quickly.

Second, to also get a smooth scrolling on the list view, another logic is responsible for updating the list when the rendering/scrolling is idle or "almost" idle, I tried to listen to touchmove and scroll events to achieve this, you can see the details in the patch.

And this patch will also fix bug 860153 and bug 868118 which are both the performance issues I didn't have time to solve before, please review this and thanks!

Comment 37

[Jim Porter (:squib)]

Comment on attachment 789497 [details]
use MediaDB.getAll() and sort the song array to display all songs list

Ok, I think this is good for the most part, although I have some review comments over at Github. If you address those, everything should be fine, although I'll want to do another quick pass just to make sure. It may also be useful to have :djf take a quick look at the changes you made to how we use the media db.

Comment 38

[Preeti Raghunath(:Preeti)]

Jim,

Did you get a chance to address the review comments in comment 37?

Comment 39

[Jim Porter (:squib)]

Do you mean Dominic? I was the one who made the review comments...

Comment 40

[Dominic Kuo [:dkuo]]

I am the one who should address the patch, I was working on some other things while waiting the review, and I will do it asap then request another review and feedback to Jim and David.

Comment 41

[Dominic Kuo [:dkuo]]

Comment on attachment 789497 [details]
use MediaDB.getAll() and sort the song array to display all songs list

Jim,

I have addressed the issues that you mentioned on github, also added more comments on the new introduced functions, now it should be clear for people to understand how my patch works, can you please take a look again? thanks.

David,

Jim mentioned that it will be nice if you can take a quick look on my patch and give some feedback, to see my changes on using the records from the MediaDB. The details is in comment 36, you can probably read it before you look on the patch, thanks.

Comment 42

[David Flanagan [:djf]]

Comment on attachment 789497 [details]
use MediaDB.getAll() and sort the song array to display all songs list

I didn't have time to look at everything in this patch. But partial comments are on github.  Basically, I think we're just not allowed to use getAll(), and I think you can do smarter batching.

That may not help with the shuffle problem.  How does the music2 app handle that? Can you emulate it?

Comment 43

[Dominic Kuo [:dkuo]]

Comment on attachment 789497 [details]
use MediaDB.getAll() and sort the song array to display all songs list

Cancelling the review request because the feedback-.

Comment 44

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #42)
> Comment on attachment 789497 [details]
> use MediaDB.getAll() and sort the song array to display all songs list
> 
> I didn't have time to look at everything in this patch. But partial comments
> are on github.  Basically, I think we're just not allowed to use getAll(),
> and I think you can do smarter batching.

I will try to get rid of getAll() and see if I can get something that it acceptable for they users.

> That may not help with the shuffle problem.  How does the music2 app handle
> that? Can you emulate it?

Actually music2 can only shuffle on artist/album, there is no shuffle all songs so probably it's not a problem for music2, and I also looked on the code that how music2 shuffle the list, and the strategy is same as music, to get the whole play list before generating the shuffled list, so once music2 provide the capability to shuffle all songs, if music2 does not address it then the same problem will occur.

Comment 45

[David Flanagan [:djf]]

One strategy that dhylands has suggested is to generate a random number in the metadata for each song and create a database index for that random number property. Then you could pick a song at random by generating a new random number and enumerating the next larger or smaller value from that number.

Can you modify the shuffle algorithm to just pick from among the songs it already has?

Or could you just disable the >> button until all the songs are available?

Comment 46

[Dominic Kuo [:dkuo]]

David,

The strategy that Dave proposed sounds workable for shuffle, however, why I used getAll() was because I also wanted to deal with both large number records and shuffle all the records.

The first requirement I need is: the total count of the records, if it's possible, I think we should set the correct(or close to) height to the list so that the scroll bar will not shrink continually while we render/append the new elements into the DOM tree. To achieve this, I think we can create a new index(playable?), or just use the current "fail" property as the index in indexedDB, and we can set the value to "1" and "0" when the metadata parser returns "success" and "error" for each file, then, use IDBKeyRange.only() or IDBKeyRange.bound() to retrieve the total count of the playable files by using the existed MediaDB.count().

The second requirement is: let the MediaDB to have the capabality to get the specific records. I am not sure if you still recalled bug 796317, which was my first time that I proposed, we should have advancedEnumerate() in MediaDB(cursor.advance()) so that we can have a flexible way to enumerate the records. With the first requirement, music app is able to grenerate a shuffled list with the correct count, and while music is retrieving the records from MediaDB, once the user start to play shuffle, and plays some indexes which are not there yet, we can use MediaDB.advancedEnumerate() to retrieve the target records right away without waiting the enumeration finished. If I recalled correctly, the response time should be acceptable and the user won't feel the asynchronous delay. More, if the list wants to do batch update, the advancedEnumerate() can help to enumerate the records in small pieces because we don't need to enumerate all the records every time when we tap on the "songs" filter, unless the user really want to see all the songs, which I think is also achievable by using the proposed advancedEnumerate().

These are just my initial thoughts and any feedback is welcome, How do you think? thanks.

Comment 47

[Preeti Raghunath(:Preeti)]

This seems to be a very healthy conversation and I wouldn't like for work to be incomplete. 

I am leaning on moving this to koi+ so that the fix can be well thought out/ unit tested and finally QA tested.

Comment 48

[David Flanagan [:djf]]

(In reply to Dominic Kuo [:dkuo] from comment #46)
> David,
> 
> The strategy that Dave proposed sounds workable for shuffle, however, why I
> used getAll() was because I also wanted to deal with both large number
> records and shuffle all the records.

And I know that I'm the one who added getAll() to mediadb.  Since then, however, Andreas has pushed back quite hard on its use, so if we can do shuffle without getAll() I think that would be good.

Can we fake it?  Like enumerate the first 10 songs, pick one of those 10 as the first song to play, and then enumerate the rest while the first song is playing and create the rest of the shuffled list?

And maybe each time you enumerate all songs for shuffle, you pick one at random and save its name in asyncStorage for use as the first song next time the user shuffles.  Could something like that work as a hack to enable shuffle all for 1.2?

> 
> The first requirement I need is: the total count of the records, if it's
> possible, I think we should set the correct(or close to) height to the list
> so that the scroll bar will not shrink continually while we render/append
> the new elements into the DOM tree. To achieve this, I think we can create a
> new index(playable?), or just use the current "fail" property as the index
> in indexedDB, and we can set the value to "1" and "0" when the metadata
> parser returns "success" and "error" for each file, then, use
> IDBKeyRange.only() or IDBKeyRange.bound() to retrieve the total count of the
> playable files by using the existed MediaDB.count().

How about just using the total count of all records, include any failed ones.  It might be an overestimate, but when you are done enumerating, you can shrink the container to the actual size and you'll avoid the shrinking scrollbar problem.

> 
> The second requirement is: let the MediaDB to have the capabality to get the
> specific records. I am not sure if you still recalled bug 796317, which was
> my first time that I proposed, we should have advancedEnumerate() in
> MediaDB(cursor.advance()) so that we can have a flexible way to enumerate
> the records. 

I don't remember it clearly and github won't let me see the old commit that defined advacnedEnumerate().  If I remember right, it was a method to find the nth record in the database for any given n. If that was it, it just seemed like it was forcing IndexedDB to do something that it really wasn't designed to do.

With the first requirement, music app is able to grenerate a
> shuffled list with the correct count, and while music is retrieving the
> records from MediaDB, once the user start to play shuffle, and plays some
> indexes which are not there yet, we can use MediaDB.advancedEnumerate() to
> retrieve the target records right away without waiting the enumeration
> finished. If I recalled correctly, the response time should be acceptable
> and the user won't feel the asynchronous delay. More, if the list wants to
> do batch update, the advancedEnumerate() can help to enumerate the records
> in small pieces because we don't need to enumerate all the records every
> time when we tap on the "songs" filter, unless the user really want to see
> all the songs, which I think is also achievable by using the proposed
> advancedEnumerate().
> 
> These are just my initial thoughts and any feedback is welcome, How do you
> think? thanks.

Comment 49

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #48)
> And I know that I'm the one who added getAll() to mediadb.  Since then,
> however, Andreas has pushed back quite **** its use, so if we can do
> shuffle without getAll() I think that would be good.

Agreed, and that's what I am planning to.

> Can we fake it?  Like enumerate the first 10 songs, pick one of those 10 as
> the first song to play, and then enumerate the rest while the first song is
> playing and create the rest of the shuffled list?
> 
> And maybe each time you enumerate all songs for shuffle, you pick one at
> random and save its name in asyncStorage for use as the first song next time
> the user shuffles.  Could something like that work as a hack to enable
> shuffle all for 1.2?

This sounds workable and probably we can use the trick above to avoid the users getting the same songs when they shuffle all.

> How about just using the total count of all records, include any failed
> ones.  It might be an overestimate, but when you are done enumerating, you
> can shrink the container to the actual size and you'll avoid the shrinking
> scrollbar problem.

This also sounds right to me, but I have noticed the scroll bar is disappeared in the recently builds, so probably this is not an issue now!

> I don't remember it clearly and github won't let me see the old commit that
> defined advacnedEnumerate().  If I remember right, it was a method to find
> the nth record in the database for any given n. If that was it, it just
> seemed like it was forcing IndexedDB to do something that it really wasn't
> designed to do.

If I go for the approach(fake the shuffle) then we don't need advacnedEnumerate().

Thanks for the feedback David, I think I should be able to write a new patch with these information.

Comment 50

[Dominic Kuo [:dkuo]]

Update:

I have finished the part on batch updating without using MediaDB.getAll(), but still struggling with the shuffle logic that without using MediaDB.getAll().

Comment 51

[Preeti Raghunath(:Preeti)]

Hema

This is a koi+ blocker. Please help provide the required help for the bug to move ahead.

Comment 52

[Hema Koka [:hema]]

Dominic has been out for a week but from last Thursday (10/11) sync up meeting, he mentioned he is working on fixing this bug. 

Dominic - If you need additional help to wrap this up soon, please let me know. Perhaps djf or jim can help you.

Comment 53

[Dominic Kuo [:dkuo]]

The new shuffle logic takes some time but I am planning to finish it by today. Sorry about not updating the progress here cause I was PTO after the summit.

Hema, I think I got enough materials to fix this issue, I will let you know if I encounter new problems, thanks.

Comment 54

[Dominic Kuo [:dkuo]]

Attachment: WIP

I have done most of the parts of the patch but still addressing the rest parts, attaching the WIP first.

Comment 55

[Dominic Kuo [:dkuo]]

(In reply to Dominic Kuo [:dkuo] from comment #53)
> The new shuffle logic takes some time but I am planning to finish it by
> today. Sorry about not updating the progress here cause I was PTO after the
> summit.
> 
> Hema, I think I got enough materials to fix this issue, I will let you know
> if I encounter new problems, thanks.

Sorry I think I was wrong on the ETA...while I was implementing the patch, because the enumeration of all songs takes a while(2500ms for 1000 songs, will be slower if we have more songs), before the enumeration finishes, if the "repeat list" option in the player is on, we need to disable the previous button to avoid the users tapping it(because we are unable to play the songs that are not retrieved yet), this ux is bad and noticeable for users.

I have thought some other strategies to fix this issue, but if we go for one strategy that doesn't get all songs at one time, then next/previous buttons must be disabled before the player is trying to play a not-retrieved record, so I think we should re-consider using the cursor.advance() to random access any indexedDB records, and that we don't have to disable next/previous buttons due to the enumeration is not finished.

But in comment 48, David mentioned that indexedDB is not really designed for retrieving the nth record in the database. I am not sure why, but if I recalled correctly, in bug 796317 I had used cursor.advance(), it retrieved the right record and the callback time is responsive(I don't really remember it but I can test again), this just my test so I need more information to judge that if we should go cursor.advance() or not.

David, would you describe why we should not use indexedDB to retrieve the nth record? it confused me that indexedDB has cursor.advance() but we shouldn't use it.

Comment 56

[David Flanagan [:djf]]

(In reply to Dominic Kuo [:dkuo] from comment #55)
> 
> David, would you describe why we should not use indexedDB to retrieve the
> nth record? it confused me that indexedDB has cursor.advance() but we
> shouldn't use it.

If cursor.advance() actually does some kind of random access, then great. But I would assume that it is still sequential and that the advance operation is O(n) rather than O(1). Trying to build random access on top of an O(n) operation does not make sense.

I think that Ben Turner is our resident expert on IndexedDB. Maybe he can tell us if using cursor.advance() is a suitable way to implement "shuffle all" and retrieve the object at an arbitrary index within an object store.

Comment 57

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Advance does indeed do an O(n) operation, though it happens all at once in the parent process.

Comment 58

[Dominic Kuo [:dkuo]]

Thanks David and Ben, and according to comment 56 and comment 57, I have done some tests with the latest pvt gecko(on unagi) and also confirmed cursor.advance() is an O(n) operation. Let's take a look on the results first:

1. I tested the existing enumerate() method in MediaDB, the average time to retrieve the first record is 34.8ms, I was assuming for every record, the retrieved time should be the same so I just tested with the first record.

2. I wrote a new method called advancedEnumerate(), the main idea is using openCursor() to get a cursor first, then use cursor.advance() to trigger another cursorRequest.onsuccess to retrieve the record we want. I tried put 2000 songs in the SD card, about 12GB which I think is a big amount for users. After that I tried to retrieve the 500th, 1000th and 2000th records, the average retrieved times are 80.5ms, 111.2ms and 174.9ms.

(I was using Performance API to test these results: https://developer.mozilla.org/en-US/docs/Web/API/Performance)

The reason why I think it's more suitable for music app to use advancedEnumerate(cursor.advance()) instead of use enumerate(cursor.continue()) when playing all songs or shuffle all is because, with the above results, although cursor.advance() is an O(n) operation, it takes only 0.063ms to advance one cursor, which means it's still responsive if music is retrieving records among large quantities. This is good for random accessing any songs if we don't fake the shuffle logic, and we can guarantee the all songs will be shuffled and the users will be listen to different songs when every time music app shuffles. More, with advancedEnumerate() we also don't have to disable the previous button if all songs are not retrieved yet and the repeat list in on.

I think this is something more practical for the users and this approach is a worthy trade-off, also my design is to use advancedEnumerate() only when the target record is not retrieved yet. If the target record exists, it will just use the record in the array that retrieved from the original enumerate() in MediaDB, so for most of the time, the play logic for all songs will still the same.

I hope my test results make sense and able to convince people, needinfo David and Ben to see what they think about this, thanks.

Comment 59

[Dominic Kuo [:dkuo]]

Also needinfo Jim because he should be the reviewer of my proposed patch, Jim, would you mind to first take a look on my test results in comment 58? thanks.

Comment 60

[Jim Porter (:squib)]

I think it'd be useful to see if your assumption in (1) is correct, i.e. that the retrieval time is always the same. Other than that, all of that sounds sane, although I'd want to see the code, since there might be a better way to design the API than to have enumerate() and advancedEnumerate().

Comment 61

[David Flanagan [:djf]]

Dominc,

If you can use cursor.advance() to pull any record out in < 200ms, that seems good enough to me at least for now. Particularly if you pick the next song while the current song is still playing.

This isn't really enumeration, though, is it?  So the method name advancedEnumerate() may not make sense. It sound to me like you're just writing a utility function: 

  getRecordByIndex(objectstore, index, callback)

Thanks for doing this investigation, Dominic.

Comment 62

[Dominic Kuo [:dkuo]]

Thanks, David.

Actually my design also considered resuming the enumeration which was cancelled, if we already retrieve many records, we can just resume, or I should say restart from the index where we cancelled the enumeration, so basically advancedEnumerate() is the variation of enumerate(), the only difference is there is an additional argument called "index".

advancedEnumerate() can be used to retrieve one or multiple records, if we want to retrieve just one record, which is our case for shuffling all songs, we can call cancelEnumeration() right after we got the first record, or, for resuming the enumeration, it's the same to wait for the last null record.

Comment 63

[Dominic Kuo [:dkuo]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/13096

Jim,

The new patch basically took the old parts for displaying the ListView elements from attachment 789497 [details], plus the new api that I proposed for the MediaDB, which you mentioned you would like to see in comment 60, would you please review it? hope my comments are clear enough for you to understand, thanks!

Comment 64

[Preeti Raghunath(:Preeti)]

10/28 and still no concrete fix.

Moving it out to 1.3 hence.

Comment 65

[Jim Porter (:squib)]

Comment on attachment 822396 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/13096

This looks good to me! It looks like David Flanagan is happy with the MediaDB changes, so I think this is ready to land after you fix the one small nitpick I had over on GitHub.

I'm still not sure "advancedEnumerate" is the best name for the new MediaDB method, but I can't think of a better one, so let's just leave it at that.

Comment 66

[Dominic Kuo [:dkuo]]

Thanks Jim! I have addressed the small nit you mentioned and did a little rearrangement about the code, the logic keeps the same so I didn't ask for another review from you.

Landed on master: dcbc64e7ec9dbe17f4151b64db937b0afc0d2120

Comment 67

[Dominic Kuo [:dkuo]]

This bug was a koi+ blocker but due to the back-and-forth discussion, it became a 1.3 bug. I am putting it back to the koi? basket for triaging. Note that the patch fixes the shuffle-all feature which was already in v1.1, also it was designed to fix bug 860153 and bug 868118(performance issues) as well, so I think it's worthy to have this patch in v1.2, thanks.

Comment 68

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted dcbc64e7ec9dbe17f4151b64db937b0afc0d2120 to:
v1.2: 487ea1febc8bbcd4f0b3fe12e67ed48431ffce93

Comment 69

[Marcia Knous [:marcia]]

Verified fixed on master, Buri, using:

Gaia   42bbe26a72e030faf07a6fc297f61a3a8ccda25b
SourceStamp 70de5e24d79b
BuildID 20131107040200
Version 28.0a1

I verified by following the steps in the initial comment - the next song option works fine in all the scenarios I tested.