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

Check if filename is a URL before checking if file exists #47

Closed tiangolo closed 8 years ago

tiangolo commented 8 years ago

Check if filename is a URL before checking if file exists.

It does so by parsing the filename and checking if it's a URL with a scheme (the "http" or "rstp" in "http://youtube.com/..." or in "rstp://192.168.1.15:5544/") before parsing if a file exists. So, if the path is a file, it will still do the check.

This should solve #35.

I needed it so I added it (and I tested it, I'm already using this fix).

willprice commented 8 years ago

I'm rejecting this on the grounds that you can just open the URI as a stream using url:

urllib.urlretrieve(file_name, 'file.mp4')
player = OMXPlayer('file.mp4')
player.play()
tiangolo commented 8 years ago

That would then disallow playing a live stream. It would have to be downloaded first to be played.

BTW, I (we at Senseta) are using the modified version in part of a complete advanced video wall solution that we are going to release as open source. And we need to be able to stream live content (as the screen from a desktop, for example with OBS, VLC). So it would be great to have this update in the official (your) version and in PyPi.

willprice commented 8 years ago

If you're happy to update your pull request to bring it up to date with the latest changes then I'll review and merge it in.

tiangolo commented 8 years ago

Cool! I'll do it.

Should I do a rebase of my changes on top of the current develop? Should I merge the current develop in my branch? Or should I create a new PR with these same changes?

(Just to know how do you want to keep the "git history").

willprice commented 8 years ago

Hi Sebastian,

A rebase would be preferred, thanks for offering to bring this up to date :+1:

codecov-io commented 8 years ago

Current coverage is 84.15% (diff: 89.47%)

Merging #47 into develop will increase coverage by 0.24%

@@            develop        #47   diff @@
==========================================
  Files             3          3          
  Lines           261        265     +4   
  Methods           0          0          
  Messages          0          0          
  Branches         14         14          
==========================================
+ Hits            219        223     +4   
  Misses           38         38          
  Partials          4          4          

Powered by Codecov. Last update 6a8392c...f206f8b

tiangolo commented 8 years ago

Done! :tada:

I don't know what should I do with the codecov/patch "failing" test... (or if I should do anything).

willprice commented 8 years ago

Can we get a test to check if we check for the file to exist when we use a normal file path, and that we don't check when we're using a URI?

I'm also unsure about leaving the filename as its currently named, as now we're more generic, perhaps we should rename to uri, but that's not great either since it can either be a file path, or a URI. uri_or_path is fairly awful too. Any ideas?

tiangolo commented 8 years ago

OK, I agree. I'll work on it this week (right now I have to go).

tiangolo commented 8 years ago

I updated the repo tests.

Nevertheless, I would like you to check the changes and see if they look good to you, as I'm not an expert with those specific testing tools.


What do you think of using source instead of filename?

If you approve (or chose a different variable name), I'll update all the references to use that.

willprice commented 8 years ago

Hi @tiangolo,

Thanks for you contributions.

source is an excellent idea! We need to make it very clear in the entry point that for most people this will be a video (on that note, I really need to fix up the RTD docs to be more friendly to newcomers).

It's quite late here, but I'll try and review your changes in detail tomorrow.

Thanks

tiangolo commented 8 years ago

I just refactored player.py to use source instead of file_name or file_path.

I added methods for player.get_source but left the player.get_filename there too for backward compatibility. Also the same with player._load_source and player._load_file.

I also updated a bit the docstrings to make it more clear that a "source" can be a file path or a URL.

Also, I updated the README.md with these live streaming options. And I added a file_path_or_url variable to both examples to make it more clear that it works with a file_path or a URL and that the syntax and usage is the same.