RTCEAGLVideoView: Fix GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT error.

RTCEAGLVideoView: Fix GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT error.

Fix an issue where using setNeedsDisplay on a GLKView which has a frame
with size zero will make GLKView/iOS output the following error:

  Failed to bind EAGLDrawable: <CAEAGLLayer: 0x1742282e0> to
    GL_RENDERBUFFER 1 Failed to make complete framebuffer object 8cd6

(The error code 8cd6 corresponds to
GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT.)

GLKView will internally setup it's render buffer when the delegate is
about to draw into it. Previously when enableSetNeedsDisplay was set to
YES (default), then GLKView would still attempt to setup it's internal
buffer even if it's frame size is zero and that would cause
GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT.

By using enableSetNeedsDisplay = NO, RTCEAGLVideoView can guard against
calling -[GLKView display] if it's current frame size is empty.

Committed: https://crrev.com/88799d9c1f496ecbe2b17c7508c95566175a29fc
Cr-Commit-Position: refs/heads/master@{#10076}

[cahlbin, 2015-09-16 10:32:13]

christoffer@sinch.com changed reviewers:
+ tommi@webrtc.org

[tommi, 2015-09-16 16:45:55]

tommi@webrtc.org changed reviewers:
+ magjed@webrtc.org

[tommi, 2015-09-16 16:45:56]

Magnus can you take a look?

[magjed_webrtc, 2015-09-17 08:59:46]

Where exactly does it crash? It should be ok to create an incomplete framebuffer
in configure() as long as it's not used until it's complete. I would guess it's
crashing in _glRenderer drawFrame? Maybe just add a guard there to not draw
frames until framebuffer is complete? Is this function what makes the
framebuffer complete in the current code:
- (void)layoutSubviews {
  [super layoutSubviews];
  _glkView.frame = self.bounds;
}
?

https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
File talk/app/webrtc/objc/RTCEAGLVideoView.m (left):

https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
talk/app/webrtc/objc/RTCEAGLVideoView.m:129: _glRenderer =
[[RTCOpenGLVideoRenderer alloc] initWithContext:glContext];
Why can't _glRenderer alloc stay in this function?

https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
File talk/app/webrtc/objc/RTCEAGLVideoView.m (right):

https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
talk/app/webrtc/objc/RTCEAGLVideoView.m:221: [_glRenderer
drawFrame:self.i420Frame];
Maybe drop frames here if framebuffer is not complete yet?

[cahlbin, 2015-09-17 13:46:25]

On 2015/09/17 08:59:46, magjed_webrtc wrote:
> Where exactly does it crash? It should be ok to create an incomplete
framebuffer
> in configure() as long as it's not used until it's complete. I would guess
it's
> crashing in _glRenderer drawFrame? Maybe just add a guard there to not draw
> frames until framebuffer is complete? Is this function what makes the
> framebuffer complete in the current code:
> - (void)layoutSubviews {
>   [super layoutSubviews];
>   _glkView.frame = self.bounds;
> }
> ?
> 
>
https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
> File talk/app/webrtc/objc/RTCEAGLVideoView.m (left):
> 
>
https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
> talk/app/webrtc/objc/RTCEAGLVideoView.m:129: _glRenderer =
> [[RTCOpenGLVideoRenderer alloc] initWithContext:glContext];
> Why can't _glRenderer alloc stay in this function?
> 
>
https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
> File talk/app/webrtc/objc/RTCEAGLVideoView.m (right):
> 
>
https://codereview.webrtc.org/1347013002/diff/1/talk/app/webrtc/objc/RTCEAGLV...
> talk/app/webrtc/objc/RTCEAGLVideoView.m:221: [_glRenderer
> drawFrame:self.i420Frame];
> Maybe drop frames here if framebuffer is not complete yet?

Sorry for not being clear about that it's not a hard crash, it's just error log
output that GLKView (or some of it's underlying components) is emitting.

I've attached a NSLog-sprinkled output below where it can be seen more clearly
that it's in between layoutSubviews and glkView:drawInRect: that GLKView is
emitting the error. I'm quite certain that it's not directly tied to
layoutSubviews, but rather to the setNeedsDisplay and how that affects when
GLKView attempts to bind it's drawable.

I've uploaded a new fix suggestion based on this new knowledge.

One thing I've considered but not sure about is whether it's necessary is to
have a separate flag which is set if the bounds change and that flag would then
nullify the condition (_glRenderer.lastDrawnFrame == self.i420Frame) for the
next draw cycle (and the reset the flag after a call to -[GLKView display]). I'm
leaning towards that for the general case it's not necessary, but it may be
needed if contentMode have been set to UIViewContentModeRedraw on the
RTCEAGLVideoView. Your thoughts on this?


===

Log output with no changes to RTCEAGLVideoView (I.e. equivalent to rev.
d2320cee876b6af48207d021f78aa8759976319c)

RTCEAGLVideoView initWithFrame: BEGIN frame={Height = 0; Width = 0; X = 0; Y =
0;}
RTCOpenGLVideoRenderer initWithContext: BEGIN
RTCOpenGLVideoRenderer initWithContext: END
GLKView alloc initWithFrame:context: BEGIN
GLKView alloc initWithFrame: END
RTCEAGLVideoView setupGL BEGIN
RTCEAGLVideoView setupGL END
RTCEAGLVideoView initWithFrame: END
RTCEAGLVideoView layoutSubviews BEGIN
RTCEAGLVideoView self.bounds: { Height = 0; Width = 0; X = 0; Y = 0; },
self.window: <UIWindow: 0x12c622db0; frame = (0 0; 320 568); gestureRecognizers
= <NSArray: 0x174243690>; layer = <UIWindowLayer: 0x17422abe0>>
RTCEAGLVideoView layoutSubviews END
Failed to bind EAGLDrawable: <CAEAGLLayer: 0x1704208e0> to GL_RENDERBUFFER 1
Failed to make complete framebuffer object 8cd6
RTCEAGLVideoView glkView:drawInRect: (w=0.000000, h=0.000000)
RTCEAGLVideoView renderFrame: (w=480.000000, h=640.000000)
RTCEAGLVideoView setSize: BEGIN (480.000000, 640.000000)
RTCEAGLVideoView setSize: END
RTCEAGLVideoView layoutSubviews BEGIN
RTCEAGLVideoView self.bounds: { Height = 280; Width = 210; X = 0; Y = 0;},
self.window: <UIWindow: 0x12c622db0; frame = (0 0; 320 568); gestureRecognizers
= <NSArray: 0x174243690>; layer = <UIWindowLayer: 0x17422abe0>>
RTCEAGLVideoView layoutSubviews END
RTCEAGLVideoView glkView:drawInRect: (w=210.000000, h=280.000000)
RTCEAGLVideoView renderFrame: (w=480.000000, h=640.000000)
... success from here on

[magjed_webrtc, 2015-09-18 09:24:14]

You are probably right that we should have a 'isDirty' flag that we set on e.g.
bounds changes and something like this:
if (!isDirty && _glRenderer.lastDrawnFrame == self.i420Frame) {
  return;
}
...
isDirty = false;
[_glkView display]
I think that is an unrelated issue so you don't need to fix it here if you don't
want to. If you do want to fix it, I would suggest a separate CL.

https://codereview.webrtc.org/1347013002/diff/20001/talk/app/webrtc/objc/RTCE...
File talk/app/webrtc/objc/RTCEAGLVideoView.m (right):

https://codereview.webrtc.org/1347013002/diff/20001/talk/app/webrtc/objc/RTCE...
talk/app/webrtc/objc/RTCEAGLVideoView.m:218: // non-empty. Calling display will
make the GLKView setup it's
nit: s/it's/its/g

https://codereview.webrtc.org/1347013002/diff/20001/talk/app/webrtc/objc/RTCE...
talk/app/webrtc/objc/RTCEAGLVideoView.m:222: [_glkView display];
It looks like you changed setNeedsDisplay to display? What's the difference?

[cahlbin, 2015-09-18 09:55:17]

Because the core of the CL is by using enableSetNeedsDisplay = NO, gain the
control necessary to not trigger the calls that make the GLKView bind the render
buffer if the drawable area is empty. So as a consequence of that, the change
from use of |setNeedsDisplay| to |display| is necessary.

When enableSetNeedsDisplay is set to NO, one must call |display| explicitly.
From GLKView.h docs:

/*
 -display should be called when the view has been set to ignore calls to
setNeedsDisplay. This method is used by
 the GLKViewController to invoke the draw method. It can also be used when not
using a GLKViewController and custom
 control of the display loop is needed.
 */


Should I include a comment about how the use of |enableSetNeedsDisplay| and
|display| is used? If so, should I put it as an implementation comment inside
the method |displayLinkTimerDidFire|, or at the top of the RTCEAGLVideoView
class?


I'll fix the typo. I'm not completely familiar with the CL flow yet, should I
fix the typo and upload as a separate patchset that will have patchset #2 as
base? Or should I squash it locally and upload as a patchset #3 that is
squashed? Thanks.

I'll make an attempt at 'isDirty' as a separate CL.

[magjed_webrtc, 2015-09-18 10:14:13]

I see. So the need for 'isDirty' flag is related to this CL because of the
change '_glkView.enableSetNeedsDisplay = NO'? Maybe setNeedsDisplay will be
triggered by a lot of other stuff as well, any layout change etc. It sounds like
a hassle to handle that ourselves, maybe your first patch set is better?

Regarding squashing, it doesn't matter if you squash locally, it will not affect
how the patches show up here. Just use 'git cl upload' on your latest code. You
can even upload from another branch or computer even if you set the issue number
first with 'git cl issue 1347013002'.

[cahlbin, 2015-09-21 10:24:37]

On 2015/09/18 10:14:13, magjed_webrtc wrote:
> I see. So the need for 'isDirty' flag is related to this CL because of the
> change '_glkView.enableSetNeedsDisplay = NO'? Maybe setNeedsDisplay will be
> triggered by a lot of other stuff as well, any layout change etc. It sounds
like
> a hassle to handle that ourselves, maybe your first patch set is better?

I realised that the previous patchset (#1) was no good at all (it would still
emit the error when setNeedsDisplay was invoked).

I don't think the dirty flag will be that big of a hassle. As long as
setNeedsDisplay is implemented (which is now (patchset #3) setting the dirty
flag), we should be fine.


> 
> Regarding squashing, it doesn't matter if you squash locally, it will not
affect
> how the patches show up here. Just use 'git cl upload' on your latest code.
You
> can even upload from another branch or computer even if you set the issue
number
> first with 'git cl issue 1347013002'.

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

Nice solution to override setNeedsDisplay().

https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
File talk/app/webrtc/objc/RTCEAGLVideoView.m (left):

https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
talk/app/webrtc/objc/RTCEAGLVideoView.m:132: _glkView = [[GLKView alloc]
initWithFrame:CGRectZero
Would it solve all problems to simply replace CGRectZero with CGRectMake(0, 0,
1, 1)? :/

https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
talk/app/webrtc/objc/RTCEAGLVideoView.m:161: // Don't render if frame hasn't
changed.
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];
  }
}
?

https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
File talk/app/webrtc/objc/RTCEAGLVideoView.m (right):

https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
talk/app/webrtc/objc/RTCEAGLVideoView.m:119: BOOL _isDirty; // this flag should
only be set and read on the main thread (e.g. by
Put this comment above the variable instead. Also s/this/This/g and keep it to
80 chars per line.

https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
talk/app/webrtc/objc/RTCEAGLVideoView.m:195: _isDirty = YES;
You still need to execute the code in displayLinkTimerDidFire() otherwise this
won't have any effect?

https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
talk/app/webrtc/objc/RTCEAGLVideoView.m:251: [_glkView display];
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.

[cahlbin, 2015-09-21 12:56:13]

On 2015/09/21 11:44:10, magjed_webrtc wrote:
> Nice solution to override setNeedsDisplay().
> 
>
https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
> File talk/app/webrtc/objc/RTCEAGLVideoView.m (left):
> 
>
https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
> talk/app/webrtc/objc/RTCEAGLVideoView.m:132: _glkView = [[GLKView alloc]
> initWithFrame:CGRectZero
> Would it solve all problems to simply replace CGRectZero with CGRectMake(0, 0,
> 1, 1)? :/

I think that will be less robust against the various uses of the
RTCEAGLVideoView. E.g. the way I noticed this issue in the first place (not in
the AppRTCDemo) is that a container view that is initialised with CGRectZero,
then lays out its subviews (i.e. a RTCEAGLVideoView) and sets all its subviews
frame and bounds to its own bounds (i.e. CGRectZero). And after that it
setNeedsDisplay was triggered in that view hierarchy, which leads to the error
being emitted. So in that use case, whatever frame the GLKView is inited with
doesn't seem to matter. I think that is a similar use case as is discussed in
https://groups.google.com/forum/#!topic/discuss-webrtc/pGzphc_5K7w 
(I'm not saying it's necessary the "correct" way of using it and one could argue
"Just don't init the container view with rect zero, or don't add it to the view
hierarchy before it has a non-empty frame set).)

>
https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
> talk/app/webrtc/objc/RTCEAGLVideoView.m:161: // Don't render if frame hasn't
> changed.
> 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];
>   }
> }
> ?
>

That would make the actual rendering be invoked immediately and "out-of-sync"
with the display's refresh rate? (i.e. not synced with the CADisplayLink).
I think setNeedsDisplay should be used only to set a dirty flag, and rely on the
regular render loop for actually rendering and flushing the render buffer.
One can also imagine that setNeedsDisplay is called "excessively" in between two
render loop cycles, and then it feels unnecessary to render multiple times?

>
https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
> File talk/app/webrtc/objc/RTCEAGLVideoView.m (right):
> 
>
https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
> talk/app/webrtc/objc/RTCEAGLVideoView.m:119: BOOL _isDirty; // this flag
should
> only be set and read on the main thread (e.g. by
> Put this comment above the variable instead. Also s/this/This/g and keep it to
> 80 chars per line.
Will fix (It's a bit unclear what the guideline is when the .clang-format in the
folder objc/ has ColumnLimit: 100 ;) )
> 
>
https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
> talk/app/webrtc/objc/RTCEAGLVideoView.m:195: _isDirty = YES;
> You still need to execute the code in displayLinkTimerDidFire() otherwise this
> won't have any effect?

Maybe I'm missing something completely here. My idea was that the _isDirty is
simply set in setNeedsDisplay, but that's it, then
the next time the CADisplayLink timer comes around, the check is:

  if (!_isDirty && _glRenderer.lastDrawnFrame == self.i420Frame) {
    return;
  }

So the only (but necessary) gain of the isDirty is if someone calls
setNeedsDisplay in between video frame updates (which I'm guessing could be a
case during animation of the frame / bounds) (if UIViewContentModeRedraw is
used)

> 
>
https://codereview.webrtc.org/1347013002/diff/40001/talk/app/webrtc/objc/RTCE...
> talk/app/webrtc/objc/RTCEAGLVideoView.m:251: [_glkView display];
> 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.

The GLKView ref docs
(https://developer.apple.com/library/prerelease/ios/documentation/GLkit/Refere...)
has the following section:

Subclassing Notes
Typically, there is no need to subclass the GLKView class. Instead, provide a
delegate object to draw the view’s contents. See GLKViewDelegate Protocol
Reference.

So I'm a bit worried that starting to subclass it could lead to other unforeseen
issues. But sure, it's not a recommendation saying it shouldn't work. If you
insist on that approach I may give it a try.

I'm surprised as well that the GLKView doesn't handle this whole issue itself.

[magjed_webrtc, 2015-09-21 14:59:56]

> That would make the actual rendering be invoked immediately and "out-of-sync"
> with the display's refresh rate? (i.e. not synced with the CADisplayLink).
> I think setNeedsDisplay should be used only to set a dirty flag, and rely on
the
> regular render loop for actually rendering and flushing the render buffer.
> One can also imagine that setNeedsDisplay is called "excessively" in between
two
> render loop cycles, and then it feels unnecessary to render multiple times?

This is a change from the current code where setNeedsDisplay triggers rendering,
but that is probably ok. This CL lgtm if you fix the comment nit.

[cahlbin, 2015-09-24 15:44:36]

On 2015/09/21 14:59:56, magjed_webrtc wrote:
> > That would make the actual rendering be invoked immediately and
"out-of-sync"
> > with the display's refresh rate? (i.e. not synced with the CADisplayLink).
> > I think setNeedsDisplay should be used only to set a dirty flag, and rely on
> the
> > regular render loop for actually rendering and flushing the render buffer.
> > One can also imagine that setNeedsDisplay is called "excessively" in between
> two
> > render loop cycles, and then it feels unnecessary to render multiple times?
> 
> This is a change from the current code where setNeedsDisplay triggers
rendering,
> but that is probably ok. This CL lgtm if you fix the comment nit.

Sorry for late reply, I thought I replied when uploading the final patch set
fixing the comment nit. Thanks for all review feedback so far.

[cahlbin, 2015-09-24 15:44:37]

On 2015/09/21 14:59:56, magjed_webrtc wrote:
> > That would make the actual rendering be invoked immediately and
"out-of-sync"
> > with the display's refresh rate? (i.e. not synced with the CADisplayLink).
> > I think setNeedsDisplay should be used only to set a dirty flag, and rely on
> the
> > regular render loop for actually rendering and flushing the render buffer.
> > One can also imagine that setNeedsDisplay is called "excessively" in between
> two
> > render loop cycles, and then it feels unnecessary to render multiple times?
> 
> This is a change from the current code where setNeedsDisplay triggers
rendering,
> but that is probably ok. This CL lgtm if you fix the comment nit.

Sorry for late reply, I thought I replied when uploading the final patch set
fixing the comment nit. Thanks for all review feedback so far.

[magjed_webrtc, 2015-09-24 16:09:57]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-24 16:10:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1347013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1347013002/60001

[commit-bot: I haz the power, 2015-09-24 18:38:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-24 18:38:05]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2015-09-24 18:48:37]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2015-09-24 18:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1347013002/#ps60001 (title:
"Fix comment: 80 chars per line")

[commit-bot: I haz the power, 2015-09-24 18:48:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1347013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1347013002/60001

[commit-bot: I haz the power, 2015-09-24 18:50:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-24 18:50:34]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/939)

[magjed_webrtc, 2015-09-25 12:18:16]

tommi - We need a rubber stamp from you.

[tommi, 2015-09-25 13:37:05]

rs lgtm

[magjed_webrtc, 2015-09-25 13:56:56]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2015-09-25 13:57:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1347013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1347013002/60001

[commit-bot: I haz the power, 2015-09-25 13:57:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-25 13:58:01]

Patchset 4 (id:??) landed as
https://crrev.com/88799d9c1f496ecbe2b17c7508c95566175a29fc
Cr-Commit-Position: refs/heads/master@{#10076}