willprice / python-omxplayer-wrapper

:tv: Control OMXPlayer, the Raspberry Pi media player, from Python
http://python-omxplayer-wrapper.readthedocs.io
GNU Lesser General Public License v3.0
253 stars 71 forks source link

pypi package 0.2.3 breaks API and is different from branch master 0.2.3 (latest) #105

Closed quantumdot closed 6 years ago

quantumdot commented 6 years ago

Issue Report

Description

Installing via pip install omxplayer-wrapper results in the installed omxplayer-wrapper module missing several methods listed in the API documentation and the latest source on branch main (listed at version 0.2.3 via init.py).

Problem reproduction

pip install omxplayer-wrapper
python
>>> from omxplayer.player import OMXPlayer
>>> dir(OMXPlayer)
['__class__', '__delattr__', '__dict__', '__doc__', '__format__', '__getattribute__', '__hash__', '__init__', '__module__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_check_player_is_active', '_duration_us', '_get_player_interface', '_get_properties_interface', '_get_root_interface', '_load_source', '_run_omxplayer', '_setup_dbus_connection', '_setup_omxplayer_process', 'action', 'can_control', 'can_go_next', 'can_go_previous', 'can_pause', 'can_play', 'can_quit', 'can_seek', 'can_set_fullscreen', 'duration', 'get_filename', 'get_source', 'identity', 'is_playing', 'list_audio', 'list_subtitles', 'list_video', 'load', 'maximum_rate', 'minimum_rate', 'mute', 'pause', 'play', 'play_pause', 'play_sync', 'playback_status', 'position', 'quit', 'seek', 'set_alpha', 'set_aspect_mode', 'set_position', 'set_video_crop', 'set_video_pos', 'set_volume', 'stop', 'unmute', 'volume']

Output should match the following:

pip install git+https://github.com/willprice/python-omxplayer-wrapper.git
python
>>> from omxplayer.player import OMXPlayer
>>> dir(OMXPlayer)
['__class__', '__delattr__', '__dict__', '__doc__', '__format__', '__getattribute__', '__hash__', '__init__', '__module__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_duration_us', '_interface_property', '_load_source', '_player_interface', '_player_interface_property', '_position_us', '_properties_interface', '_root_interface', '_root_interface_property', '_run_omxplayer', '_setup_dbus_connection', '_setup_omxplayer_process', 'action', 'aspect_ratio', 'can_control', 'can_go_next', 'can_go_previous', 'can_pause', 'can_play', 'can_quit', 'can_raise', 'can_seek', 'can_set_fullscreen', 'duration', 'fullscreen', 'get_filename', 'get_source', 'has_track_list', 'height', 'hide_subtitles', 'hide_video', 'identity', 'is_playing', 'list_audio', 'list_subtitles', 'list_video', 'load', 'maximum_rate', 'metadata', 'minimum_rate', 'mute', 'next', 'pause', 'play', 'play_pause', 'play_sync', 'playback_status', 'position', 'previous', 'quit', 'rate', 'seek', 'select_audio', 'select_subtitle', 'set_alpha', 'set_aspect_mode', 'set_position', 'set_rate', 'set_video_crop', 'set_video_pos', 'set_volume', 'show_subtitles', 'show_video', 'stop', 'supported_uri_schemes', 'unmute', 'video_pos', 'video_stream_count', 'volume', 'width']

Comparison of OMXPlayer methods:

Method In pypi In git
__class__ TRUE TRUE
__delattr__ TRUE TRUE
__dict__ TRUE TRUE
__doc__ TRUE TRUE
__format__ TRUE TRUE
__getattribute__ TRUE TRUE
__hash__ TRUE TRUE
__init__ TRUE TRUE
__module__ TRUE TRUE
__new__ TRUE TRUE
__reduce__ TRUE TRUE
__reduce_ex__ TRUE TRUE
__repr__ TRUE TRUE
__setattr__ TRUE TRUE
__sizeof__ TRUE TRUE
__str__ TRUE TRUE
__subclasshook__ TRUE TRUE
__weakref__ TRUE TRUE
_duration_us TRUE TRUE
_load_source TRUE TRUE
_run_omxplayer TRUE TRUE
_setup_dbus_connection TRUE TRUE
_setup_omxplayer_process TRUE TRUE
action TRUE TRUE
can_control TRUE TRUE
can_go_next TRUE TRUE
can_go_previous TRUE TRUE
can_pause TRUE TRUE
can_play TRUE TRUE
can_quit TRUE TRUE
can_seek TRUE TRUE
can_set_fullscreen TRUE TRUE
duration TRUE TRUE
get_filename TRUE TRUE
get_source TRUE TRUE
identity TRUE TRUE
is_playing TRUE TRUE
list_audio TRUE TRUE
list_subtitles TRUE TRUE
list_video TRUE TRUE
load TRUE TRUE
maximum_rate TRUE TRUE
minimum_rate TRUE TRUE
mute TRUE TRUE
pause TRUE TRUE
play TRUE TRUE
play_pause TRUE TRUE
play_sync TRUE TRUE
playback_status TRUE TRUE
position TRUE TRUE
quit TRUE TRUE
seek TRUE TRUE
set_alpha TRUE TRUE
set_aspect_mode TRUE TRUE
set_position TRUE TRUE
set_video_crop TRUE TRUE
set_video_pos TRUE TRUE
set_volume TRUE TRUE
stop TRUE TRUE
unmute TRUE TRUE
volume TRUE TRUE
_check_player_is_active TRUE FALSE
_get_player_interface TRUE FALSE
_get_properties_interface TRUE FALSE
_get_root_interface TRUE FALSE
_interface_property FALSE TRUE
_player_interface FALSE TRUE
_player_interface_property FALSE TRUE
_position_us FALSE TRUE
_properties_interface FALSE TRUE
_root_interface FALSE TRUE
_root_interface_property FALSE TRUE
aspect_ratio FALSE TRUE
can_raise FALSE TRUE
fullscreen FALSE TRUE
has_track_list FALSE TRUE
height FALSE TRUE
hide_subtitles FALSE TRUE
hide_video FALSE TRUE
metadata FALSE TRUE
next FALSE TRUE
previous FALSE TRUE
rate FALSE TRUE
select_audio FALSE TRUE
select_subtitle FALSE TRUE
set_rate FALSE TRUE
show_subtitles FALSE TRUE
show_video FALSE TRUE
supported_uri_schemes FALSE TRUE
video_pos FALSE TRUE
video_stream_count FALSE TRUE
width FALSE TRUE

Environment details

Software Version
python-omxplayer-wrapper 0.2.3
python-dbus (dpkg -s python-dbus) 1.2.4-1
python (python --version) 2.7.13
omxplayer (omxplayer --version) 5a25a57
willprice commented 6 years ago

I guess I should bump the version number post release rather than pre release so the version number of the docs isn't misleading.

Thanks for flagging this up.

I'll release a new version imminently.

quantumdot commented 6 years ago

Thanks for your quick reply, and appreciate the great work you're doing. The project I'm using this for has a deadline of roughly early next week. Do you foresee the mater branch being pushed to pypi in that time? How stable would you say is the master branch?

willprice commented 6 years ago

Hey,

I can probably release tomorrow.

Alternatively you can install via pip from github

The library isn't that stable sadly as there a bunch of hard problems I didn't solve when I first tackled this and built the library, its been in maintenance mode ever since.

willprice commented 6 years ago

You said I've broken the API compatibility, how? I've only changed internal methods/properties and added new methods, so the new API is strictly a superset of the old one; hence the new API is API-compatible with the old one; am I missing a change I've made to the public API?

quantumdot commented 6 years ago

Sorry if that is how you interpreted my issue report. You're correct that the changes in master should not break backwards compatibility. I was coding against the API described in the documentation for 0.2.3, which includes the new methods, and both pypi and branch master list the version as 0.2.3. So when testing my code I was getting errors that the methods we're missing until I dug through the source code of the downloaded pypi package.

On another note, what are the hard problems you need to solve for the project?

willprice commented 6 years ago

Ah OK, that makes sense.

The main difficulty is around managing the lifecycle of the omxplayer process and alerting the user's code to process exits. What should the methods do when the process has exited and we can no longer talk to the process? Should we query all the properties that are constant over the duration of the video initially and return those cached values even when the player is dead? etc etc

quantumdot commented 6 years ago

I have had some experience working with COM objects on widows, which is somewhat similar to communicating on dbus (IPC). My general feeling is that caching values is typically not worth the complexity added to the code in keeping everything in sync. As far as alerting the user to the omxplayer process exit, why don't you expose an event for this, as you are already exposing events for other things such (i.e. stop, play, etc) and you are already polling the omxplayer process. As for how the methods should behave on player death, if they cannot perform their intended functionality they must raise an exception that the player is dead. It should probably be the user's responsibility how to handle death of the player process. Just my two cents.

willprice commented 6 years ago

I think you're right, caching isn't worth the effort. I think the event for player death is sensible, however it is likely the callback will have to be run in a separate thread as the monitor is run in a separate thread.

Thanks for your suggestions.

quantumdot commented 6 years ago

Oh, I see now that the Event() object that you're using is from the evento module, which is more of a Observer pattern type of event (which I am assuming is not thread-safe). Why not use the threading.Event object or some other synchronization (i.e. Queue) to communicate the player process state from the thread running the player process to the main controller thread? I found a nice example here for using a Queue to run callbacks on the main thread.

willprice commented 6 years ago

Currently I don't require any sort of main loop which is what I'd need to implement to use the Queue example you reference, I guess I could fork evento and make it thread safe; that'd not be too much work. The threading.Event solution looks nicer and less of a drastic change to how the library currently works. Cheers