Fixes in AlsaPcmOutputStream.

Removed AlsaPcmOutputStrem::Packet. Fixed bug in AlsaPcmOutputStream::ScheduleNextWrite which would cause high CPU consumption.
BUG=28654
TEST=Audio still works on Linux

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

[Sergey Ulanov, 2010-05-11 01:57:12]

This is one more fix for the buffers bug. Also fixed CPU hogging with ALSA:
without the fix the browser process consumes 15% (a lot of writes are scheduled
with next_fill_time_ms = 0), with the fix - 5%.

[Sergey Ulanov, 2010-05-12 22:38:32]

ping.

[awong, 2010-05-12 23:45:57]

http://codereview.chromium.org/2008010/diff/5001/6001
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/2008010/diff/5001/6001#newcode403
media/audio/linux/alsa_output.cc:403: bytes_per_output_frame_ = should_downmix_
? 2 * bytes_per_sample_ :
I actually think the if-statement is easier to read.

http://codereview.chromium.org/2008010/diff/5001/6001#newcode481
media/audio/linux/alsa_output.cc:481: snd_pcm_sframes_t delay =
buffer_->forward_bytes() * bytes_per_frame_ /
are we certain about the order of operations?

http://codereview.chromium.org/2008010/diff/10002/12002#newcode625
media/audio/linux/alsa_output.cc:625: 
There's a difference between delay an available frames.  One is how long until
the frame hits the hardware (possibly includes hardware delay), and the other is
the number of spaces open in the alsa ring buffer.

We should be using available frames for calculating the next fill point, no?

[Sergey Ulanov, 2010-05-13 07:30:03]

http://codereview.chromium.org/2008010/diff/5001/6001
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/2008010/diff/5001/6001#newcode403
media/audio/linux/alsa_output.cc:403: bytes_per_output_frame_ = should_downmix_
? 2 * bytes_per_sample_ :
On 2010/05/12 23:45:57, awong wrote:
> I actually think the if-statement is easier to read.
bytes_per_output_frame_ wasn't initialized when should_downmix_ = true, that is
why I had to change this. Don't think this assignment deserves 5 lines of code
:)

http://codereview.chromium.org/2008010/diff/5001/6001#newcode481
media/audio/linux/alsa_output.cc:481: snd_pcm_sframes_t delay =
buffer_->forward_bytes() * bytes_per_frame_ /
On 2010/05/12 23:45:57, awong wrote:
> are we certain about the order of operations?
Multiply then divide, and that is what we need here.

http://codereview.chromium.org/2008010/diff/10002/12002#newcode625
media/audio/linux/alsa_output.cc:625: 
On 2010/05/12 23:45:57, awong wrote:
> There's a difference between delay an available frames.  One is how long until
> the frame hits the hardware (possibly includes hardware delay), and the other
is
> the number of spaces open in the alsa ring buffer.
Yes.

> 
> We should be using available frames for calculating the next fill point, no?
The old code calculates time when there will be enough  space for one packet in
alsa's buffer, problem is that nothing guarantees that this will happen before
buffer underrun. When we open alsa we specify latency to be double of the packet
size, but this doesn't guarantee that the ring buffer will be bigger than the
packet,  and that is why we needed Frank's hack (next_fill_time_ms /= 2). Even
with the hack i've seen buffer underruns.
My intention with the new code is to schedule next write to the middle of the
current alsa's buffer. After thinking more about it, I realized that using
snd_pcm_delay() here isn't right ether because delay includes hardware buffers,
and so underrun may still occur.
We should probably use snd_pcm_get_params() and snd_pcm_avail() to figure out
how much data is in the ring buffer.

[awong, 2010-05-13 11:14:06]

http://codereview.chromium.org/2008010/diff/10002/12002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/2008010/diff/10002/12002#newcode625
media/audio/linux/alsa_output.cc:625: sample_rate_);
On 2010/05/13 07:30:03, sergeyu wrote:
> On 2010/05/12 23:45:57, awong wrote:
> > There's a difference between delay an available frames.  One is how long
until
> > the frame hits the hardware (possibly includes hardware delay), and the
other
> is
> > the number of spaces open in the alsa ring buffer.
> Yes.
> 
> > 
> > We should be using available frames for calculating the next fill point, no?
> The old code calculates time when there will be enough  space for one packet
in
> alsa's buffer, problem is that nothing guarantees that this will happen before
> buffer underrun. When we open alsa we specify latency to be double of the
packet
> size, but this doesn't guarantee that the ring buffer will be bigger than the
> packet,  and that is why we needed Frank's hack (next_fill_time_ms /= 2). Even
> with the hack i've seen buffer underruns.
> My intention with the new code is to schedule next write to the middle of the
> current alsa's buffer. After thinking more about it, I realized that using
> snd_pcm_delay() here isn't right ether because delay includes hardware
buffers,
> and so underrun may still occur.
> We should probably use snd_pcm_get_params() and snd_pcm_avail() to figure out
> how much data is in the ring buffer.

Ah, interesting.

So, the problem manifests if the size of the ring buffer, minus the size of a
packet, is too small right?  And you're suggesting the fix is to call
snd_pcm_get_params in OpenTask, and use snd_pcm_avail() directly here (as
opposed to snd_pcm_avail_update) right?  That makes sense.

[fbarchard, 2010-05-13 17:01:22]

just minor stuff

http://codereview.chromium.org/2008010/diff/10002/12002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/2008010/diff/10002/12002#newcode443
media/audio/linux/alsa_output.cc:443: BufferPacket();
sanity check - should we still be doing preroll?

http://codereview.chromium.org/2008010/diff/10002/12002#newcode516
media/audio/linux/alsa_output.cc:516: Swizzle50Layout(packet->GetWritableData(),
packet_size);
note that this code is no longer correct.  But that should be a separate CL.

http://codereview.chromium.org/2008010/diff/10002/12006
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/2008010/diff/10002/12006#newcode71
media/base/seekable_buffer.h:71: bool GetCurrentChunk(const uint8** data,
size_t* size) const;
although it may be true you set all 3 values to 0, it would be more clear as an
API to just say one of these things.  Otherwise the user is unsure if they need
to check all 3 are zero.

[Sergey Ulanov, 2010-05-14 19:25:54]

http://codereview.chromium.org/2008010/diff/10002/12002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/2008010/diff/10002/12002#newcode443
media/audio/linux/alsa_output.cc:443: BufferPacket();
On 2010/05/13 17:01:22, fbarchard wrote:
> sanity check - should we still be doing preroll?
No, removed this code.

http://codereview.chromium.org/2008010/diff/10002/12002#newcode625
media/audio/linux/alsa_output.cc:625: sample_rate_);
Changed this code to use snd_pcm_get_params() to get size of the output buffer.

http://codereview.chromium.org/2008010/diff/10002/12006
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/2008010/diff/10002/12006#newcode71
media/base/seekable_buffer.h:71: bool GetCurrentChunk(const uint8** data,
size_t* size) const;
On 2010/05/13 17:01:22, fbarchard wrote:
> although it may be true you set all 3 values to 0, it would be more clear as
an
> API to just say one of these things.  Otherwise the user is unsure if they
need
> to check all 3 are zero.
> 

Done.

[scherkus (not reviewing), 2010-05-14 19:37:51]

nits

but overall change is LGTM

http://codereview.chromium.org/2008010/diff/25001/26001
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/2008010/diff/25001/26001#newcode651
media/audio/linux/alsa_output.cc:651: LOG(WARNING)<<next_fill_time_ms;
debugging code?

http://codereview.chromium.org/2008010/diff/30001/31007
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/2008010/diff/30001/31007#newcode72
media/base/seekable_buffer.h:72: bool GetCurrentChunk(const uint8** data,
size_t* size) const;
unit test, if possible

http://codereview.chromium.org/2008010/diff/34001/23005
File media/audio/linux/alsa_wrapper.cc (right):

http://codereview.chromium.org/2008010/diff/34001/23005#newcode48
media/audio/linux/alsa_wrapper.cc:48: int AlsaWrapper::PcmGetParams(snd_pcm_t*
handle, snd_pcm_uframes_t* buffer_size,
nit: try to keep function ordering the same (either update header file or this
file)

[awong, 2010-05-14 20:46:22]

LGTM with Andrew's comments.

On Fri, May 14, 2010 at 12:37 PM, <scherkus@chromium.org> wrote:

> nits
>
> but overall change is LGTM
>
>
> http://codereview.chromium.org/2008010/diff/25001/26001
>
> File media/audio/linux/alsa_output.cc (right):
>
> http://codereview.chromium.org/2008010/diff/25001/26001#newcode651
> media/audio/linux/alsa_output.cc:651: LOG(WARNING)<<next_fill_time_ms;
> debugging code?
>
> http://codereview.chromium.org/2008010/diff/30001/31007
>
> File media/base/seekable_buffer.h (right):
>
> http://codereview.chromium.org/2008010/diff/30001/31007#newcode72
> media/base/seekable_buffer.h:72: bool GetCurrentChunk(const uint8**
>
> data, size_t* size) const;
> unit test, if possible
>
> http://codereview.chromium.org/2008010/diff/34001/23005
> File media/audio/linux/alsa_wrapper.cc (right):
>
> http://codereview.chromium.org/2008010/diff/34001/23005#newcode48
> media/audio/linux/alsa_wrapper.cc:48: int
> AlsaWrapper::PcmGetParams(snd_pcm_t* handle, snd_pcm_uframes_t*
> buffer_size,
> nit: try to keep function ordering the same (either update header file
> or this file)
>
>
> http://codereview.chromium.org/2008010/show
>

[Sergey Ulanov, 2010-05-14 21:25:47]

http://codereview.chromium.org/2008010/diff/25001/26001
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/2008010/diff/25001/26001#newcode651
media/audio/linux/alsa_output.cc:651: LOG(WARNING)<<next_fill_time_ms;
On 2010/05/14 19:37:52, scherkus wrote:
> debugging code?
Removed

http://codereview.chromium.org/2008010/diff/30001/31007
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/2008010/diff/30001/31007#newcode72
media/base/seekable_buffer.h:72: bool GetCurrentChunk(const uint8** data,
size_t* size) const;
On 2010/05/14 19:37:52, scherkus wrote:
> unit test, if possible

Done.

http://codereview.chromium.org/2008010/diff/34001/23005
File media/audio/linux/alsa_wrapper.cc (right):

http://codereview.chromium.org/2008010/diff/34001/23005#newcode48
media/audio/linux/alsa_wrapper.cc:48: int AlsaWrapper::PcmGetParams(snd_pcm_t*
handle, snd_pcm_uframes_t* buffer_size,
On 2010/05/14 19:37:52, scherkus wrote:
> nit: try to keep function ordering the same (either update header file or this
> file)

Done.

[scherkus (not reviewing), 2010-05-14 21:28:50]

LGTM w/ one nit

http://codereview.chromium.org/2008010/diff/43001/44003
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/2008010/diff/43001/44003#newcode51
media/audio/linux/alsa_output_unittest.cc:51: MOCK_METHOD3(PcmGetParams,
int(snd_pcm_t* handle, snd_pcm_uframes_t* buffer_size,
>80 chars