Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616393003:
Periodically update channel parameters and send TargetBitrate message. (Closed)
|
Created:
3 years, 11 months ago by sprang_webrtc Modified:
3 years, 11 months ago Reviewers:
danilchap CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPeriodically update channel parameters and send TargetBitrate message.
Currently, parameters are periodically updated, but the TargetBitrate
message is only sent if a new bitrate is set. It should be sent
periodically to indicate the signaled bitrate is valid and to prevent
stale values due to loss of RTCP packets.
BUG=webrtc:6897
Review-Url: https://codereview.webrtc.org/2616393003
Cr-Commit-Position: refs/heads/master@{#16096}
Committed: https://chromium.googlesource.com/external/webrtc/+/57c2fff361fac137480a3b045d2c6b194b1d0d30
Patch Set 1 #
Total comments: 2
Patch Set 2 : ms_ suffix #Patch Set 3 : Remove deprecated tests in VideoSender, add new one in ViEEncoder #
Total comments: 6
Patch Set 4 : Use fake clock in test #
Total comments: 4
Patch Set 5 : Fixed includes #
Total comments: 1
Patch Set 6 : Revert from fake clock to sleeping #Patch Set 7 : Rebase #
Messages
Total messages: 41 (18 generated)
sprang@webrtc.org changed reviewers: + danilchap@webrtc.org
lgtm https://codereview.webrtc.org/2616393003/diff/1/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2616393003/diff/1/webrtc/video/vie_encoder.h#ne... webrtc/video/vie_encoder.h:238: rtc::Optional<int64_t> last_parameters_update_ ACCESS_ON(&encoder_queue_); add 'ms_' suffix
https://codereview.webrtc.org/2616393003/diff/1/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2616393003/diff/1/webrtc/video/vie_encoder.h#ne... webrtc/video/vie_encoder.h:238: rtc::Optional<int64_t> last_parameters_update_ ACCESS_ON(&encoder_queue_); On 2017/01/09 14:05:48, danilchap wrote: > add 'ms_' suffix Done.
The CQ bit was checked by sprang@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/2616393003/#ps20001 (title: "ms_ suffix")
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
Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/20768)
The CQ bit was checked by sprang@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.
updated the tests, you may wanna take another pass
https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:26: using ::testing::_; put using inside (2nd) unnamed namespace (or at least next to other usings in this file) https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:1019: SleepMs(kProcessIntervalMs + 30); Can Sleep be avoided using SetClockForTesting method from base/timeutils.h ? You might want to check that with nisse@ what is 30? https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:1023: kStartTsNtp + kProcessIntervalMs * 90, codec_width_, codec_height_)); though time (1st parameter to CreateFrame) is called 'ntp', it use ms units, so rename kStartTsNtp e.g. to kStartTimestampMs (it doesn't really matter for this test it supposed to be derived from ntp). Do not use *90 conversion to rtp timestamp. (or does 90 mean some other constant? name it then)
sprang@webrtc.org changed reviewers: + mflodman@webrtc.org
I switched over to using a fake clock in vie_encoder_unittest, which required an update to the build file since the module containing the fake clock included its own test main() and also depended on rtc_base rather than rtc_base_approved. +mflodman as owner for that change. https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:26: using ::testing::_; On 2017/01/10 10:32:33, danilchap wrote: > put using inside (2nd) unnamed namespace (or at least next to other usings in > this file) Done. https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:1019: SleepMs(kProcessIntervalMs + 30); On 2017/01/10 10:32:33, danilchap wrote: > Can Sleep be avoided using SetClockForTesting method from base/timeutils.h ? > You might want to check that with nisse@ > > what is 30? Turns out it can, nice! 30 was just a margin to avoid flakiness. Removed it. https://codereview.webrtc.org/2616393003/diff/40001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:1023: kStartTsNtp + kProcessIntervalMs * 90, codec_width_, codec_height_)); On 2017/01/10 10:32:33, danilchap wrote: > though time (1st parameter to CreateFrame) is called 'ntp', it use ms units, so > rename kStartTsNtp e.g. to kStartTimestampMs (it doesn't really matter for this > test it supposed to be derived from ntp). > > Do not use *90 conversion to rtp timestamp. (or does 90 mean some other > constant? name it then) OK, that's confusing. I'll update the name of the parameter to indicate it's in ms and remove all my *90 stuff.
lgtm % nits https://codereview.webrtc.org/2616393003/diff/60001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2616393003/diff/60001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:15: #include "webrtc/base/fakeclock.h" fakeclock before logging for alphabetical order https://codereview.webrtc.org/2616393003/diff/60001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:19: #include "webrtc/system_wrappers/include/sleep.h" can remove this include
https://codereview.webrtc.org/2616393003/diff/60001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2616393003/diff/60001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:15: #include "webrtc/base/fakeclock.h" On 2017/01/10 13:08:15, danilchap wrote: > fakeclock before logging for alphabetical order Done. https://codereview.webrtc.org/2616393003/diff/60001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:19: #include "webrtc/system_wrappers/include/sleep.h" On 2017/01/10 13:08:15, danilchap wrote: > can remove this include Done.
ping mflodman
https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... webrtc/video/vie_encoder_unittest.cc:14: #include "webrtc/base/fakeclock.h" AFAICT fakeclock.h depends on messagequeue.h, which is only in rtc_base and not in rtc_base_approved. Do we need to use rtc::ScopedFakeClock?
On 2017/01/13 13:29:38, mflodman wrote: > https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... > File webrtc/video/vie_encoder_unittest.cc (right): > > https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... > webrtc/video/vie_encoder_unittest.cc:14: #include "webrtc/base/fakeclock.h" > AFAICT fakeclock.h depends on messagequeue.h, which is only in rtc_base and not > in rtc_base_approved. > > Do we need to use rtc::ScopedFakeClock? Maybe can do without scoped fake clock. But without fake clock we have to do at least a one second sleep :( See patch set 4
On 2017/01/13 13:33:41, språng wrote: > On 2017/01/13 13:29:38, mflodman wrote: > > > https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... > > File webrtc/video/vie_encoder_unittest.cc (right): > > > > > https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... > > webrtc/video/vie_encoder_unittest.cc:14: #include "webrtc/base/fakeclock.h" > > AFAICT fakeclock.h depends on messagequeue.h, which is only in rtc_base and > not > > in rtc_base_approved. > > > > Do we need to use rtc::ScopedFakeClock? > > Maybe can do without scoped fake clock. But without fake clock we have to do at > least a one second sleep :( > See patch set 4 It feels like refactoring fakeclock (which is doable with a task queue instead) is out of scope for this cl. Shall I revert to sleep behavior or make a build target without the test main() included. I don't think inlcuding webrtc_base is that much of an issue for test targets?
On 2017/01/13 13:47:33, språng wrote: > On 2017/01/13 13:33:41, språng wrote: > > On 2017/01/13 13:29:38, mflodman wrote: > > > > > > https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... > > > File webrtc/video/vie_encoder_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/2616393003/diff/80001/webrtc/video/vie_encoder_... > > > webrtc/video/vie_encoder_unittest.cc:14: #include "webrtc/base/fakeclock.h" > > > AFAICT fakeclock.h depends on messagequeue.h, which is only in rtc_base and > > not > > > in rtc_base_approved. > > > > > > Do we need to use rtc::ScopedFakeClock? > > > > Maybe can do without scoped fake clock. But without fake clock we have to do > at > > least a one second sleep :( > > See patch set 4 > > It feels like refactoring fakeclock (which is doable with a task queue instead) > is out of scope for this cl. > Shall I revert to sleep behavior or make a build target without the test main() > included. I don't think inlcuding webrtc_base is that much of an issue for test > targets? I think I'm leaning towards reverting fakeclock usage for now, rather than depending on rtc_base.
sprang@webrtc.org changed reviewers: - mflodman@webrtc.org
Reverted back to sleeping instead of using fake clock, so -mflodman as reviewer. Will update fake_clock in a followup cl.
lgtm (look like Sleep can't be easily avoided, but thank you for trying)
The CQ bit was checked by sprang@webrtc.org
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
Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
The CQ bit was checked by sprang@webrtc.org
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
Failed to apply patch for webrtc/video/vie_encoder_unittest.cc:
While running git apply --index -p1;
error: patch failed: webrtc/video/vie_encoder_unittest.cc:12
error: webrtc/video/vie_encoder_unittest.cc: patch does not apply
Patch: webrtc/video/vie_encoder_unittest.cc
Index: webrtc/video/vie_encoder_unittest.cc
diff --git a/webrtc/video/vie_encoder_unittest.cc
b/webrtc/video/vie_encoder_unittest.cc
index
7dd59212e0e84fac3955ba169b6b25d95d161c6c..a099f3fd71992da27e4a4c6300b36db54eb7d6a1
100644
--- a/webrtc/video/vie_encoder_unittest.cc
+++ b/webrtc/video/vie_encoder_unittest.cc
@@ -12,10 +12,13 @@
#include <utility>
#include "webrtc/base/logging.h"
+#include
"webrtc/modules/video_coding/utility/default_video_bitrate_allocator.h"
#include "webrtc/system_wrappers/include/metrics_default.h"
+#include "webrtc/system_wrappers/include/sleep.h"
#include "webrtc/test/encoder_settings.h"
#include "webrtc/test/fake_encoder.h"
#include "webrtc/test/frame_generator.h"
+#include "webrtc/test/gmock.h"
#include "webrtc/test/gtest.h"
#include "webrtc/video/send_statistics_proxy.h"
#include "webrtc/video/vie_encoder.h"
@@ -34,6 +37,8 @@ namespace webrtc {
using DegredationPreference = VideoSendStream::DegradationPreference;
using ScaleReason = ScalingObserverInterface::ScaleReason;
+using ::testing::_;
+using ::testing::Return;
namespace {
const size_t kMaxPayloadLength = 1440;
@@ -162,19 +167,20 @@ class ViEEncoderTest : public ::testing::Test {
ConfigureEncoder(std::move(video_encoder_config), nack_enabled);
}
- VideoFrame CreateFrame(int64_t ntp_ts, rtc::Event* destruction_event) const {
+ VideoFrame CreateFrame(int64_t ntp_time_ms,
+ rtc::Event* destruction_event) const {
VideoFrame frame(new rtc::RefCountedObject<TestBuffer>(
destruction_event, codec_width_, codec_height_),
99, 99, kVideoRotation_0);
- frame.set_ntp_time_ms(ntp_ts);
+ frame.set_ntp_time_ms(ntp_time_ms);
return frame;
}
- VideoFrame CreateFrame(int64_t ntp_ts, int width, int height) const {
+ VideoFrame CreateFrame(int64_t ntp_time_ms, int width, int height) const {
VideoFrame frame(
new rtc::RefCountedObject<TestBuffer>(nullptr, width, height), 99, 99,
kVideoRotation_0);
- frame.set_ntp_time_ms(ntp_ts);
+ frame.set_ntp_time_ms(ntp_time_ms);
return frame;
}
@@ -978,4 +984,48 @@ TEST_F(ViEEncoderTest, UMACpuLimitedResolutionInPercent) {
1, metrics::NumEvents("WebRTC.Video.CpuLimitedResolutionInPercent", 50));
}
+TEST_F(ViEEncoderTest, CallsBitrateObserver) {
+ class MockBitrateObserver : public VideoBitrateAllocationObserver {
+ public:
+ MOCK_METHOD1(OnBitrateAllocationUpdated, void(const BitrateAllocation&));
+ } bitrate_observer;
+ vie_encoder_->SetBitrateObserver(&bitrate_observer);
+
+ const int kDefaultFps = 30;
+ const BitrateAllocation expected_bitrate =
+ DefaultVideoBitrateAllocator(fake_encoder_.codec_config())
+ .GetAllocation(kTargetBitrateBps, kDefaultFps);
+
+ // First called on bitrate updated, then again on first frame.
+ EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(expected_bitrate))
+ .Times(2);
+ vie_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
+
+ const int64_t kStartTimeMs = 1;
+ video_source_.IncomingCapturedFrame(
+ CreateFrame(kStartTimeMs, codec_width_, codec_height_));
+ sink_.WaitForEncodedFrame(kStartTimeMs);
+
+ // Not called on second frame.
+ EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(expected_bitrate))
+ .Times(0);
+ video_source_.IncomingCapturedFrame(
+ CreateFrame(kStartTimeMs + 1, codec_width_, codec_height_));
+ sink_.WaitForEncodedFrame(kStartTimeMs + 1);
+
+ // Called after a process interval.
+ const int64_t kProcessIntervalMs =
+ vcm::VCMProcessTimer::kDefaultProcessIntervalMs;
+ // TODO(sprang): ViEEncoder should die and/or get injectable clock.
+ // Sleep for one processing interval plus one frame to avoid flakiness.
+ SleepMs(kProcessIntervalMs + 1000 / kDefaultFps);
+ EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(expected_bitrate))
+ .Times(1);
+ video_source_.IncomingCapturedFrame(CreateFrame(
+ kStartTimeMs + kProcessIntervalMs, codec_width_, codec_height_));
+ sink_.WaitForEncodedFrame(kStartTimeMs + kProcessIntervalMs);
+
+ vie_encoder_->Stop();
+}
+
} // namespace webrtc
The CQ bit was checked by sprang@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/2616393003/#ps120001 (title: "Rebase")
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": 120001, "attempt_start_ts": 1484570221571490,
"parent_rev": "84abeb1d37ee27147674cd94a5606bd4ac725d9a", "commit_rev":
"57c2fff361fac137480a3b045d2c6b194b1d0d30"}
Message was sent while issue was closed.
Description was changed from ========== Periodically update channel parameters and send TargetBitrate message. Currently, parameters are periodically updated, but the TargetBitrate message is only sent if a new bitrate is set. It should be sent periodically to indicate the signaled bitrate is valid and to prevent stale values due to loss of RTCP packets. BUG=webrtc:6897 ========== to ========== Periodically update channel parameters and send TargetBitrate message. Currently, parameters are periodically updated, but the TargetBitrate message is only sent if a new bitrate is set. It should be sent periodically to indicate the signaled bitrate is valid and to prevent stale values due to loss of RTCP packets. BUG=webrtc:6897 Review-Url: https://codereview.webrtc.org/2616393003 Cr-Commit-Position: refs/heads/master@{#16096} Committed: https://chromium.googlesource.com/external/webrtc/+/57c2fff361fac137480a3b045... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/57c2fff361fac137480a3b045... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
