BradLarson / GPUImage

An open source iOS framework for GPU-based image and video processing
http://www.sunsetlakesoftware.com/2012/02/12/introducing-gpuimage-framework
BSD 3-Clause "New" or "Revised" License
20.25k stars 4.61k forks source link

runSynchronouslyOnVideoProcessingQueue does not reliably use correct thread #2303

Closed graph closed 8 years ago

graph commented 8 years ago

runSynchronouslyOnVideoProcessingQueue Doesn't reliably use correct thread. I'm dealing with multiple OpenGL contexts and noticed sometimes when I break and I'm inside runSynchronouslyOnVideoProcessingQueue it's on the main thread which is an issue because the code inside usually changes the contexts or does manipulations on the wrong context. The solution is simple replace with this function

void runSynchronouslyOnVideoProcessingQueue((void(^block)()) {

    dispatch_queue_t videoProcessingQueue = [GPUImageContext sharedContextQueue];
#if !OS_OBJECT_USE_OBJC
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
    if (dispatch_get_current_queue() == videoProcessingQueue)
#pragma clang diagnostic pop
#else
    if (dispatch_get_specific([GPUImageContext contextKey]))
#endif
    {
        block();
        return;
    }

    NSConditionLock *lock = [[NSConditionLock alloc] initWithCondition:NO];

    runAsynchronouslyOnVideoProcessingQueue(^{
        block();
        [lock lock];
        [lock unlockWithCondition:YES];
    });
    [lock lockWhenCondition:YES];
    [lock unlock];
}

Seeing how OpenGL is thread dependent, and dispatch queue is not, maybe these functions should explicitly spawn a new NSThread with a runloop inside so we know explicitly the same thread is always used.

BradLarson commented 8 years ago

Queues are not the same as threads. A single serial dispatch queue can use multiple different threads, including the main thread, depending on what the system determines to make sense.

The important thing here is to lock out the OpenGL context from simultaneous access. I don't care what thread is accessing the context as long as more than one thing can't access it at the same time. This is why I have to verify that the context is active on a given thread every time it's called into. If runSynchronouslyOnVideoProcessingQueue() didn't reliably lock out simultaneous accesses to the video processing context, GPUImage would crash all over the place.

Also, locks have significant overhead, which is why I use a lock-free approach. What you propose above would most likely have a large and negative impact on overall framework performance, given how often this is called.

graph commented 8 years ago

Ok I see. An issue related to this: Many of the classes when released change the context (e.g. GPUImageFramebuffer). I can put @autoreleasepool to help contain it. This makes working with multiple contexts & GPUImage too weird overall. EAGLContext has current context method. Maybe for all classes to revert the context back to what it was in dealloc methods. Should this related issue be reported as its own issue?

Thank you.

BradLarson commented 8 years ago

The framebuffers being released from the cache in an unsafe manner is a known issue. I haven't wrapped the appropriate parts of the cache in dispatch calls to the video processing context, so you can get the resources of the framebuffers being deallocated at the same time as rendering is going on in a different thread. I think I still need to fix the GPUImage 2 project to protect against this. I just keep forgetting to.

graph commented 8 years ago

Since you know I won't add another issue. I was debugging that so long lol. Things started making sense once I see that bug. Great work on GPUImage btw.

aforty commented 7 years ago

I think I'm running into problems relating to this. Any movement on this issue? Also would be nice if I could pass my own GPUImageContext to use in GPUImageView.

/edit I'm referring to your comment "The framebuffers being released from the cache in an unsafe manner is a known issue." @BradLarson

graph commented 7 years ago

If you need this for now try the locking/unlocking as I have explained above, there is a couple of functions that need to be modified all in that one file. The other solution is to intelligently place @autoreleasepool blocks. I went with the latter option. If I were to do it again I think I would make a patch file and apply the patch on every pod install just to have cleaner code. For my needs the unlocking/locking overhead would be negligible.

eliot1019 commented 7 years ago

@graph Could you let me know where you placed the autoreleasepool blocks in your solution?

graph commented 7 years ago

I very much don't remember it was long ago. From what I do remember, you must put autorelease blocks segmenting GPUImage and your additional context. That way you don't have anything being released in the middle of when you use your context. You need to be attentive to when variables are being released. It's hard to get it right.

It might be help to write a copyGlTexture() function that doesn't use GPUImage at all, that will duplicate a texture ID from GPUImage, that way you will have a texture that is not managed by GPUImage. The section where you need to pass a GPUImage texture to your additional GLContext is fairly tricky. There is no easy way just need to pay careful attention to object lives, and which is the current opengl context.

And don't forget about glFlush/glFinish(), in multithreaded and multiple contexts, between sharing boundaries you must call those explicitly.