Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1334003002:
Loopback mode for AppRTCDemo on iOS. (Closed)
DescriptionLoopback and audio only mode.
Adds a loopback button that will connect to itself by simulating another client connection to the web socket server.
Adds an audio only mode switch.
BUG=
Committed: https://crrev.com/913e645e10bd3a060add29ac634a7d28f82744f6
Cr-Commit-Position: refs/heads/master@{#10153}
Patch Set 1 #Patch Set 2 : Loopback #Patch Set 3 : Working loopback #Patch Set 4 : Add loopback button and audio only switch #Patch Set 5 : Review #
Total comments: 16
Patch Set 6 : Updated per CR #Patch Set 7 : Add call options header and start call button #
Total comments: 8
Patch Set 8 : Revisions for CR feedback #Patch Set 9 : Fixing test compile #Patch Set 10 : Fixing mac build #Patch Set 11 : Add oldest rotation mode to RTCFileLogger #Patch Set 12 : Print entire error object for capture session error handler #Messages
Total messages: 31 (12 generated)
haysc@webrtc.org changed reviewers: + henricka@webrtc.org, tkchin@webrtc.org
tkchin@webrtc.org changed reviewers: + henrika@webrtc.org - henricka@webrtc.org
https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:70: options:(NSDictionary *)options; Use either params, or the options struct. A new immutable options struct type may make sense here. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppEngineClient.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppEngineClient.m:26: @"https://apprtc.appspot.com/join/%@?debug=loopback"; I don't recall exactly, but do you actually need this? What does the server do differently if that parameter is specified again? https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.h (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.h:35: // another client. Can you elaborate on what it actually does? Loopback will likely confuse people who aren't already familiar with signaling. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.m:230: [self sendMessage:answer]; Interesting. I was thinking you could call peer connection methods directly instead of introducing another round trip but this works. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:153: _audioOnlyLabel = [[UILabel alloc] initWithFrame:CGRectZero]; I know it's a demo app, but can we make this prettier? ;-) https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:185: nit: fix indentation + remove unneeded line breaks
Thanks! LGTM (must admit that I don't know the details here) https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:67: - (void)connectToRoomWithId:(NSString *)roomId Should we add comments for all parameters here? https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:590: if (!_isAudioOnly) { Not sure what type of logging support we have but it would be nice with a log here stating that the video track is NULL but it is not an error.
Friendly Ping. Is this one ready to be landed?
Sorry this took so long to get back to Henrik. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:67: - (void)connectToRoomWithId:(NSString *)roomId On 2015/09/14 08:31:20, henrika_webrtc wrote: > Should we add comments for all parameters here? Done. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:70: options:(NSDictionary *)options; On 2015/09/11 17:53:53, tkchin_webrtc wrote: > Use either params, or the options struct. > A new immutable options struct type may make sense here. I did the simplest thing here, removed the unused options param. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:590: if (!_isAudioOnly) { On 2015/09/14 08:31:20, henrika_webrtc wrote: > Not sure what type of logging support we have but it would be nice with a log > here stating that the video track is NULL but it is not an error. it doesn't look like we do much logging at all in AppRTCDemo. Might be a useful thing to improve at some point. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDAppEngineClient.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDAppEngineClient.m:26: @"https://apprtc.appspot.com/join/%@?debug=loopback"; On 2015/09/11 17:53:53, tkchin_webrtc wrote: > I don't recall exactly, but do you actually need this? What does the server do > differently if that parameter is specified again? Yes, we need it. The server will add a special loopback user to the room. If we don't do that, the messages never get delivered because the room only has 1 participant. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.h (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.h:35: // another client. On 2015/09/11 17:53:53, tkchin_webrtc wrote: > Can you elaborate on what it actually does? Loopback will likely confuse people > who aren't already familiar with signaling. Done. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ARDWebSocketChannel.m:230: [self sendMessage:answer]; On 2015/09/11 17:53:53, tkchin_webrtc wrote: > Interesting. I was thinking you could call peer connection methods directly > instead of introducing another round trip but this works. That would work as well, I just followed the js/web model to keep them similar. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m (right): https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:153: _audioOnlyLabel = [[UILabel alloc] initWithFrame:CGRectZero]; On 2015/09/11 17:53:53, tkchin_webrtc wrote: > I know it's a demo app, but can we make this prettier? ;-) > Made a couple changes to help the new controls fit in better, if you have other suggestions I'm happy to make changes. https://codereview.webrtc.org/1334003002/diff/80001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:185: On 2015/09/11 17:53:53, tkchin_webrtc wrote: > nit: fix indentation + remove unneeded line breaks Done.
LGTM++ ;-)
Anything else you want to see @tkchin?
@tkchin PTAL
some ui nits. but lgtm! https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:624: [_loopbackChannel registerForRoomId:_roomId clientId:@"LOOPBACk_CLIENT_ID"]; all caps missing k https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m (right): https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:189: _startCallButton = [[UIButton alloc] initWithFrame:CGRectZero]; nits: make it look like a button: _startCallButton = [UIButton buttonWithType:UIButtonTypeSystem]; _startCallButton.backgroundColor = [UIColor blueColor]; _startCallButton.layer.cornerRadius = 10; _startCallButton.clipsToBounds = YES; _startCallButton.contentEdgeInsets = UIEdgeInsetsMake(5, 10, 5, 10); [_startCallButton setTitle:@"Start" forState:UIControlStateNormal]; _startCallButton.titleLabel.font = controlFont; [_startCallButton setTitleColor:[UIColor whiteColor] forState:UIControlStateNormal]; [_startCallButton setTitleColor:[UIColor lightGrayColor] forState:UIControlStateSelected]; https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:226: audioOnlyTop, align args https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:249: _startCallButton.frame = CGRectMake(kCallControlMargin * 4, nit: align button to left, and align args. _startCallButton.frame = CGRectMake(kCallControlMargin, startCallTop, _startCallButton.frame.size.width, _startCallButton.frame.size.height);
https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:624: [_loopbackChannel registerForRoomId:_roomId clientId:@"LOOPBACk_CLIENT_ID"]; On 2015/10/01 20:11:01, tkchin_webrtc wrote: > all caps missing k Done. https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m (right): https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:189: _startCallButton = [[UIButton alloc] initWithFrame:CGRectZero]; On 2015/10/01 20:11:01, tkchin_webrtc wrote: > nits: make it look like a button: > > _startCallButton = [UIButton buttonWithType:UIButtonTypeSystem]; > _startCallButton.backgroundColor = [UIColor blueColor]; > _startCallButton.layer.cornerRadius = 10; > _startCallButton.clipsToBounds = YES; > _startCallButton.contentEdgeInsets = UIEdgeInsetsMake(5, 10, 5, 10); > [_startCallButton setTitle:@"Start" > forState:UIControlStateNormal]; > _startCallButton.titleLabel.font = controlFont; > [_startCallButton setTitleColor:[UIColor whiteColor] > forState:UIControlStateNormal]; > [_startCallButton setTitleColor:[UIColor lightGrayColor] > forState:UIControlStateSelected]; Done. https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:226: audioOnlyTop, On 2015/10/01 20:11:01, tkchin_webrtc wrote: > align args Done. https://codereview.webrtc.org/1334003002/diff/120001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:249: _startCallButton.frame = CGRectMake(kCallControlMargin * 4, On 2015/10/01 20:11:01, tkchin_webrtc wrote: > nit: align button to left, and align args. > > _startCallButton.frame = CGRectMake(kCallControlMargin, > startCallTop, > _startCallButton.frame.size.width, > _startCallButton.frame.size.height); Done.
The CQ bit was checked by haysc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1334003002/#ps140001 (title: "Revisions for CR feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334003002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334003002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/4714)
The CQ bit was checked by haysc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org, henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/1334003002/#ps160001 (title: "Fixing test compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334003002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334003002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/9892)
The CQ bit was checked by haysc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org, henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/1334003002/#ps180001 (title: "Fixing mac build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334003002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334003002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by haysc@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334003002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334003002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/913e645e10bd3a060add29ac634a7d28f82744f6 Cr-Commit-Position: refs/heads/master@{#10153} |
