[Chromecast] Add explicit task_runner in CMA test

[Chromecast] Use BindToCurrentLoop in CMA test

Without this the callback posts tasks on whatever thread it's callback
was originally called on, which can cause it to not respect the CMA
pipeline's threading assumptions.

Committed: https://crrev.com/9104732746ce7dd18dee6308581e8e7327bb99f1
Cr-Commit-Position: refs/heads/master@{#341043}

[cleichner, 2015-07-29 20:57:13]

cleichner@chromium.org changed reviewers:
+ erickung@google.com, kmackay@chromium.org

[kmackay, 2015-07-29 21:09:20]

Assuming Feed() is always called on the correct thread, you can just use
media::BindToCurrentLoop() for the callback passed to PushFrame() to have the
same effect.

[cleichner, 2015-07-29 22:51:40]

On 2015/07/29 22:51:32, cleichner wrote:

Done

[kmackay, 2015-07-29 22:53:07]

lgtm

[cleichner, 2015-07-29 22:53:24]

The CQ bit was checked by cleichner@chromium.org

[commit-bot: I haz the power, 2015-07-29 22:54:50]

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

[commit-bot: I haz the power, 2015-07-29 22:54:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-29 22:54:53]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[cleichner, 2015-07-29 22:56:03]

cleichner@chromium.org changed reviewers:
+ lcwu@google.com

[cleichner, 2015-07-29 23:28:51]

The CQ bit was checked by cleichner@chromium.org

[cleichner, 2015-07-29 23:28:53]

The CQ bit was unchecked by cleichner@chromium.org

[cleichner, 2015-07-29 23:31:25]

cleichner@chromium.org changed reviewers:
+ halliwell@chromium.org

[halliwell, 2015-07-30 00:15:05]

On 2015/07/29 22:56:09, cleichner wrote:

Is there a bug link for this?  And could you elaborate more on the specifics of
the threading assumptions that are broken?

It looks like we call PushFrame here -> backend -> callback into OnFramePushed
-> posts task which calls PushFrame.

So where's the problem?  Is this being run with a backend that makes its
callback on a different thread than the one it was called on?  i.e. is
OnFramePushed being called on a different thread than the one we called
PushFrame on?


Main reason I'm asking is that I've rewritten the threading / task posting code
in my other CL, and it touches this file, so I want to make sure I don't
immediately re-break things :)

[cleichner, 2015-07-30 00:41:35]

On 2015/07/30 00:15:05, halliwell wrote:
> On 2015/07/29 22:56:09, cleichner wrote:
> 
> Is there a bug link for this?  And could you elaborate more on the specifics
of
> the threading assumptions that are broken?
> 
> It looks like we call PushFrame here -> backend -> callback into OnFramePushed
> -> posts task which calls PushFrame.
> 
> So where's the problem?  Is this being run with a backend that makes its
> callback on a different thread than the one it was called on?  i.e. is
> OnFramePushed being called on a different thread than the one we called
> PushFrame on?
> 
> 
> Main reason I'm asking is that I've rewritten the threading / task posting
code
> in my other CL, and it touches this file, so I want to make sure I don't
> immediately re-break things :)

In an internal CL I've added you to as FYI I refactor the ALSA-based CMA
pipeline such that all of the interaction without ALSA is done on another thread
with a single object encapsulating all iteration with the ALSA subsystem and the
rendering thread. In order to communicate between this thread/object and the
AudioPipelineDeviceAlsa the renderer has a Delegate class which
AudioPipelineDeviceAlsa implements. The delegate methods are called on the
renderer's thread. Since MediaComponentDeviceFeederForTest::OnFramePushed uses
ThreadTaskRunnerHandle::Get(), it grabs the renderer thread instead of the
thread that Feed runs on. This in turn calls AudioPipelineDevice::PushFrame on
the incorrect thread which causes the threading-related DCHECKs in
AudioPipelineDevice to fire and then the test crashes. 

This CL corrects the problem by binding callback passed to PushFrame to the
current loop before it is pushed, so that when it runs, it runs on the correct
message loop.

[cleichner, 2015-07-30 00:41:35]

On 2015/07/30 00:15:05, halliwell wrote:
> On 2015/07/29 22:56:09, cleichner wrote:
> 
> Is there a bug link for this?  And could you elaborate more on the specifics
of
> the threading assumptions that are broken?
> 
> It looks like we call PushFrame here -> backend -> callback into OnFramePushed
> -> posts task which calls PushFrame.
> 
> So where's the problem?  Is this being run with a backend that makes its
> callback on a different thread than the one it was called on?  i.e. is
> OnFramePushed being called on a different thread than the one we called
> PushFrame on?
> 
> 
> Main reason I'm asking is that I've rewritten the threading / task posting
code
> in my other CL, and it touches this file, so I want to make sure I don't
> immediately re-break things :)

In an internal CL I've added you to as FYI I refactor the ALSA-based CMA
pipeline such that all of the interaction without ALSA is done on another thread
with a single object encapsulating all iteration with the ALSA subsystem and the
rendering thread. In order to communicate between this thread/object and the
AudioPipelineDeviceAlsa the renderer has a Delegate class which
AudioPipelineDeviceAlsa implements. The delegate methods are called on the
renderer's thread. Since MediaComponentDeviceFeederForTest::OnFramePushed uses
ThreadTaskRunnerHandle::Get(), it grabs the renderer thread instead of the
thread that Feed runs on. This in turn calls AudioPipelineDevice::PushFrame on
the incorrect thread which causes the threading-related DCHECKs in
AudioPipelineDevice to fire and then the test crashes. 

This CL corrects the problem by binding callback passed to PushFrame to the
current loop before it is pushed, so that when it runs, it runs on the correct
message loop.

[lcwu1, 2015-07-30 00:52:35]

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

[lcwu1, 2015-07-30 00:52:35]

lgtm

[cleichner, 2015-07-30 00:55:03]

The CQ bit was checked by cleichner@chromium.org

[commit-bot: I haz the power, 2015-07-30 00:55:31]

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

[commit-bot: I haz the power, 2015-07-30 02:15:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-30 02:15:50]

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

[halliwell, 2015-07-30 02:37:33]

On 2015/07/30 00:41:35, cleichner wrote:
> On 2015/07/30 00:15:05, halliwell wrote:
> > On 2015/07/29 22:56:09, cleichner wrote:
> > 
> > Is there a bug link for this?  And could you elaborate more on the specifics
> of
> > the threading assumptions that are broken?
> > 
> > It looks like we call PushFrame here -> backend -> callback into
OnFramePushed
> > -> posts task which calls PushFrame.
> > 
> > So where's the problem?  Is this being run with a backend that makes its
> > callback on a different thread than the one it was called on?  i.e. is
> > OnFramePushed being called on a different thread than the one we called
> > PushFrame on?
> > 
> > 
> > Main reason I'm asking is that I've rewritten the threading / task posting
> code
> > in my other CL, and it touches this file, so I want to make sure I don't
> > immediately re-break things :)
> 
> In an internal CL I've added you to as FYI I refactor the ALSA-based CMA
> pipeline such that all of the interaction without ALSA is done on another
thread
> with a single object encapsulating all iteration with the ALSA subsystem and
the
> rendering thread. In order to communicate between this thread/object and the
> AudioPipelineDeviceAlsa the renderer has a Delegate class which
> AudioPipelineDeviceAlsa implements. The delegate methods are called on the
> renderer's thread. Since MediaComponentDeviceFeederForTest::OnFramePushed uses
> ThreadTaskRunnerHandle::Get(), it grabs the renderer thread instead of the
> thread that Feed runs on. This in turn calls AudioPipelineDevice::PushFrame on
> the incorrect thread which causes the threading-related DCHECKs in
> AudioPipelineDevice to fire and then the test crashes. 
> 
> This CL corrects the problem by binding callback passed to PushFrame to the
> current loop before it is pushed, so that when it runs, it runs on the correct
> message loop.

Thanks Chas, that's what I was guessing, just wanted to be sure.

Good news is my incoming CL fixes this also ... I'm assuming that vendors might
make these callbacks on arbitrary threads depending on their implementation
details, so we now post to the right thread internally and don't have this
concern.

[cleichner, 2015-07-30 18:43:49]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1254223003/ by cleichner@chromium.org.

The reason for reverting is: This was actually necessary because of a bug in
which I incorrectly used weak_ptr in the internal change. That problem was
caught in code review so this is no longer necessary..