Move DataSourceHost to BufferedDataSourceHost.

content/renderer/media/webmediaplayer_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/webmediaplayer_impl.h"
 
 #include <algorithm>
 #include <limits>
 #include <string>
 #include <vector>
@@ skipping 134 matching lines @@
     base::WeakPtr<WebMediaPlayerDelegate> delegate,
     const WebMediaPlayerParams& params)
     : frame_(frame),
       network_state_(WebMediaPlayer::NetworkStateEmpty),
       ready_state_(WebMediaPlayer::ReadyStateHaveNothing),
       main_loop_(base::MessageLoopProxy::current()),
       media_loop_(
           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),
       pending_seek_(false),
       pending_seek_seconds_(0.0f),
       client_(client),
       delegate_(delegate),
       defer_load_cb_(params.defer_load_cb()),
       accelerated_compositing_reported_(false),
       incremented_externally_allocated_memory_(false),
@@ skipping 136 matching lines @@
   media_log_->AddEvent(media_log_->CreateLoadEvent(url.spec()));
 
   // Media source pipelines can start immediately.
   if (load_type == LoadTypeMediaSource) {
     supports_save_ = false;
     StartPipeline();
     return;
   }
 
   // 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),
       base::Bind(
           &WebMediaPlayerImpl::DataSourceInitialized,
           AsWeakPtr(), gurl));
 
   is_local_source_ = !gurl.SchemeIsHTTPOrHTTPS();
 }
 
@@ skipping 160 matching lines @@
 WebMediaPlayer::NetworkState WebMediaPlayerImpl::networkState() const {
   DCHECK(main_loop_->BelongsToCurrentThread());
   return network_state_;
 }
 
 WebMediaPlayer::ReadyState WebMediaPlayerImpl::readyState() const {
   DCHECK(main_loop_->BelongsToCurrentThread());
   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_;
 }
 
 double WebMediaPlayerImpl::maxTimeSeekable() const {
   DCHECK(main_loop_->BelongsToCurrentThread());
 
   // If we haven't even gotten to ReadyStateHaveMetadata yet then just
   // return 0 so that the seekable range is empty.
   if (ready_state_ < WebMediaPlayer::ReadyStateHaveMetadata)
     return 0.0;
 
   // We don't support seeking in streaming media.
   if (data_source_ && data_source_->IsStreaming())
     return 0.0;
   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,
                                const WebRect& rect,
                                unsigned char alpha) {
   DCHECK(main_loop_->BelongsToCurrentThread());
 
   if (!accelerated_compositing_reported_) {
     accelerated_compositing_reported_ = true;
     // Normally paint() is only called in non-accelerated rendering, but there
@@ skipping 364 matching lines @@
   // TODO(xhwang): Support setMediaKeys(0) if necessary: http://crbug.com/330324
   if (!cdm)
     return;
 
   web_cdm_ = ToWebContentDecryptionModuleImpl(cdm);
 
   if (web_cdm_ && !decryptor_ready_cb_.is_null())
     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");
 
   client_->repaint();
 }
 
 void WebMediaPlayerImpl::OnPipelineSeek(PipelineStatus status) {
   DCHECK(main_loop_->BelongsToCurrentThread());
   starting_ = false;
@@ skipping 402 matching lines @@
 
   if (web_cdm_) {
     decryptor_ready_cb.Run(web_cdm_->GetDecryptor());
     return;
   }
 
   decryptor_ready_cb_ = decryptor_ready_cb;
 }
 
 }  // namespace content