From 8f1903e3cff7ca9169c3704e73420dd24c34bb08 Mon Sep 17 00:00:00 2001 From: Andreas Pehrson Date: Wed, 17 May 2023 14:03:17 +0200 Subject: [PATCH] In VideoCaptureImpl lock api_lock_ when accessing apply_rotation_ A comment says SetApplyRotation could deadlock if grabbing the lock, but it does not make any calls under the lock so that is impossible, unless the caller of SetApplyRotation involves a second lock that the callback tries to grab, in which case it appears to be a problem of the caller. Bug: webrtc:15181 Change-Id: Ie16cb01ffb84e9118dd5c87863c29bd107a6c94e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/305646 Commit-Queue: Stefan Holmer Reviewed-by: Stefan Holmer Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#40279} --- modules/video_capture/video_capture_impl.cc | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/modules/video_capture/video_capture_impl.cc b/modules/video_capture/video_capture_impl.cc index ef1300f9b6..3864ffa17e 100644 --- a/modules/video_capture/video_capture_impl.cc +++ b/modules/video_capture/video_capture_impl.cc @@ -167,10 +167,7 @@ int32_t VideoCaptureImpl::IncomingFrame(uint8_t* videoFrame, int target_width = width; int target_height = abs(height); - // SetApplyRotation doesn't take any lock. Make a local copy here. - bool apply_rotation = apply_rotation_; - - if (apply_rotation) { + if (apply_rotation_) { // Rotating resolution when for 90/270 degree rotations. if (_rotateFrame == kVideoRotation_90 || _rotateFrame == kVideoRotation_270) { @@ -186,7 +183,7 @@ int32_t VideoCaptureImpl::IncomingFrame(uint8_t* videoFrame, target_width, target_height, stride_y, stride_uv, stride_uv); libyuv::RotationMode rotation_mode = libyuv::kRotate0; - if (apply_rotation) { + if (apply_rotation_) { switch (_rotateFrame) { case kVideoRotation_0: rotation_mode = libyuv::kRotate0; @@ -221,7 +218,7 @@ int32_t VideoCaptureImpl::IncomingFrame(uint8_t* videoFrame, .set_video_frame_buffer(buffer) .set_timestamp_rtp(0) .set_timestamp_ms(rtc::TimeMillis()) - .set_rotation(!apply_rotation ? _rotateFrame : kVideoRotation_0) + .set_rotation(!apply_rotation_ ? _rotateFrame : kVideoRotation_0) .build(); captureFrame.set_ntp_time_ms(captureTime); @@ -256,14 +253,13 @@ int32_t VideoCaptureImpl::SetCaptureRotation(VideoRotation rotation) { } bool VideoCaptureImpl::SetApplyRotation(bool enable) { - // We can't take any lock here as it'll cause deadlock with IncomingFrame. - - // The effect of this is the last caller wins. + MutexLock lock(&api_lock_); apply_rotation_ = enable; return true; } bool VideoCaptureImpl::GetApplyRotation() { + MutexLock lock(&api_lock_); return apply_rotation_; }