Skip to content

Commit

Permalink
Merge M59: "Start reporting watch time if ABR adapts above 200p."
Browse files Browse the repository at this point in the history
Previously we only checked the initial resolution and assumed it
was always large enough to start watch time reporting; clearly
this is not true since YouTube offers a 140p format.

Probably we also want to drop the minimum resolution to 200x140p,
which I'll check with product about and handle in a followup CL.

BUG=711792
TEST=new tests. Manual: force 140p, adapt above, verify watch time.

Review-Url: https://codereview.chromium.org/2822543006
Cr-Commit-Position: refs/heads/master@{#464826}
(cherry picked from commit 2540556)

Review-Url: https://codereview.chromium.org/2819323002 .
Cr-Commit-Position: refs/branch-heads/3071@{#21}
Cr-Branched-From: a106f0a-refs/heads/master@{#464641}
  • Loading branch information
dalecurtis committed Apr 17, 2017
1 parent 0398002 commit 4c8965a
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 8 deletions.
9 changes: 6 additions & 3 deletions media/blink/watch_time_reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,11 @@ void WatchTimeReporter::OnHidden() {
MaybeFinalizeWatchTime(FinalizeTime::ON_NEXT_UPDATE);
}

bool WatchTimeReporter::IsSizeLargeEnoughToReportWatchTime() const {
return initial_video_size_.height() >= kMinimumVideoSize.height() &&
initial_video_size_.width() >= kMinimumVideoSize.width();
}

void WatchTimeReporter::OnPowerStateChange(bool on_battery_power) {
if (!reporting_timer_.IsRunning())
return;
Expand All @@ -184,9 +189,7 @@ bool WatchTimeReporter::ShouldReportWatchTime() {
// Report listen time or watch time only for tracks that are audio-only or
// have both an audio and video track of sufficient size.
return (!has_video_ && has_audio_) ||
(has_video_ && has_audio_ &&
initial_video_size_.height() >= kMinimumVideoSize.height() &&
initial_video_size_.width() >= kMinimumVideoSize.width());
(has_video_ && has_audio_ && IsSizeLargeEnoughToReportWatchTime());
}

void WatchTimeReporter::MaybeStartReportingTimer(
Expand Down
4 changes: 4 additions & 0 deletions media/blink/watch_time_reporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ class MEDIA_BLINK_EXPORT WatchTimeReporter : base::PowerObserver {
void OnShown();
void OnHidden();

// Returns true if the current size is large enough that watch time will be
// recorded for playback.
bool IsSizeLargeEnoughToReportWatchTime() const;

// Setup the reporting interval to be immediate to avoid spinning real time
// within the unit test.
void set_reporting_interval_for_testing() {
Expand Down
9 changes: 9 additions & 0 deletions media/blink/watch_time_reporter_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -279,39 +279,48 @@ TEST_P(WatchTimeReporterTest, WatchTimeReporter) {
Initialize(!has_video_, true, true, gfx::Size());
wtr_->OnPlaying();
EXPECT_EQ(!has_video_, IsMonitoring());
EXPECT_FALSE(wtr_->IsSizeLargeEnoughToReportWatchTime());

Initialize(true, true, true, gfx::Size());
wtr_->OnPlaying();
EXPECT_EQ(!has_video_, IsMonitoring());
EXPECT_FALSE(wtr_->IsSizeLargeEnoughToReportWatchTime());

constexpr gfx::Size kSizeTooSmall = gfx::Size(100, 100);
Initialize(!has_video_, true, true, kSizeTooSmall);
wtr_->OnPlaying();
EXPECT_EQ(!has_video_, IsMonitoring());
EXPECT_FALSE(wtr_->IsSizeLargeEnoughToReportWatchTime());

Initialize(true, true, true, kSizeJustRight);
wtr_->OnPlaying();
EXPECT_TRUE(IsMonitoring());
EXPECT_TRUE(wtr_->IsSizeLargeEnoughToReportWatchTime());

Initialize(true, false, false, kSizeJustRight);
wtr_->OnPlaying();
EXPECT_TRUE(IsMonitoring());
EXPECT_TRUE(wtr_->IsSizeLargeEnoughToReportWatchTime());

Initialize(true, true, false, kSizeJustRight);
wtr_->OnPlaying();
EXPECT_TRUE(IsMonitoring());
EXPECT_TRUE(wtr_->IsSizeLargeEnoughToReportWatchTime());

Initialize(true, true, true, gfx::Size());
wtr_->OnPlaying();
EXPECT_EQ(!has_video_, IsMonitoring());
EXPECT_FALSE(wtr_->IsSizeLargeEnoughToReportWatchTime());

Initialize(true, false, false, gfx::Size());
wtr_->OnPlaying();
EXPECT_EQ(!has_video_, IsMonitoring());
EXPECT_FALSE(wtr_->IsSizeLargeEnoughToReportWatchTime());

Initialize(true, true, false, gfx::Size());
wtr_->OnPlaying();
EXPECT_EQ(!has_video_, IsMonitoring());
EXPECT_FALSE(wtr_->IsSizeLargeEnoughToReportWatchTime());

if (!has_video_)
EXPECT_WATCH_TIME_FINALIZED();
Expand Down
10 changes: 5 additions & 5 deletions media/blink/webmediaplayer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1419,13 +1419,13 @@ void WebMediaPlayerImpl::OnVideoNaturalSizeChange(const gfx::Size& size) {
if (overlay_enabled_ && surface_manager_)
surface_manager_->NaturalSizeChanged(rotated_size);

gfx::Size old_size = pipeline_metadata_.natural_size;
pipeline_metadata_.natural_size = rotated_size;
if (old_size.IsEmpty()) {
// WatchTimeReporter doesn't report metrics for empty videos. Re-create
// |watch_time_reporter_| if we didn't originally know the video size.

// Re-create |watch_time_reporter_| if we didn't originally know the video
// size or the previous size was too small for reporting.
if (!watch_time_reporter_->IsSizeLargeEnoughToReportWatchTime())
CreateWatchTimeReporter();
}

client_->SizeChanged();

if (observer_)
Expand Down

0 comments on commit 4c8965a

Please sign in to comment.