Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271583003:
Implement CVO for iOS capturer (Closed)
|
Created:
4 years, 4 months ago by magjed_webrtc Modified:
4 years, 3 months ago Reviewers:
tkchin_webrtc 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. |
DescriptionImplement CVO for iOS capturer
The rotation is currently always applied by AVFoundation by
reconfiguring the capture connection video orientation. This CL sets the
rotation field in the frame instead. This avoids the current flash in
the video when the device is rotated, and also avoids reconfiguring the
local encoder and remote decoder when the device is rotated.
BUG=b/30651939
Committed: https://crrev.com/2ab012c41e57f21f380aec2c30818d3714fd0f6e
Cr-Commit-Position: refs/heads/master@{#13916}
Patch Set 1 : Move GN stuff. #
Total comments: 5
Patch Set 2 : Address tkchin@s comments #Patch Set 3 : Default to portrait orientation on iPhone #
Messages
Total messages: 18 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Implement CVO for iOS capturer The rotation is currently always applied by AVFoundation by reconfiguring the capture connection video orientation. This CL sets the rotation field in the frame instead. This avoids the current flash in the video when the device is rotated, and also avoids reconfiguring the local encoder and remote decoder when the device is rotated. BUG=b/30651939 ========== to ========== Implement CVO for iOS capturer The rotation is currently always applied by AVFoundation by reconfiguring the capture connection video orientation. This CL sets the rotation field in the frame instead. This avoids the current flash in the video when the device is rotated, and also avoids reconfiguring the local encoder and remote decoder when the device is rotated. BUG=b/30651939 ==========
magjed@webrtc.org changed reviewers: + tkchin@webrtc.org
Zeke - please take a look.
https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:59: void CaptureSampleBuffer(CMSampleBufferRef sampleBuffer, this wasn't you, but a relic from before the style rules were better known. mind renaming sampleBuffer to sample_buffer here? (C++ rules for C++ fn) https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:234: _rotation = webrtc::kVideoRotation_0; Can you add comment that this sets 0 rotation is orientation is unknown/faceup/facedown? What happens to local preview if say device was in landscape orientation, then set faceup/facedown on table?
https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:234: _rotation = webrtc::kVideoRotation_0; On 2016/08/23 17:33:19, tkchin_webrtc wrote: > Can you add comment that this sets 0 rotation is orientation is > unknown/faceup/facedown? > What happens to local preview if say device was in landscape orientation, then > set faceup/facedown on table? To clarify - what happens to an AVCapturePreviewLayer attached to the AVCaptureSession?
Patchset #2 (id:40001) has been deleted
https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:59: void CaptureSampleBuffer(CMSampleBufferRef sampleBuffer, On 2016/08/23 17:33:19, tkchin_webrtc wrote: > this wasn't you, but a relic from before the style rules were better known. > mind renaming sampleBuffer to sample_buffer here? (C++ rules for C++ fn) Done. https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:234: _rotation = webrtc::kVideoRotation_0; On 2016/08/23 17:37:25, tkchin_webrtc wrote: > On 2016/08/23 17:33:19, tkchin_webrtc wrote: > > Can you add comment that this sets 0 rotation is orientation is > > unknown/faceup/facedown? > > What happens to local preview if say device was in landscape orientation, then > > set faceup/facedown on table? > > To clarify - what happens to an AVCapturePreviewLayer attached to the > AVCaptureSession? Nothing happens when the device is set faceup/facedown with the new code. This is the behavior I observe for the old code as well. The only difference I can observe from the previous code overall is the brief flash when the device is rotated. I don't understand the purpose of the previous |_orientationHasChanged|. Was it only used to initialize a specific value for iPhone the first time updateOrientation is called and the orientation happens to be unknown/faceup/facedown? Do I need to preserve that behavior?
On 2016/08/24 11:08:36, magjed_webrtc wrote: > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:59: void > CaptureSampleBuffer(CMSampleBufferRef sampleBuffer, > On 2016/08/23 17:33:19, tkchin_webrtc wrote: > > this wasn't you, but a relic from before the style rules were better known. > > mind renaming sampleBuffer to sample_buffer here? (C++ rules for C++ fn) > > Done. > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:234: _rotation = > webrtc::kVideoRotation_0; > On 2016/08/23 17:37:25, tkchin_webrtc wrote: > > On 2016/08/23 17:33:19, tkchin_webrtc wrote: > > > Can you add comment that this sets 0 rotation is orientation is > > > unknown/faceup/facedown? > > > What happens to local preview if say device was in landscape orientation, > then > > > set faceup/facedown on table? > > > > To clarify - what happens to an AVCapturePreviewLayer attached to the > > AVCaptureSession? > > Nothing happens when the device is set faceup/facedown with the new code. This > is the behavior I observe for the old code as well. The only difference I can > observe from the previous code overall is the brief flash when the device is > rotated. > > I don't understand the purpose of the previous |_orientationHasChanged|. Was it > only used to initialize a specific value for iPhone the first time > updateOrientation is called and the orientation happens to be > unknown/faceup/facedown? Do I need to preserve that behavior? iirc that was that was there to initialize to portrait if orientation is faceup/facedown/unknown on init. It would be nice to preserve. This change may require clients to also make changes to their preview. Since we aren't changing the output of the capturer anymore, the preview layer will show unrotated frames since it is attached to the capture session, whereas the frames we send to remote will be rotated.
On 2016/08/24 17:21:24, tkchin_webrtc wrote: > On 2016/08/24 11:08:36, magjed_webrtc wrote: > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): > > > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:59: void > > CaptureSampleBuffer(CMSampleBufferRef sampleBuffer, > > On 2016/08/23 17:33:19, tkchin_webrtc wrote: > > > this wasn't you, but a relic from before the style rules were better known. > > > mind renaming sampleBuffer to sample_buffer here? (C++ rules for C++ fn) > > > > Done. > > > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:234: _rotation > = > > webrtc::kVideoRotation_0; > > On 2016/08/23 17:37:25, tkchin_webrtc wrote: > > > On 2016/08/23 17:33:19, tkchin_webrtc wrote: > > > > Can you add comment that this sets 0 rotation is orientation is > > > > unknown/faceup/facedown? > > > > What happens to local preview if say device was in landscape orientation, > > then > > > > set faceup/facedown on table? > > > > > > To clarify - what happens to an AVCapturePreviewLayer attached to the > > > AVCaptureSession? > > > > Nothing happens when the device is set faceup/facedown with the new code. This > > is the behavior I observe for the old code as well. The only difference I can > > observe from the previous code overall is the brief flash when the device is > > rotated. > > > > I don't understand the purpose of the previous |_orientationHasChanged|. Was > it > > only used to initialize a specific value for iPhone the first time > > updateOrientation is called and the orientation happens to be > > unknown/faceup/facedown? Do I need to preserve that behavior? > > iirc that was that was there to initialize to portrait if orientation is > faceup/facedown/unknown on init. It would be nice to preserve. > > This change may require clients to also make changes to their preview. Since we > aren't changing the output of the capturer anymore, the preview layer will show > unrotated frames since it is attached to the capture session, whereas the frames > we send to remote will be rotated. lgtm though
On 2016/08/24 17:21:24, tkchin_webrtc wrote: > On 2016/08/24 11:08:36, magjed_webrtc wrote: > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h (right): > > > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.h:59: void > > CaptureSampleBuffer(CMSampleBufferRef sampleBuffer, > > On 2016/08/23 17:33:19, tkchin_webrtc wrote: > > > this wasn't you, but a relic from before the style rules were better known. > > > mind renaming sampleBuffer to sample_buffer here? (C++ rules for C++ fn) > > > > Done. > > > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > > > > https://codereview.webrtc.org/2271583003/diff/20001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:234: _rotation > = > > webrtc::kVideoRotation_0; > > On 2016/08/23 17:37:25, tkchin_webrtc wrote: > > > On 2016/08/23 17:33:19, tkchin_webrtc wrote: > > > > Can you add comment that this sets 0 rotation is orientation is > > > > unknown/faceup/facedown? > > > > What happens to local preview if say device was in landscape orientation, > > then > > > > set faceup/facedown on table? > > > > > > To clarify - what happens to an AVCapturePreviewLayer attached to the > > > AVCaptureSession? > > > > Nothing happens when the device is set faceup/facedown with the new code. This > > is the behavior I observe for the old code as well. The only difference I can > > observe from the previous code overall is the brief flash when the device is > > rotated. > > > > I don't understand the purpose of the previous |_orientationHasChanged|. Was > it > > only used to initialize a specific value for iPhone the first time > > updateOrientation is called and the orientation happens to be > > unknown/faceup/facedown? Do I need to preserve that behavior? > > iirc that was that was there to initialize to portrait if orientation is > faceup/facedown/unknown on init. It would be nice to preserve. Done, I preserved it so that iPhone will use portrait as default. > This change may require clients to also make changes to their preview. Since we > aren't changing the output of the capturer anymore, the preview layer will show > unrotated frames since it is attached to the capture session, whereas the frames > we send to remote will be rotated. Sorry, I wasn't clear. AVCaptureVideoPreviewLayer will show the correct rotation just as before, so I don't think clients will have to do anything. At least our RTCCameraPreviewView implementation based on AVCaptureVideoPreviewLayer is unaffected by this change.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2271583003/#ps80001 (title: "Default to portrait orientation on iPhone")
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 ========== Implement CVO for iOS capturer The rotation is currently always applied by AVFoundation by reconfiguring the capture connection video orientation. This CL sets the rotation field in the frame instead. This avoids the current flash in the video when the device is rotated, and also avoids reconfiguring the local encoder and remote decoder when the device is rotated. BUG=b/30651939 ========== to ========== Implement CVO for iOS capturer The rotation is currently always applied by AVFoundation by reconfiguring the capture connection video orientation. This CL sets the rotation field in the frame instead. This avoids the current flash in the video when the device is rotated, and also avoids reconfiguring the local encoder and remote decoder when the device is rotated. BUG=b/30651939 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implement CVO for iOS capturer The rotation is currently always applied by AVFoundation by reconfiguring the capture connection video orientation. This CL sets the rotation field in the frame instead. This avoids the current flash in the video when the device is rotated, and also avoids reconfiguring the local encoder and remote decoder when the device is rotated. BUG=b/30651939 ========== to ========== Implement CVO for iOS capturer The rotation is currently always applied by AVFoundation by reconfiguring the capture connection video orientation. This CL sets the rotation field in the frame instead. This avoids the current flash in the video when the device is rotated, and also avoids reconfiguring the local encoder and remote decoder when the device is rotated. BUG=b/30651939 Committed: https://crrev.com/2ab012c41e57f21f380aec2c30818d3714fd0f6e Cr-Commit-Position: refs/heads/master@{#13916} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2ab012c41e57f21f380aec2c30818d3714fd0f6e Cr-Commit-Position: refs/heads/master@{#13916} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
