Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2991863002:
ObjC RTCEAGLVideoVideo: Check GL context is non-nil in constructor (Closed)
|
Created:
3 years, 4 months ago by magjed_webrtc Modified:
3 years, 4 months ago Reviewers:
tkchin_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionObjC RTCEAGLVideoVideo: Check GL context is non-nil in constructor
RTCEAGLVideoVideo ensureGLContext has been observed to fail because the
GL context is nil. This CL checks the GL context is non-nil in the ctor
instead.
BUG=b/62865840
Review-Url: https://codereview.webrtc.org/2991863002
Cr-Commit-Position: refs/heads/master@{#19189}
Committed: https://chromium.googlesource.com/external/webrtc/+/1c12b818b3b1ca12689d7fcbb6f629dfc80bc8d0
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add log #Patch Set 3 : RTCLogError #Messages
Total messages: 20 (13 generated)
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/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== ObjC RTCEAGLVideoVideo: Check GL context is non-nil in constructor RTCEAGLVideoVideo ensureGLContext has been observed to fail because the GL context is nil. This CL checks this immediately in the ctor instead. BUG=b/62865840 ========== to ========== ObjC RTCEAGLVideoVideo: Check GL context is non-nil in constructor RTCEAGLVideoVideo ensureGLContext has been observed to fail because the GL context is nil. This CL checks the GL context is non-nil in the ctor instead. BUG=b/62865840 ==========
magjed@webrtc.org changed reviewers: + tkchin@webrtc.org
Zeke - please take a look.
yeah I don't know what more we can do. Don't know if a retry would help either. I wonder if this is happening because we are trying to create an EAGLContext while the app is backgrounded. Would you mind trying that to see if that's the root cause of the crash? Meanwhile, lgtm https://codereview.webrtc.org/2991863002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/UI/RTCEAGLVideoView.m (right): https://codereview.webrtc.org/2991863002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/UI/RTCEAGLVideoView.m:148: return NO; nit: add an error log
On 2017/07/27 20:42:13, tkchin_webrtc wrote: > yeah I don't know what more we can do. Don't know if a retry would help either. > I wonder if this is happening because we are trying to create an EAGLContext > while the app is backgrounded. Would you mind trying that to see if that's the > root cause of the crash? How do I easily try that? I'm landing this CL so we can verify that the EAGLContext creation was indeed the problem (I can't see what else could go wrong from looking at the code). https://codereview.webrtc.org/2991863002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/UI/RTCEAGLVideoView.m (right): https://codereview.webrtc.org/2991863002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/UI/RTCEAGLVideoView.m:148: return NO; On 2017/07/27 20:42:12, tkchin_webrtc wrote: > nit: add an error log Done.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2991863002/#ps20001 (title: "Add log")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by magjed@webrtc.org
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2991863002/#ps40001 (title: "RTCLogError")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1501514728003870,
"parent_rev": "ed8ceb61e6e3f44ed960f27dc84d1389aba76387", "commit_rev":
"1c12b818b3b1ca12689d7fcbb6f629dfc80bc8d0"}
Message was sent while issue was closed.
Description was changed from ========== ObjC RTCEAGLVideoVideo: Check GL context is non-nil in constructor RTCEAGLVideoVideo ensureGLContext has been observed to fail because the GL context is nil. This CL checks the GL context is non-nil in the ctor instead. BUG=b/62865840 ========== to ========== ObjC RTCEAGLVideoVideo: Check GL context is non-nil in constructor RTCEAGLVideoVideo ensureGLContext has been observed to fail because the GL context is nil. This CL checks the GL context is non-nil in the ctor instead. BUG=b/62865840 Review-Url: https://codereview.webrtc.org/2991863002 Cr-Commit-Position: refs/heads/master@{#19189} Committed: https://chromium.googlesource.com/external/webrtc/+/1c12b818b3b1ca12689d7fcbb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/1c12b818b3b1ca12689d7fcbb...
Message was sent while issue was closed.
On 2017/07/31 15:23:23, magjed_webrtc wrote: > On 2017/07/27 20:42:13, tkchin_webrtc wrote: > > yeah I don't know what more we can do. Don't know if a retry would help > either. > > I wonder if this is happening because we are trying to create an EAGLContext > > while the app is backgrounded. Would you mind trying that to see if that's the > > root cause of the crash? > > How do I easily try that? I'm landing this CL so we can verify that the > EAGLContext creation was indeed the problem (I can't see what else could go > wrong from looking at the code). > > https://codereview.webrtc.org/2991863002/diff/1/webrtc/sdk/objc/Framework/Cla... > File webrtc/sdk/objc/Framework/Classes/UI/RTCEAGLVideoView.m (right): > > https://codereview.webrtc.org/2991863002/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/UI/RTCEAGLVideoView.m:148: return NO; > On 2017/07/27 20:42:12, tkchin_webrtc wrote: > > nit: add an error log > > Done. Can try adding a hook to AppRTCMobile in the app backgrounded handler to create the view and add it somewhere to the hierarchy in non-zero size and see if the context is nil. If it was indeed the problem, now we will instead have a blank view instead of a crash (due to nil return), so better, but still not great. |
