Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349223002:
Replace SessionPresets with AVCaptureDeviceFormats (Closed)
|
Created:
4 years, 3 months ago by daniela-webrtc Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReplaces the SessionPresets with AVCaptureDeviceFormats.
It eliminates the need for hardcoded enumerations of the supported presets.
Hopefully it enables us to support slightly bigger set of resolutions and make the code slightly cleaner.
BUG=webrtc:6355
Committed: https://crrev.com/4f15ca5d74cdb456d011efc3b133d62cbce8513c
Cr-Commit-Position: refs/heads/master@{#14547}
Patch Set 1 #Patch Set 2 : Remove unused method #
Total comments: 18
Patch Set 3 : Address reviewer's comments #
Total comments: 48
Patch Set 4 : Address comments for patch 3 #Patch Set 5 : Extracting methods to avoid duplication of code. #
Total comments: 1
Patch Set 6 : Fix logical expression. #
Total comments: 27
Patch Set 7 : Address style comments #
Total comments: 23
Patch Set 8 : Address style comments #
Total comments: 3
Messages
Total messages: 44 (19 generated)
The CQ bit was checked by denicija@chromium.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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Replace SessionPressets with AVCaptureDeviceFormats Fix line breaks. Remove unused structs and enums Fix wrong naming of local variable Replace session presset with device format Modify supported video formats creation Provide public getters for back and front capture devices Add AVCaptureDevice/cricket::VideoFormat transformers BUG = webrtc:6355 ========== to ========== Replace SessionPressets with AVCaptureDeviceFormats Fix line breaks. Remove unused structs and enums Fix wrong naming of local variable Replace session presset with device format Modify supported video formats creation Provide public getters for back and front capture devices Add AVCaptureDevice/cricket::VideoFormat transformers BUG = webrtc:6355 ==========
denicija@webrtc.org changed reviewers: - denicija@chromium.org
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org
The CQ bit was checked by denicija@webrtc.org
The CQ bit was unchecked by denicija@webrtc.org
This looks good! Could you edit the CL description so that it lays out what this patch does and why it's something we want? Also, please try to keep lines to 100 characters, it's helpful to run `git cl format` before submitting! https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:33: // Maping from cricket::VideoFormat to AVCaptureDeviceFormat. nit: mapping https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:68: code !=kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) { nit: missing space https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:75: Float64 fps = frameRate.maxFrameRate; is there a need to introduce this variable? https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:82: std::vector<cricket::VideoFormat>::iterator iterator = std::find(supportedFormats.begin(), I'd go for auto here, this type is very unwieldy. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:90: } We might be better off just using a std::set for supportedFormats, and skipping the existance check, or using .erase on the vector to get rid of duplicates. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:641: std::vector<cricket::VideoFormat> backCameraSupportedVideoFormats = I would use auto for all these types, I try to use it as much as I can. I don't know if other people here have the same opinion, but it's up to you. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:678: [session beginConfiguration]; // the session to which the receiver's AVCaptureDeviceInput is added. Is this necessary when only re-configuring the device and not the session?
This looks promising! Please edit the CL description - the default when you upload is the git log information, but this is rarely what you want. Also, the 'BUG = webrtc:6355' field should be without any spaces, i.e. 'BUG=webrtc:6355'. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:47: if (videoFormat.width == dimension.width && videoFormat.height == dimension.height) { Shouldn't we also look at fps? Maybe it would be beneficial to split out a cricket::VideoFormat AVFormatToCricketFormat(AVCaptureDeviceFormat* deviceFormat) function and use 'videoFormat == AVFormatToCricketFormat(deviceFormat)' as the check here. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:90: } On 2016/09/20 07:00:30, kthelgason wrote: > We might be better off just using a std::set for supportedFormats, and skipping > the existance check, or using .erase on the vector to get rid of duplicates. +1 Go with a std::set, it should simplify the code. You can create the std::vector for SetSupportedFormats automatically by using a vector output iterator to std::set_intersection. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:641: std::vector<cricket::VideoFormat> backCameraSupportedVideoFormats = On 2016/09/20 07:00:30, kthelgason wrote: > I would use auto for all these types, I try to use it as much as I can. I don't > know if other people here have the same opinion, but it's up to you. See https://google.github.io/styleguide/cppguide.html#auto. I would not use auto here, because the type is not clear from the local context (in the same expression or within a few lines). I also don't think the type is particularly cluttered, and for me, it adds clarity. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:673: GetDeviceFormatForVideoFormat([_capturer getActiveCaptureDevice], format); You need to indent the new line with 4 spaces. https://google.github.io/styleguide/cppguide.html#Function_Calls. Try running 'git cl format' which will fix stuff like this automagically. That commands works very well for C++, but be aware that it might have some problems with ObjC code, so a tip is to commit first before running it and then cherry-pick the changes you actually want.
Description was changed from ========== Replace SessionPressets with AVCaptureDeviceFormats Fix line breaks. Remove unused structs and enums Fix wrong naming of local variable Replace session presset with device format Modify supported video formats creation Provide public getters for back and front capture devices Add AVCaptureDevice/cricket::VideoFormat transformers BUG = webrtc:6355 ========== to ========== Replaces the SessionPresets with AVCaptureDeviceFormats. It eliminates the need for hardcoded enumerations of the supported presets. Hopefully it enables us to support slightly bigger set of resolutions and make the code slightly cleaner. BUG=webrtc:6355 ==========
https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:68: code !=kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) { On 2016/09/20 07:00:30, kthelgason wrote: > nit: missing space Acknowledged. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:82: std::vector<cricket::VideoFormat>::iterator iterator = std::find(supportedFormats.begin(), On 2016/09/20 07:00:30, kthelgason wrote: > I'd go for auto here, this type is very unwieldy. This will no longer be needed once I implement the supportedFormats as set. Thanks for the tip! https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:90: } On 2016/09/20 07:00:30, kthelgason wrote: > We might be better off just using a std::set for supportedFormats, and skipping > the existance check, or using .erase on the vector to get rid of duplicates. Acknowledged. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:678: [session beginConfiguration]; // the session to which the receiver's AVCaptureDeviceInput is added. On 2016/09/20 07:00:30, kthelgason wrote: > Is this necessary when only re-configuring the device and not the session? Yes. Because this essentially causes the session to be re-configured as well (as per Apple's documentations and examples)
https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:47: if (videoFormat.width == dimension.width && videoFormat.height == dimension.height) { On 2016/09/20 08:42:18, magjed_webrtc wrote: > Shouldn't we also look at fps? Maybe it would be beneficial to split out a > cricket::VideoFormat AVFormatToCricketFormat(AVCaptureDeviceFormat* > deviceFormat) function and use 'videoFormat == > AVFormatToCricketFormat(deviceFormat)' as the check here. Acknowledged. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:75: Float64 fps = frameRate.maxFrameRate; On 2016/09/20 07:00:30, kthelgason wrote: > is there a need to introduce this variable? Acknowledged. https://codereview.webrtc.org/2349223002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:673: GetDeviceFormatForVideoFormat([_capturer getActiveCaptureDevice], format); On 2016/09/20 08:42:18, magjed_webrtc wrote: > You need to indent the new line with 4 spaces. > https://google.github.io/styleguide/cppguide.html#Function_Calls. Try running > 'git cl format' which will fix stuff like this automagically. That commands > works very well for C++, but be aware that it might have some problems with ObjC > code, so a tip is to commit first before running it and then cherry-pick the > changes you actually want. Acknowledged.
denicija@webrtc.org changed reviewers: + tkchin@webrtc.org
would be good to take this opportunity to add unit tests (ie unit test for converting system format into webrtc format pod) https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:35: // Maping from cricket::VideoFormat to AVCaptureDeviceFormat. nit: Mapping https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:39: AVCaptureDeviceFormat* desiredDeviceFormat = nil; if treating this as a C/C++ function, should be desired_device_format likewise for other vars here Otherwise just move the * https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:40: for (AVCaptureDeviceFormat* deviceFormat in [device formats]) { dot syntax for properties. Here and elsewhere https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:46: if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange && If we care about only selectively picking out NV12 here, we could probably also pick whether we want full or video. When do we need full range? Doesn't matter for this CL really, feel free to keep both, but would be good to understand. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:55: if (!(framerate.minFrameRate >= kFramesPerSecond && perhaps frameRate.minFrameRate < kFramesPerSecond || frameRate.maxFrameRate > kFramesPerSecond up to you https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:61: // this is the preferred format so no need to wait for better option nit: Capitalize first letter of comment and end with period. here and elsewhere https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:75: AVCaptureDevice* device) { ditto pick one of C++/ObjC style conventions to use but don't mix them https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:82: if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange && I would've expected this code to output all possible formats including non NV12 based on the function name. Perhaps add comment to indicate only NV12 formats? https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:120: - (nullable AVCaptureDevice*)frontCaptureDevice; not sure we need nullability annotations for interfaces that we expect to never have to interop with swift If we add this though, we should add the assume_nonnull_begin / end macros in the right places https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:212: return _frontCameraInput.device; nit: Device *) here and below https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:614: AVCaptureDevice* newDevice = [newInput device]; Device *newDevice here and below https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:617: const cricket::VideoFormat* format = _capturer->GetCaptureFormat(); is this thread safe? (Calling from different queue) https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:619: GetDeviceFormatForVideoFormat(newDevice, *format); How expensive is this operation? Is it slow to query for available formats / should we build a cache instead? https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:623: [newDevice setActiveFormat:newFormat]; Use dot syntax where able .activeFormat = https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:626: LOG(LS_ERROR) << [NSString stringWithFormat:@"Exception occured while " We've used RTCLogError as a convenience https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:649: std::set<cricket::VideoFormat> frontCameraSupportedVideoFormats = c++ style here so front_camera_supported_video_formats https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:704: [session beginConfiguration]; // the session to which the receiver's nit: don't split comments out at end of line, place above if it spans multiple lines
https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:33: static const int kFramesPerSecond = 30; Can you add a todo, something like: // TODO(denicija): Support higher framerates. See http://crbug/webrtc/6355 for more info. You can put it in my name if you don't want to own it. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:46: if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange && On 2016/09/27 13:39:33, tkchin_webrtc wrote: > If we care about only selectively picking out NV12 here, we could probably also > pick whether we want full or video. When do we need full range? Doesn't matter > for this CL really, feel free to keep both, but would be good to understand. Ah, true. We hardcode the pixel format to FullRange in videoDataOutput so we can probably do it here as well. That would simplify the code a bit. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:56: framerate.maxFrameRate <= kFramesPerSecond)) { I think this check is incorrect, the comparison is inversed. Swap '<=' with '>=' or do like Zeke suggests. Also, I think we should base this on videoFormat.framerate() rather than kFramesPerSecond. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:65: desiredDeviceFormat = deviceFormat; I'm wondering how we should select between multiple ranges that include 30 fps, for example 2-30 fps range and 5-240 fps range. In that case we probably want 2-30 fps. I think in general we want a low lower fps bound to allow the camera to compensate for brightness. Does anyone know if the upper fps bound matter at all if we cap the fps to 30? https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:82: if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange && On 2016/09/27 13:39:33, tkchin_webrtc wrote: > I would've expected this code to output all possible formats including non NV12 > based on the function name. Perhaps add comment to indicate only NV12 formats? FYI - We currently only support NV12 because the conversion is hardcoded to libyuv::NV12ToI420 in CoreVideoFrameBuffer. I also think NV12 is the most native and fastest format for iOS so probably fine to not support anything else. A comment would be nice though. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:89: I think we should have the fps check here as well, to make sure all formats we return here are allowed in GetDeviceFormatForVideoFormat. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:706: if ([device lockForConfiguration:&error]) { Can we extract this out in a common function: void SetActiveFormat(AVCaptureDevice* device, const cricket::VideoFormat& videoFormat); and in updateSessionInputForUseBackCamera we do: SetActiveFormat([newInput device], *_capturer->GetCaptureFormat()); and here we do: [session beginConfiguration]; SetActiveFormat([_capturer getActiveCaptureDevice], format); [session commitConfiguration]; ?
https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:33: static const int kFramesPerSecond = 30; On 2016/09/27 14:36:10, magjed_webrtc wrote: > Can you add a todo, something like: > // TODO(denicija): Support higher framerates. See http://crbug/webrtc/6355 for > more info. > You can put it in my name if you don't want to own it. Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:35: // Maping from cricket::VideoFormat to AVCaptureDeviceFormat. On 2016/09/27 13:39:34, tkchin_webrtc wrote: > nit: Mapping Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:39: AVCaptureDeviceFormat* desiredDeviceFormat = nil; On 2016/09/27 13:39:34, tkchin_webrtc wrote: > if treating this as a C/C++ function, should be desired_device_format > likewise for other vars here > Otherwise just move the * Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:40: for (AVCaptureDeviceFormat* deviceFormat in [device formats]) { On 2016/09/27 13:39:33, tkchin_webrtc wrote: > dot syntax for properties. Here and elsewhere Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:46: if (code != kCVPixelFormatType_420YpCbCr8BiPlanarFullRange && On 2016/09/27 14:36:10, magjed_webrtc wrote: > On 2016/09/27 13:39:33, tkchin_webrtc wrote: > > If we care about only selectively picking out NV12 here, we could probably > also > > pick whether we want full or video. When do we need full range? Doesn't matter > > for this CL really, feel free to keep both, but would be good to understand. > > Ah, true. We hardcode the pixel format to FullRange in videoDataOutput so we can > probably do it here as well. That would simplify the code a bit. Not sure I follow. The reason why we allow both full and video range is because some devices only support video range (i.e macs). This check here is only an early return so that we don't loop through formats that are neither. Then at line 60 we prioritize FullRange over VideoRange. But if unclear I might introduce helper methods like isSubMediaTypeSupported() or something along those lines. Is that you were thinking? https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:55: if (!(framerate.minFrameRate >= kFramesPerSecond && On 2016/09/27 13:39:34, tkchin_webrtc wrote: > perhaps frameRate.minFrameRate < kFramesPerSecond || frameRate.maxFrameRate > > kFramesPerSecond > up to you Great catch. Thanks https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:56: framerate.maxFrameRate <= kFramesPerSecond)) { On 2016/09/27 14:36:10, magjed_webrtc wrote: > I think this check is incorrect, the comparison is inversed. Swap '<=' with '>=' > or do like Zeke suggests. Also, I think we should base this on > videoFormat.framerate() rather than kFramesPerSecond. Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:61: // this is the preferred format so no need to wait for better option On 2016/09/27 13:39:34, tkchin_webrtc wrote: > nit: Capitalize first letter of comment and end with period. > here and elsewhere Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:75: AVCaptureDevice* device) { On 2016/09/27 13:39:33, tkchin_webrtc wrote: > ditto pick one of C++/ObjC style conventions to use but don't mix them Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:89: On 2016/09/27 14:36:10, magjed_webrtc wrote: > I think we should have the fps check here as well, to make sure all formats we > return here are allowed in GetDeviceFormatForVideoFormat. Agreed. I don't like how I handled the filtering of device formats in both these methods (a lot of duplicate code for eligible device formats) and it leads to inconsistency like this one. I'll create separate function that returns only eligible device formats and respects all the constraints i.e subMediaType and framerate. I think it will make the code easier to understand as well. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:120: - (nullable AVCaptureDevice*)frontCaptureDevice; On 2016/09/27 13:39:34, tkchin_webrtc wrote: > not sure we need nullability annotations for interfaces that we expect to never > have to interop with swift > If we add this though, we should add the assume_nonnull_begin / end macros in > the right places In general I like nullable/nonnull annotations as it helps with static analysis and also makes the intent clearer. However it might not be suitable for this CL. I'll remove it for now. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:212: return _frontCameraInput.device; On 2016/09/27 13:39:33, tkchin_webrtc wrote: > nit: Device *) > here and below Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:614: AVCaptureDevice* newDevice = [newInput device]; On 2016/09/27 13:39:34, tkchin_webrtc wrote: > Device *newDevice > here and below Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:623: [newDevice setActiveFormat:newFormat]; On 2016/09/27 13:39:33, tkchin_webrtc wrote: > Use dot syntax where able > .activeFormat = Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:626: LOG(LS_ERROR) << [NSString stringWithFormat:@"Exception occured while " On 2016/09/27 13:39:34, tkchin_webrtc wrote: > We've used RTCLogError as a convenience Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:649: std::set<cricket::VideoFormat> frontCameraSupportedVideoFormats = On 2016/09/27 13:39:34, tkchin_webrtc wrote: > c++ style here > so front_camera_supported_video_formats Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:704: [session beginConfiguration]; // the session to which the receiver's On 2016/09/27 13:39:34, tkchin_webrtc wrote: > nit: don't split comments out at end of line, place above if it spans multiple > lines Actually this comment is not needed. I'll remove it altogether.
https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:617: const cricket::VideoFormat* format = _capturer->GetCaptureFormat(); On 2016/09/27 13:39:34, tkchin_webrtc wrote: > is this thread safe? (Calling from different queue) Not sure. If i understand correctly the capture format is only set once and only at the beginning so this read shouldn't be a problem. If I'm wrong I'll add some thread safe mechanism https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:619: GetDeviceFormatForVideoFormat(newDevice, *format); On 2016/09/27 13:39:33, tkchin_webrtc wrote: > How expensive is this operation? Is it slow to query for available formats / > should we build a cache instead? Shouldn't be too expensive and ideally shouldn't happen too often. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:626: LOG(LS_ERROR) << [NSString stringWithFormat:@"Exception occured while " On 2016/09/28 15:29:26, denicija-webrtc wrote: > On 2016/09/27 13:39:34, tkchin_webrtc wrote: > > We've used RTCLogError as a convenience > > Done. For future reference, do we use RTCLogError in Objc and LOG(LS_ERROR) in cpp classes? https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:706: if ([device lockForConfiguration:&error]) { On 2016/09/27 14:36:10, magjed_webrtc wrote: > Can we extract this out in a common function: > void SetActiveFormat(AVCaptureDevice* device, const cricket::VideoFormat& > videoFormat); > and in updateSessionInputForUseBackCamera we do: > SetActiveFormat([newInput device], *_capturer->GetCaptureFormat()); > and here we do: > [session beginConfiguration]; > SetActiveFormat([_capturer getActiveCaptureDevice], format); > [session commitConfiguration]; > ? YES! I forgot to do this. Good catch. Thanks
https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:89: On 2016/09/28 15:29:26, denicija-webrtc wrote: > On 2016/09/27 14:36:10, magjed_webrtc wrote: > > I think we should have the fps check here as well, to make sure all formats we > > return here are allowed in GetDeviceFormatForVideoFormat. > > Agreed. I don't like how I handled the filtering of device formats in both > these methods (a lot of duplicate code for eligible device formats) and it leads > to inconsistency like this one. > I'll create separate function that returns only eligible device formats and > respects all the constraints i.e subMediaType and framerate. I think it will > make the code easier to understand as well. I've uploaded a patch with this change. Please let me know what you think and if it's more readable now. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:704: [session beginConfiguration]; // the session to which the receiver's On 2016/09/28 15:29:26, denicija-webrtc wrote: > On 2016/09/27 13:39:34, tkchin_webrtc wrote: > > nit: don't split comments out at end of line, place above if it spans multiple > > lines > > Actually this comment is not needed. I'll remove it altogether. Done. https://codereview.webrtc.org/2349223002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:706: if ([device lockForConfiguration:&error]) { On 2016/09/29 11:02:31, denicija-webrtc wrote: > On 2016/09/27 14:36:10, magjed_webrtc wrote: > > Can we extract this out in a common function: > > void SetActiveFormat(AVCaptureDevice* device, const cricket::VideoFormat& > > videoFormat); > > and in updateSessionInputForUseBackCamera we do: > > SetActiveFormat([newInput device], *_capturer->GetCaptureFormat()); > > and here we do: > > [session beginConfiguration]; > > SetActiveFormat([_capturer getActiveCaptureDevice], format); > > [session commitConfiguration]; > > ? > > YES! I forgot to do this. Good catch. Thanks Done.
https://codereview.webrtc.org/2349223002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:38: return (mediaSubType != kCVPixelFormatType_420YpCbCr8PlanarFullRange && I've actually made mistake. The check should be inverted. mediaSubType == kCVPixelFormatType_420YpCbCr8BiPlanarFullRange || mediaSubType == kCVPixelFormatType_420ypCbCr8BiPlanarVideoRange Another case for adding UnitTests :)
Mind taking another look. I've address most of the comments and refactored a bit to increase the readability and to clarify the intent of the code
On 2016/10/03 11:33:58, denicija-webrtc wrote: > Mind taking another look. I've address most of the comments and refactored a bit > to increase the readability and to clarify the intent of the code lgtm % some style comments!
https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:48: static NSArray<AVCaptureDeviceFormat *>* GetEligibleDeviceFormats( nit: location of * is inconsistent, should stick to function name. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:54: for (AVCaptureDeviceFormat *format in device.formats) { Can we make this const? https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:63: for (AVFrameRateRange *frameRateRange in format ditto https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:110: NSArray<AVCaptureDeviceFormat *>* eligibleFormats = nit: inconsistent * location. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:635: #if TARGET_OS_IPHONE Maybe we should use #if defined(WEBRTC_IOS), just for consistency with the rest of the codebase? https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:662: [_captureSession beginConfiguration]; No matching call to commitConfiguration https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:706: SetSupportedFormats(intersection_video_formats); Another option that may be less verbose is to use the std::vector range constructor here, and just do: SetSupportedFormats(std::vector<T>(set.begin(), set.end())) But totally up to you.
lgtm % nits. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:33: // TODO(denicija): add support for higher frame rates nit: add dot at end of every sentence in comments. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:67: continue; nit: Use break instead of continue to avoid adding the same format multiple times. The set filtering will take care of it later anyway, but still. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:127: // TODO(denicija): When this file is split this static method should be recnosidered. nit: reconsidered https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:135: const auto fps = cricket::VideoFormat::IntervalToFps(format.interval); Use int instead of auto here. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:702: if (back_camera_video_formats.size() == 0) { nit: Use *.empty() instead of *.size() == 0. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:706: SetSupportedFormats(intersection_video_formats); On 2016/10/03 13:57:53, kthelgason wrote: > Another option that may be less verbose is to use the std::vector range > constructor here, and just do: > SetSupportedFormats(std::vector<T>(set.begin(), set.end())) > But totally up to you. Yet another way to do it: std::vector<cricket::VideoFormat> intersection_video_formats; if (back_camera_video_formats.empty()) { intersection_video_formats.assign(front_camera_video_formats.begin(), front_camera_video_formats.end()); } else if (front_camera_video_formats.empty()) { intersection_video_formats.assign(back_camera_video_formats.begin(), back_camera_video_formats.end()); } else { std::set_intersection( front_camera_video_formats.begin(), front_camera_video_formats.end(), back_camera_video_formats.begin(), back_camera_video_formats.end(), std::back_inserter(intersection_video_formats)); } SetSupportedFormats(intersection_video_formats); BTW - will front_camera_video_formats ever be empty? I see that you have marked frontCaptureDevice nonnull.
https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:564: @(kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) Couldn't we remove this?
https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:33: // TODO(denicija): add support for higher frame rates On 2016/10/03 15:32:34, magjed_webrtc wrote: > nit: add dot at end of every sentence in comments. Done. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:48: static NSArray<AVCaptureDeviceFormat *>* GetEligibleDeviceFormats( On 2016/10/03 13:57:54, kthelgason wrote: > nit: location of * is inconsistent, should stick to function name. Done. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:110: NSArray<AVCaptureDeviceFormat *>* eligibleFormats = On 2016/10/03 13:57:53, kthelgason wrote: > nit: inconsistent * location. Done. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:127: // TODO(denicija): When this file is split this static method should be recnosidered. On 2016/10/03 15:32:34, magjed_webrtc wrote: > nit: reconsidered Done. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:135: const auto fps = cricket::VideoFormat::IntervalToFps(format.interval); On 2016/10/03 15:32:34, magjed_webrtc wrote: > Use int instead of auto here. Done. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:183: - (nonnull AVCaptureDevice *)frontCaptureDevice; This should be nullable. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:184: - (AVCaptureDevice *)backCaptureDevice; This should be nullable https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:564: @(kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) On 2016/10/03 16:58:53, magjed_webrtc wrote: > Couldn't we remove this? The app crashes if this is removed. It happened because this code is called before the deviceFormat is assigned. The chosen videoOutput, in this case, is device native but RTC-not supported format. I think this can be done as part of the refactoring efforts as I don't think it would be a minor task. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:662: [_captureSession beginConfiguration]; On 2016/10/03 13:57:54, kthelgason wrote: > No matching call to commitConfiguration Good catch! https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:702: if (back_camera_video_formats.size() == 0) { On 2016/10/03 15:32:34, magjed_webrtc wrote: > nit: Use *.empty() instead of *.size() == 0. Done. https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:706: SetSupportedFormats(intersection_video_formats); On 2016/10/03 15:32:34, magjed_webrtc wrote: > On 2016/10/03 13:57:53, kthelgason wrote: > > Another option that may be less verbose is to use the std::vector range > > constructor here, and just do: > > SetSupportedFormats(std::vector<T>(set.begin(), set.end())) > > But totally up to you. > > Yet another way to do it: > std::vector<cricket::VideoFormat> intersection_video_formats; > if (back_camera_video_formats.empty()) { > intersection_video_formats.assign(front_camera_video_formats.begin(), > front_camera_video_formats.end()); > } else if (front_camera_video_formats.empty()) { > intersection_video_formats.assign(back_camera_video_formats.begin(), > back_camera_video_formats.end()); > } else { > std::set_intersection( > front_camera_video_formats.begin(), front_camera_video_formats.end(), > back_camera_video_formats.begin(), back_camera_video_formats.end(), > std::back_inserter(intersection_video_formats)); > } > SetSupportedFormats(intersection_video_formats); > > BTW - will front_camera_video_formats ever be empty? I see that you have marked > frontCaptureDevice nonnull. I'll go the assign() route to have one return point in the method. Regarding the nonnull device format, not sure what happened between patches. It should be nullable. I think having an unavailable front camera is feasible scenario.
still lgtm https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:564: @(kCVPixelFormatType_420YpCbCr8BiPlanarFullRange) On 2016/10/04 11:47:03, denicija-webrtc wrote: > On 2016/10/03 16:58:53, magjed_webrtc wrote: > > Couldn't we remove this? > > The app crashes if this is removed. It happened because this code is called > before the deviceFormat is assigned. The chosen videoOutput, in this case, is > device native but RTC-not supported format. > I think this can be done as part of the refactoring efforts as I don't think it > would be a minor task. Acknowledged. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:711: std::set_intersection( nit: At least according to C++ style you should either write it as: std::set_intersection(front_camera_video_formats.begin(), ... back_camera_video_formats.begin(), ... or break and indent 4 spaces: std::set_intersection( front_camera_video_formats.begin(), ... back_camera_video_formats.begin(), ...
https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/100001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:635: #if TARGET_OS_IPHONE On 2016/10/03 13:57:53, kthelgason wrote: > Maybe we should use #if defined(WEBRTC_IOS), just for consistency with the rest > of the codebase? Perhaps as part of the upcoming refactor?
lgtm % style nits https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:37: static inline BOOL IsMediaSubTypeSupported(FourCharCode mediaSubType) { odd to have so many static fns as opposed to having methods on the relevant class, but ok https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:42: static inline BOOL IsFrameRateWithinRange(int fps, AVFrameRateRange* range) { nit: *range https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:64: .videoSupportedFrameRateRanges) { nit: don't split properties across lines if it doesn't fit in 100 chars, declare a local instead https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:72: return [eligibleDeviceFormats copy]; safety is fine, but probably not needed. fine to leave though https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:78: const cricket::VideoFormat& videoFormat) { be consistent in where you place & and * ObjC convention is &videoFormat https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:80: NSArray<AVCaptureDeviceFormat *>* eligibleFormats = nit: *eligibleFormats https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:129: static BOOL SetFormatForCaptureDevice(AVCaptureDevice* device, ditto * placement throughout the fn https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:132: nit: remove blank line https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:138: BOOL success = YES; nit: set this to NO and modify it to YES to save one line https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:146: @"Exception occured while setting active format!\n User info:%@", nit: make the errors as short as possible e.g. Failed to set active format. UserInfo: e.g. Failed to lock device. Error: %@ https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:181: - (AVCaptureDevice *)getActiveCaptureDevice; I don't recall this bit of code. We don't prefix accessors with "get" in objc https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:183: - (nullable AVCaptureDevice *)frontCaptureDevice; I'm surprised this isn't throwing warnings at you since the rest of the decls don't have nullability attrs. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:678: const cricket::VideoFormat* format = _capturer->GetCaptureFormat(); *format
On 2016/10/05 17:33:13, tkchin_webrtc wrote: > lgtm % style nits > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:37: static inline > BOOL IsMediaSubTypeSupported(FourCharCode mediaSubType) { > odd to have so many static fns as opposed to having methods on the relevant > class, but ok > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:42: static inline > BOOL IsFrameRateWithinRange(int fps, AVFrameRateRange* range) { > nit: *range > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:64: > .videoSupportedFrameRateRanges) { > nit: don't split properties across lines > if it doesn't fit in 100 chars, declare a local instead > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:72: return > [eligibleDeviceFormats copy]; > safety is fine, but probably not needed. fine to leave though > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:78: const > cricket::VideoFormat& videoFormat) { > be consistent in where you place & and * > ObjC convention is &videoFormat > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:80: > NSArray<AVCaptureDeviceFormat *>* eligibleFormats = > nit: *eligibleFormats > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:129: static BOOL > SetFormatForCaptureDevice(AVCaptureDevice* device, > ditto * placement throughout the fn > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:132: > nit: remove blank line > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:138: BOOL success > = YES; > nit: set this to NO and modify it to YES to save one line > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:146: @"Exception > occured while setting active format!\n User info:%@", > nit: make the errors as short as possible > e.g. Failed to set active format. UserInfo: > e.g. Failed to lock device. Error: %@ > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:181: - > (AVCaptureDevice *)getActiveCaptureDevice; > I don't recall this bit of code. We don't prefix accessors with "get" in objc > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:183: - (nullable > AVCaptureDevice *)frontCaptureDevice; > I'm surprised this isn't throwing warnings at you since the rest of the decls > don't have nullability attrs. > > https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:678: const > cricket::VideoFormat* format = _capturer->GetCaptureFormat(); > *format It'd be great to look into exposing methods for unit testing in subsequent file split.
https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:42: static inline BOOL IsFrameRateWithinRange(int fps, AVFrameRateRange* range) { On 2016/10/05 17:33:12, tkchin_webrtc wrote: > nit: *range Done. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:64: .videoSupportedFrameRateRanges) { On 2016/10/05 17:33:12, tkchin_webrtc wrote: > nit: don't split properties across lines > if it doesn't fit in 100 chars, declare a local instead Done. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:78: const cricket::VideoFormat& videoFormat) { On 2016/10/05 17:33:12, tkchin_webrtc wrote: > be consistent in where you place & and * > ObjC convention is &videoFormat Done. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:80: NSArray<AVCaptureDeviceFormat *>* eligibleFormats = On 2016/10/05 17:33:12, tkchin_webrtc wrote: > nit: *eligibleFormats Done. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:129: static BOOL SetFormatForCaptureDevice(AVCaptureDevice* device, On 2016/10/05 17:33:12, tkchin_webrtc wrote: > ditto * placement throughout the fn Done. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:138: BOOL success = YES; On 2016/10/05 17:33:11, tkchin_webrtc wrote: > nit: set this to NO and modify it to YES to save one line Since there are different code paths that can cause a failure, it would actually be more code to start with a NO. (Because of the try-catch block mostly i.e how do we ensure that exception has not occured) https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:146: @"Exception occured while setting active format!\n User info:%@", On 2016/10/05 17:33:12, tkchin_webrtc wrote: > nit: make the errors as short as possible > e.g. Failed to set active format. UserInfo: > e.g. Failed to lock device. Error: %@ Done. https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:181: - (AVCaptureDevice *)getActiveCaptureDevice; On 2016/10/05 17:33:11, tkchin_webrtc wrote: > I don't recall this bit of code. We don't prefix accessors with "get" in objc I noticed that as well, so that's why I renamed the new methods without the "get". This should be changed in the upcoming refactor https://codereview.webrtc.org/2349223002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:333: This is from the rebase https://codereview.webrtc.org/2349223002/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2349223002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:172: @property(atomic, assign) BOOL isRunning; // Whether the capture session is running. This is from the rebase https://codereview.webrtc.org/2349223002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:209: @synthesize isRunning = _isRunning; This is from the rebase. https://codereview.webrtc.org/2349223002/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:512: #endif This is from the rebase
The CQ bit was checked by denicija@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 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/2349223002/#ps140001 (title: "Address style comments")
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 ========== Replaces the SessionPresets with AVCaptureDeviceFormats. It eliminates the need for hardcoded enumerations of the supported presets. Hopefully it enables us to support slightly bigger set of resolutions and make the code slightly cleaner. BUG=webrtc:6355 ========== to ========== Replaces the SessionPresets with AVCaptureDeviceFormats. It eliminates the need for hardcoded enumerations of the supported presets. Hopefully it enables us to support slightly bigger set of resolutions and make the code slightly cleaner. BUG=webrtc:6355 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Replaces the SessionPresets with AVCaptureDeviceFormats. It eliminates the need for hardcoded enumerations of the supported presets. Hopefully it enables us to support slightly bigger set of resolutions and make the code slightly cleaner. BUG=webrtc:6355 ========== to ========== Replaces the SessionPresets with AVCaptureDeviceFormats. It eliminates the need for hardcoded enumerations of the supported presets. Hopefully it enables us to support slightly bigger set of resolutions and make the code slightly cleaner. BUG=webrtc:6355 Committed: https://crrev.com/4f15ca5d74cdb456d011efc3b133d62cbce8513c Cr-Commit-Position: refs/heads/master@{#14547} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4f15ca5d74cdb456d011efc3b133d62cbce8513c Cr-Commit-Position: refs/heads/master@{#14547} |
