Change the way audio mixer gets "pending bytes" (amount of data currently buffered

Change the way audio mixer gets "pending bytes" (amount of data currently buffered
but not yet played). Underlying code expects per-logical-stream pending bytes,
while all mixer gets when called for more data is pending bytes per combined stream.

Fix is to keep track of
* amount of data in every buffer
* buffers for every logical stream
and manually calculate per-logical-stream pending bytes.

That is last CL in initial audio mixer implementation, after it go through mixer
should be ready for full testing.

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

[enal1, 2012-04-25 18:17:40]

Victoria, can you please take a look?

[vrk (LEFT CHROMIUM), 2012-04-26 00:54:57]

Hey enal, I'm actually having trouble understanding how this is supposed to work
:( I'm thinking this code might make some Windows-only assumptions?

Here's my main question: In the beginning of OnMoreData(), you add a check to
see if we've gotten the same |dest| as in a previous call to OnMoreData(). What
is that supposed to signify?

In the linux implementation, you're always creating a new DataBuffer that
allocates new memory before handing that memory into OnMoreData(), so I don't
see how the |dest| pointers are meaningful.

https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_o...
File media/audio/audio_output_mixer.cc (right):

https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_o...
media/audio/audio_output_mixer.cc:164: // Did we see this buffer before?
For linux, I believe we will always have a unique value of |dest|. See
AlsaPcmOutputStream::BufferPacket() in alsa_output.cc, we create a new
DataBuffer each time before passing it to the callback.

[enal1, 2012-04-26 01:22:59]

Oops, I checked Windows and Mac implementations, but not Linux one. Will do
tomorrow.

On Wed, Apr 25, 2012 at 5:54 PM, <vrk@chromium.org> wrote:

> Hey enal, I'm actually having trouble understanding how this is supposed
> to work
> :( I'm thinking this code might make some Windows-only assumptions?
>
> Here's my main question: In the beginning of OnMoreData(), you add a check
> to
> see if we've gotten the same |dest| as in a previous call to OnMoreData().
> What
> is that supposed to signify?
>
> In the linux implementation, you're always creating a new DataBuffer that
> allocates new memory before handing that memory into OnMoreData(), so I
> don't
> see how the |dest| pointers are meaningful.
>
>
> https://chromiumcodereview.**appspot.com/10154007/diff/**
>
3006/media/audio/audio_output_**mixer.cc<https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_output_mixer.cc>
> File media/audio/audio_output_**mixer.cc (right):
>
> https://chromiumcodereview.**appspot.com/10154007/diff/**
>
3006/media/audio/audio_output_**mixer.cc#newcode164<https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_output_mixer.cc#newcode164>
> media/audio/audio_output_**mixer.cc:164: // Did we see this buffer before?
> For linux, I believe we will always have a unique value of |dest|. See
> AlsaPcmOutputStream::**BufferPacket() in alsa_output.cc, we create a new
> DataBuffer each time before passing it to the callback.
>
>
https://chromiumcodereview.**appspot.com/10154007/<https://chromiumcodereview...
>

[enal1, 2012-04-26 16:38:43]

Victoria,

Ok, it's easy to fix AlsaPcmOutputStream, but before doing that I want to
explain what the problem is and how I am trying to solve it.

Problem happens on platforms where we have several buffers -- on Windows
and Mac. There code expects that SyncReader will send "pending bytes" --
amount of data currently buffered but not yet played -- to the renderer.

On Linux it is always zero, look at the AlsaPcmOutputStream::BufferPacket()
-- it calculates
    uint32 buffer_delay = buffer_->forward_bytes() * bytes_per_frame_ /
        bytes_per_output_frame_;
but at this moment buffer_->forward_bytes() is always zero.

So let's assume we have N buffers. Mixer does not know how many buffers we
have, that information platform-specific, but even on single platform
number of buffers may depend on type of stream (e.g. on Windows we have 2
for low-latency stream, 3 for normal stream).

So we called mixer to fill the buffer. How can it get per-proxy pending
bytes?

Suggested solution  contains of 2 parts:
* for each buffer we keep amount of data in it, i.e. we have map<buffer,
length>.
* for each proxy we remember all "in-flight" buffers containing data from
that proxy.

This way, in each OnMoreData() call, we can update "pending bytes" for each
proxy -- we know how many bytes were in the buffer given to us that
supposedly is consumed now, and which proxies are affected. We also know
how many bytes we wrote to buffer, and next OnMoreData() call would use
that.

As you rightly observed, idea works only if set of buffers is allocated
before playback started. It is right way to do things anyways -- preferable
buffers should be marked as non-swappable, as they are accessed by
real-time thread. On both Mac and Windows corresponding sound API does that.

Thanks,
Eugene

PS. I send rough idea to you on Sunday, and got full implementation
yesterday
PPS. There is also "hardware delay bytes" -- bytes that we feed to the OS
and not yet fully played, used to determine when we can send "ended" event.
That may be Ok in the initial implementation, but it is extremely hard to
get it from all OSes we are supporting, and we are not sending it to
renderer in low-latency mode. So I got rid of our dependence on them,
replacing by timing dependency -- we know how many bytes we have in the
stream, so we know earliest time it may finish. That is one more code
cleanup, at some moment we should get rid of them in AudioBufferState (not
now, I want be sure that timing mechanism is adequate).

On Wed, Apr 25, 2012 at 6:22 PM, Eugene Nalimov <enal@google.com> wrote:

> Oops, I checked Windows and Mac implementations, but not Linux one. Will
> do tomorrow.
>
> On Wed, Apr 25, 2012 at 5:54 PM, <vrk@chromium.org> wrote:
>
>> Hey enal, I'm actually having trouble understanding how this is supposed
>> to work
>> :( I'm thinking this code might make some Windows-only assumptions?
>>
>> Here's my main question: In the beginning of OnMoreData(), you add a
>> check to
>> see if we've gotten the same |dest| as in a previous call to
>> OnMoreData(). What
>> is that supposed to signify?
>>
>> In the linux implementation, you're always creating a new DataBuffer that
>> allocates new memory before handing that memory into OnMoreData(), so I
>> don't
>> see how the |dest| pointers are meaningful.
>>
>>
>> https://chromiumcodereview.**appspot.com/10154007/diff/**
>>
3006/media/audio/audio_output_**mixer.cc<https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_output_mixer.cc>
>> File media/audio/audio_output_**mixer.cc (right):
>>
>> https://chromiumcodereview.**appspot.com/10154007/diff/**
>>
3006/media/audio/audio_output_**mixer.cc#newcode164<https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_output_mixer.cc#newcode164>
>> media/audio/audio_output_**mixer.cc:164: // Did we see this buffer
>> before?
>> For linux, I believe we will always have a unique value of |dest|. See
>> AlsaPcmOutputStream::**BufferPacket() in alsa_output.cc, we create a new
>> DataBuffer each time before passing it to the callback.
>>
>>
https://chromiumcodereview.**appspot.com/10154007/<https://chromiumcodereview...
>>
>
>

[enal1, 2012-04-26 17:39:49]

...the more I am looking at the code the more I want to add "release
buffer" call to the AudioSourceCallback. That will remove some implicit
assumptions.

Will send updated CL for review today.

Thanks,
Eugene

On Thu, Apr 26, 2012 at 9:38 AM, Eugene Nalimov <enal@google.com> wrote:

> Victoria,
>
> Ok, it's easy to fix AlsaPcmOutputStream, but before doing that I want to
> explain what the problem is and how I am trying to solve it.
>
> Problem happens on platforms where we have several buffers -- on Windows
> and Mac. There code expects that SyncReader will send "pending bytes" --
> amount of data currently buffered but not yet played -- to the renderer.
>
> On Linux it is always zero, look at
> the AlsaPcmOutputStream::BufferPacket() -- it calculates
>     uint32 buffer_delay = buffer_->forward_bytes() * bytes_per_frame_ /
>         bytes_per_output_frame_;
> but at this moment buffer_->forward_bytes() is always zero.
>
> So let's assume we have N buffers. Mixer does not know how many buffers we
> have, that information platform-specific, but even on single platform
> number of buffers may depend on type of stream (e.g. on Windows we have 2
> for low-latency stream, 3 for normal stream).
>
> So we called mixer to fill the buffer. How can it get per-proxy pending
> bytes?
>
> Suggested solution  contains of 2 parts:
> * for each buffer we keep amount of data in it, i.e. we have map<buffer,
> length>.
> * for each proxy we remember all "in-flight" buffers containing data from
> that proxy.
>
> This way, in each OnMoreData() call, we can update "pending bytes" for
> each proxy -- we know how many bytes were in the buffer given to us that
> supposedly is consumed now, and which proxies are affected. We also know
> how many bytes we wrote to buffer, and next OnMoreData() call would use
> that.
>
> As you rightly observed, idea works only if set of buffers is allocated
> before playback started. It is right way to do things anyways -- preferable
> buffers should be marked as non-swappable, as they are accessed by
> real-time thread. On both Mac and Windows corresponding sound API does that.
>
> Thanks,
> Eugene
>
> PS. I send rough idea to you on Sunday, and got full implementation
> yesterday
> PPS. There is also "hardware delay bytes" -- bytes that we feed to the OS
> and not yet fully played, used to determine when we can send "ended" event.
> That may be Ok in the initial implementation, but it is extremely hard to
> get it from all OSes we are supporting, and we are not sending it to
> renderer in low-latency mode. So I got rid of our dependence on them,
> replacing by timing dependency -- we know how many bytes we have in the
> stream, so we know earliest time it may finish. That is one more code
> cleanup, at some moment we should get rid of them in AudioBufferState (not
> now, I want be sure that timing mechanism is adequate).
>
> On Wed, Apr 25, 2012 at 6:22 PM, Eugene Nalimov <enal@google.com> wrote:
>
>> Oops, I checked Windows and Mac implementations, but not Linux one. Will
>> do tomorrow.
>>
>> On Wed, Apr 25, 2012 at 5:54 PM, <vrk@chromium.org> wrote:
>>
>>> Hey enal, I'm actually having trouble understanding how this is supposed
>>> to work
>>> :( I'm thinking this code might make some Windows-only assumptions?
>>>
>>> Here's my main question: In the beginning of OnMoreData(), you add a
>>> check to
>>> see if we've gotten the same |dest| as in a previous call to
>>> OnMoreData(). What
>>> is that supposed to signify?
>>>
>>> In the linux implementation, you're always creating a new DataBuffer that
>>> allocates new memory before handing that memory into OnMoreData(), so I
>>> don't
>>> see how the |dest| pointers are meaningful.
>>>
>>>
>>> https://chromiumcodereview.**appspot.com/10154007/diff/**
>>>
3006/media/audio/audio_output_**mixer.cc<https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_output_mixer.cc>
>>> File media/audio/audio_output_**mixer.cc (right):
>>>
>>> https://chromiumcodereview.**appspot.com/10154007/diff/**
>>>
3006/media/audio/audio_output_**mixer.cc#newcode164<https://chromiumcodereview.appspot.com/10154007/diff/3006/media/audio/audio_output_mixer.cc#newcode164>
>>> media/audio/audio_output_**mixer.cc:164: // Did we see this buffer
>>> before?
>>> For linux, I believe we will always have a unique value of |dest|. See
>>> AlsaPcmOutputStream::**BufferPacket() in alsa_output.cc, we create a new
>>> DataBuffer each time before passing it to the callback.
>>>
>>>
https://chromiumcodereview.**appspot.com/10154007/<https://chromiumcodereview...
>>>
>>
>>
>

[vrk (LEFT CHROMIUM), 2012-04-26 20:27:54]

Thanks for the explanation! I have a better idea of what you're trying to do
now, and it is a clever idea.

I am thinking, though, that there is an underlying problem we can solve to
simplify the code and make our lives easier for the future.

"Pending bytes" is not the most helpful piece of information to be propagated
from the physical stream to its clients via the OnMoreData() callback. No one
actually cares about the pending bytes, but rather the renderer will take that
value and do some math to convert pending bytes into useful information.

We're always converting pending bytes into the "audio delay ms," or when is the
soonest that the next bytes you send to the device will actually get played. Why
don't we hand that information back through OnMoreData() instead of pending
bytes?

That would be pretty big change, but I think it would improve the code in many
ways and should make it easier to do things like mix streams of different sample
rates.

What do you think? We should chat about this offline as well.

[enal1, 2012-04-27 17:11:10]

I believe I now handle all the cases. Main idea is simple -- if proxy's pending
bytes are the same as pending bytes for combined stream, both are either
pre-buffering or in the steady state. In either case new pending bytes for proxy
is the same as new pending bytes for combined stream.

On 2012/04/26 20:27:54, Victoria Kirst wrote:
> Thanks for the explanation! I have a better idea of what you're trying to do
> now, and it is a clever idea.
> 
> I am thinking, though, that there is an underlying problem we can solve to
> simplify the code and make our lives easier for the future.
> 
> "Pending bytes" is not the most helpful piece of information to be propagated
> from the physical stream to its clients via the OnMoreData() callback. No one
> actually cares about the pending bytes, but rather the renderer will take that
> value and do some math to convert pending bytes into useful information.
> 
> We're always converting pending bytes into the "audio delay ms," or when is
the
> soonest that the next bytes you send to the device will actually get played.
Why
> don't we hand that information back through OnMoreData() instead of pending
> bytes?
> 
> That would be pretty big change, but I think it would improve the code in many
> ways and should make it easier to do things like mix streams of different
sample
> rates.
> 
> What do you think? We should chat about this offline as well.

[vrk (LEFT CHROMIUM), 2012-04-30 20:12:19]

LGTM

Thanks enal! 

(The clean-up I mentioned in #6 can be done in another CL, and possibly by
someone else! I'll make a bug and follow up there.)

[commit-bot: I haz the power, 2012-04-30 20:14:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10154007/18001

[commit-bot: I haz the power, 2012-04-30 21:48:58]

Try job failure for 10154007-18001 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-01 02:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10154007/18001

[commit-bot: I haz the power, 2012-05-01 04:10:30]

Change committed as 134675