Refactor player.py #753
Refactor player.py #753
Conversation
|
MPRIS integration is badly broken with this patch (but you probably already know). What is very problematic is that we use mpv's JSON IPC to get real playback status from the player. This is the only way how to get true player status since user can control the player via keys (or even maybe via JSON IPC API). This can't be done in current approach. I suggest to not touch MPRIS part (at least for now). It runs in separate process and connects to player independently. Just tell it if it should expect mpv or mplayer. btw I think it might be reasonable to just support mpv. It is clearly better for us than mplayer (JSON IPC API). It should be supported everywhere mplayer runs. Since mps-youtube now only works reasonably with mpv or mplayer I think it might not be a great loss, but that of course needs some discussion. Users can use NOTIFIER to run custom scripts. |
I completely understand. I played around mpris.py for a few days, it seems I can't make it work the way it is implemented now. I am not familiar with multiprocessing and multiprocessing seems to be doing weird stuff to global variables(they don't update, weird!). The reason I went this way initially was because this would isolate player object from all other parts of mpsyt. This way writing a new player could be done with minimal knowledge of how mpsyt works. |
Multiprocessing launches true new OS process with it's own python interpreter and everything, so this is expected. You need some IPC or shmem to communicate. For mpris we use pipe.
Problem is that we currently really rely on mpv features. By design mps-youtube is tighly integrated with player. It's not just our shitty code that blocks us from making this bring-your-own-player architecture. |
Is there some very important reason to use multiprocessing? It would be more natural to not use it (We can't even see the exceptions happening in
I believe if we could call EDIT: |
There was a problem hiding this comment.
I have not fully looked into the Mpris part of the code base so I'll just refrain from commenting on that part, but I think this is great.
What I initially wanted out of this was to break all the view altering code out of player.py so we would be left with "pure" classes that only handles controlling the player(and potentially player specific view stuff)
Even if this doesn't allow us to make it a "bring your own player" system, its a good first step in that direction.
mps_youtube/players/mplayer.py
Outdated
| known_player = util.is_known_player(config.PLAYER.get) | ||
| if known_player: | ||
| pd = g.playerargs_defaults[known_player] | ||
| args.extend((pd["title"], song.title)) |
There was a problem hiding this comment.
As in #752, not encapsulating the title string in quotes seems to break playback on some systems. Maybe we could fix this at the same time by making sure all strings passed as arguments have quotation marks around them?
mps_youtube/players/mpv.py
Outdated
| known_player = util.is_known_player(config.PLAYER.get) | ||
| if known_player: | ||
| pd = g.playerargs_defaults[known_player] | ||
| args.extend((pd["title"], song.title)) |
There was a problem hiding this comment.
Just raising the same issue as with MPlayer here
Making mpris a more integral part of mpsyt would takes us a forward in that direction. I tried to do it, but failed miserably. It would take a partial rewrite of the current mpris implementation, but it seems like the separation of mpris from other mpsyt parts is there for a reason. @hrnr Looks like you did a lot of work in that part. Can you tell me why the separation is there? |
|
I definitely agree in the idea of using classes here (#610). Although I do also see @hrnr's point that it would be easier to support only mpv. Somewhat related to this: something that should be implemented for mpv at least is the ability to run a persistent mpv instance in the background (possibly using mpv as a library instead of using IPC). This would allow #127 to be implemented, and potentially have other benefits. |
|
Switching to libVLC would be the best then. mpv doesn't seem to have good python bindings(maybe I didn't look). But this would destroy the modularity of mpsyt. Shouldn't we be thinking of a method to support #127 while being modular. On a side note, wouldn't running |
|
Part of the difficulty with running the player in the background is that you want to redirect key input to the player when it if focused. If you look at #127, I linked to a partially broken hack where I did that by creating a pty (mpv actually tests that it's input is a real terminal; mplayer does not). Recent versions of mpv have a |
|
Can't we use socket comm (quit 43, for example, like in mpris), instead of key-presses? Only capturing keys would be the problem then. |
Why MPRIS runs as a separate process?
These two makes it necessary to run MPRIS as a separate process now. I quite also like that crashes in MPRIS will not cause main process to crash and playback can continue. You can argue that it should never crash, but people have so many configurations (I think we learned that from bug reports) it is easy to miss something. I have tried really hard in the last PR to ensure that MPRIS can not crash main process. |
|
Thank you for the clarification. So I guess there is no going back from split mpris. It should be possible to use the pipe the other way to listen commands from |
|
Yes, python pipes are bidirectional I think. I think we need to decide on some model for handling player. I think what @ids1024 is proposing about having a persistent player instance makes sense. That was a direction where I was going with proposing to drop everything else except mpv. Basically we could drop the hack that the player runs in background and receives keys and instead run persistent mpv process, control it using JSON IPC we are already using and handle all frontend in mps-youtube. This way we could truly manage playback state. This would simplify MPRIS (basically we would always have playback state available) and many other things with that. I like that this PR is a first step from What I think we need is to support people that are "misusing" mps-youtube. The guy that used mps-youtube with mpd or the guy that used mps-youtube as backend for webpage (or web interface to mps-youtube?). This is (or was) a great advantage of current design where you can just stick your own command to config (but this is mostly broken now). I think for this kind of extensibility something like our current NOTIFIER api is better. An API where you can stick your own command or script and it gives you url to play for each song (and does not require anything from you). This is what I have meant by bring-your-own-player architecture, actually. |
I agree that would make sense if mpsyt were aiming to be a normal media player. It would provide a very good integration of player with other parts of mpsyt. But we would lose the hackablity that is present in mpsyt now. With this PR, anyone could make a new player (without no or minimal change to critical parts of mpsyt) and use it for his need. He doesn't even have to play the song. He could take any url (youtube url or stream url) and pass it wherever he wants, exits his program whenever he wants, and mpsyt would take care of the rest. ( This guy needs yt url ). I really don't want to lose this feature for anything else (but that's just me).
We could still do that. I think to would be a good next step. We could make the media functions abstract methods of the class and listen for inputs after the process is opened. That way we could provide a general way of controlling all players through mpsyt. (If it cannot be controlled, we can't do anything).
I was thinking that the player subclasses would bring in more people to do stuff that we never imagined to do with mpsyt. I do agree that just giving the url to a script seems promising. But this would give a lot less control to the script user (like the current problem with catt integration).
What we could do is, officially only support mpv. Independent creators can make mpsyt add-on players which will be maintained by them. We could link the other players in the documentation. The mps-youtube organisation can maintain a new repo for some players that we would like to support. UPDATE:
|
I agree with you very much. Preserving hackability is important. I'm not sure subclass approach is a good way to go. If we go for background-running mpv we will introduce a lot of requirements on subclasses, which will be impossible or hard to implement with other players and almost certainly impossible to implement with stuff-that-we-never-imagined-to-do. I'm afraid this interface will become cumber-stone for both hackability (hard to implement requirements) and better mpv integration (being blocked by subclasses) at the end. I think we need to start thinking about clearly separating these two usages to have both good integration with mpv and generic extensibility interface. It would be great to have some feedback from people doing stuff-that-we-never-imagined-to-do with mpv to decide how this interface should look like. Should it be a hook that runs your script? Should it be a python class? Could they use MPRIS to source playback status? One way or another this should be separate from local playback scenario. |
I still don't see the problem going this way. That might be because I'm too immature to see the bigger picture. Anyway, I'll wait for @ids1024 and @Razesdark before closing this PR. |
|
Sure definitely don't close this. As always final word has @ids1024. My opinions are just opinions, not a decision. Others might have different opinions. Lot of this discussion is about a path forward and this PR is probably just the first step. It contains a lot of useful refactoring - thank you. But there are also parts that may complicate next steps, so we need to decide what we want to do in the future. |
|
It certainly would be nice if a class interface could abstract away the player being used while still allowing such sophisticated integration with mpv (and other players, if someone implements it). Which is hard, but worth trying. For instance, if mpsyt changes to calling mpv as a library (which might be better; I'm not sure), your design of the Figuring out how to handle back-grounding mpv (or other players) is also a concern. Can we assume that every player can be back-grounded? It should be possible at least with the pty hack I tried a while ago. For programs that don't test if stdin/etc. is a console, normal redirection would work. If we do not want all players to be back-grounded, the class could have a boolean |
I agree, what you're saying makes sense. I will make it more abstract.
I kind of did a dirty hack which enables background playback for mpv. It breaks some display stuff (easily fixable, I think) but it works. You can get to player controls by typing |
|
@ids1024 I have done something which would facilitate what you asked for. I did not create a new class because it would over-complicate some things handling the screen. What I did was, make some changes to the base I also made the arguments which were previously passed over through functions, entities of the class(maybe I overdid this?). This made functions cleaner and with the right documentation, I believe anyone can write a new player without extensive knowledge of mpsyt code base. Please suggest any changes if required. And if you can, please suggest a not-officially-supported player. I want to test out |
Vlc is probably the most commonly used, other than mpv/mplayer. Mpc has also been used (#168). If you have a chromecast (I don't), creating a class that uses catt would be a good thing to try (#607, skorokithakis/catt#30). |
I can't seem to get mpd to work on my system. (Weird!) Any suggestions on how I should take this PR forward? |
|
I still would like It is necessary to figure out how that will work with the current |
|
@ids1024 I made the changes as requested. See if this is enough. If it is, I can make a subclass for libraries too. If it is not, please suggest the required changes. And if you could make a new branch for refactoring player, we could merge into that (after this PR becomes merge-able, of-course) and do extensive testing.
I think the user shouldn't worry whether we are using a library or not. If we find the required library on the system, then we could use the library or else go the executable-player way. |
mps_youtube/util.py
Outdated
|
|
||
| def assign_player(player): | ||
| try: | ||
| module = import_module('.{0}'.format(player), 'mps_youtube.players') |
There was a problem hiding this comment.
This will fail to detect mpv if PLAYER is set to mpv.exe, mpv.com, or /path/to/mpv, right? These work now and should continue working.
I'm not sure how best to handle this... perhaps it's better to just require manually importing players in players/__init__.py. #316 could be revived; the actual plugin loading code there was working, there just weren't sane APIs that could be made public (the point of #423). This Player class API should be reasonable for use in plugins.
There was a problem hiding this comment.
These work now and should continue working.
Will be fixed in the next commit.
I'm not sure how best to handle this... perhaps it's better to just require manually importing
I really want dynamic importing to be a part of mpsyt. This way we could have an option --install so that anyone can write a mpsyt player class and could install it with mpsyt --install ~/newPlayer.py. This way, if we have more players in the future, their maintenance of them could be handled by their original authors.
| # | ||
|
|
||
|
|
||
| class generic_player(CmdPlayer): |
There was a problem hiding this comment.
Classes in Python conventionally have CamelCase names (see PEP 8). Is there any reason not to follow that here?
There was a problem hiding this comment.
Sorry, I'm quite new to open source and good coding practices.
mps_youtube/players/mpv.py
Outdated
| line = self._make_status_line(elapsed_s, prefix, songlength, | ||
| volume=volume_level) | ||
|
|
||
| if line != last_displayed_line: |
There was a problem hiding this comment.
It would be good if the _make_status_line(), if line != last_displayed_line, screen.writestatus() logic was all handled in self._make_status_line(). Or is there a better design that removes this redundancy?
There was a problem hiding this comment.
I warped the _make_status_line() in make _status_line(). Probably not the best fix, but it works.
mps_youtube/players/mplayer.py
Outdated
|
|
||
| args = config.PLAYERARGS.get.strip().split() | ||
|
|
||
| known_player = util.is_known_player(config.PLAYER.get) |
There was a problem hiding this comment.
We already know what the player is once this class is being run, right?
util.is_known_player() probably needs to be changed or removed with this refactor.
Fixed not loading of player when using path or .exe. Renamed generic_player to GenericPlayer Moved default args of mpv and mplayer to player class. Assign g.PLAYER_OBJ during set player command.
|
In the latest commit I have moved the functionality of the below functions to player class. If they are not needed anywhere else, they can be removed.
I added the requested changes too. |
|
@ids1024 Any comments or suggestions? |
|
I'm thinking about how to add basic plugin support (#316), perhaps pluggy would be a good choice? I'm not sure. But it makes sense to allow adding players as plugins.
Perhaps instead of that, I'll release a new version of pafy and mps-youtube (it should be about time for that, and the current |
The current implementation exposes (a primitive) plugin behavior. We could have a command like But having a uniform plugin interface sure sounds interesting. Looking forward to it.
<3. It's been long due. |
|
Sorry for the delay; I've been procrastinating mainly because of the time it takes to write a change-log. |
This is my take on refactoring player.py. This is obviously not complete. I implemented mpv player and wanted some feedback before going on. Any suggestions are appreciated. @ids1024 @Razesdark @hrnr @ritiek
There are some changes to be made in mpris.py to accommodate the change in player.py. It should be easy. Other than that no major changes won't be needed in files other than player.py(I think).