Mute audio under OLA when given rate would cause poor quality.

Mute audio under OLA when given rate would cause poor quality.
BUG=18362
TEST=none

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

[kylep, 2009-08-06 00:21:41]

I didn't mute for audio @ 0.5x speed even though it sounds bad because of our
context menu option to set it to that speed. Should I? Thanks for the review!

[fbarchard, 2009-08-06 00:45:13]

LGTM
I would expand on the comment about issues with higher and lower rates.
1. both causes crashes, which should really be tracked down and fixed...
probably a buffer overrun.  The one we know about is advancing more than the
queue, which is fixed.
2. both sound poor anyway.  especially low speed.  The fix, if we choose to
improve it, is likely frequency domain.  Which in many ways is easier than what
we have.  Just do fft and ifft with new rate.  But it would be slow.  I've seen
no evidence it would help speed up quality, but you've suggested that slowdown
sounds almost perfect.
Slowdown usecase, IMO is for sports.  If you were watching a video and wanted a
super slo mo playback, its nice if the audio doesn't sound stupid.

Also can you test wav_ola_test.cc?
It seems to always fail now, only writing 44 bytes.

Great work!  You fixed all the reasonable bugs/crashes I can think of.  I can
think of less reasonable data that would cause a hang, but not a crash.
In my experience working with intern code, if theres any bugs, we throw it out.
If its rock solid and we dont have to touch it again for the next couple years,
then it was a success.  And you know that quicktime guru will be pressuring us
to do better.  So we'll need to point out that ours is reliable, fast, portable,
high quality.  I'm confident in our solution.

[fbarchard, 2009-08-06 01:00:27]

nit on the comment

http://codereview.chromium.org/164031/diff/1/2
File media/filters/audio_renderer_algorithm_ola.cc (right):

http://codereview.chromium.org/164031/diff/1/2#newcode45
Line 45: // Limited rate to what sounds acceptable (Should really be for <0.7x)
space after < and . at end of sentence.
But I'd write TODO's about why we're limiting it so we know what to fix in the
future.

[scherkus (not reviewing), 2009-08-06 03:39:13]

drive by comments:
 - change description does not match code change
 - magic numbers!
 - add more comments

http://codereview.chromium.org/164031/diff/1/2
File media/filters/audio_renderer_algorithm_ola.cc (right):

http://codereview.chromium.org/164031/diff/1/2#newcode46
Line 46: if (playback_rate() < 0.5f || playback_rate() > 4.0f) {
magic number alert!!!

please use constants, comment them and add the TODOs frank mentioned

how would a user of the public API be aware of these rates?  I would update the
.h file to say that "audio will be muted for very slow/fast rates"

[kylep, 2009-08-06 17:51:49]

Extracted constants, clarified comments, renamed some stuff to make it more
readable.

[fbarchard, 2009-08-06 18:33:52]

LGTM
nit uses more electricity on 8 bit than 16 bit. :)

http://codereview.chromium.org/164031/diff/1008/1009
File media/filters/audio_renderer_algorithm_ola.cc (right):

http://codereview.chromium.org/164031/diff/1008/1009#newcode58
Line 58: memset(dest, 0, length);
FYI Although we do this elsewhere, 0 is only correct for signed ints, not
unsigned.  This will cause an initial click on 8 bit, and use more electricity
to hold the speaker at an extreme.
Since we really do not support 8 bit, I would leave it.

[scherkus (not reviewing), 2009-08-06 19:23:35]

wait would it be possible to have a unit test for this change?

asserting that below a certain rate we get zeros

[fbarchard1, 2009-08-06 21:55:37]

don't be evil :-)yes it would be possible.  At EA we had a saying - nothing
in the game is impossible... its just highly unlikely.
okay its not __that__ hard.  but we're definitely not gonna measure the
electricity usage!

On Thu, Aug 6, 2009 at 12:23 PM, <scherkus@chromium.org> wrote:

> wait would it be possible to have a unit test for this change?
>
> asserting that below a certain rate we get zeros
>
>
> http://codereview.chromium.org/164031
>

[kylep, 2009-08-07 00:16:06]

How exactly should I write said test... pass in buffers full of random data, set
the rate to something low or high, call some FillBuffers and make sure I get 0s?
Is there an easy way to fill a DataBuffer with garbage?

On 2009/08/06 21:55:37, fbarchard wrote:
> don't be evil :-)yes it would be possible.  At EA we had a saying - nothing
> in the game is impossible... its just highly unlikely.
> okay its not __that__ hard.  but we're definitely not gonna measure the
> electricity usage!
> 
> On Thu, Aug 6, 2009 at 12:23 PM, <scherkus@chromium.org> wrote:
> 
> > wait would it be possible to have a unit test for this change?
> >
> > asserting that below a certain rate we get zeros
> >
> >
> > http://codereview.chromium.org/164031
> >
>

[fbarchard1, 2009-08-07 01:00:00]

kyle and I looked at this... I was thinking it would be like my audio_util
unittests.  But its complicated by the need for underlying IO.we kinda need
a unittest that actually plays movies.
the test itself if it just checks for zeros, is not all that interesting,
and unlikely to catch real world bugs.

so then we thought we could use ola_wav_test, kinda like I did on yuv
testing.  That would at least let us stress it on some real data and give it
reference files, run it with various parameters.  If its like YUV, it took
me a couple days to implement, but we could use the yuv unittest as a
starting point.
but then we looked at the code, and its internally streaming from file
still.  So its not super hard, but its not trivial/easy either.

also i think we should avoid testing implementation details, rather than the
behavior.  it should be possible to change implementations and still pass a
unittest.  something more like a layout test would be good here, if we could
play real data with settings and sample the final audio out.

so in conclusion, we think its a lot of work for little benefit and would
rather not do it at this time.
I suggest a TODO: be added.

On Thu, Aug 6, 2009 at 5:16 PM, <kylep@chromium.org> wrote:

> How exactly should I write said test... pass in buffers full of random
> data, set the rate to something low or high, call some FillBuffers and
> make sure I get 0s? Is there an easy way to fill a DataBuffer with
> garbage?
>
>
> On 2009/08/06 21:55:37, fbarchard wrote:
>
>> don't be evil :-)yes it would be possible.  At EA we had a saying -
>>
> nothing
>
>> in the game is impossible... its just highly unlikely.
>> okay its not __that__ hard.  but we're definitely not gonna measure
>>
> the
>
>> electricity usage!
>>
>
>  On Thu, Aug 6, 2009 at 12:23 PM, <scherkus@chromium.org> wrote:
>>
>
>  > wait would it be possible to have a unit test for this change?
>> >
>> > asserting that below a certain rate we get zeros
>> >
>> >
>> > http://codereview.chromium.org/164031
>> >
>>
>
>
>
>
> http://codereview.chromium.org/164031
>

[scherkus (not reviewing), 2009-08-10 22:06:22]

without getting into testing philosophies here, we can forgo the tests

I think you wanted to update the min/max values, though

http://codereview.chromium.org/164031/diff/1008/1009
File media/filters/audio_renderer_algorithm_ola.cc (right):

http://codereview.chromium.org/164031/diff/1008/1009#newcode20
Line 20: // TODO(kylep): These should really be limited further for slow-down.
OLA sounds
I believe you wanted to update these values

[kylep, 2009-08-10 22:44:37]

So... commit when this turns green?

http://codereview.chromium.org/164031/diff/1008/1009
File media/filters/audio_renderer_algorithm_ola.cc (right):

http://codereview.chromium.org/164031/diff/1008/1009#newcode20
Line 20: // TODO(kylep): These should really be limited further for slow-down.
OLA sounds
On 2009/08/10 22:06:22, scherkus wrote:
> I believe you wanted to update these values

Done.

[scherkus (not reviewing), 2009-08-10 22:50:53]

LGTM!