Skip to content

Commit

Permalink
Improve retry support for media network loading.
Browse files Browse the repository at this point in the history
Retry 30 times in 30 seconds instead of 3 times in 750ms.
The retries are spaced so that the first retry is 250ms, second is 300ms, then 350ms, etc.

BUG=592703

Review URL: https://codereview.chromium.org/1777153002

Cr-Commit-Position: refs/heads/master@{#380723}
(cherry picked from commit 018c484)

Review URL: https://codereview.chromium.org/1796973004 .

Cr-Commit-Position: refs/branch-heads/2661@{crosswalk-project#227}
Cr-Branched-From: ef6f6ae-refs/heads/master@{#378081}
  • Loading branch information
FredrikHubinette committed Mar 14, 2016
1 parent d69a77f commit dc4b45b
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 66 deletions.
15 changes: 10 additions & 5 deletions media/blink/buffered_data_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,16 @@ namespace {
// of FFmpeg.
const int kInitialReadBufferSize = 32768;

// Number of cache misses or read failures we allow for a single Read() before
// signaling an error.
const int kLoaderRetries = 3;

// The number of milliseconds to wait before retrying a failed load.
const int kLoaderFailedRetryDelayMs = 250;

// Each retry, add this many MS to the delay.
// total delay is:
// (kLoaderPartialRetryDelayMs +
// kAdditionalDelayPerRetryMs * (kMaxRetries - 1) / 2) * kLoaderRetries
// = 29250 ms
const int kAdditionalDelayPerRetryMs = 50;

} // namespace

namespace media {
Expand Down Expand Up @@ -491,7 +494,9 @@ void BufferedDataSource::ReadCallback(
FROM_HERE, base::Bind(&BufferedDataSource::ReadCallback,
weak_factory_.GetWeakPtr(),
BufferedResourceLoader::kCacheMiss, 0),
base::TimeDelta::FromMilliseconds(kLoaderFailedRetryDelayMs));
base::TimeDelta::FromMilliseconds(kLoaderFailedRetryDelayMs +
read_op_->retries() *
kAdditionalDelayPerRetryMs));
return;
}

Expand Down
3 changes: 3 additions & 0 deletions media/blink/buffered_data_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ class BufferedDataSourceInterface : public DataSource {
class MEDIA_BLINK_EXPORT BufferedDataSource
: NON_EXPORTED_BASE(public BufferedDataSourceInterface) {
public:
// Number of cache misses or read failures we allow for a single Read() before
// signaling an error.
enum { kLoaderRetries = 30 };
typedef base::Callback<void(bool)> DownloadingCB;

// |url| and |cors_mode| are passed to the object. Buffered byte range changes
Expand Down
34 changes: 10 additions & 24 deletions media/blink/buffered_data_source_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -633,18 +633,11 @@ TEST_F(BufferedDataSourceTest, Http_TooManyRetries) {
// Make sure there's a pending read -- we'll expect it to error.
ReadAt(0);

// It'll try three times.
ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->Generate206(0));

ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->Generate206(0));

ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->Generate206(0));
for (int i = 0; i < BufferedDataSource::kLoaderRetries; i++) {
ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->Generate206(0));
}

// It'll error after this.
EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
Expand All @@ -660,18 +653,11 @@ TEST_F(BufferedDataSourceTest, File_TooManyRetries) {
// Make sure there's a pending read -- we'll expect it to error.
ReadAt(0);

// It'll try three times.
ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->GenerateFileResponse(0));

ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->GenerateFileResponse(0));

ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->GenerateFileResponse(0));
for (int i = 0; i < BufferedDataSource::kLoaderRetries; i++) {
ExpectCreateResourceLoader();
FinishLoading();
Respond(response_generator_->GenerateFileResponse(0));
}

// It'll error after this.
EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
Expand Down
62 changes: 30 additions & 32 deletions media/blink/multibuffer_data_source_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,12 @@ class MultibufferDataSourceTest : public testing::Test {
message_loop_.RunUntilIdle();
}

void FailLoading() {
data_provider()->didFail(url_loader(),
response_generator_->GenerateError());
message_loop_.RunUntilIdle();
}

void Restart() {
EXPECT_TRUE(data_provider());
EXPECT_FALSE(active_loader_allownull());
Expand Down Expand Up @@ -793,23 +799,19 @@ TEST_F(MultibufferDataSourceTest, Http_TooManyRetries) {
// Make sure there's a pending read -- we'll expect it to error.
ReadAt(kDataSize);

// It'll try three times.
FinishLoading();
Restart();
Respond(response_generator_->Generate206(kDataSize));

FinishLoading();
Restart();
Respond(response_generator_->Generate206(kDataSize));

FinishLoading();
Restart();
Respond(response_generator_->Generate206(kDataSize));

// It'll error after this.
EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
FinishLoading();
for (int i = 0; i < ResourceMultiBufferDataProvider::kMaxRetries; i++) {
FailLoading();
data_provider()->Start();
Respond(response_generator_->Generate206(kDataSize));
}

// Stop() will also cause the readback to be called with kReadError, but
// we want to make sure it was called during FailLoading().
bool failed_ = false;
EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError))
.WillOnce(Assign(&failed_, true));
FailLoading();
EXPECT_TRUE(failed_);
EXPECT_FALSE(loading());
Stop();
}
Expand All @@ -820,23 +822,19 @@ TEST_F(MultibufferDataSourceTest, File_TooManyRetries) {
// Make sure there's a pending read -- we'll expect it to error.
ReadAt(kDataSize);

// It'll try three times.
FinishLoading();
Restart();
Respond(response_generator_->GenerateFileResponse(0));

FinishLoading();
Restart();
Respond(response_generator_->GenerateFileResponse(0));

FinishLoading();
Restart();
Respond(response_generator_->GenerateFileResponse(0));

// It'll error after this.
EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
FinishLoading();
for (int i = 0; i < ResourceMultiBufferDataProvider::kMaxRetries; i++) {
FailLoading();
data_provider()->Start();
Respond(response_generator_->Generate206(kDataSize));
}

// Stop() will also cause the readback to be called with kReadError, but
// we want to make sure it was called during FailLoading().
bool failed_ = false;
EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError))
.WillOnce(Assign(&failed_, true));
FailLoading();
EXPECT_TRUE(failed_);
EXPECT_FALSE(loading());
Stop();
}
Expand Down
14 changes: 9 additions & 5 deletions media/blink/resource_multibuffer_data_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,19 @@ namespace media {
// The number of milliseconds to wait before retrying a failed load.
const int kLoaderFailedRetryDelayMs = 250;

// Each retry, add this many MS to the delay.
// total delay is:
// (kLoaderPartialRetryDelayMs +
// kAdditionalDelayPerRetryMs * (kMaxRetries - 1) / 2) * kMaxretries = 29250 ms
const int kAdditionalDelayPerRetryMs = 50;

// The number of milliseconds to wait before retrying when the server
// decides to not give us all the data at once.
const int kLoaderPartialRetryDelayMs = 25;

const int kHttpOK = 200;
const int kHttpPartialContent = 206;
const int kHttpRangeNotSatisfiable = 416;
const int kMaxRetries = 3;

ResourceMultiBufferDataProvider::ResourceMultiBufferDataProvider(
UrlData* url_data,
Expand Down Expand Up @@ -411,12 +416,11 @@ void ResourceMultiBufferDataProvider::didFail(WebURLLoader* loader,
base::MessageLoop::current()->PostDelayedTask(
FROM_HERE, base::Bind(&ResourceMultiBufferDataProvider::Start,
weak_factory_.GetWeakPtr()),
base::TimeDelta::FromMilliseconds(kLoaderFailedRetryDelayMs));
base::TimeDelta::FromMilliseconds(
kLoaderFailedRetryDelayMs + kAdditionalDelayPerRetryMs * retries_));
} 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 = std::move(active_loader_);
// Note that calling Fail() will most likely delete this object.
url_data_->Fail();
}
}
Expand Down
3 changes: 3 additions & 0 deletions media/blink/resource_multibuffer_data_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ class MEDIA_BLINK_EXPORT ResourceMultiBufferDataProvider
: NON_EXPORTED_BASE(public MultiBuffer::DataProvider),
NON_EXPORTED_BASE(public blink::WebURLLoaderClient) {
public:
// NUmber of times we'll retry if the connection fails.
enum { kMaxRetries = 30 };

ResourceMultiBufferDataProvider(UrlData* url_data, MultiBufferBlockId pos);
~ResourceMultiBufferDataProvider() override;

Expand Down

0 comments on commit dc4b45b

Please sign in to comment.