# Converting an already-uploaded file and saving it to a model's FileField

from os import path, remove

try:
video = Video.objects.get(id=original_video_id)
except ObjectDoesNotExist:
return False

convert_command = ['ffmpeg', '-i', input_file, '-acodec',
'libmp3lame', '-y', '-ac', '2', '-ar',
'44100', '-aq', '5', '-qscale', '10',
'%s.flv' % output_file]

convert_system_call = subprocess.Popen(
convert_command,
stderr=subprocess.STDOUT,
stdout=subprocess.PIPE
)

try:

f = open('%s.flv' % output_file, 'r')
video.converted_video.save('%s.flv' % output_file,
filecontent,
save=True)
f.close()

remove('%s.flv' % output_file)

video.save()

return True
except:
return False


Clean, easy to understand code. However:

Never hide errors with a bare except. Change

try:
...
except:
return False


into

try:
...
except (IOError, AnyOther, ExceptionThat, YouExpect):
logging.exception("File conversion failed")


Also, unless you need to support Python 2.4 or earlier, you want to use the files context manager support:

with open('%s.flv' % output_file, 'r') as f:
video.converted_video.save('%s.flv' % output_file,
filecontent,
save=True)
remove('%s.flv' % output_file)


That way the file will be closed immediately after exiting the with-block, even if there is an error. For Python 2.5 you would have to from __future__ import with_statement as well.

You might also want to look at using a temporary file from tempfile for the output file.

• Thanks a lot for the review! I have updated the snippet to be pastebin.com/9fAepHaL But I am confused as to how to use tempfile. The file is already created by the "convert_system_call" before I use it's name in the try/catch block. Hmm,, maybe I use tempfile.NamedTemporaryFile()? – Chantz Feb 1 '11 at 15:15
• @Chantz: Yeah, exactly. If not you may end up with loads of temporary files in a current directory, where users won't find them and they ever get deleted. – Lennart Regebro Feb 1 '11 at 15:43
• Lennart following your & sepp2k's suggestion I modified my code to be like pastebin.com/6NpsbQhz Thanks. – Chantz Feb 2 '11 at 16:43

In addition to the points that Lennart made, there's just one minor thing I'd like to add:

You repeat the expression '%s.flv' % output_file four times in your code. You might want to store this in a variable like filename = '%s.flv' % output_file. This way there's less repetition and if you ever want to change the target file format, all you'll have to change are the filename and convert_command variables.

• Makes sense. Will make the change. Thanks! – Chantz Feb 1 '11 at 18:18
• sepp2k following your & Lennart's suggestion I modified my code to be like pastebin.com/6NpsbQhz Thanks. – Chantz Feb 2 '11 at 16:43