RTCEAGLVideoView: Fix GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT error.

talk/app/webrtc/objc/RTCEAGLVideoView.m

 /*
  * libjingle
  * Copyright 2014 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 74 matching lines @@
 - (void)invalidate {
   [_displayLink invalidate];
 }
 
 - (void)displayLinkDidFire:(CADisplayLink*)displayLink {
   _timerHandler();
 }
 
 @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.
 @property(atomic, strong) RTCI420Frame* i420Frame;
 @property(nonatomic, readonly) GLKView* glkView;
 @property(nonatomic, readonly) RTCOpenGLVideoRenderer* glRenderer;
 @end
 
 @implementation RTCEAGLVideoView {
   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 {
   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];
   }
   _glRenderer = [[RTCOpenGLVideoRenderer alloc] initWithContext:glContext];
 
   // GLKView manages a framebuffer for us.
   _glkView = [[GLKView alloc] initWithFrame:CGRectZero

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

Would it solve all problems to simply replace CGRectZero with CGRectMake(0, 0,
1, 1)? :/

                                     context:glContext];
   _glkView.drawableColorFormat = GLKViewDrawableColorFormatRGBA8888;
   _glkView.drawableDepthFormat = GLKViewDrawableDepthFormatNone;
   _glkView.drawableStencilFormat = GLKViewDrawableStencilFormatNone;
   _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
   // away.
   NSNotificationCenter* notificationCenter =
     [NSNotificationCenter defaultCenter];
   [notificationCenter addObserver:self
                          selector:@selector(willResignActive)
                              name:UIApplicationWillResignActiveNotification
                            object:nil];
   [notificationCenter addObserver:self
                          selector:@selector(didBecomeActive)
                              name:UIApplicationDidBecomeActiveNotification
                            object:nil];
 
   // 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.

[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];
 }
 
 - (void)dealloc {
   [[NSNotificationCenter defaultCenter] removeObserver:self];
   UIApplicationState appState =
       [UIApplication sharedApplication].applicationState;
   if (appState == UIApplicationStateActive) {
     [self teardownGL];
   }
   [_timer invalidate];
 }
 
 #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;
 }
 
 #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 {
@@ skipping 12 matching lines @@
     [strongSelf.delegate videoView:strongSelf didChangeVideoSize:size];
   });
 }
 
 - (void)renderFrame:(RTCI420Frame*)frame {
   self.i420Frame = 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.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];
   _timer.isPaused = NO;
 }
 
 - (void)teardownGL {
   self.i420Frame = nil;
   _timer.isPaused = YES;
   [_glkView deleteDrawable];
   [_glRenderer teardownGL];
 }
 
 - (void)didBecomeActive {
   [self setupGL];
 }
 
 - (void)willResignActive {
   [self teardownGL];
 }
 
 @end