Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11226057:
Adds Core Audio Utility methods for Windows (Closed)
|
Created:
8 years, 2 months ago by henrika (OOO until Aug 14) Modified:
8 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, tommi (sloooow) - chröme, Chris Rogers Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds new Core Audio utility methods in the media::win namespace.
End goal is to use these methods in a unified audio class implementation for Windows and thereby make the code more readable and at the same time avoid duplicating code.
BUG=154268
TEST=media_unittests.exe --gtest_filter=CoreAudio*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164633
Patch Set 1 #Patch Set 2 : Added more tests and initiated WASAPI support #
Total comments: 4
Patch Set 3 : Removed nested namespace #Patch Set 4 : Improved comments #
Total comments: 81
Patch Set 5 : Fixed unit test based on feedback from Tommi #
Total comments: 6
Patch Set 6 : Changes based on review from Tommi #
Total comments: 11
Patch Set 7 : More feedback from Tommi #
Total comments: 2
Patch Set 8 : Nit #
Messages
Total messages: 18 (0 generated)
Lot's of new code here but I really think we need these parts going forward. I am not asking for detailed comments on all Core Audio specific parts initially. Instead, I would appreciate more generic feedback on things like: - OK to add methods in media::win? - Need more CoreAudio-specific signature? - Basic design patterms OK? Note that most parts of these methods exists already today but is spread out in media/audio/win and we have separate (slightly different) version for input and output. I will add more methods once we have agreed on the style.
https://codereview.chromium.org/11226057/diff/3001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): https://codereview.chromium.org/11226057/diff/3001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:41: namespace win { jam@ has a preference to avoid nested namespaces, preferring class scoping instead. https://codereview.chromium.org/11226057/diff/3001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:44: MEDIA_EXPORT bool CoreAudioIsSupported(); if this returns false will some of the methods below not be callable?
Tommi: added you on the CC list as COM expert. Please take a look from that perspective.
https://codereview.chromium.org/11226057/diff/3001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): https://codereview.chromium.org/11226057/diff/3001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:41: namespace win { I will remove win namespace and modify signatures. Would really like to avoid creating a separate class only to add static members just to emulate a nested namespace. Hope you are OK with the new approach. https://codereview.chromium.org/11226057/diff/3001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:44: MEDIA_EXPORT bool CoreAudioIsSupported(); Good point. Added DCHECKs in all methods.
I am now in a state where it would be best to start using these methods in a real client instead of adding more functions. It is tricky to see what we need to break out without working with the actual integration, hence it would be ideal if I could land this first (with no actual users) and then expand. I do understand that the COM parts are a bit odd but this code has been developed in close cooperation with Tommi who is a real COM expert (I know COM pretty well but I am not an expert). I am also using the ScopedComPtr heavily to ensure that I don't break any COM rules.
https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... File media/audio/win/core_audio_util_win.cc (right): https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:20: #define PRINT(sz, ...) wprintf(sz L"\n", __VA_ARGS__); I'm not seeing this used -- time to remove? https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:45: return (base::win::GetVersion() >= base::win::VERSION_VISTA); as I noted in dalecurtis' http://codereview.chromium.org/11233023/ this method is identical to IsWASAPISupported() can we do w/o duplicating? Perhaps IsWASAPISupported() should be renamed to IsCoreAudioSupported()? according to msdn docs [1], WASAPI is indeed part of the Code Audio API family [1] http://msdn.microsoft.com/en-us/library/windows/desktop/dd370784(v=vs.85).aspx https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... File media/audio/win/core_audio_util_win.h (right): https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:9: #ifndef MEDIA_AUDIO_WIN_CORE_AUDIO_UTIL_WIN_H_ nit: it's your call but we don't have to add a _win.h/cc suffix -- the windows-exclusivity of this file is already covered by being in a win/ subfolder :) https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:28: // --- MMDevice I find this comment is redundant given the one immediately below this one https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:70: // --- WASAPI ditto https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:85: // TODO(henrika): add more methods here... this isn't very descriptive -- I'd either have it filled out or simply remove it as no one other than you knows what to add :)
Thanks Andrew. Dale, any comments? https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... File media/audio/win/core_audio_util_win.cc (right): https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:20: #define PRINT(sz, ...) wprintf(sz L"\n", __VA_ARGS__); Correct. Used while developing. https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.cc:45: return (base::win::GetVersion() >= base::win::VERSION_VISTA); I can make a follow-up patch when this one has landed where I replace IsWASAPI... with this method instead. I have always tried to talk about the different CA APIs (4 in total) but the "lingo" seems to be "CA ~ WASAPI" and I have been kind of forced to adopt it as well. WASAPI is the API where all the actual streaming takes place and I guess it is OK to talk about WASAPI support. Anyhow, I have tried to be more strict in this utility and point out MMDevice and WASAPI parts. To summarize: I can remove the duplicated code as soon as this one has landed. https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... File media/audio/win/core_audio_util_win.h (right): https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:9: #ifndef MEDIA_AUDIO_WIN_CORE_AUDIO_UTIL_WIN_H_ Got it. I actually think that I followed lint here. https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:28: // --- MMDevice Will modify. https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:70: // --- WASAPI Will modify. https://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audi... media/audio/win/core_audio_util_win.h:85: // TODO(henrika): add more methods here... Agree. Will delete.
lgtm though tommi or someone else should confirm the COM stuff more thoroughly. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:22: class ScopedPropertyVariant { Is this a pattern that other people would benefit from? Should it be in base::? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:45: return (base::win::GetVersion() >= base::win::VERSION_VISTA); On 2012/10/24 19:25:44, henrika wrote: > I can make a follow-up patch when this one has landed where I replace > IsWASAPI... with this method instead. > > I have always tried to talk about the different CA APIs (4 in total) but the > "lingo" seems to be "CA ~ WASAPI" and I have been kind of forced to adopt it as > well. WASAPI is the API where all the actual streaming takes place and I guess > it is OK to talk about WASAPI support. > > Anyhow, I have tried to be more strict in this utility and point out MMDevice > and WASAPI parts. > > To summarize: I can remove the duplicated code as soon as this one has landed. Is core audio always supported when this is true? Or are there edge cases where it is not supported? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:35: MEDIA_EXPORT int CoreAudioNumberOfActiveDevices(EDataFlow data_flow); kind of stinks to have to prefix all these with CoreAudio! Are you sure you don't want to wrap them in a CoreAudio class? CoreAudioUtil::NumberOfActiveDevices() looks good.
COM stuff in general looks good. However, I still have some suggestions :) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:22: class ScopedPropertyVariant { On 2012/10/25 01:08:08, DaleCurtis wrote: > Is this a pattern that other people would benefit from? Should it be in base::? PROPVARIANT isn't that widely used so having it here is OK with me. We could also move it to media/base if much of the media code uses it. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:71: std::string flow = (data_flow == eCapture) ? "[in ] " : "[out] "; if this is only for debugging, keep it in an #ifndef NDEBUG block. You could also get rid of the std::string variable and keep the ?: condition with the DVLOG. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:84: LOG_IF(ERROR, FAILED(hr)) << "CoCreateInstance(IMMDeviceEnumerator): " I'm tempted to make this a CHECK(SUCCEEDED(hr)). Since CoreAudioIsSupported is an assumption, the only way this could fail (unless something is seriously wrong), is af if the thread hasn't been CoInitialized. In either case, I think that would be a serious error and we shouldn't continue. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:106: LOG(ERROR) << "IMMDeviceEnumerator::GetDefaultAudioEndpoint: " DVLOG. There's a lot of logging in this file and the logging code costs quite a bit in code size. Unless we expect to need this logging with official builds, let's use DVLOG instead of LOG. (throughout) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:154: device_name.unique_id = WideToUTF8(static_cast<WCHAR*>(endpoint_device_id)); is the static_cast needed? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:162: PROPVARIANT friendly_name; why not use the ScopedPropVariant class? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:167: if (friendly_name.vt == VT_LPWSTR && friendly_name.pwszVal) { no {} (as is done in the if() right above) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:192: return name; nit: you don't really need the name variable. could just return device_name.device_name here and std::string() elsewhere. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:208: std::string str_default = WideToUTF8(static_cast<WCHAR*>(default_device_id)); static_cast? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:209: if (device_id.compare(str_default) != 0) { no {} http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:217: if (!device) should we instead DCHECK(device) here and assume the pointer is valid? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:221: device->QueryInterface(endpoint.Receive()); I have a feeling that this might fail unless it's true for all implementations of IMMDevice that they support IMMEndpoint. If that's true then add a note here about that, otherwise, you need to handle the error. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win_unittest.cc (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:18: // The test runs on a COM thread in a multithread apartment (MTA). nit: s/a multithread/the multithreaded (there is at most one MTA per process) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:22: : com_init_(ScopedCOMInitializer::kMTA) {} maybe DCHECK(com_init_.succeeded())? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:27: int capture_devices = CoreAudioNumberOfActiveDevices(eCapture); shouldn't you only call these methods if core_audio is true? i.e. if it is false, these will crash. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:35: // --- MMdevice remove? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:59: EXPECT_FALSE(enumerator); this isn't needed since you're not testing ScopedComPtr http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:61: EXPECT_TRUE(enumerator); I must be missing something - is the intention of the test to call CoreAudioCreateDeviceEnumerator() three times? (what's the difference between once vs twice vs three times?) Seems to me that you could just write the test like this: TEST_F(CoreAudioUtilWinTest, CreateDeviceEnumerator) { if (!CanRunAudioTest()) return; EXPECT_TRUE(CoreAudioCreateDeviceEnumerator()); } http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:73: CoreAudioCreateDefaultDevice(eRender, eCommunications); Forgot to assign to audio_device. All the checks for audio_device below are unnecessary because it's the same value as it was at line 71. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:75: CoreAudioCreateDefaultDevice(eRender, eMultimedia); forgot to assign. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:77: CoreAudioCreateDefaultDevice(eRender, eCommunications); forgot to assign... but then again, you've already checked eRender, eCommunications before. Why do you need to call it again? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:93: EXPECT_EQ(eAll, CoreAudioGetDataFlow(audio_device)); As a suggestion for writing code like this, I personally prefer data driven loops instead of copy/paste of practically the same function calls. For example I'm guessing that the entire test could be written like this (assuming CoreAudioGetDataFlow also works for eRender): ---- struct { EDataFlow flow; ERole role } data[] = { {eRender, eConsole}, {eRender, eCommunications}, {eRender, eMultimedia}, {eCapture, eConsole}, {eCapture, eCommunications}, {eCapture, eMultimedia}, }; for (int i = 0; i < arraysize(data); ++i) { ScopedComPtr<IMMDevice> audio_device = CoreAudioCreateDefaultDevice(data[i].flow, data[i].role); EXPECT_TRUE(audio_device); EXPECT_EQ(data[i].flow, CoreAudioGetDataFlow(audio_device)); } // Only eRender and eCapture are allowed as flow parameter. EXPECT_FALSE(CoreAudioCreateDefaultDevice(eAll, eConsole)); ---- I'm not so sure about the last check were you call CoreAudioGetDataFlow() with a NULL pointer. Should callers be allowed to call it with NULL? http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:106: EXPECT_TRUE(SUCCEEDED(hr)); nit: just pass the entire function call to EXPECT_TRUE(SUCCEEDED(<here>)). That way the log shows exactly what failed in case of an error instead of just "SUCCEEDED(hr)". http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:118: EXPECT_TRUE(SUCCEEDED(hr)); same here and elsewhere. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:120: device_name.unique_id.c_str()); doesn't this work? EXPECT_EQ(default_render_name.unique_id, device_name.unique_id); http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:135: remove http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:152: audio_device = CoreAudioCreateDefaultDevice(eCapture, eCommunications); you could use the same data driven approach in this test instead of copy/paste. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:174: EXPECT_STREQ(friendly_name.c_str(), device_name.device_name.c_str()); EXPECT_EQ should work for std::string (and no .c_str() should be needed) same elsewhere. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:195: const std::string id = name.unique_id; nit: const std::string& (assuming you're only adding the id variable to save on typing) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:217: EXPECT_TRUE(client); nit: EXPECT_TRUE(CoreAudioCreateClient(device)); http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:220: device = CoreAudioCreateDefaultDevice(eCapture, eConsole); this looks like a copy/paste of the block above, so another case for the data driven approach :)
Introduced CoreAudioUtil class scope and modified the unit test. Will take care of remaining comments regarding the main cc-file tomorrow. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:28: // --- MMDevice On 2012/10/24 19:25:44, henrika wrote: > Will modify. Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:35: MEDIA_EXPORT int CoreAudioNumberOfActiveDevices(EDataFlow data_flow); Agree. That is why I started out with a nested namespace. Anyhow, I'll create a class with only static members. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:70: // --- WASAPI On 2012/10/24 19:25:44, henrika wrote: > Will modify. Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win_unittest.cc (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:18: // The test runs on a COM thread in a multithread apartment (MTA). On 2012/10/25 08:42:11, tommi wrote: > nit: s/a multithread/the multithreaded > (there is at most one MTA per process) Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:22: : com_init_(ScopedCOMInitializer::kMTA) {} On 2012/10/25 08:42:11, tommi wrote: > maybe DCHECK(com_init_.succeeded())? Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:27: int capture_devices = CoreAudioNumberOfActiveDevices(eCapture); Thanks. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:35: // --- MMdevice On 2012/10/25 08:42:11, tommi wrote: > remove? Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:59: EXPECT_FALSE(enumerator); Good point. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:61: EXPECT_TRUE(enumerator); Agree. I was trying to test different use cases but that was stupid. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:73: CoreAudioCreateDefaultDevice(eRender, eCommunications); LOL. Will fix. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:75: CoreAudioCreateDefaultDevice(eRender, eMultimedia); dito http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:77: CoreAudioCreateDefaultDevice(eRender, eCommunications); see comment above about being stupid ;-) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:93: EXPECT_EQ(eAll, CoreAudioGetDataFlow(audio_device)); Cool ;-) Will fix last test as well. Something tells me that Dale has other ideas on how to rewrite this test ;-) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:106: EXPECT_TRUE(SUCCEEDED(hr)); Nice. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:118: EXPECT_TRUE(SUCCEEDED(hr)); On 2012/10/25 08:42:11, tommi wrote: > same here and elsewhere. Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:120: device_name.unique_id.c_str()); Yes. In fact it does ;-) http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:135: On 2012/10/25 08:42:11, tommi wrote: > remove Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:152: audio_device = CoreAudioCreateDefaultDevice(eCapture, eCommunications); On 2012/10/25 08:42:11, tommi wrote: > you could use the same data driven approach in this test instead of copy/paste. Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:174: EXPECT_STREQ(friendly_name.c_str(), device_name.device_name.c_str()); On 2012/10/25 08:42:11, tommi wrote: > EXPECT_EQ should work for std::string (and no .c_str() should be needed) > same elsewhere. Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:195: const std::string id = name.unique_id; less line breaking as well http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win_unittest.cc:220: device = CoreAudioCreateDefaultDevice(eCapture, eConsole); OK ;-)
FYI looks like you haven't addressed tommi's comments yet http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:20: #define PRINT(sz, ...) wprintf(sz L"\n", __VA_ARGS__); not removed :) http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:41: bool CoreAudioUtil::CoreAudioIsSupported() { TODO-ify de-duping? http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:82: virtual ~CoreAudioUtil() {} no need for virtual
Andrew: I did only have time to fix the unit test and introduce the CoreAudioUtil class today. Will deal with the remaining comments tomorrow. Perhaps it is possible to give final comments assuming that I will follow all given feedback. On Thu, Oct 25, 2012 at 6:52 PM, <scherkus@chromium.org> wrote: > FYI looks like you haven't addressed tommi's comments yet > > > http://codereview.chromium.**org/11226057/diff/13002/media/** > audio/win/core_audio_util_win.**cc<http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio_util_win.cc> > File media/audio/win/core_audio_**util_win.cc (right): > > http://codereview.chromium.**org/11226057/diff/13002/media/** > audio/win/core_audio_util_win.**cc#newcode20<http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio_util_win.cc#newcode20> > > media/audio/win/core_audio_**util_win.cc:20: #define PRINT(sz, ...) > wprintf(sz L"\n", __VA_ARGS__); > not removed :) > > http://codereview.chromium.**org/11226057/diff/13002/media/** > audio/win/core_audio_util_win.**cc#newcode41<http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio_util_win.cc#newcode41> > media/audio/win/core_audio_**util_win.cc:41: bool > CoreAudioUtil::**CoreAudioIsSupported() { > TODO-ify de-duping? > > http://codereview.chromium.**org/11226057/diff/13002/media/** > audio/win/core_audio_util_win.**h<http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio_util_win.h> > File media/audio/win/core_audio_**util_win.h (right): > > http://codereview.chromium.**org/11226057/diff/13002/media/** > audio/win/core_audio_util_win.**h#newcode82<http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio_util_win.h#newcode82> > media/audio/win/core_audio_**util_win.h:82: virtual ~CoreAudioUtil() {} > no need for virtual > > http://codereview.chromium.**org/11226057/<http://codereview.chromium.org/112... >
lgtm pending tommi's approval
Waiting for final verdict from Tommi. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:20: #define PRINT(sz, ...) wprintf(sz L"\n", __VA_ARGS__); On 2012/10/24 19:25:44, henrika wrote: > Correct. Used while developing. Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:22: class ScopedPropertyVariant { I added it when I did enumeration for Dale's IMMNotification patch. It can be useful when we expand this class. I would like to keep it here for now if that is OK. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:45: return (base::win::GetVersion() >= base::win::VERSION_VISTA); I don't have a better method. You could also do some sort of "mini test" where you tried APIs from all sub-APIs (MMDevice, WASAPI etc.) to be really sure. I have not yet seen the need to go that far. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:84: LOG_IF(ERROR, FAILED(hr)) << "CoCreateInstance(IMMDeviceEnumerator): " Agree. We should actually. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:106: LOG(ERROR) << "IMMDeviceEnumerator::GetDefaultAudioEndpoint: " On 2012/10/25 08:42:11, tommi wrote: > DVLOG. There's a lot of logging in this file and the logging code costs quite a > bit in code size. Unless we expect to need this logging with official builds, > let's use DVLOG instead of LOG. (throughout) Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:154: device_name.unique_id = WideToUTF8(static_cast<WCHAR*>(endpoint_device_id)); Changed to WideToUTF8(endpoint_device_id, wcslen(endpoint_device_id), &device_name.unique_id); But, there were presubmit warnings. " Seems like New code should not use wstrings. If you are calling a cross-platform API that accepts a wstring, fix the API." Guess it is OK though since this is Windows code. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:162: PROPVARIANT friendly_name; LOL ;-) I used it for debugging only. Forgot I could use it here. Improved it based on base::win::ScopedVariant. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:167: if (friendly_name.vt == VT_LPWSTR && friendly_name.pwszVal) { On 2012/10/25 08:42:11, tommi wrote: > no {} > (as is done in the if() right above) Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:192: return name; On 2012/10/25 08:42:11, tommi wrote: > nit: you don't really need the name variable. could just return > device_name.device_name here and std::string() elsewhere. Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:208: std::string str_default = WideToUTF8(static_cast<WCHAR*>(default_device_id)); Fixed as above. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:209: if (device_id.compare(str_default) != 0) { On 2012/10/25 08:42:11, tommi wrote: > no {} Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:217: if (!device) On 2012/10/25 08:42:11, tommi wrote: > should we instead DCHECK(device) here and assume the pointer is valid? Done. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:221: device->QueryInterface(endpoint.Receive()); On 2012/10/25 08:42:11, tommi wrote: > I have a feeling that this might fail unless it's true for all implementations > of IMMDevice that they support IMMEndpoint. > If that's true then add a note here about that, otherwise, you need to handle > the error. Done. http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:20: #define PRINT(sz, ...) wprintf(sz L"\n", __VA_ARGS__); On 2012/10/25 16:52:13, scherkus wrote: > not removed :) Done. http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:41: bool CoreAudioUtil::CoreAudioIsSupported() { On 2012/10/25 16:52:13, scherkus wrote: > TODO-ify de-duping? Done. http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): http://codereview.chromium.org/11226057/diff/13002/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:82: virtual ~CoreAudioUtil() {} On 2012/10/25 16:52:13, scherkus wrote: > no need for virtual Done.
Regarding the wstring presubmit warnings. I don't think they are because of the WideToUTF8 method I recommended that you call (the one that takes 3 arguments). The version of WideToUTF8 that you were calling before and are still calling in a couple of places, actually uses std::wstring and I recommend that you don't call that function. It's bad for two reasons, one of them is because of wstring, the other (worse imo), is that it creates a new wide string from your wide string just to be able to convert that wide string to utf8. I.e. waste. I don't think that any of the above is the reason why you're seeing the presubmit warning though. :) I think the reason is simply that you chose to name your method 'wstring', which I don't think is a good idea :) See below. With that and the other comments addressed: lgtm. http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): http://codereview.chromium.org/11226057/diff/11001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:9: #ifndef MEDIA_AUDIO_WIN_CORE_AUDIO_UTIL_WIN_H_ On 2012/10/24 19:25:44, henrika wrote: > Got it. I actually think that I followed lint here. Lint was pointing out that the #if guard must match the file name. I think Andrew is saying that the file name doesn't need to have the _win postfix since it's already in a windows-only folder. However, I checked the other files in this folder and see that they all have the _win postfix, so I think it's more consistent to keep the file names as they are. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:34: return &propvar_; Ideally you should have a DCHECK here to make sure that you don't already hold something that might leak. (see ScopedVariant for an example). An easy check (and more strict) would be to add this: DCHECK_EQ(propvar_.vt, VT_EMPTY); http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:41: LPWSTR wstring() const { to avoid confusing this with std::wstring - which I'm sure will cause lint errors - call this AsWideString(), as_wide_string() or something like that. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:42: DCHECK(type() == VT_LPWSTR); nit: DCHECK_EQ http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:181: device_name.device_name = WideToUTF8(friendly_name.wstring()); here you're using the std::wstring version of WideToUTF8. What's also confusing is that the wstring() method does not return a wstring, see comment above. Suggest switching to the other version of WideToUTF8 here as I recommended for the other place and change the name of the wstring method. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:82: ~CoreAudioUtil() {} fix indent
Thanks Tommi. Done. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:34: return &propvar_; Thanks. I will make the easy fix ;-) http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:41: LPWSTR wstring() const { On 2012/10/29 10:45:27, tommi wrote: > to avoid confusing this with std::wstring - which I'm sure will cause lint > errors - call this AsWideString(), as_wide_string() or something like that. Done. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:41: LPWSTR wstring() const { On 2012/10/29 10:45:27, tommi wrote: > to avoid confusing this with std::wstring - which I'm sure will cause lint > errors - call this AsWideString(), as_wide_string() or something like that. Done. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:42: DCHECK(type() == VT_LPWSTR); On 2012/10/29 10:45:27, tommi wrote: > nit: DCHECK_EQ Done. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:181: device_name.device_name = WideToUTF8(friendly_name.wstring()); On 2012/10/29 10:45:27, tommi wrote: > here you're using the std::wstring version of WideToUTF8. > What's also confusing is that the wstring() method does not return a wstring, > see comment above. > Suggest switching to the other version of WideToUTF8 here as I recommended for > the other place and change the name of the wstring method. Done. http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.h (right): http://codereview.chromium.org/11226057/diff/23001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.h:82: ~CoreAudioUtil() {} On 2012/10/29 10:45:27, tommi wrote: > fix indent Done.
lgtm http://codereview.chromium.org/11226057/diff/30001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/30001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:181: if (friendly_name.as_wide_string()) nit: {}
http://codereview.chromium.org/11226057/diff/30001/media/audio/win/core_audio... File media/audio/win/core_audio_util_win.cc (right): http://codereview.chromium.org/11226057/diff/30001/media/audio/win/core_audio... media/audio/win/core_audio_util_win.cc:181: if (friendly_name.as_wide_string()) On 2012/10/29 12:20:25, tommi wrote: > nit: {} Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
