4
\$\begingroup\$

I have a video which I read in a loop frame by frame. For every frame, I want to do temporal kernel filtering, the coefficients of which come from the input variable model (a dictionary). The temporal kernel is an array that has size of 15. So, basically, for every frame we apply this function. The variables scale, clip_size are constants. Firstly I add borders across the obtained frame. Then I start a buffer of size (15,240,320) where (240,320) is the clip size. Variable bindex is used to write the buffer. The buffer variable will store only last 15 modified frames, hence bindex needs to be updated at every function iteration. Variable sindex is used to traverse through the buffer and apply the buffer, hence it has to be returned too. Since the input_frames are coming from a video in a loop, I had to make some variables be both inputs and outputs. Is there a way to speed this up? Any better way to do this?

def filtered_output(input_frame,model, Buffer,bindex,sindex,scale,clip_size):
    # padding is required
    top=model['item'][0]
    bot=model['item1'].shape[1]-top-1
    left=model['item'][1]
    right=model['item1'].shape[0]-left-1
    # NewDepth = 15
    # clip_size is [240,320]
    # In the external loop, bindex = 0, sindex =0 in the beginning
    # We now create a copy of our current frame, with appropriate padding
    frame2= cv2.copyMakeBorder(input_frame,top,bot,left,right,cv2.BORDER_CONSTANT,value=0.5)
    Buffer[bindex] = scipy.signal.convolve2d(frame2, model['item1'], boundary='symm', mode='valid')
    sindex = (bindex+1) % NewDepth # point to oldest image in the buffer
    temp=np.zeros(tuple(clip_size),dtype=float)
    temp = model['TempKern'][0]*Buffer[sindex]

    sindex = (sindex+1) % NewDepth
    for j in range(1, NewDepth) :  # iterate from oldest to newest frame
        temp = temp + model['TempKern'][j]*Buffer[sindex]
        sindex = (sindex+1) % NewDepth
    bindex = (bindex+1) % NewDepth
    temp=np.nan_to_num(temp)
    output_frame = ApplyCubicSpline(model['CubicSplineCoeffs'],temp)*scale
    return output_frame, Buffer,bindex,sindex

My question is whether we can make this function faster somehow (not that it is slow)? If yes, how? Is it the best practice to use bindex, sindex as both inputs and outputs to the function? Are there any alternatives?

\$\endgroup\$
  • \$\begingroup\$ @MathiasEttinger : I tried to explain my concerns. I am not concerned whether the output is correct or not. I am mostly concerned with the ways to make it run faster and whether there are better ways or not? \$\endgroup\$ – GKS Jan 13 '17 at 13:40
  • 1
    \$\begingroup\$ It's impossible for us to help you unless you give us much more information. The first step in speeding up this kind of code would be to run a profiler on it to see where the time is being spent, but we can't do that because we can't run your code and we don't have any data to run it on. \$\endgroup\$ – Gareth Rees Jan 13 '17 at 22:42
1
\$\begingroup\$

Maybe not the answer you're looking for, but I focused on that loop:

for j in range(1, NewDepth) :  # iterate from oldest to newest frame
    temp = temp + model['TempKern'][j]*Buffer[sindex]
    sindex = (sindex+1) % NewDepth

To speed this up you could:

  • create a variable to avoid accessing model['TempKern'] (dict access)
  • use in-place adition for temp

improved:

tk = model['TempKern']
for j in range(1, NewDepth) :  # iterate from oldest to newest frame
    temp += tk[j]*Buffer[sindex]
    sindex = (sindex+1) % NewDepth

Aside: outside the loop you do:

temp=np.zeros(tuple(clip_size),dtype=float)
temp = model['TempKern'][0]*Buffer[sindex]

so you can remove the first line, as it has no effect, temp is overwritten at the next line.

\$\endgroup\$
  • \$\begingroup\$ I had doubts about avoiding dictionary access. Thanks for the answer \$\endgroup\$ – GKS Jan 16 '17 at 4:40
1
\$\begingroup\$

Aside from stylistic issues (inconsistent use of whitespace, naming...), your biggest mistake is to not take advantage of numpy features whereas you seems to be using ndarrays.

But first some miscellaneous comments:

  • You can use unpacking to simplify computation of the padding. I also don't know if model['item'] and model['item1'] get updated but you may be able to cache the computation so you don't have to perform it at each frame.
  • You override temp right after it's creation; better remove the np.zeros line.
  • sindex is overriden before being used by sindex = (bindex+1) % NewDepth so there is no need of it as a parameter. Thus it should be unnecessary as a return value as well.
  • Instead of treating Buffer like a circular buffer where you manage an explicit index, it should be better to treat it like a FIFO queue and drop older result when adding a new one. The advantage being that you know in advance that the last entry will always be Buffer[-1]. This drops the need for bindex as a parameter and a return value.

Now let's use numpy to do that:

  • Dropping an old entry and appending a new one can be done using: np.vstack(Buffer[1:], whatever).
  • The for loop can be handled entirely in numpy: model['TempKern'] * Buffer can take care of all the multiplcations and then you just need to np.sum every rows. This assumes that model['TempKern'] is a numpy array.

Revised code could look like:

def filtered_output(input_frame, model,  buffer, scale, clip_size): 
    # padding is required 
    top, left = model['item'] 
    width, height = model['item1'].shape
    bottom = height - top - 1
    right = width - left - 1 
    # Create a copy of our current frame, with appropriate padding 
    frame = cv2.copyMakeBorder(input_frame, top, bottom, left, right, cv2.BORDER_CONSTANT, value=0.5) 
    convolved_frame = scipy.signal.convolve2d(frame, model['item1'], boundary='symm', mode='valid') 
    buffer = np.vstack((buffer[1:], convolved_frame))
    # Apply kernel
    temp = np.sum(model['TempKern'] * buffer, axis=0)
    temp = np.nan_to_num(temp) 
    output_frame = ApplyCubicSpline(model['CubicSplineCoeffs'], temp) * scale 
    return output_frame, buffer

But you should use better names as temp or item does not convey much meaning.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. It is indeed helpful. I wasn't aware of numpy vstack function. Will making buffer a global variable be helpful? Like I dont have to return buffer then, I believe. I would prefer that the function just return the output_frame, because I am planning to use moviepy's fl_image function. \$\endgroup\$ – GKS Jan 16 '17 at 4:48
  • \$\begingroup\$ @GKS I don't know, there is not enough context to answer. I'd say no nonetheless since global variable are often a code smell. Maybe a class? Maybe post a followup question with a bunch more code. \$\endgroup\$ – Mathias Ettinger Jan 16 '17 at 7:50
  • \$\begingroup\$ Here is the follow up question : - codereview.stackexchange.com/questions/152771/… \$\endgroup\$ – GKS Jan 16 '17 at 14:35

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.