Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ShuffleOrder.getShuffledIndex and .getUnshuffledIndex for ease of use. #5426

Open
mathemandy opened this issue Jan 23, 2019 · 9 comments
Assignees

Comments

@mathemandy
Copy link

mathemandy commented Jan 23, 2019

Continuing from Issue #4915 i was able to come up with my own implementation of my Own Shuffle Order which is bare but works.

   
public static final String TAG = LogHelper.makeLogTag(UduxShuffleorder.class);
   private final Random random;
   private int[] shuffled;
   private int[] indexInShuffled;
   private int[] right;

   /**
    * Creates an instance with a specified length.
    *  @param length The length of the shuffle order.
    *
    * @param currentPosition The current position of the current
    *
    */

   public UduxShuffleorder(int length, int currentPosition, Random random) {
       this.random = random;
       createShuffledList(length, currentPosition,  random);
   }


   private  void createShuffledList(int length, int currentPosition , Random random){
        shuffled = new int[length];

       for (int i = 0; i < length; i++) {
           int swapIndex = random.nextInt(i + 1);
           shuffled[i] = shuffled[swapIndex];
           shuffled[swapIndex] = i;

       }
       this.indexInShuffled = new int[shuffled.length];
       for (int i = 0; i < shuffled.length; i++) {
           indexInShuffled[shuffled[i]] = i;
       }


       if (currentPosition < shuffled.length){
           int indexOfCurrentPositionInShuffle  = indexInShuffled[currentPosition];
           if(indexOfCurrentPositionInShuffle == 0){
               return;
           }else {

               int[] middle = {currentPosition};



               int amountToRight = shuffled.length - (indexOfCurrentPositionInShuffle + 1);

               right = new int[amountToRight];

               for (int i = 0 ; i < amountToRight; i++){

                   right[i] = shuffled[indexOfCurrentPositionInShuffle + i + 1];

                   LogHelper.e(TAG, "The value of " + (indexOfCurrentPositionInShuffle + i + 1)  + " = " + right[i]);
               }


               int[] left = new int[indexOfCurrentPositionInShuffle];

               for (int i = indexOfCurrentPositionInShuffle -1; i > -1; i--){

                   left[i] = shuffled[i];

                   LogHelper.e(TAG, "The value of " + indexOfCurrentPositionInShuffle + i  + " = " + left[i]);

               }


               int newLength = middle.length + left.length + right.length;
               int[] c  =  new int[newLength];

               System.arraycopy(middle, 0, c, 0, middle.length);
               System.arraycopy(left, 0, c, middle.length, left.length);
               System.arraycopy(right, 0, c , middle.length+ left.length, right.length);

               shuffled = c;

               this.indexInShuffled = new int[shuffled.length];
               for (int i = 0; i < shuffled.length; i++) {
                   indexInShuffled[shuffled[i]] = i;
               }

           }



       }



   }

   @Override
   public int getLength() {
       return shuffled.length;
   }

   @Override
   public int getNextIndex(int index) {

       int shuffledIndex = indexInShuffled[index];
       if (++shuffledIndex < shuffled.length) return shuffled[shuffledIndex];
       else return C.INDEX_UNSET;
   }

   @Override
   public int getPreviousIndex(int index) {
       int shuffledIndex = indexInShuffled[index];
       return --shuffledIndex >= 0 ? shuffled[shuffledIndex] : C.INDEX_UNSET;
   }

   @Override
   public int getLastIndex() {
       return shuffled.length > 0 ? shuffled[shuffled.length - 1] : C.INDEX_UNSET;

   }

   @Override
   public int getFirstIndex() {
       return shuffled.length > 0 ? shuffled[0] : C.INDEX_UNSET;
   }

   @Override
   public ShuffleOrder cloneAndInsert(int i, int i1) {
       LogHelper.e(TAG, "I was cloned and inserted", "");

       return null;
   }

   @Override
   public ShuffleOrder cloneAndRemove(int i) {
       LogHelper.e(TAG, "I was cloned and removed", "");

       return null;
   }

   @Override
   public ShuffleOrder cloneAndClear() {
       LogHelper.e(TAG, "I was cloned and cleared", "");

       return new UduxShuffleorder(0, 0, new Random());
  }
}

So the idea of mine is that when shuffle is clicked, the current position becomes the first song and the remaining length of the song is shuffled.

i make the player use the shuffle Order like this

            //This method is used to initialize the media source
            clearSource();
            mediaSource = new ConcatenatingMediaSource();
            for (Track track : songManager.getCurrentPlayList()) {
                if (track != null && !track.getSource().isEmpty()) {
                    HlsMediaSource source = new
                            HlsMediaSource(Uri.parse(track.getSource()), cacheDataSourceFactory(), 5, null, null);
                    mediaSource.addMediaSource(source);
                } else {
                    Log.e("Bad_Track", track.toString());
                }
            }
            //Prepare the player here
            preparePlayer();
            // we want to move to the actual song we wanna play because exoplayer
            // plays the first song immediately after preparing the source
            mExoPlayer.seekTo((int) item.getQueueId(), C.TIME_UNSET);

            mHandler.postDelayed(() -> {
                if (songManager.isShuffleMode()) {

                    ShuffleOrder order = new UduxShuffleorder(songManager.currentPlayList.size(), 
                            mExoPlayer.getCurrentWindowIndex(), new Random());
                    
                    getShuffledOrder(order);
                    //Set the shuffle Mode to the Player and Hope to God it doesnt crash
                    mediaSource.setShuffleOrder(order, () -> mExoPlayer.setShuffleModeEnabled(true));
                    songManager.setShuffleMode(true);


                    //send the signal to the player that we have shuffled the List
                    mCallback.onTracksShuffled(mExoPlayer.getCurrentWindowIndex());

                }

I used a handler to delay adding the shuffle order to the media source, because if i do it immediately, exoplayer thinks the shuffleOrder is longer than the mediaSource length and calls CloneAndRemove method of the shuffle order which isn't implemented and crashes the app. So with the handler, its sure the player would be ready before the shuffle order is set :) .

What brings me here is the unimplemented Methods
ShuffleOrder cloneAndRemove(int removalIndex);
ShuffleOrder cloneAndInsert(int insertionIndex, int insertionCount);
which i didnt implement because i didnt understand what the defaultShuffleOrder was doing.

Observation.
When i am in shuffle mode and i try to move a song to a new position, the above methods .are called, so because initially i returned null the app crashes at that point. to avoid the app crash, i took Exoplayer's default implementation of that method to mine. it does not crash but it does scatter the list :)

i would need help as to how to implement those method that when i move a song in shuffled mode to another position, it behaves normally without randomising the list anymore
Assuming i have a Shuffled list

1 
3
2
4

moving song 1 to song 3 should not change the order of 2 and 4.

I hope i am clear enough. Thank you.

@tonihei tonihei self-assigned this Jan 23, 2019
@tonihei
Copy link
Collaborator

tonihei commented Jan 23, 2019

The closeAndInsert and cloneAndRemove methods need to be implemented so that all the dynamic updates work correctly. As far as I understand, your main requirement is that the shuffle order starts with the current window index. Instead of implementing your own ShuffleOrder, you could use our DefaultShuffleOrder and provide the pre-shuffled list as you like to have it in the constructor. Something like:

int[] shuffledIndices = 
    createMyOwnShuffleOrder(
        /* firstIndex= */ player.getCurrentWindowIndex(),
        /* totalNumberOfSources= */ mediaSource.getCount());
ShuffleOrder shuffleOrder = new DefaultShuffleOrder(shuffledIndices, randomSeed);
mediaSource.setShuffleOrder(shuffleOrder, () -> player.setShuffledModeEnabled(true);)

If you make changes to the playlist after that point, the changes are automatically reflected in the shuffled order. For example if you move the source C from a shuffled order of DACB somewhere else, C also gets moved in the shuffled order to a new random place. For example CDAB. The rest of the sources stay in the same shuffled order.

Two further points:

  • The posting with the Handler shouldn't be necessary. Setting a new shuffle order which has the same length as mediaSource.getCount() should work at any point in time.
  • If you want playback to start at an index which is not 0, please call player.seekTo before calling player.prepare so that the player can immediately start loading the correct source.

@mathemandy
Copy link
Author

mathemandy commented Jan 23, 2019

The method

  private DefaultShuffleOrder(int[] shuffled, Random random) {
      this.shuffled = shuffled;
      this.random = random;
      this.indexInShuffled = new int[shuffled.length];
      for (int i = 0; i < shuffled.length; i++) {
        indexInShuffled[shuffled[i]] = i;
      }
    }

is private in Exoplayer and i do not have access to it
Exo Version : implementation 'com.google.android.exoplayer:exoplayer:2.9.1'

@tonihei
Copy link
Collaborator

tonihei commented Jan 23, 2019

It's public in the current release, so maybe you need to update.

@mathemandy
Copy link
Author

mathemandy commented Jan 23, 2019

alright , i updated, here is where it gets tricky :)
We have List A

Track A
Track B
Track C
Track D
Track E

on Shuffle we have

Track A
Track E 
Track C
Track B
Track D

so when i move Track A to Track E
so my UI ends up like this

Track E 
Track A
Track C
Track B
Track D

when i skip to next track , it plays someother track apart instead of C then returns to the normal flow. something like that

so my question is am i to send the position of current track in original list to exoplayer or the position of current track in shuffled list.

for the test above, i am using position of the list on the UI which should mean position of song in shuffled list.

@tonihei
Copy link
Collaborator

tonihei commented Jan 23, 2019

Enabling the shuffle mode does only change the order in which media sources are played, not their actual order in the playlist. That means player.getCurrentWindowIndex() and concatMediaSource.getMediaSource(index) return the same values as before no matter if you enable or disable shuffle mode. The only thing that changes is that the player no longer plays index i+1 after playing index i. You can query the Timeline to get all the details of the current shuffled order.

This behaviour also means that concatMediaSource.move(from, to) still refers to the positions in the unshuffled, original playlist.

Would it help to have methods ShuffleOrder.getShuffledIndex(unshuffledIndex) and ShuffleOrder.getUnshuffledIndex(shuffledIndex)? I guess that would simplify move operations.

@mathemandy
Copy link
Author

mathemandy commented Jan 23, 2019

that would be nice because i have something like that to0

 public int getPositionInShuffledMode(int currentPosition) {
        songManager.activeTrack = songManager.getPreviousPlaylist().get(currentPosition);
        int indexInShuffledList = songManager.currentPlayList.indexOf(songManager.activeTrack);
        return indexInShuffledList == -1 ? currentPosition : indexInShuffledList;

    }

i just needed a pointer. I will work accordingly.
it would be nice to have those methods handy :)

Thanks for all

@tonihei
Copy link
Collaborator

tonihei commented Jan 23, 2019

Reopening to mark as an enhancement to add these methods.

@tonihei tonihei reopened this Jan 23, 2019
@tonihei tonihei changed the title Exoplayer Shuffle order Add ShuffleOrder.getShuffledIndex and .getUnshuffledIndex for ease of use. Feb 8, 2019
@tonihei
Copy link
Collaborator

tonihei commented Oct 15, 2019

To further simplify updates to the shuffled order, we should also add a method like int[] getShuffledIndices() so that this array can be updated and then passed back to new DefaultShuffleOrder(shuffledIndices) if needed.

@naveensingh
Copy link

when i skip to next track , it plays someother track apart instead of C then returns to the normal flow.

@tonihei I'm also facing a similar issue as @mathemandy in the comment quoted above. The next track reported by the currentTimeline.getNextWindowIndex(unshuffledIndex) is not the one that is actually played. This is not about having the convenience methods but those would be nice too.

Steps to reproduce:

  1. Prepare media items in ABCDE order and play media item at first index i.e. A
  2. Enable shuffle mode. Let's say the order is now EACDB (remember, as per the docs, the underlying playlist is never shuffled i.e. it's still in the original ABCDE order)
  3. We'll call Player.moveMediaItem(unshuffledIndexOfD, Player.currentMediaItemIndex + 1) such that D is the next media item in the original playlist after current item A (in my case, this has no apparent effect on the shuffled timeline)
  4. Construct the shuffled indices from the timeline, and move the index of D to just after the index of A to create a new shuffle order such that the shuffled order EACDB is changed to EADCB so that D is the next media item that is actually played.
  5. Listen for onTimelineChanged (as a result of step 3) and set the new shuffle order created in step 4.
  6. At this point, the shuffled timeline should have an order like EADCB. If I query the timeline for shuffled indices, it's the same as the one set with shuffle order in the previous step but the nextMediaItem always returns some other item, and the expected item is only played after this other item.

Other observations:

  • The problem is gone if I remove step 3 and just set the shuffle order directly without modifying the original playlist.
  • If I replace moveMediaItem(index, currentMediaItemIndex +1) in step 3 with addMediaItem(currentMediaItemIndex, mediaItem), everything works flawlessly. The problem only happens if I do moveMediaItem and then set a new shuffle order according as per the new index. (either immediately or in onTimelineChanged)
  • I'm not sure how everything works internally but it seems the timeline is not properly updated if setShuffleOrder is called after moveMediaItem but it is updated properly if one calls addMediaItem. If I do some other variation like, moveMediaItem then wait for onTimelineChanged before querying the timeline for indices, and then immediately set the shuffle order, some other random behavior emerges (you could basically say it reshuffles some of the items instead of just moving one)

Code used to query the timeline:

val Player.currentMediaItemsShuffled: List<MediaItem>
    get() = if (shuffleModeEnabled) {
        shuffledMediaItemsIndices.map { getMediaItemAt(it) }
    } else {
        currentMediaItems
    }

val Player.shuffledMediaItemsIndices: List<Int>
    get() {
        val indices = mutableListOf<Int>()
        var index = currentTimeline.getFirstWindowIndex(shuffleModeEnabled)
        if (index == -1) {
            return emptyList()
        }

        repeat(currentTimeline.windowCount) {
            indices += index
            index = currentTimeline.getNextWindowIndex(index, Player.REPEAT_MODE_OFF, shuffleModeEnabled)
        }

        return indices
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants