Introduce CancelUponDeferral() to buffered media loaders.

Introduce CancelUponDeferral() to buffered media loaders.

Fixes hitting the maximum connection limit of 6 per domain when a
media tag is used with preload=metadata.

Deferred network connections will be cancelled once reaching the
HAVE_ENOUGH_DATA state for preload=metadata, non-streaming
resources that have not started playback.

A soft cancel is introduced which allows us to preserve the data
cached thus far in the BufferedResourceLoader. Once this cache is
exhausted the loader will fulfill subsequent reads with a cache
miss and continue on as normal.

preload=metadata intentionally does not cache enough data for a
smooth playback experience on slow connections, so the cache miss
here does not increase jank relative to the current status quo.

For similar reasons, since preload=auto does load enough data for
an uninterrupted playback, we can't use this method there or we'll
introduce jank during the cache miss and subsequent catch up.

Note: This does not resolve hung connections if multiple media
elements are actually playing back or if they are served from
a server without range requests.

Note 2: This also fixes a broken media_blink_unittests, which was
missing some Gin dependency.

BUG=162627
TEST=new unittest, passes layout tests.

Committed: https://crrev.com/849cf4b2ae0cd74c9f618b0950fee69f075de492
Cr-Commit-Position: refs/heads/master@{#322610}

[DaleCurtis, 2015-03-24 02:02:52]

dalecurtis@chromium.org changed reviewers:
+ philipj@opera.com, xhwang@chromium.org

[DaleCurtis, 2015-03-24 02:03:33]

dalecurtis@chromium.org changed reviewers:
+ jochen@chromium.org

[DaleCurtis, 2015-03-24 02:03:33]

+jochen for +gin DEP to get media_blink_unittests working, which since they
weren't loading the isolates properly.

[mmenke, 2015-03-24 02:05:43]

On 2015/03/24 02:03:33, DaleCurtis wrote:
> +jochen for +gin DEP to get media_blink_unittests working, which since they
> weren't loading the isolates properly.

Awesome!  Great to see this!

[DaleCurtis, 2015-03-24 02:59:40]

Actually, I'm beginning to think this should work for the preload=auto case
too... If we hit capacity defer and then close the connection, so long as we
keep the buffered data around, there shouldn't be any performance hit...

[philipj_slow, 2015-03-24 07:26:52]

Note that the default preload state is auto, which is allowed but not what the
spec recommends (the comment in HTMLMediaElement::parseAttribute is wrong). I
suspect most sites do note set an explicit preload, so if this cannot be made to
work for preload=auto, then changing the default to metadata would seem to be
necessary. Incidentally, that's something I think we should do anyway.

[philipj_slow, 2015-03-24 07:35:03]

From the description:
> The WebMediaPlayerImpl will now cancel deferred connections once
> reaching the HAVE_METADATA state if the element is not playing.

Is this guaranteed to happen after any events associated with reaching
HAVE_METADATA have already been fired at the page? It would be reasonable to
expect that if one starts playing in the loadedmetadata event or any time before
the suspend event is fired, that the network connection isn't going to be
restarted.

Assuming the first frame is decoded we should reach readyState
HAVE_CURRENT_DATA, so unless there's something to clamp readyState (not sure)
there are a few more events to worry about.

[jochen (gone - plz use gerrit), 2015-03-24 09:25:16]

lgtm

[DaleCurtis, 2015-03-24 17:14:14]

On 2015/03/24 07:35:03, philipj_UTC7 wrote:
> From the description:
> > The WebMediaPlayerImpl will now cancel deferred connections once
> > reaching the HAVE_METADATA state if the element is not playing.
> 
> Is this guaranteed to happen after any events associated with reaching
> HAVE_METADATA have already been fired at the page? It would be reasonable to
> expect that if one starts playing in the loadedmetadata event or any time
before
> the suspend event is fired, that the network connection isn't going to be
> restarted.
> 
> Assuming the first frame is decoded we should reach readyState
> HAVE_CURRENT_DATA, so unless there's something to clamp readyState (not sure)
> there are a few more events to worry about.

I don't think this is a problem in the preload=metadata case, since the
connection
doesn't defer immediately, only when reads are done. For preload=auto you're
right
that we want to abort the cancel if playback starts before hitting capacity
deferr.

I just realized the main downside of this change though, the loader is not
preemptively
restarted once playback starts, so cached data is exhausted and then a cache
miss occurs
which can take several hundred milliseconds (or more depending on connection
speed) to
restart the download.

I'll see if there's a non-invasive way to add such support. I don't want to do
too much
here though since ultimately we want to nuke this whole system.

[DaleCurtis, 2015-03-24 20:29:08]

On 2015/03/24 17:14:14, DaleCurtis wrote:
> On 2015/03/24 07:35:03, philipj_UTC7 wrote:
> > From the description:
> > > The WebMediaPlayerImpl will now cancel deferred connections once
> > > reaching the HAVE_METADATA state if the element is not playing.
> > 
> > Is this guaranteed to happen after any events associated with reaching
> > HAVE_METADATA have already been fired at the page? It would be reasonable to
> > expect that if one starts playing in the loadedmetadata event or any time
> before
> > the suspend event is fired, that the network connection isn't going to be
> > restarted.
> > 
> > Assuming the first frame is decoded we should reach readyState
> > HAVE_CURRENT_DATA, so unless there's something to clamp readyState (not
sure)
> > there are a few more events to worry about.
> 
> I don't think this is a problem in the preload=metadata case, since the
> connection
> doesn't defer immediately, only when reads are done. For preload=auto you're
> right
> that we want to abort the cancel if playback starts before hitting capacity
> deferr.

Actually you were right, canceling in OnPipelineMetadata() doesn't
allow any play() attempts to come in; so I've moved the functionality
to when we hit HAVE_ENOUGH.

> 
> I just realized the main downside of this change though, the loader is not
> preemptively
> restarted once playback starts, so cached data is exhausted and then a cache
> miss occurs
> which can take several hundred milliseconds (or more depending on connection
> speed) to
> restart the download.
> 
> I'll see if there's a non-invasive way to add such support. I don't want to do
> too much
> here though since ultimately we want to nuke this whole system.

Unfortunately, there doesn't appear to be a simple way to fix the preload=auto
case with our current architecture. The cache miss is too expensive on low
bandwidth connections and there's no easy way to gracefully restart the
connection preemptively before running out of data. :(

E.g., previously a user with a slow connection and preload=auto would be able
to play through in cases where the forward capacity was sufficient, but now
they'll hang once the cache capacity is exhausted while we try to catch up.

As such, I also think the performance with preload=metadata isn't good enough
for slow connections to change the default away from preload=auto.

It's not impossible, but I think it's outside the scope of the changes we want
to make to this code given its life support status. I'll see about fixing up
the cache architecture in the coming months.

[xhwang, 2015-03-26 07:21:55]

I am a little concerned about the preload=auto case. Will this cause extra
buffering time for users with slow connection (after the cache miss)? I think
most pages have <6 videos and we don't want to sacrifice the performance for
most users for some rate case... Does that make sense?

https://codereview.chromium.org/1029763002/diff/20001/media/blink/buffered_re...
File media/blink/buffered_resource_loader.cc (right):

https://codereview.chromium.org/1029763002/diff/20001/media/blink/buffered_re...
media/blink/buffered_resource_loader.cc:579: cancel_upon_deferral_ = false;
Move this block to l.585?

https://codereview.chromium.org/1029763002/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1029763002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:832: }
Seems like |data_source_| knows the preload type, IsStreaming() and
media_has_played(), can we move these logic to |data_source_|?

e.g.

if (data_source_)
  data_source_->OnBufferingHaveEnough();

[DaleCurtis, 2015-03-26 17:22:00]

Can you elaborate on your concern for preload=auto? This is limited to
preload=metadata for the reasons outlined in my previous comments, so there
should be no change for preload=auto.

https://codereview.chromium.org/1029763002/diff/20001/media/blink/buffered_re...
File media/blink/buffered_resource_loader.cc (right):

https://codereview.chromium.org/1029763002/diff/20001/media/blink/buffered_re...
media/blink/buffered_resource_loader.cc:579: cancel_upon_deferral_ = false;
On 2015/03/26 07:21:55, xhwang wrote:
> Move this block to l.585?

Done.

https://codereview.chromium.org/1029763002/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1029763002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:832: }
On 2015/03/26 07:21:55, xhwang wrote:
> Seems like |data_source_| knows the preload type, IsStreaming() and
> media_has_played(), can we move these logic to |data_source_|?
> 
> e.g.
> 
> if (data_source_)
>   data_source_->OnBufferingHaveEnough();

Great idea!

[xhwang, 2015-03-26 20:07:51]

On 2015/03/26 17:22:00, DaleCurtis wrote:
> Can you elaborate on your concern for preload=auto? This is limited to
> preload=metadata for the reasons outlined in my previous comments, so there
> should be no change for preload=auto.

Right. I misunderstood you previous comments. Please ignore it.

LGTM

[DaleCurtis, 2015-03-27 16:42:43]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-03-27 16:42:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1029763002/#ps40001
(title: "Comments")

[commit-bot: I haz the power, 2015-03-27 16:43:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029763002/40001

[commit-bot: I haz the power, 2015-03-27 18:35:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-27 18:36:50]

Patchset 3 (id:??) landed as
https://crrev.com/849cf4b2ae0cd74c9f618b0950fee69f075de492
Cr-Commit-Position: refs/heads/master@{#322610}