Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495483002:
Replace SequencedTaskChecker in RTPSenderVideo (Closed)
|
Created:
4 years, 1 month ago by kthelgason Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman, brandtr Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReplace SequencedTaskChecker in RTPSenderVideo
This is not the right place for a SequencedTaskChecker, as we can
not make any guarantees about the thread this method runs on.
We were hitting this check on Android and iOS whenever the encoder
would be reconfigured. Access to these ivars should be guarded
by a lock.
As a bonus, an unused method declaration was removed.
BUG=webrtc:6686
Committed: https://crrev.com/917a4eeb60dd177c781cd1c024b2d72be73078ee
Cr-Commit-Position: refs/heads/master@{#15019}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add todo #
Messages
Total messages: 25 (12 generated)
kthelgason@webrtc.org changed reviewers: + danilchap@webrtc.org, magjed@webrtc.org
https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:267: rtc::CritScope cs(&crit_); Can you easily merge this crit scope with the other crit scope in this function? From a quick glance, it looks safe to move that crit scope up here.
https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:267: rtc::CritScope cs(&crit_); On 2016/11/10 10:04:03, magjed_webrtc wrote: > Can you easily merge this crit scope with the other crit scope in this function? > From a quick glance, it looks safe to move that crit scope up here. no, it is not that easy unfortunately: extensions need to be set for accurate calculate of max_data_payload_length, that one required for packetizer. It should be easier to merge this lock with hidden lock in FecPacketOverhead function though.
https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:267: rtc::CritScope cs(&crit_); On 2016/11/10 10:14:53, danilchap wrote: > On 2016/11/10 10:04:03, magjed_webrtc wrote: > > Can you easily merge this crit scope with the other crit scope in this > function? > > From a quick glance, it looks safe to move that crit scope up here. > > no, it is not that easy unfortunately: > extensions need to be set for accurate calculate of max_data_payload_length, > that one required for packetizer. > > It should be easier to merge this lock with hidden lock in FecPacketOverhead > function though. on a second though I'm wrong, it is not hard: just remember retransmission_settings_ inside a lock, packetizer->GetStorateType can be called outside lock after that.
https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:267: rtc::CritScope cs(&crit_); On 2016/11/10 10:17:32, danilchap wrote: > On 2016/11/10 10:14:53, danilchap wrote: > > On 2016/11/10 10:04:03, magjed_webrtc wrote: > > > Can you easily merge this crit scope with the other crit scope in this > > function? > > > From a quick glance, it looks safe to move that crit scope up here. > > > > no, it is not that easy unfortunately: > > extensions need to be set for accurate calculate of max_data_payload_length, > > that one required for packetizer. > > > > It should be easier to merge this lock with hidden lock in FecPacketOverhead > > function though. > > on a second though I'm wrong, it is not hard: just remember > retransmission_settings_ inside a lock, > > packetizer->GetStorateType can be called outside lock after that. Is there a good reason to not just lock during the whole function scope?
lgtm if you want to postpone reducing number of locks later https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2495483002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:267: rtc::CritScope cs(&crit_); On 2016/11/10 10:38:26, kthelgason wrote: > On 2016/11/10 10:17:32, danilchap wrote: > > On 2016/11/10 10:14:53, danilchap wrote: > > > On 2016/11/10 10:04:03, magjed_webrtc wrote: > > > > Can you easily merge this crit scope with the other crit scope in this > > > function? > > > > From a quick glance, it looks safe to move that crit scope up here. > > > > > > no, it is not that easy unfortunately: > > > extensions need to be set for accurate calculate of max_data_payload_length, > > > that one required for packetizer. > > > > > > It should be easier to merge this lock with hidden lock in FecPacketOverhead > > > function though. > > > > on a second though I'm wrong, it is not hard: just remember > > retransmission_settings_ inside a lock, > > > > packetizer->GetStorateType can be called outside lock after that. > > Is there a good reason to not just lock during the whole function scope? It is too dangerous: SendVideoPacket* functions may call transport function internally, you definitely want to exclude that. calls to rtp_sender have own locks, creating lock dependencies i.e. increasing chance of deadlock. creating packetizer may be can be under lock, but since it is not hard to avoid that, better to. May it is ok to land as it is and solve 'too many locks' problem in a different CL.
Description was changed from ========== Replace SequencedTaskChecker in RTPSenderVideo This is not the right place for a SequencedTaskChecker, as we can not make any guarantees about the thread this method runs on. We were hitting this check on Android and iOS whenever the encoder would be reconfigured. Access to these ivars should be guarded by a lock. As a bonus, an unused method declaration was removed. BUG=webrtc:6686 ========== to ========== Replace SequencedTaskChecker in RTPSenderVideo This is not the right place for a SequencedTaskChecker, as we can not make any guarantees about the thread this method runs on. We were hitting this check on Android and iOS whenever the encoder would be reconfigured. Access to these ivars should be guarded by a lock. As a bonus, an unused method declaration was removed. BUG=webrtc:6686 ==========
The CQ bit was checked by kthelgason@webrtc.org 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: Try jobs failed on following builders: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/20036) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/6687)
not lgtm https://codereview.webrtc.org/2495483002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2495483002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:281: if (video_header) { no, it is not that easy: that block should run before max_data_payload_length is calculated at line 270. move this crit block above line 265 (except line storage = packetizer->....) add a line to remember retransmission_settings_ to avoid retaking lock.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by kthelgason@webrtc.org 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/...
On 2016/11/10 12:37:34, danilchap wrote: > not lgtm > > https://codereview.webrtc.org/2495483002/diff/20001/webrtc/modules/rtp_rtcp/s... > File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): > > https://codereview.webrtc.org/2495483002/diff/20001/webrtc/modules/rtp_rtcp/s... > webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:281: if (video_header) { > no, it is not that easy: that block should run before max_data_payload_length is > calculated at line 270. > move this crit block above line 265 (except line storage = packetizer->....) > add a line to remember retransmission_settings_ to avoid retaking lock. Thanks for taking a second look. I'm too scared to change anything here so I will leave it as two crit scopes for now.
lgtm
lgtm
The CQ bit was checked by kthelgason@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Replace SequencedTaskChecker in RTPSenderVideo This is not the right place for a SequencedTaskChecker, as we can not make any guarantees about the thread this method runs on. We were hitting this check on Android and iOS whenever the encoder would be reconfigured. Access to these ivars should be guarded by a lock. As a bonus, an unused method declaration was removed. BUG=webrtc:6686 ========== to ========== Replace SequencedTaskChecker in RTPSenderVideo This is not the right place for a SequencedTaskChecker, as we can not make any guarantees about the thread this method runs on. We were hitting this check on Android and iOS whenever the encoder would be reconfigured. Access to these ivars should be guarded by a lock. As a bonus, an unused method declaration was removed. BUG=webrtc:6686 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Replace SequencedTaskChecker in RTPSenderVideo This is not the right place for a SequencedTaskChecker, as we can not make any guarantees about the thread this method runs on. We were hitting this check on Android and iOS whenever the encoder would be reconfigured. Access to these ivars should be guarded by a lock. As a bonus, an unused method declaration was removed. BUG=webrtc:6686 ========== to ========== Replace SequencedTaskChecker in RTPSenderVideo This is not the right place for a SequencedTaskChecker, as we can not make any guarantees about the thread this method runs on. We were hitting this check on Android and iOS whenever the encoder would be reconfigured. Access to these ivars should be guarded by a lock. As a bonus, an unused method declaration was removed. BUG=webrtc:6686 Committed: https://crrev.com/917a4eeb60dd177c781cd1c024b2d72be73078ee Cr-Commit-Position: refs/heads/master@{#15019} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/917a4eeb60dd177c781cd1c024b2d72be73078ee Cr-Commit-Position: refs/heads/master@{#15019} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
