rasmuslos / AmpFin

Native Jellyfin music player for iOS & iPadOS
Other
143 stars 11 forks source link

Set playSessionId to control cache behavior #57

Closed gnattu closed 1 month ago

gnattu commented 1 month ago

After further investigation, I noticed that the server's cache behavior is actually controllable. The client can supply a PlaySessionId in the request query string, which will be passed to the HLS builder and ultimately the transcode manager. When the client does not specify a session ID, the transcoder will only hash the transcode output filename with the input filename, the user agent, and the device ID. These values do not change if the client device is the same and is requesting the same audio file, effectively caching the transcoded file for that client for 24 hours.

When the client supplies a session ID, the server will hash the transcoded filename additionally with the session ID, so that requests from the same device will not share the cache when the session ID is different. This PR added a mechanism to use the session ID to control the server cache behavior. When requesting the audio stream, hash the trackId and the current bitrate limit as the PlaySessionId. This would force the server to generate a new stream immediately when the bitrate limit has changed. The preloading would still work when the bitrate limit does not change, as the PlaySessionId would still be the same in this case, meaning the server would return the pre-generated HLS playlist instead of creating a new one.

When playback is confirmed to stop, we can supply this PlaySessionId to the ReportPlaybackStopped endpoint. The server would then look up if this PlaySessionId has an associated transcoding job, and if so, it will clean the transcoded files. For any reason that the playback stopped event is not sent to the server, the file will be cleaned in 24 hours.

This PR also changed the deprecated api_key query string to new ApiKey. We are going to remove some deprecated endpoints in 10.10 so change the query string to accommodate for this change. This query string would still work for 10.9 servers. I have not tested this with 10.8 server though, and if that does not work you might want to add extra options.

The download logic has not changed yet because I'm still thinking about the correct way of doing that. Obviously downloads does not call the playback has stopped endpoint so the files are not cleaned up. Is it OK to leave the transcode cache stay for 24hours?

rasmuslos commented 1 month ago

This is great, way better then Bitrate changes taking 24 hours to process. I don’t have access to a computer until next month, so I will not be able to merge this, but I noticed two things while looking at this on my phone:

  1. When the Bitrate changes https://github.com/rasmuslos/AmpFin/blob/8ad41716074e43f5942836fff8ec6ca1db95d697/AmpFinKit/Sources/AFPlayback/LocalAudioEndpoint/LocalAudioEndpoint%2BHelper.swift#L123 is called, which should also update the session id in the playback reporter. The old cached file will not be cleaned, but the new session id is the correct one, not the old one
  2. Waiting 24 hours for downloads to get cleaned up is not to bad in my option, way better than reporting inaccurate listening events. But I would append a a session id to download requests, too, to ensure that they have the correct bitrate
gnattu commented 1 month ago

The old cached file will not be cleaned, but the new session id is the correct one, not the old one

This is a minor issue as the version returned to us is the correct one. Just the cache cleaning is delayed to the next 24 hour, but yes it would be better if we can handle this more accurately.

But I would append a a session id to download requests, too, to ensure that they have the correct bitrate

Sounds reasonable

gnattu commented 1 month ago

Pushed changes that updates playback reporter's session id and also attach the seesion id to downloads. With this change, when bitrate changed during the playback the playbackreporter will update the session id and send the playback stopped event with the last updated session id. This will instruct the server to delete the last used cache, but the cache for previously used bitrate will still stay for 24 hours. I personally think this is acceptable so that we can simplify our implementation and not re-create a new instance of a playback reporter every time the bitrate changes. This can also potentially reduce the server load as the device might be in an unideal wifi range which jumps back and forth from cellar and wifi, with our current implementation, the server can simply reuse the cache instead of keep spawning ffmpeg for transcoding.

rasmuslos commented 1 month ago

I got food poisoning, so I am back early. Looks good, thanks! 👍