Android studio: thread app not returning same value as non thread app

Question

I'm working on an app that goes through all of the phone directories, and collecting all the songs. When i run it normally it works fine, just takes about 6 seconds to go through everything, and causes the app to skip a lot of frames. I changed it so every time a file is found, a different thread reads the metadata and saves it. I'm also waiting for all of them in the end, because after that I'm trying to use that list. All of a sudden several of the song are null, even though they're not initialized as such anywhere. What can cause that? an app that works fine, but not with threads..?

the constructor that calls the search:

phoneSongsList = new Playlist();
findSongs(Environment.getExternalStorageDirectory().getAbsolutePath()); //.concat("/Music")
for (Thread thread : threads) {
    try {
        thread.join();
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}

The function that recursively looks for songs:

public void findSongs(String path) {
    File home = new File(path);
    for (final File file : home.listFiles()) {
        if (file.isDirectory())
            findSongs(path.concat("/" + file.getName()));
        else if (isAcceptableExtension(file.getName())) {
            Thread t = new Thread(new Runnable() {
                @Override
                public void run() {
                    phoneSongsList.add(fileToSong(file));
                }
            });
            t.start();
            threads.add(t);
        }
    }
}

The function that converts the file to a song object:

private Song fileToSong(File file) {
    final Album album = new Album();
    Song song = new Song();
    song.setName(file.getName().substring(0, (file.getName().length() - 4))); // remove suffix
    song.setPath(file.getPath());

    final MediaMetadataRetriever metaRetriever = new MediaMetadataRetriever();
    metaRetriever.setDataSource(file.getPath());

    song.setArtists(metaRetriever.extractMetadata(MediaMetadataRetriever.METADATA_KEY_ARTIST));
    album.setName(metaRetriever.extractMetadata(MediaMetadataRetriever.METADATA_KEY_ALBUM));
    album.setYear(metaRetriever.extractMetadata(MediaMetadataRetriever.METADATA_KEY_YEAR));
    album.setCover(metaRetriever.getEmbeddedPicture(), context);
    song.setAlbum(album);
    song.setDuration(Long.parseLong(metaRetriever.extractMetadata(MediaMetadataRetriever.METADATA_KEY_DURATION)));
    song.setGenre(metaRetriever.extractMetadata(MediaMetadataRetriever.METADATA_KEY_GENRE));

    metaRetriever.release();
    return song;
}

The Playlist.add function:

public void add(Song song) {
    add(list.size(), song);
}
public void add(int index, Song song) {
    if(song==null)
        return;
    if (index > list.size())
        index = list.size();
    if (list.contains(song))
        list.remove(song);
    list.add(index, song);
}

Even when i explicitly specify that no null objects would be added to the list, it runs fine when it saves the songs, but give a null error when trying to read. Each time i run different songs and a different number of songs are set to null.

Please help.

Why are you creating a new thread for each subdirectory? That's not going to speed things up, if anything its going to be slower do to task switching. — Gabe Sechan, May 27 '21 at 23:13

score 1 · Accepted Answer · answered May 27 '21 at 23:15

1

You're dynamically trying to add new Threads to the list of threads on other threads, but reading that thread on one thread. That means you'll finish the loop on some subset of those threads before all of them are added. This entire approach is one big race condition.

This isn't something threads are going to speed up much, and you're doing your threading all wrong anyway. Throw this out and just do it on a single background thread, and do not join on that thread (or you may as well do it sequentially)- have it post back to the main thread when done.

answered May 27 '21 at 23:15

Gabe Sechan

90,003
9
87
127

Where is the right place start a thread so it will be efficient? And i thought i was joining on the main since it's the one creating the others.. I'm pretty new at this, can i have more explanations? – Yaels May 27 '21 at 23:51
You don't need to join on a thread at all. Joining will wait for it to finish. That's usually (although not always) counterproductive. As for starting multiple threads to do this- this isn't really the type of operation that benefits from that. Most of the time is spent doing disk reads. Doing the entire thing on 1 background thread may make sense, but spreading to multiple probably will take more time in task switching and forking (creating a thread isn't free) than it will save. – Gabe Sechan May 28 '21 at 00:06
I think i got it. I put the line calling to this constructor in a thread and used post to show the list. It's working fine (no skipping frames) but still takes a long time (I think even longer than before).. I hope i did it right. – Yaels May 28 '21 at 10:37

Android studio: thread app not returning same value as non thread app

1 Answers1