Recording timeback seconds of video based on compared pixels from an image

Question

The code reads from the screen, compares a pixel in a previously provided sample image and, if the pixels match, it records timeback seconds of video leading up to it. The output is put into a folder 'tm'. Since I do not assume the pixel to be occurring all the time, the writing process is buffered with a thread, so as not to slow down the capturing process. The pictures in the folder 'tm' can in the end be combined to make up a shortened movie, containing all the frames with the pixel in place.

I don't even know where to start optimizing. What should I improve in terms of readability and speed?

import PIL.ImageGrab
import PIL.Image
import time
from collections import deque
import threading 
from numpy import *

def get_pixel_colour(i_x, i_y):
    return PIL.ImageGrab.grab().load()[i_x, i_y]

class framebuffer:
    def __init__(self,size):
        self.buffer = deque(maxlen=size)
        self.i= 0
    def add_frame(self, frame):
        self.buffer.append(frame)
    def remove_frame(self):
        return self.buffer.popleft()
    def write_frame(self):
        x = self.buffer.popleft()
        self.i+=1
        x.save("tm\screengrab"+ str(self.i).zfill(10) +".jpg")
        print "tm\screengrab"+ str(self.i).zfill(10) +".jpg written, ", len(self.buffer), "to go " 
    def is_empfty(self):
        return (len(self.buffer)>0)

class mythread(threading.Thread):
    def run (self):
        while(True):
            if (len(self.frameb.buffer) > 0):
                self.frameb.write_frame()
            else:
                time.sleep(1)
    def setbuffer(self,frameb):
            self.frameb = frameb


seedimage = PIL.Image.open("seed.bmp") 
pos_x = 983
pos_y = 263
recpixel = seedimage.load()[pos_x, pos_y]
print "The pixel is now", recpixel
fps = 50
skipticks = 1/(fps*1.0)
i= 0
timeback = fps * 3
allframes=framebuffer(timeback)
nextsnap=time.clock()
recordframes = framebuffer(None)
t= mythread()
t.setbuffer(recordframes)


print 'Starting thread'
t.setDaemon(True)
t.start()
print 'starting program'
print skipticks, fps
while (True):
    tim= time.clock()
    i=i+1
    x = PIL.ImageGrab.grab()  
    allframes.add_frame(x)
    imp_pixel = x.load()[pos_x, pos_y]
    r, i = array(recpixel),array(imp_pixel)
    if (sum(r-i)<1 and sum(r-i)>-1 ):
        print "recognized", sum(r-i),"with size:", len(allframes.buffer)
        recordframes.buffer.extend(allframes.buffer)
        allframes.buffer.clear()
        sleep(10)

    # this is the sleep that limits the fps
    nextsnap+=skipticks
    sleeptime = nextsnap-time.clock()
    if (sleeptime>0):
        time.sleep (sleeptime)

Answer 1

import PIL.ImageGrab
import PIL.Image
import time
from collections import deque
import threading 
from numpy import *

Generally, import * is frowned upon because it makes it hard to figure out where things come frome

def get_pixel_colour(i_x, i_y):
    return PIL.ImageGrab.grab().load()[i_x, i_y]

You don't appear to actually use this function

class framebuffer:

Python style guide recommends CamelCase for class names

    def __init__(self,size):
        self.buffer = deque(maxlen=size)
        self.i= 0

i is kinda hard to guess what its for.

    def add_frame(self, frame):
        self.buffer.append(frame)
    def remove_frame(self):
        return self.buffer.popleft()

You don't ever use this method

    def write_frame(self):
        x = self.buffer.popleft()
        self.i+=1
        x.save("tm\screengrab"+ str(self.i).zfill(10) +".jpg")
        print "tm\screengrab"+ str(self.i).zfill(10) +".jpg written, ", len(self.buffer), "to go " 
    def is_empfty(self):

Misspelled name and never used

        return (len(self.buffer)>0)

You don't need the parens around the whole expression

class mythread(threading.Thread):
    def run (self):
        while(True):

No need for those parens. Let your tokens breath

            if (len(self.frameb.buffer) > 0):

Its odd that you define a is_empfty function but don't call it.

                self.frameb.write_frame()
            else:
                time.sleep(1)

Python has a Queue.Queue class which implements a thread-safe queue. That'll be a better idea then a list because its guaranteed to be thread safe. Also, if you try to pop from an empty queue it will wait until an item is pushed onto the queue. That way you don't need to do the time.sleep() and loop thing here

    def setbuffer(self,frameb):

The python style guide recommends names like set_buffer

            self.frameb = frameb

I'd also implement something like this by passing the frame buffer to the constructor. I'd also probably combine these two classes. They are tightly coupled anyways.

seedimage = PIL.Image.open("seed.bmp") 
pos_x = 983
pos_y = 263

Instead, how about storing the position as a tuple? You can pass a tuple to index the arrays

recpixel = seedimage.load()[pos_x, pos_y]
print "The pixel is now", recpixel
fps = 50
skipticks = 1/(fps*1.0)
i= 0

Any sort of complex logic like this should really be in a function. It'll be somewhat faster that way.

timeback = fps * 3
allframes=framebuffer(timeback)
nextsnap=time.clock()
recordframes = framebuffer(None)
t= mythread()
t.setbuffer(recordframes)


print 'Starting thread'
t.setDaemon(True)
t.start()

I'd do at least the setDaemon instead the thread. (And are you sure you want a Deamon thread?)

print 'starting program'
print skipticks, fps
while (True):
    tim= time.clock()
    i=i+1

use for i in itertools.count(): instead the while loop, and you want have to manually track the i

    x = PIL.ImageGrab.grab()  
    allframes.add_frame(x)

avoid names like x. Makes it hard to follow your code

    imp_pixel = x.load()[pos_x, pos_y]
    r, i = array(recpixel),array(imp_pixel)

This override your previous i, suggesting it wasn't really important enough to keep track of anyways

    if (sum(r-i)<1 and sum(r-i)>-1 ):

python has a syntax -1 < sum(r-1) < 1 which is nicer for these sorts of things

        print "recognized", sum(r-i),"with size:", len(allframes.buffer)
        recordframes.buffer.extend(allframes.buffer)

Why don't you make allframes a list? It doesn't use any of the other behavior of the class you made.

        allframes.buffer.clear()
        sleep(10)

I don't know why you are sleeping here

    # this is the sleep that limits the fps
    nextsnap+=skipticks
    sleeptime = nextsnap-time.clock()
    if (sleeptime>0):
        time.sleep (sleeptime)    

Note that time.sleep isn't very accurate as timing goes.