Feat/seek initial time

[jspizziri]

Based on the conversation and decisions made on this PR. This PR implements the following interface:

interface TrackPlayer {
  public async skipToNext(initialTime?: number);
  public async skipToPrevious(initialTime?: number);
  public async skip(index: number, initialTime?: number);
}

[dcvz]

Thanks for working on this @jspizziri ! As far as I can see, for the methods:

public async skipToNext(initialTime?: number);
public async skipToPrevious(initialTime?: number);
public async skip(index: number, initialTime?: number);

Internally you're just immediately seeking after you skip.. I think we might not actually have to modify these methods to take an initial time. It seems like we can already just do this directly from the JS side:

TrackPlayer.skipToNext()
TrackPlayer.seek(XXX)

[jspizziri]

@dcvz that is true. I'm not entirely sure if their 100% equivalent due to the RN bridge execution time but I'm no expert there. Doing it this way seems closer to the metal, but like I said, not an expert. What I will say is that it's definitely a nice convenience for what I believe is a very common use case, however if we don't want to support it I can understand that, and at that point my only potential concern would be the bridge execution time.

[dcvz]

@jspizziri could you do some tests of this? I think it should work.
Overall changing the API minimally I think is optimal

[jspizziri]

@dcvz I'm not 100% sure how to set up a reliable test for something like this but I'll do some more research on the react native/js bridge and see what I can find.

Anecdotally, I had a project with RNTP before where seek after skip was working, but the audio would play from position 0 for a fraction of a second before the seek happened. It could've been related to the async loading issue, but that's the kind of behavior I'm concerned about.

In the meantime I'm going to pull out all the mergeable work from this PR so that it doesn't get held up and submit another PR for it.

[dcvz]

Merged @jspizziri

[jspizziri]

@dcvz lol, you merged that one so quick I thought there was a bug with GitHub because when I went to go look for it I didn't see it in open PRs so I created a duplicate. 🤣

[jspizziri]

Just pushed a trimmed version of this PR rebased on the updates in main.

[jspizziri]

So the fundamental question is:

await TrackPlayer.skip();
// Can there ever be a significant delay between the execution of the above and below calls 
// which would result in them being executed in the native modules with enough delay to
// cause a noticeable stutter in playback for a user.
await TrackPlayer.seek(x);

TL;DR; based on my limited understanding, I believe it's theoretically possible.

This is an answer I don't feel super confident in, but here goes (I can't find much canonical information to make this more definitive):

Based on my (admittedly limited) understanding of how the JS event loop/queue works in combination with some of the documentation on RN Performance, my understanding of RN's use of their JS threads for execution (specifically the use of a single thread for business logic execution), and my understanding around how JS fundamentally async and non-blocking architecture works.

It seems entirely theoretically possible that a back to back calls could get placed on the JS execution queue with several other calls/operations getting queued between them (e.g things happening asynchronously elsewhere in the app). If there are several operations that require some noticeable amount of time to process that get queued between the two calls then the native side will start playing and then execute the seek once the other queued JS operations have been processed, thus resulting in a delay from the users perspective.

If the above understanding is correct it could be very challenging (at least for someone with my current level of understanding) to contrive a reproducible example where this is occurring, as it would be depending upon so many other factors of execution in the application & subsequently the JS thread in RN.

A more definitive answer would require input from someone who is far more deeply versed and experienced in the JS runtime and the React Native bridge than I am or will be able to become on a reasonable timeline for the purposes of this PR. Perhaps someone from the community can chime in.

Thoughts?

[dcvz]

Overall it sounds completely plausible and even expected that we cannot guarantee the calls run right after each other.. but I think fundamentally that does not change whether or not it would work.

I guess what might be possible as you mentioned is that maybe you hear audio for a bit before it actually seeks. But I'd honestly say, let's try to example project and attempt to skip then seek on it and see if we experience this. This might still give us an idea of how "likely/often/easily" this happens, or if it might just be in case we get some bigger call in between the two that causes a bigger delay before we seek.

Down the line this is something that the TurboModules changes might actually help us fix.

[dcvz]

We might also be able to ask the people on that feature request thread to give this a spin and see how it performs in their applications, so we have a concrete problem to solve and that we know is occurring regularly.

[jspizziri]

@dcvz I'm okay with that approach given your reluctance to change the API.

I'm going to upgrade to v2.2.0-rc1 and see if I notice anything in the app that we're working on now.

I also think that we can safely close #713 at this point which will hopefully push people over to this MR and may help with getting feedback from folks. I'll let people on that issue know and perhaps you can close it.

[dcvz]

I'm definitely reluctant to change API's as those need to be maintained in the future. But that doesn't mean we may not have to make changes, I'd first just like to see if it ends up being an issue in practice first.

That sounds like a good plan of action. I'll close it some time after your post.

[jspizziri]

@dcvz

Here's what I've found in my project after around 3hrs of testing different scenarios. The following scenarios have been tested on both Android and iOS with consistent results.

Things That Work

await TrackPlayer.skip(0, initialTime);
await TrackPlayer.play();

await TrackPlayer.skip(0);
await TrackPlayer.seekTo(initialTime);
await TrackPlayer.play();

Things That Don't

Stutters almost every time when skipping between tracks.

await TrackPlayer.skip(0);
await TrackPlayer.play();
await TrackPlayer.seekTo(initialTime);

Conclusion

While I personally think the proposed API is nicer to work with and makes more sense it seems that there's a workable solution (based on my testing). Given that you're the one on the hook for maintaining this package long-term I understand if you don't want to move forward with this PR until new evidence comes to light.

[dcvz]

Sounds like it won't really work the way I want it too 🥲 We may have to incorporate something like this in the end..

I'll do some thinking as well wether we'd be able to make it work. But I think probably not

[dcvz]

@jspizziri I actually have one more request. Could we make this PR against the kotlinaudio branch?

Have you already been trying to use the new betas/rc? We're getting ready to release that soon so it'd be great to have this there. Android side would have to be remade otherwise.

[jspizziri]

@dcvz , I'll do that today.

I only started using the 2.2.x version yesterday since that's where the SwiftAudioEx patch landed.

[jspizziri]

@dcvz I pushed up a functioning version of the kotlin implementation.

As you're looking at it there were a few questions I had:

1. I wasn't sure if you wanted to leave the seekTo(initialTime) behavior at the module level, or if you wanted to push that into the respective service layer for each skip call. LMK and I can adjust that (for both iOS and Android).
2. I wasn't sure how the player behaved if a user called skipToNext but there wasn't anything else in the queue. It doesn't seem like there's a success or failure response sent back. One potential concern I have is if they skipToNext(initialTime) but there's nothing in the queue. If it's a silent failure the current implementation would presumably result in the seeking to the track they're on currently.

I also want to be clear that I have not tested the windows implementation. I can do it if needed, but if its easy for someone else to do it it would probably be faster. I'd need to provision a windows device which I don't have atm.

[jspizziri]

@dcvz I know the world (and Europe in particular) is crazy right now, so I hope all is well with you. Assuming that everything ok I just wanted to ping you on this PR.

[jspizziri]

@mpivchev , any chance you could work with me to get this across the finish line?

Thanks!

[dcvz]

Thanks for following up again and being patient @jspizziri. It's been a bit hectic for us in the past weeks! I'll follow up on this later today or tomorrow morning.

I think its good to leave this at the module level as mentioned, with Turbomodules, we may be able to fix some of these kinks.

[dcvz]

Didn't get a chance yet @jspizziri but have not forgotten :) hoping to tomorrow.

[jspizziri]

Thanks @dcvz , appreciate you keeping me in the loop!

[martinmidtsund]

You asked for feedback from the other PR, sorry I'm a bit late, hectic here as well! 😅 So this is only input, I have not been able to test the code yet, sorry.

If this now works for both platforms, as you mention @jspizziri :

await TrackPlayer.skip(0);
await TrackPlayer.seekTo(initialTime);
await TrackPlayer.play();

Then that is sufficient for me, then we can use this to implement the same functionality in our app as skip(0, initialTime) would, wouldn't we? By listening to Event.PlaybackTrackChanged and seekTo if initialTime is set on the track? At least it is sufficient for our use case right now, though this will perhaps change in the future.

Great work!

[jspizziri]

@dcvz just wanting to check in on this. Thanks!

[jspizziri]

Just pinging this MR. Thanks!

[dcvz]

LGTM 👍 Thanks for this @jspizziri !
Sorry for the longer wait

[dcvz]

@jspizziri looks like we need one rebase, then I'll merge it in! Thanks a lot and sorry for the delay

[jspizziri]

@dcvz great news! And no worries at all, I know how it can go sometimes.

I just rebased and pushed, I'm going to run one more build & test just to make sure all is well and I'll let you know.

[jspizziri]

@dcvz alrighty everything looks good from my end. Let me know if you need anything else from me.

[jspizziri]

@dcvz just wanted to bump this a bit. Everything should be good to go!

[dcvz]

Finally merged @jspizziri ! Thanks a lot for this and your patience :)
I think in a different thread you expressed your interest in being a contributor and I see there's already a problem you want to tackle. Feel free to reach out to me via Discord or email - david [at] doublesymmetry (com) and we can find a way to involve you!