Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776703002:
New RTCCameraVideoCapturer. (Closed)
DescriptionNew RTCCameraVideoCapturer.
Implements a new capturer based on the new video source design.
BUG=webrtc:7177
Review-Url: https://codereview.webrtc.org/2776703002
Cr-Commit-Position: refs/heads/master@{#17486}
Committed: https://chromium.googlesource.com/external/webrtc/+/3883ccb5d78dc7368c2f57cf3f4a477f83547f01
Patch Set 1 : Safe #
Total comments: 38
Patch Set 2 : Address comments and cleanup. #
Total comments: 9
Patch Set 3 : Fix compilation on mac. #Patch Set 4 : remove begin/end configuration #Patch Set 5 : Make RTCCameraVideoCapturer pure Objective-C. #
Total comments: 3
Patch Set 6 : Make a copy of the inputs. #Patch Set 7 : Address denicija's comments. #
Total comments: 6
Messages
Total messages: 36 (19 generated)
Description was changed from ========== New RTCCameraVideoCapturer. Implements a new capturer based on the new video source design. BUG=webrtc:7177 ========== to ========== New RTCCameraVideoCapturer. Implements a new capturer based on the new video source design. BUG=webrtc:7177 ==========
sakal@webrtc.org changed reviewers: + denicija@webrtc.org, magjed@webrtc.org
PTAL
Patchset #1 (id:1) has been deleted
https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:11: #import "WebRTC/RTCCameraVideoCapturer.h" Systems import first. Then empty line, then local frameworks imports, empty line then local files https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:87: static inline BOOL IsMediaSubTypeSupported(FourCharCode mediaSubType) { Usually static function go at the top of the file together with the static properties. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:106: - (void)dealloc { We should stop the capture session somewhere as well. The dealloc seems like a good place to do it. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:106: - (void)dealloc { Idea: How about we make sure that the session is not capturing when we are being dealloc-ated. That way we force devs to call `-stop` and give this class opportunity to clean up and also we would prevent simple mistakes (like this object not living long enough) to be prevented. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:107: RTCLogInfo("dealloc"); I don't think this logging is useful. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:109: } [super dealloc]; at the end of the method. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:121: - (void)startCaptureWithDevice:(AVCaptureDevice *)device In implementation file, keep public methods together and follow the order in which the methods were defined in the header file. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:125: dispatchAsyncOnType:RTCDispatcherTypeCaptureSession Suggestion: extract some of the functionality from this block in methods. It will make testing easier and also increase readability. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:172: - (void)stop { I don't see a reason with this type of workload in `stop` to perform it async-ly. As a matter of fact the dispatch here might be an overhead. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:177: [_captureSession beginConfiguration]; No need to wrap the input removal in `beginConfiguration` and `commitConfiguration` according to the documentation https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:328: [_captureSession startRunning]; Not sure if this is still proper way of handling this use case as the captureSession runs throughout the life of this object. Have you tested this code path what happens? https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:335: #pragma mark - Private This pragma mark should be way above, there are plenty of other private methods above. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:338: _captureSession = [[AVCaptureSession alloc] init]; Add asserts (for instance if the _captureSession already exists etc) here to make sure `setupCaptureSession` is called only once https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:353: [self updateOrientation]; No need to call this here yet. We'll update rotation once the capturing for particular device starts https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:355: [[UIDevice currentDevice] beginGeneratingDeviceOrientationNotifications]; Let's remove this from the dispatch block and from the setup as well. No need to respond to orientation updates if no capture device is running. Better add this in `startCaptureForDevice...` and then `endGeneratingDeviceOrientationNotifications` in `stop...` https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:382: bool usingFrontCamera = _currentDevice.position == AVCaptureDevicePositionFront; Preferably use BOOL in ObjC code. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h (right): https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:19: @interface RTCCameraVideoCapturer : RTCVideoCapturer<AVCaptureVideoDataOutputSampleBufferDelegate> Remove the <AVCaptureVideoDataOutputSampleBufferDelegate> from the header into a class extension in the implementation file. It's implementation detail not needed in the header https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:19: @interface RTCCameraVideoCapturer : RTCVideoCapturer<AVCaptureVideoDataOutputSampleBufferDelegate> Add documentation comments to this interface and all methods bellow. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:31: @property(readonly, nonatomic) AVCaptureSession *captureSession; Not sure if the capture session needs to be exposed with the new design. Is it gonna be used somewhere?
https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:135: // TODO(magjed): Error callback? Remove TODOs if you are not going to add the error callback in this CL. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:172: - (void)stop { On 2017/03/26 18:08:58, daniela-webrtc wrote: > I don't see a reason with this type of workload in `stop` to perform it > async-ly. As a matter of fact the dispatch here might be an overhead. Maybe it's necessary for thread safety? We need to be careful here that it's ok to stop async with respect to e.g. dealloc. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h (right): https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:31: @property(readonly, nonatomic) AVCaptureSession *captureSession; On 2017/03/26 18:08:58, daniela-webrtc wrote: > Not sure if the capture session needs to be exposed with the new design. Is it > gonna be used somewhere? It's used for RTCCameraPreviewView.m. I guess it's an optimization since we could use RTCEAGLVideoView instead and not expose this.
magjed@webrtc.org changed reviewers: + tkchin@webrtc.org
Adding Zeke as reviewer in case he has any comments on webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h in particular.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:11: #import "WebRTC/RTCCameraVideoCapturer.h" On 2017/03/26 18:08:58, daniela-webrtc wrote: > Systems import first. Then empty line, then local frameworks imports, empty line > then local files > Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:87: static inline BOOL IsMediaSubTypeSupported(FourCharCode mediaSubType) { On 2017/03/26 18:08:58, daniela-webrtc wrote: > Usually static function go at the top of the file together with the static > properties. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:106: - (void)dealloc { On 2017/03/26 18:08:58, daniela-webrtc wrote: > We should stop the capture session somewhere as well. The dealloc seems like a > good place to do it. Capture session is now stopped in stopCapture. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:107: RTCLogInfo("dealloc"); On 2017/03/26 18:08:57, daniela-webrtc wrote: > I don't think this logging is useful. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:109: } On 2017/03/26 18:08:57, daniela-webrtc wrote: > [super dealloc]; > at the end of the method. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:121: - (void)startCaptureWithDevice:(AVCaptureDevice *)device On 2017/03/26 18:08:57, daniela-webrtc wrote: > In implementation file, keep public methods together and follow the order in > which the methods were defined in the header file. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:125: dispatchAsyncOnType:RTCDispatcherTypeCaptureSession On 2017/03/26 18:08:57, daniela-webrtc wrote: > Suggestion: extract some of the functionality from this block in methods. It > will make testing easier and also increase readability. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:135: // TODO(magjed): Error callback? On 2017/03/27 13:11:48, magjed_webrtc wrote: > Remove TODOs if you are not going to add the error callback in this CL. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:177: [_captureSession beginConfiguration]; On 2017/03/26 18:08:57, daniela-webrtc wrote: > No need to wrap the input removal in `beginConfiguration` and > `commitConfiguration` according to the documentation Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:328: [_captureSession startRunning]; On 2017/03/26 18:08:57, daniela-webrtc wrote: > Not sure if this is still proper way of handling this use case as the > captureSession runs throughout the life of this object. Have you tested this > code path what happens? This is what we used to do and seems to work fine for me. I added a check so we don't restart the session if stopCapture has been called. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:335: #pragma mark - Private On 2017/03/26 18:08:58, daniela-webrtc wrote: > This pragma mark should be way above, there are plenty of other private methods > above. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:353: [self updateOrientation]; On 2017/03/26 18:08:58, daniela-webrtc wrote: > No need to call this here yet. We'll update rotation once the capturing for > particular device starts Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:355: [[UIDevice currentDevice] beginGeneratingDeviceOrientationNotifications]; On 2017/03/26 18:08:58, daniela-webrtc wrote: > Let's remove this from the dispatch block and from the setup as well. > No need to respond to orientation updates if no capture device is running. > Better add this in `startCaptureForDevice...` and then > `endGeneratingDeviceOrientationNotifications` in `stop...` Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:382: bool usingFrontCamera = _currentDevice.position == AVCaptureDevicePositionFront; On 2017/03/26 18:08:57, daniela-webrtc wrote: > Preferably use BOOL in ObjC code. Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h (right): https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:19: @interface RTCCameraVideoCapturer : RTCVideoCapturer<AVCaptureVideoDataOutputSampleBufferDelegate> On 2017/03/26 18:08:58, daniela-webrtc wrote: > Remove the <AVCaptureVideoDataOutputSampleBufferDelegate> from the header into a > class extension in the implementation file. It's implementation detail not > needed in the header Done. https://codereview.webrtc.org/2776703002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:31: @property(readonly, nonatomic) AVCaptureSession *captureSession; On 2017/03/27 13:11:48, magjed_webrtc wrote: > On 2017/03/26 18:08:58, daniela-webrtc wrote: > > Not sure if the capture session needs to be exposed with the new design. Is it > > gonna be used somewhere? > > It's used for RTCCameraPreviewView.m. I guess it's an optimization since we > could use RTCEAGLVideoView instead and not expose this. Yes, this is the reason it exists here.
The CQ bit was checked by sakal@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: linux_arm on master.tryserver.webrtc (JOB_FAILED, build hasn't started yet, builder probably lacks capacity)
https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:92: NSAssert( On second thought NSAssert might be bit aggressive here (some projects have NSAsserts enabled in release builds as well for instance). Maybe RTCDCheck would be better? https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:134: [_currentDevice unlockForConfiguration]; This should be in [updateDeviceCaptureFormat..]. Locking is needed when setting format and from what I can see nothing else requires locking the device. https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:378: [_captureSession beginConfiguration]; Documentation doesn't state that addInput should be wrapped in beginConfiguration/commitConfiguration. Is it necessary?
The CQ bit was checked by sakal@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/...
https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:92: NSAssert( On 2017/03/29 14:17:46, daniela-webrtc wrote: > On second thought NSAssert might be bit aggressive here (some projects have > NSAsserts enabled in release builds as well for instance). > Maybe RTCDCheck would be better? RTC_DCHECK is not available because this is a pure Objective-C file, right? https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:134: [_currentDevice unlockForConfiguration]; On 2017/03/29 14:17:46, daniela-webrtc wrote: > This should be in [updateDeviceCaptureFormat..]. Locking is needed when setting > format and from what I can see nothing else requires locking the device. The reason I moved it outside, quote from Apple's documentation: """ In macOS, a capture session can still automatically configure the capture format after you make changes. To prevent automatic changes to the capture format in macOS, follow these steps: 1. Lock the device with the lockForConfiguration() method. 2. Change the device’s activeFormat property. 3. Begin capture with the session’s startRunning() method. 4. Unlock the device with the unlockForConfiguration() method. """ https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:378: [_captureSession beginConfiguration]; On 2017/03/29 14:17:46, daniela-webrtc wrote: > Documentation doesn't state that addInput should be wrapped in > beginConfiguration/commitConfiguration. Is it necessary? I guess not.
https://codereview.webrtc.org/2776703002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2776703002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m:296: - (void)handleApplicationDidBecomeActive:(NSNotification *)notification { This doesn't seem to be needed necessarily but a similar method was added to the old implementation here: https://codereview.webrtc.org/2258583004 Do you think we should keep it?
https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:92: NSAssert( On 2017/03/30 11:57:29, sakal wrote: > On 2017/03/29 14:17:46, daniela-webrtc wrote: > > On second thought NSAssert might be bit aggressive here (some projects have > > NSAsserts enabled in release builds as well for instance). > > Maybe RTCDCheck would be better? > > RTC_DCHECK is not available because this is a pure Objective-C file, right? Ah forgot about that :) In that case it's not worth converting the file for this. https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:134: [_currentDevice unlockForConfiguration]; On 2017/03/30 11:57:29, sakal wrote: > On 2017/03/29 14:17:46, daniela-webrtc wrote: > > This should be in [updateDeviceCaptureFormat..]. Locking is needed when > setting > > format and from what I can see nothing else requires locking the device. > > The reason I moved it outside, quote from Apple's documentation: > """ > In macOS, a capture session can still automatically configure the capture format > after you make changes. To prevent automatic changes to the capture format in > macOS, follow these steps: > 1. Lock the device with the lockForConfiguration() method. > 2. Change the device’s activeFormat property. > 3. Begin capture with the session’s startRunning() method. > 4. Unlock the device with the unlockForConfiguration() method. > """ I see. In that case, can we move the `lockForConfiguration` outside of `updateDeviceCaptureFormat` and call it before line 130? https://codereview.webrtc.org/2776703002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2776703002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m:296: - (void)handleApplicationDidBecomeActive:(NSNotification *)notification { On 2017/03/31 07:42:44, sakal wrote: > This doesn't seem to be needed necessarily but a similar method was added to the > old implementation here: https://codereview.webrtc.org/2258583004 > > Do you think we should keep it? According to the CL it's a theoretical fix. I think it's better that we keep it.
https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2776703002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.mm:134: [_currentDevice unlockForConfiguration]; On 2017/03/31 09:24:19, daniela-webrtc wrote: > On 2017/03/30 11:57:29, sakal wrote: > > On 2017/03/29 14:17:46, daniela-webrtc wrote: > > > This should be in [updateDeviceCaptureFormat..]. Locking is needed when > > setting > > > format and from what I can see nothing else requires locking the device. > > > > The reason I moved it outside, quote from Apple's documentation: > > """ > > In macOS, a capture session can still automatically configure the capture > format > > after you make changes. To prevent automatic changes to the capture format in > > macOS, follow these steps: > > 1. Lock the device with the lockForConfiguration() method. > > 2. Change the device’s activeFormat property. > > 3. Begin capture with the session’s startRunning() method. > > 4. Unlock the device with the unlockForConfiguration() method. > > """ > I see. In that case, can we move the `lockForConfiguration` outside of > `updateDeviceCaptureFormat` and call it before line 130? Done.
lgtm https://codereview.webrtc.org/2776703002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2776703002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m:296: - (void)handleApplicationDidBecomeActive:(NSNotification *)notification { On 2017/03/31 07:42:44, sakal wrote: > This doesn't seem to be needed necessarily but a similar method was added to the > old implementation here: https://codereview.webrtc.org/2258583004 > > Do you think we should keep it? Let's keep it, we can't simplify the rest of the code anyway if we remove it, and it looks like it was added for a reason.
lgtm
The CQ bit was checked by sakal@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.
The CQ bit was checked by sakal@webrtc.org
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": 180001, "attempt_start_ts": 1490962171519000,
"parent_rev": "ee8b8611904ec0eaf600acba8561750a9e45cb85", "commit_rev":
"3883ccb5d78dc7368c2f57cf3f4a477f83547f01"}
Message was sent while issue was closed.
Description was changed from ========== New RTCCameraVideoCapturer. Implements a new capturer based on the new video source design. BUG=webrtc:7177 ========== to ========== New RTCCameraVideoCapturer. Implements a new capturer based on the new video source design. BUG=webrtc:7177 Review-Url: https://codereview.webrtc.org/2776703002 Cr-Commit-Position: refs/heads/master@{#17486} Committed: https://chromium.googlesource.com/external/webrtc/+/3883ccb5d78dc7368c2f57cf3... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/3883ccb5d78dc7368c2f57cf3...
Message was sent while issue was closed.
Any way to add tests here? https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h (right): https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:29: - (void)startCaptureWithDevice:(AVCaptureDevice *)device This is pretty unusual. How are we supposed to change front/rear camera easily? https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:36: @property(readonly, nonatomic) AVCaptureSession *captureSession; nit: properties before methods
Message was sent while issue was closed.
https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m:361: #pragma mark - Private, called inside capture queue assert queue in these methods?
Message was sent while issue was closed.
Changes here: https://codereview.webrtc.org/2800853006 https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCCameraVideoCapturer.m:361: #pragma mark - Private, called inside capture queue On 2017/04/06 18:13:45, tkchin_webrtc wrote: > assert queue in these methods? Done. https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h (right): https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:29: - (void)startCaptureWithDevice:(AVCaptureDevice *)device On 2017/04/06 18:06:43, tkchin_webrtc wrote: > This is pretty unusual. How are we supposed to change front/rear camera easily? We wanted to add more flexibility to the application in choosing the format. Switching camera involves enumerating capture devices and restarting the capture with a new device. https://codereview.webrtc.org/2776703002/diff/180001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCCameraVideoCapturer.h:36: @property(readonly, nonatomic) AVCaptureSession *captureSession; On 2017/04/06 18:06:43, tkchin_webrtc wrote: > nit: properties before methods Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
