Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349113002:
Adding reordering logic in audio network adaptor. (Closed)
|
Created:
4 years, 3 months ago by minyue-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding reordering logic in audio network adaptor.
BUG=webrtc:6303
Committed: https://crrev.com/bc77ed7657e19fc916beaff7f830c4d712ac99e7
Cr-Commit-Position: refs/heads/master@{#14344}
Patch Set 1 #
Total comments: 24
Patch Set 2 : On Henrik's comments #
Total comments: 2
Messages
Total messages: 18 (7 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Adding reordering logic in audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding reordering logic in audio network adaptor. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
Hi Michael, I added reordering logic to audio network adaptor. Please take a look, thanks!
https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:68: if (last_reordering_time_ms_ && should work even without "last_reordering_time_ms_ &&" right ? https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:120: constexpr int kMaxUplinkBandwidthBps = 120000; should we inject this by the config. Or if not we should at least use 64kbps as max. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:54: std::vector<std::unique_ptr<Controller>> controllers); How a bout a vector with a pair with std::unique_ptr<Controller> and rtc::Optional<ScoringPoint>. I think this way the interface would be more clear to me ?
https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:68: if (last_reordering_time_ms_ && On 2016/09/19 08:22:45, michaelt wrote: > should work even without "last_reordering_time_ms_ &&" right ? If last_reordering_time_ms_ is none, it will try the following sorting. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:120: constexpr int kMaxUplinkBandwidthBps = 120000; On 2016/09/19 08:22:45, michaelt wrote: > should we inject this by the config. > Or if not we should at least use 64kbps as max. I think the definition of distance can be hard coded, and let min_reordering_distance to be trained for it. But I am open to suggestions regarding this. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:54: std::vector<std::unique_ptr<Controller>> controllers); On 2016/09/19 08:22:45, michaelt wrote: > How a bout a vector with a pair with std::unique_ptr<Controller> and > rtc::Optional<ScoringPoint>. I think this way the interface would be more clear > to me ? I like the idea, but I want to keep ScoringPoint private, so it will become std::vector<std::pair<std::unique_ptr<Controller>>, rtc::Optional<std::pair<int, float>>>> It looks cumbersome, and we cannot assign values to |controllers_| easily. Another solution can be to use 2 arguments: std::vector<std::unique_ptr<Controller>> controllers std::vector<rtc::Optional<std::pair<int, float>>> chracteristic_points // one-one mapped to |controllers| But this is not much better than std::map<const Controller*, std::pair<int, float>>. An additional benefit of std::map<const Controller*, std::pair<int, float>> is that it matches the member variable |controller_scoring_points_| in a nice way.
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Hi Henrik, Michael has taken a look at this. I now add you to review it.
https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:88: if (lhs_scoring_point != controller_scoring_points_.end() && You can swap the order of these two "end-cases": if (lhs_scoring_point == controller_scoring_points_.end()) return false; if (rhs_scoring_point == controller_scoring_points_.end()) return true; https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:122: float NormalizeUplinkBandwidth(int uplink_Bandwidth_bps) { Small 'b' in the parameter name. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:131: // |uplink_packet_loss_fraction| is seldom smaller than 0.3, so we scale it up "seldom smaller" -> "seldom larger"? https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:20: #include "webrtc/system_wrappers/include/clock.h" Forward-declare here, and include in .cc and unit test instead. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:54: std::vector<std::unique_ptr<Controller>> controllers); It's not immediately clear that |controllers| is actually moved from. I would argue (in conflict with the style guide), that you you apply rvalue notation here, i.e., "std::vector<std::unique_ptr<Controller>>&& controllers". https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:74: // |expected_order| contains the expected indices of all controllers in the Remove extra space in "the expected" https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:76: // means that we do not care its exact place, but we do check that it exists care *about* its https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:83: RTC_DCHECK_EQ(kNumControllers, expected_order.size()); Don't DCHECK in tests. Use EXPECT or ASSERT
Hi Henrik, Thanks for the comments! I uploaded a new patch and PTAL again! https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:88: if (lhs_scoring_point != controller_scoring_points_.end() && On 2016/09/20 08:44:00, hlundin-webrtc wrote: > You can swap the order of these two "end-cases": > > if (lhs_scoring_point == controller_scoring_points_.end()) > return false; > if (rhs_scoring_point == controller_scoring_points_.end()) > return true; Done. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:122: float NormalizeUplinkBandwidth(int uplink_Bandwidth_bps) { On 2016/09/20 08:44:00, hlundin-webrtc wrote: > Small 'b' in the parameter name. oh, yes, my bad. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:131: // |uplink_packet_loss_fraction| is seldom smaller than 0.3, so we scale it up On 2016/09/20 08:44:00, hlundin-webrtc wrote: > "seldom smaller" -> "seldom larger"? oh, my bad https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:20: #include "webrtc/system_wrappers/include/clock.h" On 2016/09/20 08:44:01, hlundin-webrtc wrote: > Forward-declare here, and include in .cc and unit test instead. Done. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:54: std::vector<std::unique_ptr<Controller>> controllers); On 2016/09/20 08:44:00, hlundin-webrtc wrote: > It's not immediately clear that |controllers| is actually moved from. I would > argue (in conflict with the style guide), that you you apply rvalue notation > here, i.e., "std::vector<std::unique_ptr<Controller>>&& controllers". rvalue notation is very new to me. But I think it fits here extremely well, adopted, thanks! https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:74: // |expected_order| contains the expected indices of all controllers in the On 2016/09/20 08:44:01, hlundin-webrtc wrote: > Remove extra space in "the expected" Done. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:76: // means that we do not care its exact place, but we do check that it exists On 2016/09/20 08:44:01, hlundin-webrtc wrote: > care *about* its Done. https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:83: RTC_DCHECK_EQ(kNumControllers, expected_order.size()); On 2016/09/20 08:44:01, hlundin-webrtc wrote: > Don't DCHECK in tests. Use EXPECT or ASSERT OK. but I deliberately made this since this detects wrongly written tests. while EXPECT are used for checking expected results. https://codereview.webrtc.org/2349113002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2349113002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:53: std::vector<std::unique_ptr<Controller>>&& controllers, I switched order to 2nd and 3rd args. It reads more natural.
lgtm https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:83: RTC_DCHECK_EQ(kNumControllers, expected_order.size()); On 2016/09/21 07:42:59, minyue-webrtc wrote: > On 2016/09/20 08:44:01, hlundin-webrtc wrote: > > Don't DCHECK in tests. Use EXPECT or ASSERT > > OK. but I deliberately made this since this detects wrongly written tests. while > EXPECT are used for checking expected results. Well, ok. I can accept that. https://codereview.webrtc.org/2349113002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2349113002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:53: std::vector<std::unique_ptr<Controller>>&& controllers, On 2016/09/21 07:42:59, minyue-webrtc wrote: > I switched order to 2nd and 3rd args. It reads more natural. Acknowledged.
lgtm https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2349113002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:68: if (last_reordering_time_ms_ && On 2016/09/19 10:06:57, minyue-webrtc wrote: > On 2016/09/19 08:22:45, michaelt wrote: > > should work even without "last_reordering_time_ms_ &&" right ? > > If last_reordering_time_ms_ is none, it will try the following sorting. Acknowledged.
The CQ bit was checked by minyue@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 ========== Adding reordering logic in audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding reordering logic in audio network adaptor. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Adding reordering logic in audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding reordering logic in audio network adaptor. BUG=webrtc:6303 Committed: https://crrev.com/bc77ed7657e19fc916beaff7f830c4d712ac99e7 Cr-Commit-Position: refs/heads/master@{#14344} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bc77ed7657e19fc916beaff7f830c4d712ac99e7 Cr-Commit-Position: refs/heads/master@{#14344} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
