Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 6 months ago by tkchin_webrtc Modified:
4 years, 6 months ago Reviewers:
Chuck CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, ivoc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd RTCEventLog API to ObjC.
NOTRY=True
BUG=
Committed: https://crrev.com/204177f9670919ce0294611bd7846bb461e50c04
Cr-Commit-Position: refs/heads/master@{#13144}
Patch Set 1 #
Total comments: 5
Patch Set 2 : CR comments #Patch Set 3 : fix build error. #Patch Set 4 : Add guard back. #
Created: 4 years, 6 months ago
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Add RTCEventLog API to ObjC. BUG= ========== to ========== Add RTCEventLog API to ObjC. BUG= ==========
tkchin@webrtc.org changed reviewers: + haysc@webrtc.org
On 2016/06/13 22:17:19, tkchin_webrtc wrote: Chuck, mind taking a look? @ivoc / @terelius: How do I actually visualize the generated data?
On 2016/06/13 22:18:04, tkchin_webrtc wrote: > On 2016/06/13 22:17:19, tkchin_webrtc wrote: > > Chuck, mind taking a look? > > @ivoc / @terelius: How do I actually visualize the generated data? There program rtc_event_log2rtp_dump converts the logs to rtp dumps, which can then be analysed using any tool that operates on rtp dumps. The visualization tool is currently under review, but you can download the patch from https://codereview.webrtc.org/1995523002/ and run it as out/Debug/event_log_visualizer event_log_file | python
lgtm https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:315: [_factory stopRtcEventLog]; Do you need the #if guards here? https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:533: [_factory startRtcEventLogWithFilePath:filePath Your comment suggests this is temporary, but should you check the return value here and log the result?
https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:315: [_factory stopRtcEventLog]; On 2016/06/14 14:01:17, Chuck wrote: > Do you need the #if guards here? Prob not. https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:533: [_factory startRtcEventLogWithFilePath:filePath On 2016/06/14 14:01:17, Chuck wrote: > Your comment suggests this is temporary, but should you check the return value > here and log the result? Done.
https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:315: [_factory stopRtcEventLog]; On 2016/06/14 17:40:59, tkchin_webrtc wrote: > On 2016/06/14 14:01:17, Chuck wrote: > > Do you need the #if guards here? > > Prob not. I think I confused myself, I didn't see them in the original CL (since they were already there), and was actually suggesting to add them if you needed them.
On 2016/06/14 19:36:44, Chuck wrote: > https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... > File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): > > https://codereview.webrtc.org/2067683002/diff/1/webrtc/examples/objc/AppRTCDe... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:315: [_factory stopRtcEventLog]; > On 2016/06/14 17:40:59, tkchin_webrtc wrote: > > On 2016/06/14 14:01:17, Chuck wrote: > > > Do you need the #if guards here? > > > > Prob not. > > I think I confused myself, I didn't see them in the original CL (since they were > already there), and was actually suggesting to add them if you needed them. haha ok I'll add them back so that the calls are balanced then.
Description was changed from ========== Add RTCEventLog API to ObjC. BUG= ========== to ========== Add RTCEventLog API to ObjC. NOTRY=True BUG= ==========
The CQ bit was checked by tkchin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from haysc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2067683002/#ps60001 (title: "Add guard back.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067683002/60001
Message was sent while issue was closed.
Description was changed from ========== Add RTCEventLog API to ObjC. NOTRY=True BUG= ========== to ========== Add RTCEventLog API to ObjC. NOTRY=True BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add RTCEventLog API to ObjC. NOTRY=True BUG= ========== to ========== Add RTCEventLog API to ObjC. NOTRY=True BUG= Committed: https://crrev.com/204177f9670919ce0294611bd7846bb461e50c04 Cr-Commit-Position: refs/heads/master@{#13144} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/204177f9670919ce0294611bd7846bb461e50c04 Cr-Commit-Position: refs/heads/master@{#13144} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
