Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669733002:
Add 120ms frame ability to ANA (Closed)
|
Created:
3 years, 10 months ago by michaelt Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), peah-webrtc, qiang.lu, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, niklas.enbom, kwiberg-webrtc, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd 120ms frame ability to ANA
BUG=webrtc:7093
Review-Url: https://codereview.webrtc.org/2669733002
Cr-Commit-Position: refs/heads/master@{#16416}
Committed: https://chromium.googlesource.com/external/webrtc/+/a55f021d4351d94a257a16b72f45d843841252c1
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change kWebRtcOpusMaxEncodeFrameSizeMs for the 120ms case. #Patch Set 3 : Add define for opus_interface.c. #Patch Set 4 : Rebased #
Total comments: 2
Patch Set 5 : Rebased #Patch Set 6 : Response to comments. #
Total comments: 16
Patch Set 7 : Respond to comments. #Patch Set 8 : Respond to comments. #
Total comments: 11
Patch Set 9 : Respond to comments. #
Total comments: 3
Patch Set 10 : Respond to comments. #Messages
Total messages: 36 (16 generated)
minyue@webrtc.org changed reviewers: + minyue@webrtc.org
https://codereview.webrtc.org/2669733002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2669733002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:501: {kOpusCodecName, 48000, 2, 111, true, {10, 20, 40, 60}, kOpusMaxBitrateBps}, don't we need to add 120 here?
Description was changed from ========== Add 120ms frame ability to ANA BUG= ========== to ========== Add 120ms frame ability to ANA BUG=webrtc:7093 ==========
https://codereview.webrtc.org/2669733002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2669733002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:501: {kOpusCodecName, 48000, 2, 111, true, {10, 20, 40, 60}, kOpusMaxBitrateBps}, This will just be necessary for the non ANA case.
On 2017/02/01 11:38:46, michaelt wrote: > https://codereview.webrtc.org/2669733002/diff/1/webrtc/media/engine/webrtcvoi... > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/2669733002/diff/1/webrtc/media/engine/webrtcvoi... > webrtc/media/engine/webrtcvoiceengine.cc:501: {kOpusCodecName, 48000, 2, 111, > true, {10, 20, 40, 60}, kOpusMaxBitrateBps}, > This will just be necessary for the non ANA case. Sure, please consider rebasing on top of https://codereview.webrtc.org/2668633004/
Done
https://codereview.webrtc.org/2669733002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2669733002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:83: RTC_CHECK(config.has_fl_60ms_to_120ms_bandwidth_bps()); this must be carefully treated for backward-compatibility.
https://codereview.webrtc.org/2669733002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2669733002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:83: RTC_CHECK(config.has_fl_60ms_to_120ms_bandwidth_bps()); Impl. as discussed offline.
this looks neat and clean to me. See only few comments https://codereview.webrtc.org/2669733002/diff/100001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/media/BUILD.gn#ne... webrtc/media/BUILD.gn:136: defines = [] rebased needed here https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:860: if (rtc_opus_support_120ms_ptime) { rebase needed here https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (left): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:37: // Uplink packet loss fraction below which frame length can increase. why removing comment? https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:26: struct FrameLengthChange { how about wrapping this into Config? https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:46: std::map<FrameLengthChange, int> frame_length_change_criteria; how about fl_changing_bandwidths_bps? and add a comment to state what maps to what https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:47: Create20msTo60msChangeCriteria() { CreateChangCriteriaFor20msAnd60ms https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:60: Create20msTo120msChangeCriteria() { CreateChangeCriteriaFor20ms60msAnd120ms
https://codereview.webrtc.org/2669733002/diff/100001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/media/BUILD.gn#ne... webrtc/media/BUILD.gn:136: defines = [] On 2017/02/02 10:42:08, minyue-webrtc wrote: > rebased needed here Done. https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:860: if (rtc_opus_support_120ms_ptime) { On 2017/02/02 10:42:08, minyue-webrtc wrote: > rebase needed here Done. https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (left): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:37: // Uplink packet loss fraction below which frame length can increase. Oops https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:26: struct FrameLengthChange { Good idea. https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:46: std::map<FrameLengthChange, int> frame_length_change_criteria; Would prefer to keep the old name. But if you insist we could use the new name as well. https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:47: Create20msTo60msChangeCriteria() { Done https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:60: Create20msTo120msChangeCriteria() { On 2017/02/02 10:42:09, minyue-webrtc wrote: > CreateChangeCriteriaFor20ms60msAnd120ms Done.
https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:46: std::map<FrameLengthChange, int> frame_length_change_criteria; On 2017/02/02 11:17:18, michaelt wrote: > Would prefer to keep the old name. But if you insist we could use the new name > as well. > problem is that the old one does not say it is criteria on bandwidth. it was ok since it was private. Now it would be better to make it more explicit.
https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2669733002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:46: std::map<FrameLengthChange, int> frame_length_change_criteria; ok, will even fit better to the rest of the config.
lgtm
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org
On 2017/02/02 12:03:24, minyue-webrtc wrote: > lgtm +karl, Hi Karl, This is fairly urgent. I have reviewed it. It would be good that you take another (quick) look.
lgtm, but see comments https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:91: config.fl_60ms_to_20ms_bandwidth_bps())); I think you can replace line 84-91 with something like std::map<FrameLengthController::Config::FrameLengthChange, int> fl_changing_bandwidths_bps = {{FrameLengthController::Config::FrameLengthChange(20, 60), config.fl_20ms_to_60ms_bandwidth_bps()}, {FrameLengthController::Config::FrameLengthChange(60, 20), config.fl_60ms_to_20ms_bandwidth_bps()}}; https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:105: config.fl_decreasing_packet_loss_fraction(), fl_changing_bandwidths_bps); If you std::move fl_changing_bandwidths_bps here and change the argument type from const T& to just T, you won't have to pay for copying and destroying an extra map instance. I guess this code may not be performance critical, but it's good to get into the habit... https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:29: ~FrameLengthChange(); This class should have a trivial destructor---remove this line. https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:56: return frame_length_change_criteria; You can create the map with a single assignment expression as I described in an earlier comment. https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:75: return frame_length_change_criteria; And here.
https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:91: config.fl_60ms_to_20ms_bandwidth_bps())); Cool https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:105: config.fl_decreasing_packet_loss_fraction(), fl_changing_bandwidths_bps); On 2017/02/02 12:34:02, kwiberg-webrtc wrote: > If you std::move fl_changing_bandwidths_bps here and change the argument type > from const T& to just T, you won't have to pay for copying and destroying an > extra map instance. I guess this code may not be performance critical, but it's > good to get into the habit... Done. https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:29: ~FrameLengthChange(); On 2017/02/02 12:34:02, kwiberg-webrtc wrote: > This class should have a trivial destructor---remove this line. Done. https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:56: return frame_length_change_criteria; On 2017/02/02 12:34:02, kwiberg-webrtc wrote: > You can create the map with a single assignment expression as I described in an > earlier comment. Done. https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:75: return frame_length_change_criteria; On 2017/02/02 12:34:02, kwiberg-webrtc wrote: > And here. Done.
The CQ bit was checked by michaelt@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/...
lgtm once you add the missing std::move call https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2669733002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:91: config.fl_60ms_to_20ms_bandwidth_bps())); On 2017/02/02 13:04:34, michaelt wrote: > Cool Nice. I suspect it's a bit more efficient, but mainly it's more readable. https://codereview.webrtc.org/2669733002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2669733002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:30: fl_changing_bandwidths_bps(fl_changing_bandwidths_bps) {} You'll want to use std::move on this line too---otherwise you get an unnecessary copy here instead. When you're dealing with values directly rather than references, every assignment needs to be a move. You know the old saying: the price of improving performance with move semantics is eternal vigilance...
https://codereview.webrtc.org/2669733002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2669733002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:30: fl_changing_bandwidths_bps(fl_changing_bandwidths_bps) {} On 2017/02/02 13:18:40, kwiberg-webrtc wrote: > You'll want to use std::move on this line too---otherwise you get an unnecessary > copy here instead. When you're dealing with values directly rather than > references, every assignment needs to be a move. > > You know the old saying: the price of improving performance with move semantics > is eternal vigilance... Done.
The CQ bit was checked by michaelt@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/...
https://codereview.webrtc.org/2669733002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2669733002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:30: fl_changing_bandwidths_bps(fl_changing_bandwidths_bps) {} On 2017/02/02 13:30:40, michaelt wrote: > On 2017/02/02 13:18:40, kwiberg-webrtc wrote: > > You'll want to use std::move on this line too---otherwise you get an > unnecessary > > copy here instead. When you're dealing with values directly rather than > > references, every assignment needs to be a move. > > > > You know the old saying: the price of improving performance with move > semantics > > is eternal vigilance... > > Done. The new patch set you uploaded has no changes in it relative to the previous patch set. In particular, this fix is missing.
Patchset #10 (id:180001) has been deleted
Oops missed to commit the file :)
The CQ bit was checked by michaelt@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 michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2669733002/#ps200001 (title: "Respond to comments.")
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": 200001, "attempt_start_ts": 1486050298695670,
"parent_rev": "ed01647ea97dbe0ea25ab915237e39143b1978d7", "commit_rev":
"a55f021d4351d94a257a16b72f45d843841252c1"}
Message was sent while issue was closed.
Description was changed from ========== Add 120ms frame ability to ANA BUG=webrtc:7093 ========== to ========== Add 120ms frame ability to ANA BUG=webrtc:7093 Review-Url: https://codereview.webrtc.org/2669733002 Cr-Commit-Position: refs/heads/master@{#16416} Committed: https://chromium.googlesource.com/external/webrtc/+/a55f021d4351d94a257a16b72... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/a55f021d4351d94a257a16b72... |
