Added support for multiple sampling rates on Windows.

Added support for multiple sampling rates on Windows.


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

[kxing, 2012-07-24 20:38:41]

Could someone PTAL?

[Wez, 2012-07-25 09:23:48]

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.h
File remoting/host/audio_capturer.h (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.h...
remoting/host/audio_capturer.h:24: // Capturers should sample at a 44.1 kHz
sampling rate, in uncompressed PCM
nit: This comment needs updating if AudioCapturer is going to support a variety
of rates.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.h...
remoting/host/audio_capturer.h:34: static bool IsValidSampleRate(int
sample_rate);
Does it make sense for this to be a static? Won't the available sample rates
vary by AudioCapturer implementation? Or does this refer to the sample rates
that our protocol supports? Or that the current client implementation supports?

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:136: if
(!AudioCapturer::IsValidSampleRate(wave_format_ex_->nSamplesPerSec)) {
Rather than have a static test for validity, why not pass a set of acceptable
capture rates to the AudioCapturer when it's started - we can then still
hard-code specific rates e.g. into the AudioScheduler, but we also have the
option of feeding back supported playback rates from the connected client.

[Sergey Ulanov, 2012-07-26 03:40:24]

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.cc
File remoting/host/audio_capturer.cc (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.c...
remoting/host/audio_capturer.cc:14: // Intentional fall-through.
it would be more readable without this comment. Such comment would be useful
only if you had some other code here.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:67: :
sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID) {
nit: initializer list should be indented 4 spaces (6 spaces after the first
line).

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:138: return false;
Can we still try to request some known sampling rate?

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:158:
wave_format_extensible->Format.nSamplesPerSec)) {
nit: incorrect indentation. move it four spaces to the right.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:164:
wave_format_extensible->Format.nSamplesPerSec);
nit: indent 4 spaces relative to the previous line. Or you can fit this
statement in two lines.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:183: LOG(ERROR) << "Failed to force 16-bit
samples";
is this a correct error message?

[kxing, 2012-07-26 22:47:17]

Could someone PTAL again?

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.cc
File remoting/host/audio_capturer.cc (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.c...
remoting/host/audio_capturer.cc:14: // Intentional fall-through.
On 2012/07/26 03:40:24, sergeyu wrote:
> it would be more readable without this comment. Such comment would be useful
> only if you had some other code here.

Done.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.h
File remoting/host/audio_capturer.h (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.h...
remoting/host/audio_capturer.h:24: // Capturers should sample at a 44.1 kHz
sampling rate, in uncompressed PCM
On 2012/07/25 09:23:48, Wez wrote:
> nit: This comment needs updating if AudioCapturer is going to support a
variety
> of rates.

Done.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer.h...
remoting/host/audio_capturer.h:34: static bool IsValidSampleRate(int
sample_rate);
It represents the rates the current client implementation supports, so it should
be the same across all platforms.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:67: :
sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID) {
On 2012/07/26 03:40:24, sergeyu wrote:
> nit: initializer list should be indented 4 spaces (6 spaces after the first
> line).

Done.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:136: if
(!AudioCapturer::IsValidSampleRate(wave_format_ex_->nSamplesPerSec)) {
The set of valid sampling rates is the same across all platforms (since they are
the sampling rates supported by Pepper). I'm not sure if the extra flexibility
of passed-in playback rates is worth it.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:138: return false;
No, Pepper only supports 44.1 kHz and 48 kHz, and Windows only allows sampling
at the current output rate. Thus, if the output rate is neither 44.1 nor 48 kHz,
we'll have to resample.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:158:
wave_format_extensible->Format.nSamplesPerSec)) {
On 2012/07/26 03:40:24, sergeyu wrote:
> nit: incorrect indentation. move it four spaces to the right.

Done.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:164:
wave_format_extensible->Format.nSamplesPerSec);
On 2012/07/26 03:40:24, sergeyu wrote:
> nit: indent 4 spaces relative to the previous line. Or you can fit this
> statement in two lines.

Done.

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:183: LOG(ERROR) << "Failed to force 16-bit
samples";
On 2012/07/26 03:40:24, sergeyu wrote:
> is this a correct error message?

Done.

[Sergey Ulanov, 2012-07-27 04:22:37]

LGTM provided my last style nit is addressed

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10818010/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:136: if
(!AudioCapturer::IsValidSampleRate(wave_format_ex_->nSamplesPerSec)) {
On 2012/07/25 09:23:48, Wez wrote:
> Rather than have a static test for validity, why not pass a set of acceptable
> capture rates to the AudioCapturer when it's started - we can then still
> hard-code specific rates e.g. into the AudioScheduler, but we also have the
> option of feeding back supported playback rates from the connected client.

I discussed this with Kerry. This might be useful in the future, but there is no
point in doing it in this CL - we can add it later when we need it.

http://codereview.chromium.org/10818010/diff/8001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10818010/diff/8001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:142: wave_format_ex_->nSamplesPerSec);
nit: incorrect indentation

[kxing, 2012-07-27 14:39:51]

I'll commit this soon, unless someone objects.

http://codereview.chromium.org/10818010/diff/8001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10818010/diff/8001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:142: wave_format_ex_->nSamplesPerSec);
On 2012/07/27 04:22:38, sergeyu wrote:
> nit: incorrect indentation

Done.

[commit-bot: I haz the power, 2012-07-27 14:49:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10818010/14001

[commit-bot: I haz the power, 2012-07-27 15:06:47]

Try job failure for 10818010-14001 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-07-27 15:56:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10818010/14001

[commit-bot: I haz the power, 2012-07-27 17:19:06]

Change committed as 148766