Modify the PPB_Audio_Shared code for NaCl.

Modify the PPB_Audio_Shared code for NaCl.
BUG=116317
TEST=none

This adds code for the NaCl untrusted build of the PPAPI proxy. The NaCl IRT can't
create threads that call back into user code, so the Audio thread must be created
using a special NaCl API.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148423

[bbudge, 2012-07-24 20:10:46]

http://codereview.chromium.org/10809079/diff/7001/ppapi/proxy/plugin_main_nac...
File ppapi/proxy/plugin_main_nacl.cc (right):

http://codereview.chromium.org/10809079/diff/7001/ppapi/proxy/plugin_main_nac...
ppapi/proxy/plugin_main_nacl.cc:106:
ppapi::PPB_Audio_Shared::SetThreadFunctions(thread_functions);
We have to call down to shared code rather than the other way around because of
DEPS layering.

[dmichael (off chromium), 2012-07-24 20:55:21]

http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
ppapi/shared_impl/ppb_audio_shared.cc:16: PP_ThreadFunctions thread_functions;
Later on, you're relying on it getting zero initialized because it has static
storage duration...  see C++98 3.6.2.1:
http://www-d0.fnal.gov/~dladams/cxx_standard.pdf
Is this worth a comment, or maybe using = {NULL, NULL} to make it clearer?

http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
ppapi/shared_impl/ppb_audio_shared.cc:114: memset(shared_memory_->memory(), 0,
shared_memory_size_);
I'd rather the comment said why we're doing this. Do we need to do this at all?
If you're trying to avoid leaking data, the clearing should really happen in
trusted code.

[bbudge, 2012-07-24 21:15:44]

http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
ppapi/shared_impl/ppb_audio_shared.cc:16: PP_ThreadFunctions thread_functions;
On 2012/07/24 20:55:21, dmichael wrote:
> Later on, you're relying on it getting zero initialized because it has static
> storage duration...  see C++98 3.6.2.1:
> http://www-d0.fnal.gov/%7Edladams/cxx_standard.pdf
> Is this worth a comment, or maybe using = {NULL, NULL} to make it clearer?

I'll add a comment. Done.

http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
ppapi/shared_impl/ppb_audio_shared.cc:114: memset(shared_memory_->memory(), 0,
shared_memory_size_);
On 2012/07/24 20:55:21, dmichael wrote:
> I'd rather the comment said why we're doing this. Do we need to do this at
all?
> If you're trying to avoid leaking data, the clearing should really happen in
> trusted code.

I copied this from the SRPC proxy code. I agree, it's not sufficient to do this
here. Removed.

[nfullagar, 2012-07-24 21:28:43]

I believe the clearing was some defensive programming - if the callback isn't
fired in time (for what ever reason, maybe the audio thread didn't spin up fast
enough), the audio back end will scoop out what ever is in the shm and send it
out the speaker.  Clearing the buffer here might avoid a burst of static if the
buffer isn't cleared elsewhere.

On 2012/07/24 21:15:44, bbudge1 wrote:
>
http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
> File ppapi/shared_impl/ppb_audio_shared.cc (right):
> 
>
http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
> ppapi/shared_impl/ppb_audio_shared.cc:16: PP_ThreadFunctions thread_functions;
> On 2012/07/24 20:55:21, dmichael wrote:
> > Later on, you're relying on it getting zero initialized because it has
static
> > storage duration...  see C++98 3.6.2.1:
> > http://www-d0.fnal.gov/%257Edladams/cxx_standard.pdf
> > Is this worth a comment, or maybe using = {NULL, NULL} to make it clearer?
> 
> I'll add a comment. Done.
> 
>
http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
> ppapi/shared_impl/ppb_audio_shared.cc:114: memset(shared_memory_->memory(), 0,
> shared_memory_size_);
> On 2012/07/24 20:55:21, dmichael wrote:
> > I'd rather the comment said why we're doing this. Do we need to do this at
> all?
> > If you're trying to avoid leaking data, the clearing should really happen in
> > trusted code.
> 
> I copied this from the SRPC proxy code. I agree, it's not sufficient to do
this
> here. Removed.

[bbudge, 2012-07-24 23:07:24]

On 2012/07/24 21:28:43, nfullagar wrote:
> I believe the clearing was some defensive programming - if the callback isn't
> fired in time (for what ever reason, maybe the audio thread didn't spin up
fast
> enough), the audio back end will scoop out what ever is in the shm and send it
> out the speaker.  Clearing the buffer here might avoid a burst of static if
the
> buffer isn't cleared elsewhere.
> 
> On 2012/07/24 21:15:44, bbudge1 wrote:
> >
>
http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
> > File ppapi/shared_impl/ppb_audio_shared.cc (right):
> > 
> >
>
http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
> > ppapi/shared_impl/ppb_audio_shared.cc:16: PP_ThreadFunctions
thread_functions;
> > On 2012/07/24 20:55:21, dmichael wrote:
> > > Later on, you're relying on it getting zero initialized because it has
> static
> > > storage duration...  see C++98 3.6.2.1:
> > > http://www-d0.fnal.gov/%25257Edladams/cxx_standard.pdf
> > > Is this worth a comment, or maybe using = {NULL, NULL} to make it clearer?
> > 
> > I'll add a comment. Done.
> > 
> >
>
http://codereview.chromium.org/10809079/diff/7001/ppapi/shared_impl/ppb_audio...
> > ppapi/shared_impl/ppb_audio_shared.cc:114: memset(shared_memory_->memory(),
0,
> > shared_memory_size_);
> > On 2012/07/24 20:55:21, dmichael wrote:
> > > I'd rather the comment said why we're doing this. Do we need to do this at
> > all?
> > > If you're trying to avoid leaking data, the clearing should really happen
in
> > > trusted code.
> > 
> > I copied this from the SRPC proxy code. I agree, it's not sufficient to do
> this
> > here. Removed.

I added it back with a comment.

[dmichael (off chromium), 2012-07-25 03:12:26]

lgtm

http://codereview.chromium.org/10809079/diff/13007/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10809079/diff/13007/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:48: thread_id_(),
It would be clearer to put 0 in explicitly.

[bbudge, 2012-07-25 12:04:37]

http://codereview.chromium.org/10809079/diff/13007/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

http://codereview.chromium.org/10809079/diff/13007/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_shared.cc:48: thread_id_(),
On 2012/07/25 03:12:26, dmichael wrote:
> It would be clearer to put 0 in explicitly.

I copied it from the SRPC code because I thought it nicely captured the
"don't-care" nature of this. OK. Done.