Conversation
There was a problem hiding this comment.
Probably better to move these callbacks to the ExoPlayer and listen to state changes from the player itself.
There was a problem hiding this comment.
Same here I'm not even sure we need to use these callbacks.
The player already reports back it's state to flutter we could probably use that for reporting seek/pause/play states. That way we don't rely on any additional kotlin implementation.
| modifier: Modifier = Modifier | ||
| ) { | ||
| val syncPlayState by VideoPlayerObject.syncPlayCommandState.collectAsState() | ||
| val visible = syncPlayState.processing && syncPlayState.commandType != null |
There was a problem hiding this comment.
This will recalculate whenever the state changes. Probably fine for a small composable but lets change this to
| val visible = syncPlayState.processing && syncPlayState.commandType != null | |
| val visible by remember(syncPlayState) { | |
| derivedStateOf { | |
| syncPlayState.processing && syncPlayState.commandType != null | |
| } | |
| } |
| Translate( | ||
| callback = { cb -> | ||
| when (syncPlayState.commandType) { | ||
| "Pause" -> Localized.syncPlayCommandPausing(cb) |
There was a problem hiding this comment.
Create a enum for this. Pigeon supports enum's that way both flutter/kotlin are in sync and we don't rely on Strings.
| } | ||
|
|
||
| // SyncPlay command state for overlay | ||
| data class SyncPlayCommandState( |
There was a problem hiding this comment.
Let's put this in pigeon.
| key: const Key("Search"), | ||
| onPressed: () => context.router.navigate(LibrarySearchRoute()), | ||
| child: const Icon(IconsaxPlusLinear.search_normal_1), | ||
| ); |
There was a problem hiding this comment.
Not sure about the position of this widget. Let's remove it from the dashboard for now.
We should not be using multiple fabs together in a single navigation rail.
We'll have to find a better spot.
| final isProcessing = ref.watch(syncPlayProvider.select((s) => s.isProcessingCommand)); | ||
| final processingCommand = ref.watch(syncPlayProvider.select((s) => s.processingCommandType)); | ||
|
|
||
| final (icon, color) = switch (groupState) { |
|
|
||
| String _getProcessingText(BuildContext context, String? command) { | ||
| return switch (command) { | ||
| 'Pause' => context.localized.syncPlaySyncingPause, |
There was a problem hiding this comment.
This is re-used quite a lot just adding a reminder to replace this with the enum and extension method.
| _loadGroups(); | ||
| } | ||
|
|
||
| Future<void> _loadGroups() async { |
There was a problem hiding this comment.
Would be cleaner to lift this state out of the widget and put it in a provider.
| flutter_native_splash: ^2.4.7 | ||
| macos_window_utils: ^1.9.0 | ||
|
|
||
| web_socket_channel: ^3.0.3 |
There was a problem hiding this comment.
Let's move it to "# Network and HTTP" group.
There was a problem hiding this comment.
First of thanks for implementing this, pretty big PR. But something a lot of people where requesting 👍🏼.
Works pretty well for the most part, some notes/quirks though. These are some initial findings will have to go over it after some changes.
About the UI itself. I left some comments about UI choices. However I will probably go over it myself to make some changes to bring it more in line with Fladder as it currently is.
UX:
We should show a loading indicator when any of the users press a play button. Now it has to load for a bit before the playback starts because Fladder is still synchronising.
When a player “stops” playback should all other participants return to the previous screen as well?
Architecture:
Currently most of the calls inside of the.UI go to videoplayerprovider but it now either calls the original player.pause or a new syncprovider.pause.
Like mentioned in the comments it would be better to listen to the players state stream and adjust everything based on that.
Bugs:
Playback stops working when syncplay becomes out of sync. Leaving/creating a group does nothing to change this state.
Fladder starts playback and finishes loading the video but it remains in a “paused” state as if it’s awaiting the syncplay to synchronize.
Sometimes “play” commands seem to not propagate to other users
|
Also mentioned in some comments. But there is a lot of re-formatting making it difficult to review the changes. Please re-format all files using the .vscode/settings.json. The biggest issue being the 120 line length currently not being used in your formatter. |
|
Hi! What's the status of this PR? Are the fixes discussed above still needed, or has everything been fixed? I'm curious because I'm really looking forward to this feature. Thanks for such a great app! |
|
Hi ! |
|
I you need someone to test it out I can help. |
|
I've pushed the fixes and added sync correction (SpeedToSync and SkipToSync) that mirrors the behavior of the syncplay plugin on the official jellyfin-web. @PartyDonut I did not change the added UI for the syncplay. If you have an idea of how it should look I can implement it or we can merge this PR to another branch where you can make the UI changes |
|
Thanks for the improvements, had a quick test it does seem more stable then previously. I will have a closer look next week. When we are in a group and the users presses play nothing really happens, however in the background it is starting the playback. We should show a "loading" overlay/pop-up for all the people in that group so users don't start pressing play multiple times. Can we also have a "user has joined" pop-up. Currently the groups creator does not see a message. If any of the users stops playback the other users keep on viewing is this the intended behavior? If yes should a user be able to join back? About the UI, it's fine to leave it as is when you are done with your work on the PR I will just merge this and create a new PR for easier reviewing. |
|
Hi! I built a build based on this PR and noticed the following: The next episode switcher isn't working correctly. When I press the button to start an episode, it starts right at the end of the timeline. Sometimes, after pausing and unpausing, someone in the group gets kicked out and has to rejoin, but there's no notification about leaving the group. Do you need any examples from me in the form of screenshots, screen recordings, or logs? The server logs show nothing unusual, but the client logs only show errors, which are also blank. |
PartyDonut
left a comment
PartyDonut
left a comment
There was a problem hiding this comment.
Sorry for the late response, have been a bit busy. Also sorry about the merge conflicts quite a lot of re-work needed to get music working.
I took a quick glance through the files, at this point there is no way for me to properly review this PR.
There are a lot of files that are marked as changed but are not related to the PR at hand (might be because of the music re-work?), at this point the best path forward would be to re-base it on develop make sure all the changes are related to the sync-play functionality.
|
While testing this branch I hit a bug: starting playback from Fladder in a SyncPlay group didn't start it on the official clients — TV episodes never started (movies did), and "Continue Watching" restarted the group from 0:00. Verified against both the official Jellyfin web client and the LG webOS app. Root cause: Fix: I opened a PR against the |
|
Thanks for the feedback. You were right that the diff was unreviewable. I've rebased onto the current
Happy to split further or adjust anything if it helps the review. |
|
I'd love to see this added, will it ever be merged? |
|
I pray this gets added soon. We need a syncplay feature for Android TV and any app that has it will immediately gain my #1 player! |
|
Sorry have been busy fixing other stuff and adding different features. @irican-f let me know if you still like to work on this I can have a look at merging this when the conflicts are resolved after the next release. |
Adapt the navigation rail FAB helper to upstream's ConsumerWidget refactor of SideNavigationRail (the widget.* accessors no longer exist).
…e close handling - Exponential backoff (2 s base, 30 s cap, +/-20 % jitter) that never gives up; one reconnect timer at a time. - Close and error events are matched to the channel that raised them, and the stream subscription is stored and cancelled on disconnect, so a forceReconnect() can no longer null out its successor or open a third socket. A handshake that loses the race with disconnect() closes itself, and a synchronous constructor failure drops to disconnected instead of wedging in connecting. - App resume only checks the socket (KeepAlive when healthy, reconnect otherwise); coming back online forces a reconnect.
…aiting state - PlaybackChangeSource.user becomes userPlayPause / userSeek so Flutter never has to guess what a paused-while-buffering frame means. ExoPlayer consumes a seek tag on the very next frame and a play/pause tag only when isPlaying actually flips, so the periodic poll can no longer spend the tag on a no-op frame. - The unused onUserPlay/onUserPause/onUserSeek Pigeon callbacks are gone; MediaPlay on the remote tags and plays like the other keys. - SyncPlayCommandType.waiting and TranslationsPigeon.syncPlayStateWaiting let the native overlay show "Waiting for others..." like the Flutter one. - New localized strings for halt/resume, not-following, locked speed, library access denied, disabled access and the drift-correction setting.
…e machine Audit of the port against jellyfin-web's SyncPlay plugin and the server's group states, plus the fixes from two reviews and device testing. Protocol - StateUpdate frames only update state; they never reply Ready. The server's Unpause command owns the start time; a paused player is only recovered when nothing of ours is armed, queued or executing. - Duplicate commands are correction checks (jellyfin-web applyCommand) against the server's 500 ms tolerance; commands for another playlist item are dropped (Stop excepted); commands wait for the first clock measurement, with a 2 s fail-open. - Positions are reported from the live player; a load and a Seek report the requested position with isPlaying=false. Spontaneous buffering is debounced for 3 s and closed by exactly one Ready. - Halt/resume via SetIgnoreWait: closing the player or "Stop group playback" halts and stops locally; commands received while halted are recorded so a resume starts at the live group position (last command, then PlayQueue timing). A silent rejoin after a socket drop keeps the halt and re-asserts IgnoreWait. Stop stops and closes the route. - Same-item PlayQueue frames attach in place; creating a group while watching seeds it with the local queue; next/previous go through NextItem/PreviousItem/SetPlaylistItem. LibraryAccessDenied is handled; a failed join never stops local playback. Player - User pause and seek are applied locally then requested; seek stays paused until the group's Unpause. play() is re-issued until the backend really plays (media-kit can drop it, and its web element swallows a rejected play promise); the real state comes from BasePlayer.isPlaying, not the wrapper's intent flag. - Track switches on a transcode no longer wait 5 s for an mpv track that a burned-in subtitle or a server-picked audio track cannot have. - Drift correction is throttled to 1.5 s and can be disabled in the player settings (enableSyncPlayCorrection). Tests cover command scheduling, duplicates, the recovery gate, the clock gate, recording while halted, queue navigation, position estimation, buffering debounce, message handling and reconnect backoff.
- Group sheet: create/join gated by the server's SyncPlay access policy, an explicit view when access is disabled, "Stop group playback" and "Resume playback" buttons, a not-following state, no duplicate toasts. The navigation-rail FAB is gone. - Centre overlay shows one thing per device from a shared resolution: queue switch, the command being applied, or the group waiting for a participant. Drift corrections stay in the badge. - Progress bar sends one Seek per release and never pauses the group on drag; the close paths halt group playback before stopping; playback speed controls are locked in a group. - Subtitle/audio picker highlights the tapped entry at once with a spinner while the switch and any reload run. - Player settings gain the SyncPlay drift-correction toggle.
|
@PartyDonut I will keep this PR up-to-date until you have some bandwith to review and test it |
|
Unfortunately, it seems that this doesn't work with jellyfin 12.0 (at least on the latest RC). I am not able to neither create nor join any room. Works fine with jf 10.11 |
|
For the people who want to use syncplay now, I am maintaining a fork with a fully working syncplay implementation. Works on jellyfin 12 and I plan on maintaining the project for upcoming releases. |
Any reason there couldn't be a PR rather than a separate fork? |
|
Obviously people are free to cherry pick my commits from Chudder but there are too many other changes made to my project for there to be a clean PR. A lot of the changes made on my fork are opinionated and are not likely to be changed in this repo. Syncplay is on android tv ofcourse and I've also made a lot of android tv optimizations. |
d9e7e85 to
97e30c8
Compare
|
I've added the fix to handle jellyfin 12 |
|
Thanks for keeping this branch up-to-date with develop so far 👍 While I'm not against the use of AI entirely. I do have to ask to remove claude or any other model/agent/llm as committer. You've also added changes to fix Jellyfin 12.0 authentication these are added to develop so they can be reverted as well. After those changes I can have a look at merging this branch to develop. |
Jellyfin reads the socket token from `ApiKey` unconditionally, but from `api_key` only when `EnableLegacyAuthorization` is set, which Jellyfin 12 turns off by default. A socket sending `api_key` alone is refused before the handshake completes, so every SyncPlay create and join timed out on a stock 12 server while the REST API kept working off the Authorization header. Match the rest of the app and send `ApiKey`. Extract the URI builder as a free function so the parameters are covered by a test, matching isPhonePlatform and reconnectDelay in the same file.
# Conflicts: # lib/providers/video_player_provider.dart
… out of Waiting The player registered `ref.listen(syncPlayProvider)` in init(), which made it a dependent of SyncPlay. The controller reads `videoPlayerProvider` back whenever the player route is open, and Riverpod's debug circular-dependency assertion then aborted every queue switch after the first start. Subscribe to the controller's state stream instead, and cancel it with the buffering debouncer in dispose(). loadPlaybackItem fired its Buffering report without awaiting it. An item that starts at 00:00 loads fast enough for the Ready to reach the server first, so the session was marked buffering again with no Ready to follow and the group stayed in Waiting. Await the report so the server always sees it before Ready. Document both scenarios and the behaviour decisions from the jellyfin-web audit.
This branch has not been deployed
9 participants
Pull Request Description
Adds Jellyfin SyncPlay support so users can watch media together in sync across devices.
docs/syncplay-implementation.mddocuments the protocol and architecture.Issue Being Fixed
Feature request: SyncPlay support for watching together with other Jellyfin clients.
Screenshots / Recordings
fladder_syncplay_demo_beta_compressed.mp4
Checklist
(Added:
web_socket_channel^3.0.3 — used for SyncPlay WebSocket. pub.dev; cross-platform.)