Introduce the concept of media task runner.

Introduce the concept of media task runner.

A media task runner can receive tasks whose scheduling
depends on a media timestamp.
The balanced media task runner factory generates some media task
runners whose task schedulings are loosely synchronized between
each other. A direct application is to query audio and video frames
in a balanced way, which minimizes the memory used for muxed streams.

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

[damienv1, 2014-09-03 02:06:45]

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

[damienv1, 2014-09-03 02:06:58]

Please take a look. Thanks !

[xhwang, 2014-09-03 17:06:40]

In media code, we try to do things in the simplest way possible. For example:
- don't use an interface if there's only one implementation,
- don't use wrapper class if it only wraps one particular class and only does
trivial work

But I definitely understand the style you are using :) So the CL looks good to
me % some nits.

I'll defer to chromecast owners to finish the review.

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/ba...
File chromecast/media/cma/base/balanced_media_task_runner_factory.cc (right):

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/ba...
chromecast/media/cma/base/balanced_media_task_runner_factory.cc:22: : public
MediaTaskRunner {
fit in one line?

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/ba...
chromecast/media/cma/base/balanced_media_task_runner_factory.cc:27: const
base::Closure shutdown_cb);
Document what the two callbacks are and when they are expected to be called.

Also, pass callbacks by const-ref.

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/me...
File chromecast/media/cma/base/media_task_runner.h (right):

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/me...
chromecast/media/cma/base/media_task_runner.h:29: base::TimeDelta timestamp) =
0;
How is media |timestamp| used to calculate the time when the task will actually
be run? I can understand it with the context of the balanced task runner. But it
seems a bit odd to look at this as a standalong interface. Since we have only
one impl of this interface, can we just use the concrete class without having
this interface?

[damienv1, 2014-09-03 22:01:03]

Regarding the use of a wrapper class (I assume you talk about 
MediaTaskRunnerWithNotification), this is needed so that the factory does not
hold a ref on MediaTaskRunnerWithNotification but only on
BalancedMediaTaskRunner.
I added a note in the code.

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/ba...
File chromecast/media/cma/base/balanced_media_task_runner_factory.cc (right):

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/ba...
chromecast/media/cma/base/balanced_media_task_runner_factory.cc:22: : public
MediaTaskRunner {
On 2014/09/03 17:06:39, xhwang wrote:
> fit in one line?

Done.

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/ba...
chromecast/media/cma/base/balanced_media_task_runner_factory.cc:27: const
base::Closure shutdown_cb);
On 2014/09/03 17:06:39, xhwang wrote:
> Document what the two callbacks are and when they are expected to be called.
> 
> Also, pass callbacks by const-ref.

Good catch. That's an unwilling mistake on my side.

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/me...
File chromecast/media/cma/base/media_task_runner.h (right):

https://codereview.chromium.org/534893002/diff/1/chromecast/media/cma/base/me...
chromecast/media/cma/base/media_task_runner.h:29: base::TimeDelta timestamp) =
0;
On 2014/09/03 17:06:39, xhwang wrote:
> How is media |timestamp| used to calculate the time when the task will
actually
> be run? I can understand it with the context of the balanced task runner. But
it
> seems a bit odd to look at this as a standalong interface. Since we have only
> one impl of this interface, can we just use the concrete class without having
> this interface?

1) Note added about how |timestamp| should be used.

2) Agree that currently all the media task runners could be "hard-coded" without
using a base class.
   However, I wanted to make it similar to the base::TaskRunner design
   and I was planning initially to use the same interface for synchronization (a
derived class using a Clock could be used to synchronized audio with video and
that would fix some of the TODOs we have in the current video_renderer_impl).
   So I'd like to keep it for now.

[lcwu1, 2014-09-04 00:32:54]

LGTM with nit.

https://codereview.chromium.org/534893002/diff/20001/chromecast/media/cma/bas...
File chromecast/media/cma/base/balanced_media_task_runner_factory.h (right):

https://codereview.chromium.org/534893002/diff/20001/chromecast/media/cma/bas...
chromecast/media/cma/base/balanced_media_task_runner_factory.h:21: class
BalancedMediaTaskRunner;
nit: Add an empty line between namespace and class lines.

[damienv1, 2014-09-04 17:55:56]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-09-04 17:57:53]

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

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

Committed patchset #3 (id:40001) as 81c670096ffb89002ad3459cce9f7da44425762b

[commit-bot: I haz the power, 2014-09-10 03:33:46]

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