Connect MojoAudioDecoder to the service

Connect MojoAudioDecoder to the service

MojoAudioDecoder is the client side part of the decoder
that runs in the renderer process.

This CL connects it to the remote service and propagates
the media task runner to it.

BUG=542910
Committed: https://crrev.com/fe54753dafe23379ba6ec9d20d08d1a32b9ecc13
Cr-Commit-Position: refs/heads/master@{#381861}

[Tima Vaisburd, 2016-03-17 20:14:44]

timav@chromium.org changed reviewers:
+ sandersd@chromium.org, xhwang@chromium.org

[Tima Vaisburd, 2016-03-17 20:14:44]

PTAL.

[sandersd (OOO until July 31), 2016-03-17 20:24:07]

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
The task_runner_ is discarded here. If this works (ie. the constructor thread is
the correct thread), why pass a task runner at all?

If this is a bug, another alternative is to require the caller to bind the
callbacks to the correct loop themselves. Do we have a reason to pick one
solution over the other?

[xhwang, 2016-03-17 20:24:07]

In the CL description s/propages/propagates. Otherwise LGTM

[xhwang, 2016-03-17 20:26:25]

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/03/17 20:24:07, sandersd wrote:
> The task_runner_ is discarded here. If this works (ie. the constructor thread
is
> the correct thread), why pass a task runner at all?
> 
> If this is a bug, another alternative is to require the caller to bind the
> callbacks to the correct loop themselves. Do we have a reason to pick one
> solution over the other?

Good catch. This seems a bug. We should probably always use the |task_runner|
passed in the ctor.

[xhwang, 2016-03-17 20:27:27]

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/03/17 20:26:25, xhwang wrote:
> On 2016/03/17 20:24:07, sandersd wrote:
> > The task_runner_ is discarded here. If this works (ie. the constructor
thread
> is
> > the correct thread), why pass a task runner at all?
> > 
> > If this is a bug, another alternative is to require the caller to bind the
> > callbacks to the correct loop themselves. Do we have a reason to pick one
> > solution over the other?
> 
> Good catch. This seems a bug. We should probably always use the |task_runner|
> passed in the ctor.

Also, this class should be single threaded. We should check that we are on
|task_runner_| in every call.

[Tima Vaisburd, 2016-03-17 20:31:24]

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/03/17 20:24:07, sandersd wrote:
> The task_runner_ is discarded here. If this works (ie. the constructor thread
is
> the correct thread), why pass a task runner at all?
> 
> If this is a bug, another alternative is to require the caller to bind the
> callbacks to the correct loop themselves. Do we have a reason to pick one
> solution over the other?

This is a bug, thank you.
I meant to use the task_runner from ctor that originates in
https://code.google.com/p/chromium/codesearch#chromium/src/media/renderers/de...

I think we want to respect this parameter?

[Tima Vaisburd, 2016-03-17 20:39:58]

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/03/17 20:27:26, xhwang wrote:
> On 2016/03/17 20:26:25, xhwang wrote:
> > On 2016/03/17 20:24:07, sandersd wrote:
> > > The task_runner_ is discarded here. If this works (ie. the constructor
> thread
> > is
> > > the correct thread), why pass a task runner at all?
> > > 
> > > If this is a bug, another alternative is to require the caller to bind the
> > > callbacks to the correct loop themselves. Do we have a reason to pick one
> > > solution over the other?
> > 
> > Good catch. This seems a bug. We should probably always use the
|task_runner|
> > passed in the ctor.
> 
> Also, this class should be single threaded. We should check that we are on
> |task_runner_| in every call.

Done.

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/03/17 20:31:24, Tima Vaisburd wrote:
> On 2016/03/17 20:24:07, sandersd wrote:
> > The task_runner_ is discarded here. If this works (ie. the constructor
thread
> is
> > the correct thread), why pass a task runner at all?
> > 
> > If this is a bug, another alternative is to require the caller to bind the
> > callbacks to the correct loop themselves. Do we have a reason to pick one
> > solution over the other?
> 
> This is a bug, thank you.
> I meant to use the task_runner from ctor that originates in
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/renderers/de...
> 
> I think we want to respect this parameter?

Done.

[xhwang, 2016-03-17 20:48:11]

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1797393007/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:35: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/03/17 20:31:24, Tima Vaisburd wrote:
> On 2016/03/17 20:24:07, sandersd wrote:
> > The task_runner_ is discarded here. If this works (ie. the constructor
thread
> is
> > the correct thread), why pass a task runner at all?
> > 
> > If this is a bug, another alternative is to require the caller to bind the
> > callbacks to the correct loop themselves. Do we have a reason to pick one
> > solution over the other?
> 
> This is a bug, thank you.
> I meant to use the task_runner from ctor that originates in
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/renderers/de...
> 
> I think we want to respect this parameter?

Yes, that's the media task runner we have. All our decoders should run on it.

[xhwang, 2016-03-17 20:54:20]

Just to make it clearer, the Renderer (along with it's decoders) are created on
the main thread (note that MediaInterfaceProvider is single threaded). But the
Renderer and decoders are used on the |task_runner_| which is our case is the
media thread on the renderer process.

With that lgtm++

[sandersd (OOO until July 31), 2016-03-17 20:56:21]

lgtm

https://codereview.chromium.org/1797393007/diff/20001/media/base/decoder_fact...
File media/base/decoder_factory.h (right):

https://codereview.chromium.org/1797393007/diff/20001/media/base/decoder_fact...
media/base/decoder_factory.h:27: 
Please document what the task runner is for.

[Tima Vaisburd, 2016-03-17 21:07:45]

Description was changed from

==========
Connect MojoAudioDecoder to the service

MojoAudioDecoder is the client side part of the decoder
that runs in the renderer process.

This CL connects it to the remote service and propages
the media task runner to it.

BUG=542910
==========

to

==========
Connect MojoAudioDecoder to the service

MojoAudioDecoder is the client side part of the decoder
that runs in the renderer process.

This CL connects it to the remote service and propagates
the media task runner to it.

BUG=542910
==========

[Tima Vaisburd, 2016-03-17 21:08:34]

https://codereview.chromium.org/1797393007/diff/20001/media/base/decoder_fact...
File media/base/decoder_factory.h (right):

https://codereview.chromium.org/1797393007/diff/20001/media/base/decoder_fact...
media/base/decoder_factory.h:27: 
On 2016/03/17 20:56:20, sandersd wrote:
> Please document what the task runner is for.

Done.

[Tima Vaisburd, 2016-03-18 01:04:03]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2016-03-18 01:04:03]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1797393007/#ps60001
(title: "Created AudioDecoder in MediaInterfaceProvider")

[commit-bot: I haz the power, 2016-03-18 01:04:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1797393007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1797393007/60001

[commit-bot: I haz the power, 2016-03-18 01:14:46]

Description was changed from

==========
Connect MojoAudioDecoder to the service

MojoAudioDecoder is the client side part of the decoder
that runs in the renderer process.

This CL connects it to the remote service and propagates
the media task runner to it.

BUG=542910
==========

to

==========
Connect MojoAudioDecoder to the service

MojoAudioDecoder is the client side part of the decoder
that runs in the renderer process.

This CL connects it to the remote service and propagates
the media task runner to it.

BUG=542910
==========

[commit-bot: I haz the power, 2016-03-18 01:14:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-18 01:15:50]

Description was changed from

==========
Connect MojoAudioDecoder to the service

MojoAudioDecoder is the client side part of the decoder
that runs in the renderer process.

This CL connects it to the remote service and propagates
the media task runner to it.

BUG=542910
==========

to

==========
Connect MojoAudioDecoder to the service

MojoAudioDecoder is the client side part of the decoder
that runs in the renderer process.

This CL connects it to the remote service and propagates
the media task runner to it.

BUG=542910

Committed: https://crrev.com/fe54753dafe23379ba6ec9d20d08d1a32b9ecc13
Cr-Commit-Position: refs/heads/master@{#381861}
==========

[commit-bot: I haz the power, 2016-03-18 01:15:51]

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