Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798993002:
Fixed that RTCCameraPreviewView did not rotate the video on device rotation. (Closed)
|
Created:
3 years, 8 months ago by meetAkshay99 Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixed that RTCCameraPreviewView did not rotate the video on device rotation.
BUG=webrtc:6749
Review-Url: https://codereview.webrtc.org/2798993002
Cr-Commit-Position: refs/heads/master@{#17742}
Committed: https://chromium.googlesource.com/external/webrtc/+/0d335c7756e172c38f5a86287aec7b62ff288be8
Patch Set 1 #Patch Set 2 : yes #
Total comments: 15
Patch Set 3 : y #Patch Set 4 : y #Patch Set 5 : y #Messages
Total messages: 43 (15 generated)
Description was changed from ========== Fixed that RTCCameraPreviewView did not rotate the video on device rotation. BUG=webrtc:6749 ========== to ========== Fixed that RTCCameraPreviewView did not rotate the video on device rotation. BUG=webrtc:6749 ==========
meetakshay99@gmail.com changed reviewers: + henrika@webrtc.org, magjed@webrtc.org, tkchin@webrtc.org
On 2017/04/05 17:04:19, meetAkshay99 wrote: Can you introduce some sort of settable mode on it instead? e.g. @property(nonatomic, assign) shouldAutoRotate and then it can listen to orientation directly
magjed@webrtc.org changed reviewers: - henrika@webrtc.org
Is the app responsible for calling setCorrectVideoOrientation on orientation changes with the current code? It would be nice if RTCCameraPreviewView could handle that itself.
On 2017/04/05 19:04:46, magjed_webrtc wrote: > Is the app responsible for calling setCorrectVideoOrientation on orientation > changes with the current code? It would be nice if RTCCameraPreviewView could > handle that itself. Have updated the patch to handle the orientation internally based on a property.
denicija@webrtc.org changed reviewers: + denicija@webrtc.org
Thanks for the patch. Some comments below https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:39: - (void) setShouldAutoRotate:(BOOL)shouldAutoRotate { Remove space. Hint: use `git cl format before` submitting. https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:40: if (shouldAutoRotate) { You are not assigning the synthesized ivar _shouldAutoRotate anywhere in the setter. Doesn't the compiler complain about this? https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:41: [[NSNotificationCenter defaultCenter] addObserver:self This seems like overhead - (Un)Registering for notifications etc inside the view. It's also finicky and can cause observations leaks (you are not removing the observer in dealloc for instance). It's common for UIViews to perform this kind of updates in `layoutSubview` and pass the responsibility for updating the layout upon orientation change to the superviews and their hosting controllers.
https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:40: if (shouldAutoRotate) { On 2017/04/06 09:45:37, daniela-webrtc wrote: > You are not assigning the synthesized ivar _shouldAutoRotate anywhere in the > setter. Doesn't the compiler complain about this? My bad. Yes thats required. https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:41: [[NSNotificationCenter defaultCenter] addObserver:self On 2017/04/06 09:45:37, daniela-webrtc wrote: > This seems like overhead - (Un)Registering for notifications etc inside the > view. It's also finicky and can cause observations leaks (you are not removing > the observer in dealloc for instance). > It's common for UIViews to perform this kind of updates in `layoutSubview` and > pass the responsibility for updating the layout upon orientation change to the > superviews and their hosting controllers. That is what I was having earlier. A method was defined and the view controller was responsible for calling that method to make sure the video orientation is correct. However, Other reviewers commented that RTCCameraPreviewView should handle it internally. Hence I went by this approach. And yes, the observer should had been removed in dealloc too.
https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:41: [[NSNotificationCenter defaultCenter] addObserver:self On 2017/04/06 10:24:34, meetAkshay99 wrote: > On 2017/04/06 09:45:37, daniela-webrtc wrote: > > This seems like overhead - (Un)Registering for notifications etc inside the > > view. It's also finicky and can cause observations leaks (you are not removing > > the observer in dealloc for instance). > > It's common for UIViews to perform this kind of updates in `layoutSubview` and > > pass the responsibility for updating the layout upon orientation change to the > > superviews and their hosting controllers. > > That is what I was having earlier. A method was defined and the view controller > was responsible for calling that method to make sure the video orientation is > correct. However, Other reviewers commented that RTCCameraPreviewView should > handle it internally. Hence I went by this approach. > And yes, the observer should had been removed in dealloc too. I think the other reviewers meant that there is no reason to expose specific method for this purpose other than the _shouldAutoRotate property and that RTCCameraView should handle the rotations internally. I agree with that. My suggestion was to check the device orientation in `layoutSubviews`. Basically: setCorrectVideoOrientation -> layoutSubviews.
tkchin@webrtc.org changed reviewers: + haysc@webrtc.org
@haysc can you take a look I don't remember if the connection affects the capture session as well, or just the connection to the preview layer. https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:39: - (void) setShouldAutoRotate:(BOOL)shouldAutoRotate { On 2017/04/06 09:45:37, daniela-webrtc wrote: > Remove space. > Hint: use `git cl format before` submitting. Is that what your team has decided on now? Is there a .clang-format somewhere?
> I don't remember if the connection affects the capture session as well, or just > the connection to the preview layer. Just the connection as far as I know. Setting that property is what we use to handle rotation. We do it in response to layoutSubviews being called as a result of rotation.
https://codereview.webrtc.org/2798993002/diff/20001/AUTHORS File AUTHORS (right): https://codereview.webrtc.org/2798993002/diff/20001/AUTHORS#newcode4 AUTHORS:4: Akshay Shah <meetakshay99@gmail.com> This should be after Adam Fedor (alpha order) https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:60: if (deviceOrientation == UIInterfaceOrientationPortraitUpsideDown) { This header docs say you should only set this property if isVideoOrientationSupported is YES. "This property is only applicable to AVCaptureConnection instances involving video. In such connections, the videoOrientation property may only be set if -isVideoOrientationSupported returns YES." https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:61: [previewLayer.connection setVideoOrientation:AVCaptureVideoOrientationPortraitUpsideDown]; Please use property syntax here: previewLayer.connection.videoOrientation =
https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:39: - (void) setShouldAutoRotate:(BOOL)shouldAutoRotate { On 2017/04/06 09:45:37, daniela-webrtc wrote: > Remove space. > Hint: use `git cl format before` submitting. I tried that but got following error: YAML:8:11: error: unknown enumerated scalar Language: ObjC ^~~~ Error reading /Users/akshayshah/WebRTC/src/.clang-format: Invalid argument https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:41: [[NSNotificationCenter defaultCenter] addObserver:self On 2017/04/06 14:44:46, daniela-webrtc wrote: > On 2017/04/06 10:24:34, meetAkshay99 wrote: > > On 2017/04/06 09:45:37, daniela-webrtc wrote: > > > This seems like overhead - (Un)Registering for notifications etc inside the > > > view. It's also finicky and can cause observations leaks (you are not > removing > > > the observer in dealloc for instance). > > > It's common for UIViews to perform this kind of updates in `layoutSubview` > and > > > pass the responsibility for updating the layout upon orientation change to > the > > > superviews and their hosting controllers. > > > > That is what I was having earlier. A method was defined and the view > controller > > was responsible for calling that method to make sure the video orientation is > > correct. However, Other reviewers commented that RTCCameraPreviewView should > > handle it internally. Hence I went by this approach. > > And yes, the observer should had been removed in dealloc too. > > I think the other reviewers meant that there is no reason to expose specific > method for this purpose other than the _shouldAutoRotate property and that > RTCCameraView should handle the rotations internally. > I agree with that. My suggestion was to check the device orientation in > `layoutSubviews`. > Basically: setCorrectVideoOrientation -> layoutSubviews. Yeah, updating to call setCorrectVideoOrientation from layoutSubviews. I agree, its a better solution than handling device orientation notifications. https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:60: if (deviceOrientation == UIInterfaceOrientationPortraitUpsideDown) { On 2017/04/06 19:06:46, Chuck wrote: > This header docs say you should only set this property if > isVideoOrientationSupported is YES. > > "This property is only applicable to AVCaptureConnection instances involving > video. In such connections, the videoOrientation property may only be set if > -isVideoOrientationSupported returns YES." Added a check for the same. https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:61: [previewLayer.connection setVideoOrientation:AVCaptureVideoOrientationPortraitUpsideDown]; On 2017/04/06 19:06:46, Chuck wrote: > Please use property syntax here: > previewLayer.connection.videoOrientation = I would update it to property syntax, but would like to know - is there a specific reason for doing so?
https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:61: [previewLayer.connection setVideoOrientation:AVCaptureVideoOrientationPortraitUpsideDown]; On 2017/04/07 05:19:29, meetAkshay99 wrote: > On 2017/04/06 19:06:46, Chuck wrote: > > Please use property syntax here: > > previewLayer.connection.videoOrientation = > > I would update it to property syntax, but would like to know - is there a > specific reason for doing so? It's modern objective-c syntax and to match the style of the rest of the code.
On 2017/04/06 17:39:22, tkchin_webrtc wrote: > @haysc can you take a look > > I don't remember if the connection affects the capture session as well, or just > the connection to the preview layer. > > https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): > > https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:39: - (void) > setShouldAutoRotate:(BOOL)shouldAutoRotate { > On 2017/04/06 09:45:37, daniela-webrtc wrote: > > Remove space. > > Hint: use `git cl format before` submitting. > > Is that what your team has decided on now? Is there a .clang-format somewhere? Yes there was .clang-format file before that was unsuitable for us to use because clang had no support for cpp and objc configurations. That changed from Clang 5.0 where now it's possible to specify objc specific style-guides. It's a recent change https://codereview.webrtc.org/2775613002/ and we're still testing it out, but looks promising.
On 2017/04/10 07:55:02, daniela-webrtc wrote: > On 2017/04/06 17:39:22, tkchin_webrtc wrote: > > @haysc can you take a look > > > > I don't remember if the connection affects the capture session as well, or > just > > the connection to the preview layer. > > > > > https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m (right): > > > > > https://codereview.webrtc.org/2798993002/diff/20001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m:39: - (void) > > setShouldAutoRotate:(BOOL)shouldAutoRotate { > > On 2017/04/06 09:45:37, daniela-webrtc wrote: > > > Remove space. > > > Hint: use `git cl format before` submitting. > > > > Is that what your team has decided on now? Is there a .clang-format somewhere? > > Yes there was .clang-format file before that was unsuitable for us to use > because clang had no support for cpp and objc configurations. That changed from > Clang 5.0 where now it's possible to specify objc specific style-guides. > It's a recent change https://codereview.webrtc.org/2775613002/ and we're still > testing it out, but looks promising. So now, does this change look good for commiting to the master branch?
lgtm
On 2017/04/12 11:31:22, daniela-webrtc wrote: > lgtm Do I have to wait until everyone in the reviewer list replies as "lgtm"? Or I can go ahead for merge with one lgtm?
On 2017/04/14 14:11:24, meetAkshay99 wrote: > On 2017/04/12 11:31:22, daniela-webrtc wrote: > > lgtm > > Do I have to wait until everyone in the reviewer list replies as "lgtm"? Or I > can go ahead for merge with one lgtm? You should be ok to proceed. lgtm as well
The CQ bit was checked by meetakshay99@gmail.com 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 meetakshay99@gmail.com
The CQ bit was unchecked by meetakshay99@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/04/14 16:23:18, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... the commit failed with message: CQ rejected the patch (CQ bit was unchecked.) Can someone please have a look. I am not sure how to fix this.
On 2017/04/17 04:34:56, meetAkshay99 wrote: > On 2017/04/14 16:23:18, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... > > the commit failed with message: > > CQ rejected the patch > (CQ bit was unchecked.) > > Can someone please have a look. I am not sure how to fix this. When trying to do "git cl land" I got following error: ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m who is the owner of this file?
On 2017/04/17 13:13:31, meetAkshay99 wrote: > On 2017/04/17 04:34:56, meetAkshay99 wrote: > > On 2017/04/14 16:23:18, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... > > > > the commit failed with message: > > > > CQ rejected the patch > > (CQ bit was unchecked.) > > > > Can someone please have a look. I am not sure how to fix this. > > When trying to do "git cl land" I got following error: > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for these files: > webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m > > who is the owner of this file? Looks like you need tkchin@webrtc.org: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/sdk/objc/OW...
meetakshay99@gmail.com changed reviewers: + meetakshay99@gmail.com, tkchin@chromium.org - meetAkshay99@gmail.com
On 2017/04/17 13:50:11, Chuck wrote: > On 2017/04/17 13:13:31, meetAkshay99 wrote: > > On 2017/04/17 04:34:56, meetAkshay99 wrote: > > > On 2017/04/14 16:23:18, commit-bot: I haz the power wrote: > > > > CQ is trying da patch. Follow status at > > > > > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... > > > > > > the commit failed with message: > > > > > > CQ rejected the patch > > > (CQ bit was unchecked.) > > > > > > Can someone please have a look. I am not sure how to fix this. > > > > When trying to do "git cl land" I got following error: > > > > ** Presubmit ERRORS ** > > Missing LGTM from an OWNER for these files: > > webrtc/sdk/objc/Framework/Classes/RTCCameraPreviewView.m > > > > who is the owner of this file? > > Looks like you need mailto:tkchin@webrtc.org: > > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/sdk/objc/OW... @tkchin Can you please review and approve the changes so that I can merge them to master branch?
The CQ bit was checked by magjed@webrtc.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492521460276570,
"parent_rev": "ba6aa90c0417ebe30bd674f34c5180c74932f3df", "commit_rev":
"0d335c7756e172c38f5a86287aec7b62ff288be8"}
Message was sent while issue was closed.
Description was changed from ========== Fixed that RTCCameraPreviewView did not rotate the video on device rotation. BUG=webrtc:6749 ========== to ========== Fixed that RTCCameraPreviewView did not rotate the video on device rotation. BUG=webrtc:6749 Review-Url: https://codereview.webrtc.org/2798993002 Cr-Commit-Position: refs/heads/master@{#17742} Committed: https://chromium.googlesource.com/external/webrtc/+/0d335c7756e172c38f5a86287... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/0d335c7756e172c38f5a86287...
Message was sent while issue was closed.
On 2017/04/05 17:37:53, tkchin_webrtc wrote: > On 2017/04/05 17:04:19, meetAkshay99 wrote: > > Can you introduce some sort of settable mode on it instead? > e.g. @property(nonatomic, assign) shouldAutoRotate > > and then it can listen to orientation directly This is breaking my app. My app embeds iOS framework and works only on landscape orientation. With this change, it forces the camera view orientation to portrait mode all the time. Probably some timing issue when this code is called. For now I commented this change out locally to workaround the issue. --Suman
Message was sent while issue was closed.
On 2017/05/01 03:57:42, suman wrote: > On 2017/04/05 17:37:53, tkchin_webrtc wrote: > > On 2017/04/05 17:04:19, meetAkshay99 wrote: > > > > Can you introduce some sort of settable mode on it instead? > > e.g. @property(nonatomic, assign) shouldAutoRotate > > > > and then it can listen to orientation directly > > This is breaking my app. My app embeds iOS framework and works only on > landscape orientation. With this change, it forces the camera view orientation > to portrait mode all the time. Probably some timing issue when this code is > called. > > For now I commented this change out locally to workaround the issue. > > --Suman The behavior of this class should've been made optional to avoid breakage like you mention Suman. @magjed can you fix / revert? @haysc does this break us?
Message was sent while issue was closed.
On 2017/05/01 22:36:21, tkchin_webrtc wrote: > On 2017/05/01 03:57:42, suman wrote: > > On 2017/04/05 17:37:53, tkchin_webrtc wrote: > > > On 2017/04/05 17:04:19, meetAkshay99 wrote: > > > > > > Can you introduce some sort of settable mode on it instead? > > > e.g. @property(nonatomic, assign) shouldAutoRotate > > > > > > and then it can listen to orientation directly > > > > This is breaking my app. My app embeds iOS framework and works only on > > landscape orientation. With this change, it forces the camera view > orientation > > to portrait mode all the time. Probably some timing issue when this code is > > called. > > > > For now I commented this change out locally to workaround the issue. > > > > --Suman > > The behavior of this class should've been made optional to avoid breakage like > you mention Suman. > > @magjed can you fix / revert? > @haysc does this break us? I posted a fix that allows this to be turned off: https://codereview.webrtc.org/2859573002/ @tkchin - Might break AppRTCMobile, I can't test because Apprtc is down. Not used in our app though. |
