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

Use Twisted for VLC communication #353

Merged
merged 6 commits into from
Sep 20, 2020
Merged

Use Twisted for VLC communication #353

merged 6 commits into from
Sep 20, 2020

Conversation

Et0h
Copy link
Contributor

@Et0h Et0h commented Sep 17, 2020

No description provided.

albertosottile and others added 3 commits May 28, 2019 22:03
Rationale: asyncore/asynchat are deprecated since Python 3.6 and
are going to be removed from the standard library from Python 3.10.
It is unclear if these libraries will be picked up by maintainers
and independently published on PyPI. At the moment, we are working
on replacing them, in this commit with Twisted LineReceiver.

Known issues: does not work with GUI. There is a conflict with
qt5reactor -> "QSocketNotifier: Can only be used with threads
started with QThread".
Sending the call to transport.write wrapped in a
self.reactor.callFromThread instead of directly does the trick.
Include also fixes to allow correct quit of VLC and Syncplay when
either one is closed.

Known issues: there is a noticeable lag (~ 1 second) between the
start of Syncplay MainWindow and the start of VLC.
@Et0h Et0h requested a review from albertosottile September 17, 2020 20:40
Copy link
Member

@albertosottile albertosottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember writing this a year ago... I if I recall this correctly, preliminary tests on macOS worked fine, but this MR should be extensively tested on all the other platforms.

Also, there is this note in one of the commit messages that probably deserves an investigation:

Known issues: there is a noticeable lag (~ 1 second) between the start of Syncplay MainWindow and the start of VLC.

syncplay/players/vlc.py Outdated Show resolved Hide resolved
syncplay/players/vlc.py Show resolved Hide resolved
syncplay/players/vlc.py Outdated Show resolved Hide resolved
@Et0h
Copy link
Contributor Author

Et0h commented Sep 20, 2020

Known issues: there is a noticeable lag (~ 1 second) between the start of Syncplay MainWindow and the start of VLC.

It's slightly faster than that for me on Windows - there is only usually a notable delay if I'm running VLC for the first time.

@Et0h
Copy link
Contributor Author

Et0h commented Sep 20, 2020

Thanks for your work on this Alberto.

@Et0h Et0h merged commit dd8c864 into master Sep 20, 2020
albertosottile added a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
* Port VLCPlayer from asyncore/asynchat to Twisted

Rationale: asyncore/asynchat are deprecated since Python 3.6 and
are going to be removed from the standard library from Python 3.10.
It is unclear if these libraries will be picked up by maintainers
and independently published on PyPI. At the moment, we are working
on replacing them, in this commit with Twisted LineReceiver.

Known issues: does not work with GUI. There is a conflict with
qt5reactor -> "QSocketNotifier: Can only be used with threads
started with QThread".

* Fix QSocketNotifier issue with qt5reactor

Sending the call to transport.write wrapped in a
self.reactor.callFromThread instead of directly does the trick.
Include also fixes to allow correct quit of VLC and Syncplay when
either one is closed.

Known issues: there is a noticeable lag (~ 1 second) between the
start of Syncplay MainWindow and the start of VLC.

* Re-add try/except to VLC

* Bring back missing try

Co-authored-by: Alberto Sottile <alby128@gmail.com>
albertosottile added a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
* Port VLCPlayer from asyncore/asynchat to Twisted

Rationale: asyncore/asynchat are deprecated since Python 3.6 and
are going to be removed from the standard library from Python 3.10.
It is unclear if these libraries will be picked up by maintainers
and independently published on PyPI. At the moment, we are working
on replacing them, in this commit with Twisted LineReceiver.

Known issues: does not work with GUI. There is a conflict with
qt5reactor -> "QSocketNotifier: Can only be used with threads
started with QThread".

* Fix QSocketNotifier issue with qt5reactor

Sending the call to transport.write wrapped in a
self.reactor.callFromThread instead of directly does the trick.
Include also fixes to allow correct quit of VLC and Syncplay when
either one is closed.

Known issues: there is a noticeable lag (~ 1 second) between the
start of Syncplay MainWindow and the start of VLC.

* Re-add try/except to VLC

* Bring back missing try

Co-authored-by: Alberto Sottile <alby128@gmail.com>
@smiba
Copy link

smiba commented Oct 25, 2020

This may have broken VLC support for some, my friend coudn't use VLC with Syncplay v1.6.6 anymore. Syncplay would just simply not find the running VLC instance and it behaved as nothing was connected.

Downgrading to v1.6.5 fixed it.

Let me know if you would rather have me make a issue about it

@Et0h
Copy link
Contributor Author

Et0h commented Oct 25, 2020

@smiba Thanks for bringing this to our attention. It would be helpful if you could create a new issue about this which includes the following information:

  • What version of VLC was used.
  • What Operating System was used.
  • By what means was Syncplay installed.
  • What is meant by: "Syncplay would just simply not find the running VLC instance and it behaved as nothing was connected."

@Et0h
Copy link
Contributor Author

Et0h commented Nov 25, 2020

The issue raised by @smiba is hopefully resolved by the fix discussed in #365.

@Et0h Et0h deleted the twistedVLC branch December 4, 2020 22:27
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.

3 participants