audio_manager_linux: Allow CRAS to decide if sample rate conversion is needed.

audio_manager_linux: Allow CRAS to decide if sample rate conversion is needed.

When using CRAS, set up audio streams based on the desired sample rate,
not the "hardware" sample rate.  CRAS will configure the hardware for
the sample rate if possible and sample rate convert if not.

BUG=chromium-os:34724
TEST=play 44.1 video on youtube, check that the hardware is configured
for 44.1.

Signed-off-by: Dylan Reid <dgreid@chromium.org>;


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

[dgreid, 2012-09-25 19:42:56]

There is a bug to for the audio_manager and avoid ifdefs hopefully I'll have
time to do that soon (after link/daisy/parrot/etc.)  For now,-- something simple
that can be merged back to 23.

[DaleCurtis, 2012-09-25 19:56:55]

Slightly off topic, but ideally in the plain Linux case we'd query the supported
sample rates and pass through if the requested rate is listed. Do you know how
hard that would be to do?

http://codereview.chromium.org/10991019/diff/1/media/audio/linux/audio_manage...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/10991019/diff/1/media/audio/linux/audio_manage...
media/audio/linux/audio_manager_linux.cc:354: if
(static_cast<size_t>(input_params.frames_per_buffer()) <
Mind collapsing this to:

int buffer_size = GetAudioHardwareBufferSize();
if (input_params.frames_per_buffer() < buffer_size)
  buffer_size = input_params.frames_per_buffer();

While you're here?

http://codereview.chromium.org/10991019/diff/1/media/audio/linux/audio_manage...
media/audio/linux/audio_manager_linux.cc:362: // CRAS will sample rate convert
if it is needed, pass the desired frame rate.
To keep style with the above code, why not:

int sample_rate = GetAudioHardwareSampleRate();
if (UseCras())
  sample_rate = input_params.sample_rate();

I don't think the IF_DEF is necessary since UseCras is always defined and
returns false in non-cras case.

[dgreid, 2012-09-25 20:41:07]

On 2012/09/25 19:56:55, DaleCurtis wrote:
> Slightly off topic, but ideally in the plain Linux case we'd query the
supported
> sample rates and pass through if the requested rate is listed. Do you know how
> hard that would be to do?

That's what the audio server does.  It involves trying the sample rates you are
interested in with snd_pcm_hw_params_test_rate.  There is also an optimization
to be had in the pulse audio case.  PA recently had a feature added where it
supports a main sample rate and an alternative sample rate (normally 48k and
44.1 but configurable).

> 
>
http://codereview.chromium.org/10991019/diff/1/media/audio/linux/audio_manage...
> File media/audio/linux/audio_manager_linux.cc (right):
> 
>
http://codereview.chromium.org/10991019/diff/1/media/audio/linux/audio_manage...
> media/audio/linux/audio_manager_linux.cc:354: if
> (static_cast<size_t>(input_params.frames_per_buffer()) <
> Mind collapsing this to:
> 
> int buffer_size = GetAudioHardwareBufferSize();
> if (input_params.frames_per_buffer() < buffer_size)
>   buffer_size = input_params.frames_per_buffer();
> 
> While you're here?
> 
>
http://codereview.chromium.org/10991019/diff/1/media/audio/linux/audio_manage...
> media/audio/linux/audio_manager_linux.cc:362: // CRAS will sample rate convert
> if it is needed, pass the desired frame rate.
> To keep style with the above code, why not:
> 
> int sample_rate = GetAudioHardwareSampleRate();
> if (UseCras())
>   sample_rate = input_params.sample_rate();
> 
> I don't think the IF_DEF is necessary since UseCras is always defined and
> returns false in non-cras case.

Thanks, good suggestions. changes made, PTAL.

[DaleCurtis, 2012-09-25 20:49:46]

LGTM % nits.

Nice to know about the Linux stuff, I'll probably talk to you again in the
future when it comes time to change the AudioUtil::GetHardware* methods over to
manager-specific methods.

http://codereview.chromium.org/10991019/diff/7001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/10991019/diff/7001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:354: if
(static_cast<size_t>(input_params.frames_per_buffer()) < buffer_size)
Drop static_cast now.

http://codereview.chromium.org/10991019/diff/7001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:358: // CRAS will sample rate convert
if needed, pass the desired sample rate.
s/pass/so pass through input sample rate./

[scherkus (not reviewing), 2012-09-25 20:52:38]

lgtm w/ nit

http://codereview.chromium.org/10991019/diff/7001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/10991019/diff/7001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:354: if
(static_cast<size_t>(input_params.frames_per_buffer()) < buffer_size)
nit: you can remove static_cast<>

[commit-bot: I haz the power, 2012-09-25 23:11:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgreid@chromium.org/10991019/3

[commit-bot: I haz the power, 2012-09-26 00:16:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgreid@chromium.org/10991019/3

[commit-bot: I haz the power, 2012-09-26 08:58:51]

Change committed as 158772

[dgreid, 2012-10-03 20:25:54]

Could one of you drover this into M23 for me?

merge approved here:
crosbug.com/34724

Thanks,

Dylan

[dgreid, 2012-10-09 16:52:47]

Sorry to bother you again but could one of you merge this to 23 for me?

Thanks,

Dylan

[Ami GONE FROM CHROMIUM, 2012-10-09 16:57:46]

I'll merge r158772 to M23.

On Tue, Oct 9, 2012 at 9:52 AM, <dgreid@chromium.org> wrote:

> Sorry to bother you again but could one of you merge this to 23 for me?
>
> Thanks,
>
> Dylan
>
>
http://codereview.chromium.**org/10991019/<http://codereview.chromium.org/109...
>

[Ami GONE FROM CHROMIUM, 2012-10-09 16:59:35]

On Tue, Oct 9, 2012 at 9:57 AM, Ami Fischman <fischman@chromium.org> wrote:

> I'll merge r158772 to M23.


Committed revision 160859.