Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2231033002:
Add support for more resolutions on iOS/macOS (Closed)
|
Created:
4 years, 4 months ago by kthelgason Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd support for more resolutions on iOS/macOS
BUG=
Committed: https://crrev.com/4a85abb80ea996ea1f8cb0c9bcbcfb41fdeec415
Cr-Commit-Position: refs/heads/master@{#13828}
Patch Set 1 #Patch Set 2 : rebase onto master #
Total comments: 8
Patch Set 3 : Represent capture format mappings with a struct #
Total comments: 11
Patch Set 4 : Avoid setting session preset twice. #
Total comments: 4
Patch Set 5 : Move struct member inline and fix up comments #
Total comments: 3
Patch Set 6 : Address review comments #Patch Set 7 : Set capture device framerate from VideoFormat #
Total comments: 3
Patch Set 8 : Persist device frame rate setting when switching inputs #Messages
Total messages: 33 (8 generated)
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1711) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/1734) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/9433) ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/11748) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/11679) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16990) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/15640) mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/5145) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7338)
kthelgason@webrtc.org changed reviewers: + magjed@webrtc.org, tkchin@webrtc.org
Please take a look. The trybots are having a bad time right now, but I think this is ready for initial review anyway.
It seems to be possible to get the resolution of presets low/medium/high (in a cumbersome way), see: http://stackoverflow.com/a/35490266 for more info. You don't have to do it in this CL, but we can consider it for a follow-up CL. I think adding support for 1280x720 is highest priority right now. https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (left): https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:594: [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this]; This looks like critical code we can't just remove? https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:38: if (preset == AVCaptureSessionPreset1280x720) { I would prefer if this logic was data driven instead and not spread out between kSupportedPresets, GetVideoFormatForSessionPreset, and GetSessionPresetForVideoFormat. So how about adding a static table like: struct AVCaptureSessionPresetResolution { NSString *sessionPreset; int width; int height; } static const kAVCaptureSessionPresetLookupTable[] = { {AVCaptureSessionPreset352x288, 352, 288}, {AVCaptureSessionPreset640x480, 640, 480}, {AVCaptureSessionPreset1280x720, 1280, 720}, }; and then implement GetVideoFormatForSessionPreset and GetSessionPresetForVideoFormat by looping over that table? https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:52: static cricket::VideoFormat const kIPhone4SFormat = This variable is unused now so you can remove it. I think we need to preserve the behavior that iPhone4S uses 352x288 instead of 640x480 until external apps have been updated to control the resolution themselves. https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:619: if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) { Can we use '_capturer.captureSession canSetSessionPreset' to filter out the supported formats instead of hardcoding it like this? You would need to move this code down under where _capturer is initialized.
https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (left): https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:594: [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this]; On 2016/08/11 12:34:07, magjed_webrtc wrote: > This looks like critical code we can't just remove? Ugh, sorry about that. I'd fixed this locally but somehow it didn't make it into the CL! https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:38: if (preset == AVCaptureSessionPreset1280x720) { I like that, That's a way better idea. Thanks! https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:52: static cricket::VideoFormat const kIPhone4SFormat = On 2016/08/11 12:34:07, magjed_webrtc wrote: > This variable is unused now so you can remove it. I think we need to preserve > the behavior that iPhone4S uses 352x288 instead of 640x480 until external apps > have been updated to control the resolution themselves. Acknowledged. https://codereview.webrtc.org/2231033002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:619: if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) { On 2016/08/11 12:34:07, magjed_webrtc wrote: > Can we use '_capturer.captureSession canSetSessionPreset' to filter out the > supported formats instead of hardcoding it like this? You would need to move > this code down under where _capturer is initialized. I did that initially but decided against it as I thought there might be some reason that it was initialized after this. If that's not the case then certainly that would be a better and more general solution. Will try it out.
https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:30: struct AVCaptureSessionPresetResolution { Some notes about this file: ObjC++ is gnarly and should be avoided as much as possible except to interop with WebRTC's video APIs. This file has both ObjC and C++ impls because previously everything was in one directory, and we wanted to hide the internal ObjC impl because no one should be using it. This is now a relic because we have a public header directory now (Framework/Headers/WebRTC). It would be better to split out RTCAVFoundationVideoCapturerInternal into its own file in this Classes/ private directory, and only use ObjC-isms there (ie use NSObject containers instead of structs). webrtc::AVFoundationVideoCapturer can then be just the ObjC++ bridge between ObjC and C++. Essentially, in objc/ dirs, avoid C++ unless you absolutely have to use it, otherwise we'll end up in a confusing ObjC++ tangle. https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:41: static const AVCaptureSessionPresetResolution kSupportedPresets[] = { Not all of these is supported for every device. The presets need to be queried at runtime. This file should be mac-compatible as well, so while we're adding these, might as well add some of the rest. kAvailablePresets? https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:604: int framerate = 30; There's no guarantee that you will get these frame rates. The presets will set a default and later if you want to use something different you need to set it on the device. https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:641: LOG(LS_ERROR) << "Unsupported video format."; do you need beginconfiguration to check session preset? Calls to begin should be matched with commit. https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:654: nit: remove blank line
https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:614: if ([_capturer.captureSession canSetSessionPreset:preset.sessionPreset]) { I'm not sure we want to go down the preset route just yet for doing supported formats, we probably should check what else the preset is setting. The usual way of iterating formats is: https://developer.apple.com/library/ios/documentation/AVFoundation/Reference/...
https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:30: struct AVCaptureSessionPresetResolution { On 2016/08/11 16:51:52, tkchin_webrtc wrote: > Essentially, in objc/ dirs, avoid C++ unless you absolutely have to use it, > otherwise we'll end up in a confusing ObjC++ tangle. That's good to know, thanks. I'll revise this. Perhaps splitting the classes up between files should be done in a separate CL? https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:41: static const AVCaptureSessionPresetResolution kSupportedPresets[] = { On 2016/08/11 16:51:52, tkchin_webrtc wrote: > Not all of these is supported for every device. The presets need to be queried > at runtime. This file should be mac-compatible as well, so while we're adding > these, might as well add some of the rest. > > kAvailablePresets? > Acknowledged. https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:604: int framerate = 30; On 2016/08/11 16:51:52, tkchin_webrtc wrote: > There's no guarantee that you will get these frame rates. The presets will set a > default and later if you want to use something different you need to set it on > the device. I understand. I'd pretty much come to that conclusion as well, but I kept it like this as the previous code sets these framerates explicitly. Would you suggest removing this always using 30 for the `cricket::VideoFormat`s? https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:641: LOG(LS_ERROR) << "Unsupported video format."; On 2016/08/11 16:51:52, tkchin_webrtc wrote: > do you need beginconfiguration to check session preset? Calls to begin should be > matched with commit. From Apple's documentation: "After calling beginConfiguration, you can for example add or remove outputs, alter the sessionPreset, or configure individual capture input or output properties". I will add a `commitConfiguration` call before the return in the failure case as well. https://codereview.webrtc.org/2231033002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:654: On 2016/08/11 16:51:52, tkchin_webrtc wrote: > nit: remove blank line Acknowledged.
https://codereview.webrtc.org/2231033002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (left): https://codereview.webrtc.org/2231033002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:374: captureSession.sessionPreset = preset; I don't see why we set the sessionPreset here, and again in `Start`. I tried removing this and everything _seems_ to work. Please let me know if/why this should be here.
On 2016/08/11 17:17:55, tkchin_webrtc wrote: > I'm not sure we want to go down the preset route just yet for doing supported > formats, we probably should check what else the preset is setting. I looked into the approach you linked to, and I agree that is the ideal way to do this. It is however quite a big change, and requires rewriting a lot of this code. Part of the issue is that we're actually dealing with two capture devices with different capabilities, and switch between them. This means that we can't just report what foramts one device supports, as the other might not. One possibility is enumerating all supported configurations for all attached devices, and finding the intersection of them. In any case, this will be a bit more work. I'm not opposed to going down that route though, if you think it's advisable.
On 2016/08/15 09:10:45, kthelgason wrote: > On 2016/08/11 17:17:55, tkchin_webrtc wrote: > > I'm not sure we want to go down the preset route just yet for doing supported > > formats, we probably should check what else the preset is setting. > > I looked into the approach you linked to, and I agree that is the ideal way to > do this. It is however quite a big change, and requires rewriting a lot of this > code. Part of the issue is that we're actually dealing with two capture devices > with different capabilities, and switch between them. This means that we can't > just report what foramts one device supports, as the other might not. > > One possibility is enumerating all supported configurations for all attached > devices, and finding the intersection of them. > > In any case, this will be a bit more work. I'm not opposed to going down that > route though, if you think it's advisable. I suggest we start of with this smaller CL to enumerate formats, and see how that works with the rest of the pipeline. Then we can follow up with a bigger refactoring where we split out RTCAVFoundationVideoCapturerInternal into its own file, and use AVCaptureDevice to enumerate and select capture formats. I think reporting the intersection of the supported formats for the front and back camera is a good start. Hopefully we just miss some high resolution formats, e.g. 1920x1080, for the back camera.
lgtm if you address my final comments. https://codereview.webrtc.org/2231033002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (left): https://codereview.webrtc.org/2231033002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:374: captureSession.sessionPreset = preset; On 2016/08/15 08:43:37, kthelgason wrote: > I don't see why we set the sessionPreset here, and again in `Start`. > I tried removing this and everything _seems_ to work. Please let me know if/why > this should be here. sessionPreset used to be set only from here, so you have simply moved it to 'Start'. I think that is fine. https://codereview.webrtc.org/2231033002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:34: cricket::VideoFormat getVideoFormat(int framerate) const { Structs are not allowed to have methods providing behavior. https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes I suggest you just inline this function and use preset.width/preset.height directly. https://codereview.webrtc.org/2231033002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:58: // mapping from cricket::VideoFormat to AVCaptureSession presets Super-nit: comments should have proper capitalization and punctuation. https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... This applies to your other comments as well.
Thanks for the review magjed, I've addressed your comments. I'm still unsure of what to do regarding the frame rate explicitly being set to 15 for iPhone 4S. It feels like something that definitely does not belong here. What are your thoughts?
On 2016/08/15 12:31:44, kthelgason wrote: > Thanks for the review magjed, I've addressed your comments. > I'm still unsure of what to do regarding the frame rate explicitly being set to > 15 for iPhone 4S. It feels like something that definitely does not belong here. > What are your thoughts? Right, it definitely does not belong there. If you want to fix it in this CL, you should use the framerate you receive from the format in Start() instead, i.e. cricket::VideoFormat::FpsToInterval(format.interval). The format passed to Start() is saved with SetCaptureFormat(), so you can access it again with GetCaptureFormat() from other functions. I don't know how to enumerate the framerate easily using sessionPreset, so just use 30 fps for all devices.
On 2016/08/15 13:41:41, magjed_webrtc wrote: > On 2016/08/15 12:31:44, kthelgason wrote: > > Thanks for the review magjed, I've addressed your comments. > > I'm still unsure of what to do regarding the frame rate explicitly being set > to > > 15 for iPhone 4S. It feels like something that definitely does not belong > here. > > What are your thoughts? > > Right, it definitely does not belong there. If you want to fix it in this CL, > you should use the framerate you receive from the format in Start() instead, > i.e. cricket::VideoFormat::FpsToInterval(format.interval). The format passed to > Start() is saved with SetCaptureFormat(), so you can access it again with > GetCaptureFormat() from other functions. I don't know how to enumerate the > framerate easily using sessionPreset, so just use 30 fps for all devices. The problem that this class was attempting to fix was that the constraints API does not work well with swapping front/rear cameras. On iOS you should not be recreating the capture session when you want to swap cameras - it causes a major noticeable delay. I'm not convinced that going back down the constraints route is a good long term solution unless it is reworked to address this issue. The short term fix to get everything working for front/rear was to just fix the capturer and ignore constraints temporarily. As for the 15fps iPhone 4S - you will never want more than 15fps on the 4S because it cannot handle it - it will certainly try, but quality will be low. I agree that application should be able to have more control over this, which is why I suggested being able to specify presets during the video source construction before.
lgtm to unblock, but please propose solution (or help me to understand your solution :)) to handle front/rear camera scenario together with constraints. For now using same resolution for both makes sense. re: 15fps, I think it's weird to make all webrtc clients have to manually tell us to use it since everyone will want to do that. Can we have it as a default optional constraint perhaps? (But agree resolution can be 480p, the 352x288 default should be removed completely). https://codereview.webrtc.org/2231033002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:593: [[RTCAVFoundationVideoCapturerInternal alloc] initWithCapturer:this]; indent 4 not 2 https://codereview.webrtc.org/2231033002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:634: NSString *desiredPreset = GetSessionPresetForVideoFormat(format); dcheck that you get a preset? https://codereview.webrtc.org/2231033002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:639: return cricket::CaptureState::CS_FAILED; commit configuration before return
On 2016/08/15 17:28:36, tkchin_webrtc wrote: > lgtm to unblock, but please propose solution (or help me to understand your > solution :)) to handle front/rear camera scenario together with constraints. For > now using same resolution for both makes sense. > > re: 15fps, I think it's weird to make all webrtc clients have to manually tell > us to use it since everyone will want to do that. Can we have it as a default > optional constraint perhaps? (But agree resolution can be 480p, the 352x288 > default should be removed completely). I addressed your comments and pushed, thanks! I've left the framerate stuff in the code for now, as there is really not a good solution for it until we move off the presets and start enumerating formats properly. Then we can decide whether to skip reporting 30fps configurations as supported on the 4S. My suggestion going forward is to refactor this code as suggested by tkchin, splitting it up into two files. Additionally, keep a reference to all AVCaptureDevices in use, and query these for all their supported formats. We then take the intersection of these for all(both) devices and report that as the set of supported formats. This way we will only report formats that can be used by any of the devices.
> The problem that this class was attempting to fix was that the constraints > API does not work well with swapping front/rear cameras. I don't understand the downside with constraints when swapping the front/rear camera compared to specifying a preset. Is it because you want to use e.g. AVCaptureSessionPresetMedium that can represent different resolutions for the front and rear camera? I think using the intersection of the supported formats is a good short term solution, but we could also apply the constraints separately on the front and rear camera formats if we want to implement it more proper. The only reason I'm advocating contraints is because it's already wired up and working. Re: 15fps, I would still prefer to leave the decision making to the WebRTC clients, but I see your point. There is currently no good way of setting a default format or default constraints. The VideoCapturerTrackSource filters the supported formats with the constraints and then selects the best of the remaining ones with GetBestCaptureFormat (https://cs.chromium.org/chromium/src/third_party/webrtc/api/videocapturertrac...). It would be easy to make GetBestCaptureFormat a virtual function in cricket::VideoCapturer that we can override for iOS, but if we want to go down that route it's probably better to just pass the constraints to AVFoundationVideoCapturer so that we can apply the constraints separately for the front and rear camera and use whatever default formats we want. I'm ok with capping the fps for iPhone4S to 15 for now, but I think it would be cleaner to use the framerate we get in the format passed to Start instead of hardcoding: #if TARGET_OS_IPHONE if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) { [self setMinFrameDuration:CMTimeMake(1, 15) forDevice:input.device]; } #endif in a couple of places. The framerate passed to Start will not be greater than 15 for iPhone4S as long as we report 15 fps in the supported formats, and then WebRTC clients can decide to go even lower using constraints if they want.
On 2016/08/16 13:08:12, magjed_webrtc wrote:
> I'm ok with capping the fps for iPhone4S to 15 for now, but I think it would
be
> cleaner to use the framerate we get in the format passed to Start instead of
> hardcoding:
> #if TARGET_OS_IPHONE
> if ([UIDevice deviceType] == RTCDeviceTypeIPhone4S) {
> [self setMinFrameDuration:CMTimeMake(1, 15) forDevice:input.device];
> }
> #endif
> in a couple of places. The framerate passed to Start will not be greater than
15
> for iPhone4S as long as we report 15 fps in the supported formats, and then
> WebRTC clients can decide to go even lower using constraints if they want.
I agree with this. The only special-case we need to handle is restricting the
formats we report as supported for the iPhone 4S.
I agree that using the intersection of resolutions for front/rear and constraints is a good short term solution as well. My concern is that we start with short term solution and then later have to do more heavy refactoring for long term solution, so just trying to understand how long term solution using constraints will work. Say constraints allow us to select different resolutions for front/rear camera somehow, maybe by introducing new keys. If at this point we can still keep one AVCaptureSession for fast front/rear camera swap then I have no complaints. My understanding of how constraints and device picking worked before is that a separate capturer would be created for each, but doing this on iOS causes bad camera switch performance. But my knowledge might be out of date. Maybe we can schedule a quick meeting? Feels like something we'd be able to sort out in 5-10 minutes of chatting. re: setting 15fps, the min frame duration needs to be set per device. It was in two places originally to make sure that it gets set correctly after device has been added/removed from the capture session. If we move it to one place, can we make sure that it behaves correctly on camera switch too? Thanks!
On 2016/08/16 15:20:24, tkchin_webrtc wrote: > ... Maybe we can > schedule a quick meeting? Feels like something we'd be able to sort out in 5-10 > minutes of chatting. Absolutely. Do you want to schedule a meeting tomorrow, perhaps with magjed as well? > re: setting 15fps, the min frame duration needs to be set per device. It was in > two places originally to make sure that it gets set correctly after device has > been added/removed from the capture session. If we move it to one place, can we > make sure that it behaves correctly on camera switch too? Thanks! Ok, I'll have a look at that as well. Thanks for pointing that out.
PTAL
lgtm https://codereview.webrtc.org/2231033002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:503: return; nit: continue might be more appropriate than return here. https://codereview.webrtc.org/2231033002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:653: const auto fps = cricket::VideoFormat::IntervalToFps(format.interval); nit: use int instead of auto here. We typically only use auto when the type is long and cluttered or clear from the context. https://google.github.io/styleguide/cppguide.html#auto
lgtm https://codereview.webrtc.org/2231033002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2231033002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:499: [AVCaptureDevice devicesWithMediaType:AVMediaTypeVideo]) { Seems odd to do this for every capture device. Could be laptop camera + other things. I guess it's unlikely that user has a lot of video devices though so it should be fine as long as this doesn't end up blocking the thread.
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2231033002/#ps140001 (title: "Persist device frame rate setting when switching inputs")
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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add support for more resolutions on iOS/macOS BUG= ========== to ========== Add support for more resolutions on iOS/macOS BUG= Committed: https://crrev.com/4a85abb80ea996ea1f8cb0c9bcbcfb41fdeec415 Cr-Commit-Position: refs/heads/master@{#13828} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4a85abb80ea996ea1f8cb0c9bcbcfb41fdeec415 Cr-Commit-Position: refs/heads/master@{#13828} |
