Don't report video memory usage estimate when no frames are present.

preload=metadata suspend prevents decoding and rendering of the
first video frame. In these cases the video memory usage is zero,
but WMPI always includes an estimate for the frame held by the
compositor even if no frame is being held.

This artificially inflates the memory used in the preload=metadata
experiment. The fix is to not include this estimate if we've never
painted a frame before.

BUG=833564
TEST=new unittest.

Change-Id: I0b8811f04b27e5cc5c4ea5789cbd3d9247720771
Reviewed-on: https://chromium-review.googlesource.com/1014387
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551167}

media/blink/webmediaplayer_impl.cc

@@ -2729,15 +2729,17 @@ void WebMediaPlayerImpl::FinishMemoryUsageReport(int64_t demuxer_memory_usage) {
   const int64_t data_source_memory_usage =
       data_source_ ? data_source_->GetMemoryUsage() : 0;
 
-  // If we have video and no video memory usage, assume the VideoFrameCompositor
-  // is holding onto the last frame after we've suspended the pipeline; which
-  // thus reports zero memory usage from the video renderer.
+  // If we have video and no video memory usage and we've rendered the first
+  // frame, assume the VideoFrameCompositor is holding onto the last frame after
+  // we've suspended the pipeline; which thus reports zero memory usage from the
+  // video renderer.
   //
   // Technically this should use the coded size, but that requires us to hop to
   // the compositor to get and byte-perfect accuracy isn't important here.
   const int64_t video_memory_usage =
       stats.video_memory_usage +
-      (pipeline_metadata_.has_video && !stats.video_memory_usage
+      ((pipeline_metadata_.has_video && !stats.video_memory_usage &&
+        has_first_frame_)
            ? VideoFrame::AllocationSize(PIXEL_FORMAT_I420,
                                         pipeline_metadata_.natural_size)
            : 0);
@@ -3132,6 +3134,7 @@ void WebMediaPlayerImpl::SetTickClockForTest(
 void WebMediaPlayerImpl::OnFirstFrame(base::TimeTicks frame_time) {
   DCHECK(!load_start_time_.is_null());
   DCHECK(!skip_metrics_due_to_startup_suspend_);
+  has_first_frame_ = true;
   const base::TimeDelta elapsed = frame_time - load_start_time_;
   media_metrics_provider_->SetTimeToFirstFrame(elapsed);
   RecordTimingUMA("Media.TimeToFirstFrame", elapsed);

media/blink/webmediaplayer_impl.h

@@ -897,6 +897,9 @@ class MEDIA_BLINK_EXPORT WebMediaPlayerImpl
   // preload=metadata. Cleared upon pipeline startup.
   bool attempting_suspended_start_ = false;
 
+  // True if a frame has ever been rendered.
+  bool has_first_frame_ = false;
+
   // Keeps track of the SurfaceId for Picture-in-Picture. This is used to
   // route the video to be shown in the Picture-in-Picture window.
   viz::SurfaceId pip_surface_id_;

media/blink/webmediaplayer_impl_unittest.cc

@@ -84,10 +84,6 @@ const base::TimeDelta kMaxKeyframeDistanceToDisableBackgroundVideo =
 const base::TimeDelta kMaxKeyframeDistanceToDisableBackgroundVideoMSE =
     base::TimeDelta::FromSeconds(10);
 
-int64_t OnAdjustAllocatedMemory(int64_t delta) {
-  return 0;
-}
-
 MATCHER(WmpiDestroyed, "") {
   return CONTAINS_STRING(arg, "WEBMEDIAPLAYER_DESTROYED {}");
 }
@@ -358,8 +354,9 @@ class WebMediaPlayerImplTest : public testing::Test {
         std::move(media_log), WebMediaPlayerParams::DeferLoadCB(), audio_sink_,
         media_thread_.task_runner(), base::ThreadTaskRunnerHandle::Get(),
         base::ThreadTaskRunnerHandle::Get(), media_thread_.task_runner(),
-        base::BindRepeating(&OnAdjustAllocatedMemory), nullptr, nullptr,
-        RequestRoutingTokenCallback(), nullptr,
+        base::BindRepeating(&WebMediaPlayerImplTest::OnAdjustAllocatedMemory,
+                            base::Unretained(this)),
+        nullptr, nullptr, RequestRoutingTokenCallback(), nullptr,
         kMaxKeyframeDistanceToDisableBackgroundVideo,
         kMaxKeyframeDistanceToDisableBackgroundVideoMSE, false, false,
         std::move(provider),
@@ -406,6 +403,11 @@ class WebMediaPlayerImplTest : public testing::Test {
     return std::move(surface_layer_bridge_);
   }
 
+  int64_t OnAdjustAllocatedMemory(int64_t delta) {
+    reported_memory_ += delta;
+    return 0;
+  }
+
   void SetNetworkState(blink::WebMediaPlayer::NetworkState state) {
     EXPECT_CALL(client_, NetworkStateChanged());
     wmpi_->SetNetworkState(state);
@@ -498,6 +500,10 @@ class WebMediaPlayerImplTest : public testing::Test {
 
   bool IsSuspended() { return wmpi_->pipeline_controller_.IsSuspended(); }
 
+  int64_t GetDataSourceMemoryUsage() const {
+    return wmpi_->data_source_->GetMemoryUsage();
+  }
+
   void AddBufferedRanges() {
     wmpi_->buffered_data_source_host_.AddBufferedByteRange(0, 1);
   }
@@ -667,6 +673,9 @@ class WebMediaPlayerImplTest : public testing::Test {
   // verifying a subset of potential media logs.
   NiceMock<MockMediaLog>* media_log_ = nullptr;
 
+  // Total memory in bytes allocated by the WebMediaPlayerImpl instance.
+  int64_t reported_memory_ = 0;
+
   // The WebMediaPlayerImpl instance under test.
   std::unique_ptr<WebMediaPlayerImpl> wmpi_;
 
@@ -691,6 +700,13 @@ TEST_F(WebMediaPlayerImplTest, LoadAndDestroy) {
   EXPECT_FALSE(IsSuspended());
   LoadAndWaitForMetadata(kAudioOnlyTestFile);
   EXPECT_FALSE(IsSuspended());
+  base::RunLoop().RunUntilIdle();
+
+  // The data source contains the entire file, so subtract it from the memory
+  // usage to ensure we're getting audio buffer and demuxer usage too.
+  const int64_t data_source_size = GetDataSourceMemoryUsage();
+  EXPECT_GT(data_source_size, 0);
+  EXPECT_GT(reported_memory_ - data_source_size, 0);
 }
 
 // Verify that preload=metadata suspend works properly.
@@ -705,6 +721,33 @@ TEST_F(WebMediaPlayerImplTest, LoadPreloadMetadataSuspend) {
   EXPECT_CALL(client_, ReadyStateChanged()).Times(AnyNumber());
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(IsSuspended());
+
+  // The data source contains the entire file, so subtract it from the memory
+  // usage to ensure there's no other memory usage.
+  const int64_t data_source_size = GetDataSourceMemoryUsage();
+  EXPECT_GT(data_source_size, 0);
+  EXPECT_EQ(reported_memory_ - data_source_size, 0);
+}
+
+// Verify that preload=metadata suspend video w/ poster uses zero video memory.
+TEST_F(WebMediaPlayerImplTest, LoadPreloadMetadataSuspendNoVideoMemoryUsage) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(media::kPreloadMetadataSuspend);
+  InitializeWebMediaPlayerImpl();
+  EXPECT_CALL(client_, CouldPlayIfEnoughData()).WillRepeatedly(Return(false));
+  wmpi_->SetPreload(blink::WebMediaPlayer::kPreloadMetaData);
+  wmpi_->SetPoster(blink::WebURL(GURL("file://example.com/sample.jpg")));
+  LoadAndWaitForMetadata("bear-320x240-video-only.webm");
+  testing::Mock::VerifyAndClearExpectations(&client_);
+  EXPECT_CALL(client_, ReadyStateChanged()).Times(AnyNumber());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(IsSuspended());
+
+  // The data source contains the entire file, so subtract it from the memory
+  // usage to ensure there's no other memory usage.
+  const int64_t data_source_size = GetDataSourceMemoryUsage();
+  EXPECT_GT(data_source_size, 0);
+  EXPECT_EQ(reported_memory_ - data_source_size, 0);
 }
 
 // Verify that preload=metadata suspend is aborted if we know the element will