Rejigger audio capture pipeline to work with separate main+worker threads (Mac).

Rejigger audio capture pipeline to work with separate main+worker threads (Mac).

The audio capture pipeline (specifically: WebContentsAudioInputStream, VirtualAudio[In|Out]putStream, and FakeAudioConsumer) was implemented under the assumption that the thread pumping audio data was the same as the thread invoking Open/Start/Stop/Close.  This assumption was correct until SVN rev 204130 (https://codereview.chromium.org/14273018/), where these tasks were split across two separate threads.

This change restores functionality by utilizing the same design assumptions as the normal, "non-virtual" input/output stream implementations.  Data pumping is assumed to occur by one thread, and the implementation must block the control thread on a call to Stop() and not return from Stop() until the data pumping has ceased.

Augmented VirtualAudioInputStream unit testing to run all tests under the one-thread (non-Mac platforms) versus two-thread (Mac only) scenario.

BUG=249089
TEST=media_unittests, content_unittests, manual confirmation

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207105

[miu, 2013-06-15 04:38:17]

Dale,

Please review Patch Set 2 as a candidate for commit.

Patch Set 1 is what we talked about on Wednesday.  However, using
base::WaitableEvent is forbidden on the UI thread (a debug build triggers
base::ThreadRestrictions)  Even though we discussed it should be acceptable to
block the UI thread for that short amount of time, the existing code
infrastructure forbids it.

Therefore, I had to do a bit more of an invasive change to FakeAudioConsumer
(Patch Set 2).  I followed a design pattern we've already used elsewhere (e.g.,
WebContentsAudioInputStream).  Meaning, I took the meat of the implementation
and shoved it into a ref-counted "Worker" class.  The ref-counting was necessary
since it must live long enough for tasks posted to the worker message loop to be
canceled after the call to FakeAudioConsumer::Stop() on the main thread.  In
other words, the main thread could delete FakeAudioConsumer before the worker
thread executes the cancellation task.

Thanks,
Yuri

[miu, 2013-06-17 19:33:55]

Hey Dale,

Hold off on the review.  Or, go ahead and review, but note I'm going to try
replacing the ref-counting of FakeAudioConsumer::Worker with a scoped_ptr<> +
DeleteSoon() scheme.  I'll send another message once that's done.

Thanks,
Yuri

[DaleCurtis, 2013-06-17 19:50:36]

FYI, there's base::ThreadRestrictions which has a ScopedAllowWait() method, we
just need to talk to jam@ first.

[DaleCurtis, 2013-06-17 19:51:21]

Also, feel free to iterate, but remember that if dev channel still shows crashes
i'll be reverting the UI thread change.  Should know for sure within the next
couple days.

[miu, 2013-06-18 01:43:05]

On 2013/06/17 19:51:21, DaleCurtis wrote:
> Also, feel free to iterate, but remember that if dev channel still shows
crashes
> i'll be reverting the UI thread change.  Should know for sure within the next
> couple days.

BTW--Forgot to mention: Credit for this suggestion (replacing scoped_ptr<> with
DeleteSoon) goes to wez@.

I'd prefer we proceed anyway, if that's okay with you.  Regardless of the
"speculative crash fix" you're trying out, there have been tab capture bug
reports from canary users that I think are probably related to these threading
problems.  It'd make me feel a bit reassured if those bug reports went away with
this change.  (That way, I can focus on other bugs and maybe get a few more
squashed before the M29 branch.)

Thanks,
Yuri

[miu, 2013-06-18 01:47:51]

On 2013/06/18 01:43:05, Yuri wrote:
> I'd prefer we proceed anyway, if that's okay with you.  Regardless of the
> "speculative crash fix" you're trying out, there have been tab capture bug
> reports from canary users that I think are probably related to these threading
> problems.

Example: http://crbug.com/249795 (it's a Pri-1 crash bug)

[DaleCurtis, 2013-06-18 19:27:19]

lgtm

[miu, 2013-06-18 20:05:48]

On 2013/06/18 19:27:19, DaleCurtis wrote:
> lgtm

Thanks!  :)

[commit-bot: I haz the power, 2013-06-18 20:06:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/17122006/20001

[commit-bot: I haz the power, 2013-06-18 23:10:59]

Change committed as 207105