Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488973002:
Split avfoundationcapturer classes in separate files. (Closed)
|
Created:
4 years, 1 month ago by daniela-webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSplit avfoundationcapturer classes in separate files.
Increases readibility and helps with the formating.
BUG=webrtc:6680
Committed: https://crrev.com/71caaca2544963a446e189e37beec5111d918177
Cr-Commit-Position: refs/heads/master@{#15206}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Decrease the similarity to 10% #
Total comments: 10
Patch Set 3 : Fix formatting #Patch Set 4 : Remove static function from global namespace and add it to cpp class #
Total comments: 2
Patch Set 5 : Add wrapper class for static methosd #
Total comments: 6
Patch Set 6 : Renaming of classes and methods #
Total comments: 8
Patch Set 7 : Fix comments. #
Total comments: 7
Patch Set 8 : Format #
Total comments: 15
Patch Set 9 : Replace objc static methods with c externs #Patch Set 10 : Replace objc++ static methods with c++ externs. #
Total comments: 2
Patch Set 11 : Alphabetize imports #
Total comments: 2
Messages
Total messages: 38 (6 generated)
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org
First CL towards the "split the capture classes" goal. For simplifying the review I've only plucked out the RTCAVFoundationVideoCapturerInternal class and dumped it into separate file with minimum alteration in import headers and public interfaces (other than what was needed to compile). Once this get's merged * I'll clean up the ObjC file a bit as I've noticed some unused imports and some unused ivars * Introduce separate helper class for all the AVDeviceVideoFormat<->cricket::VideoFormat transitions and add proper tests for them to replace the static methods that have piled up.
A few comments: https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:21: @interface RTCAVFoundationVideoCapturerInternal Don't we prefer classname+private.h over classnameInternal? Or are these different concepts? https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:128: return _backCameraInput != nil; doesn't this have to be @synchronized as well? https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:178: }]; This indentation is off. Should be 4 spaces according to https://google.github.io/styleguide/objcguide.xml#Blocks https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:197: }]; Ditto indentation. https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:279: }]; ditto indentation. https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:30: // Perhaps adding a category on AVCaptureDevice would be better. Update TODO, since this file is now split :) https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:122: // Perhaps adding a category on AVCaptureDevice would be better. Update TODO, since this file is now split :) https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:124: AVCaptureSession* session, Change this back, star should be on the right for consistency. Source: https://google.github.io/styleguide/objcguide.xml#Example
https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. Try 'git cl upload --similarity=10' to get the diff working.
On 2016/11/10 10:32:52, magjed_webrtc wrote: > https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... > File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm > (right): > > https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:2: * > Copyright 2016 The WebRTC project authors. All Rights Reserved. > Try 'git cl upload --similarity=10' to get the diff working. Can you check it out now?
Looks good overall, but indentation is incorrect in a couple of places. https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:174: [[UIDevice currentDevice] beginGeneratingDeviceOrientationNotifications]; This line is too long now. Is this how it should be indented? https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:234: && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_9_0 I don't think this indentation is right? https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:236: notification.userInfo[AVCaptureSessionInterruptionReasonKey]; need to indent 4 spaces https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:265: [notification.userInfo objectForKey:AVCaptureSessionErrorKey]; ditto https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:31: BOOL SetFormatForCaptureDevice(AVCaptureDevice* device, You are not allowed to add stuff to the global namespace, so I would make this a static function in AVFoundationVideoCapturer if you want to test it.
https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:21: @interface RTCAVFoundationVideoCapturerInternal On 2016/11/10 09:28:10, kthelgason wrote: > Don't we prefer classname+private.h over classnameInternal? Or are these > different concepts? I think they are different concepts. Class+Private.h is used for adding extension to certain class (slightly different concept than extension). From Apple docs: "Class extensions are often used to extend the public interface with additional private methods or properties for use within the implementation of the class itself." The most common case of class extension in the form of Class+Private I've seen are for testing purposes. This class however I believe it was suffixed Internal, to indicate that it's not part of the public headers and that it's meant for internal use within the framework only. https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:128: return _backCameraInput != nil; On 2016/11/10 09:28:10, kthelgason wrote: > doesn't this have to be @synchronized as well? Not entirely sure :/ seems like it does not need to be, as the _backCameraInput assignment has no thread guards either. I need some more time to inspect the code and understand better. It either is perfectly thread safe or we've had potentially dangerous code all this time :| https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:178: }]; On 2016/11/10 09:28:10, kthelgason wrote: > This indentation is off. Should be 4 spaces according to > https://google.github.io/styleguide/objcguide.xml#Blocks Done. https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:197: }]; On 2016/11/10 09:28:10, kthelgason wrote: > Ditto indentation. Done. https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:279: }]; On 2016/11/10 09:28:10, kthelgason wrote: > ditto indentation. Done. https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:174: [[UIDevice currentDevice] beginGeneratingDeviceOrientationNotifications]; On 2016/11/10 13:22:45, magjed_webrtc wrote: > This line is too long now. Is this how it should be indented? Done. https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:234: && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_9_0 On 2016/11/10 13:22:45, magjed_webrtc wrote: > I don't think this indentation is right? Done. https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:236: notification.userInfo[AVCaptureSessionInterruptionReasonKey]; On 2016/11/10 13:22:45, magjed_webrtc wrote: > need to indent 4 spaces Done. https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:265: [notification.userInfo objectForKey:AVCaptureSessionErrorKey]; On 2016/11/10 13:22:45, magjed_webrtc wrote: > ditto Done.
https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:122: // Perhaps adding a category on AVCaptureDevice would be better. On 2016/11/10 09:28:10, kthelgason wrote: > Update TODO, since this file is now split :) Done. https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:124: AVCaptureSession* session, On 2016/11/10 09:28:10, kthelgason wrote: > Change this back, star should be on the right for consistency. Source: > https://google.github.io/styleguide/objcguide.xml#Example Done. https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:31: BOOL SetFormatForCaptureDevice(AVCaptureDevice* device, On 2016/11/10 13:22:45, magjed_webrtc wrote: > You are not allowed to add stuff to the global namespace, so I would make this a > static function in AVFoundationVideoCapturer if you want to test it. Done.
On 2016/11/11 13:23:10, denicija-webrtc wrote: > https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:122: // Perhaps > adding a category on AVCaptureDevice would be better. > On 2016/11/10 09:28:10, kthelgason wrote: > > Update TODO, since this file is now split :) > > Done. > > https://codereview.webrtc.org/2488973002/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:124: > AVCaptureSession* session, > On 2016/11/10 09:28:10, kthelgason wrote: > > Change this back, star should be on the right for consistency. Source: > > https://google.github.io/styleguide/objcguide.xml#Example > > Done. > > https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): > > https://codereview.webrtc.org/2488973002/diff/20001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:31: BOOL > SetFormatForCaptureDevice(AVCaptureDevice* device, > On 2016/11/10 13:22:45, magjed_webrtc wrote: > > You are not allowed to add stuff to the global namespace, so I would make this > a > > static function in AVFoundationVideoCapturer if you want to test it. > > Done. The block indentation is still off, I think we should definitely fix it before landing. It messes up the diff and will create noise in the git history as well. I don't have any comments apart from that.
Should avfoundationvideocapturer.mm be renamed to avfoundationvideocapturer.cc now? https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:491: _capturer->SetFormatForCaptureDevice(newDevice, _captureSession, *format); If SetFormatForCaptureDevice is a static method, you should call it like webrtc::AVFoundationVideoCapturer::SetFormatForCaptureDevice. Also, this line is too long. https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:62: // depending This function should be static. On a second look, it looks like it would be better to move this function into RTCAVFoundationVideoCapturerInternal like: - (void)setFormat:(cricket::VideoFormat format)format; and call that function from AVFoundationVideoCapturer::Start.
On 2016/11/14 13:48:52, magjed_webrtc wrote: > Should avfoundationvideocapturer.mm be renamed to avfoundationvideocapturer.cc > now? > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm > (right): > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:491: > _capturer->SetFormatForCaptureDevice(newDevice, _captureSession, *format); > If SetFormatForCaptureDevice is a static method, you should call it like > webrtc::AVFoundationVideoCapturer::SetFormatForCaptureDevice. Also, this line is > too long. > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:62: // depending > This function should be static. On a second look, it looks like it would be > better to move this function into RTCAVFoundationVideoCapturerInternal like: > - (void)setFormat:(cricket::VideoFormat format)format; > and call that function from AVFoundationVideoCapturer::Start. I agree that this method semantically would be better elsewhere. If I move it away from AVFoundationVideoCapturer I'd need to move all other static methods currently present in this file (that was my intention once this CL lands). Should I go ahead and add all static methods to RTCAVFoundationVideoCapturerInternal in this CL? Or, since that file is quite big, maybe it's better to put them in separate thin wrapper class RTCVideoCapturerFormatMapper Then this class would have two public static methods + (std::set<cricket::VideoFormat>)getSupportedVideoFormatsForDevice:(AVCaptureDevice *)device; + (BOOL)setFormatForCaptureDevice:(AVCaptureDevice *)device captureSession:(AVCaptureSession *)session format:(const cricket::VideoFormat)format; The implementation would contain the rest of the static methods dealing with the whole AVCaptureDevice->cricket::VideoFormat logic. What do you think?
On 2016/11/14 15:22:43, denicija-webrtc wrote: > On 2016/11/14 13:48:52, magjed_webrtc wrote: > > Should avfoundationvideocapturer.mm be renamed to avfoundationvideocapturer.cc > > now? > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm > > (right): > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:491: > > _capturer->SetFormatForCaptureDevice(newDevice, _captureSession, *format); > > If SetFormatForCaptureDevice is a static method, you should call it like > > webrtc::AVFoundationVideoCapturer::SetFormatForCaptureDevice. Also, this line > is > > too long. > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:62: // depending > > This function should be static. On a second look, it looks like it would be > > better to move this function into RTCAVFoundationVideoCapturerInternal like: > > - (void)setFormat:(cricket::VideoFormat format)format; > > and call that function from AVFoundationVideoCapturer::Start. > > I agree that this method semantically would be better elsewhere. If I move it > away from AVFoundationVideoCapturer I'd need to move all other static methods > currently present in this file (that was my intention once this CL lands). > Should I go ahead and add all static methods to > RTCAVFoundationVideoCapturerInternal in this CL? > Or, since that file is quite big, maybe it's better to put them in separate thin > wrapper class > RTCVideoCapturerFormatMapper > Then this class would have two public static methods > > + > (std::set<cricket::VideoFormat>)getSupportedVideoFormatsForDevice:(AVCaptureDevice > *)device; > + (BOOL)setFormatForCaptureDevice:(AVCaptureDevice *)device > captureSession:(AVCaptureSession *)session > format:(const cricket::VideoFormat)format; > > The implementation would contain the rest of the static methods dealing with the > whole AVCaptureDevice->cricket::VideoFormat logic. > > What do you think? Yeah, I like the approach of putting these functions in a separate file. Then we can easily write unittests for it as well. Should the separate file be an ObjC file or C++ file? If C++, you don't need a class and can just make them functions inside the webrtc namespace.
On 2016/11/14 16:04:22, magjed_webrtc wrote: > On 2016/11/14 15:22:43, denicija-webrtc wrote: > > On 2016/11/14 13:48:52, magjed_webrtc wrote: > > > Should avfoundationvideocapturer.mm be renamed to > avfoundationvideocapturer.cc > > > now? > > > > > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > > File > webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm > > > (right): > > > > > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > > > webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:491: > > > _capturer->SetFormatForCaptureDevice(newDevice, _captureSession, *format); > > > If SetFormatForCaptureDevice is a static method, you should call it like > > > webrtc::AVFoundationVideoCapturer::SetFormatForCaptureDevice. Also, this > line > > is > > > too long. > > > > > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): > > > > > > > > > https://codereview.webrtc.org/2488973002/diff/60001/webrtc/sdk/objc/Framework... > > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:62: // > depending > > > This function should be static. On a second look, it looks like it would be > > > better to move this function into RTCAVFoundationVideoCapturerInternal like: > > > - (void)setFormat:(cricket::VideoFormat format)format; > > > and call that function from AVFoundationVideoCapturer::Start. > > > > I agree that this method semantically would be better elsewhere. If I move it > > away from AVFoundationVideoCapturer I'd need to move all other static methods > > currently present in this file (that was my intention once this CL lands). > > Should I go ahead and add all static methods to > > RTCAVFoundationVideoCapturerInternal in this CL? > > Or, since that file is quite big, maybe it's better to put them in separate > thin > > wrapper class > > RTCVideoCapturerFormatMapper > > Then this class would have two public static methods > > > > + > > > (std::set<cricket::VideoFormat>)getSupportedVideoFormatsForDevice:(AVCaptureDevice > > *)device; > > + (BOOL)setFormatForCaptureDevice:(AVCaptureDevice *)device > > captureSession:(AVCaptureSession *)session > > format:(const cricket::VideoFormat)format; > > > > The implementation would contain the rest of the static methods dealing with > the > > whole AVCaptureDevice->cricket::VideoFormat logic. > > > > What do you think? > > Yeah, I like the approach of putting these functions in a separate file. Then we > can easily write unittests for it as well. Should the separate file be an ObjC > file or C++ file? If C++, you don't need a class and can just make them > functions inside the webrtc namespace. I'd personally prefer ObjC if it makes no difference :)
On 2016/11/17 08:40:12, denicija-webrtc wrote: > On 2016/11/14 16:04:22, magjed_webrtc wrote: > > Yeah, I like the approach of putting these functions in a separate file. Then > > we > > can easily write unittests for it as well. Should the separate file be an ObjC > > file or C++ file? If C++, you don't need a class and can just make them > > functions inside the webrtc namespace. > > I'd personally prefer ObjC if it makes no difference :) Ok, let's go with ObjC :)
I've split out the static methods into a separate class (RTCVideoFormatMapper). I'll add unit tests for that class once this gets merged. Please take another look and let me know what you think.
https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.h:1: /* I think the name of this class too generic for what it does, I would like to include 'camera' or 'AVFoundation'. Maybe RTCAVFoundationFormatMapper? https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.mm (left): https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.mm:49: static NSArray<AVCaptureDeviceFormat *> *GetEligibleDeviceFormats( Please keep the same order of the functions so the diff will be smaller.
lgtm % small nit. Feel free to ignore https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.h:52: format:(const cricket::VideoFormat)format; suggestion: rename to setFormat:(format)forCaptureDevice:(device)captureSession:(session)
https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.h:1: /* On 2016/11/17 14:11:31, magjed_webrtc wrote: > I think the name of this class too generic for what it does, I would like to > include 'camera' or 'AVFoundation'. Maybe RTCAVFoundationFormatMapper? Done. https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.h:52: format:(const cricket::VideoFormat)format; On 2016/11/21 08:38:14, kthelgason wrote: > suggestion: rename to > setFormat:(format)forCaptureDevice:(device)captureSession:(session) Done. https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.mm (left): https://codereview.webrtc.org/2488973002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoFormatMapper.mm:49: static NSArray<AVCaptureDeviceFormat *> *GetEligibleDeviceFormats( On 2016/11/17 14:11:31, magjed_webrtc wrote: > Please keep the same order of the functions so the diff will be smaller. Done.
https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h:23: * Maps the list of eligible AVCaptureDeviceFormat to cricket::VideoFormat. nit: I think we should move some of these comments to the implementation in RTCAVFoundationFormatMapper.mm. For example, we don't need to mention AVCaptureDeviceFormat in the public function comments since they are not part of the interface and having to deal with that type is what we encapsulate and abstract away with this class. I think we can make the comments here more concise if we focus only on what the caller has to care about. Something like this: "Returns the set of eligible formats for the given device as cricket::VideoFormats. The returned set is a filtered subset of all the formats from the provided device. The filtering is based on pixel color format and framerate (for performance reasons). ... + (std::set<cricket::VideoFormat>)supportedVideoFormatsForDevice:(AVCaptureDevice *)device;" "Set new capture format for the provided session and device. The provided format must be one of the formats returned from supportedVideoFormatsForDevice. ... + (BOOL)setFormat:(const cricket::VideoFormat)format ... " https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h (right): https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:17: // This class used to capture frames using AVFoundation APIs on iOS. It is meant nit: We still use this class to capture frames using AVFoundation, so the first comment is a bit misleading. Maybe change it to something like: "This class is an implementation detail of AVFoundationVideoCapturer and handles the ObjC integration with the AVFoundation APIs." https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:378: // currently supported on iPhone / iPad. Please add a TODO here so we remember to investigate if we can remove this color conversion. TODO(denicija/magjed): Remove this color conversion and use the original capture format directly. https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:492: const cricket::VideoFormat *format = _capturer->GetCaptureFormat(); nit: Too long line.
https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h:23: * Maps the list of eligible AVCaptureDeviceFormat to cricket::VideoFormat. On 2016/11/21 10:59:46, magjed_webrtc wrote: > nit: I think we should move some of these comments to the implementation in > RTCAVFoundationFormatMapper.mm. For example, we don't need to mention > AVCaptureDeviceFormat in the public function comments since they are not part of > the interface and having to deal with that type is what we encapsulate and > abstract away with this class. I think we can make the comments here more > concise if we focus only on what the caller has to care about. Something like > this: > > "Returns the set of eligible formats for the given device as > cricket::VideoFormats. > > The returned set is a filtered subset of all the formats from the provided > device. The filtering is > based on pixel color format and framerate (for performance reasons). > ... > + > (std::set<cricket::VideoFormat>)supportedVideoFormatsForDevice:(AVCaptureDevice > *)device;" > > > "Set new capture format for the provided session and device. > > The provided format must be one of the formats returned from > supportedVideoFormatsForDevice. > ... > + (BOOL)setFormat:(const cricket::VideoFormat)format > ... > " Done. https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h (right): https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:17: // This class used to capture frames using AVFoundation APIs on iOS. It is meant On 2016/11/21 10:59:46, magjed_webrtc wrote: > nit: We still use this class to capture frames using AVFoundation, so the first > comment is a bit misleading. Maybe change it to something like: > "This class is an implementation detail of AVFoundationVideoCapturer and handles > the ObjC integration with the AVFoundation APIs." Done. https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:378: // currently supported on iPhone / iPad. On 2016/11/21 10:59:46, magjed_webrtc wrote: > Please add a TODO here so we remember to investigate if we can remove this color > conversion. > TODO(denicija/magjed): Remove this color conversion and use the original capture > format directly. Done. https://codereview.webrtc.org/2488973002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:492: const cricket::VideoFormat *format = _capturer->GetCaptureFormat(); On 2016/11/21 10:59:46, magjed_webrtc wrote: > nit: Too long line. Done.
lgtm % nits https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h:36: * supportedVideoFormatsForDevice:. nit: remove colon. https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:382: //TODO(denicija): Remove this color conversion and use the original capture format directly. nit: Add space between // and TODO. https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:494: _capturer->GetCaptureFormat(); nit: The indentation here should be 4 spaces.
denicija@webrtc.org changed reviewers: + tkchin@webrtc.org
https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h:36: * supportedVideoFormatsForDevice:. On 2016/11/21 13:57:52, magjed_webrtc wrote: > nit: remove colon. Done. https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:382: //TODO(denicija): Remove this color conversion and use the original capture format directly. On 2016/11/21 13:57:52, magjed_webrtc wrote: > nit: Add space between // and TODO. Done. https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:494: _capturer->GetCaptureFormat(); On 2016/11/21 13:57:52, magjed_webrtc wrote: > nit: The indentation here should be 4 spaces. All these classes use 2 space indentation throughout. Would it be ok to leave it as it is in the rest of the files?
https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:494: _capturer->GetCaptureFormat(); On 2016/11/21 16:29:50, denicija-webrtc wrote: > On 2016/11/21 13:57:52, magjed_webrtc wrote: > > nit: The indentation here should be 4 spaces. > > All these classes use 2 space indentation throughout. Would it be ok to leave > it as it is in the rest of the files? No, 2 spaces is used for indenting after e.g. an if-statement, but if you have to wrap a line like this, you should use 4 spaces. This is done consistently in all files I'm aware of, including this one. For example, look at line 416 in this file.
https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h:30: + (std::set<cricket::VideoFormat>)supportedVideoFormatsForDevice:(AVCaptureDevice *)device; I don't recommend returning C++ objects in ObjC headers. This forces importers of this header to support C++ despite believing that it is an ObjC file from its name. These methods are both static - you can define C++ externed methods instead. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.mm (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.mm:13: //#import "RTCDispatcher+Private.h" ? https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:14: #include "avfoundationvideocapturer.h" Don't import C++ headers in ObjC header files. Otherwise you force importers to support C++. I know that in this case it doesn't really matter because this class will never be used by an ObjC class. However, I think it's best to stay consistent in the codebase where when we import an ObjC header, we know it can be imported into any ObjC file without converting it to an ObjC++ file (which changes the compiler from clang to clang++ for the importer). Forward declare interfaces that you need instead. In this case you will need some magic by checking #ifdef __cplusplus and add the namespace for C++ imports, but otherwise declare it as a struct. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:24: : NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> NSObject <AV... https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:22: NS_ASSUME_NONNULL_BEGIN Shouldn't need this in implementation files. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:161: [RTCDispatcher restore indentation to previous here and elsewhere
https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.h:30: + (std::set<cricket::VideoFormat>)supportedVideoFormatsForDevice:(AVCaptureDevice *)device; On 2016/11/21 18:51:47, tkchin_webrtc wrote: > I don't recommend returning C++ objects in ObjC headers. This forces importers > of this header to support C++ despite believing that it is an ObjC file from its > name. > > These methods are both static - you can define C++ externed methods instead. Done. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.mm (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationFormatMapper.mm:13: //#import "RTCDispatcher+Private.h" On 2016/11/21 18:51:47, tkchin_webrtc wrote: > ? Done. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:14: #include "avfoundationvideocapturer.h" On 2016/11/21 18:51:47, tkchin_webrtc wrote: > Don't import C++ headers in ObjC header files. Otherwise you force importers to > support C++. > > I know that in this case it doesn't really matter because this class will never > be used by an ObjC class. However, I think it's best to stay consistent in the > codebase where when we import an ObjC header, we know it can be imported into > any ObjC file without converting it to an ObjC++ file (which changes the > compiler from clang to clang++ for the importer). > > Forward declare interfaces that you need instead. > > In this case you will need some magic by checking #ifdef __cplusplus and add the > namespace for C++ imports, but otherwise declare it as a struct. Done. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:14: #include "avfoundationvideocapturer.h" On 2016/11/21 18:51:47, tkchin_webrtc wrote: > Don't import C++ headers in ObjC header files. Otherwise you force importers to > support C++. > > I know that in this case it doesn't really matter because this class will never > be used by an ObjC class. However, I think it's best to stay consistent in the > codebase where when we import an ObjC header, we know it can be imported into > any ObjC file without converting it to an ObjC++ file (which changes the > compiler from clang to clang++ for the importer). > > Forward declare interfaces that you need instead. > > In this case you will need some magic by checking #ifdef __cplusplus and add the > namespace for C++ imports, but otherwise declare it as a struct. That's great tip! Thanks https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.h:24: : NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> On 2016/11/21 18:51:47, tkchin_webrtc wrote: > NSObject <AV... Done. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:22: NS_ASSUME_NONNULL_BEGIN On 2016/11/21 18:51:47, tkchin_webrtc wrote: > Shouldn't need this in implementation files. Done. https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:161: [RTCDispatcher On 2016/11/21 18:51:47, tkchin_webrtc wrote: > restore indentation to previous > here and elsewhere Is this remarks towards decreasing the diff size? I know that it might be a bit difficult for review with all the changes. However, we discussed this a bit with Magnus and Kari (some of it in comments on this CL as well) and we agreed that the previous indentation was not according to the guidelines and that it would be better to fix it in this CL. Do you have objections to leaving it as it is?
lgtm https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:161: [RTCDispatcher On 2016/11/22 11:43:44, denicija-webrtc wrote: > On 2016/11/21 18:51:47, tkchin_webrtc wrote: > > restore indentation to previous > > here and elsewhere > > Is this remarks towards decreasing the diff size? I know that it might be a bit > difficult for review with all the changes. However, we discussed this a bit with > Magnus and Kari (some of it in comments on this CL as well) and we agreed that > the previous indentation was not according to the guidelines and that it would > be better to fix it in this CL. > Do you have objections to leaving it as it is? The previous indentation conforms to indentation used in much of google though. Where is the guideline against this? I won't block CL for it, but I think adding horizontal whitespace like this makes it difficult to read, and difficult to code as well because it will force you to hit 100 char limit much faster. The only reason I can think of that you would want this is if this is the output of the auto-formatter, which we aren't using anyway because of ObjC++. https://codereview.webrtc.org/2488973002/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2488973002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:20: #include "avfoundationformatmapper.h" nit: alphabetize
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, magjed@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2488973002/#ps200001 (title: "Alphabetize imports")
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": 200001, "attempt_start_ts": 1479889692479180,
"parent_rev": "f2b50e711fa037bc068ecb640125a48a52c38c7c", "commit_rev":
"b0b805896aafdb02b95147cdcfd12b7503eb71e6"}
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Split avfoundationcapturer classes in separate files. Increases readibility and helps with the formating. BUG=webrtc:6680 ========== to ========== Split avfoundationcapturer classes in separate files. Increases readibility and helps with the formating. BUG=webrtc:6680 Committed: https://crrev.com/71caaca2544963a446e189e37beec5111d918177 Cr-Commit-Position: refs/heads/master@{#15206} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/71caaca2544963a446e189e37beec5111d918177 Cr-Commit-Position: refs/heads/master@{#15206}
Message was sent while issue was closed.
https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2488973002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoCapturerInternal.mm:161: [RTCDispatcher On 2016/11/22 21:26:07, tkchin_webrtc wrote: > On 2016/11/22 11:43:44, denicija-webrtc wrote: > > On 2016/11/21 18:51:47, tkchin_webrtc wrote: > > > restore indentation to previous > > > here and elsewhere > > > > Is this remarks towards decreasing the diff size? I know that it might be a > bit > > difficult for review with all the changes. However, we discussed this a bit > with > > Magnus and Kari (some of it in comments on this CL as well) and we agreed that > > the previous indentation was not according to the guidelines and that it would > > be better to fix it in this CL. > > Do you have objections to leaving it as it is? > > The previous indentation conforms to indentation used in much of google though. > > Where is the guideline against this? > > I won't block CL for it, but I think adding horizontal whitespace like this > makes it difficult to read, and difficult to code as well because it will force > you to hit 100 char limit much faster. The only reason I can think of that you > would want this is if this is the output of the auto-formatter, which we aren't > using anyway because of ObjC++. I see your point. Yes hitting the 100 char line happens much faster and it's not nice :) We were looking at this guideline https://google.github.io/styleguide/objcguide.xml#Blocks Namely this example where the block indents 4 spaces from the first wrap line. // An example where the parameter wraps and the block declaration does // not fit on the same line as the name. [[SessionService sharedService] loadWindowWithCompletionBlock: ^(SessionWindow *window) { if (window) { [self windowDidLoad:window]; } else { [self errorLoadingWindow]; } }]; Perhaps we can mitigate this at certain places by moving out blocks in local variables? https://codereview.webrtc.org/2488973002/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2488973002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:20: #include "avfoundationformatmapper.h" On 2016/11/22 21:26:07, tkchin_webrtc wrote: > nit: alphabetize Done.
Message was sent while issue was closed.
https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h (right): https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h:14: #include "webrtc/media/base/videocapturer.h" Include webrtc/media/base/videocommon.h instead. https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h:19: extern std::set<cricket::VideoFormat> GetSupportedVideoFormatsForDevice( These functions should not be marked extern, unless there is some ObjC reason I'm not aware of.
Message was sent while issue was closed.
On 2016/11/24 13:35:40, magjed_webrtc wrote: > https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h (right): > > https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h:14: #include > "webrtc/media/base/videocapturer.h" > Include webrtc/media/base/videocommon.h instead. Is it OK to do this in the follow up CL so that I don't need to re-land this one? > https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h:19: extern > std::set<cricket::VideoFormat> GetSupportedVideoFormatsForDevice( > These functions should not be marked extern, unless there is some ObjC reason > I'm not aware of. I made them extern as per Zeke's comment. I don't know of any ObjC particular reason to be honest. It should be the same as cpp and c.
Message was sent while issue was closed.
On 2016/11/25 08:25:10, denicija-webrtc wrote: > On 2016/11/24 13:35:40, magjed_webrtc wrote: > > > https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... > > File webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h (right): > > > > > https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... > > webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h:14: #include > > "webrtc/media/base/videocapturer.h" > > Include webrtc/media/base/videocommon.h instead. > > Is it OK to do this in the follow up CL so that I don't need to re-land this > one? > > > > https://codereview.webrtc.org/2488973002/diff/200001/webrtc/sdk/objc/Framewor... > > webrtc/sdk/objc/Framework/Classes/avfoundationformatmapper.h:19: extern > > std::set<cricket::VideoFormat> GetSupportedVideoFormatsForDevice( > > These functions should not be marked extern, unless there is some ObjC reason > > I'm not aware of. > > I made them extern as per Zeke's comment. I don't know of any ObjC particular > reason to be honest. It should be the same as cpp and c. Yes, do these changes in https://codereview.webrtc.org/2526813002/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
