Python image combiner reading writing

Question

I am writing a simple script that will combine two images at a pixel level, selecting every other pixel from one image, and the other pixels from a second image.

from PIL import Image
import struct
print("Photo 1: ", end="")
photo1 = Image.open(input()) # First image, 1920 by 1080
photo1 = photo1.convert("RGB")
print("Photo 2: ", end="")
photo2 = Image.open(input()) # Second image, 1920 x 1080
photo2 = photo2.convert("RGB")
width = photo1.size[0] # define W and H
height = photo1.size[1]
print("Output file: ", end="")
output = input()
data = str.encode("")
for y in range(0, height): # counts to 1079
 row = ""
 for x in range(0, width):# counts to 1920
 if ((y + x) % 2) == 0: # makes every other pixel and alternates
 # on each line, making a checker board pattern
 RGB = photo1.getpixel((x, y))
 print("Even", y) # how far am I?
 else:
 RGB = photo2.getpixel((x, y))
 print("Odd", y) # how far aim I?
 R, G, B = RGB # now you can use the RGB value
 data += struct.pack("B", R) + struct.pack("B", G) + struct.pack("B",B)
 # store RGB values in byte format in variable "data"
new = Image.frombytes("RGB", (1920, 1080), data)# change value if not 1920 x 1080
new.save(output, "PNG") # make new image

Sorry if the copy/paste ing messes with python indentation. This script take a good 10 minutes to run on my laptop

• Intel Core i5
• 8 GB ram
• Running linux (Fedora 30) Is this just going to take 10 minutes to grab all the individual pixel values, or is there any thing that is making this code inefficient?

Also, I am using python3.7 and 3 at the moment, but I also can use 2.7 if needed.

These are the two photos I am using:

enter image description here enter image description here

• RomanPerekhrest: Can you add a thumbnails of input 2 images and the final resulting image into your question?
• Alex O'Connor: Here are the photos I am using. They are just free photos found online.

Answer 1

In addition to Reinderein's answer, there are still a lot of things that could be improved in your code.

Hardcoded values

You use hardcoded values, which make the code hard to use if you want to modify the inputs or the code itself. Consider the following line:

new = Image.frombytes("RGB", (1920, 1080), data)# change value if not 1920 x 1080

Why not just use the height and width variables that are already defined?

Data checks

Your procedure requires that the 2 input images are the same size. It would be good practice to check if this is the case before trying to combine them, and raise a relevant exception if they are not.

Encapsulation

It makes sense to encapsulate the image combining procedure in a function: pass that function 2 images, output the combined image. That way, the code is much more reusable and easy to test.

Adding a __main__ guard is a good idea for using the function.

Single responsibility

The image combining procedure should do just that. Your code also prints a bunch of stuff in the middle of doing that, and it really shouldn't.

Style

Your use of comments is inconsistent and all over the place. It makes the code hard to read.

Performance

There is a lot of room for improvements here, and the previous review barely tackles it. Using a single call to struct.pack() in the loop is probably better than 3, but the codes still takes minutes to run with 1920x1080 images. To be fair, I didn't have the patience to wait until it's done processing.

Given how slow the code is, I understand why you included print statements to your loop to monitor how far along the processing is, but consider this code:

for y in range(1080):
 for x in range(1920):
 if ((y + x) % 2) == 0: 
 print("Even", y)
 else:
 print("Odd", y)

This alone takes minutes to run. Comparing to this:

for y in range(1080):
 for x in range(1920):
 if ((y + x) % 2) == 0: 
 _ = "foo"
 else:
 _ = "bar"

This runs almost instantly. The lesson here is that print is slow, and should not be in an inner loop, even if you discard the single responsibility principle.

Another bottleneck is the following line in the inner loop:

data += struct.pack("B", R) + struct.pack("B", G) + struct.pack("B",B)

This appends more data to the data variable each time the line is run, which is a bit over 2 million times for your sample images. This means reallocating memory to fit the progressively larger variable a LOT of times, which is a slow process.

I have no experience with struct binary data and don't know how to preallocate the memory to fit the final data. Luckily for me, PIL has a Image.fromarray() method which take NumPy arrays as an input, which are easy to work with. That way, you can pre-allocate the required memory with something like data = np.zeros((height, width, 3), "uint8"), and modify the values as needed without reallocating memory.

Using this method, the required time to combine two 1920x1080 px images drops to a couple of seconds.

Putting it all together

Here is an example of what the code could look like after applying all these recommendations:

from PIL import Image
import numpy as np
def combine_images(image_1, image_2):
 """Combines two PIL images in a checkerboard pattern
 :param image_1: the first PIL image to combine
 :param image_2: the second PIL image to combine
 :return: A PIL Image"""
 image_1 = image_1.convert("RGB")
 image_2 = image_2.convert("RGB")
 if (image_1.size != image_2.size):
 raise ValueError("Images must be the same size")
 width, height = image_1.size
 data = np.zeros((height, width, 3), "uint8")
 for y in range(height): 
 for x in range(width):
 if ((y + x) % 2) == 0:
 data[y][x] = image_1.getpixel((x, y))
 else:
 data[y][x] = image_2.getpixel((x, y))
 return Image.fromarray(data)
if __name__ == "__main__":
 image_1 = Image.open(input("Path to image 1:\n"))
 image_2 = Image.open(input("Path to image 2:\n"))
 output = input("Path to output:\n")
 combine_images(image_1, image_2).save(output, "PNG")
 print("Done!")

Answer 2

Tuple unpacking

width = photo1.size[0] # define W and H
height = photo1.size[1]

can be

width, height = photo1.size

Use input properly

Don't print here:

print("Output file: ", end="")
output = input()

Instead,

output = input('Output file: ')

Range default start

Don't include the 0 in these calls:

for y in range(0, height): # counts to 1079
 for x in range(0, width):# counts to 1920

Only call pack once

 data += struct.pack("B", R) + struct.pack("B", G) + struct.pack("B",B)

should become, I think,

data += struct.pack('BBB', R, G, B)