ObjC: Remove RTCOpenGLVideoRenderer

webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m

 /*
  *  Copyright 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #import "WebRTC/RTCEAGLVideoView.h"
 
 #import <GLKit/GLKit.h>
 
-#import "RTCOpenGLVideoRenderer.h"
+#import "RTCShader+Private.h"
 #import "WebRTC/RTCVideoFrame.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;
 
@@ skipping 55 matching lines @@
 // the method that will trigger the binding of the render
 // buffer. Because the standard behaviour of -[UIView setNeedsDisplay]
 // is disabled for the reasons above, the RTCEAGLVideoView maintains
 // its own |isDirty| flag.
 
 @interface RTCEAGLVideoView () <GLKViewDelegate>
 // |videoFrame| 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) RTCVideoFrame *videoFrame;
 @property(nonatomic, readonly) GLKView *glkView;
-@property(nonatomic, readonly) RTCOpenGLVideoRenderer *glRenderer;
 @end
 
 @implementation RTCEAGLVideoView {
   RTCDisplayLinkTimer *_timer;
   EAGLContext *_glContext;
   // This flag should only be set and read on the main thread (e.g. by
   // setNeedsDisplay)
   BOOL _isDirty;
+
+  BOOL _isInitialized;

[sakal, 2017/04/11 11:00:21]

Why is this only needed on iOS and not on Mac?

[magjed_webrtc, 2017/04/11 12:05:43]

It was obvious from the code that it wasn't necessary on Mac so I removed it. I
want to be careful and not make any functional changes, so I kept it for iOS. I
looked into it more now, and it looks like it's possible to remove for iOS as
well.

+  id<RTCShader> _i420Shader;
+  id<RTCShader> _nv12Shader;
+  RTCVideoFrame *_lastDrawnFrame;
 }
 
 @synthesize delegate = _delegate;
 @synthesize videoFrame = _videoFrame;
 @synthesize glkView = _glkView;
-@synthesize glRenderer = _glRenderer;
 
 - (instancetype)initWithFrame:(CGRect)frame {
   if (self = [super initWithFrame:frame]) {
     [self configure];
   }
   return self;
 }
 
 - (instancetype)initWithCoder:(NSCoder *)aDecoder {
   if (self = [super initWithCoder:aDecoder]) {
     [self configure];
   }
   return self;
 }
 
 - (void)configure {
   EAGLContext *glContext =
     [[EAGLContext alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
   if (!glContext) {
     glContext = [[EAGLContext alloc] initWithAPI:kEAGLRenderingAPIOpenGLES2];
   }
   _glContext = glContext;
-  _glRenderer = [[RTCOpenGLVideoRenderer alloc] initWithContext:_glContext];
 
   // GLKView manages a framebuffer for us.
   _glkView = [[GLKView alloc] initWithFrame:CGRectZero
                                     context:_glContext];
   _glkView.drawableColorFormat = GLKViewDrawableColorFormatRGBA8888;
   _glkView.drawableDepthFormat = GLKViewDrawableDepthFormatNone;
   _glkView.drawableStencilFormat = GLKViewDrawableStencilFormatNone;
   _glkView.drawableMultisample = GLKViewDrawableMultisampleNone;
   _glkView.delegate = self;
   _glkView.layer.masksToBounds = YES;
@@ skipping 54 matching lines @@
   _glkView.frame = self.bounds;
 }
 
 #pragma mark - GLKViewDelegate
 
 // This method is called when the GLKView's content is dirty and needs to be
 // redrawn. This occurs on main thread.
 - (void)glkView:(GLKView *)view drawInRect:(CGRect)rect {
   // The renderer will draw the frame to the framebuffer corresponding to the
   // one used by |view|.
-  [_glRenderer drawFrame:self.videoFrame];
+  RTCVideoFrame *frame = self.videoFrame;
+  if (!_isInitialized || !frame || frame == _lastDrawnFrame) {

[sakal, 2017/04/11 11:00:21]

Is this something we expect to happen or should we log here?

[magjed_webrtc, 2017/04/11 12:05:43]

Yes, it can happen that we try to render the same frame multiple times.

+    return;
+  }
+  [self ensureGLContext];
+  glClear(GL_COLOR_BUFFER_BIT);
+  id<RTCShader> shader = nil;
+  if (frame.nativeHandle) {
+    if (!_nv12Shader) {
+      _nv12Shader = [[RTCNativeNV12Shader alloc] initWithContext:_glContext];
+    }
+    shader = _nv12Shader;
+  } else {
+    if (!_i420Shader) {
+      _i420Shader = [[RTCI420Shader alloc] initWithContext:_glContext];
+    }
+    shader = _i420Shader;
+  }
+  if (shader && [shader drawFrame:frame]) {

[sakal, 2017/04/11 11:00:21]

Log on failure?

[magjed_webrtc, 2017/04/11 12:05:43]

We didn't do it previously, but I guess we can add a log. We will log closer to
the actual failure as well when drawFrame fails.

+    _lastDrawnFrame = frame;
+  }
 }
 
 #pragma mark - RTCVideoRenderer
 
 // These methods may be called on non-main thread.
 - (void)setSize:(CGSize)size {
   __weak RTCEAGLVideoView *weakSelf = self;
   dispatch_async(dispatch_get_main_queue(), ^{
     RTCEAGLVideoView *strongSelf = weakSelf;
     [strongSelf.delegate videoView:strongSelf didChangeVideoSize:size];
   });
 }
 
 - (void)renderFrame:(RTCVideoFrame *)frame {
   self.videoFrame = frame;
 }
 
 #pragma mark - Private
 
 - (void)displayLinkTimerDidFire {
   // Don't render unless video frame have changed or the view content
   // has explicitly been marked dirty.
-  if (!_isDirty && _glRenderer.lastDrawnFrame == self.videoFrame) {
+  if (!_isDirty && _lastDrawnFrame == self.videoFrame) {
     return;
   }
 
   // Always reset isDirty at this point, even if -[GLKView display]
   // won't be called in the case the drawable size is empty.
   _isDirty = NO;
 
   // Only call -[GLKView display] if the drawable size is
   // non-empty. Calling display will make the GLKView setup its
   // render buffer if necessary, but that will fail with error
   // GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT if size is empty.
   if (self.bounds.size.width > 0 && self.bounds.size.height > 0) {
     [_glkView display];
   }
 }
 
 - (void)setupGL {
   self.videoFrame = nil;
-  [_glRenderer setupGL];
+  if (!_isInitialized) {

[sakal, 2017/04/11 11:00:21]

I don't like the way isInitialized is used. It seems to only refer to parts of
the class. Can it be changed to affect the whole class? i.e. check if
if(_isInitialized) and return at the start. Same for teardown.

[magjed_webrtc, 2017/04/11 12:05:43]

It looks like we don't need it at all.

+    [self ensureGLContext];
+    _isInitialized = YES;
+  }
   _timer.isPaused = NO;
 }
 
 - (void)teardownGL {
   self.videoFrame = nil;
   _timer.isPaused = YES;
   [_glkView deleteDrawable];
-  [_glRenderer teardownGL];
+  if (_isInitialized) {
+    [self ensureGLContext];
+    _i420Shader = nil;
+    _nv12Shader = nil;
+    _isInitialized = NO;
+  }
 }
 
 - (void)didBecomeActive {
   [self setupGL];
 }
 
 - (void)willResignActive {
   [self teardownGL];
 }
 
+- (void)ensureGLContext {
+  NSAssert(_glContext, @"context shouldn't be nil");
+  if ([EAGLContext currentContext] != _glContext) {
+    [EAGLContext setCurrentContext:_glContext];
+  }
+}
+
 @end