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

Improve playlist advancement for VLC and mpv (#334) #397

Merged
merged 23 commits into from
Mar 28, 2021
Merged

Conversation

Et0h
Copy link
Contributor

@Et0h Et0h commented Mar 4, 2021

This change is intended to improve the reliability of playlist advancement in VLC and mpv.

This is achieved in two separate ways:

  • For mpv, Syncplay now detects when the 'eof-reached' property in mpv is set to true.
  • For VLC, Syncplay now detects changes to duration when playing from a URL (which was previously stuck at 0:00 if duration was not detected after a short delay when the YouTube video etc was opened) which allows it to more reliably detect EOF (end of file). This does result in a file change message to appear when the duration is updated. If this really bothers anyone then they can make a PR which hides that message.

PLEASE TEST THIS AND LET ME KNOW IF THINGS WORK WELL FOR PLAYLIST ADVANCEMENT FOR BOTH FILES AND STREAMS (YOUTUBE) FOR MPV AND VLC.

Because this required changes to both the Syncplay-side code and the player-side code I will leave this pull request open longer than usual to give people time to test this and let me know if it works or if it creates new problems.

Note: If you are alone in the room then by default Syncplay should advance to the next item in the playlist but should not start playing it.

You can download a portable version of this PR for Windows from: https://ci.appveyor.com/api/buildjobs/l41deaa3lo5kbwrb/artifacts/Syncplay_1.6.8_Portable.zip

daniel-123 and others added 13 commits March 5, 2021 23:10
Removing old and unmaintained changelog file.
* Disable AppVeyor

* Actions: disable other jobs

* Actions: implement windows job

* Use requirements

* Typo in version parser

* Replace type nul for PowerShell

* Change Python version to 3.7

* buildPy2exe: exclude tcl and tkinter

* buildPy2exe: rename zip archive to include extension

* Actions for Windows: build on Python 3.8

* Re-enable build flow for other platforms

* Remove AppVeyor configuration file
Make it so that server and full packages are separate artifacts in CI rather than single zip containing both.
Update to version 1.1.13 to hopefully address #322 fully.
Temporary workaround for AppImage built on Ubuntu 20.04 crashing with segmentation fault. #401
@Et0h
Copy link
Contributor Author

Et0h commented Mar 14, 2021

Could you please test this @Canageek and @nnamrrehdlopoel and let me know if it works for you?

@Canageek
Copy link

Spent an evening testing with a friend with both Syncplay programs set to use VLC, and that worked perfectly. Will try with other clients soon.

@Et0h
Copy link
Contributor Author

Et0h commented Mar 14, 2021

Spent an evening testing with a friend with both Syncplay programs set to use VLC, and that worked perfectly. Will try with other clients soon.

Thanks for testing. Were you trying YouTube videos in VLC or just regular files?

@Canageek
Copy link

With youtube videos, should we also test files?

I've got mpv, mpv.net, Media Player Classic: Black Edition, and VLC set up and ready for testing.

@Et0h
Copy link
Contributor Author

Et0h commented Mar 14, 2021

With youtube videos, should we also test files?

Testing with both would be ideal.

I've got mpv, mpv.net, Media Player Classic: Black Edition, and VLC set up and ready for testing.

I don't think I made any changes to the MPC code but nothing wrong with testing it. I'm not sure if there is proper YouTube handling for MPC-BE.

@Canageek
Copy link

Canageek commented Mar 14, 2021

mpv.net/mpv.net
Testing setup: /. Both connected to internet over the same connection (desktop wired, laptop wifi) so likely connecting to the same area of Youtube's CDN. The latest version of mpv.net and youtube-dl has been installed (mpv.net 5.4.8.0, youtube-dl 2021.03.14) with the linked Syncplay 1.6.8 Development.

Had video playing on both laptop and desktop. On Desktop right-clicked on second video in playlist and selected Open Media Stream URL. Desktop started playing this right away. Laptop took several seconds to catch up with the change, and reset video back to the start twice before both were in sync.

On advancement from 2nd to 3rd video in playlist got the following messages:
[12:13:14] Connection with server timed out
[12:13:14] Storm-Laptop has left
The laptop shows a Connection with server timed out error.
Could no longer continue testing with this session
Moved to Syncplay:8996 after this

After this video both desktop and laptop got "[12:19:39] Connection with server timed out" (laptop timestamp was 12:19:42)

@Canageek
Copy link

Canageek commented Mar 14, 2021

vlc/--
With only one client connected to the room VLC advanced smoothly to the next video (tested as I was installing updates to VLC on the other computer)

VLC 3.0.12 Vetinari

@Canageek
Copy link

vlc/vlc (laptop/desktop) advanced from video 1 on playlist to video 2 on playlist smoothly.

@Canageek
Copy link

mpv/mpv,

mpv 0.33.0-88-gd1be8bb606 Copyright © 2000-2020 mpv/MPlayer/mplayer2 projects
built on Sun Feb 28 09:25:17 +08 2021
FFmpeg library versions:
libavutil 56.66.100
libavcodec 58.126.100
libavformat 58.68.100
libswscale 5.8.100
libavfilter 7.107.100
libswresample 3.8.100
FFmpeg version: git-2021-02-28-85ab9deb

Same issue as mpv.net, [13:17:45] Connection with server timed out

@Canageek
Copy link

Going to stop testing until that error is fixed, then I'll work on mpv/mpv.net, vlc/mpv, and vlc/mpv.net at the very least

@Et0h
Copy link
Contributor Author

Et0h commented Mar 18, 2021

Are there any relevant errors in %APPDATA%/.syncplay.log?

The mpv issue with the playlist might be resolved by #400 so when that's merged we can update this accordingly.

@Canageek
Copy link

Canageek commented Mar 18, 2021

So, .syncplay.log was in the Syncplay_1.6.8_Portable folder. However, it had a bunch of stuff in there from when I tried to use mplayer that was unrelated, so I deleted it and tried this again. Only one client connected to the room, using the same version of mpv.net as last time.

Same set of errors:

[12:31:44] [myname] jumped from 00:00 to 00:00
[12:31:44] [myname] paused
[12:31:56] Connection with server timed out

and no log was generated in Syncplay_1.6.8_Portable this time.

@Et0h
Copy link
Contributor Author

Et0h commented Mar 22, 2021

@Canageek Could you please test https://github.com/Syncplay/syncplay/suites/2310287485/artifacts/48495030 and let me know if it fixes the issues you had with mpv.

@Et0h
Copy link
Contributor Author

Et0h commented Mar 28, 2021

No issues have been removed about the update version of this PR, so I'll be merging this to allow for broader testing in advance of the a planned beta release (which will hopefully be around the 5th of April if everything goes smoothly).

@Et0h Et0h merged commit c063369 into master Mar 28, 2021
@Et0h Et0h deleted the playlist-eof branch March 28, 2021 19:19
@Canageek
Copy link

Canageek commented Apr 2, 2021

Sorry, I've been sick and just got to this today. Should I still test the linked version?

@Canageek
Copy link

Canageek commented Apr 2, 2021

With mpv 0.33.0-102-g5824d9fff8 and youtube-dl 2021.04.01 it no longer disconnects and does transition to the next video, BUT does goes back to pausing after each video. Tested with only one client connected to the room.

@Et0h
Copy link
Contributor Author

Et0h commented Apr 4, 2021

@Canageek Thanks for your testing. It has now been merged into the main branch so the ideal version to test is the artefact from the latest commit which is https://github.com/Syncplay/syncplay/suites/2363843003/artifacts/50062530 but functionally it should be equivalent to testing the one I posted 13 days ago.

In terms of not unpausing when you change file, as per #315 it is the current expected behaviour that the auto-advancement will not result in an auto-play if you are the only one in the room:

Get to the end of a file with shared playlists are enabled It advances to the next file, resets to 0:00. Then auto-plays after 3 seconds unless someone in the room is set to not being 'ready' (or unless you are the only one in the room). (emphasis added)

@Canageek
Copy link

Canageek commented Apr 7, 2021

Have tested this for one morning with mpv on two computers, and one with mpv.net on two computers. This now advances as expected.

@Et0h
Copy link
Contributor Author

Et0h commented Apr 7, 2021

Have tested this for one morning with mpv on two computers, and one with mpv.net on two computers. This now advances as expected.

That is fantastic news. I look forward to including the improvements in the forthcoming release!

Et0h added a commit that referenced this pull request Aug 2, 2021
* Create pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Do not show playback speed change notifications in OSD

* Upbuild and remove debug message

* Strip quotation marks from per-player arguments (#226)

* Delete unmaintained changelog.

Removing old and unmaintained changelog file.

* Add more MPC-HC paths (#398)

* Migrate from AppVeyor to GitHub Actions (#399)

* Disable AppVeyor

* Actions: disable other jobs

* Actions: implement windows job

* Use requirements

* Typo in version parser

* Replace type nul for PowerShell

* Change Python version to 3.7

* buildPy2exe: exclude tcl and tkinter

* buildPy2exe: rename zip archive to include extension

* Actions for Windows: build on Python 3.8

* Re-enable build flow for other platforms

* Remove AppVeyor configuration file

* Add manpages courtesy of Bruno Kleinert #387

* Install the manpages.

* Separate debian package artifacts

Make it so that server and full packages are separate artifacts in CI rather than single zip containing both.

* Fix server deb deployment name in CI workflow

* Fix package filename for debs

* Update mpv json ipc vendor code

Update to version 1.1.13 to hopefully address #322 fully.

* Update setup.py for vendor code of mpv jsonipc

* Build AppImage on Ubuntu 18.04

Temporary workaround for AppImage built on Ubuntu 20.04 crashing with segmentation fault. #401

* Update python_mpv_jsonipc to 1.1.13 keeping our changes

* Remove the AppVeyor badge as we stopped using it.

* Improve playlist advancement for VLC and mpv (#334) (#397)

* Advance playlist on end of file in mpv and VLC
* Update duration for streams to fix playlist advancement (#334)

* Add notice for Python in third party collection file (#404)

* Add notice for Python in third party collection file

* Convert third party notices file to plain text

* Adapt codebase to third party notices format change

* Mark as beta 1 (release 97)

* Mark as 1.6.8 final (build 98)

* Update pt_BR translation, fix typo (#422)

* Bundle libgthread-2.0.so.0 into AppImage

* add libxcb manually

* Add missing libxcb-util to build environment

* Enable GitHub Actions on pull requests

* Bundle libxcb1 into AppImage to fix #380

* Send 32-bit/64-bit context when updating

* Upver to 1.6.9 release 99

* Remove references to IRC (#430)

* Add reference to GitHub discussions

* Update issue templates

* Revert "Merge branch 'master' into master"

This reverts commit 173007e, reversing
changes made to 6105da8.

Co-authored-by: Daniel Wróbel <[email protected]>
Co-authored-by: Alberto Sottile <[email protected]>
Co-authored-by: Daniel Wróbel <[email protected]>
Co-authored-by: Atílio Antônio <[email protected]>
Co-authored-by: Teoh Han Hui <[email protected]>
Co-authored-by: Alberto Sottile <[email protected]>
Co-authored-by: Assistant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants