Fix openal NaCl port.

Fix openal NaCl port.

The example code was only decoding the first few openAl
buffers and then just playing the same buffer in loop.
This was due to MinBufferSizeInBytes being less than the
PPAPI buffer size which could lead to the buffer being
too empty for the reader (PPAPI) and not empty enough for 
writer.

This change consists of:
 - go-between buffer size takes into account PPAPI
   buffer size as well as openAL buffer size.
 - having PPAPI callback buffer whatever samples are in
   the buffer, even if there are not enough

BUG=None
TEST=run openal-ogg example app

Committed: https://code.google.com/p/naclports/source/detail?r=699

[bradn, 2013-01-30 22:08:32]

lgtm

[nfullagar1, 2013-01-30 22:19:31]

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
File libraries/openal-soft/nacl-openal-soft.patch (right):

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:153: +   
pthread_mutex_unlock(&data->mutex);
Putting locks in the audio callback is usually not recommended.  If the lock is
held elsewhere, the scheduler might swap this thread out, and the deadline for
this callback might be missed, causing a glitch in low latency buffers.  Even
through this lock is taken at the end (after data has been copied) it still
might glitch depending on thread quantum and when the next callback needs to
fire.  A lock-free solution would be preferable, see gcc's __sync_schronize
intrinsic as a possible workaround to updating the read_ptr without using a
pthread mutex.

[Sam Clegg, 2013-01-30 22:53:41]

On 2013/01/30 22:19:31, nfullagar1 wrote:
>
https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
> File libraries/openal-soft/nacl-openal-soft.patch (right):
> 
>
https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
> libraries/openal-soft/nacl-openal-soft.patch:153: +   
> pthread_mutex_unlock(&data->mutex);
> Putting locks in the audio callback is usually not recommended.  If the lock
is
> held elsewhere, the scheduler might swap this thread out, and the deadline for
> this callback might be missed, causing a glitch in low latency buffers.  Even
> through this lock is taken at the end (after data has been copied) it still
> might glitch depending on thread quantum and when the next callback needs to
> fire.  A lock-free solution would be preferable, see gcc's __sync_schronize
> intrinsic as a possible workaround to updating the read_ptr without using a
> pthread mutex.

In this case they is almost no contention for the lock.  The lock
here is only uses in order to send the pthread_signal and neither
thread holds onto ti.

I can't think of any other way to notify the writer thread that
reading is complete other than:

a) having the writer spin-wait (old implemenation)
b) having the reader use a pthread_signal (new implementation).

So... given that neither thread holds the lock for any significant
amount of time..  what do you think?

[Sam Clegg, 2013-01-30 22:56:28]

On 2013/01/30 22:53:41, Sam Clegg wrote:
> On 2013/01/30 22:19:31, nfullagar1 wrote:
> >
>
https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
> > File libraries/openal-soft/nacl-openal-soft.patch (right):
> > 
> >
>
https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
> > libraries/openal-soft/nacl-openal-soft.patch:153: +   
> > pthread_mutex_unlock(&data->mutex);
> > Putting locks in the audio callback is usually not recommended.  If the lock
> is
> > held elsewhere, the scheduler might swap this thread out, and the deadline
for
> > this callback might be missed, causing a glitch in low latency buffers. 
Even
> > through this lock is taken at the end (after data has been copied) it still
> > might glitch depending on thread quantum and when the next callback needs to
> > fire.  A lock-free solution would be preferable, see gcc's __sync_schronize
> > intrinsic as a possible workaround to updating the read_ptr without using a
> > pthread mutex.
> 
> In this case they is almost no contention for the lock.  The lock
> here is only uses in order to send the pthread_signal and neither
> thread holds onto ti.
> 
> I can't think of any other way to notify the writer thread that
> reading is complete other than:
> 
> a) having the writer spin-wait (old implemenation)
> b) having the reader use a pthread_signal (new implementation).
> 
> So... given that neither thread holds the lock for any significant
> amount of time..  what do you think?

Also, in the old method the writer is waking up every millisecond
rather than every PPAPI callback interval.. which will create more
CPU contention.

[Sam Clegg, 2013-01-30 23:47:33]

+elijahtaylor (the original author)

[elijahtaylor1, 2013-01-31 00:32:24]

Please wait for L_GTM from nfullagar from mutex comments, but in general I'm ok
with these changes.

Please fix commit message:

"- co-between buffer size takes into account PPAPI 
buffer size as will as openAL buffer size."

Do you mean go-between? also "well"

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
File libraries/openal-soft/nacl-openal-soft.patch (right):

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:127: +        buffer_size_in_bytes
= available_bytes;
If you're going to fill in part of the buffer, you might want to zero out the
rest?

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:173: +    const ALuint
MinBufferSizeInBytes = max(SampleFrameInBytes,
Did you mean (SampleFrameInBytes*2, UpdateSizeInBytes)?

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:191: +            // untill there
are more then MinBufferSizeInBytes available.
until

than

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:199: +                    /*
Spilled over the edge, copy the last bits to the beginning
80 cols.  I know it's annoying because there might be 2 extra chars because it's
a patch

[Sam Clegg, 2013-01-31 01:10:30]

I think I'm going to split the pthread part of this change
into a seperate CL until we can determine if it is really
a step in the right direction.

[Sam Clegg, 2013-01-31 01:11:14]

+mseaborn (for his input on locks/signal/schedulers).

[Sam Clegg, 2013-01-31 17:13:45]

Removed thread communication changes.

PTAL.

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
File libraries/openal-soft/nacl-openal-soft.patch (right):

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:127: +        buffer_size_in_bytes
= available_bytes;
On 2013/01/31 00:32:24, elijahtaylor1 wrote:
> If you're going to fill in part of the buffer, you might want to zero out the
> rest?

Done.

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:153: +   
pthread_mutex_unlock(&data->mutex);
On 2013/01/30 22:19:31, nfullagar1 wrote:
> Putting locks in the audio callback is usually not recommended.  If the lock
is
> held elsewhere, the scheduler might swap this thread out, and the deadline for
> this callback might be missed, causing a glitch in low latency buffers.  Even
> through this lock is taken at the end (after data has been copied) it still
> might glitch depending on thread quantum and when the next callback needs to
> fire.  A lock-free solution would be preferable, see gcc's __sync_schronize
> intrinsic as a possible workaround to updating the read_ptr without using a
> pthread mutex.

Removed the pthread stuff from this change.

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:173: +    const ALuint
MinBufferSizeInBytes = max(SampleFrameInBytes,
On 2013/01/31 00:32:24, elijahtaylor1 wrote:
> Did you mean (SampleFrameInBytes*2, UpdateSizeInBytes)?

Done.

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:191: +            // untill there
are more then MinBufferSizeInBytes available.
On 2013/01/31 00:32:24, elijahtaylor1 wrote:
> until
> 
> than

Reverted this stuff until we can find a non-busy solution
that we like.

https://codereview.chromium.org/12114013/diff/4001/libraries/openal-soft/nacl...
libraries/openal-soft/nacl-openal-soft.patch:199: +                    /*
Spilled over the edge, copy the last bits to the beginning
On 2013/01/31 00:32:24, elijahtaylor1 wrote:
> 80 cols.  I know it's annoying because there might be 2 extra chars because
it's
> a patch




Reverted this stuff until we can find a non-busy solution
that we like.

[nfullagar1, 2013-01-31 18:58:50]

optional suggestion otherwise lgtm...

https://codereview.chromium.org/12114013/diff/11003/libraries/openal-soft/nac...
File libraries/openal-soft/nacl-openal-soft.patch (right):

https://codereview.chromium.org/12114013/diff/11003/libraries/openal-soft/nac...
libraries/openal-soft/nacl-openal-soft.patch:124: +        /* Zero the part of
the buffer that we cannot fill */
Might sound slightly better to replicate the last known sample than zero-fill.

[nfullagar1, 2013-01-31 19:05:36]

Also, please update the CL description - still has the pthread changes listed.