Fire canplaythrough as soon as download defers to fix autoplay

media/base/download_rate_monitor_unittest.cc

Index: media/base/download_rate_monitor_unittest.cc
diff --git a/media/base/download_rate_monitor_unittest.cc b/media/base/download_rate_monitor_unittest.cc
index 321ad630e51c0011a8551386120f22f1846bb851..b689df2f38c863d917c5165a751f215dc6e861fd 100644
--- a/media/base/download_rate_monitor_unittest.cc
+++ b/media/base/download_rate_monitor_unittest.cc
@@ -15,8 +15,11 @@ namespace media {
 
 class DownloadRateMonitorTest : public ::testing::Test {
  public:
-  DownloadRateMonitorTest() {
-    monitor_.set_total_bytes(kMediaSizeInBytes);
+  DownloadRateMonitorTest()
+      : canplaythrough_cb_(base::Bind(&DownloadRateMonitorTest::CanPlayThrough,
+                                      base::Unretained(this))) {
+      data_source_.reset(
+          new FakeDataSource(kMediaSizeInBytes, kDeferThreshold, 0, &host_));
   }
 
   virtual ~DownloadRateMonitorTest() { }
@@ -24,142 +27,224 @@ class DownloadRateMonitorTest : public ::testing::Test {
   MOCK_METHOD0(CanPlayThrough, void());
 
  protected:
-  static const int kMediaSizeInBytes = 20 * 1024 * 1024;
+  static const int kMediaDuration = 120;
+  static const int kMediaBitrate = 1024 * 1024 * 8;
+  static const int kMediaByterate = kMediaBitrate / 8;
+  static const int kMediaSizeInBytes = kMediaDuration * kMediaByterate;
+  static const int kDeferThreshold = 30 * kMediaByterate;
 
   // Simulates downloading of the media file. Packets are timed evenly in
   // |ms_between_packets| intervals, starting at |starting_time|, which is
   // number of seconds since unix epoch (Jan 1, 1970).
   // Returns the number of bytes buffered in the media file after the
   // network activity.

[acolwell GONE FROM CHROMIUM, 2012/01/09 23:16:57]

Is this last sentence true still? Isn't the return value the amount of data
"received" inside this call?

[vrk (LEFT CHROMIUM), 2012/01/11 00:23:13]

No longer true, but n/a now.

-  int SimulateNetwork(double starting_time,
-                      int starting_bytes,
-                      int bytes_per_packet,
-                      int ms_between_packets,
-                      int number_of_packets) {
-    int bytes_buffered = starting_bytes;
+  int SimulateNetwork(double starting_time, int bytes_per_packet,

[acolwell GONE FROM CHROMIUM, 2012/01/09 23:16:57]

I think this signature still makes the tests a little confusing. Don't you just
want to say, "Download x bytes at y rate?" It seems like packet size/count could
be hidden in this method. I think start_time could be a member variable that
could get incremented on each 'receive'. If we need to introduce a delay, we
could add a SimulateNetworkDelay(). What do you think?

[vrk (LEFT CHROMIUM), 2012/01/11 00:23:13]

Yes, a lot cleaner! Thanks, done!

+                      int ms_between_packets, int number_of_packets) {
+    int bytes_buffered = 0;
     base::Time packet_time = base::Time::FromDoubleT(starting_time);
 
-    monitor_.SetNetworkActivity(true);
     // Loop executes (number_of_packets + 1) times because a packet needs a
     // starting and end point.
     for (int i = 0; i < number_of_packets + 1; ++i) {
-      monitor_.SetBufferedBytes(bytes_buffered, packet_time);
-      packet_time += base::TimeDelta::FromMilliseconds(ms_between_packets);
+      if (data_source_->is_deferred())
+        break;
+
+      data_source_->ReceiveData(bytes_per_packet, packet_time);
       bytes_buffered += bytes_per_packet;
+
+      packet_time += base::TimeDelta::FromMilliseconds(ms_between_packets);
     }
-    monitor_.SetNetworkActivity(false);
     return bytes_buffered;
   }
 
-  void StartMonitor(int bitrate) {
-    StartMonitor(bitrate, false, false);
+  void Initialize() {
+    Initialize(false, false);
+  }
+
+  void Initialize(bool streaming, bool local_source) {
+    host_.Initialize(
+        kMediaBitrate, streaming, local_source, canplaythrough_cb_);
   }
 
-  void StartMonitor(int bitrate, bool streaming, bool local_source) {
-    monitor_.Start(base::Bind(&DownloadRateMonitorTest::CanPlayThrough,
-                   base::Unretained(this)), bitrate, streaming, local_source);
+  void SeekTo(int byte_position) {
+    data_source_.reset(new FakeDataSource(

[acolwell GONE FROM CHROMIUM, 2012/01/09 23:16:57]

Consider converting this to a Seek() method on FakeDataSource. I think that
would be clearer.

[vrk (LEFT CHROMIUM), 2012/01/11 00:23:13]

Yeah, I think I was trying too hard to replicate what the Pipeline actually does
:) (recreates the resource loader on seeks). Done.

+        kMediaSizeInBytes, kDeferThreshold, byte_position, &host_));
   }
 
-  DownloadRateMonitor monitor_;
+  bool DownloadIsDeferred() {
+    return data_source_->is_deferred();
+  }
 
  private:
+  // Helper class that simulates the filter host that owns and communicates with
+  // the |monitor_|.
+  class FakeFilterHost {

[acolwell GONE FROM CHROMIUM, 2012/01/09 23:16:57]

I think you should just put these methods in FakeDataSource. I don't think
pulling them out into a separate class makes things clearer.

[vrk (LEFT CHROMIUM), 2012/01/11 00:23:13]

Done.

+   public:
+    void UpdateHostState(bool is_downloading_data, int total_bytes,
+                         int buffered_bytes, base::Time last_packet_time) {
+      monitor_.SetNetworkActivity(is_downloading_data);
+      monitor_.set_total_bytes(total_bytes);
+      monitor_.SetBufferedBytes(buffered_bytes, last_packet_time);
+    }
+
+    void Initialize(int bitrate, bool streaming, bool local_source,
+                    const base::Closure& canplaythrough_cb) {
+      monitor_.Start(canplaythrough_cb, bitrate, streaming, local_source);
+    }
+
+   private:
+    DownloadRateMonitor monitor_;
+  };
+
+  // Helper class to simulate a buffered data source.
+  class FakeDataSource {
+   public:
+    FakeDataSource(int total_bytes, int defer_threshold, int offset,
+                   FakeFilterHost* host)
+        : total_bytes_(total_bytes),
+          buffered_bytes_(0),
+          offset_(offset),
+          host_(host),
+          defer_threshold_(defer_threshold),
+          is_downloading_data_(true) { }
+
+    bool is_deferred() { return !is_downloading_data_; }
+
+    void ReceiveData(int bytes_received, base::Time packet_time) {
+      CHECK(is_downloading_data_);
+      buffered_bytes_ += bytes_received;
+      if (buffered_bytes_ > defer_threshold_)

[acolwell GONE FROM CHROMIUM, 2012/01/09 23:16:57]

How about a comment stating that you are simulating a deferral here?

[vrk (LEFT CHROMIUM), 2012/01/11 00:23:13]

Done.

+        is_downloading_data_ = false;
+      host_->UpdateHostState(is_downloading_data_, total_bytes_,
+                             buffered_bytes_ + offset_, packet_time);
+    }
+
+   private:
+    // Size of the media file being downloaded, in bytes.
+    int total_bytes_;
+
+    // Number of bytes currently in buffer.
+    int buffered_bytes_;
+
+    // The byte offset into the file from which the download began.
+    int offset_;
+
+    // Unowned reference to the filter host.
+    FakeFilterHost* host_;
+
+    // The size of |buffered_bytes_| at which downloading should defer.
+    int defer_threshold_;
+
+    // True if download is active (not deferred_, false otherwise.
+    bool is_downloading_data_;
+  };
+
+  FakeFilterHost host_;
+  scoped_ptr<FakeDataSource> data_source_;
+  base::Closure canplaythrough_cb_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadRateMonitorTest);
 };
 
 TEST_F(DownloadRateMonitorTest, DownloadRateGreaterThanBitrate) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-
   // Simulate downloading at double the media's bitrate.
-  StartMonitor(media_bitrate);
+  Initialize();
   EXPECT_CALL(*this, CanPlayThrough());
-  SimulateNetwork(1, 0, 2 * media_bitrate / 8, 1000, 10);
+  SimulateNetwork(1, 2 * kMediaByterate, 1000, 10);
 }
 
-// If the user pauses and the pipeline stops downloading data, make sure the
-// DownloadRateMonitor understands that the download is not stalling.
-TEST_F(DownloadRateMonitorTest, DownloadRateGreaterThanBitrate_Pause) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-  static const int download_byte_rate = 1.1 * media_bitrate / 8;
+TEST_F(DownloadRateMonitorTest, DownloadRateLessThanBitrate_Defer) {
+  Initialize();
 
-  // Start downloading faster than the media's bitrate.
-  StartMonitor(media_bitrate);
+  // Download slower than the media's bitrate, but buffer enough data such that
+  // the data source defers.
+  int bytes_buffered = 0;
   EXPECT_CALL(*this, CanPlayThrough());
-  int buffered = SimulateNetwork(1, 0, download_byte_rate, 1000, 2);
-
-  // Then "pause" for 3 minutes and continue downloading at same rate.
-  SimulateNetwork(60 * 3, buffered, download_byte_rate, 1000, 4);
+  int time = 1;
+  while(bytes_buffered < kDeferThreshold) {
+    int downloaded_bytes =
+        SimulateNetwork(time++, 0.3 * kMediaByterate, 1000, 1);
+    if (downloaded_bytes == 0)
+      break;
+    bytes_buffered += downloaded_bytes;
+  }
+  CHECK(DownloadIsDeferred());
 }
 
 TEST_F(DownloadRateMonitorTest, DownloadRateGreaterThanBitrate_SeekForward) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-  static const int download_byte_rate = 1.1 * media_bitrate / 8;
-
   // Start downloading faster than the media's bitrate.
-  EXPECT_CALL(*this, CanPlayThrough());
-  StartMonitor(media_bitrate);
-  SimulateNetwork(1, 0, download_byte_rate, 1000, 2);
+  Initialize();
+  SimulateNetwork(1, 1.1 * kMediaByterate, 1000, 2);
 
   // Then seek forward mid-file and continue downloading at same rate.
-  SimulateNetwork(4, kMediaSizeInBytes / 2, download_byte_rate, 1000, 4);
+  SeekTo(kMediaSizeInBytes / 2);
+  EXPECT_CALL(*this, CanPlayThrough());
+  SimulateNetwork(4, 1.1 * kMediaByterate, 1000, 4);
+
+  // CanPlayThrough should not be fired because of deferring.
+  CHECK(!DownloadIsDeferred());
 }
 
 TEST_F(DownloadRateMonitorTest, DownloadRateGreaterThanBitrate_SeekBackward) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-  static const int download_byte_rate = 1.1 * media_bitrate / 8;
-
   // Start downloading faster than the media's bitrate, in middle of file.
-  StartMonitor(media_bitrate);
-  SimulateNetwork(1, kMediaSizeInBytes / 2, download_byte_rate, 1000, 2);
+  Initialize();
+  SeekTo(kMediaSizeInBytes / 2);
+  SimulateNetwork(1, 1.1 * kMediaByterate, 1000, 2);
 
   // Then seek back to beginning and continue downloading at same rate.
+  SeekTo(0);
   EXPECT_CALL(*this, CanPlayThrough());
-  SimulateNetwork(4, 0, download_byte_rate, 1000, 4);
+  SimulateNetwork(4, 1.1 * kMediaByterate, 1000, 4);
+
+  // CanPlayThrough should not be fired because of deferring.
+  CHECK(!DownloadIsDeferred());
 }
 
 TEST_F(DownloadRateMonitorTest, DownloadRateLessThanBitrate) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-
   // Simulate downloading at half the media's bitrate.
   EXPECT_CALL(*this, CanPlayThrough())
       .Times(0);
-  StartMonitor(media_bitrate);
-  SimulateNetwork(1, 0, media_bitrate / 8 / 2, 1000, 10);
+  Initialize();
+  SimulateNetwork(1, kMediaByterate / 2, 1000, 10);
 }
 
 TEST_F(DownloadRateMonitorTest, MediaSourceIsLocal) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-
   // Simulate no data downloaded.
   EXPECT_CALL(*this, CanPlayThrough());
-  StartMonitor(media_bitrate, false, true);
+  Initialize(false, true);
 }
 
 TEST_F(DownloadRateMonitorTest, MediaSourceIsStreaming) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-
   // Simulate downloading at the media's bitrate while streaming.
   EXPECT_CALL(*this, CanPlayThrough());
-  StartMonitor(media_bitrate, true, false);
-  SimulateNetwork(1, 0, media_bitrate / 8, 1000, 10);
+  Initialize(true, false);
+  SimulateNetwork(1, kMediaByterate, 1000, 10);
 }
 
 TEST_F(DownloadRateMonitorTest, VeryFastDownloadRate) {
-  static const int media_bitrate = 1024 * 1024 * 8;
-
   // Simulate downloading half the video very quickly in one chunk.
-  StartMonitor(media_bitrate);
+  Initialize();
   EXPECT_CALL(*this, CanPlayThrough());
-  SimulateNetwork(1, 0, kMediaSizeInBytes / 2, 10, 1);
+  SimulateNetwork(1, kMediaSizeInBytes / 2, 10, 1);
 }
 
 TEST_F(DownloadRateMonitorTest, DownloadEntireVideo) {
-  static const int seconds_of_data = 20;
-  static const int media_bitrate = kMediaSizeInBytes * 8 / seconds_of_data;
-
   // Simulate downloading entire video at half the bitrate of the video.
-  StartMonitor(media_bitrate);
+  Initialize();
   EXPECT_CALL(*this, CanPlayThrough());
-  SimulateNetwork(1, 0, media_bitrate / 8 / 2, 1000, seconds_of_data * 2);
+  SimulateNetwork(1, kMediaByterate / 2, 1000, kMediaDuration * 2);
+}
+
+TEST_F(DownloadRateMonitorTest, DownloadEndOfVideo) {
+  Initialize();
+  // Seek to 10s before the end of the file, then download the remainder of the
+  // file at half the bitrate.
+  SeekTo((kMediaDuration - 10) * kMediaByterate);
+  EXPECT_CALL(*this, CanPlayThrough());
+  SimulateNetwork(1, kMediaByterate / 2, 1000, 20);
+
+  // CanPlayThrough should not be fired because of deferring.
+  CHECK(!DownloadIsDeferred());
 }
 

[acolwell GONE FROM CHROMIUM, 2012/01/09 23:16:57]

We should also have some test simulating the SimpleDataSource & FileDataSource
cases where only SetTotalBytes() & SetBufferedBytes() are called w/o a
SetNetworkActivity().

[vrk (LEFT CHROMIUM), 2012/01/11 00:23:13]

Both SimpleDataSource and FileDataSource are "local" (non-http/https) sources,
so I think the MediaSourceIsLocal() test should cover it. Let me know if you
were thinking of something else.

 }  // namespace media