RTCEAGLVideoView: Fix GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT error.

talk/app/webrtc/objc/RTCEAGLVideoView.m

Index: talk/app/webrtc/objc/RTCEAGLVideoView.m
diff --git a/talk/app/webrtc/objc/RTCEAGLVideoView.m b/talk/app/webrtc/objc/RTCEAGLVideoView.m
index 61040ba1025c2f23eed07f916df93d36b2955d76..6192b65867dcb4cc98a2f8f72e13f6798f5c1052 100644
--- a/talk/app/webrtc/objc/RTCEAGLVideoView.m
+++ b/talk/app/webrtc/objc/RTCEAGLVideoView.m
@@ -92,6 +92,18 @@
 
 @end
 
+// RTCEAGLVideoView wraps a GLKView which is setup with
+// enableSetNeedsDisplay = NO for the purpose of gaining control of
+// exactly when to call -[GLKView display]. This need for extra
+// control is required to avoid triggering method calls on GLKView
+// that results in attempting to bind the underlying render buffer
+// when the drawable size would be empty which would result in the
+// error GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT. -[GLKView display] is
+// 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>
 // |i420Frame| is set when we receive a frame from a worker thread and is read
 // from the display link callback so atomicity is required.
@@ -104,6 +116,8 @@
   RTCDisplayLinkTimer* _timer;
   GLKView* _glkView;
   RTCOpenGLVideoRenderer* _glRenderer;
+  BOOL _isDirty; // this flag should only be set and read on the main thread (e.g. by

[magjed_webrtc, 2015/09/21 11:44:10]

Put this comment above the variable instead. Also s/this/This/g and keep it to
80 chars per line.

+                 // setNeedsDisplay)
 }
 
 - (instancetype)initWithFrame:(CGRect)frame {
@@ -137,6 +151,7 @@
   _glkView.drawableMultisample = GLKViewDrawableMultisampleNone;
   _glkView.delegate = self;
   _glkView.layer.masksToBounds = YES;
+  _glkView.enableSetNeedsDisplay = NO;
   [self addSubview:_glkView];
 
   // Listen to application state in order to clean up OpenGL before app goes
@@ -158,13 +173,7 @@
   __weak RTCEAGLVideoView* weakSelf = self;
   _timer = [[RTCDisplayLinkTimer alloc] initWithTimerHandler:^{
       RTCEAGLVideoView* strongSelf = weakSelf;
-      // Don't render if frame hasn't changed.

[magjed_webrtc, 2015/09/21 11:44:10]

What if you keep this code unchanged, remove the variable |isDirty|, and call a
custom display function with the bounds check in setNeedsDisplay() instead? I.e.
something like:
- (void)setNeedsDisplay {
  [super setNeedsDisplay];
  displayIfReady();
}    
- (void)setNeedsDisplayInRect:(CGRect)rect {
  [super setNeedsDisplayInRect:rect];
  displayIfReady();
}
- (void)displayIfReady {
  // 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];
  }
}
?

-      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];
+      [strongSelf displayLinkTimerDidFire];
     }];
   [self setupGL];
 }
@@ -181,6 +190,16 @@
 
 #pragma mark - UIView
 
+- (void)setNeedsDisplay {
+  [super setNeedsDisplay];
+  _isDirty = YES;

[magjed_webrtc, 2015/09/21 11:44:10]

You still need to execute the code in displayLinkTimerDidFire() otherwise this
won't have any effect?

+}
+
+- (void)setNeedsDisplayInRect:(CGRect)rect {
+  [super setNeedsDisplayInRect:rect];
+  _isDirty = YES;
+}
+
 - (void)layoutSubviews {
   [super layoutSubviews];
   _glkView.frame = self.bounds;
@@ -213,6 +232,26 @@
 
 #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.i420Frame) {
+    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];

[magjed_webrtc, 2015/09/21 11:44:10]

Is there any way to override the display call and inject the bounds check there
instead? It seems like that is the only problem with the current code. I'm
surprised GLKView doesn't handle that itself.

+  }
+}
+
 - (void)setupGL {
   self.i420Frame = nil;
   [_glRenderer setupGL];