I stumbled upon a post on Reddit a few days ago and I thought I make my own! It's a program to extract from a movie file (.mkv, .mp4, etc.) and stack them horizontally next to each other and make one picture of all the frames available in the movie file. how can I make it more pythonic, readable, and sufficient?

Also, THIS is the GitHub link, pull requests are welcome.

Also, the default frame rate (the number of frames to extract one from, to stack up in the output image) is 5.

from natsort import natsorted
from ffprobe import FFProbe
from time import sleep
from tqdm import tqdm
from PIL import Image
import cv2
import os

# get and format file path
path = input('All modules loaded, press enter to proceed...\nEnter file path to the file(w/ filename, w/out the last backslash):\n')

if not os.path.exists(path):
    print("Path is incorrect, quitting")
if path[0] == "~":
    os.path.expanduser(path)  # replace ~ with literal home address
meta = FFProbe(path)
# get metadata
video_duration = meta.metadata["Duration"][:-3]
rate = meta.video[0].framerate
height = int(meta.video[0].width)
# one shot per 5 seconds
freq = int(rate) * 5

# info for the process bar
hours = int(video_duration[1]) * 3600
minutes = int(video_duration[3:5]) * 60
seconds = int(video_duration[-2:])
duration_seconds = hours + minutes + seconds
frames = rate * duration_seconds
output_images = frames // freq  # number of extracted images

(directory, video_name) = os.path.split(path)

dst_path = directory + "/images/"
os.system("mkdir " + dst_path)
os.system("mkdir " + dst_path + "resized")
print("Destination folder created.\n")

print("Extracting frames...")
cap = cv2.VideoCapture(path)
i = 1  # current captured frame, how many 24's has already been iterated
with tqdm(total=frames) as pbar:
    while cap.isOpened():
        frame_id = cap.get(1)  # first frame in the current stream
        ret, frame = cap.read()
        if ret != True:  # if run out of frames, video finished
        if frame_id % freq == 0:  # if frame# is a multiplicand of freq=24
            filename = dst_path + str(i) + ".jpg"
            i += 1
            cv2.imwrite(filename, frame)  # create the image
print(str(output_images) + " images extracted successfully.")
del pbar

src_path = directory + "/images/"
dst_path = src_path + "resized/"

images = natsorted(os.listdir(src_path))

i = 1
max_counter = len(images) - 1  # all images count
with tqdm(total=output_images) as pbar:
    for image in images:
        if i <= max_counter:  # process until all images are done
            read_path = src_path + image
            img = cv2.imread(read_path, cv2.IMREAD_UNCHANGED)
            w = 2  # convert to 2px
            h = img.shape[1]  # unchanged
            dim = (w, h)
            resized = cv2.resize(img, dim)
            filename = dst_path + str(i) + "_re.jpg"
            cv2.imwrite(filename, resized)  # create the image
            if i == 1:  # do only once
                print(str(img.shape) + " convert to " + str(dim))
            i += 1
            print("Finished " + str(len(images) - 1) + " pictures.")

images_list = natsorted(os.listdir(dst_path))

images_list = [Image.open(dst_path + x) for x in images_list]

# we have the height, user input #3
width = len(images_list) * 2  # number of small pictures, each 2px
out = Image.new("RGB", (width, height))  # create an empty image

x_offset = 0
for im in images_list:
    out.paste(im, (x_offset, 0))
    x_offset += im.size[0]

out.save(directory + "/" + video_name[:-4] + ".jpg")
print("Cleaning Up...")
os.system("rm -rf " + src_path)

print("All set, finishing now.")

1 Answer 1


Some suggestions aimed at more general issues rather than specific details in your lines of code:

  • Computers are not our friends, no need to chat. Get user input from the command line, not from input(). Interactive user input is a bad usage model for the default behavior of a script -- with rare exceptions. Use interactivity only for special circumstances, and always provide a command-line option to bypass that interactivity. In addition, input() is terrible for development because it requires you (the coder) to go into a dialogue with your script every damn time you run it. Super annoying. One option for handling command-line input is argparse. See the script below for the template that I usually follow in my scripts.

  • But functions are our friends. Organize your code into functions. Have each function do one thing. Among other things, this approach allows you to test each small piece of behavior as you write it.

  • Either do or talk. Do not print() inside your functions that do the actual work (aside from some quite rare exceptions). Functions should take arguments, calculate something or do something, and return some meaningful data (or None). If the larger program requires printing ultimately, use the data returned from functions to do that printing. But keep computation and printing in separate realms.

  • No magical tokens in the castle. Keep literal strings and values out of your code. Instead put them in one or more containers. In many situations, I usually just use a Constants class and module-level global (con), as shown below. Good practice, but easy and low-tech.

The script template:

import argparse
import sys

class Constants:

    EXIT_OK = 0
    EXIT_FAIL = 1

    NEWLINE = '\n'
    NAMES = 'names'
    BAR = 'BAR'
    BAR_PROPAGANDA = 'Sucker, should have picked --bar!'

            NAMES: 'path',
            NAMES: '-b --bar',
            'action': 'store_true',

con = Constants()

def main(args):
    ap, opts = parse_args(args)
    if opts.bar:
        print(con.BAR, opts)
        exit(con.EXIT_FAIL, con.BAR_PROPAGANDA)

def parse_args(args):
    ap = argparse.ArgumentParser()
    for oc in con.OPTS_CONFIG:
        kws = dict(oc)
        xs = kws.pop(con.NAMES).split()
        ap.add_argument(*xs, **kws)
    opts = ap.parse_args(args)
    return (ap, opts)

def exit(code = None, msg = None):
    code = con.EXIT_OK if code is None else code
    fh = sys.stderr if code else sys.stdout
    if msg:
        nl = con.NEWLINE
        msg = msg if msg.endswith(nl) else msg + nl

def your_function_foo(opts):

def your_function_bar(opts):

# Etc.

if __name__ == '__main__':
  • \$\begingroup\$ I always learn something here. Thank you for mentioning argparse. I am currently working on a project that needs this sorely. I was trying to roll my own solution. \$\endgroup\$
    – Preston
    Aug 24, 2020 at 6:54
  • 2
    \$\begingroup\$ why create an instance of Constants, when you can just reference the static members inside without one? \$\endgroup\$
    – lights0123
    Aug 24, 2020 at 16:50
  • 1
    \$\begingroup\$ @lights0123 Laziness and readability: con is easier to type, read, and visually scan. \$\endgroup\$
    – FMc
    Aug 24, 2020 at 17:15
  • \$\begingroup\$ 'Constans` class is very interesting, never seen that before. I usually just define constants after import like TOKEN = "asd". Is it a bad practice? \$\endgroup\$ Aug 25, 2020 at 6:46
  • \$\begingroup\$ @politicalscientist No, your approach is fine too. I slightly prefer bundling them under Constants for ease of importing. In larger projects, you can end up have a lot of constants, and it can become a drag to import each one you need. (And I dislike import *, for different reasons.) \$\endgroup\$
    – FMc
    Aug 25, 2020 at 14:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.