Move DataSourceHost to BufferedDataSourceHost.

content/renderer/media/webmediaplayer_impl.cc

Index: content/renderer/media/webmediaplayer_impl.cc
diff --git a/content/renderer/media/webmediaplayer_impl.cc b/content/renderer/media/webmediaplayer_impl.cc
index 813e103f726742fb5dccb694c92ca6150fa77447..47cbfffe96d57ff8584ce91cdb2bed038f88fd83 100644
--- a/content/renderer/media/webmediaplayer_impl.cc
+++ b/content/renderer/media/webmediaplayer_impl.cc
@@ -152,6 +152,8 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
           RenderThreadImpl::current()->GetMediaThreadMessageLoopProxy()),
       media_log_(new RenderMediaLog()),
       pipeline_(media_loop_, media_log_.get()),
+      total_bytes_(0),
+      did_loading_progress_(false),
       paused_(true),
       seeking_(false),
       playback_rate_(0.0f),
@@ -308,13 +310,11 @@ void WebMediaPlayerImpl::DoLoad(LoadType load_type,
   }
 
   // Otherwise it's a regular request which requires resolving the URL first.
-  // TODO(sandersd): Make WMPI a DataSourceHost and pass |this| instead of
-  // |&pipeline_|.
   data_source_.reset(new BufferedDataSource(
       main_loop_,
       frame_,
       media_log_.get(),
-      &pipeline_,
+      this,
       base::Bind(&WebMediaPlayerImpl::NotifyDownloading, AsWeakPtr())));
   data_source_->Initialize(
       url, static_cast<BufferedResourceLoader::CORSMode>(cors_mode),
@@ -495,11 +495,38 @@ WebMediaPlayer::ReadyState WebMediaPlayerImpl::readyState() const {
   return ready_state_;
 }
 
+namespace {
+
+base::TimeDelta TimeForByteOffset(

[scherkus (not reviewing), 2014/04/03 21:59:37]

re: anon namespace vs. static ... try to go w/ consistency in the file

(I usually prefer static since it makes for better looking stack traces and
easier to set breakpoints when using gdb, but it's splitting hairs)

it looks like we've got plenty of other static functions in this file, so let's
go with static

[sandersd (OOO until July 31), 2014/04/04 23:48:41]

Done.

+    int64 byte_offset, int64 total_bytes, base::TimeDelta duration) {
+  double position = static_cast<double>(byte_offset) / total_bytes;
+  // Snap to the beginning/end where the approximation can look especially bad.
+  if (position < 0.01)

[scherkus (not reviewing), 2014/04/03 21:59:37]

this looks changed from the code in pipeline.cc -- intentional?

[sandersd (OOO until July 31), 2014/04/04 23:48:41]

Computing and using an epsilon seemed needlessly complicated to me; the behavior
should be identical but this I think it is easier to read.

+    return base::TimeDelta();
+  if (position > 0.99)
+    return duration;
+  return base::TimeDelta::FromMilliseconds(
+      static_cast<int64>(position * duration.InMilliseconds()));
+}
+
+}
+
 const blink::WebTimeRanges& WebMediaPlayerImpl::buffered() {
   DCHECK(main_loop_->BelongsToCurrentThread());
-  blink::WebTimeRanges web_ranges(
-      ConvertToWebTimeRanges(pipeline_.GetBufferedTimeRanges()));
-  buffered_.swap(web_ranges);
+  media::Ranges<base::TimeDelta> buffered_time_ranges =
+      pipeline_.GetBufferedTimeRanges();
+  if (total_bytes_ && buffered_byte_ranges_.size()) {

[scherkus (not reviewing), 2014/04/03 21:59:37]

hmm... I wonder whether we can tighten up the BDSHost API to guarantee to never
report a range unless a total bytes has been set first

that way we could change this into a check for buffered_byte_ranges_.size() + a
DCHECK(total_bytes_) inside the if statement

[sandersd (OOO until July 31), 2014/04/04 23:48:41]

It's possible to not know the total size of the source until you find the end.
We could specify that in that case no ranges are reported at all; I suspect the
user experience is bad in either case.

+    base::TimeDelta duration = pipeline_.GetMediaDuration();
+    for (size_t i = 0; i < buffered_byte_ranges_.size(); ++i) {
+      int64 start = buffered_byte_ranges_.start(i);
+      int64 end = buffered_byte_ranges_.end(i);
+      buffered_time_ranges.Add(TimeForByteOffset(start, total_bytes_, duration),
+                               TimeForByteOffset(end, total_bytes_, duration));
+    }
+  }
+  // TODO(sandersd): Why return a reference at all?

[scherkus (not reviewing), 2014/04/03 21:59:37]

see my explanation below -- it's probably because it somehow ended up that way
initially and no one has signed up for doing an N-sided CL  :\

I doubt our ranges are so large that we're taking a perf hit by the copy ... so
it'd be nice to verify that and clean this up

[sandersd (OOO until July 31), 2014/04/04 23:48:41]

I've created a bug.

+  blink::WebTimeRanges buffered = ConvertToWebTimeRanges(buffered_time_ranges);
+  buffered_.swap(buffered);
   return buffered_;
 }
 
@@ -517,10 +544,12 @@ double WebMediaPlayerImpl::maxTimeSeekable() const {
   return duration();
 }
 
+// TODO(sandersd): Why is this const!?

[scherkus (not reviewing), 2014/04/03 21:59:37]

likely because const spreads and we used to share a lot of the same interfaces
w/ other WebKit ports:

HTMLMediaElement has const methods [1], which spreads to MediaPlayer [2], which
spreads to WebMediaPlayer [3], and finally WMPI and Pipeline, etc...

funny enough, the calling code in question *isn't* const [4], so yes we could
likely change it in different CLs (note: it's an N-sided change between
repositories since we can't break any builds due to unimplemented interface
methods, which is probably the real reason why we haven't made the change yet)

[1]
http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLMediaElement.h
[2]
http://src.chromium.org/viewvc/blink/trunk/Source/platform/graphics/media/Med...
[3] http://src.chromium.org/viewvc/blink/trunk/public/platform/WebMediaPlayer.h
[4]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[sandersd (OOO until July 31), 2014/04/04 23:48:41]

I created a bug and added a comment in WMPI.h.

 bool WebMediaPlayerImpl::didLoadingProgress() const {
   DCHECK(main_loop_->BelongsToCurrentThread());
-
-  return pipeline_.DidLoadingProgress();
+  bool merged = pipeline_.DidLoadingProgress() || did_loading_progress_;
+  did_loading_progress_ = false;
+  return merged;
 }
 
 void WebMediaPlayerImpl::paint(WebCanvas* canvas,
@@ -905,6 +934,19 @@ void WebMediaPlayerImpl::setContentDecryptionModule(
     base::ResetAndReturn(&decryptor_ready_cb_).Run(web_cdm_->GetDecryptor());
 }
 
+void WebMediaPlayerImpl::SetTotalBytes(int64 total_bytes) {
+  DCHECK(main_loop_->BelongsToCurrentThread());
+  TRACE_EVENT0("media", "WebMediaPlayerImpl::SetTotalBytes");

[scherkus (not reviewing), 2014/04/03 21:59:37]

no need to have trace macros here -- I don't see us being interested in seeing
these in a chrome://tracing trace nor knowing how long it takes to, say, assign
a boolean value :)

[sandersd (OOO until July 31), 2014/04/04 23:48:41]

Done.

+  total_bytes_ = total_bytes;
+}
+
+void WebMediaPlayerImpl::AddBufferedByteRange(int64 start, int64 end) {
+  DCHECK(main_loop_->BelongsToCurrentThread());
+  TRACE_EVENT0("media", "WebMediaPlayerImpl::AddBufferedByteRange");

[scherkus (not reviewing), 2014/04/03 21:59:37]

dittop

[sandersd (OOO until July 31), 2014/04/04 23:48:41]

Done.

+  buffered_byte_ranges_.Add(start, end);
+  did_loading_progress_ = true;
+}
+
 void WebMediaPlayerImpl::InvalidateOnMainThread() {
   DCHECK(main_loop_->BelongsToCurrentThread());
   TRACE_EVENT0("media", "WebMediaPlayerImpl::InvalidateOnMainThread");