Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(375)

Issue 89663004: Don't start WASAPI render thread until setup completes. (Closed)

Created:
7 years ago by DaleCurtis
Modified:
7 years ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, tommi (sloooow) - chröme
Visibility:
Public.

Description

Don't start WASAPI render thread until setup completes. Creating the render thread before setting up the |audio_client_| may lead to race conditions when repeatedly starting and stopping a stream. With this patch, nothing touches |audio_client_| until IAudioClient triggers the render event for data. As a bonus, it fixes a destruction issue where |audio_clock| was being destructed on the wrong thread (render vs audio), see docs: http://msdn.microsoft.com/en-us/library/windows/desktop/dd370873.aspx BUG=310838 TEST=none Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237694

Patch Set 1 : Fixes. #

Patch Set 2 : Add IAudioClock. #

Total comments: 2

Patch Set 3 : Cleanup member variables. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -36 lines) Patch
M media/audio/win/audio_low_latency_output_win.h View 1 2 4 chunks +3 lines, -7 lines 0 comments Download
M media/audio/win/audio_low_latency_output_win.cc View 1 2 9 chunks +29 lines, -29 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
DaleCurtis
https://codereview.chromium.org/89663004/diff/80001/media/audio/win/audio_low_latency_output_win.cc File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/89663004/diff/80001/media/audio/win/audio_low_latency_output_win.cc#newcode388 media/audio/win/audio_low_latency_output_win.cc:388: hr = audio_clock_->GetFrequency(&device_frequency); I left this here, but if ...
7 years ago (2013-11-27 02:45:08 UTC) #1
tommi (sloooow) - chröme
Great! Lgtm
7 years ago (2013-11-27 04:33:39 UTC) #2
henrika (OOO until Aug 14)
Thanks for helping out doing this work (which I should have done ;-)). The clock ...
7 years ago (2013-11-27 08:24:38 UTC) #3
DaleCurtis
Thanks guys! While looking for any other ScopedComPtr dangers, I also noticed some member variables ...
7 years ago (2013-11-27 19:21:44 UTC) #4
henrika (OOO until Aug 14)
LGTM++
7 years ago (2013-11-27 19:23:56 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/89663004/100001
7 years ago (2013-11-27 19:30:46 UTC) #6
tommi (sloooow) - chröme
lgtm. nice! 1 extra citizenship point for each variable.
7 years ago (2013-11-27 19:38:42 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/89663004/100001
7 years ago (2013-11-28 01:43:02 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/89663004/100001
7 years ago (2013-11-28 02:10:54 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/89663004/100001
7 years ago (2013-11-28 02:42:08 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/89663004/100001
7 years ago (2013-11-28 03:09:58 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/89663004/100001
7 years ago (2013-11-28 03:36:17 UTC) #12
commit-bot: I haz the power
7 years ago (2013-11-28 05:52:00 UTC) #13
Message was sent while issue was closed.
Change committed as 237694

Powered by Google App Engine
This is Rietveld 408576698