On windows, create PCMWaveInAudioInputStream instance with correct device ID.

On windows, create PCMWaveInAudioInputStream instance with correct device ID.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126779

[yzshen1, 2012-03-05 01:22:54]

Hi, Henrik.

Would you please take a look? Thanks!

[henrika (OOO until Aug 14), 2012-03-05 08:50:44]

Looks great. Could you please extend the device enumeration unit test as well to
ensure that your changes are well tested.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
File media/audio/win/device_enumeration_win.cc (right):

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:20: namespace {
Guess you can remove the namespace part here. Should not have any effect on
#define:s.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:143: result =
waveInMessage(reinterpret_cast<HWAVEIN>(i),
I would add a comment (like in the MSDN example) here: "Get the size (including
the terminating null) of
the endpoint ID string of the waveIn device."

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:154: result = waveInMessage(
Please comment to make the code more readable. Thanks.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:160: std::string utf8_id =
WideToUTF8(static_cast<WCHAR*>(id));
// Check whether the endpoint ID string of this waveIn
// device matches that of the audio endpoint device.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:168: result = waveInGetDevCaps(i,
&capabilities, sizeof(capabilities));
Comment that we convert the unique endpoint ID string to a standard friendly
name with max 32 characters.

[henrika (OOO until Aug 14), 2012-03-05 08:51:42]

+xians

[no longer working on chromium, 2012-03-05 09:58:38]

Thanks for adding this feature, it looks really good.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:316: return NULL;
It looks like an unexpected error to me, adding a DLOG?

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:321: device_id);
Just a personal preference, feel free to ignore if you don't like the style. It
looks too many "return" in such a short function to me, what about doing it in
another way:

std::string new_device_id = device_id;
if (device_id != AudioManagerBase::kDefaultDeviceId ...) {
  new_device_id = ConvertToWinXPDeviceId(device_id);
  if (new_device_id.empty()) {
    DLOG()?
    return NULL;
  }
}
  return new PCMWaveInAudioInputStream(, , , new_device_id);

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
File media/audio/win/device_enumeration_win.cc (right):

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:28: 
Since we have been here.
Preferably we should put all the following functions under a namespace, for
example, media or device_enumeration.
It should be quite easy, how about doing it in this CL?

[yzshen1, 2012-03-13 21:09:23]

Thanks!
And sorry for late reply, I got stuck with a few P1 bugs last week.

I have made changes according to your suggestions and added unittests. Please
take another look.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:316: return NULL;
On 2012/03/05 09:58:39, xians1 wrote:
> It looks like an unexpected error to me, adding a DLOG?

Done.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:321: device_id);
On 2012/03/05 09:58:39, xians1 wrote:
> Just a personal preference, feel free to ignore if you don't like the style.
It
> looks too many "return" in such a short function to me, what about doing it in
> another way:
> 
> std::string new_device_id = device_id;
> if (device_id != AudioManagerBase::kDefaultDeviceId ...) {
>   new_device_id = ConvertToWinXPDeviceId(device_id);
>   if (new_device_id.empty()) {
>     DLOG()?
>     return NULL;
>   }
> }
>   return new PCMWaveInAudioInputStream(, , , new_device_id);

Done.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
File media/audio/win/device_enumeration_win.cc (right):

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:20: namespace {
On 2012/03/05 08:50:45, henrika wrote:
> Guess you can remove the namespace part here. Should not have any effect on
> #define:s.

Done.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:28: 
Sure. :)
On 2012/03/05 09:58:39, xians1 wrote:
> Since we have been here.
> Preferably we should put all the following functions under a namespace, for
> example, media or device_enumeration.
> It should be quite easy, how about doing it in this CL?

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:143: result =
waveInMessage(reinterpret_cast<HWAVEIN>(i),
On 2012/03/05 08:50:45, henrika wrote:
> I would add a comment (like in the MSDN example) here: "Get the size
(including
> the terminating null) of
> the endpoint ID string of the waveIn device."

Done.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:154: result = waveInMessage(
On 2012/03/05 08:50:45, henrika wrote:
> Please comment to make the code more readable. Thanks.

Done.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:160: std::string utf8_id =
WideToUTF8(static_cast<WCHAR*>(id));
On 2012/03/05 08:50:45, henrika wrote:
> // Check whether the endpoint ID string of this waveIn
> // device matches that of the audio endpoint device.

Done.

http://codereview.chromium.org/9566002/diff/3001/media/audio/win/device_enume...
media/audio/win/device_enumeration_win.cc:168: result = waveInGetDevCaps(i,
&capabilities, sizeof(capabilities));
On 2012/03/05 08:50:45, henrika wrote:
> Comment that we convert the unique endpoint ID string to a standard friendly
> name with max 32 characters.

Done.

[henrika (OOO until Aug 14), 2012-03-14 15:33:35]

Looks great. I actually have no comments ;-)

LGTM.

[henrika (OOO until Aug 14), 2012-03-14 15:35:03]

FYI, we added some information to
http://code.google.com/p/chromium/issues/detail?id=109278 which is listed in
suppressions_full.txt.

[no longer working on chromium, 2012-03-14 15:49:27]

lgtm

[commit-bot: I haz the power, 2012-03-14 19:05:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/9566002/13008

[commit-bot: I haz the power, 2012-03-14 23:16:46]

Change committed as 126779