Move DataSourceHost to BufferedDataSourceHost.

Move DataSourceHost to BufferedDataSourceHost.

BUG=122071
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262892

[sandersd (OOO until July 31), 2014-04-03 21:23:27]

Draft of the second part of this cleanup.

I'm confused why didLoadingProgress() is const, as its definition is clearly not
const, and why GetBufferedTimeRanges() returns a reference, but both are done
that way throughout the codebase. Are there any legitimate reasons for it or
should I clean it up?

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

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.cc:10: #include
"content/public/common/url_constants.h"
is this because you removed the kHttpScheme constants?

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.h:32: // Set the total size of the
media file.
we should tweak this similar to below ("Notify the host that the size of the
media file is known", etc...)

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.h:41: virtual
~BufferedDataSourceHost();
it's ok to inline this:
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:500: base::TimeDelta
TimeForByteOffset(
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

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:504: if (position < 0.01)
this looks changed from the code in pipeline.cc -- intentional?

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:518: if (total_bytes_ &&
buffered_byte_ranges_.size()) {
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

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:527: // TODO(sandersd): Why return
a reference at all?
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

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:547: // TODO(sandersd): Why is
this const!?
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...

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:939: TRACE_EVENT0("media",
"WebMediaPlayerImpl::SetTotalBytes");
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 :)

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:945: TRACE_EVENT0("media",
"WebMediaPlayerImpl::AddBufferedByteRange");
dittop

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.h:272: int64 total_bytes_;
nit: I'd stash all these variables together around line ~338 since they're only
used/valid when data_source_ != NULL (except for buffered_ -- you might as well
leave that where it was in the original code on line ~248)

https://codereview.chromium.org/224093011/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/224093011/diff/1/media/base/pipeline.h#newcod...
media/base/pipeline.h:174: int64 GetTotalBytes() const;
remove?

https://codereview.chromium.org/224093011/diff/1/media/base/pipeline.h#newcod...
media/base/pipeline.h:344: // Amount of available buffered data.  Set by
filters.
s/Set by filters/Reported by |demuxer_|./?

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

Addressed comments and updated existing tests.

Still need to write tests for WMPI.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.cc:10: #include
"content/public/common/url_constants.h"
On 2014/04/03 21:59:37, scherkus wrote:
> is this because you removed the kHttpScheme constants?

Yes, but I removed them because of a link error.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.h:32: // Set the total size of the
media file.
On 2014/04/03 21:59:37, scherkus wrote:
> we should tweak this similar to below ("Notify the host that the size of the
> media file is known", etc...)

Done.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.h:41: virtual
~BufferedDataSourceHost();
On 2014/04/03 21:59:37, scherkus wrote:
> it's ok to inline this:
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....

Done.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:500: base::TimeDelta
TimeForByteOffset(
On 2014/04/03 21:59:37, scherkus wrote:
> 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

Done.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:504: if (position < 0.01)
On 2014/04/03 21:59:37, scherkus wrote:
> this looks changed from the code in pipeline.cc -- intentional?

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

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:518: if (total_bytes_ &&
buffered_byte_ranges_.size()) {
On 2014/04/03 21:59:37, scherkus wrote:
> 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

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.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:527: // TODO(sandersd): Why return
a reference at all?
On 2014/04/03 21:59:37, scherkus wrote:
> 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

I've created a bug.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:547: // TODO(sandersd): Why is
this const!?
On 2014/04/03 21:59:37, scherkus wrote:
> 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...

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

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:939: TRACE_EVENT0("media",
"WebMediaPlayerImpl::SetTotalBytes");
On 2014/04/03 21:59:37, scherkus wrote:
> 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 :)

Done.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:945: TRACE_EVENT0("media",
"WebMediaPlayerImpl::AddBufferedByteRange");
On 2014/04/03 21:59:37, scherkus wrote:
> dittop

Done.

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/224093011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.h:272: int64 total_bytes_;
On 2014/04/03 21:59:37, scherkus wrote:
> nit: I'd stash all these variables together around line ~338 since they're
only
> used/valid when data_source_ != NULL (except for buffered_ -- you might as
well
> leave that where it was in the original code on line ~248)

Done.

https://codereview.chromium.org/224093011/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/224093011/diff/1/media/base/pipeline.h#newcod...
media/base/pipeline.h:174: int64 GetTotalBytes() const;
On 2014/04/03 21:59:37, scherkus wrote:
> remove?

Done.

https://codereview.chromium.org/224093011/diff/1/media/base/pipeline.h#newcod...
media/base/pipeline.h:344: // Amount of available buffered data.  Set by
filters.
On 2014/04/03 21:59:37, scherkus wrote:
> s/Set by filters/Reported by |demuxer_|./?

Done.

[scherkus (not reviewing), 2014-04-05 00:00:02]

nits

re: WMPI test ... you might be able to get away with creating a WMPI and calling
the public BDSH methods and verifying what buffered() reports

if you spend more than, like, 20 minutes let me know because I fear this could
easily turn into a unproductive massive yak shave

https://codereview.chromium.org/224093011/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/224093011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:542: // TODO(sandersd): Why is
this const!?
remove this TODO as you now have it in the .h file w/ a bug

https://codereview.chromium.org/224093011/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/224093011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:119: // FIXME: Change this to
non-const in blink::WebMediaPlayer.
sillyness: blink uses FIXME (leftover from WebKit days), Chromium uses
TODO(my_ldap)

https://codereview.chromium.org/224093011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:255: // FIXME: Remove this cache.
http://crbug.com/360254
ditto

[scherkus (not reviewing), 2014-04-07 22:03:24]

https://codereview.chromium.org/224093011/diff/60001/content/renderer/media/b...
File content/renderer/media/buffered_data_source_host_impl.cc (right):

https://codereview.chromium.org/224093011/diff/60001/content/renderer/media/b...
content/renderer/media/buffered_data_source_host_impl.cc:9:
BufferedDataSourceHostImpl(media::Pipeline pipeline) : pipeline_(pipeline);
we should try to minimize the # of dependencies this code has by removing
Pipeline from the equation as well as blink::WebTimeRanges

we can change BufferedTimeRanges() into something like:

// WMPI passes in pipeline_.GetBufferedTimeRanges() and
pipeline_.GetMediaDuration()
void BFSHI::CalculateBufferedTimeRanges(
    media::Ranges<base::TimeDelta>* time_ranges,
    base::TimeDelta duration) {
  // do stuff
}

// WMPI does the conversion to blink::WebTimeRanges

This way:
  1) BDSHI doesn't have to depend on a much larger/complex object like Pipeline
  2) We keep chromium-to-blink type conversions closer to where they need to be
done
  3) Unit tests will be waaayyy easier to write


Ditto for dLP() -- WMPI should do the merging as its aware of both BDSHI and
Pipeline

[sandersd (OOO until July 31), 2014-04-07 23:46:42]

https://codereview.chromium.org/224093011/diff/60001/content/renderer/media/b...
File content/renderer/media/buffered_data_source_host_impl.cc (right):

https://codereview.chromium.org/224093011/diff/60001/content/renderer/media/b...
content/renderer/media/buffered_data_source_host_impl.cc:9:
BufferedDataSourceHostImpl(media::Pipeline pipeline) : pipeline_(pipeline);
On 2014/04/07 22:03:25, scherkus wrote:
> we should try to minimize the # of dependencies this code has by removing
> Pipeline from the equation as well as blink::WebTimeRanges
> 
> we can change BufferedTimeRanges() into something like:
> 
> // WMPI passes in pipeline_.GetBufferedTimeRanges() and
> pipeline_.GetMediaDuration()
> void BFSHI::CalculateBufferedTimeRanges(
>     media::Ranges<base::TimeDelta>* time_ranges,
>     base::TimeDelta duration) {
>   // do stuff
> }
> 
> // WMPI does the conversion to blink::WebTimeRanges
> 
> This way:
>   1) BDSHI doesn't have to depend on a much larger/complex object like
Pipeline
>   2) We keep chromium-to-blink type conversions closer to where they need to
be
> done
>   3) Unit tests will be waaayyy easier to write
> 
> 
> Ditto for dLP() -- WMPI should do the merging as its aware of both BDSHI and
> Pipeline

Done.

[scherkus (not reviewing), 2014-04-08 04:36:17]

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl.cc (right):

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
ditto

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl.h (right):

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
heads up -- it's 2014!

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.h:15: // Implementation of
BufferedDataSourceHost for use by WebMediaPlayerImpl.
nit: a better comment would answer why / the main purpose of this class, which
is namely to convert buffered byte information into a buffered time range
approximation

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.h:19:
BufferedDataSourceHostImpl() {};
these should go to the .cc since we don't have a pure POD class (media::Ranges
is more or less a wrapper around a std::vector)

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

... also you should initialize all your member variables

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl_unittest.cc (right):

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl_unittest.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
ditto

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl_unittest.cc:24: 
should include a test post-construction to test the empty case and when total
bytes haven't been set yet

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl_unittest.cc:30:
EXPECT_EQ(ranges_.size(), 1u);
the param order for EXPECT_EQ is (expected, actual) -- when/if it fails it
prints out better, so flip all of these around in the test

[sandersd (OOO until July 31), 2014-04-08 22:42:13]

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl.cc (right):

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2014/04/08 04:36:18, scherkus wrote:
> ditto

Done.

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl.h (right):

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/04/08 04:36:18, scherkus wrote:
> heads up -- it's 2014!

Oops, clearly I was on auto-pilot.

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.h:19:
BufferedDataSourceHostImpl() {};
On 2014/04/08 04:36:18, scherkus wrote:
> these should go to the .cc since we don't have a pure POD class (media::Ranges
> is more or less a wrapper around a std::vector)
> 
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....
> 
> ... also you should initialize all your member variables

Sorry, I'll get the hang of this eventually. You'll probably also need to
carefully check my visibility and virtual choices for a while yet :-(.

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl_unittest.cc (right):

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl_unittest.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/04/08 04:36:18, scherkus wrote:
> ditto

Done.

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl_unittest.cc:24: 
On 2014/04/08 04:36:18, scherkus wrote:
> should include a test post-construction to test the empty case and when total
> bytes haven't been set yet

Done.

https://codereview.chromium.org/224093011/diff/120001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl_unittest.cc:30:
EXPECT_EQ(ranges_.size(), 1u);
On 2014/04/08 04:36:18, scherkus wrote:
> the param order for EXPECT_EQ is (expected, actual) -- when/if it fails it
> prints out better, so flip all of these around in the test

Done.

[scherkus (not reviewing), 2014-04-08 22:45:21]

lgtm w/ nits

https://codereview.chromium.org/224093011/diff/130001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl.cc (right):

https://codereview.chromium.org/224093011/diff/130001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.cc:11:
did_loading_progress_(0) {
s/0/false/ seeing as it's a bool

https://codereview.chromium.org/224093011/diff/130001/content/renderer/media/...
File content/renderer/media/buffered_data_source_host_impl.h (right):

https://codereview.chromium.org/224093011/diff/130001/content/renderer/media/...
content/renderer/media/buffered_data_source_host_impl.h:21:
~BufferedDataSourceHostImpl() {};
move this to .cc as well, otherwise you're telling the compiler it should inline
destructor code (which in this case includes auto-generated STL stuff which can
be hefty)

[sandersd (OOO until July 31), 2014-04-09 18:02:21]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-09 18:02:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/224093011/170001

[commit-bot: I haz the power, 2014-04-09 19:13:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 19:13:19]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[sandersd (OOO until July 31), 2014-04-09 20:15:40]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-09 20:15:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/224093011/190001

[commit-bot: I haz the power, 2014-04-09 22:33:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 22:33:50]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[sandersd (OOO until July 31), 2014-04-09 22:39:53]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-04-09 22:40:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/224093011/210001

[commit-bot: I haz the power, 2014-04-10 01:45:48]

Change committed as 262892