Implementing preload=metadata for video

Implementing preload=metadata for video

This patch implements the logic necessary to respect the preload attribute
when it is set to MetaData.  This also refactors the BufferedResourceLoader
to determine its buffering techniques based on a DeferStrategy value.

BUG=16482, 76555
TEST=media/video-preload.html, test_shell_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80465

[vrk (LEFT CHROMIUM), 2011-03-07 19:10:41]

So this still needs finishing up in terms of polish and robustness, but I wanted
to upload what I had so far in case there were fundamental objections to the way
I was implementing this.

A few things to note:

* DeferStrategy! The defer logic was getting to be an unreadable, unmaintainable
mess, and I found myself doing a lot of:

  if (Strategy == blah) {
    if (other ugly conditions)
      ...
  } else if (Strategy == blah2)
   ...

So I encapsulated the strategies in objects and the chooser of strategies in a
ChooseDeferStrategy method. I think it makes the code cleaner and easier to read
(it was certainly easier to write) but if there are objections/better
suggestions I can rewrite it.

* If we're OK with DeferStrategy, then I need to write unit tests and might make
ChooseDeferStrategy an interface -- BufferedResourceLoader doesn't need access
to the whole DataSource API

* This patch can't be committed as-is. I need to add the piping on WebKit to
pass the preload value down to the WebMediaPlayerImpl. (I have done this on my
local machine; the changes are very minor.)

[vrk (LEFT CHROMIUM), 2011-03-07 21:45:20]

Will need to merge with acolwell's DataSource refactor after it lands!
(http://codereview.chromium.org/6480050/)

Luckily it doesn't look like these CLs conflict very much at all. At any rate,
acolwell just ignore this CL as you work on your stuff; I will adjust to
whatever you commit!

[acolwell GONE FROM CHROMIUM, 2011-03-08 22:25:51]

You'll probably want to wait until my changes
(http://codereview.chromium.org/6480050/) and Ami's changes
(http://codereview.chromium.org/6648004/) land. Pipeline will no longer know
about DataSource or be accessible via pipeline_filter_. 

You'll probably have to access the DataSources via WebMediaPlayerImpl::Proxy.

On 2011/03/07 21:45:20, Victoria Kirst wrote:
> Will need to merge with acolwell's DataSource refactor after it lands!
> (http://codereview.chromium.org/6480050/)
> 
> Luckily it doesn't look like these CLs conflict very much at all. At any rate,
> acolwell just ignore this CL as you work on your stuff; I will adjust to
> whatever you commit!

[vrk (LEFT CHROMIUM), 2011-03-08 22:42:13]

Cool, sounds good!

[vrk (LEFT CHROMIUM), 2011-03-24 01:39:31]

Updated this patch to be compatible with the changes made in Pipeline.

Adding preload=metadata also requires a WebKit change; here is that for
reference if you need it:
https://bugs.webkit.org/show_bug.cgi?id=56983

I'll need to commit the WebKit patch before this one can land!

[vrk (LEFT CHROMIUM), 2011-03-25 02:00:43]

Hey all FYI, I updated this patch a little bit to clean up the code, add a few
comments and fix a few little bugs.

Also if you've been following the thread for issue 16482, turns out preload=none
is actually easy to implement (and totally WebKit-side change). I added
preload=none support to the WebKit patch, so that should be in soon too!

[acolwell GONE FROM CHROMIUM, 2011-03-25 04:35:28]

Initial set of comments.

I think the defer_strategy stuff should be reworked. All of the friending and
accessing of private data doesn't feel right to me. Perhaps if you just made
some helper functions in BufferedResourceLoader that wrap the various
Enable/Disable conditions you wouldn't need those classes. I'd be happy to talk
through it with you tomorrow.

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.cc
File media/base/filters.cc (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.cc#newcode73
media/base/filters.cc:73: void Demuxer::SetPreload(Preload preload) {}
No longer needed once this is declared pure virtual.

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.cc#newcode75
media/base/filters.cc:75: void DataSource::SetPreload(Preload preload) {}
No longer needed once this is declared pure virtual.

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.cc#newcode87
media/base/filters.cc:87: 
Remove

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h#newcode46
media/base/filters.h:46: // Used to specify video preload states.
You should add a note here that this enum must match the WebKit enum especially
if you use static_cast instead of a mapping function.

A reference to the HTTP spec or a description of the meanings of each value
might be good.

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h#newcode139
media/base/filters.h:139: virtual void SetPreload(Preload preload);
Make this pure virtual like the other methods.

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h#newcode195
media/base/filters.h:195: virtual void SetPreload(Preload preload);
Make this pure virtual like the other method so implementers are forced to think
about this.

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline.h#newcode15
media/base/pipeline.h:15: #include "media/base/filters.h"
It is unfortunate that we have to expose filters.h here. I'm tempted to suggest
putting the enum declaration in pipeline_status.h . Perhaps we should discuss
this with Ami & Andrew.

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1083: pipeline_init_state_->demuxer_ = demuxer;
Remove demuxer_ from pipeline_init_state since PipelineImpl now has requires a
reference. This just makes things consistent with audio_renderer_ &
video_renderer_

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:117: virtual Preload GetPreload() const;
Move to private section since this isn't part of the Pipeline interface.

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:314: void FFmpegDemuxer::SetPlaybackRate(float
playback_rate) {
This is necessary because we accidentally broke the existing functionality when
DataSources became owned by the demuxer instead of the CompositeFilter? Just
checking.

If so could you add a unit test to make sure we capture this behavior.

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:319: void
FFmpegDemuxer::SetPreload(media::Preload preload) {
shouldn't need media::

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:140: virtual void SetPreload(media::Preload
preload);
shouldn't need media::

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:189: DisableDeferIfNeeded();
Why is this call needed now?

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:208: DeferStrategy* strategy =
data_source->ChooseDeferStrategy();
Why don't you do the ChooseDeferStrategy() in the call to this function? That
way this code doesn't have to know about BufferedDataSource.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:212: DisableDeferIfNeeded();
The EnableDeferIfNeeded() followed by DisableDeferIfNeeded() is confusing. Is it
true that one of them will do something and the other is a noop? Will the defer
status get toggled on and then off?

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.h:91: virtual void
UpdateDeferStrategy(BufferedDataSource*);
missing parameter name. Shouldn't this take a DeferStrategy instead of a
BufferedDataSource?

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.h:151: friend class
ReadThenDeferStrategy;
Not thrilled by all this friending. It feels like something isn't encapsulated
correctly.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/defer_stra...
File webkit/glue/media/defer_strategy.cc (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/defer_stra...
webkit/glue/media/defer_strategy.cc:20: return false;
Shouldn't this a method in BufferedResourceLoader? This code shouldn't have to
know the details of BufferedResourceLoader.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:386:
setPreload(static_cast<WebMediaPlayer::Preload>(GetClient()->preload()));
What is the type of the enum we are casting from? Do we have 3 enums we have to
keep in sync?

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:505: return true;
Why is the return value changing?

[vrk (LEFT CHROMIUM), 2011-03-25 05:55:56]

> I think the defer_strategy stuff should be reworked. All of the friending and
> accessing of private data doesn't feel right to me. Perhaps if you just made
> some helper functions in BufferedResourceLoader that wrap the various
> Enable/Disable conditions you wouldn't need those classes. I'd be happy to
talk
> through it with you tomorrow.

Hey Aaron! Wanted to reply here real quick while this is still on my mind
because I think I might be OOO most of tomorrow tomorrow!  

Anyway I am naive, still-mostly-fresh-college-grad, so I have kind of
embarrassing pedantic questions for you :)

Full disclosure: I am pretty much stealing this pattern from the GoF C++ design
patterns book. Actually when I looked it up again just now, I realized this
technically isn't the Strategy pattern but it's the "State" pattern:
http://en.wikipedia.org/wiki/State_pattern

When I was coding this originally, I just had an enum for the DeferStrategy
(misnomer). But the situation seemed almost like a textbook "State pattern"
scenario, where for each of these functions I was pretty much making a
switch/case statement per state, where I wanted to do a completely different
thing for each enum value. So then when I encapsulated stuff in DeferStrategy, 
the code cleaned up considerably.

What part of this approach makes you most uncomfortable? Is it the friend
classes? 

The  friend class is also part of the original GoF pattern, though I must have
executed this wrong because in the original example only the base State needed
to be a friend class:
http://en.wikipedia.org/wiki/Talk%3AState_pattern#TCPConnection_class_the_GoF...

If it's not the friend classes, are you concerned that this is overkill by
making a pattern? Or is it the incorrect use of a pattern? Or is it the right
pattern but just poorly executed? (Personally I suspect it's that last one, but
want to hear your thoughts)

Anyway sorry for the interrogation! I desperately want to grow out of my
"textbook programming naivety" so your advice and guidance is muchly
appreciated!

[vrk (LEFT CHROMIUM), 2011-03-25 15:02:25]

WHOOPS OK ignore me, I just thought of a way simpler and superior way to
implement this lol. 

Aaron I'll run it by you in person today, but yeah sorry haha, I think I should
have gone through one more round of clean-up before submitting this to code
review!

[vrk (LEFT CHROMIUM), 2011-03-25 21:33:32]

Phew ok! Rewrote state objects -> enums. 

That was how I originally had written it, but there was a lot more behavior
happening per case in my first try than after I finally got it working and
cleaned it up a little haha. So even though there are still a couple of
switch/cases around, I think they are small enough that they are not worth the
trouble of encapsulating behavior in objects.

Hope this patch makes more sense!

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h#newcode46
media/base/filters.h:46: // Used to specify video preload states.
On 2011/03/25 04:35:28, acolwell wrote:
> You should add a note here that this enum must match the WebKit enum
especially
> if you use static_cast instead of a mapping function.
> 
> A reference to the HTTP spec or a description of the meanings of each value
> might be good.

Done.

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h#newcode139
media/base/filters.h:139: virtual void SetPreload(Preload preload);
On 2011/03/25 04:35:28, acolwell wrote:
> Make this pure virtual like the other methods.

Done.

http://codereview.chromium.org/6625059/diff/7016/media/base/filters.h#newcode195
media/base/filters.h:195: virtual void SetPreload(Preload preload);
On 2011/03/25 04:35:28, acolwell wrote:
> Make this pure virtual like the other method so implementers are forced to
think
> about this.

Done.

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline.h#newcode15
media/base/pipeline.h:15: #include "media/base/filters.h"
On 2011/03/25 04:35:28, acolwell wrote:
> It is unfortunate that we have to expose filters.h here. I'm tempted to
suggest
> putting the enum declaration in pipeline_status.h . Perhaps we should discuss
> this with Ami & Andrew. 

OK! For now I've kept it in the same place, but yeah, this should be discussed.

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:1083: pipeline_init_state_->demuxer_ = demuxer;
On 2011/03/25 04:35:28, acolwell wrote:
> Remove demuxer_ from pipeline_init_state since PipelineImpl now has requires a
> reference. This just makes things consistent with audio_renderer_ &
> video_renderer_

Done.

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:117: virtual Preload GetPreload() const;
On 2011/03/25 04:35:28, acolwell wrote:
> Move to private section since this isn't part of the Pipeline interface.

Done.

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:314: void FFmpegDemuxer::SetPlaybackRate(float
playback_rate) {
On 2011/03/25 04:35:28, acolwell wrote:
> This is necessary because we accidentally broke the existing functionality
when
> DataSources became owned by the demuxer instead of the CompositeFilter? Just
> checking.
> 
> If so could you add a unit test to make sure we capture this behavior.

Sure, will do!

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:319: void
FFmpegDemuxer::SetPreload(media::Preload preload) {
On 2011/03/25 04:35:28, acolwell wrote:
> shouldn't need media::

Done.

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/6625059/diff/7016/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:140: virtual void SetPreload(media::Preload
preload);
On 2011/03/25 04:35:28, acolwell wrote:
> shouldn't need media::

Done.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:189: DisableDeferIfNeeded();
On 2011/03/25 04:35:28, acolwell wrote:
> Why is this call needed now?

Ah, a little hard to explain :) 

This call is needed for the case when you cannot fulfill the read because you
don't have enough data, and you're deferred.

In the old version, the buffer would defer only if there was >5mb of data in the
buffer. This means that if you don't have enough data to fulfill the read, you
would have to be in process of loading data (not deferred) - unless your read
request was > 5 mb (which is a bug! issue 42956)

But now that I have the "ReadThenDefer" buffering scheme that reads only as
little as possible before deferring, it's much more likely that you're deferred
and won't be able to get more data.

Added a comment to try to explain. Let me know if you still have questions or
have another idea for implementation.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:208: DeferStrategy* strategy =
data_source->ChooseDeferStrategy();
On 2011/03/25 04:35:28, acolwell wrote:
> Why don't you do the ChooseDeferStrategy() in the call to this function? That
> way this code doesn't have to know about BufferedDataSource.

Done.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.cc:212: DisableDeferIfNeeded();
On 2011/03/25 04:35:28, acolwell wrote:
> The EnableDeferIfNeeded() followed by DisableDeferIfNeeded() is confusing. Is
it
> true that one of them will do something and the other is a noop? Will the
defer
> status get toggled on and then off?

Redid this to something more intuitive.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.h:91: virtual void
UpdateDeferStrategy(BufferedDataSource*);
On 2011/03/25 04:35:28, acolwell wrote:
> missing parameter name. Shouldn't this take a DeferStrategy instead of a
> BufferedDataSource?

Done.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/buffered_r...
webkit/glue/media/buffered_resource_loader.h:151: friend class
ReadThenDeferStrategy;
On 2011/03/25 04:35:28, acolwell wrote:
> Not thrilled by all this friending. It feels like something isn't encapsulated
> correctly.

N/A now!

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/defer_stra...
File webkit/glue/media/defer_strategy.cc (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/media/defer_stra...
webkit/glue/media/defer_strategy.cc:20: return false;
On 2011/03/25 04:35:28, acolwell wrote:
> Shouldn't this a method in BufferedResourceLoader? This code shouldn't have to
> know the details of BufferedResourceLoader.

lol yeah, oops. Fixed/NA anymore! :)

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:386:
setPreload(static_cast<WebMediaPlayer::Preload>(GetClient()->preload()));
Oops, I think this is a mistake because of a mistake in my WebKit interface.
Will update in the next patch.

http://codereview.chromium.org/6625059/diff/7016/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:505: return true;
On 2011/03/25 04:35:28, acolwell wrote:
> Why is the return value changing?

Actually I don't think the return value is used or needed (other than "bool" is
needed to comply with WebKit side of things). I will update the WebKit patch,
then this patch accordingly.

[acolwell GONE FROM CHROMIUM, 2011-03-25 23:08:13]

Looks good. Just a few more comments

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:166:
BufferedDataSource::ChooseDeferStrategy() {
Remove the indent. I looked at the style rules and they don't address this
directly. I consulted with Ami and he suggested removing the indent.

If you've seen examples elsewhere with the indent let me know.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:405: loader_->UpdateDeferBehavior();
SetDeferStrategy() doesn't update the behavior? Why is an UpdateDeferBehavior()
call needed here? Is it because the previous calls happen before Start()? It
might be better to hide this difference in SetDeferStrategy() so that this
object doesn't have to worry about which situations require a
UpdateDeferBehavior() call.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:39:
BufferedResourceLoader::DeferStrategy ChooseDeferStrategy();
Move to private:

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:14: #include
"webkit/glue/media/buffered_data_source.h"
Do you still need this?

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:345: if (buffer_->forward_bytes()
> buffer_->forward_capacity()) {
This looks like a change in behavior. why is this run all the time instead of
only when deferral is not allowed?

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:478: // We have an outstanding
read request, and we have not buffered enoug
enoug -> enough

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:501: deferred_ = newDeferSetting;
deferred_ = !deferred_

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:503:
url_loader_->setDefersLoading(newDeferSetting);
deferred_

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:31: class BufferedDataSource;
No longer necessary.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:40: enum DeferStrategy {
Brief comments about each of these.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:151: void UpdateDeferBehavior();
I believe this can become private once the one call in BufferedDataSource gets
moved to SetDeferStrategy().

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:158: 
Remove blank line

[vrk (LEFT CHROMIUM), 2011-03-29 00:53:00]

Fixed and replied to stuff!

I also updated the latest patch to match my changes in WebKit (SetPreload
doesn't return bool, one of the nasty casts is removed).

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:166:
BufferedDataSource::ChooseDeferStrategy() {
On 2011/03/25 23:08:14, acolwell wrote:
> Remove the indent. I looked at the style rules and they don't address this
> directly. I consulted with Ami and he suggested removing the indent.
> 
> If you've seen examples elsewhere with the indent let me know.

Yeah, it looked weird to me, too; I just made it that way because the script
complained about >80 chars. Done!

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:405: loader_->UpdateDeferBehavior();
On 2011/03/25 23:08:14, acolwell wrote:
> SetDeferStrategy() doesn't update the behavior? Why is an
UpdateDeferBehavior()
> call needed here? Is it because the previous calls happen before Start()? It
> might be better to hide this difference in SetDeferStrategy() so that this
> object doesn't have to worry about which situations require a
> UpdateDeferBehavior() call.

Good idea, done!

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.h:39:
BufferedResourceLoader::DeferStrategy ChooseDeferStrategy();
On 2011/03/25 23:08:14, acolwell wrote:
> Move to private:

Done.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:14: #include
"webkit/glue/media/buffered_data_source.h"
On 2011/03/25 23:08:14, acolwell wrote:
> Do you still need this?

Nope! Removed :)

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:345: if (buffer_->forward_bytes()
> buffer_->forward_capacity()) {
On 2011/03/25 23:08:14, acolwell wrote:
> This looks like a change in behavior. why is this run all the time instead of
> only when deferral is not allowed?

Ah I meant to tell you about this! Brief storytime:

As you probably know, the SeekableBuffer buffer_ does not actually impose a
limit on the amount of data it holds.

In the olden days, we only had the "ThresholdDefer" behavior, and we would defer
loading whenever we buffered 10mb or more; if there was a little bit more than
10mb in the buffer before we deferred loading, we wouldn't consume the small
amount of extra data.

Then I added the "NeverDefer" behavior. In this strategy, it's super important
to keep consuming the in-memory buffer to keep it at 10mb, otherwise you can end
up storing the whole video in memory. At this point I added the logic: if we
never defer, then we must consume memory when it hits 10mb; otherwise we don't
care to consume when we go over.

Now I have added the 3rd buffering strategy, and it also falls in the, "I don't
care about keeping the in-memory buffer exactly capped at 10mb because I will
defer before it becomes an issue" category. So originally I had some like
"AllowsDefer" method for which NeverDefer returned false and the other things
returned true.

But this is a weird/hard thing to explain, and it seems simpler just to keep a
hard 10mb limit for every strategy. What do you think, though?

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:478: // We have an outstanding
read request, and we have not buffered enoug
On 2011/03/25 23:08:14, acolwell wrote:
> enoug -> enough

Done.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:501: deferred_ = newDeferSetting;
On 2011/03/25 23:08:14, acolwell wrote:
> deferred_ = !deferred_

Done.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:503:
url_loader_->setDefersLoading(newDeferSetting);
On 2011/03/25 23:08:14, acolwell wrote:
> deferred_

Done.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:31: class BufferedDataSource;
On 2011/03/25 23:08:14, acolwell wrote:
> No longer necessary.

Done.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:40: enum DeferStrategy {
On 2011/03/25 23:08:14, acolwell wrote:
> Brief comments about each of these.

Done.

I also changed the naming style to match the style of the State enum in
pipeline_impl.h, but the convention is different in pipeline_status.h? Style
guide is a little ambiguous; I could read it saying we only use MACRO_STYLE or
either is OK based on what existing code was doing?

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:151: void UpdateDeferBehavior();
On 2011/03/25 23:08:14, acolwell wrote:
> I believe this can become private once the one call in BufferedDataSource gets
> moved to SetDeferStrategy().

Done.

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:158: 
On 2011/03/25 23:08:14, acolwell wrote:
> Remove blank line

Done.

[acolwell GONE FROM CHROMIUM, 2011-03-29 05:52:03]

LGTM

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:345: if (buffer_->forward_bytes()
> buffer_->forward_capacity()) {
Ok. I think I understand. It sounds reasonable to me.

On 2011/03/29 00:53:00, Victoria Kirst wrote:
> On 2011/03/25 23:08:14, acolwell wrote:
> > This looks like a change in behavior. why is this run all the time instead
of
> > only when deferral is not allowed?
> 
> Ah I meant to tell you about this! Brief storytime:
> 
> As you probably know, the SeekableBuffer buffer_ does not actually impose a
> limit on the amount of data it holds.
> 
> In the olden days, we only had the "ThresholdDefer" behavior, and we would
defer
> loading whenever we buffered 10mb or more; if there was a little bit more than
> 10mb in the buffer before we deferred loading, we wouldn't consume the small
> amount of extra data.
> 
> Then I added the "NeverDefer" behavior. In this strategy, it's super important
> to keep consuming the in-memory buffer to keep it at 10mb, otherwise you can
end
> up storing the whole video in memory. At this point I added the logic: if we
> never defer, then we must consume memory when it hits 10mb; otherwise we don't
> care to consume when we go over.
> 
> Now I have added the 3rd buffering strategy, and it also falls in the, "I
don't
> care about keeping the in-memory buffer exactly capped at 10mb because I will
> defer before it becomes an issue" category. So originally I had some like
> "AllowsDefer" method for which NeverDefer returned false and the other things
> returned true.
> 
> But this is a weird/hard thing to explain, and it seems simpler just to keep a
> hard 10mb limit for every strategy. What do you think, though?

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6625059/diff/18001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:40: enum DeferStrategy {
I think the kXXX style is good here. I think PipelineStatus is MACRO_STYLE
because it looks more like error code macros  in Unix & Windows. I don't know
the history, but that would be my guess.
On 2011/03/29 00:53:00, Victoria Kirst wrote:
> On 2011/03/25 23:08:14, acolwell wrote:
> > Brief comments about each of these.
> 
> Done.
> 
> I also changed the naming style to match the style of the State enum in
> pipeline_impl.h, but the convention is different in pipeline_status.h? Style
> guide is a little ambiguous; I could read it saying we only use MACRO_STYLE or
> either is OK based on what existing code was doing?

http://codereview.chromium.org/6625059/diff/25001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/6625059/diff/25001/media/base/filters.h#newcode53
media/base/filters.h:53: PRELOAD_NONE,
nit: How about dropping the PRELOAD_ prefix now that your comments got me
thinking about this. We should probably keep it MACRO_STYLE here since the other
enums in here are in that style.

http://codereview.chromium.org/6625059/diff/25001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6625059/diff/25001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:503:
pipeline_->SetPreload(static_cast<media::Preload>(preload));
This just makes me nervous. Can we just put a switch here w/o default: so that
any new values added to WebKit::WebMediaPlayer::Preload will cause a compile
error? I just want to make sure we are protected from subtle bugs if WebKit
decides to reverse alphabetize the constants or something. ;)

[vrk (LEFT CHROMIUM), 2011-03-30 17:41:43]

Thanks for the review, acolwell!

http://codereview.chromium.org/6625059/diff/25001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/6625059/diff/25001/media/base/filters.h#newcode53
media/base/filters.h:53: PRELOAD_NONE,
On 2011/03/29 05:52:03, acolwell wrote:
> nit: How about dropping the PRELOAD_ prefix now that your comments got me
> thinking about this. We should probably keep it MACRO_STYLE here since the
other
> enums in here are in that style.

Done.

http://codereview.chromium.org/6625059/diff/25001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6625059/diff/25001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:503:
pipeline_->SetPreload(static_cast<media::Preload>(preload));
On 2011/03/29 05:52:03, acolwell wrote:
> This just makes me nervous. Can we just put a switch here w/o default: so that
> any new values added to WebKit::WebMediaPlayer::Preload will cause a compile
> error? I just want to make sure we are protected from subtle bugs if WebKit
> decides to reverse alphabetize the constants or something. ;)

Agreed! Added compile-time asserts.

[acolwell GONE FROM CHROMIUM, 2011-03-30 19:48:22]

LGTM
On 2011/03/30 17:41:43, Victoria Kirst wrote:
> Thanks for the review, acolwell!
> 
> http://codereview.chromium.org/6625059/diff/25001/media/base/filters.h
> File media/base/filters.h (right):
> 
>
http://codereview.chromium.org/6625059/diff/25001/media/base/filters.h#newcode53
> media/base/filters.h:53: PRELOAD_NONE,
> On 2011/03/29 05:52:03, acolwell wrote:
> > nit: How about dropping the PRELOAD_ prefix now that your comments got me
> > thinking about this. We should probably keep it MACRO_STYLE here since the
> other
> > enums in here are in that style.
> 
> Done.
> 
>
http://codereview.chromium.org/6625059/diff/25001/webkit/glue/webmediaplayer_...
> File webkit/glue/webmediaplayer_impl.cc (right):
> 
>
http://codereview.chromium.org/6625059/diff/25001/webkit/glue/webmediaplayer_...
> webkit/glue/webmediaplayer_impl.cc:503:
> pipeline_->SetPreload(static_cast<media::Preload>(preload));
> On 2011/03/29 05:52:03, acolwell wrote:
> > This just makes me nervous. Can we just put a switch here w/o default: so
that
> > any new values added to WebKit::WebMediaPlayer::Preload will cause a compile
> > error? I just want to make sure we are protected from subtle bugs if WebKit
> > decides to reverse alphabetize the constants or something. ;)
> 
> Agreed! Added compile-time asserts.

[scherkus (not reviewing), 2011-03-31 23:40:21]

looks awesome!!

minor style nits

http://codereview.chromium.org/6625059/diff/34001/media/filters/file_data_sou...
File media/filters/file_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/34001/media/filters/file_data_sou...
media/filters/file_data_source.cc:119: void FileDataSource::SetPreload(Preload
preload) {
nit: empty methods should be {} if it's a one-liner

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:391:
BufferedResourceLoader::DeferStrategy BufferedDataSource::ChooseDeferStrategy()
{
nit: >80 chars

drop the method name onto next line

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:503: deferred_ = !deferred_;
nit: over-indented by 2 spaces

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:45: kThresholdDefer
add trailing ,

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:142: void
SimpleDataSource::SetPreload(media::Preload preload) {
nit: one-liner -> {}

[scherkus (not reviewing), 2011-03-31 23:40:28]

oh that means LGTM :)

[vrk (LEFT CHROMIUM), 2011-04-01 22:52:12]

Thanks everyone!

The WebKit side of this landed (thanks scherkus!) and DEPS were rolled
appropriately far in Chromium, now waiting to see try bot results. If try bots
are clear then I'll land.

http://codereview.chromium.org/6625059/diff/34001/media/filters/file_data_sou...
File media/filters/file_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/34001/media/filters/file_data_sou...
media/filters/file_data_source.cc:119: void FileDataSource::SetPreload(Preload
preload) {
On 2011/03/31 23:40:21, scherkus wrote:
> nit: empty methods should be {} if it's a one-liner

Done.

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:391:
BufferedResourceLoader::DeferStrategy BufferedDataSource::ChooseDeferStrategy()
{
On 2011/03/31 23:40:21, scherkus wrote:
> nit: >80 chars
> 
> drop the method name onto next line

Done.

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.cc:503: deferred_ = !deferred_;
On 2011/03/31 23:40:21, scherkus wrote:
> nit: over-indented by 2 spaces

Done.

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_resource_loader.h:45: kThresholdDefer
On 2011/03/31 23:40:21, scherkus wrote:
> add trailing ,

Done.

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/simple_da...
File webkit/glue/media/simple_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/34001/webkit/glue/media/simple_da...
webkit/glue/media/simple_data_source.cc:142: void
SimpleDataSource::SetPreload(media::Preload preload) {
On 2011/03/31 23:40:21, scherkus wrote:
> nit: one-liner -> {}

Done.

[vrk (LEFT CHROMIUM), 2011-04-05 00:56:17]

Oops, I forgot about unit tests. But try bots reminded me!

The old, existing tests I had written were pretty crappy  so I rewrote them
almost completely to a) compile and b) make sense with the new code.

@scherkus/acolwell could you take another look? Sorry and thanks!

[scherkus (not reviewing), 2011-04-05 06:22:45]

LGTM w/ 1 µ-nit

http://codereview.chromium.org/6625059/diff/44001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/44001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:386:
BufferedDataSource::ChooseDeferStrategy() {
nit: method name should be completely un-indented

[acolwell GONE FROM CHROMIUM, 2011-04-05 15:56:18]

LGTM
On 2011/04/05 00:56:17, Victoria Kirst wrote:
> Oops, I forgot about unit tests. But try bots reminded me!
> 
> The old, existing tests I had written were pretty crappy  so I rewrote them
> almost completely to a) compile and b) make sense with the new code.
> 
> @scherkus/acolwell could you take another look? Sorry and thanks!

[vrk (LEFT CHROMIUM), 2011-04-05 16:17:41]

Awesome! Thanks everyone, will land today!

http://codereview.chromium.org/6625059/diff/44001/webkit/glue/media/buffered_...
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/6625059/diff/44001/webkit/glue/media/buffered_...
webkit/glue/media/buffered_data_source.cc:386:
BufferedDataSource::ChooseDeferStrategy() {
On 2011/04/05 06:22:45, scherkus wrote:
> nit: method name should be completely un-indented

Done.