Switch to STA mode for audio thread and WASAPI I/O streams.

Switch to STA mode for audio thread and WASAPI I/O streams.

Using COM marshalling to share necessary worker thread pointers
with the render and capture threads.

BUG=422522
TEST=all unit tests pass, audio input and output work.

Committed: https://crrev.com/43f3b0d25752d924390a4d6fc868d6e9469edc4f
Cr-Commit-Position: refs/heads/master@{#326360}

[DaleCurtis, 2015-04-16 21:33:04]

dalecurtis@chromium.org changed reviewers:
+ henrika@chromium.org, tommi@chromium.org

[DaleCurtis, 2015-04-16 21:33:05]

Per discussion via chat with Tommi, lets see if this doesn't blow up the world
:O

[DaleCurtis, 2015-04-16 21:37:21]

Patchset #2 (id:20001) has been deleted

[DaleCurtis, 2015-04-16 21:44:55]

Patchset #2 (id:40001) has been deleted

[DaleCurtis, 2015-04-17 01:23:50]

dalecurtis@chromium.org changed reviewers:
+ mcasas@chromium.org

[DaleCurtis, 2015-04-17 01:23:51]

+mcasas since I needed to revert the MediaStreamManager changes to avoid race
conditions in the AudioManager device enumeration startup.

Unfortunately this seems to break the AudioInputVolume test, one of the input
devices fails to open if that test is run after an input device has previously
been opened:

[1672:21592:0416/182157:2342764483:ERROR:audio_low_latency_input_win.cc(690)]
Failed to initalize IAudioClient: 8889000a : : The requested lookup key was not
found in any active activation context. (0x36B7)

Still trying to figure out why :( Also seeing if a less invasive change to
AudioManager will work...

[DaleCurtis, 2015-04-17 02:20:29]

Actually, I think that failure may be the test having too high of expectations.
The input device it was trying to open is listed as "currently unavailable" in
windows; though not disabled.

I'm not sure why it works sometimes and not, but it does seem like Open() can
legitimately fail for these devices in that state. I've changed the test to
reflect this; let me know if you feel that's an error.

Fingers crossed this passes the try bots! If you have some time, please check it
out locally on your end as well.

[henrika (OOO until Aug 14), 2015-04-17 09:16:46]

Wow, impressive work. I will run tests locally on my machine and report back.

LGTM % (the fact that I rely on Mr tommi@ for more detailed feedback)

Thanks Dale.

[henrika (OOO until Aug 14), 2015-04-17 09:17:03]

https://codereview.chromium.org/1097553003/diff/120001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1097553003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:86: // Do not use MTA mode initalization as it
causes hangs for a significant
are we certain that is does?

[henrika (OOO until Aug 14), 2015-04-17 10:33:33]

Regarding the AudioInputVolume test. I could make it timeout without your patch.
It failed to open a mic connected to a webcam. When I unplugged and restored the
device things were OK again and the test passed.

The test passed with your patch as well. I repeated the test 100 times.

Also tried Chrome with your patch and it worked well.

[mcasas, 2015-04-17 17:15:56]

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1670: // buggy third
party Direct Show modules, http://crbug.com/428958.
I would like to keep this indication of "buggy third party..."
and the bug, somehow. Other than that the change LGTM, 
the gist is to make the Capture Thread STA.

There's the situation that Video Capture hijacks the
Audio Capture thread, pretending it's Capture Thread.
Creation of this thread happens inside AudioManagerBase.
I'm not familiar with the arcanes of the audio side, but
I think it would be best created in MediaStreamManager, 
specially in the light of this thread getting more and more
workload in the near future. WDYT?

[DaleCurtis, 2015-04-17 17:58:32]

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1670: // buggy third
party Direct Show modules, http://crbug.com/428958.
On 2015/04/17 17:15:56, mcasas wrote:
> I would like to keep this indication of "buggy third party..."
> and the bug, somehow. Other than that the change LGTM, 
> the gist is to make the Capture Thread STA.
> 
> There's the situation that Video Capture hijacks the
> Audio Capture thread, pretending it's Capture Thread.
> Creation of this thread happens inside AudioManagerBase.
> I'm not familiar with the arcanes of the audio side, but
> I think it would be best created in MediaStreamManager, 
> specially in the light of this thread getting more and more
> workload in the near future. WDYT? 

Where would you prefer this text goes? I've added similar text in
AudioManagerBase when the audio thread is created.

Notably, the audio capture thread is just a control thread (start/stop/etc),
actual capture happens on another OS level thread.

[DaleCurtis, 2015-04-17 17:58:55]

https://codereview.chromium.org/1097553003/diff/120001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1097553003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:86: // Do not use MTA mode initalization as it
causes hangs for a significant
On 2015/04/17 09:17:03, henrika wrote:
> are we certain that is does?

If it doesn't I'll be reverting this CL :)

[DaleCurtis, 2015-04-17 19:01:55]

While writing this CL I wondered if ScopedComPtr should use a TheadChecker when
receiving objects created on an STA thread. WDYT tommi?

[mcasas, 2015-04-17 20:23:31]

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1670: // buggy third
party Direct Show modules, http://crbug.com/428958.
On 2015/04/17 17:58:32, DaleCurtis wrote:
> On 2015/04/17 17:15:56, mcasas wrote:
> > I would like to keep this indication of "buggy third party..."
> > and the bug, somehow. Other than that the change LGTM, 
> > the gist is to make the Capture Thread STA.
> > 
> > There's the situation that Video Capture hijacks the
> > Audio Capture thread, pretending it's Capture Thread.
> > Creation of this thread happens inside AudioManagerBase.
> > I'm not familiar with the arcanes of the audio side, but
> > I think it would be best created in MediaStreamManager, 
> > specially in the light of this thread getting more and more
> > workload in the near future. WDYT? 
> 
> Where would you prefer this text goes? I've added similar text in
> AudioManagerBase when the audio thread is created.
> 
> Notably, the audio capture thread is just a control thread (start/stop/etc),
> actual capture happens on another OS level thread.

Perhaps add them next to l.1676 of this side of the diff?
Something explaining that |device_task_runner_| must be
STA in Win blabla. Maybe add a DCHECK for OS_WIN?
(don't know if that's possible)

[DaleCurtis, 2015-04-18 01:40:19]

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1670: // buggy third
party Direct Show modules, http://crbug.com/428958.
On 2015/04/17 20:23:31, mcasas wrote:
> On 2015/04/17 17:58:32, DaleCurtis wrote:
> > On 2015/04/17 17:15:56, mcasas wrote:
> > > I would like to keep this indication of "buggy third party..."
> > > and the bug, somehow. Other than that the change LGTM, 
> > > the gist is to make the Capture Thread STA.
> > > 
> > > There's the situation that Video Capture hijacks the
> > > Audio Capture thread, pretending it's Capture Thread.
> > > Creation of this thread happens inside AudioManagerBase.
> > > I'm not familiar with the arcanes of the audio side, but
> > > I think it would be best created in MediaStreamManager, 
> > > specially in the light of this thread getting more and more
> > > workload in the near future. WDYT? 
> > 
> > Where would you prefer this text goes? I've added similar text in
> > AudioManagerBase when the audio thread is created.
> > 
> > Notably, the audio capture thread is just a control thread (start/stop/etc),
> > actual capture happens on another OS level thread.
> 
> Perhaps add them next to l.1676 of this side of the diff?
> Something explaining that |device_task_runner_| must be
> STA in Win blabla. Maybe add a DCHECK for OS_WIN?
> (don't know if that's possible)

Didn't make sense here, so I put it up where device_task_runner_ is set. It
looks like there's only a way to query the apartment type on Windows 7 or higher
unfortunately:

https://msdn.microsoft.com/en-us/library/windows/desktop/dd542641.aspx

[DaleCurtis, 2015-04-18 02:36:48]

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/1097553003/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1670: // buggy third
party Direct Show modules, http://crbug.com/428958.
On 2015/04/18 01:40:19, DaleCurtis wrote:
> On 2015/04/17 20:23:31, mcasas wrote:
> > On 2015/04/17 17:58:32, DaleCurtis wrote:
> > > On 2015/04/17 17:15:56, mcasas wrote:
> > > > I would like to keep this indication of "buggy third party..."
> > > > and the bug, somehow. Other than that the change LGTM, 
> > > > the gist is to make the Capture Thread STA.
> > > > 
> > > > There's the situation that Video Capture hijacks the
> > > > Audio Capture thread, pretending it's Capture Thread.
> > > > Creation of this thread happens inside AudioManagerBase.
> > > > I'm not familiar with the arcanes of the audio side, but
> > > > I think it would be best created in MediaStreamManager, 
> > > > specially in the light of this thread getting more and more
> > > > workload in the near future. WDYT? 
> > > 
> > > Where would you prefer this text goes? I've added similar text in
> > > AudioManagerBase when the audio thread is created.
> > > 
> > > Notably, the audio capture thread is just a control thread
(start/stop/etc),
> > > actual capture happens on another OS level thread.
> > 
> > Perhaps add them next to l.1676 of this side of the diff?
> > Something explaining that |device_task_runner_| must be
> > STA in Win blabla. Maybe add a DCHECK for OS_WIN?
> > (don't know if that's possible)
> 
> Didn't make sense here, so I put it up where device_task_runner_ is set. It
> looks like there's only a way to query the apartment type on Windows 7 or
higher
> unfortunately:
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd542641.aspx

Actually, I misread that, it looks like there's a multistep way to get that info
via casting IUnknown somehow. Not sure it's worth it.

[DaleCurtis, 2015-04-20 16:32:30]

tommi: Ping? Let me know if you're too busy digging out of your mailbox and I'll
see if one of our Windows folk can take a look.

[tommi (sloooow) - chröme, 2015-04-20 18:23:38]

Hey Dale - sorry for the delay.  Here's my first set of comments.

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.h (right):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.h:135: bool MarshalComPointers();
Just checking... do these methods dcheck that they're called on the expected
thread?

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:73: com_stream_(NULL),
is this a COM pointer?  If so, use ScopedComPtr

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:271: // TODO(dalecurtis): Do we
need a lock on this with STA mode?
One thing to be aware of with STA is reentrancy.  I don't know if you have that
problem here, but it is possible in some situations to end up where you started
so to speak, on the same thread and repeat a call that's already in progress. 
Just fyi.

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:287: // TODO(dalecurtis): Do we
need a lock on this with STA mode?
if a lock wasn't needed before, I don't think it's needed now

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:364: IAudioClient*
thread_audio_client = NULL;
use ScopedComPtr instead of raw pointers?  actually... where are all these
released?

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.h (right):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.h:241: IStream* com_stream_;
ScopedComPtr

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_manager_win.cc (left):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_manager_win.cc:137:
enumeration_type_(CoreAudioUtil::IsSupported() ?
Is it safe to remove this call?  The comment explains why it's necessary to call
IsSupported here, so perhaps you've removed that requirement now?

[DaleCurtis, 2015-04-20 18:54:07]

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.h (right):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.h:135: bool MarshalComPointers();
On 2015/04/20 18:23:37, tommi wrote:
> Just checking... do these methods dcheck that they're called on the expected
> thread?

I'll see if I can add them, I speculatively didn't since I remember the unit
tests calling from the wrong thread.

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:73: com_stream_(NULL),
On 2015/04/20 18:23:38, tommi wrote:
> is this a COM pointer?  If so, use ScopedComPtr

I think so, but it's seemingly manually released via the UnmarshalMethod, I'll
see about wrapping it in a ScopedComPtr for exceptional cases and detach it from
the ScopedComPtr before the Unmarshal methods release it.

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:287: // TODO(dalecurtis): Do we
need a lock on this with STA mode?
On 2015/04/20 18:23:38, tommi wrote:
> if a lock wasn't needed before, I don't think it's needed now

Hmm, previously we were talking to the same instance of the object in MTA mode,
which I would have assumed meant that it knew to do locking as necessary.

Now we're talking to two different objects, one of which is a proxy to the main
object it seems. Is it smart enough to know it needs a lock under the hood in
that case?

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:364: IAudioClient*
thread_audio_client = NULL;
On 2015/04/20 18:23:38, tommi wrote:
> use ScopedComPtr instead of raw pointers?  actually... where are all these
> released?

I have no idea what I'm doing here :) Marshal tutorials seem to suggest this is
handled as part of the Marshal/Unmarshal process, but in hindsight this seems
unlikely. Looking at the docs I don't see anything about releasing these
pointers:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms693382.aspx

I'll switch them over to ScopedComPtrs and see if everything still works
correctly.

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_manager_win.cc (left):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_manager_win.cc:137:
enumeration_type_(CoreAudioUtil::IsSupported() ?
On 2015/04/20 18:23:38, tommi wrote:
> Is it safe to remove this call?  The comment explains why it's necessary to
call
> IsSupported here, so perhaps you've removed that requirement now?

I've moved video capture enumeration back to the audio thread which should be
the only caller into the audio manager this early.  I've also added a
kNoEnumeration state + DCHECK() to ensure no one is calling it.

That said, I just realized that this comment is talking about the IsSupported()
method itself and not the setup of AudioManager. Looking through the calls, the
problematic one looks like SystemMessageWindowWin() which happens slightly after
AudioManager initialization:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/sy...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

Or possibly even Ash initialization if the AudioManager thread is delayed
enough:

https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/tray/sy...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I don't know if we should be initializing COM / changing the COM model for the
UI thread in these spots... Is there harm in that? Certainly it means we won't
have COM initialization until it's retried later on the audio thread... probably
this explains the mass crashes that we had previously here.

I think what we want to do is switch to a LazyInitializer in
CoreAudioUtil::IsSupported() and have it block until initialization completes on
the audio thread. All subsequent calls will use the cached value w/o blocking.
WDYT?

[DaleCurtis, 2015-04-20 23:06:10]

Leaving the CoreAudio initializer where it is seems the only sane approach right
now.  I still worry that we'll try to reinitialize MTA on an STA thread... It'd
be nice if there an easy way to DCHECK that.

As such I reverted my VideoCaptureDevice manager changes. Those can be done in a
separate CL.  PTAL.

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_manager_win.cc (left):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_manager_win.cc:137:
enumeration_type_(CoreAudioUtil::IsSupported() ?
On 2015/04/20 18:54:07, DaleCurtis wrote:
> On 2015/04/20 18:23:38, tommi wrote:
> > Is it safe to remove this call?  The comment explains why it's necessary to
> call
> > IsSupported here, so perhaps you've removed that requirement now?
> 
> I've moved video capture enumeration back to the audio thread which should be
> the only caller into the audio manager this early.  I've also added a
> kNoEnumeration state + DCHECK() to ensure no one is calling it.
> 
> That said, I just realized that this comment is talking about the
IsSupported()
> method itself and not the setup of AudioManager. Looking through the calls,
the
> problematic one looks like SystemMessageWindowWin() which happens slightly
after
> AudioManager initialization:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/sy...
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
> 
> Or possibly even Ash initialization if the AudioManager thread is delayed
> enough:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/tray/sy...
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> I don't know if we should be initializing COM / changing the COM model for the
> UI thread in these spots... Is there harm in that? Certainly it means we won't
> have COM initialization until it's retried later on the audio thread...
probably
> this explains the mass crashes that we had previously here.
> 
> I think what we want to do is switch to a LazyInitializer in
> CoreAudioUtil::IsSupported() and have it block until initialization completes
on
> the audio thread. All subsequent calls will use the cached value w/o blocking.
> WDYT?

Hmm, things are worse than I thought here, TrayAudioDelegateWin() is actually
creating IMMDevice, ISimpleAudioVolume, and IAudioSessionManager objects on the
UI thread. These calls would need to trampoline to the audio thread if we want
to enforce calls to the audio thread.

Instead, I checked into the browser threads and it seems they're all initialized
with STA mode, so I think it's fine to let these calls go through. It does mean
keeping the AudioManagerWin constructor as is, so I've reverted all of those
changes and left only the STA changes for the WASAPI drivers.

[DaleCurtis, 2015-04-22 03:31:13]

tommi: Can you take a look today? Thanks!

[tommi (sloooow) - chröme, 2015-04-22 10:31:52]

hey dale - can you ping me if you get through these set of comments today?  I
can do another round today if you'd like. I don't want to hold this up too long

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:54: com_stream_(NULL),
necessary?

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:691: << " : ";
is this line left over from some previous code? (or does plog add the error
information after the message?... I don't remember)

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:799: if (FAILED(hr))
Should we have a DCHECK here that hr either succeeded or that com_stream_ is
null?  If we don't pass that test, we leak the stream.
Hmm... what about having a DCHECK(com_stream_) at the top of the function and a
DCHECK(SUCCEEDED(hr)) here?  Something really unexpected would have had to
happen for this to fail (I think), so DLOG() might be too soft.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.h (right):

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.h:134: // Handles sharing of COM
pointers between the audio and capture threads.
maybe add a note about thread requirements? (these methods need to be called on
a specific thread, right?)

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:429: IAudioClient*
thread_audio_client,
nit: Does the 'thread_' prefix add context?  I think we can just skip it, but
it's up to you.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:648: bool
WASAPIAudioOutputStream::MarshalComPointers() {
Can we add a thread checker for these methods?  There's no lock etc for
com_stream_ etc, so it would be nice to have that.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:649: HRESULT hr =
CreateStreamOnHGlobal(NULL, TRUE, com_stream_.Receive());
what about using a local variable for the stream here and assign to com_stream_
at the end of this function if all ends well?  Otherwise I worry about the risk
of not releasing com_stream_ for some early return that might get added down the
line and possibly cause subtle errors.  Same for UnmarshalComPointers.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:655: hr =
CoMarshalInterface(com_stream_.get(), __uuidof(IAudioClient),
was there a particular reason you decided to go with CoMarshalInterface and not
the GIT? (IGlobalInterfaceTable)

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:697: HRESULT hr =
CoUnmarshalInterface(com_stream_.get(), __uuidof(IAudioClient),
nit: would be nice to detach com_stream_ here to a local variable and skip
calling com_stream_.Release() at every return point:

ScopedComPtr<IStream> stream(com_stream_.Detach());

[DaleCurtis, 2015-04-22 16:08:24]

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1097553003/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:287: // TODO(dalecurtis): Do we
need a lock on this with STA mode?
On 2015/04/20 18:54:07, DaleCurtis wrote:
> On 2015/04/20 18:23:38, tommi wrote:
> > if a lock wasn't needed before, I don't think it's needed now
> 
> Hmm, previously we were talking to the same instance of the object in MTA
mode,
> which I would have assumed meant that it knew to do locking as necessary.
> 
> Now we're talking to two different objects, one of which is a proxy to the
main
> object it seems. Is it smart enough to know it needs a lock under the hood in
> that case?

Can you add some more details here?

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:648: bool
WASAPIAudioOutputStream::MarshalComPointers() {
On 2015/04/22 10:31:52, tommi wrote:
> Can we add a thread checker for these methods?  There's no lock etc for
> com_stream_ etc, so it would be nice to have that.

I'll see if I can. I forget if the unit tests try to make any calls directly
into the render/capture threads.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:649: HRESULT hr =
CreateStreamOnHGlobal(NULL, TRUE, com_stream_.Receive());
On 2015/04/22 10:31:52, tommi wrote:
> what about using a local variable for the stream here and assign to
com_stream_
> at the end of this function if all ends well?  Otherwise I worry about the
risk
> of not releasing com_stream_ for some early return that might get added down
the
> line and possibly cause subtle errors.  Same for UnmarshalComPointers.

Good idea, I'll do this.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:655: hr =
CoMarshalInterface(com_stream_.get(), __uuidof(IAudioClient),
On 2015/04/22 10:31:52, tommi wrote:
> was there a particular reason you decided to go with CoMarshalInterface and
not
> the GIT? (IGlobalInterfaceTable)

Yes, but maybe not good ones, as I basically just found a bunch of internet
tutorials.  The CoMarshalInterface method looked like a better scoped transfer
mechanism and examples used far less code.

[DaleCurtis, 2015-04-22 16:09:28]

I'm heading into the office now, but will try to resolve all of these comments
by 11:30AM PST.

[DaleCurtis, 2015-04-22 17:48:54]

Okay, should be good to go. Thanks tommi!

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:54: com_stream_(NULL),
On 2015/04/22 10:31:52, tommi wrote:
> necessary?

Done.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:691: << " : ";
On 2015/04/22 10:31:51, tommi wrote:
> is this line left over from some previous code? (or does plog add the error
> information after the message?... I don't remember)

It was the method that was silently failing during one of my earlier unit test
runs, so I added a log :) PLOG does add the GetLastError() information to the
end of the line.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:799: if (FAILED(hr))
On 2015/04/22 10:31:52, tommi wrote:
> Should we have a DCHECK here that hr either succeeded or that com_stream_ is
> null?  If we don't pass that test, we leak the stream.
> Hmm... what about having a DCHECK(com_stream_) at the top of the function and
a
> DCHECK(SUCCEEDED(hr)) here?  Something really unexpected would have had to
> happen for this to fail (I think), so DLOG() might be too soft.

IStream is always released according to the API docs for this function, I've
added the DCHECK(com_stream_); though.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.h (right):

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.h:134: // Handles sharing of COM
pointers between the audio and capture threads.
On 2015/04/22 10:31:52, tommi wrote:
> maybe add a note about thread requirements? (these methods need to be called
on
> a specific thread, right?)

Done.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:270: // TODO(dalecurtis): Do we
need a lock on this with STA mode?
Tentatively removed this assuming you POV is correct.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:429: IAudioClient*
thread_audio_client,
On 2015/04/22 10:31:52, tommi wrote:
> nit: Does the 'thread_' prefix add context?  I think we can just skip it, but
> it's up to you.

Done.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:648: bool
WASAPIAudioOutputStream::MarshalComPointers() {
On 2015/04/22 16:08:23, DaleCurtis wrote:
> On 2015/04/22 10:31:52, tommi wrote:
> > Can we add a thread checker for these methods?  There's no lock etc for
> > com_stream_ etc, so it would be nice to have that.
> 
> I'll see if I can. I forget if the unit tests try to make any calls directly
> into the render/capture threads.

Done.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:649: HRESULT hr =
CreateStreamOnHGlobal(NULL, TRUE, com_stream_.Receive());
On 2015/04/22 16:08:23, DaleCurtis wrote:
> On 2015/04/22 10:31:52, tommi wrote:
> > what about using a local variable for the stream here and assign to
> com_stream_
> > at the end of this function if all ends well?  Otherwise I worry about the
> risk
> > of not releasing com_stream_ for some early return that might get added down
> the
> > line and possibly cause subtle errors.  Same for UnmarshalComPointers.
> 
> Good idea, I'll do this.

Done.

https://codereview.chromium.org/1097553003/diff/160001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:697: HRESULT hr =
CoUnmarshalInterface(com_stream_.get(), __uuidof(IAudioClient),
On 2015/04/22 10:31:52, tommi wrote:
> nit: would be nice to detach com_stream_ here to a local variable and skip
> calling com_stream_.Release() at every return point:
> 
> ScopedComPtr<IStream> stream(com_stream_.Detach());

Done.

[tommi (sloooow) - chröme, 2015-04-22 18:10:08]

looking pretty good, just some minor comments

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:380:
ScopedComPtr<IAudioCaptureClient> thread_audio_capture_client;
nit: audio_capture_client

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:802: if (FAILED(hr))
DCHECK(SUCCEEDED(hr)) ?

I think this should only fail on non-COM threads and that would probably be a
programmer error.

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:690: bool
WASAPIAudioOutputStream::UnmarshalComPointers(
just throwing this out there... we could implement this function as an all or
nothing thing...

We could unmarshall into local variables and at the end of the function do a
.Pass() to the output parameters.

I also think that the unmarshal operation should never fail under expected
circumstances, so ... dcheck maybe?

[tommi (sloooow) - chröme, 2015-04-22 18:12:19]

lgtm as discussed offline. I'll leave the dchecks up to  you.

[DaleCurtis, 2015-04-22 18:31:47]

Switched to a VOID method with CHECKs(). Thanks for review!

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:380:
ScopedComPtr<IAudioCaptureClient> thread_audio_capture_client;
On 2015/04/22 18:10:08, tommi wrote:
> nit: audio_capture_client

Done.

https://codereview.chromium.org/1097553003/diff/180001/media/audio/win/audio_...
media/audio/win/audio_low_latency_input_win.cc:802: if (FAILED(hr))
On 2015/04/22 18:10:08, tommi wrote:
> DCHECK(SUCCEEDED(hr)) ?
> 
> I think this should only fail on non-COM threads and that would probably be a
> programmer error.

Done.

[DaleCurtis, 2015-04-22 18:31:54]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-22 18:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
henrika@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1097553003/#ps200001
(title: "Fixes.")

[commit-bot: I haz the power, 2015-04-22 18:33:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097553003/200001

[commit-bot: I haz the power, 2015-04-22 19:42:56]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-04-22 19:43:52]

Patchset 9 (id:??) landed as
https://crrev.com/43f3b0d25752d924390a4d6fc868d6e9469edc4f
Cr-Commit-Position: refs/heads/master@{#326360}

[DaleCurtis, 2015-04-27 20:20:33]

A revert of this CL (patchset #9 id:200001) has been created in
https://codereview.chromium.org/1111503003/ by dalecurtis@chromium.org.

The reason for reverting is: Does not seem to have helped anything, crashes are
still present and UMA metrics are unchanged. If anything it may have caused
issues like: http://crbug.com/480476 and http://crbug.com/481581.

[henrika (OOO until Aug 14), 2015-04-28 07:16:01]

;-( LGTM