Tie multibuffers to URLs

media/blink/resource_multibuffer_data_provider.cc

+// Copyright 2015 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 "media/blink/resource_multibuffer_data_provider.h"
+
+#include "base/bind.h"
+#include "base/bits.h"
+#include "base/callback_helpers.h"
+#include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
+#include "media/blink/active_loader.h"
+#include "media/blink/cache_util.h"
+#include "media/blink/media_blink_export.h"
+#include "media/blink/url_index.h"
+#include "net/http/http_byte_range.h"
+#include "net/http/http_request_headers.h"
+#include "third_party/WebKit/public/platform/WebURLError.h"
+#include "third_party/WebKit/public/platform/WebURLResponse.h"
+
+using blink::WebFrame;
+using blink::WebString;
+using blink::WebURLError;
+using blink::WebURLLoader;
+using blink::WebURLLoaderOptions;
+using blink::WebURLRequest;
+using blink::WebURLResponse;
+
+namespace media {
+
+// The number of milliseconds to wait before retrying a failed load.
+const int kLoaderFailedRetryDelayMs = 250;
+
+static const int kHttpOK = 200;
+static const int kHttpPartialContent = 206;
+static const int kMaxRetries = 3;

[xhwang, 2015/11/19 23:34:17]

"static" not needed, also be consistent with l.33

[hubbe, 2015/11/20 23:24:23]

Done.

+
+ResourceMultiBufferDataProvider::ResourceMultiBufferDataProvider(
+    UrlData* url_data,
+    MultiBufferBlockId pos)
+    : pos_(pos),
+      url_data_(url_data),
+      retries_(0),
+      cors_mode_(url_data->cors_mode()),
+      origin_(url_data->url().GetOrigin()),
+      weak_factory_(this) {
+  DCHECK(url_data_) << " pos = " << pos;
+  DCHECK_GE(pos, 0);
+}
+
+void ResourceMultiBufferDataProvider::Start() {

[xhwang, 2015/11/19 23:34:17]

definition order doesn't match declaration order

[hubbe, 2015/11/20 23:24:23]

Done.

+  // In the case of a re-start, throw away any half-finished blocks.
+  fifo_.clear();
+  // Prepare the request.
+  WebURLRequest request(url_data_->url());
+  // TODO(mkwst): Split this into video/audio.
+  request.setRequestContext(WebURLRequest::RequestContextVideo);
+
+  request.setHTTPHeaderField(
+      WebString::fromUTF8(net::HttpRequestHeaders::kRange),
+      WebString::fromUTF8(
+          net::HttpByteRange::RightUnbounded(byte_pos()).GetHeaderValue()));
+
+  url_data_->multibuffer()->frame()->setReferrerForRequest(request,
+                                                           blink::WebURL());
+
+  // Disable compression, compression for audio/video doesn't make sense...
+  request.setHTTPHeaderField(
+      WebString::fromUTF8(net::HttpRequestHeaders::kAcceptEncoding),
+      WebString::fromUTF8("identity;q=1, *;q=0"));
+
+  WebURLLoaderOptions options;
+  if (url_data_->cors_mode() == UrlData::kUnspecified) {
+    options.allowCredentials = true;
+    options.crossOriginRequestPolicy =
+        WebURLLoaderOptions::CrossOriginRequestPolicyAllow;
+  } else {
+    options.exposeAllResponseHeaders = true;
+    // The author header set is empty, no preflight should go ahead.
+    options.preflightPolicy = WebURLLoaderOptions::PreventPreflight;
+    options.crossOriginRequestPolicy =
+        WebURLLoaderOptions::CrossOriginRequestPolicyUseAccessControl;
+    if (url_data_->cors_mode() == UrlData::kUseCredentials)
+      options.allowCredentials = true;
+  }
+  scoped_ptr<WebURLLoader> loader(
+      url_data_->multibuffer()->frame()->createAssociatedURLLoader(options));

[xhwang, 2015/11/19 23:34:17]

It seems we plumb the |frame| all the way here through multiple layers only to
create the loader. ResourceMultiBufferDataProvider doesn't really need to know
WebFrame (which is a pretty big inteface) other than this.

Since we pretty much get everything from url_data_ to create the loader. Does it
make sense to ask url_data_ to create the loader for us? Or get some callback in
the constructor that can do so?

[hubbe, 2015/11/20 23:24:23]

To me, it makes sense to have "creating a loader" in the same module as the
blink::WebUrlLoaderClient implementation. I've moved the frame() function from
the multibuffer to the url_data_, which simplifies things a bit, but doesn't
address the issue of making a broad interface available.

+
+  // Start the resource loading.
+  loader->loadAsynchronously(request, this);
+  active_loader_.reset(new ActiveLoader(loader.Pass()));
+}
+
+ResourceMultiBufferDataProvider::~ResourceMultiBufferDataProvider() {}
+
+/////////////////////////////////////////////////////////////////////////////
+// MultiBufferDataProvider implementation.

[xhwang, 2015/11/19 23:34:17]

MultiBuffer::DataProvider

[hubbe, 2015/11/20 23:24:23]

Done.

+MultiBufferBlockId ResourceMultiBufferDataProvider::Tell() const {
+  return pos_;
+}
+
+bool ResourceMultiBufferDataProvider::Available() const {
+  if (fifo_.empty())
+    return false;
+  if (fifo_.back()->end_of_stream())
+    return true;
+  if (fifo_.front()->data_size() == block_size())
+    return true;
+  return false;
+}
+
+scoped_refptr<DataBuffer> ResourceMultiBufferDataProvider::Read() {
+  DCHECK(Available());
+  scoped_refptr<DataBuffer> ret = fifo_.front();
+  fifo_.pop_front();
+  ++pos_;
+  return ret;
+}
+
+void ResourceMultiBufferDataProvider::SetDeferred(bool deferred) {
+  if (active_loader_) {

[xhwang, 2015/11/19 23:34:17]

nit: We like to return early

if (!active_loader)
  return;

// handle normal cases

[hubbe, 2015/11/20 23:24:23]

I tend to prefer non-negative if-statements over returning early, but when in
rome...

+    if (active_loader_->deferred() != deferred) {
+      active_loader_->SetDeferred(deferred);
+    }
+  }
+}
+
+/////////////////////////////////////////////////////////////////////////////
+// WebURLLoaderClient implementation.
+
+void ResourceMultiBufferDataProvider::willFollowRedirect(
+    WebURLLoader* loader,
+    WebURLRequest& newRequest,
+    const WebURLResponse& redirectResponse) {
+  redirects_to_ = newRequest.url();
+  url_data_->set_valid_until(GetMemoryCacheValidUntil(redirectResponse));
+
+  // This test is vital for security!
+  if (cors_mode_ == UrlData::kUnspecified) {
+    if (origin_ != redirects_to_.GetOrigin()) {
+      url_data_->Fail();
+    }
+  }
+}
+
+void ResourceMultiBufferDataProvider::didSendData(
+    WebURLLoader* loader,
+    unsigned long long bytes_sent,
+    unsigned long long total_bytes_to_be_sent) {
+  NOTIMPLEMENTED();
+}
+
+void ResourceMultiBufferDataProvider::didReceiveResponse(
+    WebURLLoader* loader,
+    const WebURLResponse& response) {
+  DVLOG(1) << "didReceiveResponse: HTTP/"
+           << (response.httpVersion() == WebURLResponse::HTTPVersion_0_9
+                   ? "0.9"
+                   : response.httpVersion() == WebURLResponse::HTTPVersion_1_0
+                         ? "1.0"
+                         : response.httpVersion() ==
+                                   WebURLResponse::HTTPVersion_1_1
+                               ? "1.1"
+                               : response.httpVersion() ==
+                                         WebURLResponse::HTTPVersion_2_0
+                                     ? "2.0"
+                                     : "Unknown")

[xhwang, 2015/11/19 23:34:17]

Have a helper function to convert WebURLResponse::HTTPVersion_* to a string?

[hubbe, 2015/11/20 23:24:23]

Helper function seems overkill since it's just for debugging.
This was pretty until git cl format got it's grubby hands on it.
Better now?

[xhwang, 2015/11/23 23:09:21]

Acknowledged.

+           << " " << response.httpStatusCode();
+  DCHECK(active_loader_);
+
+  scoped_refptr<UrlData> destination_url_data(url_data_);
+
+  base::Time last_modified;
+  if (base::Time::FromString(
+          response.httpHeaderField("Last-Modified").utf8().data(),
+          &last_modified)) {
+    url_data_->set_last_modified(last_modified);
+  }
+
+  url_data_->set_valid_until(GetMemoryCacheValidUntil(response));
+
+  UrlIndex* url_index = url_data_->url_index();
+
+  if (!redirects_to_.is_empty()) {
+    if (!url_index) {
+      // We've been disconnected from the url index.
+      // That means the url_index_ has been destroyed, which means we do not
+      // need to do anything clever.
+      return;
+    }
+    destination_url_data = url_index->GetByUrl(redirects_to_, cors_mode_);
+    redirects_to_ = GURL();
+  }
+
+  uint32 reasons = GetReasonsForUncacheability(response);
+  url_data_->set_cacheable(reasons == 0);
+  UMA_HISTOGRAM_BOOLEAN("Media.CacheUseful", reasons == 0);
+  int shift = 0;
+  int max_enum = base::bits::Log2Ceiling(kMaxReason);
+  while (reasons) {
+    DCHECK_LT(shift, max_enum);  // Sanity check.
+    if (reasons & 0x1) {
+      UMA_HISTOGRAM_ENUMERATION("Media.UncacheableReason", shift,
+                                max_enum);  // PRESUBMIT_IGNORE_UMA_MAX
+    }
+
+    reasons >>= 1;
+    ++shift;
+  }
+
+  // Expected content length can be |kPositionNotSpecified|, in that case
+  // |content_length_| is not specified and this is a streaming response.
+  int64 content_length = response.expectedContentLength();
+
+  // We make a strong assumption that when we reach here we have either
+  // received a response from HTTP/HTTPS protocol or the request was
+  // successful (in particular range request). So we only verify the partial
+  // response for HTTP and HTTPS protocol.
+  if (url_data_->url().SchemeIsHTTPOrHTTPS()) {
+    bool partial_response = (response.httpStatusCode() == kHttpPartialContent);
+    bool ok_response = (response.httpStatusCode() == kHttpOK);
+
+    // Check to see whether the server supports byte ranges.
+    std::string accept_ranges =
+        response.httpHeaderField("Accept-Ranges").utf8();
+    if (accept_ranges.find("bytes") != std::string::npos)
+      url_data_->set_range_supported();

[xhwang, 2015/11/19 23:34:17]

We do similar operations in multiple places. Can we make a helper function to
so?

[hubbe, 2015/11/20 23:24:23]

A helper function that calls url_data_->set_range_supported()?
Or am I missing something?

[xhwang, 2015/11/23 23:09:21]

Sorry for not being clear. A helper function that reads out data for a field in
a |response|.

[hubbe, 2015/11/24 22:55:10]

Doesn't seem to be a lot of re-usable code here.
Just read out the data for a field is already a single line.
I looked at some of the functions in cache_util to see if there are some common
patterns, but the only thing that seems potentially worth it is to make a
function that reads out "Date" and "Expires" and returns a base::Time.

+
+    // If we have verified the partial response and it is correct, we will
+    // return kOk. It's also possible for a server to support range requests

[xhwang, 2015/11/19 23:34:17]

I don't see where "kOk" is returned.

[hubbe, 2015/11/20 23:24:23]

Removed reference to kOk.

+    // without advertising "Accept-Ranges: bytes".
+    if (partial_response && VerifyPartialResponse(response)) {
+      url_data_->set_range_supported();
+    } else if (ok_response && pos_ == 0) {
+      // We accept a 200 response for a Range:0- request, trusting the
+      // Accept-Ranges header, because Apache thinks that's a reasonable thing
+      // to return.
+      url_data_->set_length(content_length);
+    } else if (response.httpStatusCode() == 416) {

[xhwang, 2015/11/19 23:34:17]

We don't like hardcoded numbers, define a const similar to kHttpPartialContent?

Actually I am surprised to see net/ code also use numbers directly...

[hubbe, 2015/11/20 23:24:23]

Done.

+      // Really, we should never request a range that doesn't exist, but
+      // if we do, let's handle it in a sane way.
+      // Unsatisfiable range
+      fifo_.push_back(DataBuffer::CreateEOSBuffer());
+      url_data_->multibuffer()->DataProviderEvent(this);

[xhwang, 2015/11/19 23:34:17]

We are using the multibuffer a lot, I kinda feel we have a layering issue... But
I am not familiar with the code enough to give better suggestions.

[hubbe, 2015/11/20 23:24:23]

To me, using url_data_ seems more questionable than using the multibuffer..

+      return;
+    } else {
+      url_data_->Fail();
+      return;
+    }
+  } else {
+    url_data_->set_range_supported();
+    if (content_length != kPositionNotSpecified) {
+      url_data_->set_length(content_length + byte_pos());
+    }
+  }
+
+  if (url_index) {
+    destination_url_data = url_index->TryInsert(destination_url_data);
+  }
+
+  if (destination_url_data != url_data_) {
+    scoped_refptr<UrlData> old_url_data(url_data_);
+    destination_url_data->Use();
+
+    // Take ownership of ourselves.
+    scoped_ptr<DataProvider> self(
+        url_data_->multibuffer()->RemoveProvider(this));
+    url_data_ = destination_url_data.get();

[xhwang, 2015/11/19 23:34:17]

Can we remove the .get() part?

[hubbe, 2015/11/20 23:24:23]

No.

+    url_data_->multibuffer()->AddProvider(self.Pass());
+
+    // Transfer data, writers, readers, to new UrlData.
+    old_url_data->RedirectTo(destination_url_data);

[xhwang, 2015/11/19 23:34:17]

These operations are confusing. Can we wrap 263-273 into a function, something
like swap()?

[hubbe, 2015/11/20 23:24:23]

I decided to add a bunch of comments instead.
Let me know if you still prefer a helper function.

+  }
+}
+
+void ResourceMultiBufferDataProvider::didReceiveData(WebURLLoader* loader,
+                                                     const char* data,
+                                                     int data_length,
+                                                     int encoded_data_length) {
+  DVLOG(1) << "didReceiveData: " << data_length << " bytes";
+  DCHECK(!Available());
+  DCHECK(active_loader_);
+  DCHECK_GT(data_length, 0);
+
+  // When we receive data, we allow more retries.
+  retries_ = 0;
+
+  while (data_length) {
+    if (fifo_.empty() || fifo_.back()->data_size() == block_size()) {

[xhwang, 2015/11/19 23:34:17]

nit: fifo_.back()->data_size() appeared 4 times, worth a local variable? e.g.
block_data_size or existing_data_size?

[hubbe, 2015/11/20 23:24:23]

Done (but only for 3 out of the 4 cases.)

+      fifo_.push_back(new DataBuffer(block_size()));
+      fifo_.back()->set_data_size(0);
+    }
+    int to_append =
+        std::min<int>(data_length, block_size() - fifo_.back()->data_size());
+    DCHECK_GT(to_append, 0);
+    memcpy(fifo_.back()->writable_data() + fifo_.back()->data_size(), data,
+           to_append);
+    data += to_append;
+    fifo_.back()->set_data_size(fifo_.back()->data_size() + to_append);
+    data_length -= to_append;
+  }
+
+  if (Available())
+    url_data_->multibuffer()->DataProviderEvent(this);
+
+  // Beware, this object might be deleted here.
+}
+
+void ResourceMultiBufferDataProvider::didDownloadData(WebURLLoader* loader,
+                                                      int dataLength,
+                                                      int encoded_data_length) {
+  NOTIMPLEMENTED();
+}
+
+void ResourceMultiBufferDataProvider::didReceiveCachedMetadata(
+    WebURLLoader* loader,
+    const char* data,
+    int data_length) {
+  NOTIMPLEMENTED();
+}
+
+void ResourceMultiBufferDataProvider::didFinishLoading(
+    WebURLLoader* loader,
+    double finishTime,
+    int64_t total_encoded_data_length) {
+  DVLOG(1) << "didFinishLoading";
+  DCHECK(active_loader_.get());
+  DCHECK(!Available());
+
+  // We're done with the loader.
+  active_loader_.reset();
+
+  // If we didn't know the |instance_size_| we do now.
+  int64_t size = byte_pos();
+  if (!fifo_.empty())
+    size += fifo_.back()->data_size();
+
+  // This request reports something smaller than what we've seen in the past,
+  // Maybe it's transient error?
+  if (url_data_->length() != kPositionNotSpecified &&
+      size < url_data_->length()) {
+    if (retries_ < kMaxRetries) {
+      fifo_.clear();
+      retries_++;
+      base::MessageLoop::current()->PostDelayedTask(
+          FROM_HERE, base::Bind(&ResourceMultiBufferDataProvider::Start,
+                                weak_factory_.GetWeakPtr()),
+          base::TimeDelta::FromMilliseconds(kLoaderFailedRetryDelayMs));
+      return;
+    } else {
+      scoped_ptr<ActiveLoader> active_loader = active_loader_.Pass();
+      url_data_->Fail();
+      return;
+    }
+  }
+
+  url_data_->set_length(size);
+  fifo_.push_back(DataBuffer::CreateEOSBuffer());
+
+  DCHECK(Available());
+  url_data_->multibuffer()->DataProviderEvent(this);
+
+  // Beware, this object might be deleted here.
+}
+
+void ResourceMultiBufferDataProvider::didFail(WebURLLoader* loader,
+                                              const WebURLError& error) {
+  DVLOG(1) << "didFail: reason=" << error.reason
+           << ", isCancellation=" << error.isCancellation
+           << ", domain=" << error.domain.utf8().data()
+           << ", localizedDescription="
+           << error.localizedDescription.utf8().data();
+  DCHECK(active_loader_.get());
+
+  if (retries_ < kMaxRetries) {
+    retries_++;
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE, base::Bind(&ResourceMultiBufferDataProvider::Start,
+                              weak_factory_.GetWeakPtr()),
+        base::TimeDelta::FromMilliseconds(kLoaderFailedRetryDelayMs));
+  } else {
+    // We don't need to continue loading after failure.
+    //
+    // Keep it alive until we exit this method so that |error| remains valid.
+    scoped_ptr<ActiveLoader> active_loader = active_loader_.Pass();
+    url_data_->Fail();
+  }
+}
+
+bool ResourceMultiBufferDataProvider::ParseContentRange(
+    const std::string& content_range_str,
+    int64* first_byte_position,
+    int64* last_byte_position,
+    int64* instance_size) {
+  const std::string kUpThroughBytesUnit = "bytes ";
+  if (content_range_str.find(kUpThroughBytesUnit) != 0)
+    return false;
+  std::string range_spec =
+      content_range_str.substr(kUpThroughBytesUnit.length());
+  size_t dash_offset = range_spec.find("-");
+  size_t slash_offset = range_spec.find("/");
+
+  if (dash_offset == std::string::npos || slash_offset == std::string::npos ||
+      slash_offset < dash_offset || slash_offset + 1 == range_spec.length()) {
+    return false;
+  }
+  if (!base::StringToInt64(range_spec.substr(0, dash_offset),
+                           first_byte_position) ||
+      !base::StringToInt64(
+          range_spec.substr(dash_offset + 1, slash_offset - dash_offset - 1),
+          last_byte_position)) {
+    return false;
+  }
+  if (slash_offset == range_spec.length() - 2 &&
+      range_spec[slash_offset + 1] == '*') {
+    *instance_size = kPositionNotSpecified;
+  } else {
+    if (!base::StringToInt64(range_spec.substr(slash_offset + 1),
+                             instance_size)) {
+      return false;
+    }
+  }
+  if (*last_byte_position < *first_byte_position ||
+      (*instance_size != kPositionNotSpecified &&
+       *last_byte_position >= *instance_size)) {
+    return false;
+  }
+
+  return true;
+}
+
+int64_t ResourceMultiBufferDataProvider::byte_pos() const {
+  int64_t ret = pos_;
+  return ret << url_data_->multibuffer()->block_size_shift();
+}
+
+int64_t ResourceMultiBufferDataProvider::block_size() const {
+  int64_t ret = 1;
+  return ret << url_data_->multibuffer()->block_size_shift();
+}
+
+bool ResourceMultiBufferDataProvider::VerifyPartialResponse(
+    const WebURLResponse& response) {
+  int64 first_byte_position, last_byte_position, instance_size;
+  if (!ParseContentRange(response.httpHeaderField("Content-Range").utf8(),
+                         &first_byte_position, &last_byte_position,
+                         &instance_size)) {
+    return false;
+  }
+
+  if (url_data_->length() == kPositionNotSpecified) {
+    url_data_->set_length(instance_size);
+  }
+
+  if (byte_pos() != first_byte_position) {
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace media