Simplified AudioOutputStream interface.

Simplified AudioOutputStream interface.

1. Removed packet_size parameter from Open().
2. Removed OnClose() from the callback. Now the callback is guaranteed to be called only between Start() and Stop().
3. Added samples_per_packet in the AudioParameters struct.

BUG=39825
TEST=Unittests

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

[Sergey Ulanov, 2010-11-08 20:28:07]

This is one more (and probably last) cleanup before fixing 39825.

[scherkus (not reviewing), 2010-11-09 02:28:31]

lots of nits but overall looking pretty good

high level things:
  - AudioParameters can probably compute packet size instead of having
duplicated code
  - Consider passing by const-ref
  - File bugs for cleanup work

http://codereview.chromium.org/4661001/diff/8001/9006
File chrome/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/4661001/diff/8001/9006#newcode260
chrome/renderer/media/audio_renderer_impl.cc:260:
params.params.samples_per_packet = 0;
is this documented anywhere in params or IPC message?

http://codereview.chromium.org/4661001/diff/8001/9009
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/4661001/diff/8001/9009#newcode34
media/audio/audio_input_controller.cc:34: if (!params.IsValid() ||
single line

http://codereview.chromium.org/4661001/diff/8001/9013
File media/audio/audio_io.h (right):

http://codereview.chromium.org/4661001/diff/8001/9013#newcode71
media/audio/audio_io.h:71: // Open the stream.
so a lot of the comments went away

does it not apply anymore or can we at least be a bit more descriptive here? 
what does returning true/false mean? etc

http://codereview.chromium.org/4661001/diff/8001/9019
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/4661001/diff/8001/9019#newcode36
media/audio/audio_parameters.h:36: int samples_per_packet;  // Size of a packet
in frames.
nit: i'd realign these comments

http://codereview.chromium.org/4661001/diff/8001/9021
File media/audio/fake_audio_input_stream.h (right):

http://codereview.chromium.org/4661001/diff/8001/9021#newcode23
media/audio/fake_audio_input_stream.h:23: static AudioInputStream*
MakeFakeStream(AudioParameters params);
I'm tempted to say make all these functions use const AudioParameters& params

(it's a whole 20 bytes being allocated on the stack!!!!!!!!!!!)

http://codereview.chromium.org/4661001/diff/8001/9023
File media/audio/fake_audio_output_stream.cc (right):

http://codereview.chromium.org/4661001/diff/8001/9023#newcode14
media/audio/fake_audio_output_stream.cc:14: AudioOutputStream*
FakeAudioOutputStream::MakeFakeStream(AudioParameters params) {
>80 chars

http://codereview.chromium.org/4661001/diff/8001/9024
File media/audio/fake_audio_output_stream.h (right):

http://codereview.chromium.org/4661001/diff/8001/9024#newcode34
media/audio/fake_audio_output_stream.h:34: FakeAudioOutputStream(AudioParameters
params);
explicit

http://codereview.chromium.org/4661001/diff/8001/9028
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/4661001/diff/8001/9028#newcode30
media/audio/linux/alsa_output.h:30: // to the same thread), so it doesn't need
to be thread-safe anymore.
can you file Mstone-X clean up bugs for these two?

http://codereview.chromium.org/4661001/diff/8001/9029
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/4661001/diff/8001/9029#newcode107
media/audio/linux/alsa_output_unittest.cc:107: channels, kTestFramesPerPacket);
nit: can fit on single line

http://codereview.chromium.org/4661001/diff/8001/9029#newcode110
media/audio/linux/alsa_output_unittest.cc:110: AlsaPcmOutputStream*
CreateStream(int channels,
nit: can fit on single line

http://codereview.chromium.org/4661001/diff/8001/9029#newcode260
media/audio/linux/alsa_output_unittest.cc:260: test_stream_ =
CreateStream(kTestChannels, (int)kPacketFramesInMinLatency);
no c-style casts please (or change type of constant, or change type of
parameter)

http://codereview.chromium.org/4661001/diff/8001/9036
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/4661001/diff/8001/9036#newcode71
media/audio/mac/audio_output_mac.cc:71: packet_size_ = params.samples_per_packet
* params.channels *
this seems to be duplicated a bit... can packet_size_ be made a
function/computed property of AudioParameters?

[Sergey Ulanov, 2010-11-09 22:29:58]

Added GetPacketSize() in AudioParameters and filed a bug for cleanup work. Added
TODO to switch to const refs for AudioParameters.

http://codereview.chromium.org/4661001/diff/8001/chrome/renderer/media/audio_...
File chrome/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/4661001/diff/8001/chrome/renderer/media/audio_...
chrome/renderer/media/audio_renderer_impl.cc:260:
params.params.samples_per_packet = 0;
On 2010/11/09 02:28:31, scherkus wrote:
> is this documented anywhere in params or IPC message?
Added comments about it in render_messages_params.h

http://codereview.chromium.org/4661001/diff/8001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:34: if (!params.IsValid() ||
On 2010/11/09 02:28:31, scherkus wrote:
> single line

Done.

http://codereview.chromium.org/4661001/diff/8001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/audio_io.h#newco...
media/audio/audio_io.h:71: // Open the stream.
On 2010/11/09 02:28:31, scherkus wrote:
> so a lot of the comments went away
> 
> does it not apply anymore or can we at least be a bit more descriptive here? 
> what does returning true/false mean? etc

The comments about |packet_size| do not apply anymore because we don't have this
parameter. The paragraph about AUDIO_PCM_LOW_LATENCY was invalid because it was
true only for Windows, on mac and linux you can't create stream with
AUDIO_PCM_LOW_LATENCY (BTW, do we need AUDIO_PCM_LOW_LATENCY at all? It doesn't
seem to be used anywhere)
I've added more comments here and above.

http://codereview.chromium.org/4661001/diff/8001/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/audio_parameters...
media/audio/audio_parameters.h:36: int samples_per_packet;  // Size of a packet
in frames.
On 2010/11/09 02:28:31, scherkus wrote:
> nit: i'd realign these comments

Done.

http://codereview.chromium.org/4661001/diff/8001/media/audio/fake_audio_input...
File media/audio/fake_audio_input_stream.h (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/fake_audio_input...
media/audio/fake_audio_input_stream.h:23: static AudioInputStream*
MakeFakeStream(AudioParameters params);
On 2010/11/09 02:28:31, scherkus wrote:
> I'm tempted to say make all these functions use const AudioParameters& params
> 
> (it's a whole 20 bytes being allocated on the stack!!!!!!!!!!!)
It is not any different from passing 5 parameters for this call, as we used to. 
There are many other places where AudioParameters are passed by value, so I
think it is better to fix this in a separate change. I've added todo in
audio_manager.h

http://codereview.chromium.org/4661001/diff/8001/media/audio/fake_audio_outpu...
File media/audio/fake_audio_output_stream.cc (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/fake_audio_outpu...
media/audio/fake_audio_output_stream.cc:14: AudioOutputStream*
FakeAudioOutputStream::MakeFakeStream(AudioParameters params) {
On 2010/11/09 02:28:31, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/4661001/diff/8001/media/audio/fake_audio_outpu...
File media/audio/fake_audio_output_stream.h (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/fake_audio_outpu...
media/audio/fake_audio_output_stream.h:34: FakeAudioOutputStream(AudioParameters
params);
On 2010/11/09 02:28:31, scherkus wrote:
> explicit

Done.

http://codereview.chromium.org/4661001/diff/8001/media/audio/linux/alsa_output.h
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/linux/alsa_outpu...
media/audio/linux/alsa_output.h:30: // to the same thread), so it doesn't need
to be thread-safe anymore.
On 2010/11/09 02:28:31, scherkus wrote:
> can you file Mstone-X clean up bugs for these two?

Opened bug 62588

http://codereview.chromium.org/4661001/diff/8001/media/audio/linux/alsa_outpu...
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/linux/alsa_outpu...
media/audio/linux/alsa_output_unittest.cc:107: channels, kTestFramesPerPacket);
On 2010/11/09 02:28:31, scherkus wrote:
> nit: can fit on single line

Done.

http://codereview.chromium.org/4661001/diff/8001/media/audio/linux/alsa_outpu...
media/audio/linux/alsa_output_unittest.cc:110: AlsaPcmOutputStream*
CreateStream(int channels,
On 2010/11/09 02:28:31, scherkus wrote:
> nit: can fit on single line

Done.

http://codereview.chromium.org/4661001/diff/8001/media/audio/linux/alsa_outpu...
media/audio/linux/alsa_output_unittest.cc:260: test_stream_ =
CreateStream(kTestChannels, (int)kPacketFramesInMinLatency);
On 2010/11/09 02:28:31, scherkus wrote:
> no c-style casts please (or change type of constant, or change type of
> parameter)

Done.

http://codereview.chromium.org/4661001/diff/8001/media/audio/mac/audio_output...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/4661001/diff/8001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:71: packet_size_ = params.samples_per_packet
* params.channels *
On 2010/11/09 02:28:31, scherkus wrote:
> this seems to be duplicated a bit... can packet_size_ be made a
> function/computed property of AudioParameters?

Done.

[scherkus (not reviewing), 2010-11-11 01:25:31]

LGTM w/ tiny nits

thanks for doing this sergey!

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_manager.h...
media/audio/audio_manager.h:38: // will be ready to be filled in the call to
AudioSourceCallback::OnMoreData().
nit: >80 chars

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_parameter...
media/audio/audio_parameters.h:31: int GetPacketSize() const;
could you add some documentation to these two methods?

doesn't have to be much.. one line sentence would do :)

[Sergey Ulanov, 2010-11-11 03:06:24]

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_manager.h...
media/audio/audio_manager.h:38: // will be ready to be filled in the call to
AudioSourceCallback::OnMoreData().
On 2010/11/11 01:25:32, scherkus wrote:
> nit: >80 chars

Done.

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/4661001/diff/22001/media/audio/audio_parameter...
media/audio/audio_parameters.h:31: int GetPacketSize() const;
On 2010/11/11 01:25:32, scherkus wrote:
> could you add some documentation to these two methods?
> 
> doesn't have to be much.. one line sentence would do :)

Done.