Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2687433004:
TWCC-PLR -based FecController doesn’t need smoothing (Closed)
|
Created:
3 years, 10 months ago by elad.alon Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTWCC-PLR -based FecController doesn’t need smoothing
TWCC-PLR -based FecController doesn’t need smoothing; instead, use "null-smoothing", which just returns the last value (if any).
If we end up using TWCC-PLR, we'll just remove smoothing altogether. Until then, this is the least intrusive way to modify the code while still letting it work correctly for RTCP-PLR.
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2687433004
Cr-Commit-Position: refs/heads/master@{#17375}
Committed: https://chromium.googlesource.com/external/webrtc/+/f0e1f60b0c253377be6cc7fa3745467503d2631b
Patch Set 1 #Patch Set 2 : Rebased. #
Total comments: 2
Patch Set 3 : CR response #
Total comments: 2
Patch Set 4 : nit fixed #Messages
Total messages: 16 (6 generated)
Description was changed from ========== TWCC-PLR -based FecController doesn’t need smoothing TWCC-PLR -based FecController doesn’t need smoothing; instead, use "null-smoothing", which just returns the last value (if any). If we end up using TWCC-PLR, we'll just remove smoothing altogether. Until then, this is the least intrusive way to modify the code while still letting it work correctly for RTCP-PLR. BUG=webrtc:7058 ========== to ========== TWCC-PLR -based FecController doesn’t need smoothing TWCC-PLR -based FecController doesn’t need smoothing; instead, use "null-smoothing", which just returns the last value (if any). If we end up using TWCC-PLR, we'll just remove smoothing altogether. Until then, this is the least intrusive way to modify the code while still letting it work correctly for RTCP-PLR. BUG=webrtc:7058 ==========
elad.alon@webrtc.org changed reviewers: + michaelt@webrtc.org, minyue@webrtc.org, sprang@webrtc.org, stefan@webrtc.org
Ready for review.
https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.h (right): https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.h:71: class NullSmoothingFilter final : public SmoothingFilter { can we move NullSmoothingFilter to fec controller ? It is very special and used only there.
https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.h (right): https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.h:71: class NullSmoothingFilter final : public SmoothingFilter { On 2017/03/22 09:40:55, minyue-webrtc wrote: > can we move NullSmoothingFilter to fec controller > ? It is very special and used only there. That's possible. On the other hand, what if somebody else reinvents it in another file, because it wasn't very visible? Or if someone else needs it, he would have to move it here. Wouldn't it be best to keep it here to begin with?
On 2017/03/22 10:14:25, elad.alon wrote: > https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... > File webrtc/common_audio/smoothing_filter.h (right): > > https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... > webrtc/common_audio/smoothing_filter.h:71: class NullSmoothingFilter final : > public SmoothingFilter { > On 2017/03/22 09:40:55, minyue-webrtc wrote: > > can we move NullSmoothingFilter to fec controller > > ? It is very special and used only there. > > That's possible. On the other hand, what if somebody else reinvents it in > another file, because it wasn't very visible? Or if someone else needs it, he > would have to move it here. Wouldn't it be best to keep it here to begin with? It is too special and the chance of using it is almost none. Even in FEC controller, the need is only due to we want to handle two types of PLR in a unified way.
On 2017/03/22 10:37:24, minyue-webrtc wrote: > On 2017/03/22 10:14:25, elad.alon wrote: > > > https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... > > File webrtc/common_audio/smoothing_filter.h (right): > > > > > https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... > > webrtc/common_audio/smoothing_filter.h:71: class NullSmoothingFilter final : > > public SmoothingFilter { > > On 2017/03/22 09:40:55, minyue-webrtc wrote: > > > can we move NullSmoothingFilter to fec controller > > > ? It is very special and used only there. > > > > That's possible. On the other hand, what if somebody else reinvents it in > > another file, because it wasn't very visible? Or if someone else needs it, he > > would have to move it here. Wouldn't it be best to keep it here to begin with? > > It is too special and the chance of using it is almost none. Even in FEC > controller, the need is only due to we want to handle two types of PLR in a > unified way. Yes, you're right. Will do.
On 2017/03/22 10:45:04, elad.alon wrote: > On 2017/03/22 10:37:24, minyue-webrtc wrote: > > On 2017/03/22 10:14:25, elad.alon wrote: > > > > > > https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... > > > File webrtc/common_audio/smoothing_filter.h (right): > > > > > > > > > https://codereview.webrtc.org/2687433004/diff/20001/webrtc/common_audio/smoot... > > > webrtc/common_audio/smoothing_filter.h:71: class NullSmoothingFilter final : > > > public SmoothingFilter { > > > On 2017/03/22 09:40:55, minyue-webrtc wrote: > > > > can we move NullSmoothingFilter to fec controller > > > > ? It is very special and used only there. > > > > > > That's possible. On the other hand, what if somebody else reinvents it in > > > another file, because it wasn't very visible? Or if someone else needs it, > he > > > would have to move it here. Wouldn't it be best to keep it here to begin > with? > > > > It is too special and the chance of using it is almost none. Even in FEC > > controller, the need is only due to we want to handle two types of PLR in a > > unified way. > > Yes, you're right. Will do. Done.
lgtm % nit https://codereview.webrtc.org/2687433004/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc (right): https://codereview.webrtc.org/2687433004/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:22: constexpr bool USE_TWCC_PLR_FOR_ANA = false; nit: kUseTwccPlrForAna because it is not a macro
https://codereview.webrtc.org/2687433004/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc (right): https://codereview.webrtc.org/2687433004/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:22: constexpr bool USE_TWCC_PLR_FOR_ANA = false; On 2017/03/24 12:30:11, minyue-webrtc wrote: > nit: kUseTwccPlrForAna > > because it is not a macro Done.
The CQ bit was checked by elad.alon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2687433004/#ps60001 (title: "nit fixed")
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": 1490359300242880,
"parent_rev": "670a7f3611243bf7df5f82d91e8ead2e9951b51c", "commit_rev":
"f0e1f60b0c253377be6cc7fa3745467503d2631b"}
Message was sent while issue was closed.
Description was changed from ========== TWCC-PLR -based FecController doesn’t need smoothing TWCC-PLR -based FecController doesn’t need smoothing; instead, use "null-smoothing", which just returns the last value (if any). If we end up using TWCC-PLR, we'll just remove smoothing altogether. Until then, this is the least intrusive way to modify the code while still letting it work correctly for RTCP-PLR. BUG=webrtc:7058 ========== to ========== TWCC-PLR -based FecController doesn’t need smoothing TWCC-PLR -based FecController doesn’t need smoothing; instead, use "null-smoothing", which just returns the last value (if any). If we end up using TWCC-PLR, we'll just remove smoothing altogether. Until then, this is the least intrusive way to modify the code while still letting it work correctly for RTCP-PLR. BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2687433004 Cr-Commit-Position: refs/heads/master@{#17375} Committed: https://chromium.googlesource.com/external/webrtc/+/f0e1f60b0c253377be6cc7fa3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/f0e1f60b0c253377be6cc7fa3... |
