|
|
DescriptionTemporarily remove SSRC DCHECK in RTPSender::SendToNetwork.
Removing the DCHECK due to (sometimes) failing voe_auto_test.
Long-term, this DCHECK should be readded. Before that can happen,
the SSRC in the RTPSender should be made immutable.
TESTED=No failures when running third_party/gtest-parallel/gtest-parallel --repeat=5000 --gtest_filter="VolumeTest.ManualInputMutingMutesMicrophone" out/Debug/voe_auto_test.
BUG=webrtc:6887
Review-Url: https://codereview.webrtc.org/2610873002
Cr-Commit-Position: refs/heads/master@{#15962}
Committed: https://chromium.googlesource.com/external/webrtc/+/075c6d7f7ed6586f7ccdf5c3eed77b0b0afdd434
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase. #Patch Set 3 : Remove DCHECK without adding clause to conditional. #
Total comments: 2
Patch Set 4 : danilchap comments 1. #Messages
Total messages: 27 (15 generated)
The CQ bit was checked by brandtr@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/...
Description was changed from ========== Replace SSRC DCHECK with conditional clause in RTPSender::SendToNetwork. Prior to the change, if an old value of |ssrc_| was used to set packet SSRCs, the (now removed) DCHECK would fire when the packet was sent to RTPSender::SendToNetwork. Example scenario: RTPSender::SSRC() is called to set the SSRC of a packet to be sent, RTPSender::SetSSRC(.) is called with a new SSRC, and finally RTPSender::SendToNetwork(.) is called with the packet with the old SSRC. After the change, this scenario will simply lead to the packet not being stored in the packet history. This is the correct behaviour, since the packet history is not SSRC-aware. TESTED=No failures when running third_party/gtest-parallel/gtest-parallel --repeat=10000 --gtest_filter="VolumeTest.ManualInputMutingMutesMicrophone" out/Debug/voe_auto_test. BUG=webrtc:6887 ========== to ========== Replace SSRC DCHECK with conditional clause in RTPSender::SendToNetwork. Prior to the change, if an old value of |ssrc_| was used to set packet SSRCs, the (now removed) DCHECK would fire when the packet was sent to RTPSender::SendToNetwork. Example scenario: RTPSender::SSRC() is called to set the SSRC of a packet to be sent, RTPSender::SetSSRC(.) is called with a new SSRC, and finally RTPSender::SendToNetwork(.) is called with the packet with the old SSRC. After the change, this scenario will simply lead to the packet not being stored in the packet history. This is the correct behaviour, since the packet history is not SSRC-aware. TESTED=No failures when running third_party/gtest-parallel/gtest-parallel --repeat=5000 --gtest_filter="VolumeTest.ManualInputMutingMutesMicrophone" out/Debug/voe_auto_test. BUG=webrtc:6887 ==========
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
DCHECK is there because of assumption ssrc will not change once packets start to fly. What cause this assumption to break? If this assumption is rightfully false, there might be more work to do, e.g. clear packet_history when ssrc changes.
On 2017/01/04 16:39:04, danilchap wrote: > DCHECK is there because of assumption ssrc will not change once packets start to > fly. > What cause this assumption to break? If a collision between the remote SSRC (obtained from a received RTP packet) and local SSRC is detected in the RTP module, a new SSRC is generated: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour.... This happens in particular in the voe_auto_test suite, where my feeling is that a single RTP module is sending data to itself (not 100% certain this is the case). This leads to repeated SSRC collisions, but only the first one leads to a new SSRC being generated. > > If this assumption is rightfully false, there might be more work to do, e.g. > clear packet_history when ssrc changes. Yes, this would be appropriate.
https://codereview.webrtc.org/2610873002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2610873002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:884: if (storage == kAllowRetransmission && ssrc == SSRC()) { alternatively to clearing packet_history/having ssrc check here it should be easier and safer to check ssrc when retrieving packet for resending.
Rebase.
> This happens in particular in the voe_auto_test suite, where my feeling is that > a single RTP module is sending data to itself (not 100% certain this is the > case). This leads to repeated SSRC collisions, but only the first one leads to a > new SSRC being generated. Had the test been wired up to higher layers, this check https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... would have stopped the problem from happening.
On 2017/01/09 08:45:51, brandtr wrote: > > This happens in particular in the voe_auto_test suite, where my feeling is > that > > a single RTP module is sending data to itself (not 100% certain this is the > > case). This leads to repeated SSRC collisions, but only the first one leads to > a > > new SSRC being generated. > > Had the test been wired up to higher layers, this check > https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... > would have stopped the problem from happening. (possibly). Didn't verify that is the case.
Description was changed from ========== Replace SSRC DCHECK with conditional clause in RTPSender::SendToNetwork. Prior to the change, if an old value of |ssrc_| was used to set packet SSRCs, the (now removed) DCHECK would fire when the packet was sent to RTPSender::SendToNetwork. Example scenario: RTPSender::SSRC() is called to set the SSRC of a packet to be sent, RTPSender::SetSSRC(.) is called with a new SSRC, and finally RTPSender::SendToNetwork(.) is called with the packet with the old SSRC. After the change, this scenario will simply lead to the packet not being stored in the packet history. This is the correct behaviour, since the packet history is not SSRC-aware. TESTED=No failures when running third_party/gtest-parallel/gtest-parallel --repeat=5000 --gtest_filter="VolumeTest.ManualInputMutingMutesMicrophone" out/Debug/voe_auto_test. BUG=webrtc:6887 ========== to ========== Temporarily remove SSRC DCHECK in RTPSender::SendToNetwork. Removing the DCHECK due to (sometimes) failing voe_auto_test. Long-term, this DCHECK should be readded. Before that can happen, the SSRC in the RTPSender should be made immutable. TESTED=No failures when running third_party/gtest-parallel/gtest-parallel --repeat=5000 --gtest_filter="VolumeTest.ManualInputMutingMutesMicrophone" out/Debug/voe_auto_test. BUG=webrtc:6887 ==========
As discussed.
lgtm https://codereview.webrtc.org/2610873002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2610873002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:885: // TODO(brandtr): Here we should RTC_DCHECK_EQ(ssrc, SSRC()), but that is Better to phrase TODO by stating what and when should be done. e.g.: Uncomment RTC_DCHECK_EQ(ssrc, SSRC()) when ssrc will not change after a packet is send. For details see // bug-link as it is now.
https://codereview.webrtc.org/2610873002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2610873002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:885: // TODO(brandtr): Here we should RTC_DCHECK_EQ(ssrc, SSRC()), but that is On 2017/01/09 12:08:59, danilchap wrote: > Better to phrase TODO by stating what and when should be done. e.g.: > Uncomment RTC_DCHECK_EQ(ssrc, SSRC()) when ssrc will not change after a packet > is send. For details see > // bug-link as it is now. Done.
The CQ bit was checked by brandtr@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: This issue passed the CQ dry run.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2610873002/#ps60001 (title: "danilchap comments 1.")
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": 60001, "attempt_start_ts": 1483967350760920, "parent_rev": "29fe6f338fdd632708073e03dba21f41599a4a2e", "commit_rev": "075c6d7f7ed6586f7ccdf5c3eed77b0b0afdd434"}
Message was sent while issue was closed.
Description was changed from ========== Temporarily remove SSRC DCHECK in RTPSender::SendToNetwork. Removing the DCHECK due to (sometimes) failing voe_auto_test. Long-term, this DCHECK should be readded. Before that can happen, the SSRC in the RTPSender should be made immutable. TESTED=No failures when running third_party/gtest-parallel/gtest-parallel --repeat=5000 --gtest_filter="VolumeTest.ManualInputMutingMutesMicrophone" out/Debug/voe_auto_test. BUG=webrtc:6887 ========== to ========== Temporarily remove SSRC DCHECK in RTPSender::SendToNetwork. Removing the DCHECK due to (sometimes) failing voe_auto_test. Long-term, this DCHECK should be readded. Before that can happen, the SSRC in the RTPSender should be made immutable. TESTED=No failures when running third_party/gtest-parallel/gtest-parallel --repeat=5000 --gtest_filter="VolumeTest.ManualInputMutingMutesMicrophone" out/Debug/voe_auto_test. BUG=webrtc:6887 Review-Url: https://codereview.webrtc.org/2610873002 Cr-Commit-Position: refs/heads/master@{#15962} Committed: https://chromium.googlesource.com/external/webrtc/+/075c6d7f7ed6586f7ccdf5c3e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/075c6d7f7ed6586f7ccdf5c3e... |