|
|
DescriptionRTCEAGLVideoView: 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}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Reworked patch based on enableSetNeedsDisplay #
Total comments: 2
Patch Set 3 : Add isDirty flag #
Total comments: 5
Patch Set 4 : Fix comment: 80 chars per line #Messages
Total messages: 30 (8 generated)
christoffer@sinch.com changed reviewers: + tommi@webrtc.org
tommi@webrtc.org changed reviewers: + magjed@webrtc.org
Magnus can you take a look?
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?
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
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?
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.
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'.
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'.
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.
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.
> 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.
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.
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.
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by magjed@webrtc.org
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")
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
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/939)
tommi - We need a rubber stamp from you.
rs lgtm
The CQ bit was checked by magjed@webrtc.org
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
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/88799d9c1f496ecbe2b17c7508c95566175a29fc Cr-Commit-Position: refs/heads/master@{#10076} |