Syncplay / syncplay

Client/server to synchronize media playback on mpv/VLC/MPC-HC/MPC-BE on many computers
http://syncplay.pl/
Apache License 2.0
2.09k stars 213 forks source link

Add --profile=pseudo-gui to mpv arguments #59

Closed wiiaboo closed 9 years ago

wiiaboo commented 9 years ago

Introduced in mpv 0.9.0 and in git master a bit after 0.8.3 was released. It doesn't ruin anything if run on older mpv and it's equivalent to running mpv with

terminal=no
force-window=yes
idle=once

This profile is used by default when opening mpv.exe/mpv.app/mpv.desktop but through Syncplay it gets disabled for some reason, so configuration that depends on pseudo-gui doesn't work.

I'm not sure if Syncplay depends on --terminal because --terminal=no disables stdout/stdin.

Information on pseudo-gui.

Et0h commented 9 years ago

Thanks for the suggestion. To test this please run Syncplay with -- --profile=pseudo-gui --terminal=yes as the last Syncplay command line argument (this uses the prepend feature explained in http://syncplay.pl/guide/client/ which passes arguments onto the media player).

Using the pseudo-gui profile seems to break things without --terminal=yes because, as you say, the terminal=no bit disables the stdin/stdout functionality that Syncplay needs. As such, I guess it might be wise for us to specifically add terminal=yes as a default Syncplay command line switch for mpv, so that if users decide to run mpv via Syncplay with the psuedo-gui profile it won't break things.

Given that Syncplay already uses the --idle and --force-window command line switches I'm not sure what running it with the pseudo-gui profile would really add. Yes, it would mean that it would use any user-specified options from the pseudo-gui profile, but they can already do that using the command line switch and users might be better using a Syncplay-specific profile in any case.

One option would be for Syncplay to run with --profile=Syncplay by default, and then people can specify a Syncplay-specific profile (which could be identical to their pseudo-gui profile if they wished, or could be configured for the particular use-case of Syncplay).

The current default arguments for mpv with Syncplay are --force-window --idle --hr-seek=always --quiet --keep-open -af scaletempo --input-terminal=no --input-file=/dev/stdin --term-playing-msg=\nANS_filename=${filename}\nANS_length=${=length}\nANS_path=${path}\n

wiiaboo commented 9 years ago

It was mostly because of custom configs dependant on pseudo-gui, but since --profile is supported since before mpv existed it could be just a matter of adding --profile=syncplay and then you could use your own configs anyway.

In my case I used loadscripts=no for the default profile and loadscripts=yes in pseudo-gui, so lua scripts didn't work as expected when I used syncplay.

My reasoning was that I expected mpv to behave in Syncplay like it does when I open it directly or through opening associated files.

wiiaboo commented 9 years ago

On second thought, we can't really use a separate profile, since mpv errors out if a profile doesn't exist.

Isn't there a way to detect 0.9.0 or newer and just send --profile=pseudo-gui --terminal in that case?

Et0h commented 9 years ago

Syncplay can detect version on *nix but not Windows. It would be useful to hear from other mpv users regarding what profile behaviour they think Syncplay should exhibit.

wiiaboo commented 9 years ago

Why's that? Because lachs0r's Windows builds are always from Git?

Et0h commented 9 years ago

That's my understanding. I've not actually tested it in the past few months, so it is possible that things have changed since last time there was cause to check.

Et0h commented 9 years ago

Apparently mpv Linux builds can also fail to give a proper version when they are just Git HEAD.

wiiaboo commented 9 years ago

Well, yeah, since mpv only gets proper versions on the respective branches, never on git master.

On 28 April 2015 at 23:49, Etoh notifications@github.com wrote:

Apparently mpv Linux builds can also fail to give a proper version when they are just Git HEAD.

— Reply to this email directly or view it on GitHub https://github.com/Syncplay/syncplay/issues/59#issuecomment-97248151.

Et0h commented 9 years ago

I've discussed your suggestion with others and the conclusion is that we shouldn't force people into any specific profile and it could cause issues with older versions of mpv, so it's probably best not to add it by default. However, in light of your suggestion we have added load_scripts=yes and terminal=yes as default arguments to prevent profiles from breaking Syncplay, and we have added an explanation of how to set the profile to pseudo-gui to the tips and tricks part of the Syncplay client guide.

wiiaboo commented 9 years ago

Hmm, i really don't think load-scripts should be hardcoded like that. It is set by default but options set by syncplay will override what users have in their configs and that's not good. Syncplay doesn't need load-scripts at all. Also, you can detect for pseudo-gui by reading --profile=help. Even if it's an older version, if it shows there it won't error out.

On Fri, 8 May 2015 09:49 Etoh notifications@github.com wrote:

Closed #59 https://github.com/Syncplay/syncplay/issues/59.

— Reply to this email directly or view it on GitHub https://github.com/Syncplay/syncplay/issues/59#event-300053848.

Et0h commented 9 years ago

Oh sorry, I thought you were saying it was necessary. I'll not hard code it if it isn't

wiiaboo commented 9 years ago
>>> import subprocess
>>> d=subprocess.check_output(['mpv','--profile=help'])
>>> d
'Available profiles:\r\n\textension.webm\t\r\n\textension.gif\t\r\n\tpseudo-gui\t\r\n\r\n'
>>> 'pseudo-gui' in d
True

We don't even need re.search/match here.

Et0h commented 9 years ago

As anyone capable of configuring the profile would be power users, they would be more than capable of using a command line argument to tell Syncplay to pass on a message to mpv to load a profile, and they may very well want to use a profile other than pseudo-gui for any case (either one or more Syncplay-specific profiles). Thanks for putting the time into making your suggestion and helping to clarify matters.

wiiaboo commented 9 years ago

Might as well take --terminal=yes off too since if --profile=pseudo-gui is used as argument for syncplay it'll override --terminal again.

Et0h commented 9 years ago

Good point. I've now made hardcoded command line arguments take precedence, so --terminal=yes should make a difference.