Add a buffering controller for Chromecast.

Add a buffering controller for Chromecast.

The buffering controller serves two purposes:
- for URL playback, this will smooth playback when network
  condition is not good enough. This basically avoids continuous
  stuttering in that case.
- for MSE playback, it stops playback when either audio or video
  underruns (which is a special case of the low buffer level state).

BUG=408189
Committed: https://crrev.com/63429df7b27fefd8d7af9f73986b3e08b343a430
Cr-Commit-Position: refs/heads/master@{#292764}

[damienv1, 2014-08-28 00:50:14]

damienv@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@google.com, scherkus@chromium.org,
xhwang@chromium.org

[damienv1, 2014-08-28 00:50:51]

This is an initial overview of the buffering controller on Chromecast. I still
need to fix the GYP structure.

[damienv1, 2014-08-28 01:20:12]

Please take a look. Thanks !

[lcwu1, 2014-08-28 02:14:58]

lcwu@chromium.org changed reviewers:
+ lcwu@chromium.org

[lcwu1, 2014-08-28 02:14:58]

https://codereview.chromium.org/509213002/diff/60001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:157: 'cast_metrics',
Should we add cast_media here as a dependency so that the new source files at
least get built when building cast_shell?

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_controller.cc (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_controller.cc:20: : weak_factory_(this),
While it won't affect code correctness language-wise, I think in general people
initialize the class members in the same order as they are declared. (I don't
know whether there is a style guide about it though.)

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_state.h (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:11: #include "base/time/time.h"
We should also include ref_counted.h. (IWYU)

[gunsch, 2014-08-28 02:39:54]

Just a few style/documentation nits. Code is LGTM, but obviously someone on
media/ team should approve as well :)

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_state.h (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:11: #include "base/time/time.h"
On 2014/08/28 02:14:58, lcwu1 wrote:
> We should also include ref_counted.h. (IWYU)

also base/macros.h, here and in buffering_controller.h

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:53: // Create a new buffering state.
The initial state is |kLowLevel|.
style nit (throughout the CL): "Function comments should be descriptive ("Opens
the file") rather than imperative ("Open the file");"

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:70: // Set the current rendering
time for that stream.
"this" stream ? (here and below)

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:85: // Notify the buffering has
reached its maximum capacity.
Clarify: who is being notified?

[xhwang, 2014-08-28 20:44:43]

I have some general questions about the upsteaming process:
1, Will everything compiles if this CL is landed?
2, How should we review these CLs? Shall we enforce media coding
convention/styles, or just treat chromecast folder a separate folder than media
(e.g. net, cc...). But in that case, shall we use a chromecast namespace instead
of media?

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_controller.h (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_controller.h:23: typedef
base::Callback<void(bool)> BufferingNotificationCb;
nit: in media code, we use FooCB for callback type. Note it's CB instead of Cb.

[lcwu1, 2014-08-28 21:11:43]

On 2014/08/28 20:44:43, xhwang wrote:
> I have some general questions about the upsteaming process:
> 1, Will everything compiles if this CL is landed?

Not talking about this particular CL, in general any CL we land should not break
cast_shell build. In some cases where we have to break up a feature into several
CLs (each of which alone would break cast_shell) in order to make code review
easier, they should be landed together.


> 2, How should we review these CLs? Shall we enforce media coding
> convention/styles, or just treat chromecast folder a separate folder than
media
> (e.g. net, cc...). But in that case, shall we use a chromecast namespace
instead
> of media?

We definitely want the code inside chromecast/ to be conforming to Chromium's
coding style. And if there are any conventions (not specified in either chromium
or google style guides) in a particular component, we want to follow them too if
we subclass its classes or implement its client/delegate/observer APIs.

In terms of namespace, I think we should use 'chromecast' as our top-level
namespace. (That is, the components inside chromecast/ should have namespace
like 'chromecast::shell', 'chromecast::media', 'chromecast::gfx',
'chromecast::net', etc.). I don't know whether Damien had any practical
concerns/issues so that he chose to use 'media' as the top-level namespace here.
I will defer to him to answer this question.


> 
>
https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
> File chromecast/media/cma/base/buffering_controller.h (right):
> 
>
https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
> chromecast/media/cma/base/buffering_controller.h:23: typedef
> base::Callback<void(bool)> BufferingNotificationCb;
> nit: in media code, we use FooCB for callback type. Note it's CB instead of
Cb.

[damienv1, 2014-08-29 15:09:06]

https://codereview.chromium.org/509213002/diff/60001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:157: 'cast_metrics',
On 2014/08/28 02:14:58, lcwu1 wrote:
> Should we add cast_media here as a dependency so that the new source files at
> least get built when building cast_shell?

Done.

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_controller.cc (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_controller.cc:20: : weak_factory_(this),
On 2014/08/28 02:14:58, lcwu1 wrote:
> While it won't affect code correctness language-wise, I think in general
people
> initialize the class members in the same order as they are declared. (I don't
> know whether there is a style guide about it though.)

I changed the order in the header file and forgot to change the initialization
list.
Done.

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_controller.h (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_controller.h:23: typedef
base::Callback<void(bool)> BufferingNotificationCb;
On 2014/08/28 20:44:43, xhwang wrote:
> nit: in media code, we use FooCB for callback type. Note it's CB instead of
Cb.

Done.

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
File chromecast/media/cma/base/buffering_state.h (right):

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:11: #include "base/time/time.h"
On 2014/08/28 02:14:58, lcwu1 wrote:
> We should also include ref_counted.h. (IWYU)

Done.

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:11: #include "base/time/time.h"
On 2014/08/28 02:14:58, lcwu1 wrote:
> We should also include ref_counted.h. (IWYU)

Done.

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:53: // Create a new buffering state.
The initial state is |kLowLevel|.
On 2014/08/28 02:39:54, gunsch wrote:
> style nit (throughout the CL): "Function comments should be descriptive
("Opens
> the file") rather than imperative ("Open the file");"

Done.

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:70: // Set the current rendering
time for that stream.
On 2014/08/28 02:39:54, gunsch wrote:
> "this" stream ? (here and below)

Done.

https://codereview.chromium.org/509213002/diff/60001/chromecast/media/cma/bas...
chromecast/media/cma/base/buffering_state.h:85: // Notify the buffering has
reached its maximum capacity.
On 2014/08/28 02:39:54, gunsch wrote:
> Clarify: who is being notified?

Done.

[gunsch, 2014-08-29 15:34:38]

lgtm

[lcwu1, 2014-08-29 16:16:28]

lgtm

[damienv1, 2014-08-29 23:18:52]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-08-29 23:24:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/damienv@chromium.org/509213002/80001

[damienv1, 2014-08-29 23:46:24]

The CQ bit was unchecked by damienv@chromium.org

[xhwang, 2014-08-29 23:49:23]

looking good.

One question: since we are dropping the cma namespace and use chromecast/media,
does it still make sense to keep the cma folder?

[gunsch, 2014-08-29 23:54:43]

On 2014/08/29 23:49:23, xhwang wrote:
> looking good.
> 
> One question: since we are dropping the cma namespace and use
chromecast/media,
> does it still make sense to keep the cma folder?

I would lean towards yes. We have a separation (locally) between other code in
chromecast/media (generic media handling code) and what we call "CMA" in
chromecast/media/cma (specifically related to our IPC-based rendering
path/pipeline).

[damienv1, 2014-08-29 23:57:05]

@xhwang
In media, we have also CDM related components, and possibly others (e.g. related
to the audio backend, when going through WebAudio).
I wanted to have "cma" be the place holder for media playback inside
chromecast/media

[xhwang, 2014-08-30 00:10:19]

On 2014/08/29 23:57:05, damienv1 wrote:
> @xhwang
> In media, we have also CDM related components, and possibly others (e.g.
related
> to the audio backend, when going through WebAudio).
> I wanted to have "cma" be the place holder for media playback inside
> chromecast/media

That makes sense. rblgtm!

[xhwang, 2014-08-30 00:11:04]

On 2014/08/30 00:10:19, xhwang wrote:
> On 2014/08/29 23:57:05, damienv1 wrote:
> > @xhwang
> > In media, we have also CDM related components, and possibly others (e.g.
> related
> > to the audio backend, when going through WebAudio).
> > I wanted to have "cma" be the place holder for media playback inside
> > chromecast/media
> 
> That makes sense. rblgtm!

I guess I meant rubberstamp lgtm :)

[damienv1, 2014-08-30 00:39:15]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-08-30 00:40:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/damienv@chromium.org/509213002/100001

[commit-bot: I haz the power, 2014-08-30 01:25:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-30 02:28:54]

Committed patchset #6 (id:100001) as c6accfb59d16c29af868cec3ea7607b1bdd33f03

[commit-bot: I haz the power, 2014-09-10 03:13:30]

Patchset 6 (id:??) landed as
https://crrev.com/63429df7b27fefd8d7af9f73986b3e08b343a430
Cr-Commit-Position: refs/heads/master@{#292764}