AudioSharedBuffer

A simple buffer struct for synchronizing audio buses on separate processes 
without needing to block.

So far, |AudioInputBuffer|'s |AudioInputBufferParameters| are used for such a purpose.
This struct would potentially introduce more reusability and make more sense to what's 
actually being done for synchronization. 

Example use case:
=================

Delivering audio data from renderer to browser using |SyncSocket| and |SharedMemory| 
without blocking the audio capture thread on the renderer.

1. Renderer reads the |buffer_index| from shared memory and compares to local counter.
If matched, continue, otherwise try to recover (just store to FIFO) or raise an error event.

2. Renderer gets audio data on the audio capture thread, stores it to a FIFO
and releases it to the output bus on the shared memory. Signals the client
(browser) the data is ready via |SyncSocket::Send(...)| and increments local buffer
counter.

3. Browser receives on SyncSocket, consumes the shared audio bus and increments
the |buffer_index| by 1 in shared memory.

Both counters matching indicate everything is in sync, with additional counting feature.
Blocking is done only on the separate browser thread. 
Out of sync is detected by mismatched counters.

---

[burnik, 2014-09-01 14:16:47]

burnik@chromium.org changed reviewers:
+ dalecurtis@chromium.org, tommi@chromium.org, xians@chromium.org

[burnik, 2014-09-01 14:20:59]

Please take a look.

[no longer working on chromium, 2014-09-01 14:38:53]

On 2014/09/01 14:20:59, burnik wrote:
> Please take a look.


I am going to defer this CL to Dale to decide if we should make a new struct in
audio_parameters.h.

[no longer working on chromium, 2014-09-01 14:39:12]

xians@chromium.org changed reviewers:
- xians@chromium.org

[DaleCurtis, 2014-09-03 21:53:23]

I don't follow what problem this is solving? What do you mean by blocking the
capturer thread? The thread only wakes up when there is something to capture,
no?

[burnik, 2014-09-04 08:46:59]

To answer about blocking: 
The point is that if we call SyncSocket's |Receive(...)| on the capture thread
of the renderer - e.g. in |OnData()|, that would block and be generally bad.
That's why shared memory is used to synchronize (e.g. count buffers on both
ends). 

This new struct is very suitable for this kind of sync and the name indicates it
is to be shared by processes (browser & renderer).



On 2014/09/03 21:53:23, DaleCurtis wrote:
> I don't follow what problem this is solving? What do you mean by blocking the
> capturer thread? The thread only wakes up when there is something to capture,
> no?

[DaleCurtis, 2014-09-04 18:14:58]

It only blocks if there's nothing to do though right?  If it blocks there's no
data in the shared memory for consumption IIUC.

[burnik, 2014-09-04 18:42:52]

On 2014/09/04 18:14:58, DaleCurtis wrote:
> It only blocks if there's nothing to do though right?  If it blocks there's no
> data in the shared memory for consumption IIUC.

The |Receive(...)| is generally a blocking call. 

If the other process (browser) is not sending, this would make the capture
thread wait forever and effectively make the renderer unresponsive or more
likely make the capture thread occasionally wait and potentially miss a buffer -
or even worse, depending on the input source impl (e.g. overflow a FIFO)... 

This is why I don't receive on the renderer, I just check the |buffer_index|
which is increased by the other process (browser) since it lives in shared
memory.  I check it against a |local_buffer_index| maintained on the renderer
too make sure they match before more more data is pushed to the browser.

I just use |Send(...)| on the renderer to signal a new buffer is ready to be
consumed.

[DaleCurtis, 2014-09-04 19:24:42]

I must be missing something fundamental about what you're trying to do... As I
understand it, the capture thread (renderer side) does nothing when there are no
buffers available; it blocks in Receive() as such.  The blocked read is
optimally power-efficient since it gives cycles to other threads that a simple
while() loop wouldn't.

On the browser side, we notify the renderer with a receive() for each available
buffer via a Send(). Each Send() results in a single wakeup of receive.

The only way I can understand this is, is if you are trying to batch recieves
when possible to avoid going back to the blocking Recieve when outstanding
buffers are available.

[burnik, 2014-09-04 20:05:04]

On 2014/09/04 19:24:42, DaleCurtis wrote:
> I must be missing something fundamental about what you're trying to do... As I
> understand it, the capture thread (renderer side) does nothing when there are
no
> buffers available; it blocks in Receive() as such.  The blocked read is
> optimally power-efficient since it gives cycles to other threads that a simple
> while() loop wouldn't.
> 
> On the browser side, we notify the renderer with a receive() for each
available
> buffer via a Send(). Each Send() results in a single wakeup of receive.
> 
> The only way I can understand this is, is if you are trying to batch recieves
> when possible to avoid going back to the blocking Recieve when outstanding
> buffers are available.

Browser has a thread which blocks with Receive(). Renderer does not.

One should not make a blocking call on a capture thread - not to block other
sinks on that same thread.
That's why there is no Receive on the renderer.

Renderer on the capture thread only issues a Send() when the data is ready on
the shared memory audio bus.
This send is complemented by a Receive() on the Browser (a thread which is
allowed to block). Browser then increments the |buffer_index| which lives in
shared memory.

--- There is no Browser.Send() nor Renderer.Receive() ---

Renderer, next time an |OnData(...)| is called on the capture thread, looks to
see if the |buffer_index| which lives in shared memory (and should have been
incremented by the browser once received a signal) matches the
|local_buffer_index| we (the renderer) incremented once the last |Send()| was
issued. If they don't match - we're already out of sync - therefore we
accumulate to FIFO or do an error callback.

Please take a look at draft diagrams on pages 2 & 3 if it's still unclear:
http://goo.gl/9Ot3PC

Thx.

[tommi (sloooow) - chröme, 2014-09-04 20:17:21]

(caveat: I haven't fully grokked everything on this thread yet)

This scheme sounds pretty fragile to me.  How are underruns handled and
what about time-of-check-to-time-of-use issues? E.g. if there are queued up
notifications from the renderer in the socket and the browser increments
the counter right after the renderer checks it and they go out of sync?



On Thu, Sep 4, 2014 at 10:05 PM, <burnik@chromium.org> wrote:

> On 2014/09/04 19:24:42, DaleCurtis wrote:
>
>> I must be missing something fundamental about what you're trying to do...
>> As I
>> understand it, the capture thread (renderer side) does nothing when there
>> are
>>
> no
>
>> buffers available; it blocks in Receive() as such.  The blocked read is
>> optimally power-efficient since it gives cycles to other threads that a
>> simple
>> while() loop wouldn't.
>>
>
>  On the browser side, we notify the renderer with a receive() for each
>>
> available
>
>> buffer via a Send(). Each Send() results in a single wakeup of receive.
>>
>
>  The only way I can understand this is, is if you are trying to batch
>> recieves
>> when possible to avoid going back to the blocking Recieve when outstanding
>> buffers are available.
>>
>
> Browser has a thread which blocks with Receive(). Renderer does not.
>
> One should not make a blocking call on a capture thread - not to block
> other
> sinks on that same thread.
> That's why there is no Receive on the renderer.
>
> Renderer on the capture thread only issues a Send() when the data is ready
> on
> the shared memory audio bus.
> This send is complemented by a Receive() on the Browser (a thread which is
> allowed to block). Browser then increments the |buffer_index| which lives
> in
> shared memory.
>
> --- There is no Browser.Send() nor Renderer.Receive() ---
>
> Renderer, next time an |OnData(...)| is called on the capture thread,
> looks to
> see if the |buffer_index| which lives in shared memory (and should have
> been
> incremented by the browser once received a signal) matches the
> |local_buffer_index| we (the renderer) incremented once the last |Send()|
> was
> issued. If they don't match - we're already out of sync - therefore we
> accumulate to FIFO or do an error callback.
>
> Please take a look at draft diagrams on pages 2 & 3 if it's still unclear:
> http://goo.gl/9Ot3PC
>
> Thx.
>
>
>
>
>
>
>
>
>
> https://codereview.chromium.org/526113002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2014-09-04 21:03:21]

On 2014/09/04 20:05:04, burnik wrote:
> On 2014/09/04 19:24:42, DaleCurtis wrote:
> > I must be missing something fundamental about what you're trying to do... As
I
> > understand it, the capture thread (renderer side) does nothing when there
are
> no
> > buffers available; it blocks in Receive() as such.  The blocked read is
> > optimally power-efficient since it gives cycles to other threads that a
simple
> > while() loop wouldn't.
> > 
> > On the browser side, we notify the renderer with a receive() for each
> available
> > buffer via a Send(). Each Send() results in a single wakeup of receive.
> > 
> > The only way I can understand this is, is if you are trying to batch
recieves
> > when possible to avoid going back to the blocking Recieve when outstanding
> > buffers are available.
> 
> Browser has a thread which blocks with Receive(). Renderer does not.

I don't understand, either your terminology is wrong or you misunderstand the
system; this is definitely not true:

https://chromium.googlesource.com/chromium/chromium/+/trunk/content/browser/r...

Note the Send() call, but no Receive()...

> 
> One should not make a blocking call on a capture thread - not to block other
> sinks on that same thread.
> That's why there is no Receive on the renderer.

Again, this is not true:

https://chromium.googlesource.com/chromium/chromium/+/trunk/media/audio/audio...

Note the Receive() call...

> 
> Renderer on the capture thread only issues a Send() when the data is ready on
> the shared memory audio bus.
> This send is complemented by a Receive() on the Browser (a thread which is
> allowed to block). Browser then increments the |buffer_index| which lives in
> shared memory.
> 
> --- There is no Browser.Send() nor Renderer.Receive() ---
> 
> Renderer, next time an |OnData(...)| is called on the capture thread, looks to
> see if the |buffer_index| which lives in shared memory (and should have been
> incremented by the browser once received a signal) matches the
> |local_buffer_index| we (the renderer) incremented once the last |Send()| was
> issued. If they don't match - we're already out of sync - therefore we
> accumulate to FIFO or do an error callback.
> 
> Please take a look at draft diagrams on pages 2 & 3 if it's still unclear:
> http://goo.gl/9Ot3PC

These diagrams are not correct as indicated above.  Are you planning to change
the code to reflect the diagrams?

> 
> Thx.

[chromium-reviews, 2014-09-05 08:10:42]

On Thu, Sep 4, 2014 at 11:03 PM, <dalecurtis@chromium.org> wrote:

> On 2014/09/04 20:05:04, burnik wrote:
>
>> On 2014/09/04 19:24:42, DaleCurtis wrote:
>> > I must be missing something fundamental about what you're trying to
>> do... As
>>
> I
>
>> > understand it, the capture thread (renderer side) does nothing when
>> there
>>
> are
>
>> no
>> > buffers available; it blocks in Receive() as such.  The blocked read is
>> > optimally power-efficient since it gives cycles to other threads that a
>>
> simple
>
>> > while() loop wouldn't.
>> >
>> > On the browser side, we notify the renderer with a receive() for each
>> available
>> > buffer via a Send(). Each Send() results in a single wakeup of receive.
>> >
>> > The only way I can understand this is, is if you are trying to batch
>>
> recieves
>
>> > when possible to avoid going back to the blocking Recieve when
>> outstanding
>> > buffers are available.
>>
>
>  Browser has a thread which blocks with Receive(). Renderer does not.
>>
>
> I don't understand, either your terminology is wrong or you misunderstand
> the
> system; this is definitely not true:
>
> https://chromium.googlesource.com/chromium/chromium/+/trunk/
> content/browser/renderer_host/media/audio_input_sync_writer.cc
>
> Note the Send() call, but no Receive()...


 I know of this. However, this is not what I'm describing.


>
>  One should not make a blocking call on a capture thread - not to block
>> other
>> sinks on that same thread.
>> That's why there is no Receive on the renderer.
>>
>
> Again, this is not true:
>
> https://chromium.googlesource.com/chromium/chromium/+/trunk/
> media/audio/audio_device_thread.cc
>
> Note the Receive() call...
>
>
This is the device thread and not a client which receives audio data as a
sink. Please read comment about OnData:
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...


>
>
>  Renderer on the capture thread only issues a Send() when the data is
>> ready on
>> the shared memory audio bus.
>> This send is complemented by a Receive() on the Browser (a thread which is
>> allowed to block). Browser then increments the |buffer_index| which lives
>> in
>> shared memory.
>>
>
>  --- There is no Browser.Send() nor Renderer.Receive() ---
>>
>
>  Renderer, next time an |OnData(...)| is called on the capture thread,
>> looks to
>> see if the |buffer_index| which lives in shared memory (and should have
>> been
>> incremented by the browser once received a signal) matches the
>> |local_buffer_index| we (the renderer) incremented once the last |Send()|
>> was
>> issued. If they don't match - we're already out of sync - therefore we
>> accumulate to FIFO or do an error callback.
>>
>
>  Please take a look at draft diagrams on pages 2 & 3 if it's still unclear:
>> http://goo.gl/9Ot3PC
>>
>
> These diagrams are not correct as indicated above.  Are you planning to
> change
> the code to reflect the diagrams?
>
>
I don't plan to change code of any existing cases of using the SyncSocket.

But see this example
https://codereview.chromium.org/510913005/patch/80001/90005

It's basically the same idea if you swap the renderer and the browser.
However, making the size 0 looks hacky as opposed to counting buffers.



>
>  Thx.
>>
>
>
> https://codereview.chromium.org/526113002/
>



-- 

Regards,
Kristijan Burnik,
SWE Intern

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2014-09-05 23:13:44]

Sorry, I don't follow any of this discussion.  I defer to tommi@ to assess this
more helpfully in your own timezone.

As a final note, the output side already uses synchronized counters, so you
should just be able to duplicate that solution instead of writing into the
shared memory like you are on your other CL.  Note my comment there that this
will fail with PPAPI / NaCl and must be resolved before committing.

[burnik, 2014-09-22 08:05:30]

tommi: Any new comments on this struct?

[tommi (sloooow) - chröme, 2014-09-22 20:31:36]

sorry,  I don't see much point in checking this struct in without any
functionality, tests etc :-/

[burnik, 2014-09-23 09:06:50]

On 2014/09/22 20:31:36, tommi wrote:
> sorry,  I don't see much point in checking this struct in without any
> functionality, tests etc :-/

Ok, please see CL http://crrev.com/594063002. See diff of patchset 2 against 1.
At least, the functionality would surely be there.