Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528493004:
Add method on AVFoundation capturer to adapt output format. (Closed)
|
Created:
4 years ago by kthelgason Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd method on AVFoundation capturer to adapt output format.
This CL makes a method available on the AVFoundationVideoCapturer
that adapts the output format of captured video to the specified
width and height.
BUG=webrtc:6753
Committed: https://crrev.com/2bc324cc5a97e051b45e8776e842d701bced7e2e
Cr-Commit-Position: refs/heads/master@{#15351}
Patch Set 1 #
Total comments: 13
Patch Set 2 : align colons in method name #
Messages
Total messages: 17 (5 generated)
kthelgason@webrtc.org changed reviewers: + denicija@webrtc.org, magjed@webrtc.org, tkchin@webrtc.org
Short and sweet, ptal :)
lgtm % nits https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm:35: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps { nit: I'm not sure if you need to line up the colons or not. https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:116: void AVFoundationVideoCapturer::AdaptOutputFormat(int width, int height, int fps) { nit: Too long lines.
https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm:35: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps { On 2016/11/23 13:53:40, magjed_webrtc wrote: > nit: I'm not sure if you need to line up the colons or not. The styleguide reads to me like you only need to do that if it's too long to fit on one line. From google obj-c guide: "If you have too many parameters to fit on one line, giving each its own line is preferred. If multiple lines are used, align each using the colon before the parameter." https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:116: void AVFoundationVideoCapturer::AdaptOutputFormat(int width, int height, int fps) { On 2016/11/23 13:53:40, magjed_webrtc wrote: > nit: Too long lines. Don't we have 100 char limits in objc files?
lgtm
lgtm https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm:35: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps { On 2016/11/23 14:01:48, kthelgason wrote: > On 2016/11/23 13:53:40, magjed_webrtc wrote: > > nit: I'm not sure if you need to line up the colons or not. > > The styleguide reads to me like you only need to do that if it's too long to fit > on one line. > > From google obj-c guide: > "If you have too many parameters to fit on one line, giving each its own line is > preferred. If multiple lines are used, align each using the colon before the > parameter." Acknowledged. https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:116: void AVFoundationVideoCapturer::AdaptOutputFormat(int width, int height, int fps) { On 2016/11/23 14:01:48, kthelgason wrote: > On 2016/11/23 13:53:40, magjed_webrtc wrote: > > nit: Too long lines. > > Don't we have 100 char limits in objc files? Yeah, maybe, but the rest of this file is indented at 80 char at least. I'll leave it up to Zeke.
lgtm just a few qns https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm:35: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps { On 2016/11/24 13:04:31, magjed_webrtc wrote: > On 2016/11/23 14:01:48, kthelgason wrote: > > On 2016/11/23 13:53:40, magjed_webrtc wrote: > > > nit: I'm not sure if you need to line up the colons or not. > > > > The styleguide reads to me like you only need to do that if it's too long to > fit > > on one line. > > > > From google obj-c guide: > > "If you have too many parameters to fit on one line, giving each its own line > is > > preferred. If multiple lines are used, align each using the colon before the > > parameter." > > Acknowledged. there is some leeway here. I would prefer them on separate lines because it's easier to read that way. https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:116: void AVFoundationVideoCapturer::AdaptOutputFormat(int width, int height, int fps) { On 2016/11/24 13:04:31, magjed_webrtc wrote: > On 2016/11/23 14:01:48, kthelgason wrote: > > On 2016/11/23 13:53:40, magjed_webrtc wrote: > > > nit: Too long lines. > > > > Don't we have 100 char limits in objc files? > > Yeah, maybe, but the rest of this file is indented at 80 char at least. I'll > leave it up to Zeke. 100char change came after the file was written. 100 is fine. https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Hea... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCAVFoundationVideoSource.h (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCAVFoundationVideoSource.h:40: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps; is there a way to do this without touching the capturer directly like this? ie what is the "WebRTC" way of doing things here
https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/RTCAVFoundationVideoSource.mm:35: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps { On 2016/11/30 22:08:52, tkchin_webrtc wrote: > On 2016/11/24 13:04:31, magjed_webrtc wrote: > > On 2016/11/23 14:01:48, kthelgason wrote: > > > On 2016/11/23 13:53:40, magjed_webrtc wrote: > > > > nit: I'm not sure if you need to line up the colons or not. > > > > > > The styleguide reads to me like you only need to do that if it's too long to > > fit > > > on one line. > > > > > > From google obj-c guide: > > > "If you have too many parameters to fit on one line, giving each its own > line > > is > > > preferred. If multiple lines are used, align each using the colon before the > > > parameter." > > > > Acknowledged. > > there is some leeway here. I would prefer them on separate lines because it's > easier to read that way. Acknowledged. https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:116: void AVFoundationVideoCapturer::AdaptOutputFormat(int width, int height, int fps) { On 2016/11/30 22:08:52, tkchin_webrtc wrote: > On 2016/11/24 13:04:31, magjed_webrtc wrote: > > On 2016/11/23 14:01:48, kthelgason wrote: > > > On 2016/11/23 13:53:40, magjed_webrtc wrote: > > > > nit: Too long lines. > > > > > > Don't we have 100 char limits in objc files? > > > > Yeah, maybe, but the rest of this file is indented at 80 char at least. I'll > > leave it up to Zeke. > > 100char change came after the file was written. 100 is fine. Acknowledged.
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, denicija@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2528493004/#ps20001 (title: "align colons in method name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Hea... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCAVFoundationVideoSource.h (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCAVFoundationVideoSource.h:40: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps; On 2016/11/30 22:08:52, tkchin_webrtc wrote: > is there a way to do this without touching the capturer directly like this? > ie what is the "WebRTC" way of doing things here I think this is the "WebRTC" way of doing it. We have pushed all cropping and scaling to the video source since it's most efficient to do it as early as possible (this was more a concern when we did like 5 frame copies in the pipeline before scaling). So the one scaler we have is a cricket::VideoAdapter in the video source. CPU overuse detector and QualityScaler will signal back to this cricket::VideoAdapter. I think it makes sense to also expose a direct way in the video source to control the VideoAdapter. This is how it's done for Android as well. One alternative might be to use media constraints in PeerConnectionFactory::CreateVideoSource to control this, but we are trying to move away from media constraints for that function, see https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectionfac.... We are trying to move towards a more simple and direct interface and leave the decision logic to the app. So the app has to select one of the supported resolutions from the capturer and can also control the VideoAdapter directly.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480583831164910,
"parent_rev": "ddea35978640f9d32687f8b1b1a8c8997c186744", "commit_rev":
"216df5e155198a1d4c84fffc6ff28ab1a22a10a3"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add method on AVFoundation capturer to adapt output format. This CL makes a method available on the AVFoundationVideoCapturer that adapts the output format of captured video to the specified width and height. BUG=webrtc:6753 ========== to ========== Add method on AVFoundation capturer to adapt output format. This CL makes a method available on the AVFoundationVideoCapturer that adapts the output format of captured video to the specified width and height. BUG=webrtc:6753 Committed: https://crrev.com/2bc324cc5a97e051b45e8776e842d701bced7e2e Cr-Commit-Position: refs/heads/master@{#15351} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2bc324cc5a97e051b45e8776e842d701bced7e2e Cr-Commit-Position: refs/heads/master@{#15351}
Message was sent while issue was closed.
https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Hea... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCAVFoundationVideoSource.h (right): https://codereview.webrtc.org/2528493004/diff/1/webrtc/sdk/objc/Framework/Hea... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCAVFoundationVideoSource.h:40: - (void)adaptOutputFormatToWidth:(int)width height:(int)height fps:(int)fps; On 2016/12/01 09:31:44, magjed_webrtc wrote: > On 2016/11/30 22:08:52, tkchin_webrtc wrote: > > is there a way to do this without touching the capturer directly like this? > > ie what is the "WebRTC" way of doing things here > > I think this is the "WebRTC" way of doing it. We have pushed all cropping and > scaling to the video source since it's most efficient to do it as early as > possible (this was more a concern when we did like 5 frame copies in the > pipeline before scaling). So the one scaler we have is a cricket::VideoAdapter > in the video source. CPU overuse detector and QualityScaler will signal back to > this cricket::VideoAdapter. I think it makes sense to also expose a direct way > in the video source to control the VideoAdapter. This is how it's done for > Android as well. > > One alternative might be to use media constraints in > PeerConnectionFactory::CreateVideoSource to control this, but we are trying to > move away from media constraints for that function, see > https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectionfac.... > We are trying to move towards a more simple and direct interface and leave the > decision logic to the app. So the app has to select one of the supported > resolutions from the capturer and can also control the VideoAdapter directly. I'm totally with you that mobile is a different world, and should be treated differently when needed, so this change looks good. I guess what I meant was, how would you do this in JS? And I guess the answer is, mobile is different. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
