Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2253013006:
Adding AecDump functionality to AppRTCDemo for iOS (Closed)
|
Created:
4 years, 4 months ago by peah-webrtc Modified:
4 years, 3 months ago Reviewers:
tkchin_webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), kwiberg-webrtc, Andrew MacDonald, hlundin-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding AecDump functionality to AppRTCDemo for iOS
BUG=webrtc:6229
Committed: https://crrev.com/5085b0ca94c96ebf85fa2631e8f1facdf07d56da
Cr-Commit-Position: refs/heads/master@{#13927}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 27
Patch Set 3 : Rebase #Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Hardcoded (for now) aecdump functionality to be off for the mac AppRTC demo client #Patch Set 6 : Fixed failing test #
Total comments: 12
Patch Set 7 : Changes in response to reviewer comments #Messages
Total messages: 43 (28 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== Adding AecDump functionality to AppRTCDemo for iOS BUG= ========== to ========== Adding AecDump functionality to AppRTCDemo for iOS BUG=webrtc:6229 ==========
peah@webrtc.org changed reviewers: + tkchin@webrtc.org
https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:67: // If |makeAecDump| is true, an aecdump will be created for the call. nit: shouldMakeAecDump (avoid verbs in argument names) here and elsewhere https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:576: if (![_factory startAecDump: filePath]) { nit: no space after : https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:33: NSAssert(result, @"Failed to start network thread."); These should probably be updated to return nil if creation fails. Don't have to do it in this CL though. https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:55: NSAssert(_nativeFactory, @"Failed to create the aecdump file!"); Why is there an assert on the factory here? Should this just be a log? https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:56: return false; BOOL is YES or NO. return NO; https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: - (BOOL) startAecDump:(NSString *)filename; These are part of public APIs. Is this something we want to expose for everyone, or do we want to make it more private? The RTCEventLog functions were added to peerconnection directly, should this follow the same example so all the logging functions can be in one place? nit: - (BOOL)startAecDumpWithFilePath:(NSString *)filePath; no max size?
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:67: // If |makeAecDump| is true, an aecdump will be created for the call. On 2016/08/19 17:31:42, tkchin_webrtc wrote: > nit: shouldMakeAecDump (avoid verbs in argument names) > here and elsewhere Done. https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:576: if (![_factory startAecDump: filePath]) { On 2016/08/19 17:31:42, tkchin_webrtc wrote: > nit: no space after : Done. https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:33: NSAssert(result, @"Failed to start network thread."); On 2016/08/19 17:31:42, tkchin_webrtc wrote: > These should probably be updated to return nil if creation fails. Don't have to > do it in this CL though. Acknowledged. https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:55: NSAssert(_nativeFactory, @"Failed to create the aecdump file!"); On 2016/08/19 17:31:42, tkchin_webrtc wrote: > Why is there an assert on the factory here? Should this just be a log? That would definitely make sense. I moved this code to ARDAppClient.m and replaced the assert with a log. Done. https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:56: return false; On 2016/08/19 17:31:42, tkchin_webrtc wrote: > BOOL is YES or NO. > > return NO; Done. https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h (right): https://codereview.webrtc.org/2253013006/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: - (BOOL) startAecDump:(NSString *)filename; On 2016/08/19 17:31:42, tkchin_webrtc wrote: > These are part of public APIs. Is this something we want to expose for everyone, > or do we want to make it more private? > > The RTCEventLog functions were added to peerconnection directly, should this > follow the same example so all the logging functions can be in one place? > > nit: > - (BOOL)startAecDumpWithFilePath:(NSString *)filePath; > > no max size? I changed this to follow what is done in the Android counterpart to this API (webrtc/api/android/java/src/org/webrtc/PeerConnectionFactory.java), i.e., The method takes a file descriptor and a limit, and passes this to the native code. I change the max size to be a parameter, but for the app I will pass -1 to specify that there should be no limit in size for the recording.
I would prefer to not have to pass in a fd and instead pass in a file path. fd is too low level for most ObjC users. startAecDumpWithFilePath:maxFileSizeInBytes would be the most ideal. And it would be good to move the C++ API to PeerConnection instead of PeerConnectionFactory in the future. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:71: shouldMakeAecDump:(BOOL)shouldMakeAecDump; nit: align : https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:133: nit: remove extra line https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:226: shouldMakeAecDump:(BOOL)shouldMakeAecDump { nit: align : https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:316: if (_shouldMakeAecDump) { is this conditional useful? Could be that it failed to start. are multiple calls to start idempotent? https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:579: } nit: else on same line as } https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:581: if (![_factory startAecDump:fd fileSizeLimitBytes:-1]) { nit: remove extra space after fd https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h:21: shouldMakeAecDump:(BOOL)shouldMakeAecDump nit: align : in cases like these where aligning will cause args to be less than 4 from leftmost margin, indent the longest arg name by 4 and align : to that instead (just like how useManualAudio was aligned because it was too long) https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:60: shouldMakeAecDump:(BOOL)shouldMakeAecDump nit: align https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:100: shouldMakeAecDump:shouldMakeAecDump nit: align https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h:27: shouldMakeAecDump:(BOOL)shouldMakeAecDump nit: align https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.m (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.m:42: shouldMakeAecDump:(BOOL)shouldMakeAecDump ditto. here and rest of review https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:52: - (BOOL) startAecDump:(int)fileDescriptor nit: remove space after (BOOL) https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:53: fileSizeLimitBytes:(int)fileSizeLimitBytes { nit: objC naming conventions startAecDumpWithFileDescriptor:maxFileSizeInBytes https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: - (BOOL) startAecDump:(int)fileDescriptor nit: no space after (BOOL)
On 2016/08/24 00:42:20, tkchin_webrtc wrote: > I would prefer to not have to pass in a fd and instead pass in a file path. fd > is too low level for most ObjC users. > > startAecDumpWithFilePath:maxFileSizeInBytes > > would be the most ideal. And it would be good to move the C++ API to > PeerConnection instead of PeerConnectionFactory in the future. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:71: > shouldMakeAecDump:(BOOL)shouldMakeAecDump; > nit: align : > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:133: > nit: remove extra line > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:226: > shouldMakeAecDump:(BOOL)shouldMakeAecDump { > nit: align : > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:316: if (_shouldMakeAecDump) { > is this conditional useful? Could be that it failed to start. > are multiple calls to start idempotent? > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:579: } > nit: else on same line as } > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:581: if (![_factory > startAecDump:fd fileSizeLimitBytes:-1]) { > nit: remove extra space after fd > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h:21: > shouldMakeAecDump:(BOOL)shouldMakeAecDump > nit: align : > in cases like these where aligning will cause args to be less than 4 from > leftmost margin, indent the longest arg name by 4 and align : to that instead > (just like how useManualAudio was aligned because it was too long) > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:60: > shouldMakeAecDump:(BOOL)shouldMakeAecDump > nit: align > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:100: > shouldMakeAecDump:shouldMakeAecDump > nit: align > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h:27: > shouldMakeAecDump:(BOOL)shouldMakeAecDump > nit: align > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.m (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.m:42: > shouldMakeAecDump:(BOOL)shouldMakeAecDump > ditto. here and rest of review > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:52: - (BOOL) > startAecDump:(int)fileDescriptor > nit: remove space after (BOOL) > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:53: > fileSizeLimitBytes:(int)fileSizeLimitBytes { > nit: objC naming conventions > > startAecDumpWithFileDescriptor:maxFileSizeInBytes > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h > (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: - (BOOL) > startAecDump:(int)fileDescriptor > nit: no space after (BOOL) Thanks for the review! I think you definitely have a point with the file path as the actual opening of the file is anyway done at a lower level, and inside native code. As it is now, however, the C++ API in peerconnectionfactory.h does only have a startaecdump call that uses the filedescriptor and not any call that uses a filename. Therefore I implemented the call here to use that api call, which then also makes the objective C sdk call to match the corresponding Java api in PeerConnectionFactory.java. I don't know the reasoning/background about the peerconnection and peerconnectionfactory, and do not have a real opinion about whether to merge these but it sounds sensible to do so. I guess, however, that doing so is a bit tricky since both are part of the webrtc api. I think the most important part is, however, to harmonize the Java and Objective C APIs for webrtc.
Thanks for the review. I've updated the CL according to your comments. Please note that I needed to do a rebase to the latest master as well. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:71: shouldMakeAecDump:(BOOL)shouldMakeAecDump; On 2016/08/24 00:42:19, tkchin_webrtc wrote: > nit: align : Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:133: On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: remove extra line Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:226: shouldMakeAecDump:(BOOL)shouldMakeAecDump { On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: align : Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:316: if (_shouldMakeAecDump) { On 2016/08/24 00:42:20, tkchin_webrtc wrote: > is this conditional useful? Could be that it failed to start. > are multiple calls to start idempotent? The StopAecDump method does an internal check to see whether an aecdump is being done or not, so the conditional is not needed. I think it does, however, make sense to not rely on that fact in this code as it not really part of the documented api. I added a bool that keeps track inside this class of whether an aecdump recording has been successfully started. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:579: } On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: else on same line as } Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:581: if (![_factory startAecDump:fd fileSizeLimitBytes:-1]) { On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: remove extra space after fd Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h:21: shouldMakeAecDump:(BOOL)shouldMakeAecDump On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: align : > in cases like these where aligning will cause args to be less than 4 from > leftmost margin, indent the longest arg name by 4 and align : to that instead > (just like how useManualAudio was aligned because it was too long) Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:60: shouldMakeAecDump:(BOOL)shouldMakeAecDump On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: align Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:100: shouldMakeAecDump:shouldMakeAecDump On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: align Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h:27: shouldMakeAecDump:(BOOL)shouldMakeAecDump On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: align Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:52: - (BOOL) startAecDump:(int)fileDescriptor On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: remove space after (BOOL) Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:53: fileSizeLimitBytes:(int)fileSizeLimitBytes { On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: objC naming conventions > > startAecDumpWithFileDescriptor:maxFileSizeInBytes Done. https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h (right): https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: - (BOOL) startAecDump:(int)fileDescriptor On 2016/08/24 00:42:20, tkchin_webrtc wrote: > nit: no space after (BOOL) Done.
Patchset #6 (id:300001) has been deleted
Patchset #5 (id:280001) has been deleted
The CQ bit was checked by peah@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: Try jobs failed on following builders: ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/9879)
The CQ bit was checked by peah@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.
On 2016/08/25 09:25:08, peah-webrtc wrote: > Thanks for the review. I've updated the CL according to your comments. > > Please note that I needed to do a rebase to the latest master as well. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ARDAppClient.h (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.h:71: > shouldMakeAecDump:(BOOL)shouldMakeAecDump; > On 2016/08/24 00:42:19, tkchin_webrtc wrote: > > nit: align : > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:133: > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: remove extra line > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:226: > shouldMakeAecDump:(BOOL)shouldMakeAecDump { > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: align : > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:316: if (_shouldMakeAecDump) { > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > is this conditional useful? Could be that it failed to start. > > are multiple calls to start idempotent? > > The StopAecDump method does an internal check to see whether an aecdump is being > done or not, so the conditional is not needed. > > I think it does, however, make sense to not rely on that fact in this code as it > not really part of the documented api. > > I added a bool that keeps track inside this class of whether an aecdump > recording has been successfully started. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:579: } > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: else on same line as } > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:581: if (![_factory > startAecDump:fd fileSizeLimitBytes:-1]) { > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: remove extra space after fd > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.h:21: > shouldMakeAecDump:(BOOL)shouldMakeAecDump > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: align : > > in cases like these where aligning will cause args to be less than 4 from > > leftmost margin, indent the longest arg name by 4 and align : to that instead > > (just like how useManualAudio was aligned because it was too long) > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:60: > shouldMakeAecDump:(BOOL)shouldMakeAecDump > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: align > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDMainViewController.m:100: > shouldMakeAecDump:shouldMakeAecDump > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: align > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > File webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/examples/objc/App... > webrtc/examples/objc/AppRTCDemo/ios/ARDVideoCallViewController.h:27: > shouldMakeAecDump:(BOOL)shouldMakeAecDump > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: align > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:52: - (BOOL) > startAecDump:(int)fileDescriptor > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: remove space after (BOOL) > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnectionFactory.mm:53: > fileSizeLimitBytes:(int)fileSizeLimitBytes { > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: objC naming conventions > > > > startAecDumpWithFileDescriptor:maxFileSizeInBytes > > Done. > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h > (right): > > https://codereview.webrtc.org/2253013006/diff/220001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: - (BOOL) > startAecDump:(int)fileDescriptor > On 2016/08/24 00:42:20, tkchin_webrtc wrote: > > nit: no space after (BOOL) > > Done. lgtm to unblock, but please add comment that API may change. These set of public headers should really not change so much because it will break clients when we do change it. I don't think ObjC needs to follow the same Java convention. In my opinion it makes using our APIs harder. There are pretty much no iOS APIs for interacting with files that take fds directly in the Foundation classes. e.g. NSData. Each language specific wrapper should behave in a way that is consistent with the language and the WebRTC spec, not with the convention of the underlying C++ implementation. With Java, I don't think there is a choice because there is no way to pass Java native file objects into JNI/C++ other than through a fd. That was likely the motivation behind using a fd in C++ API.
Furthermore, this aecdump API does not harmonize with the startRtcEventLog API, which is of a similar nature. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:177: _isAecDumpActive = NO; nit: not needed, BOOL ivars in ObjC are init-ed to NO by default. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:586: _isAecDumpActive = NO; nit: align https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m (right): https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:293: aecdumpModeTop, nit: align https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:300: CGRectGetMidY(aecdumpModeRect)); nit: align https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:361: isLoopback:_loopbackSwitch.isOn nit: align ":" like before
https://codereview.webrtc.org/2253013006/diff/340001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h (right): https://codereview.webrtc.org/2253013006/diff/340001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: /** Start an aecdump recording with a file descriptor and nit: place this after the rest of the methods.
Thanks for the review! The API issue sounds like an important and good discussion to have. Lets pick that up as soon as possible. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right): https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:177: _isAecDumpActive = NO; On 2016/08/25 17:22:57, tkchin_webrtc wrote: > nit: not needed, BOOL ivars in ObjC are init-ed to NO by default. Done. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:586: _isAecDumpActive = NO; On 2016/08/25 17:22:57, tkchin_webrtc wrote: > nit: align Done. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... File webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m (right): https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:293: aecdumpModeTop, On 2016/08/25 17:22:57, tkchin_webrtc wrote: > nit: align Done. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:300: CGRectGetMidY(aecdumpModeRect)); On 2016/08/25 17:22:57, tkchin_webrtc wrote: > nit: align Done. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/examples/objc/App... webrtc/examples/objc/AppRTCDemo/ios/ARDMainView.m:361: isLoopback:_loopbackSwitch.isOn On 2016/08/25 17:22:57, tkchin_webrtc wrote: > nit: align ":" like before Done. https://codereview.webrtc.org/2253013006/diff/340001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h (right): https://codereview.webrtc.org/2253013006/diff/340001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCPeerConnectionFactory.h:32: /** Start an aecdump recording with a file descriptor and On 2016/08/25 17:25:40, tkchin_webrtc wrote: > nit: place this after the rest of the methods. Done.
The CQ bit was checked by peah@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/2253013006/#ps360001 (title: "Changes in response to reviewer comments")
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
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Adding AecDump functionality to AppRTCDemo for iOS BUG=webrtc:6229 ========== to ========== Adding AecDump functionality to AppRTCDemo for iOS BUG=webrtc:6229 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Adding AecDump functionality to AppRTCDemo for iOS BUG=webrtc:6229 ========== to ========== Adding AecDump functionality to AppRTCDemo for iOS BUG=webrtc:6229 Committed: https://crrev.com/5085b0ca94c96ebf85fa2631e8f1facdf07d56da Cr-Commit-Position: refs/heads/master@{#13927} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5085b0ca94c96ebf85fa2631e8f1facdf07d56da Cr-Commit-Position: refs/heads/master@{#13927} |
