beetbox / audioread

cross-library (GStreamer + Core Audio + MAD + FFmpeg) audio decoding for Python
MIT License
483 stars 108 forks source link

Don't open a window when supported #89

Closed ghost closed 5 years ago

ghost commented 5 years ago

The console window which opens every time when accessing ffmpeg sucks. This PR will stop this behaviour when it's supported by the Python version (introduced in 3.7)

Successor of #88.

sampsyo commented 5 years ago

Cool; thanks! It looks like the existence test is necessary even on 3.7, because the constant is only available on Windows. In fact, it looks like the common thing to do on GitHub is to hard-code the value of this constant: https://github.com/search?q=subprocess.CREATE_NO_WINDOW&type=Code

Perhaps we should just do that?

Also, instead of modifying the popen_multiple function to do this dynamically, which has the unfortunate consequence of preventing over creationflags from being added in the future, what do you think about doing this at the top level (module scope)?

if hasattr(subprocess, 'CREATE_NO_WINDOW'):
    PROC_FLAGS = subprocess.CREATE_NO_WINDOW
else:
    PROC_FLAGS = 0

Then, pass creationflags=PROC_FLAGS in both calls to popen_multiple.

ghost commented 5 years ago

Assuming we'd hardcode it, wouldn't we have to check to see if we're on Windows? (Look in the subprocess code l45, l185) And we can remove the version condition for that, right?

if sys.platform == "win32":
    PROC_FLAGS = 0x08000000
else:
    PROC_FLAGS = 0
sampsyo commented 5 years ago

Yeah, that seems about right!

sampsyo commented 5 years ago

Looks perfect; thank you!! :sparkles: :rocket: