[Pipeline] Fixed framebuffers not being properly locked (overrelease warnings), added remove(target:) support, fixed exc_bad_access crashes, PictureInput throw support, etc.

[joshbernfeld]

• Added remove(target:) support.

• Fixed transmitPreviousImage() being called on main thread via addTarget(). This would cause seemingly random exc_bad_access crashes from random opengl functions.

  Two tasks that access the same context may never execute simultaneously

  https://developer.apple.com/library/archive/documentation/3DDrawing/Conceptual/OpenGLES_ProgrammingGuide/ConcurrencyandOpenGLES/ConcurrencyandOpenGLES.html

  Calls to transmitPreviousImage() were being made from the main thread which was causing two threads to access the same context. This resolves random crashes that would occur when swapping out targets on an existing ImageOutput that has an existing rendered framebuffer. Calls to transmitPreviousImage() are now only done from the sharedImageProcessingContext. I should note that the best way to debug these exc_bad_access crashes is to check if there are any two threads accessing opengl functions at the same time.

• Fixed additional exc_bad_access crashes caused by Framebuffer glDelete functions being called from the wrong thread inside deinit() and ImageGenerator and TextureInput framebuffer creation called from the wrong thread inside init().

• Added lookup table for detailed framebuffer creation error messages.

• Fixed issue where targets were not getting properly locked. This was one source of the framebuffer overrelease warnings. See below comments inside updateTargetsWithFramebuffer() for an explanation of the issue.

public func updateTargetsWithFramebuffer(_ framebuffer:Framebuffer) {
    // This retrieves the count as it is currently
    // not factoring in any targets that are waiting in the target serial queue to be added
    // and may include targets which have since been released
    if targets.count == 0 {
        framebuffer.lock()
        framebuffer.unlock()
    } else {
        for _ in targets {
            framebuffer.lock()// This does not get called when it should get called
        }
    }
    // This jumps onto the target serial queue before iterating
    // So if there were any targets that had not yet been added they are added now and iterated through
    // But this is a problem because we didn't account for locking the frame buffer above for these new targets since the count did not reflect them
    // This is why when we retrieve the targets count, it should only be after any targets in the target serial queue are finished being added
    // Even if we did that, its still not comprehensive enough because there is still the risk that another target could be added
    // in between retrieving the count and iterating over them here.
    // So we need to make sure that all of the work we do here is done on the target serial queue in order to guarantee its done atomically.
    // That or we should guarantee only one operation is needed, which is what the new solution does.
    for (target, index) in targets {
        target.newFramebufferAvailable(framebuffer, fromSourceIndex:index)
    }
}

The solution was to retrieve a single list of targets that will not change during the execution of the function. Since we have to iterate through the targets in this solution before retrieving the count it also accounts for any targets which have since been released (which count did not do before). In addition, the count and target variables have been made private to prevent future errors.

public func updateTargetsWithFramebuffer(_ framebuffer:Framebuffer) {
    var foundTargets = [(ImageConsumer, UInt)]()
    for target in targets {
        foundTargets.append(target)
    }
        
    if foundTargets.count == 0 { // Deal with the case where no targets are attached by immediately returning framebuffer to cache
        framebuffer.lock()
        framebuffer.unlock()
    } else {
        // Lock first for each output, to guarantee proper ordering on multi-output operations
        for _ in foundTargets {
            framebuffer.lock()
        }
    }
    for (target, index) in foundTargets {
        target.newFramebufferAvailable(framebuffer, fromSourceIndex:index)
    }
}

Related issue: #84

For anyone looking to debug future framebuffer overelease warnings you can replace the lock() and unlock() functions with the following functions and then set a breakpoint on the warning and inspect the contents of the call stack history / retain count history variables in your debugger.

var callLockStackHistory = [[String]]()
var retainCountLockHistory = [Int]()
func lock() {
    framebufferRetainCount += 1
        
    retainCountLockHistory.append(framebufferRetainCount)
    callLockStackHistory.append(Thread.callStackSymbols)
}

func resetRetainCount() {
    framebufferRetainCount = 0
}
    
var callUnlockStackHistory = [[String]]()
var retainCountUnlockHistory = [Int]()
public func unlock() {
    framebufferRetainCount -= 1
        
    retainCountUnlockHistory.append(framebufferRetainCount)
    callUnlockStackHistory.append(Thread.callStackSymbols)
        
    if (framebufferRetainCount < 1) {
        if ((framebufferRetainCount < 0) && (cache != nil)) {
            print("WARNING: Tried to overrelease a framebuffer")
        }
        framebufferRetainCount = 0
        cache?.returnToCache(self)
    }
}

• Fixed BasicOperation renderFrameBuffer not being locked. This resolves issues where many still images would be generated with prior operations, or combinations of prior operations or black frames would be produced.

  When transmitPreviousImage() locks the renderFramebuffer, it becomes a candidate for the frame buffer cache in the future, which opens the possibility of it being written to when it is then unlocked and added to the cache. After it gets unlocked, the BasicOperation still has a reference to that renderFramebuffer and may then try to forward it again in the future. The BasicOperation should lock the framebuffer right after it is created, and unlock the framebuffer when the BasicOperation deinitializes since that framebuffer can be forwarded at any point in the BasicOperations lifetime.

  I was unable to lock the framebuffer after it was created without issues, so for now I have disabled transmitPreviousImage() on BasicOperation which is not critical functionality.

  If people need to change their pipeline on demand without changing the PictureInput, for now they should call processImage() again on the PictureInput to forward the framebuffer up their new pipeline.

  Related issue: Tried to overrelease a framebuffer #120

• Fixed PictureInput imageFramebuffer not being locked. This issue was identical to the above BasicOperation issue but in this case I was able to successfully lock the framebuffer for a permanent fix.

• Added throw support to PictureInput init() and handle occasional case for when image.dataProvider would come back nil causing a crash.

• Fixed PictureOutput warning CGImageCreate: invalid image alphaInfo: kCGImageAlphaNone. It should be kCGImageAlphaNoneSkipLast. CGImageAlphaInfo.none was being used instead of CGImageAlphaInfo.last

• Added open access level to BasicOperation to allow people to subclass it for their own operations.

• Added a warning on the FramebufferCache if it grows uncontrollably.

• Fixed RenderView background thread warning, fixed drawable properties (pr Fix EAGLContext drawableProperties dictionary in RenderView #125) and make the RenderView more forgiving if display buffer creation fails.

• Added support for RenderView resizing and added warning for when the view is too large.

[Jane930525]

@joshbernfeld Receiving this error when I try to initialize Camera and apply filter.This would eventually lead to crash at method 'clearFramebufferWithColor' in OpenGLRendering.swift. Please help.
`
Warning: tried to add target beyond target's input capacity --> Pipeline.swift: addTarget(:atTargetIndex:): 43
Warning: tried to add target beyond target's input capacity --> Pipeline.swift: addTarget(:atTargetIndex:): 43
Warning: tried to add target beyond target's input capacity --> Pipeline.swift: addTarget(_:atTargetInde

Thanks

[joshbernfeld]

@Jane930525 can you attach a sample project that reproduces the issue?