I'm extracting a few frames from a video:

  • Comparing the similarity or equality of pixel color (from first two images)

  • Saving to a new image

  • Comparing the new image (conjunction of first two images) and the next image, etc.

Can you review my codes for efficiency and best coding practices?


import sys
import os
import numpy as np
from PIL import Image, ImageDraw

def main(obr1,obr2):
    img1= Image.open("%s" %(obr1))
    img2= Image.open("%s" %(obr2))
    im1 = img1.convert("RGBA")
    im2 = img2.convert("RGBA")
    pix1 = im1.load()
    pix2 = im2.load()
    im = Image.new("RGBA", (im1.width, im1.height), (0, 0, 0, 0))
    draw = ImageDraw.Draw(im)
    x = 0
    y = 0
    while y != im1.height-1 or x != im1.width-1:
        if pix1[x,y] == pix2[x,y]:
            p1 = np.array([(pix1[x,y][0]),(pix1[x,y][1]),(pix1[x,y][2])])
            p2 = np.array([(pix2[x,y][0]),(pix1[x,y][1]),(pix1[x,y][2])])
            squared_dist = np.sum(p1**2 + p2**2, axis=0)
            dist = np.sqrt(squared_dist)
            if dist < 200 and pix1[x,y] !=(0,0,0,0) and pix2[x,y] != (0,0,0,0):
                color = (round(pix1[x,y][0]+pix2[x,y][0]/2), round(pix1[x,y][1]+pix2[x,y][1]/2), round(pix1[x,y][2]+pix2[x,y][2]/2), round(pix1[x,y][3]+pix2[x,y][3]/2))
        if x == im1.width-1:
    im.save('test%s.png' %(z), 'PNG')
    print("Zapisano obraz test%s.png" %(z))

imglist = sys.argv[1:]
while imglist != []:
    exists = os.path.isfile("./test%s.png" % (z-1))
    if exists:
        obr1="test%s.png" % (z-1)
        print("Porównywanie obraza %s i %s" % (obr1,obr2))
        print("Analiza skończona")
        print("Porównywanie obraza %s i %s" % (obr1,obr2))
        print("Analiza skończona")
  • \$\begingroup\$ Are you working with Python 2 or Python 3? (Hint: You can also add this information as tag to your question). \$\endgroup\$ – AlexV Apr 15 '19 at 8:44
  • \$\begingroup\$ Are you sure that your code is working as intended? To me it seems like you're using the wrong image while creating p2. Probably copy and paste? \$\endgroup\$ – AlexV Apr 15 '19 at 10:31

Best practices

A collection of general best practices for Python code can be found in the infamous Style Guide for Python Code (also called PEP8). While your code looks quite reasonable, there a two major points from the Style Guide I would like to point out to you.
First, add documentation to your functions (and maybe choose a more descriptive name than main, more on that shortly). Future-you will be very grateful for this.
Second, always use a single whitespace before and after = when assigning to a variable (no whitespace if used as keyword arguments in a function ! Relevant section of PEP8 here). The code is also generally better to read if you add a trailing whitespace after , like so: main(obr1, obr2) instead of main(obr1,obr2).

Another thing that I would consider a Python best practice, is to wrap code that is to be executed in a "scripty" manner in a if __name__ == "__main__": clause (also see the official documentation on that topic). That would allow you reuse/import the function currently named main into other functions without running the while loop. Therefore, I would like to suggest the following coarse code-level structure:

# imports would go here ...

def compare_images(filename1, filename2):
    """Compare two images and store the comparison to file"""
    # function logic would go here

def main():
    """Process arguments from command line"""
    imglist = sys.argv[1:]
    z = 0
    while imglist != []:
        # ...

if __name__ == "__main__":

I would also recommend to give some of the variables a more descriptive name (what do obr1 and obr2 stand for?). Also keep in mind that most of the people reading your code (including me) do not speak your mother tongue, so it's always nice to translate console output to English before posting it here.


.load() should probably not be necessary as per the documentation (this assumes your actually using Pillow fork and not the old and crusty PIL).

The most striking point in terms of efficiency is that Python is often terribly slow at loops. So the easiest way to gain performance is to get rid of them. But how? NumPy to the rescue! NumPy does all those pesky loops in C and is therefore orders of magnitudes faster compared to looping over array data in Python "by hand".

So what you would generally do to benefit from this is to get your image data as NumPy array (see this SO answer for a hint) and then work on those NumPy arrays with array operations, like masking. I will try to convey what I mean by that in a short example, maybe I can fully adapt it to your example later.

im1_np = ... # get data as numpy array, see SO post
im2_np = ... # get data as numpy array, see SO post
result = np.zeros_like(im1_np)   # same dtype and shape as input
matching_pixels = im1_np == im2_np   # boolean mask with true where pixels match exactly
result[matching_pixels] = im1_np[matching_pixels]   # this is your if clause

As you can see, there a no "manual" loops involved, everything is done by NumPy in the background.

Now to the else path. First, I think there might be some errors here, feel free to comment if I'm wrong. What (I think) you basically want to do, is to compute the difference between corresponding pixels and set them to a certain color if they are below a given threshold. Mathematically this would be expressed similar to this:

$$ \sqrt{(r_1-r_2)^2 + (g_1-g_2)^2 + (b_1-b_2)^2} < 200 $$

Your code does the following at the moment:

$$ \sqrt{r_1^2 + r_2^2 + g_1^2+g_2^2 + b_1^2+b_2^2} < 200 $$

When working from my definition above, the code becomes as follows:

dist_mask = np.sum(im1_np-im2_np, axis=2) < threshold
# remove pixels already set in the if clause
dist_mask = np.logical_and(dist_mask, np.logical_not(matching_pixels))
# remove all-zero pixels
dist_mask = np.logical_and(dist_mask, np.sum(im1_np, axis=2) > 0)
dist_mask = np.logical_and(dist_mask, np.sum(im2_np, axis=2) > 0)
# set color in result image as mean of both source pixels
result[dist_mask] = (im1_np[dist_mask]+im2_np[dist_mask]) / 2.

I leave threshold as variable since I'm not sure your original computation works the way you expect it and the threshold as chosen by you is meaningful. (Note: You can simply leave out the sqrt if you square the threshold value). Apart from that, the code is a relatively strict transformation of your original conditions, it's just that instead of looping over the images pixel by pixel, everything is done in array operations.

Under the assumption that you actually want to assign the average pixel value of both source images, this can be optimized further, since the if condition of exact pixel equality is a subset of distance < threshold. This would save you a mask computation (matching_pixels would not be needed anymore) and the negation/and operation with the dist_mask. In case of exact equality, summing both values and dividing them by two should leave you with the original value (Warning: Watch out for quirks with floating point values and/or range-limited integer values).

To be fully compatible with your original code you would then have to go back to PIL to store the image to disk. This should also be described in the SO post linked above.

Other things

You are sometimes using string formatting in a weird way. If you just want make sure that a variable is a string, pass it to str(...) instead of using string formatting. If you really need string formatting such as where you create the output filename, it is often recommended to use .format(...) (Python 2, Python 3) or f-strings (Python 3) to format string output. There is a nice blog post here that compares all ways of doing string formatting in Python I mentioned.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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