Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
186043003:
Cast: Using a min filter to compute time offset (Closed)
|
Created:
6 years, 9 months ago by mikhal1 Modified:
6 years, 9 months ago CC:
chromium-reviews, hclam+watch_chromium.org, mikhal+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, pwestin+watch_google.com, feature-media-reviews_chromium.org, hubbe+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCast: Using a min filter to compute time offset
Modifying the time offset filter to take to minimum of the first 10 values.
Once ten offset values were seen, the value will be constant for the entire session.
This may be justified by the fact we are seeking the offset between sender and
receiver without accounting the network jitter.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255166
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256134
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256342
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Responding to review #
Total comments: 8
Patch Set 4 : Responding to review #
Total comments: 2
Patch Set 5 : Changing to std::min #Patch Set 6 : Updates #Patch Set 7 : rebase #Patch Set 8 : Updated test and rebased #
Messages
Total messages: 34 (0 generated)
https://codereview.chromium.org/186043003/diff/100001/media/cast/video_receiv... File media/cast/video_receiver/video_receiver.cc (right): https://codereview.chromium.org/186043003/diff/100001/media/cast/video_receiv... media/cast/video_receiver/video_receiver.cc:379: ++time_offset_counter_; The previous filter was basically doing: time_offset_ = ((7 * time_offset_) + time_offset) / 8; Could the issue have been that when you only have for example 2 samples this was basically doing: time_offset_ = ((7 * first_time_offset_sample) + second_time_offset_sample) / 8; In other words, the first sample was counting for 7/8 of the new average, vs. only 1/8 for the second sample. In general, first samples were counting for more.
https://chromiumcodereview.appspot.com/186043003/diff/100001/media/cast/video... File media/cast/video_receiver/video_receiver.cc (right): https://chromiumcodereview.appspot.com/186043003/diff/100001/media/cast/video... media/cast/video_receiver/video_receiver.cc:379: ++time_offset_counter_; That is indeed part of the problem, which can be described as follows: 1. The initial value can be very off if sender starts before receiver. 2. Given the slow adaptation of this filter, conversion from a bad value took a very long time. 3. In an unstable wi-fi setting, these values may fluctuate significantly. We are not interested in these fluctuations, but in finding the offset, which will be best estimated at the best network setting, i.e. minimum value over time. On 2014/03/03 23:20:59, hguihot1 wrote: > The previous filter was basically doing: > > time_offset_ = ((7 * time_offset_) + time_offset) / 8; > > Could the issue have been that when you only have for example 2 samples this was > basically doing: > > time_offset_ = ((7 * first_time_offset_sample) + second_time_offset_sample) / 8; > > In other words, the first sample was counting for 7/8 of the new average, vs. > only 1/8 for the second sample. In general, first samples were counting for > more.
2 comments https://codereview.chromium.org/186043003/diff/100001/media/cast/video_receiv... File media/cast/video_receiver/video_receiver.cc (right): https://codereview.chromium.org/186043003/diff/100001/media/cast/video_receiv... media/cast/video_receiver/video_receiver.cc:379: ++time_offset_counter_; On 2014/03/03 23:20:59, hguihot1 wrote: > The previous filter was basically doing: > > time_offset_ = ((7 * time_offset_) + time_offset) / 8; > > Could the issue have been that when you only have for example 2 samples this was > basically doing: > > time_offset_ = ((7 * first_time_offset_sample) + second_time_offset_sample) / 8; > > In other words, the first sample was counting for 7/8 of the new average, vs. > only 1/8 for the second sample. In general, first samples were counting for > more. Mikhal based on the date we have seen in testing I think we should ignore the first value https://codereview.chromium.org/186043003/diff/100001/media/cast/video_receiv... media/cast/video_receiver/video_receiver.cc:415: base::TimeDelta::FromMilliseconds(kMinTimeBetweenOffsetUpdatesMs)) { kMinTimeBetweenOffsetUpdatesMs should be decreased to get a faster convergence
PTAL https://chromiumcodereview.appspot.com/186043003/diff/100001/media/cast/video... File media/cast/video_receiver/video_receiver.cc (right): https://chromiumcodereview.appspot.com/186043003/diff/100001/media/cast/video... media/cast/video_receiver/video_receiver.cc:379: ++time_offset_counter_; True, just thought of it as I was responding to hguihot. On 2014/03/03 23:35:47, pwestin wrote: > On 2014/03/03 23:20:59, hguihot1 wrote: > > The previous filter was basically doing: > > > > time_offset_ = ((7 * time_offset_) + time_offset) / 8; > > > > Could the issue have been that when you only have for example 2 samples this > was > > basically doing: > > > > time_offset_ = ((7 * first_time_offset_sample) + second_time_offset_sample) / > 8; > > > > In other words, the first sample was counting for 7/8 of the new average, vs. > > only 1/8 for the second sample. In general, first samples were counting for > > more. > > Mikhal based on the date we have seen in testing I think we should ignore the > first value https://chromiumcodereview.appspot.com/186043003/diff/100001/media/cast/video... media/cast/video_receiver/video_receiver.cc:415: base::TimeDelta::FromMilliseconds(kMinTimeBetweenOffsetUpdatesMs)) { On 2014/03/03 23:35:47, pwestin wrote: > kMinTimeBetweenOffsetUpdatesMs should be decreased to get a faster convergence Done.
https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... File media/cast/video_receiver/video_receiver.cc (right): https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:378: time_offset_ = time_offset; This will select the minimum value within the first 10 samples (9 actually since the first one is ignored). Is that the intent? There is still no guarantee we'll get a 'good' time_offset_ during that time, so wouldn't it make more sense the select the minimum within a moving window?
more comments https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... File media/cast/video_receiver/video_receiver.cc (right): https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:110: time_offset_counter_(0), we should initialize time_offset_ https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:368: ++time_offset_counter_; This will result in a bad first value; since we have no valid time_offset_; I think we should return now https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:377: time_offset < time_offset_) we should do if (time_offset_counter_ == 1) time_offset_ = time_offset; else if (time_offset_counter_ < kTimeOffsetMaxCounter) time_offset_ = std::min(time_offset_, time_offset);
PTAL https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... File media/cast/video_receiver/video_receiver.cc (right): https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:110: time_offset_counter_(0), will be initialized by the default const' On 2014/03/04 00:10:54, pwestin wrote: > we should initialize time_offset_ https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:368: ++time_offset_counter_; true. fixed. On 2014/03/04 00:10:54, pwestin wrote: > This will result in a bad first value; since we have no valid time_offset_; I > think we should return now https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:377: time_offset < time_offset_) updated. On 2014/03/04 00:10:54, pwestin wrote: > we should do > if (time_offset_counter_ == 1) > time_offset_ = time_offset; > else if (time_offset_counter_ < kTimeOffsetMaxCounter) > time_offset_ = std::min(time_offset_, time_offset); https://chromiumcodereview.appspot.com/186043003/diff/170001/media/cast/video... media/cast/video_receiver/video_receiver.cc:378: time_offset_ = time_offset; That indeed is the intention, the underlying assumption is that the drift is insignificant (for the duration of a reasonable session), and therefore can be ignored. Added a comment. On 2014/03/04 00:06:40, hguihot1 wrote: > This will select the minimum value within the first 10 samples (9 actually since > the first one is ignored). Is that the intent? There is still no guarantee we'll > get a 'good' time_offset_ during that time, so wouldn't it make more sense the > select the minimum within a moving window?
https://chromiumcodereview.appspot.com/186043003/diff/190001/media/cast/video... File media/cast/video_receiver/video_receiver.cc (right): https://chromiumcodereview.appspot.com/186043003/diff/190001/media/cast/video... media/cast/video_receiver/video_receiver.cc:386: time_offset_ = time_offset; I think taking the std::min makes it more readable
PTAL https://chromiumcodereview.appspot.com/186043003/diff/190001/media/cast/video... File media/cast/video_receiver/video_receiver.cc (right): https://chromiumcodereview.appspot.com/186043003/diff/190001/media/cast/video... media/cast/video_receiver/video_receiver.cc:386: time_offset_ = time_offset; On 2014/03/04 00:34:40, pwestin wrote: > I think taking the std::min makes it more readable Done.
lgtm
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/210001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/210001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/210001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/210001
Message was sent while issue was closed.
Change committed as 255166
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for media/cast/test/end2end_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file media/cast/test/end2end_unittest.cc
Hunk #1 FAILED at 396.
Hunk #2 succeeded at 894 (offset 6 lines).
Hunk #3 succeeded at 942 (offset 6 lines).
1 out of 3 hunks FAILED -- saving rejects to file
media/cast/test/end2end_unittest.cc.rej
Patch: media/cast/test/end2end_unittest.cc
Index: media/cast/test/end2end_unittest.cc
diff --git a/media/cast/test/end2end_unittest.cc
b/media/cast/test/end2end_unittest.cc
index
1524c8f4d7a9b6889863a42991ebd531802fc49b..db4bd9a8bf04efd5519d480a88409ae3e70b05cf
100644
--- a/media/cast/test/end2end_unittest.cc
+++ b/media/cast/test/end2end_unittest.cc
@@ -396,8 +396,8 @@ class End2EndTest : public ::testing::Test {
: start_time_(),
testing_clock_sender_(new base::SimpleTestTickClock()),
testing_clock_receiver_(new base::SimpleTestTickClock()),
- task_runner_(new test::FakeSingleThreadTaskRunner(
- testing_clock_sender_)),
+ task_runner_(
+ new test::FakeSingleThreadTaskRunner(testing_clock_sender_)),
logging_config_(GetLoggingConfigWithRawEventsAndStatsEnabled()),
cast_environment_sender_(new CastEnvironment(
scoped_ptr<base::TickClock>(testing_clock_sender_).Pass(),
@@ -888,21 +888,24 @@ TEST_F(End2EndTest, GlitchWith3Buffers) {
Create();
int video_start = kVideoStart;
- base::TimeTicks send_time = testing_clock_sender_->NowTicks();
- SendVideoFrame(video_start, send_time);
- RunTasks(kFrameTimerMs);
-
- test_receiver_video_callback_->AddExpectedResult(video_start,
- video_sender_config_.width,
- video_sender_config_.height,
- send_time);
- frame_receiver_->GetRawVideoFrame(
- base::Bind(&TestReceiverVideoCallback::CheckVideoFrame,
- test_receiver_video_callback_));
-
- RunTasks(750); // Make sure that we send a RTCP packet.
-
- video_start++;
+ base::TimeTicks send_time;
+ // Frames will rendered on completion until the render time stabilizes, i.e.
+ // we got enough data.
+ const int frames_before_glitch = 20;
+ for (int i = 0; i < frames_before_glitch; ++i) {
+ send_time = testing_clock_sender_->NowTicks();
+ SendVideoFrame(video_start, send_time);
+ test_receiver_video_callback_->AddExpectedResult(
+ video_start,
+ video_sender_config_.width,
+ video_sender_config_.height,
+ send_time);
+ frame_receiver_->GetRawVideoFrame(
+ base::Bind(&TestReceiverVideoCallback::CheckVideoFrame,
+ test_receiver_video_callback_));
+ RunTasks(kFrameTimerMs);
+ video_start++;
+ }
// Introduce a glitch lasting for 10 frames.
sender_to_receiver_.SetSendPackets(false);
@@ -933,7 +936,8 @@ TEST_F(End2EndTest, GlitchWith3Buffers) {
test_receiver_video_callback_));
RunTasks(2 * kFrameTimerMs + 1); // Empty the receiver pipeline.
- EXPECT_EQ(2, test_receiver_video_callback_->number_times_called());
+ EXPECT_EQ(frames_before_glitch + 1,
+ test_receiver_video_callback_->number_times_called());
}
TEST_F(End2EndTest, DropEveryOtherFrame3Buffers) {
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/240001
Message was sent while issue was closed.
Change committed as 256134
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/186043003/260001
Message was sent while issue was closed.
Change committed as 256342 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
