Add sample rates checking in MediaStreamAudioProcessor

Add sample rates checking in MediaStreamAudioProcessor

This patch enforced checking the input sample rate to MediaStreamAudioProcessor
by adding checking routines in MediaStreamAudioFifo.

BUG=536607
Committed: https://crrev.com/c5eb149ca476765840fe4fa9b4762f97cea3dd92
Cr-Commit-Position: refs/heads/master@{#353932}

[msu.koo, 2015-09-30 04:35:58]

msu.koo@samsung.com changed reviewers:
+ grunell@chromium.org

[msu.koo, 2015-09-30 04:35:58]

I made a patch on your issue.
PTAL :)

[Henrik Grunell, 2015-09-30 08:19:25]

grunell@chromium.org changed reviewers:
+ henrika@chromium.org

[Henrik Grunell, 2015-09-30 08:19:25]

Thanks for working on this! :)

+Henrik A to answer about supported sample rates.

https://codereview.chromium.org/1377103002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/1377103002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_processor.cc:141:
DCHECK_GE(sample_rate_, media::limits::kMinSampleRate);
I was thinking to test the set of explicit samples rates that we support, e.g.
8000, 16000, and so on. However I don't know exactly what we support and if
that's defined somewhere. Deferring to Henrik A to answer that.

[henrika (OOO until Aug 14), 2015-09-30 08:49:18]

https://codereview.chromium.org/1377103002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/1377103002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_processor.cc:141:
DCHECK_GE(sample_rate_, media::limits::kMinSampleRate);
Have not done the latest changes in this area and can't say for sure but it
seems like we support "all rates". See
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
and\
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/sample...

[henrika (OOO until Aug 14), 2015-09-30 08:50:59]

But should it not be the MSAP that sets the limitations based on what it can
support? Don't know what we are trying to solve here actually.

[Henrik Grunell, 2015-09-30 09:12:58]

On 2015/09/30 08:50:59, henrika wrote:
> But should it not be the MSAP that sets the limitations based on what it can
> support? Don't know what we are trying to solve here actually.

Sounds good. Let's continue that discussion in the bug since it's a general
thing we need to sort out.

[Henrik Grunell, 2015-10-02 13:50:17]

On 2015/09/30 09:12:58, Henrik Grunell wrote:
> On 2015/09/30 08:50:59, henrika wrote:
> > But should it not be the MSAP that sets the limitations based on what it can
> > support? Don't know what we are trying to solve here actually.
> 
> Sounds good. Let's continue that discussion in the bug since it's a general
> thing we need to sort out.

A per bug discussion, dcheck the interval [8, 48] since that's what the audio
processing module supports.

[msu.koo, 2015-10-06 06:40:40]

On 2015/10/02 13:50:17, Henrik Grunell wrote:
> On 2015/09/30 09:12:58, Henrik Grunell wrote:
> > On 2015/09/30 08:50:59, henrika wrote:
> > > But should it not be the MSAP that sets the limitations based on what it
can
> > > support? Don't know what we are trying to solve here actually.
> > 
> > Sounds good. Let's continue that discussion in the bug since it's a general
> > thing we need to sort out.
> 
> A per bug discussion, dcheck the interval [8, 48] since that's what the audio
> processing module supports.

From https://code.google.com/p/chromium/issues/detail?id=536607#c8
Thank you for your comments :)

I'm not sure that the rage of |sample_rate_| in MSAP is [8, 48], because the
|sample_rate_| comes from AudioParameters([1]), which takes the sample rate as
Hz not kHz([2]).

IMHO, it's not a bad idea using |media::limits::kMinSampleRate| and 
|media::limits::kMaxSampleRate| for this as on the
CL(https://crrev.com/1377103002).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...

[msu.koo, 2015-10-06 06:40:42]

On 2015/10/02 13:50:17, Henrik Grunell wrote:
> On 2015/09/30 09:12:58, Henrik Grunell wrote:
> > On 2015/09/30 08:50:59, henrika wrote:
> > > But should it not be the MSAP that sets the limitations based on what it
can
> > > support? Don't know what we are trying to solve here actually.
> > 
> > Sounds good. Let's continue that discussion in the bug since it's a general
> > thing we need to sort out.
> 
> A per bug discussion, dcheck the interval [8, 48] since that's what the audio
> processing module supports.

From https://code.google.com/p/chromium/issues/detail?id=536607#c8
Thank you for your comments :)

I'm not sure that the rage of |sample_rate_| in MSAP is [8, 48], because the
|sample_rate_| comes from AudioParameters([1]), which takes the sample rate as
Hz not kHz([2]).

IMHO, it's not a bad idea using |media::limits::kMinSampleRate| and 
|media::limits::kMaxSampleRate| for this as on the
CL(https://crrev.com/1377103002).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...

[msu.koo, 2015-10-07 00:42:14]

PTAL :)

[Henrik Grunell, 2015-10-07 10:09:14]

LGTM.

Note that you also need an owner to stamp/review.

[msu.koo, 2015-10-07 10:12:21]

msu.koo@samsung.com changed reviewers:
+ mcasas@chromium.org

[msu.koo, 2015-10-07 10:12:21]

@mcasas, Could you take a look this CL?

[mcasas, 2015-10-09 23:48:49]

RS LGTM

https://chromiumcodereview.appspot.com/1377103002/diff/20001/content/renderer...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://chromiumcodereview.appspot.com/1377103002/diff/20001/content/renderer...
content/renderer/media/media_stream_audio_processor.cc:141:
DCHECK_LE(sample_rate_, 48000);
Nit: Could |sample_rate| be a |size_t| ?

[msu.koo, 2015-10-10 11:29:26]

The CQ bit was checked by msu.koo@samsung.com

[commit-bot: I haz the power, 2015-10-10 11:29:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377103002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377103002/20001

[commit-bot: I haz the power, 2015-10-10 11:52:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-10 11:52:52]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Henrik Grunell, 2015-10-12 08:02:58]

https://chromiumcodereview.appspot.com/1377103002/diff/20001/content/renderer...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://chromiumcodereview.appspot.com/1377103002/diff/20001/content/renderer...
content/renderer/media/media_stream_audio_processor.cc:141:
DCHECK_LE(sample_rate_, 48000);
On 2015/10/09 23:48:48, mcasas wrote:
> Nit: Could |sample_rate| be a |size_t| ?

Not sure what you're after here. Why should it be a size_t?

[msu.koo, 2015-10-12 08:07:33]

On 2015/10/10 11:52:52, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

bot reported that this change cannot pass
WebContentsVideoCaptureDeviceTest.VariableResolution_FixedAspectRatio unittest,
because it tries to set 88200 which is bigger than 48000.

Please let me know how I can handle this: By changing unittest? or changing max
sample rate from 48000 to 88200.

[msu.koo, 2015-10-12 08:07:35]

On 2015/10/10 11:52:52, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

bot reported that this change cannot pass
WebContentsVideoCaptureDeviceTest.VariableResolution_FixedAspectRatio unittest,
because it tries to set 88200 which is bigger than 48000.

Please let me know how I can handle this: By changing unittest? or changing max
sample rate from 48000 to 88200.

[Henrik Grunell, 2015-10-12 11:13:53]

On 2015/10/12 08:07:35, msu.koo wrote:
> On 2015/10/10 11:52:52, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> 
> bot reported that this change cannot pass
> WebContentsVideoCaptureDeviceTest.VariableResolution_FixedAspectRatio
unittest,
> because it tries to set 88200 which is bigger than 48000.
> 
> Please let me know how I can handle this: By changing unittest? or changing
max
> sample rate from 48000 to 88200.

The unit test needs to be updated accordingly.

[msu.koo, 2015-10-13 23:53:24]

The CQ bit was checked by msu.koo@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-13 23:54:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377103002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377103002/40001

[commit-bot: I haz the power, 2015-10-14 00:53:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-14 00:53:42]

Dry run: This issue passed the CQ dry run.

[msu.koo, 2015-10-14 00:54:26]

The CQ bit was checked by msu.koo@samsung.com

[msu.koo, 2015-10-14 00:54:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
grunell@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1377103002/#ps40001
(title: "unittest updated")

[commit-bot: I haz the power, 2015-10-14 00:55:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377103002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377103002/40001

[commit-bot: I haz the power, 2015-10-14 01:04:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-14 01:05:45]

Patchset 3 (id:??) landed as
https://crrev.com/c5eb149ca476765840fe4fa9b4762f97cea3dd92
Cr-Commit-Position: refs/heads/master@{#353932}

[Henrik Grunell, 2015-10-14 08:03:13]

Unit test lgtm.