Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964703002:
[iOS] Fix incorrectly oriented frames when rapidly switching between cameras. (Closed)
|
Created:
3 years, 5 months ago by jtt_webrtc Modified:
3 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, tkchin_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
Description[iOS] Fix incorrectly oriented frames when rapidly switching between cameras.
During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down.
There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using.
So use the active input's camera to check instead of the client state.
BUG=webrtc:7898
Review-Url: https://codereview.webrtc.org/2964703002
Cr-Commit-Position: refs/heads/master@{#19139}
Committed: https://chromium.googlesource.com/external/webrtc/+/61b0ed039dcd937d92d49a666c504a0eb2b475c6
Patch Set 1 #
Total comments: 8
Patch Set 2 : During a call, with both phones in horizontal or landscape mode, rapidly switching between the fron… #Patch Set 3 : Used a new flag _changingCamera to gate the frame handling instead. #
Total comments: 1
Patch Set 4 : Check the EXIF of the image for which camera it came from. #Patch Set 5 : Added unit tests for frame camera different from connection camera. #Patch Set 6 : Formatting #Patch Set 7 : Merge branch 'master' of https://chromium.googlesource.com/external/webrtc into exif #Patch Set 8 : Change true/false to YES/NO #
Total comments: 4
Patch Set 9 : Moved common code to new class. #
Total comments: 5
Patch Set 10 : Added test to test isFrontCameraFromSampleBuffer. #Patch Set 11 : Renamed RTCImageHelper method, removed gate check for switching cameras. #
Total comments: 20
Patch Set 12 : Fix method rename. #
Total comments: 1
Patch Set 13 : use gate keeper for getting exif. #
Total comments: 1
Patch Set 14 : Use AcquireLoad, renamed RTCImageHelper to AVCaptureSession Private. #
Total comments: 3
Patch Set 15 : Removed gate keeper. Renamed AVCaptureSession+Private to AVCaptureSession+Device. #
Total comments: 2
Patch Set 16 : Renamed RTCCameraVideoCapturer.mm back to RTCCameraVideoCapturer.m. #
Total comments: 4
Patch Set 17 : Refactoring of checking for image device source. #Patch Set 18 : Refactored checking of image's source device. #
Total comments: 3
Messages
Total messages: 68 (17 generated)
Description was changed from ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG= ========== to ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 Use this cl instead of 2966563002 ==========
jtteh@webrtc.org changed reviewers: + magjed@webrtc.org
I had to add additional checks to make sure we're not using frames while we're switching the inputs.
https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: _isRunning = false; Why is this needed? https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:140: _isRunning = true; Same here, why is this change needed? https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:191: if (!_isRunning || !_willBeRunning || Why do we need these extra checks? What happens otherwise?
https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: _isRunning = false; On 2017/06/30 17:50:50, magjed_webrtc wrote: > Why is this needed? We need to stop frames from being processed until the inputs have been switched over. Otherwise, we still see the upside down video artifact. https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:140: _isRunning = true; On 2017/06/30 17:50:50, magjed_webrtc wrote: > Same here, why is this change needed? See above. We don't want to process frames until the switch over is complete. If there was an error, the switch didn't happen, so we continue running. https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:191: if (!_isRunning || !_willBeRunning || On 2017/06/30 17:50:50, magjed_webrtc wrote: > Why do we need these extra checks? What happens otherwise? See above, we still get the flicker but more rare.
https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: _isRunning = false; On 2017/06/30 17:50:50, magjed_webrtc wrote: > Why is this needed? I removed this check and the flicker came back. The switch over now has a very momentary black frame when it switches cameras.
https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: _isRunning = false; On 2017/06/30 18:07:47, jtt wrote: > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > Why is this needed? > > I removed this check and the flicker came back. > > The switch over now has a very momentary black frame when it switches cameras. Why does it flicker if we remove this? I thought it would be synchronized by the fact that we check from what camera device each frame comes from. And why do we need the other checks _willBeRunning, connection.enabled, and connection.active? I don't want to sprinkle new conditions without understanding what's happening.
Description was changed from ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 Use this cl instead of 2966563002 ========== to ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 Replaced with https://codereview.webrtc.org/2970983002 ==========
Description was changed from ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 Replaced with https://codereview.webrtc.org/2970983002 ========== to ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. ### Replaced with https://codereview.webrtc.org/2970983002/ BUG=webrtc:7898 ==========
On 2017/07/03 08:23:46, magjed_webrtc wrote: > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > (right): > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > _isRunning = false; > On 2017/06/30 18:07:47, jtt wrote: > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > Why is this needed? > > > > I removed this check and the flicker came back. > > > > The switch over now has a very momentary black frame when it switches cameras. > > Why does it flicker if we remove this? I thought it would be synchronized by the > fact that we check from what camera device each frame comes from. And why do we > need the other checks _willBeRunning, connection.enabled, and connection.active? > I don't want to sprinkle new conditions without understanding what's happening. See the other cl - https://codereview.webrtc.org/2970983002/ To answer your question, we delete the old inputs, then add the new input for the new camera. In the short period between deleting the old input and adding the new input, we are still getting frames from the old input and the new input before the connections are created for the input to the output. So the old AVCaptureConnection may still be valid when getting new frames from the new input.
On 2017/07/05 16:54:52, jtt wrote: > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > (right): > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > _isRunning = false; > > On 2017/06/30 18:07:47, jtt wrote: > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > Why is this needed? > > > > > > I removed this check and the flicker came back. > > > > > > The switch over now has a very momentary black frame when it switches > cameras. > > > > Why does it flicker if we remove this? I thought it would be synchronized by > the > > fact that we check from what camera device each frame comes from. And why do > we > > need the other checks _willBeRunning, connection.enabled, and > connection.active? > > I don't want to sprinkle new conditions without understanding what's > happening. > > See the other cl - https://codereview.webrtc.org/2970983002/ > > To answer your question, we delete the old inputs, then add the new input for > the new camera. In the short period between deleting the old input and adding > the new input, we are still getting frames from the old input and the new input > before the connections are created for the input to the output. > > So the old AVCaptureConnection may still be valid when getting new frames from > the new input. I understand we might get frames from the previous connection while we re-configure, but if the previous AVCaptureConnection is valid why is this a problem? Why would any frame be shown upside down? And my question is also why it's not enough to synchronzie with |_isRunning|? Why do we also need to check _willBeRunning, connection.enabled, and connection.active? Also, please upload new patch sets to the existing CL instead of creating new ones.
Patchset #2 (id:20001) has been deleted
On 2017/07/06 08:12:52, magjed_webrtc wrote: > On 2017/07/05 16:54:52, jtt wrote: > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > File > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > (right): > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > _isRunning = false; > > > On 2017/06/30 18:07:47, jtt wrote: > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > Why is this needed? > > > > > > > > I removed this check and the flicker came back. > > > > > > > > The switch over now has a very momentary black frame when it switches > > cameras. > > > > > > Why does it flicker if we remove this? I thought it would be synchronized by > > the > > > fact that we check from what camera device each frame comes from. And why do > > we > > > need the other checks _willBeRunning, connection.enabled, and > > connection.active? > > > I don't want to sprinkle new conditions without understanding what's > > happening. > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > To answer your question, we delete the old inputs, then add the new input for > > the new camera. In the short period between deleting the old input and adding > > the new input, we are still getting frames from the old input and the new > input > > before the connections are created for the input to the output. > > > > So the old AVCaptureConnection may still be valid when getting new frames from > > the new input. > > I understand we might get frames from the previous connection while we > re-configure, but if the previous AVCaptureConnection is valid why is this a > problem? Why would any frame be shown upside down? > > And my question is also why it's not enough to synchronzie with |_isRunning|? > Why do we also need to check _willBeRunning, connection.enabled, and > connection.active? > > Also, please upload new patch sets to the existing CL instead of creating new > ones. I added some logging and what I see is that between when the switch camera button is pressed and the camera inputs are switched, frames are still being processed by captureOutput:didOutputSampleBuffer:fromConnection:. The log shows that a frame is processed with the previous camera, then with the new camera. The camera is obtained from connection.inputPorts.firstObject).input. For example, switching from BACK to FRONT camera, the logs are: CAMERA LOG: rotation: 180 usingFrontCamera: BACK CAMERA LOG: rotation: 0 usingFrontCamera: FRONT CAMERA LOG: rotation: 0 usingFrontCamera: FRONT So, IF the first frame is actually from the FRONT camera and not the BACK camera, the frame will be marked with the wrong rotation, ie. it will appear upside down. Following is speculation and what I'm guessing is happening.... When we call removeInput, then addInput, the connections are automatically created by iOS. In the short time interval between addInput and the connection created, a frame from the FRONT camera is received, but the previous connection that says is the BACK camera is received. So, we get an incorrectly labeled rotation for the frame. As this artifact happens about one in ten or twenty tries, it is probably due to a timing/race condition like the above.
Description was changed from ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. ### Replaced with https://codereview.webrtc.org/2970983002/ BUG=webrtc:7898 ========== to ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 ==========
On 2017/07/06 15:53:21, jtt wrote: > On 2017/07/06 08:12:52, magjed_webrtc wrote: > > On 2017/07/05 16:54:52, jtt wrote: > > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > File > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > > (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > > _isRunning = false; > > > > On 2017/06/30 18:07:47, jtt wrote: > > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > > Why is this needed? > > > > > > > > > > I removed this check and the flicker came back. > > > > > > > > > > The switch over now has a very momentary black frame when it switches > > > cameras. > > > > > > > > Why does it flicker if we remove this? I thought it would be synchronized > by > > > the > > > > fact that we check from what camera device each frame comes from. And why > do > > > we > > > > need the other checks _willBeRunning, connection.enabled, and > > > connection.active? > > > > I don't want to sprinkle new conditions without understanding what's > > > happening. > > > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > > > To answer your question, we delete the old inputs, then add the new input > for > > > the new camera. In the short period between deleting the old input and > adding > > > the new input, we are still getting frames from the old input and the new > > input > > > before the connections are created for the input to the output. > > > > > > So the old AVCaptureConnection may still be valid when getting new frames > from > > > the new input. > > > > I understand we might get frames from the previous connection while we > > re-configure, but if the previous AVCaptureConnection is valid why is this a > > problem? Why would any frame be shown upside down? > > > > And my question is also why it's not enough to synchronzie with |_isRunning|? > > Why do we also need to check _willBeRunning, connection.enabled, and > > connection.active? > > > > Also, please upload new patch sets to the existing CL instead of creating new > > ones. > > I added some logging and what I see is that between when the switch camera > button is pressed and the camera inputs are switched, frames are still being > processed by captureOutput:didOutputSampleBuffer:fromConnection:. > > The log shows that a frame is processed with the previous camera, then with the > new camera. The camera is obtained from > connection.inputPorts.firstObject).input. > > For example, switching from BACK to FRONT camera, the logs are: > CAMERA LOG: rotation: 180 usingFrontCamera: BACK > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > So, IF the first frame is actually from the FRONT camera and not the BACK > camera, the frame will be marked with the wrong rotation, ie. it will appear > upside down. > > Following is speculation and what I'm guessing is happening.... > When we call removeInput, then addInput, the connections are automatically > created by iOS. In the short time interval between addInput and the connection > created, a frame from the FRONT camera is received, but the previous connection > that says is the BACK camera is received. > So, we get an incorrectly labeled rotation for the frame. > > As this artifact happens about one in ten or twenty tries, it is probably due to > a timing/race condition like the above. Ok, can you do dispatchAsyncOnType:RTCDispatcherTypeCaptureSession from captureOutput:didOutputSampleBuffer:fromConnection: to synchronize it with the reconfig and see if that solves the problem?
On 2017/07/07 08:28:32, magjed_webrtc wrote: > On 2017/07/06 15:53:21, jtt wrote: > > On 2017/07/06 08:12:52, magjed_webrtc wrote: > > > On 2017/07/05 16:54:52, jtt wrote: > > > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > File > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > > > _isRunning = false; > > > > > On 2017/06/30 18:07:47, jtt wrote: > > > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > > > Why is this needed? > > > > > > > > > > > > I removed this check and the flicker came back. > > > > > > > > > > > > The switch over now has a very momentary black frame when it switches > > > > cameras. > > > > > > > > > > Why does it flicker if we remove this? I thought it would be > synchronized > > by > > > > the > > > > > fact that we check from what camera device each frame comes from. And > why > > do > > > > we > > > > > need the other checks _willBeRunning, connection.enabled, and > > > > connection.active? > > > > > I don't want to sprinkle new conditions without understanding what's > > > > happening. > > > > > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > > > > > To answer your question, we delete the old inputs, then add the new input > > for > > > > the new camera. In the short period between deleting the old input and > > adding > > > > the new input, we are still getting frames from the old input and the new > > > input > > > > before the connections are created for the input to the output. > > > > > > > > So the old AVCaptureConnection may still be valid when getting new frames > > from > > > > the new input. > > > > > > I understand we might get frames from the previous connection while we > > > re-configure, but if the previous AVCaptureConnection is valid why is this a > > > problem? Why would any frame be shown upside down? > > > > > > And my question is also why it's not enough to synchronzie with > |_isRunning|? > > > Why do we also need to check _willBeRunning, connection.enabled, and > > > connection.active? > > > > > > Also, please upload new patch sets to the existing CL instead of creating > new > > > ones. > > > > I added some logging and what I see is that between when the switch camera > > button is pressed and the camera inputs are switched, frames are still being > > processed by captureOutput:didOutputSampleBuffer:fromConnection:. > > > > The log shows that a frame is processed with the previous camera, then with > the > > new camera. The camera is obtained from > > connection.inputPorts.firstObject).input. > > > > For example, switching from BACK to FRONT camera, the logs are: > > CAMERA LOG: rotation: 180 usingFrontCamera: BACK > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > So, IF the first frame is actually from the FRONT camera and not the BACK > > camera, the frame will be marked with the wrong rotation, ie. it will appear > > upside down. > > > > Following is speculation and what I'm guessing is happening.... > > When we call removeInput, then addInput, the connections are automatically > > created by iOS. In the short time interval between addInput and the connection > > created, a frame from the FRONT camera is received, but the previous > connection > > that says is the BACK camera is received. > > So, we get an incorrectly labeled rotation for the frame. > > > > As this artifact happens about one in ten or twenty tries, it is probably due > to > > a timing/race condition like the above. > > Ok, can you do dispatchAsyncOnType:RTCDispatcherTypeCaptureSession from > captureOutput:didOutputSampleBuffer:fromConnection: to synchronize it with the > reconfig and see if that solves the problem? The sample buffer passed in needs to be in the same queue, otherwise it goes out of scope. I get crashes when accessing the sampleBuffer attributes within the dispatch queue.
On 2017/07/07 15:37:30, jtt wrote: > On 2017/07/07 08:28:32, magjed_webrtc wrote: > > On 2017/07/06 15:53:21, jtt wrote: > > > On 2017/07/06 08:12:52, magjed_webrtc wrote: > > > > On 2017/07/05 16:54:52, jtt wrote: > > > > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > File > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > > > > _isRunning = false; > > > > > > On 2017/06/30 18:07:47, jtt wrote: > > > > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > > > > Why is this needed? > > > > > > > > > > > > > > I removed this check and the flicker came back. > > > > > > > > > > > > > > The switch over now has a very momentary black frame when it > switches > > > > > cameras. > > > > > > > > > > > > Why does it flicker if we remove this? I thought it would be > > synchronized > > > by > > > > > the > > > > > > fact that we check from what camera device each frame comes from. And > > why > > > do > > > > > we > > > > > > need the other checks _willBeRunning, connection.enabled, and > > > > > connection.active? > > > > > > I don't want to sprinkle new conditions without understanding what's > > > > > happening. > > > > > > > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > > > > > > > To answer your question, we delete the old inputs, then add the new > input > > > for > > > > > the new camera. In the short period between deleting the old input and > > > adding > > > > > the new input, we are still getting frames from the old input and the > new > > > > input > > > > > before the connections are created for the input to the output. > > > > > > > > > > So the old AVCaptureConnection may still be valid when getting new > frames > > > from > > > > > the new input. > > > > > > > > I understand we might get frames from the previous connection while we > > > > re-configure, but if the previous AVCaptureConnection is valid why is this > a > > > > problem? Why would any frame be shown upside down? > > > > > > > > And my question is also why it's not enough to synchronzie with > > |_isRunning|? > > > > Why do we also need to check _willBeRunning, connection.enabled, and > > > > connection.active? > > > > > > > > Also, please upload new patch sets to the existing CL instead of creating > > new > > > > ones. > > > > > > I added some logging and what I see is that between when the switch camera > > > button is pressed and the camera inputs are switched, frames are still being > > > processed by captureOutput:didOutputSampleBuffer:fromConnection:. > > > > > > The log shows that a frame is processed with the previous camera, then with > > the > > > new camera. The camera is obtained from > > > connection.inputPorts.firstObject).input. > > > > > > For example, switching from BACK to FRONT camera, the logs are: > > > CAMERA LOG: rotation: 180 usingFrontCamera: BACK > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > > > So, IF the first frame is actually from the FRONT camera and not the BACK > > > camera, the frame will be marked with the wrong rotation, ie. it will appear > > > upside down. > > > > > > Following is speculation and what I'm guessing is happening.... > > > When we call removeInput, then addInput, the connections are automatically > > > created by iOS. In the short time interval between addInput and the > connection > > > created, a frame from the FRONT camera is received, but the previous > > connection > > > that says is the BACK camera is received. > > > So, we get an incorrectly labeled rotation for the frame. > > > > > > As this artifact happens about one in ten or twenty tries, it is probably > due > > to > > > a timing/race condition like the above. > > > > Ok, can you do dispatchAsyncOnType:RTCDispatcherTypeCaptureSession from > > captureOutput:didOutputSampleBuffer:fromConnection: to synchronize it with the > > reconfig and see if that solves the problem? > > The sample buffer passed in needs to be in the same queue, otherwise it goes out > of scope. > I get crashes when accessing the sampleBuffer attributes within the dispatch > queue. I added a new flag _changingCamera to gate keep the frame handling instead.
On 2017/07/07 15:37:30, jtt wrote: > On 2017/07/07 08:28:32, magjed_webrtc wrote: > > On 2017/07/06 15:53:21, jtt wrote: > > > On 2017/07/06 08:12:52, magjed_webrtc wrote: > > > > On 2017/07/05 16:54:52, jtt wrote: > > > > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > File > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > > > > _isRunning = false; > > > > > > On 2017/06/30 18:07:47, jtt wrote: > > > > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > > > > Why is this needed? > > > > > > > > > > > > > > I removed this check and the flicker came back. > > > > > > > > > > > > > > The switch over now has a very momentary black frame when it > switches > > > > > cameras. > > > > > > > > > > > > Why does it flicker if we remove this? I thought it would be > > synchronized > > > by > > > > > the > > > > > > fact that we check from what camera device each frame comes from. And > > why > > > do > > > > > we > > > > > > need the other checks _willBeRunning, connection.enabled, and > > > > > connection.active? > > > > > > I don't want to sprinkle new conditions without understanding what's > > > > > happening. > > > > > > > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > > > > > > > To answer your question, we delete the old inputs, then add the new > input > > > for > > > > > the new camera. In the short period between deleting the old input and > > > adding > > > > > the new input, we are still getting frames from the old input and the > new > > > > input > > > > > before the connections are created for the input to the output. > > > > > > > > > > So the old AVCaptureConnection may still be valid when getting new > frames > > > from > > > > > the new input. > > > > > > > > I understand we might get frames from the previous connection while we > > > > re-configure, but if the previous AVCaptureConnection is valid why is this > a > > > > problem? Why would any frame be shown upside down? > > > > > > > > And my question is also why it's not enough to synchronzie with > > |_isRunning|? > > > > Why do we also need to check _willBeRunning, connection.enabled, and > > > > connection.active? > > > > > > > > Also, please upload new patch sets to the existing CL instead of creating > > new > > > > ones. > > > > > > I added some logging and what I see is that between when the switch camera > > > button is pressed and the camera inputs are switched, frames are still being > > > processed by captureOutput:didOutputSampleBuffer:fromConnection:. > > > > > > The log shows that a frame is processed with the previous camera, then with > > the > > > new camera. The camera is obtained from > > > connection.inputPorts.firstObject).input. > > > > > > For example, switching from BACK to FRONT camera, the logs are: > > > CAMERA LOG: rotation: 180 usingFrontCamera: BACK > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > > > So, IF the first frame is actually from the FRONT camera and not the BACK > > > camera, the frame will be marked with the wrong rotation, ie. it will appear > > > upside down. > > > > > > Following is speculation and what I'm guessing is happening.... > > > When we call removeInput, then addInput, the connections are automatically > > > created by iOS. In the short time interval between addInput and the > connection > > > created, a frame from the FRONT camera is received, but the previous > > connection > > > that says is the BACK camera is received. > > > So, we get an incorrectly labeled rotation for the frame. > > > > > > As this artifact happens about one in ten or twenty tries, it is probably > due > > to > > > a timing/race condition like the above. > > > > Ok, can you do dispatchAsyncOnType:RTCDispatcherTypeCaptureSession from > > captureOutput:didOutputSampleBuffer:fromConnection: to synchronize it with the > > reconfig and see if that solves the problem? > > The sample buffer passed in needs to be in the same queue, otherwise it goes out > of scope. > I get crashes when accessing the sampleBuffer attributes within the dispatch > queue. You need to retain the sample buffer manually.
On 2017/07/10 09:39:30, magjed_webrtc wrote: > On 2017/07/07 15:37:30, jtt wrote: > > On 2017/07/07 08:28:32, magjed_webrtc wrote: > > > On 2017/07/06 15:53:21, jtt wrote: > > > > On 2017/07/06 08:12:52, magjed_webrtc wrote: > > > > > On 2017/07/05 16:54:52, jtt wrote: > > > > > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > File > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > > > > > _isRunning = false; > > > > > > > On 2017/06/30 18:07:47, jtt wrote: > > > > > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > > > > > Why is this needed? > > > > > > > > > > > > > > > > I removed this check and the flicker came back. > > > > > > > > > > > > > > > > The switch over now has a very momentary black frame when it > > switches > > > > > > cameras. > > > > > > > > > > > > > > Why does it flicker if we remove this? I thought it would be > > > synchronized > > > > by > > > > > > the > > > > > > > fact that we check from what camera device each frame comes from. > And > > > why > > > > do > > > > > > we > > > > > > > need the other checks _willBeRunning, connection.enabled, and > > > > > > connection.active? > > > > > > > I don't want to sprinkle new conditions without understanding what's > > > > > > happening. > > > > > > > > > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > > > > > > > > > To answer your question, we delete the old inputs, then add the new > > input > > > > for > > > > > > the new camera. In the short period between deleting the old input and > > > > adding > > > > > > the new input, we are still getting frames from the old input and the > > new > > > > > input > > > > > > before the connections are created for the input to the output. > > > > > > > > > > > > So the old AVCaptureConnection may still be valid when getting new > > frames > > > > from > > > > > > the new input. > > > > > > > > > > I understand we might get frames from the previous connection while we > > > > > re-configure, but if the previous AVCaptureConnection is valid why is > this > > a > > > > > problem? Why would any frame be shown upside down? > > > > > > > > > > And my question is also why it's not enough to synchronzie with > > > |_isRunning|? > > > > > Why do we also need to check _willBeRunning, connection.enabled, and > > > > > connection.active? > > > > > > > > > > Also, please upload new patch sets to the existing CL instead of > creating > > > new > > > > > ones. > > > > > > > > I added some logging and what I see is that between when the switch camera > > > > button is pressed and the camera inputs are switched, frames are still > being > > > > processed by captureOutput:didOutputSampleBuffer:fromConnection:. > > > > > > > > The log shows that a frame is processed with the previous camera, then > with > > > the > > > > new camera. The camera is obtained from > > > > connection.inputPorts.firstObject).input. > > > > > > > > For example, switching from BACK to FRONT camera, the logs are: > > > > CAMERA LOG: rotation: 180 usingFrontCamera: BACK > > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > > > > > So, IF the first frame is actually from the FRONT camera and not the BACK > > > > camera, the frame will be marked with the wrong rotation, ie. it will > appear > > > > upside down. > > > > > > > > Following is speculation and what I'm guessing is happening.... > > > > When we call removeInput, then addInput, the connections are automatically > > > > created by iOS. In the short time interval between addInput and the > > connection > > > > created, a frame from the FRONT camera is received, but the previous > > > connection > > > > that says is the BACK camera is received. > > > > So, we get an incorrectly labeled rotation for the frame. > > > > > > > > As this artifact happens about one in ten or twenty tries, it is probably > > due > > > to > > > > a timing/race condition like the above. > > > > > > Ok, can you do dispatchAsyncOnType:RTCDispatcherTypeCaptureSession from > > > captureOutput:didOutputSampleBuffer:fromConnection: to synchronize it with > the > > > reconfig and see if that solves the problem? > > > > The sample buffer passed in needs to be in the same queue, otherwise it goes > out > > of scope. > > I get crashes when accessing the sampleBuffer attributes within the dispatch > > queue. > > You need to retain the sample buffer manually. Added a CFRetain(sampleBuffer) and a CFRelease(sampleBuffer) inside the block. I still get the artifact but instead of an upside down frame of the new camera being switched to, I get an upside down frame of the current camera being switched from. So we are still processing incorrectly a frame inside the block.
On 2017/07/10 15:37:46, jtt wrote: > On 2017/07/10 09:39:30, magjed_webrtc wrote: > > On 2017/07/07 15:37:30, jtt wrote: > > > On 2017/07/07 08:28:32, magjed_webrtc wrote: > > > > On 2017/07/06 15:53:21, jtt wrote: > > > > > On 2017/07/06 08:12:52, magjed_webrtc wrote: > > > > > > On 2017/07/05 16:54:52, jtt wrote: > > > > > > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > File > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > > > > > > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > > > > > > _isRunning = false; > > > > > > > > On 2017/06/30 18:07:47, jtt wrote: > > > > > > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > > > > > > Why is this needed? > > > > > > > > > > > > > > > > > > I removed this check and the flicker came back. > > > > > > > > > > > > > > > > > > The switch over now has a very momentary black frame when it > > > switches > > > > > > > cameras. > > > > > > > > > > > > > > > > Why does it flicker if we remove this? I thought it would be > > > > synchronized > > > > > by > > > > > > > the > > > > > > > > fact that we check from what camera device each frame comes from. > > And > > > > why > > > > > do > > > > > > > we > > > > > > > > need the other checks _willBeRunning, connection.enabled, and > > > > > > > connection.active? > > > > > > > > I don't want to sprinkle new conditions without understanding > what's > > > > > > > happening. > > > > > > > > > > > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > > > > > > > > > > > To answer your question, we delete the old inputs, then add the new > > > input > > > > > for > > > > > > > the new camera. In the short period between deleting the old input > and > > > > > adding > > > > > > > the new input, we are still getting frames from the old input and > the > > > new > > > > > > input > > > > > > > before the connections are created for the input to the output. > > > > > > > > > > > > > > So the old AVCaptureConnection may still be valid when getting new > > > frames > > > > > from > > > > > > > the new input. > > > > > > > > > > > > I understand we might get frames from the previous connection while we > > > > > > re-configure, but if the previous AVCaptureConnection is valid why is > > this > > > a > > > > > > problem? Why would any frame be shown upside down? > > > > > > > > > > > > And my question is also why it's not enough to synchronzie with > > > > |_isRunning|? > > > > > > Why do we also need to check _willBeRunning, connection.enabled, and > > > > > > connection.active? > > > > > > > > > > > > Also, please upload new patch sets to the existing CL instead of > > creating > > > > new > > > > > > ones. > > > > > > > > > > I added some logging and what I see is that between when the switch > camera > > > > > button is pressed and the camera inputs are switched, frames are still > > being > > > > > processed by captureOutput:didOutputSampleBuffer:fromConnection:. > > > > > > > > > > The log shows that a frame is processed with the previous camera, then > > with > > > > the > > > > > new camera. The camera is obtained from > > > > > connection.inputPorts.firstObject).input. > > > > > > > > > > For example, switching from BACK to FRONT camera, the logs are: > > > > > CAMERA LOG: rotation: 180 usingFrontCamera: BACK > > > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > > > > > > > So, IF the first frame is actually from the FRONT camera and not the > BACK > > > > > camera, the frame will be marked with the wrong rotation, ie. it will > > appear > > > > > upside down. > > > > > > > > > > Following is speculation and what I'm guessing is happening.... > > > > > When we call removeInput, then addInput, the connections are > automatically > > > > > created by iOS. In the short time interval between addInput and the > > > connection > > > > > created, a frame from the FRONT camera is received, but the previous > > > > connection > > > > > that says is the BACK camera is received. > > > > > So, we get an incorrectly labeled rotation for the frame. > > > > > > > > > > As this artifact happens about one in ten or twenty tries, it is > probably > > > due > > > > to > > > > > a timing/race condition like the above. > > > > > > > > Ok, can you do dispatchAsyncOnType:RTCDispatcherTypeCaptureSession from > > > > captureOutput:didOutputSampleBuffer:fromConnection: to synchronize it with > > the > > > > reconfig and see if that solves the problem? > > > > > > The sample buffer passed in needs to be in the same queue, otherwise it goes > > out > > > of scope. > > > I get crashes when accessing the sampleBuffer attributes within the dispatch > > > queue. > > > > You need to retain the sample buffer manually. > > Added a CFRetain(sampleBuffer) and a CFRelease(sampleBuffer) inside the block. > > I still get the artifact but instead of an upside down frame of the new camera > being switched to, I get an upside down frame of the current camera being > switched from. > > So we are still processing incorrectly a frame inside the block. Used CVPixelBufferRetain(pixelBuffer) and CVPixelBufferRelease(pixelBuffer) and still got the upside down frame. We should be dropping the frame while switching cameras using the flag I added as I think it is due to using the outdated AVCaptureConnection.
On 2017/07/10 16:14:28, jtt wrote: > On 2017/07/10 15:37:46, jtt wrote: > > On 2017/07/10 09:39:30, magjed_webrtc wrote: > > > On 2017/07/07 15:37:30, jtt wrote: > > > > On 2017/07/07 08:28:32, magjed_webrtc wrote: > > > > > On 2017/07/06 15:53:21, jtt wrote: > > > > > > On 2017/07/06 08:12:52, magjed_webrtc wrote: > > > > > > > On 2017/07/05 16:54:52, jtt wrote: > > > > > > > > On 2017/07/03 08:23:46, magjed_webrtc wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > > File > > > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2964703002/diff/1/webrtc/sdk/objc/Framework/Cla... > > > > > > > > > > > > > > > > > > > > > > > > > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:123: > > > > > > > > > _isRunning = false; > > > > > > > > > On 2017/06/30 18:07:47, jtt wrote: > > > > > > > > > > On 2017/06/30 17:50:50, magjed_webrtc wrote: > > > > > > > > > > > Why is this needed? > > > > > > > > > > > > > > > > > > > > I removed this check and the flicker came back. > > > > > > > > > > > > > > > > > > > > The switch over now has a very momentary black frame when it > > > > switches > > > > > > > > cameras. > > > > > > > > > > > > > > > > > > Why does it flicker if we remove this? I thought it would be > > > > > synchronized > > > > > > by > > > > > > > > the > > > > > > > > > fact that we check from what camera device each frame comes > from. > > > And > > > > > why > > > > > > do > > > > > > > > we > > > > > > > > > need the other checks _willBeRunning, connection.enabled, and > > > > > > > > connection.active? > > > > > > > > > I don't want to sprinkle new conditions without understanding > > what's > > > > > > > > happening. > > > > > > > > > > > > > > > > See the other cl - https://codereview.webrtc.org/2970983002/ > > > > > > > > > > > > > > > > To answer your question, we delete the old inputs, then add the > new > > > > input > > > > > > for > > > > > > > > the new camera. In the short period between deleting the old input > > and > > > > > > adding > > > > > > > > the new input, we are still getting frames from the old input and > > the > > > > new > > > > > > > input > > > > > > > > before the connections are created for the input to the output. > > > > > > > > > > > > > > > > So the old AVCaptureConnection may still be valid when getting new > > > > frames > > > > > > from > > > > > > > > the new input. > > > > > > > > > > > > > > I understand we might get frames from the previous connection while > we > > > > > > > re-configure, but if the previous AVCaptureConnection is valid why > is > > > this > > > > a > > > > > > > problem? Why would any frame be shown upside down? > > > > > > > > > > > > > > And my question is also why it's not enough to synchronzie with > > > > > |_isRunning|? > > > > > > > Why do we also need to check _willBeRunning, connection.enabled, and > > > > > > > connection.active? > > > > > > > > > > > > > > Also, please upload new patch sets to the existing CL instead of > > > creating > > > > > new > > > > > > > ones. > > > > > > > > > > > > I added some logging and what I see is that between when the switch > > camera > > > > > > button is pressed and the camera inputs are switched, frames are still > > > being > > > > > > processed by captureOutput:didOutputSampleBuffer:fromConnection:. > > > > > > > > > > > > The log shows that a frame is processed with the previous camera, then > > > with > > > > > the > > > > > > new camera. The camera is obtained from > > > > > > connection.inputPorts.firstObject).input. > > > > > > > > > > > > For example, switching from BACK to FRONT camera, the logs are: > > > > > > CAMERA LOG: rotation: 180 usingFrontCamera: BACK > > > > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > > > CAMERA LOG: rotation: 0 usingFrontCamera: FRONT > > > > > > > > > > > > So, IF the first frame is actually from the FRONT camera and not the > > BACK > > > > > > camera, the frame will be marked with the wrong rotation, ie. it will > > > appear > > > > > > upside down. > > > > > > > > > > > > Following is speculation and what I'm guessing is happening.... > > > > > > When we call removeInput, then addInput, the connections are > > automatically > > > > > > created by iOS. In the short time interval between addInput and the > > > > connection > > > > > > created, a frame from the FRONT camera is received, but the previous > > > > > connection > > > > > > that says is the BACK camera is received. > > > > > > So, we get an incorrectly labeled rotation for the frame. > > > > > > > > > > > > As this artifact happens about one in ten or twenty tries, it is > > probably > > > > due > > > > > to > > > > > > a timing/race condition like the above. > > > > > > > > > > Ok, can you do dispatchAsyncOnType:RTCDispatcherTypeCaptureSession from > > > > > captureOutput:didOutputSampleBuffer:fromConnection: to synchronize it > with > > > the > > > > > reconfig and see if that solves the problem? > > > > > > > > The sample buffer passed in needs to be in the same queue, otherwise it > goes > > > out > > > > of scope. > > > > I get crashes when accessing the sampleBuffer attributes within the > dispatch > > > > queue. > > > > > > You need to retain the sample buffer manually. > > > > Added a CFRetain(sampleBuffer) and a CFRelease(sampleBuffer) inside the block. > > > > I still get the artifact but instead of an upside down frame of the new camera > > being switched to, I get an upside down frame of the current camera being > > switched from. > > > > So we are still processing incorrectly a frame inside the block. > > Used CVPixelBufferRetain(pixelBuffer) and CVPixelBufferRelease(pixelBuffer) and > still got the upside down frame. > > We should be dropping the frame while switching cameras using the flag I added > as I think it is due to using the outdated AVCaptureConnection. I also tried creating the AVCaptureConnection myself and added the input and outputs but I ended up not getting any video in the output.
Adding more debug logs, here's what I discovered. I logged the EXIF data from the sample buffer, and the input device's and connection's cameras. Here is when I switched from the front camera to the back camera. START CAMERA SWITCH Start remove input End remove input Start add input End add input #1: iPhone SE front camera 2.15mm f/2.4videoOrientation: 3, videoMirrored: NOT MIRRORED #2: connection camera : BACK, rotation : 180, image flipped: FLIPPED #3: session.input : BACK #4: iPhone SE back camera 4.15mm f/2.2videoOrientation: 3, videoMirrored: NOT MIRRORED #5: connection camera : BACK, rotation : 180, image flipped: FLIPPED #6: session.input : BACK End commit configuration END CAMERA SWITCH Look at line #1 - the EXIF data of the sample image says it's from the FRONT camera. But at #2-#3, we think it's from the BACK camera. At #4 the next image comes in from the correct BACK camera. So, we are getting an 'old' image from the previous camera even though all the inputs and connections say we have the new camera. The correct fix would be to set the rotation based on the EXIF data of the image received, but it means we have to extract the EXIF data and scan for front or back in the LensModel field of the EXIF. This test would not be reliable if the EXIF data changes between devices.
jtteh@webrtc.org changed reviewers: + tkchin@webrtc.org
Check the EXIF of the image to determine which camera it's coming from. Check only when switching cameras.
Currently in the process of adding unit tests.
Added unit tests for RTCCameraVideoCapturer which is the refactored video apis.
Added unit test for the old video capturer, RTCAVFoundationVideoCapturerInternal. Added an info.plist for the unit tests as RTCAVFoundationVideoCapturerInternal accesses the camera and needed to have a plist and the NSCameraUsageDescription description in the plist.
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
Removed the unit test for the old video capturer as we're transitioning to the new one and it was very difficult to mock the old C++ non-virtual classes.
jtteh@webrtc.org changed reviewers: - magjed@webrtc.org
-magjed_webrtc, as magjed_webrtc is OOO.
jtteh@webrtc.org changed reviewers: + kthelgason@webrtc.org
Can you set up a time for us to chat in person next week when I'm back in office? I still don't follow the root cause of the bug and how we end up with a badly oriented frame. https://codereview.webrtc.org/2964703002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:144: _changingCamera = true; YES / NO not true / false
From above (edited to remove extraneous logging): START CAMERA SWITCH Start remove input End remove input Start add input End add input #1: iPhone SE front camera 2.15mm f/2.4 <<<< EXIF says it's FRONT camera #2: connection camera : BACK, rotation : 180 <<<< But connection says it's BACK camera #3: session.input : BACK #4: iPhone SE back camera 4.15mm f/2.2 #5: connection camera : BACK, rotation : 180 #6: session.input : BACK End commit configuration END CAMERA SWITCH Look at line #1 - the EXIF data of the sample image says it's from the FRONT camera. But at #2-#3, we think it's from the BACK camera. At #4 the next image comes in from the correct BACK camera. So, we are getting an 'old' image from the previous camera even though all the inputs and connections say we have the new camera.
On 2017/07/19 21:15:25, jtt_webrtc wrote: > From above (edited to remove extraneous logging): > > START CAMERA SWITCH > Start remove input > End remove input > Start add input > End add input > #1: iPhone SE front camera 2.15mm f/2.4 <<<< EXIF says it's FRONT camera > #2: connection camera : BACK, rotation : 180 <<<< But connection says it's BACK > camera > #3: session.input : BACK > #4: iPhone SE back camera 4.15mm f/2.2 > #5: connection camera : BACK, rotation : 180 > #6: session.input : BACK > End commit configuration > END CAMERA SWITCH > > Look at line #1 - the EXIF data of the sample image says it's from the FRONT > camera. > But at #2-#3, we think it's from the BACK camera. > > At #4 the next image comes in from the correct BACK camera. > > So, we are getting an 'old' image from the previous camera even though all the > inputs and connections say we have the new camera. Wouldn't it be simpler to just lock while switching cameras and wait with processing the captured frame? Or do you think that would cause us to drop frames?
https://codereview.webrtc.org/2964703002/diff/200001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2964703002/diff/200001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:174: self.hasStarted = YES; This change seems unrelated. https://codereview.webrtc.org/2964703002/diff/200001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:250: } Can we put this logic into it's own class instead of duplicating it?
On 2017/07/20 08:05:10, kthelgason wrote: > On 2017/07/19 21:15:25, jtt_webrtc wrote: > > From above (edited to remove extraneous logging): > > > > START CAMERA SWITCH > > Start remove input > > End remove input > > Start add input > > End add input > > #1: iPhone SE front camera 2.15mm f/2.4 <<<< EXIF says it's FRONT camera > > #2: connection camera : BACK, rotation : 180 <<<< But connection says it's > BACK > > camera > > #3: session.input : BACK > > #4: iPhone SE back camera 4.15mm f/2.2 > > #5: connection camera : BACK, rotation : 180 > > #6: session.input : BACK > > End commit configuration > > END CAMERA SWITCH > > > > Look at line #1 - the EXIF data of the sample image says it's from the FRONT > > camera. > > But at #2-#3, we think it's from the BACK camera. > > > > At #4 the next image comes in from the correct BACK camera. > > > > So, we are getting an 'old' image from the previous camera even though all the > > inputs and connections say we have the new camera. > > Wouldn't it be simpler to just lock while switching cameras and wait with > processing the captured frame? > > Or do you think that would cause us to drop frames? Magjed didn't like dropping frames while we're switching cameras, so I had to check the EXIF data on the image.
Moved common code to RTCImageHelper. https://codereview.webrtc.org/2964703002/diff/200001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2964703002/diff/200001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:174: self.hasStarted = YES; On 2017/07/20 08:07:54, kthelgason wrote: > This change seems unrelated. We don't really want to mark the session as started until the capture session has started running. https://codereview.webrtc.org/2964703002/diff/200001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:250: } On 2017/07/20 08:07:54, kthelgason wrote: > Can we put this logic into it's own class instead of duplicating it? I moved the EXIF check to it's own class RTCImageHelper.
Description was changed from ========== During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 ========== to ========== [iOS] Fix incorrectly oriented frames when rapidly switching between cameras. During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 ==========
This lgtm. However, I'm not an owner of these files so you will probably need magjed to stamp it as well. He should be back on monday.
jtteh@webrtc.org changed reviewers: + magjed@webrtc.org
+magjed as he'll be back Monday.
https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:33: @property(assign) BOOL switchingCameras; It looks like we are accessing this variable from multiple threads without synchronization. https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:217: usingFrontCamera = [RTCImageHelper isFrontCameraFromSampleBuffer:sampleBuffer]; Is this method guaranteed to work on all devices? How expensive is it to call it? If it's reliable and cheap to call it, can we always use it to simplify the code? https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/UnitTests/RTCCameraVideoCapturerTests.mm (right): https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/UnitTests/RTCCameraVideoCapturerTests.mm:324: if ([self.capturer respondsToSelector:selector]) { I don't like white-box testing. I would rather you just unittest the isFrontCameraFromSampleBuffer function instead and/or come up with a way to test this using the public functions.
https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:217: usingFrontCamera = [RTCImageHelper isFrontCameraFromSampleBuffer:sampleBuffer]; On 2017/07/24 10:12:50, magjed_webrtc wrote: > Is this method guaranteed to work on all devices? How expensive is it to call > it? If it's reliable and cheap to call it, can we always use it to simplify the > code? I've tested it with an iPhone SE and iPhone 7 and 7+. Elapsed time to call is ~0.004ms, so I'll remove the switchingCameras flag. https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/UnitTests/RTCCameraVideoCapturerTests.mm (right): https://codereview.webrtc.org/2964703002/diff/220001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/UnitTests/RTCCameraVideoCapturerTests.mm:324: if ([self.capturer respondsToSelector:selector]) { On 2017/07/24 10:12:51, magjed_webrtc wrote: > I don't like white-box testing. I would rather you just unittest the > isFrontCameraFromSampleBuffer function instead and/or come up with a way to test > this using the public functions. Added test to test just [RTCImageHelper isFrontCameraFromSampleBuffer:].
https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:208: AVCaptureDevicePosition cameraPosition = [RTCImageHelper cameraFromSampleBuffer:sampleBuffer]; nit: name the method more appropriately for the return type. The method isn't returning a camera from a sample buffer. devicePositionForSampleBuffer? https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:212: switch (_orientation) { orientation is set from the capture session queue, but you are accessing it on the frame queue. Will this cause problems? This is old behavior anyway, so I won't block CL. But would be good to understand / fix later. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:173: self.hasStarted = YES; This should be reverted. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h:9: */ nit: blank line https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:10: #import "RTCImageHelper.h" nit: blank line before #import https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:13: CFRange foundRange; nit: always initialize vars probably set length to zero, index to NSNotFound https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:31: CFDictionaryRef cfExifDictVal = NULL; nit: nullptr https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:33: attachments, (const void *)CFSTR("{Exif}"), (const void **)&cfExifDictVal)) { does the return val need to be released? what are the semantics for ownership here? https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:33: attachments, (const void *)CFSTR("{Exif}"), (const void **)&cfExifDictVal)) { Consider declaring NSString * and toll bridge to CFString instead, or declaring a static value somewhere https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:36: (const void *)CFSTR("LensModel"), similarly here
Waiting on an iPhone 4S to test performance of retriving the EXIF on every frame. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:208: AVCaptureDevicePosition cameraPosition = [RTCImageHelper cameraFromSampleBuffer:sampleBuffer]; On 2017/07/24 21:30:37, tkchin_webrtc wrote: > nit: name the method more appropriately for the return type. The method isn't > returning a camera from a sample buffer. > devicePositionForSampleBuffer? Done. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:212: switch (_orientation) { On 2017/07/24 21:30:37, tkchin_webrtc wrote: > orientation is set from the capture session queue, but you are accessing it on > the frame queue. Will this cause problems? > > This is old behavior anyway, so I won't block CL. But would be good to > understand / fix later. > Acknowledged. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:173: self.hasStarted = YES; On 2017/07/24 21:30:37, tkchin_webrtc wrote: > This should be reverted. Done. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h:9: */ On 2017/07/24 21:30:37, tkchin_webrtc wrote: > nit: blank line Done. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm (right): https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:10: #import "RTCImageHelper.h" On 2017/07/24 21:30:37, tkchin_webrtc wrote: > nit: blank line before #import Done. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:13: CFRange foundRange; On 2017/07/24 21:30:37, tkchin_webrtc wrote: > nit: always initialize vars > probably set length to zero, index to NSNotFound Since it's not used, I'll pass in nil. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:31: CFDictionaryRef cfExifDictVal = NULL; On 2017/07/24 21:30:37, tkchin_webrtc wrote: > nit: nullptr Using nil as nullptr isn't defined here. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:33: attachments, (const void *)CFSTR("{Exif}"), (const void **)&cfExifDictVal)) { On 2017/07/24 21:30:37, tkchin_webrtc wrote: > Consider declaring NSString * and toll bridge to CFString instead, or declaring > a static value somewhere CFSTR calls __builtin___CFStringMakeConstantString which adds it to an internal dictionary and the string is reused, not created/released. So there is no need to create a toll bridged NSString. See https://opensource.apple.com/source/CF/CF-855.17/CFString.c https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:33: attachments, (const void *)CFSTR("{Exif}"), (const void **)&cfExifDictVal)) { On 2017/07/24 21:30:37, tkchin_webrtc wrote: > does the return val need to be released? what are the semantics for ownership > here? Retain count on the returned value is 1. So it is owned by the dictionary, hence no need to release. https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:36: (const void *)CFSTR("LensModel"), On 2017/07/24 21:30:37, tkchin_webrtc wrote: > similarly here See above.
On 2017/07/24 22:29:52, jtt_webrtc wrote: > Waiting on an iPhone 4S to test performance of retriving the EXIF on every > frame. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > (right): > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:208: > AVCaptureDevicePosition cameraPosition = [RTCImageHelper > cameraFromSampleBuffer:sampleBuffer]; > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > nit: name the method more appropriately for the return type. The method isn't > > returning a camera from a sample buffer. > > devicePositionForSampleBuffer? > > Done. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:212: > switch (_orientation) { > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > orientation is set from the capture session queue, but you are accessing it on > > the frame queue. Will this cause problems? > > > > This is old behavior anyway, so I won't block CL. But would be good to > > understand / fix later. > > > > Acknowledged. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > File > webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm > (right): > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:173: > self.hasStarted = YES; > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > This should be reverted. > > Done. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h (right): > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h:9: */ > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > nit: blank line > > Done. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm (right): > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:10: #import > "RTCImageHelper.h" > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > nit: blank line before #import > > Done. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:13: CFRange > foundRange; > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > nit: always initialize vars > > probably set length to zero, index to NSNotFound > > Since it's not used, I'll pass in nil. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:31: CFDictionaryRef > cfExifDictVal = NULL; > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > nit: nullptr > > Using nil as nullptr isn't defined here. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:33: attachments, > (const void *)CFSTR("{Exif}"), (const void **)&cfExifDictVal)) { > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > Consider declaring NSString * and toll bridge to CFString instead, or > declaring > > a static value somewhere > > CFSTR calls __builtin___CFStringMakeConstantString which adds it to an internal > dictionary and the string is reused, not created/released. So there is no need > to create a toll bridged NSString. See > https://opensource.apple.com/source/CF/CF-855.17/CFString.c > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:33: attachments, > (const void *)CFSTR("{Exif}"), (const void **)&cfExifDictVal)) { > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > does the return val need to be released? what are the semantics for ownership > > here? > > Retain count on the returned value is 1. So it is owned by the dictionary, hence > no need to release. > > https://codereview.webrtc.org/2964703002/diff/260001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.mm:36: (const void > *)CFSTR("LensModel"), > On 2017/07/24 21:30:37, tkchin_webrtc wrote: > > similarly here > > See above. On iPhone 4S, first time is 4ms, dropping to 0.005 ms to extract the Exif
lgtm https://codereview.webrtc.org/2964703002/diff/280001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h (right): https://codereview.webrtc.org/2964703002/diff/280001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCImageHelper.h:16: @interface RTCImageHelper : NSObject nit: consider putting this in to a category of AVCaptureSession e.g. AVCaptureSession+DevicePosition
I added back the gate keeper to only check the Exif when switching cameras. Please review.
https://codereview.webrtc.org/2964703002/diff/300001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2964703002/diff/300001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm:215: if (_switchingCameras) { since you are using atomicops, use AcquireLoad (the alternative would've been to use an atomic BOOL property)
On 2017/07/24 23:25:21, tkchin_webrtc wrote: > https://codereview.webrtc.org/2964703002/diff/300001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm > (right): > > https://codereview.webrtc.org/2964703002/diff/300001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm:215: > if (_switchingCameras) { > since you are using atomicops, use AcquireLoad > (the alternative would've been to use an atomic BOOL property) Renamed RTCImageHelper to AVCaptureSession+Private. Added use of AcquireLoad.
https://codereview.webrtc.org/2964703002/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2964703002/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm:156: rtc::AtomicOps::Decrement(&_switchingCameras); thinking about this again, I'm not even sure this gate will even work The frames in progress could occur even after the unlock configuration is called if there's already a backlog it would be safer to always check the exif if that operation is cheap https://codereview.webrtc.org/2964703002/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Private.h (right): https://codereview.webrtc.org/2964703002/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Private.h:16: @interface AVCaptureSession(Private) space before ( also don't call it Private. It's not a class we own.
Removed the gate keeper check and renamed the AVCaptureSession helper class. https://codereview.webrtc.org/2964703002/diff/320001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm (right): https://codereview.webrtc.org/2964703002/diff/320001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm:156: rtc::AtomicOps::Decrement(&_switchingCameras); On 2017/07/24 23:43:04, tkchin_webrtc wrote: > thinking about this again, I'm not even sure this gate will even work > The frames in progress could occur even after the unlock configuration is called > if there's already a backlog > > it would be safer to always check the exif if that operation is cheap Done.
https://codereview.webrtc.org/2964703002/diff/340001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2964703002/diff/340001/webrtc/sdk/BUILD.gn#newc... webrtc/sdk/BUILD.gn:263: "objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm", do you still need this?
On 2017/07/25 00:03:43, tkchin_webrtc wrote: > https://codereview.webrtc.org/2964703002/diff/340001/webrtc/sdk/BUILD.gn > File webrtc/sdk/BUILD.gn (right): > > https://codereview.webrtc.org/2964703002/diff/340001/webrtc/sdk/BUILD.gn#newc... > webrtc/sdk/BUILD.gn:263: > "objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm", > do you still need this? Renamed back.
lgtm https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:209: if (cameraPosition != AVCaptureDevicePositionUnspecified) { Since we always check the EXIF now and it's likely to succeed, I would prefer this order of the logic to make it more intuitive: BOOL usingFrontCamera; AVCaptureDevicePosition cameraPosition = [AVCaptureSession devicePositionForSampleBuffer:sampleBuffer]; if (cameraPosition != AVCaptureDevicePositionUnspecified) { usingFrontCamera = cameraPosition == AVCaptureDevicePositionFront; } else { AVCaptureDeviceInput *deviceInput = (AVCaptureDeviceInput *)((AVCaptureInputPort *)connection.inputPorts.firstObject).input; usingFrontCamera = deviceInput.device.position == AVCaptureDevicePositionFront; } https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h (right): https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h:18: // Check the image's EXIF for the actual camera the image came. nit: "... the camera the image came from."
Refactored how we get the camera position. https://codereview.webrtc.org/2964703002/diff/340001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2964703002/diff/340001/webrtc/sdk/BUILD.gn#newc... webrtc/sdk/BUILD.gn:263: "objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.mm", On 2017/07/25 00:03:43, tkchin_webrtc wrote: > do you still need this? Done. https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:209: if (cameraPosition != AVCaptureDevicePositionUnspecified) { On 2017/07/25 08:30:10, magjed_webrtc wrote: > Since we always check the EXIF now and it's likely to succeed, I would prefer > this order of the logic to make it more intuitive: > > BOOL usingFrontCamera; > AVCaptureDevicePosition cameraPosition = > [AVCaptureSession devicePositionForSampleBuffer:sampleBuffer]; > if (cameraPosition != AVCaptureDevicePositionUnspecified) { > usingFrontCamera = cameraPosition == AVCaptureDevicePositionFront; > } else { > AVCaptureDeviceInput *deviceInput = > (AVCaptureDeviceInput *)((AVCaptureInputPort > *)connection.inputPorts.firstObject).input; > usingFrontCamera = deviceInput.device.position == > AVCaptureDevicePositionFront; > } Done. https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h (right): https://codereview.webrtc.org/2964703002/diff/350001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h:18: // Check the image's EXIF for the actual camera the image came. On 2017/07/25 08:30:10, magjed_webrtc wrote: > nit: "... the camera the image came from." Done.
The CQ bit was checked by jtteh@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, tkchin@webrtc.org, magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2964703002/#ps390001 (title: "Refactored checking of image's source device.")
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": 390001, "attempt_start_ts": 1500999032034760,
"parent_rev": "96b69bdbee32062c199c31859f833f2761b89567", "commit_rev":
"61b0ed039dcd937d92d49a666c504a0eb2b475c6"}
Message was sent while issue was closed.
Description was changed from ========== [iOS] Fix incorrectly oriented frames when rapidly switching between cameras. During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 ========== to ========== [iOS] Fix incorrectly oriented frames when rapidly switching between cameras. During a call, with both phones in horizontal or landscape mode, rapidly switching between the front and back camera sometimes causes the remote video to be shown upside down. There seems to be a race condition when setting the rotation based on the orientation of the device and which camera we're using. So use the active input's camera to check instead of the client state. BUG=webrtc:7898 Review-Url: https://codereview.webrtc.org/2964703002 Cr-Commit-Position: refs/heads/master@{#19139} Committed: https://chromium.googlesource.com/external/webrtc/+/61b0ed039dcd937d92d49a666... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:390001) as https://chromium.googlesource.com/external/webrtc/+/61b0ed039dcd937d92d49a666...
Message was sent while issue was closed.
https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m (right): https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:200: RTCVideoRotation rotation = RTCVideoRotation_90; shouldn't the logic re-ordering also apply here? https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h (right): https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h:16: @interface AVCaptureSession (Device) DevicePosition? https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm (right): https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:221: BOOL usingFrontCamera; init to NO
Message was sent while issue was closed.
On 2017/07/25 19:33:30, tkchin_webrtc wrote: > https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m > (right): > > https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCCameraVideoCapturer.m:200: > RTCVideoRotation rotation = RTCVideoRotation_90; > shouldn't the logic re-ordering also apply here? Change in 2988783002 > > https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... > File webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h (right): > > https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/AVCaptureSession+Device.h:16: @interface > AVCaptureSession (Device) > DevicePosition? Change in 2988783002 > > https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... > File > webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm > (right): > > https://codereview.webrtc.org/2964703002/diff/390001/webrtc/sdk/objc/Framewor... > webrtc/sdk/objc/Framework/Classes/Video/RTCAVFoundationVideoCapturerInternal.mm:221: > BOOL usingFrontCamera; > init to NO Change in 2988783002 |
