senko/python-video-converter

Windows compatible?

Opened this issue · 12 comments

I'm trying to use the module on Windows 7 64bit with python 2.7 and get this error when trying to invoke the convert method of the Converter class -

File "C:\python27\lib\subprocess.py", line 637, in init
raise ValueError("close_fds is not supported on Windows "
ValueError: close_fds is not supported on Windows platforms if you redirect stdi
n/stdout/stderr

Is there a work around for this or is the module simply incompatible with windows?

I haven't tried it on Windows, but it should be compatible in general.

The particular error you mention is due to the way I used subprocess.Popen here: https://github.com/senko/python-video-converter/blob/master/converter/ffmpeg.py#L263

If you can test whether it works on Windows if close_fds argument is removed, I'd gladly accept a pull request.

as I need it for a current project I am working on a windows compatible version supporting current ffmpeg build from http://ffmpeg.zeranoe.com/builds/# and as soon as its finished I will send you a pull request

Great, thanks!

hi guys,does it work on windows properly now ? i means i just download the latest version and test it.i am petty sure i was already install ffmpeg.but when i run it occur a error said not found ffmpeg.so.i dig into souces and change the detect function.as the below pictures.

qq20140113170632

but still raise ValueError("close_fds is not supported on Windows ")
any help will be thankful 😄

I have a patch to port to Windows. In addition to the above mentioned post, the SIGALRM is not supported on Windows (see doc for signal.alarm), so I have turned off timeout on Win.

Does anyone know how to turn off markdown while posting something? I am trying to share a patch file I generated. But I can't attach it (unsupported format), nor can I copy paste the contents, as github is messing up the formatting. I tried wrapping the text with html div tags, but that is removing the newline characters, making it worse. I'll be glad to email the patch to the author if you can give me the address.

OK I figured out how to post my patch without formatting issues.

Authors, could you please merge this for everyone's benefit?

At a later time, someone (or I, if I find the time) could code the support for timeout on Windows, perhaps using a thread that sleeps for the timeout period and calls the alarm function. For now, since it doesn't work anyway on Windows (throws exception), I've turned it off.

converter/ffmpeg.py | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/converter/ffmpeg.py b/converter/ffmpeg.py
index 6b14133..b40c2e1 100644
--- a/converter/ffmpeg.py
+++ b/converter/ffmpeg.py
@@ -7,11 +7,13 @@ import signal
 from subprocess import Popen, PIPE
 import logging
 import locale
+import platform

 logger = logging.getLogger(__name__)

 console_encoding = locale.getdefaultlocale()[1] or 'UTF-8'

+windows = platform.system() == 'Windows'

 class FFMpegError(Exception):
     pass
@@ -321,21 +323,21 @@ class FFMpeg(object):

         def which(name):
             path = os.environ.get('PATH', os.defpath)
-            for d in path.split(':'):
+            for d in path.split(os.pathsep):
                 fpath = os.path.join(d, name)
                 if os.path.exists(fpath) and os.access(fpath, os.X_OK):
                     return fpath
             return None

         if ffmpeg_path is None:
-            ffmpeg_path = 'ffmpeg'
+            ffmpeg_path = 'ffmpeg.exe' if windows else 'ffmpeg'

         if ffprobe_path is None:
-            ffprobe_path = 'ffprobe'
+            ffprobe_path = 'ffprobe.exe' if windows else 'ffprobe'

-        if '/' not in ffmpeg_path:
+        if os.path.sep not in ffmpeg_path:
             ffmpeg_path = which(ffmpeg_path) or ffmpeg_path
-        if '/' not in ffprobe_path:
+        if os.path.sep not in ffprobe_path:
             ffprobe_path = which(ffprobe_path) or ffprobe_path

         self.ffmpeg_path = ffmpeg_path
@@ -351,7 +353,7 @@ class FFMpeg(object):
     def _spawn(cmds):
         logger.debug('Spawning ffmpeg with command: ' + ' '.join(cmds))
         return Popen(cmds, shell=False, stdin=PIPE, stdout=PIPE, stderr=PIPE,
-                     close_fds=True)
+                     close_fds=(not windows))

     def probe(self, fname, posters_as_video=True):
         """
@@ -422,6 +424,9 @@ class FFMpeg(object):
         cmds.extend(opts)
         cmds.extend(['-y', outfile])

+        # Windows does not support SIGALRM - need another mechanism to timeout
+        if windows: timeout = None
+
         if timeout:
             def on_sigalrm(*_):
                 signal.signal(signal.SIGALRM, signal.SIG_DFL)

It's still not Windows compatible

So, someone posted this up onto the python package manager thing, and i just discovered, after doing most of this fixing legwork shown above in one evening myself, that, this repo is the original source which the author jamieoglindsey never pointed to but just pushed up into https://pypi.org/project/PythonVideoConverter/ without credit? Not sure, but it looks that way. So I'm tempted to make a patch, and try apply it to this repo and PR it here.

I have emailed the pypi thingy package owner, if they don't respond, will test and raise a PR in a few days. Not guaranteeing the ported changes i have is 100% handling all error cases, but happy to address any as they come.

senko commented

Hey @zaphodikus thanks for the note and apologies for the delayed response!

I've really dropped the ball on this project a few years ago and am not maintaining (or using) it currently. I would love if someone actively using it would take over the ownership. Looking at the GitLab repo, it seems like that fork had a lot of additional work done and if Jaime and team are interested in actively maintaining it, I would have no objection (other than asking to re-add original authors back to AUTHORS.txt alongside new contributors).

Ah, just noticed your response, the gitlab repo fork, where is that? Yes I'm still using it every so often. Will try reach out to Jamie again to be sure to credit people and work out how to merge any easier looking PRs waiting here.

I'll have to set up so I can run the tests on a quick unix-like box and add loads more test cases first, then yes I can help to maintain it @senko .