Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1826093004:
Avoid rescheduling the next RTCP packet if the RTCP sender status doesn't change. (Closed)
|
Created:
4 years, 9 months ago by skvlad Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid rescheduling the next RTCP packet if the RTCP sender status doesn't change.
The change made in https://codereview.webrtc.org/1757683002 introduced an extra call to RTCPSender::SetRTCPStatus after the video receive stream is created. The SetRTCPStatus call results in no state change, as the RTCP sender is already enabled, however, it reschedules the next RTCP packet to be RTCP_INTERVAL_VIDEO_MS/2 (500) ms in the future.
Before the change, the next packet time was only set by the previous call to RTCPSender::SetSSRC, which placed it 100 ms in the future. The change, therefore, changed the timing of multiple performance tests - as it now takes a different length of time to ramp up to the same bandwidth.
BUG=chromium:597332
Committed: https://crrev.com/1c392cc5cf83630eaaa0401954b66149c1760ebc
Cr-Commit-Position: refs/heads/master@{#12203}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed the extra parenthesis #Patch Set 3 : Rewrote the condition to make it easier to read #Messages
Total messages: 29 (11 generated)
skvlad@webrtc.org changed reviewers: + pbos@webrtc.org, pthatcher@webrtc.org, stefan@webrtc.org
Please take a look. This change fixes performance bugs https://bugs.chromium.org/p/chromium/issues/detail?id=597332 and https://bugs.chromium.org/p/chromium/issues/detail?id=597714, which happened because the extra call to RTCPSender::SetRTCPStatus() rescheduled the first RTCP packet to a different point in time.
https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:220: } Might be more clear as: if (method == method_) return; method_ = method; if (method == RtcpMode::kOff) return; next_time_to_send_rtcp_ = ...; Only the first early return needs to be added.
lgtm The previous comment was not important enough to block submitting.
lgtm https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:210: bool was_off = (method_ == RtcpMode::kOff); remove ()s
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:210: bool was_off = (method_ == RtcpMode::kOff); On 2016/03/29 17:48:46, pbos-webrtc wrote: > remove ()s Done. https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:220: } On 2016/03/25 23:03:40, pthatcher1 wrote: > Might be more clear as: > > if (method == method_) > return; > > method_ = method; > > if (method == RtcpMode::kOff) > return; > > next_time_to_send_rtcp_ = ...; > > > Only the first early return needs to be added. There are three possible values for RtcpMode: kOff, kCompound, and kReducedSize. My understanding is that only switching from kOff to one of the active modes should reschedule the packet. Rewriting the condition as you described does indeed make it clearer, but it makes the RTCP packet be rescheduled when the mode is changed from kCompound to kReducedSize which is probably not necessary.
pthatcher@google.com changed reviewers: + pthatcher@google.com
https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:220: } On 2016/03/29 18:09:25, skvlad wrote: > On 2016/03/25 23:03:40, pthatcher1 wrote: > > Might be more clear as: > > > > if (method == method_) > > return; > > > > method_ = method; > > > > if (method == RtcpMode::kOff) > > return; > > > > next_time_to_send_rtcp_ = ...; > > > > > > Only the first early return needs to be added. > There are three possible values for RtcpMode: kOff, kCompound, and kReducedSize. > My understanding is that only switching from kOff to one of the active modes > should reschedule the packet. > > Rewriting the condition as you described does indeed make it clearer, but it > makes the RTCP packet be rescheduled when the mode is changed from kCompound to > kReducedSize which is probably not necessary. Ah, true. Well, how about this? if (method == method_) return; if (method_ == RtcpMode::kOff) // When switching off => on, reschedule. next_time_to_send_rtcp_ = ...; method_ = method;
https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1826093004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:220: } On 2016/03/29 18:53:09, pthatcher wrote: > On 2016/03/29 18:09:25, skvlad wrote: > > On 2016/03/25 23:03:40, pthatcher1 wrote: > > > Might be more clear as: > > > > > > if (method == method_) > > > return; > > > > > > method_ = method; > > > > > > if (method == RtcpMode::kOff) > > > return; > > > > > > next_time_to_send_rtcp_ = ...; > > > > > > > > > Only the first early return needs to be added. > > There are three possible values for RtcpMode: kOff, kCompound, and > kReducedSize. > > My understanding is that only switching from kOff to one of the active modes > > should reschedule the packet. > > > > Rewriting the condition as you described does indeed make it clearer, but it > > makes the RTCP packet be rescheduled when the mode is changed from kCompound > to > > kReducedSize which is probably not necessary. > > Ah, true. > > > Well, how about this? > > if (method == method_) > return; > > if (method_ == RtcpMode::kOff) > // When switching off => on, reschedule. > next_time_to_send_rtcp_ = ...; > > method_ = method; Good idea; i've replaced it with a slightly modified version of this. Please take a look.
lgtm
The CQ bit was checked by skvlad@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1826093004/#ps60001 (title: "Rewrote the condition to make it easier to read")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826093004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826093004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4488)
skvlad@webrtc.org changed reviewers: + asapersson@webrtc.org, henrik.lundin@webrtc.org, mflodman@webrtc.org
Adding more owners for rtcp_sender.cc. Please take a look, it's a very small change.
On 2016/03/30 18:30:31, skvlad wrote: > Adding more owners for rtcp_sender.cc. Please take a look, it's a very small > change. Ping. Can you please take a look? This fix is needed to get some Chromium perf test results back to normal.
lgtm
The CQ bit was checked by skvlad@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826093004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826093004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by skvlad@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826093004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826093004/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Avoid rescheduling the next RTCP packet if the RTCP sender status doesn't change. The change made in https://codereview.webrtc.org/1757683002 introduced an extra call to RTCPSender::SetRTCPStatus after the video receive stream is created. The SetRTCPStatus call results in no state change, as the RTCP sender is already enabled, however, it reschedules the next RTCP packet to be RTCP_INTERVAL_VIDEO_MS/2 (500) ms in the future. Before the change, the next packet time was only set by the previous call to RTCPSender::SetSSRC, which placed it 100 ms in the future. The change, therefore, changed the timing of multiple performance tests - as it now takes a different length of time to ramp up to the same bandwidth. BUG=chromium:597332 ========== to ========== Avoid rescheduling the next RTCP packet if the RTCP sender status doesn't change. The change made in https://codereview.webrtc.org/1757683002 introduced an extra call to RTCPSender::SetRTCPStatus after the video receive stream is created. The SetRTCPStatus call results in no state change, as the RTCP sender is already enabled, however, it reschedules the next RTCP packet to be RTCP_INTERVAL_VIDEO_MS/2 (500) ms in the future. Before the change, the next packet time was only set by the previous call to RTCPSender::SetSSRC, which placed it 100 ms in the future. The change, therefore, changed the timing of multiple performance tests - as it now takes a different length of time to ramp up to the same bandwidth. BUG=chromium:597332 Committed: https://crrev.com/1c392cc5cf83630eaaa0401954b66149c1760ebc Cr-Commit-Position: refs/heads/master@{#12203} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1c392cc5cf83630eaaa0401954b66149c1760ebc Cr-Commit-Position: refs/heads/master@{#12203} |
