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

ffmpeg.py does not check for unicode strings #241

Closed
flokadillo opened this issue Dec 22, 2016 · 3 comments · Fixed by #305
Closed

ffmpeg.py does not check for unicode strings #241

flokadillo opened this issue Dec 22, 2016 · 3 comments · Fixed by #305

Comments

@flokadillo
Copy link
Contributor

flokadillo commented Dec 22, 2016

In madmom.audio.ffmpeg.py, there are several lines where isinstance(infile, str): is used. We should replace this by isinstance(infile, (str, unicode)): to also support python 2.7's unicode string type.

@superbock
Copy link
Collaborator

superbock commented Dec 22, 2016

Actually, it is only a single line (https://github.com/CPJKU/madmom/blob/master/madmom/audio/ffmpeg.py#L170), but using the above suggestion breaks Python 3, since it does not have unicode type any more.

Can you provide an example (or even better: a test) which shows the problem? I fixed something related quite recently #236. Seems like this fix was not enough.

EDIT: just searched for exact matches, missed many other instances.

@flokadillo
Copy link
Contributor Author

Actually, this happens also in line 184, 243, and 292.
Here is a small example:
from madmom.audio.ffmpeg import decode_to_memory
infile = u'C:\\Users\\krf\\dev\\python\\madmom\\tests\\data\\audio\\sample.wav'
s=decode_to_memory(infile)
This produces the following error:
ValueError: only file names or Signal instances are supported as infile, not C:\Users\krf\dev\python\madmom\tests\data\audio\sample.wav.
Maybe we have to add an if clause to check types for different versions of Python...

@superbock
Copy link
Collaborator

@flokadillo, could you try the fix provided in #305 and see if it fixes your issue?

superbock pushed a commit that referenced this issue Aug 6, 2017
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 a pull request may close this issue.

2 participants