From 738df8913db644d33c717b11b9155a2e10e3c6cf Mon Sep 17 00:00:00 2001 From: "tkchin@webrtc.org" Date: Wed, 4 Jun 2014 20:19:39 +0000 Subject: [PATCH] Fix retain cycle in RTCEAGLVideoView. CADisplayLink increases its target's refcount. In order to break retain cycle, we wrap CADisplayLink in a new RTCDisplayLinkTimer class and use that instead. R=fischman@webrtc.org, noahric@chromium.org BUG=3391 Review URL: https://webrtc-codereview.appspot.com/16599006 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6331 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/objc/RTCEAGLVideoView.m | 105 +++++++++++++----- .../objc/AppRTCDemo/APPRTCConnectionManager.m | 1 + 2 files changed, 81 insertions(+), 25 deletions(-) diff --git a/talk/app/webrtc/objc/RTCEAGLVideoView.m b/talk/app/webrtc/objc/RTCEAGLVideoView.m index a9198614d..5365d9821 100644 --- a/talk/app/webrtc/objc/RTCEAGLVideoView.m +++ b/talk/app/webrtc/objc/RTCEAGLVideoView.m @@ -37,14 +37,72 @@ #import "RTCVideoRenderer.h" #import "RTCVideoTrack.h" +// RTCDisplayLinkTimer wraps a CADisplayLink and is set to fire every two screen +// refreshes, which should be 30fps. We wrap the display link in order to avoid +// a retain cycle since CADisplayLink takes a strong reference onto its target. +// The timer is paused by default. +@interface RTCDisplayLinkTimer : NSObject + +@property(nonatomic) BOOL isPaused; + +- (instancetype)initWithTimerHandler:(void (^)(void))timerHandler; +- (void)invalidate; + +@end + +@implementation RTCDisplayLinkTimer { + CADisplayLink* _displayLink; + void (^_timerHandler)(void); +} + +- (instancetype)initWithTimerHandler:(void (^)(void))timerHandler { + NSParameterAssert(timerHandler); + if (self = [super init]) { + _timerHandler = timerHandler; + _displayLink = + [CADisplayLink displayLinkWithTarget:self + selector:@selector(displayLinkDidFire:)]; + _displayLink.paused = YES; + // Set to half of screen refresh, which should be 30fps. + [_displayLink setFrameInterval:2]; + [_displayLink addToRunLoop:[NSRunLoop currentRunLoop] + forMode:NSRunLoopCommonModes]; + } + return self; +} + +- (void)dealloc { + [self invalidate]; +} + +- (BOOL)isPaused { + return _displayLink.paused; +} + +- (void)setIsPaused:(BOOL)isPaused { + _displayLink.paused = isPaused; +} + +- (void)invalidate { + [_displayLink invalidate]; +} + +- (void)displayLinkDidFire:(CADisplayLink*)displayLink { + _timerHandler(); +} + +@end + @interface RTCEAGLVideoView () // |i420Frame| is set when we receive a frame from a worker thread and is read // from the display link callback so atomicity is required. @property(atomic, strong) RTCI420Frame* i420Frame; +@property(nonatomic, readonly) GLKView* glkView; +@property(nonatomic, readonly) RTCOpenGLVideoRenderer* glRenderer; @end @implementation RTCEAGLVideoView { - CADisplayLink* _displayLink; + RTCDisplayLinkTimer* _timer; GLKView* _glkView; RTCOpenGLVideoRenderer* _glRenderer; RTCVideoRenderer* _videoRenderer; @@ -79,14 +137,21 @@ selector:@selector(didBecomeActive) name:UIApplicationDidBecomeActiveNotification object:nil]; - _displayLink = - [CADisplayLink displayLinkWithTarget:self - selector:@selector(displayLinkDidFire:)]; - _displayLink.paused = YES; - // Set to half of screen refresh, which should be 30fps. - [_displayLink setFrameInterval:2]; - [_displayLink addToRunLoop:[NSRunLoop currentRunLoop] - forMode:NSRunLoopCommonModes]; + + // Frames are received on a separate thread, so we poll for current frame + // using a refresh rate proportional to screen refresh frequency. This + // occurs on the main thread. + __weak RTCEAGLVideoView* weakSelf = self; + _timer = [[RTCDisplayLinkTimer alloc] initWithTimerHandler:^{ + RTCEAGLVideoView* strongSelf = weakSelf; + // Don't render if frame hasn't changed. + if (strongSelf.glRenderer.lastDrawnFrame == strongSelf.i420Frame) { + return; + } + // This tells the GLKView that it's dirty, which will then call the + // GLKViewDelegate method implemented below. + [strongSelf.glkView setNeedsDisplay]; + }]; _videoRenderer = [[RTCVideoRenderer alloc] initWithDelegate:self]; [self setupGL]; } @@ -100,6 +165,7 @@ if (appState == UIApplicationStateActive) { [self teardownGL]; } + [_timer invalidate]; } - (void)setVideoTrack:(RTCVideoTrack*)videoTrack { @@ -134,26 +200,13 @@ #pragma mark - Private -// Frames are received on a separate thread, so we poll for current frame -// using a refresh rate proportional to screen refresh frequency. This occurs -// on main thread. -- (void)displayLinkDidFire:(CADisplayLink*)displayLink { - // Don't render if frame hasn't changed. - if (_glRenderer.lastDrawnFrame == self.i420Frame) { - return; - } - // This tells the GLKView that it's dirty, which will then call the the - // GLKViewDelegate method implemented above. - [_glkView setNeedsDisplay]; -} - - (void)setupGL { [_glRenderer setupGL]; - _displayLink.paused = NO; + _timer.isPaused = NO; } - (void)teardownGL { - _displayLink.paused = YES; + _timer.isPaused = YES; [_glkView deleteDrawable]; [_glRenderer teardownGL]; } @@ -176,8 +229,10 @@ // provide. This occurs on non-main thread. - (void)renderer:(RTCVideoRenderer*)renderer didSetSize:(CGSize)size { + __weak RTCEAGLVideoView* weakSelf = self; dispatch_async(dispatch_get_main_queue(), ^{ - [self.delegate videoView:self didChangeVideoSize:size]; + RTCEAGLVideoView* strongSelf = weakSelf; + [strongSelf.delegate videoView:strongSelf didChangeVideoSize:size]; }); } diff --git a/talk/examples/objc/AppRTCDemo/APPRTCConnectionManager.m b/talk/examples/objc/AppRTCDemo/APPRTCConnectionManager.m index 6d0a5a215..b411a6215 100644 --- a/talk/examples/objc/AppRTCDemo/APPRTCConnectionManager.m +++ b/talk/examples/objc/AppRTCDemo/APPRTCConnectionManager.m @@ -101,6 +101,7 @@ [self.peerConnection close]; self.peerConnection = nil; self.client = nil; + self.videoSource = nil; self.queuedRemoteCandidates = nil; }