Introduce shared_memory_support media target for PPAPI

Introduce shared_memory_support media target for PPAPI.

Allows PPAPI to directly utilize shared memory utility code instead
of duplicating it and CHECK'ing for equivalency.

Required to eventually convert PPAPI to using an AudioBus and floats
in PPB_Audio_Shared::Run().  http://crbug.com/114700

BUG=123203
TEST=unittests


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

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

[DaleCurtis, 2012-08-14 04:31:51]

PTAL guys.  I'm not sure what I should be doing with the ppapi_shared_untrusted
gyp.

I need these changes to eventually switch PPAPI over to passing an AudioBus
across the shared memory. (WIP CL over at:
https://chromiumcodereview.appspot.com/10832285/)

I'm open to naming suggestions instead of shared_memory_support as well.

[DaleCurtis, 2012-08-14 04:34:10]

On 2012/08/14 04:31:51, DaleCurtis wrote:
> PTAL guys.  I'm not sure what I should be doing with the
ppapi_shared_untrusted
> gyp.
> 
> I need these changes to eventually switch PPAPI over to passing an AudioBus
> across the shared memory. (WIP CL over at:
> https://chromiumcodereview.appspot.com/10832285/)
> 
> I'm open to naming suggestions instead of shared_memory_support as well.

Also is it expected that ppapi_unittests blow up under ASAN?

[scherkus (not reviewing), 2012-08-14 19:27:33]

brettw: care to weigh in on PPAPI gyp dependencies + targets for untrusted code?

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
File media/audio/shared_memory_util.cc (right):

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
media/audio/shared_memory_util.cc:12: const uint32 kUnknownDataSize =
static_cast<uint32>(-1);
static

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
File media/audio/shared_memory_util.h (right):

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
media/audio/shared_memory_util.h:17: kPauseMark = -1
don't indent OR even better just one-line this

https://chromiumcodereview.appspot.com/10826296/diff/1/media/media.gyp
File media/media.gyp (right):

https://chromiumcodereview.appspot.com/10826296/diff/1/media/media.gyp#newcode15
media/media.gyp:15: 'target_name': 'shared_memory_support',
based on the contents I'd be tempted to say this is audio_support

[DaleCurtis, 2012-08-15 01:11:14]

Done. croger's suggested +nfullagar might be a better reviewer. brettw to cc.

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
File media/audio/shared_memory_util.cc (right):

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
media/audio/shared_memory_util.cc:12: const uint32 kUnknownDataSize =
static_cast<uint32>(-1);
On 2012/08/14 19:27:33, scherkus wrote:
> static

Done.

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
File media/audio/shared_memory_util.h (right):

https://chromiumcodereview.appspot.com/10826296/diff/1/media/audio/shared_mem...
media/audio/shared_memory_util.h:17: kPauseMark = -1
On 2012/08/14 19:27:33, scherkus wrote:
> don't indent OR even better just one-line this

Done.

https://chromiumcodereview.appspot.com/10826296/diff/1/media/media.gyp
File media/media.gyp (right):

https://chromiumcodereview.appspot.com/10826296/diff/1/media/media.gyp#newcode15
media/media.gyp:15: 'target_name': 'shared_memory_support',
On 2012/08/14 19:27:33, scherkus wrote:
> based on the contents I'd be tempted to say this is audio_support

I thought about that, but it's not even a fraction of the code we use for Audio
or even a fraction of the code that might belong in a target called
audio_support.

Everything in this target is intended to help interface with the shared memory
and audio_shared_memory_support seemed a bit long.

[nfullagar, 2012-08-15 18:01:56]

quick comment for NaCl:
I think you're going to want to also make changes for the current (but soon to
be deprecated) NaCl proxy as part of this check-in.  These files are in:
  trunk/chrome/src/ppapi/native_client/src/shared/ppapi_proxy/

The files of interest in this directory are:
  plugin_ppb_audio.cc
  plugin_ppb_audio.h
  browser_ppb_audio_rpc_server.cc

In browser_ppb_audio_rpc_server.cc there is trusted side code to duplicate the
shm handle before it is passed into the untrusted client.  (Note the size bump
when the handle is dup'd) plugin_ppb_audio.cc is untrusted side NaCl code that
receives the handles and includes the main loop for requesting audio buffers
from the application.  To test NaCl audio, I would recommend starting with
Bastion in the Chrome Web Store.

[DaleCurtis, 2012-08-15 19:11:50]

On 2012/08/15 18:01:56, nfullagar wrote:
> quick comment for NaCl:
> I think you're going to want to also make changes for the current (but soon to
> be deprecated) NaCl proxy as part of this check-in.  These files are in:
>   trunk/chrome/src/ppapi/native_client/src/shared/ppapi_proxy/
> 
> The files of interest in this directory are:
>   plugin_ppb_audio.cc
>   plugin_ppb_audio.h
>   browser_ppb_audio_rpc_server.cc
> 
> In browser_ppb_audio_rpc_server.cc there is trusted side code to duplicate the
> shm handle before it is passed into the untrusted client.  (Note the size bump
> when the handle is dup'd) plugin_ppb_audio.cc is untrusted side NaCl code that
> receives the handles and includes the main loop for requesting audio buffers
> from the application.  To test NaCl audio, I would recommend starting with
> Bastion in the Chrome Web Store.

Hmmm, can the NaCl build even depend on media.gyp? Note: this CL isn't changing
any functionality, so the existing NaCl proxy code should continue to work fine.

Though, depending on how deprecated you're talking about here (deleted?), we'll
need to figure out a way to get NaCl proxy using AudioBus when we switch to
float. In the interim that could just mean a naive or wholesale copy of AudioBus
functionality into NaCl.

[nfullagar, 2012-08-16 18:38:18]

You're going to want to port parts of media to NaCl for this CL anyway.  The new
NaCl proxy is based on Chrome IPC, and uses
ppapi/shared_impl/ppb_audio_shared.cc (PPB_Audio_Shared::Run()).  While we're
not up and running by default yet with the new proxy for NaCl, talking with
bbudge, it is compiling today on build bots.  I suspect this CL won't pass on
those bots until media/media.gyp:shared_memory_support is ported to NaCl.

On 2012/08/15 19:11:50, DaleCurtis wrote:
> On 2012/08/15 18:01:56, nfullagar wrote:
> > quick comment for NaCl:
> > I think you're going to want to also make changes for the current (but soon
to
> > be deprecated) NaCl proxy as part of this check-in.  These files are in:
> >   trunk/chrome/src/ppapi/native_client/src/shared/ppapi_proxy/
> > 
> > The files of interest in this directory are:
> >   plugin_ppb_audio.cc
> >   plugin_ppb_audio.h
> >   browser_ppb_audio_rpc_server.cc
> > 
> > In browser_ppb_audio_rpc_server.cc there is trusted side code to duplicate
the
> > shm handle before it is passed into the untrusted client.  (Note the size
bump
> > when the handle is dup'd) plugin_ppb_audio.cc is untrusted side NaCl code
that
> > receives the handles and includes the main loop for requesting audio buffers
> > from the application.  To test NaCl audio, I would recommend starting with
> > Bastion in the Chrome Web Store.
> 
> Hmmm, can the NaCl build even depend on media.gyp? Note: this CL isn't
changing
> any functionality, so the existing NaCl proxy code should continue to work
fine.
> 
> Though, depending on how deprecated you're talking about here (deleted?),
we'll
> need to figure out a way to get NaCl proxy using AudioBus when we switch to
> float. In the interim that could just mean a naive or wholesale copy of
AudioBus
> functionality into NaCl.

[DaleCurtis, 2012-08-16 23:32:43]

PTAL. This set compiles and runs naclbox demos fine.

http://codereview.chromium.org/10826296/diff/12001/media/audio/shared_memory_...
File media/audio/shared_memory_util.h (right):

http://codereview.chromium.org/10826296/diff/12001/media/audio/shared_memory_...
media/audio/shared_memory_util.h:28: MEDIA_EXPORT void
SetActualDataSizeInBytes(void* shared_memory_ptr,
Necessary since the NaCl version doesn't pass around base::SharedMemory.

http://codereview.chromium.org/10826296/diff/12001/media/media_untrusted.gyp
File media/media_untrusted.gyp (left):

http://codereview.chromium.org/10826296/diff/12001/media/media_untrusted.gyp#...
media/media_untrusted.gyp:11: 'ppapi_shared.gypi',
For some reason git cl upload thinks I moved this file from
ppapi/ppapi_shared_untrusted.gyp ???

http://codereview.chromium.org/10826296/diff/12001/ppapi/native_client/native...
File ppapi/native_client/native_client.gyp (right):

http://codereview.chromium.org/10826296/diff/12001/ppapi/native_client/native...
ppapi/native_client/native_client.gyp:189:
'../../base/base_untrusted.gyp:base_untrusted',
I don't know if adding these here is acceptable?

http://codereview.chromium.org/10826296/diff/12001/ppapi/native_client/src/sh...
File ppapi/native_client/src/shared/ppapi_proxy/DEPS (right):

http://codereview.chromium.org/10826296/diff/12001/ppapi/native_client/src/sh...
ppapi/native_client/src/shared/ppapi_proxy/DEPS:7: "+media",
??? Not sure if this is necessary.

http://codereview.chromium.org/10826296/diff/12001/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/plugin.gyp (right):

http://codereview.chromium.org/10826296/diff/12001/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/plugin.gyp:63:
'<(DEPTH)/media/media.gyp:shared_memory_support',
This might need base as well??? Should this be the untrusted version?

http://codereview.chromium.org/10826296/diff/12001/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/plugin.gyp:96:
'<(DEPTH)/media/media.gyp:shared_memory_support',
Ditto.

[nfullagar, 2012-08-17 00:52:07]

Some notes on the DEPS between NaCl and Chrome.  Summary from offline
discussions: we can add chrome's media directory to NaCl DEPS after this CL so
NaCl has everything in needs to build the plugin.  (WIP, soon only Chrome repo
will need to build NaCl plugin)

https://chromiumcodereview.appspot.com/10826296/diff/12001/media/audio/shared...
File media/audio/shared_memory_util.cc (right):

https://chromiumcodereview.appspot.com/10826296/diff/12001/media/audio/shared...
media/audio/shared_memory_util.cc:17: // in bytes. Actual data starts
immediately after it.
Comment above: ...last word is size of data in bytes.

https://chromiumcodereview.appspot.com/10826296/diff/12001/ppapi/native_clien...
File ppapi/native_client/native_client.gyp (right):

https://chromiumcodereview.appspot.com/10826296/diff/12001/ppapi/native_clien...
ppapi/native_client/native_client.gyp:189:
'../../base/base_untrusted.gyp:base_untrusted',
On 2012/08/16 23:32:43, DaleCurtis wrote:
> I don't know if adding these here is acceptable?
Same deps concern as before...

https://chromiumcodereview.appspot.com/10826296/diff/12001/ppapi/native_clien...
File ppapi/native_client/src/trusted/plugin/plugin.gyp (right):

https://chromiumcodereview.appspot.com/10826296/diff/12001/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/plugin.gyp:63:
'<(DEPTH)/media/media.gyp:shared_memory_support',
This will probably break when building the plugin from the nacl repo.  The nacl
repo DEPS' in only the parts of chrome it needs.  So... before this CL lands,
we'll want to add media directory to the nacl repo.  (the nacl repo has ppapi &
base already)

On 2012/08/16 23:32:43, DaleCurtis wrote:
> This might need base as well??? Should this be the untrusted version?

https://chromiumcodereview.appspot.com/10826296/diff/12001/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/plugin.gyp:96:
'<(DEPTH)/media/media.gyp:shared_memory_support',
as above
On 2012/08/16 23:32:43, DaleCurtis wrote:
> Ditto.

[DaleCurtis, 2012-08-17 02:27:15]

PTAL. I've put out: https://chromiumcodereview.appspot.com/10829369/ to handle
the NaCl side changes.

[DaleCurtis, 2012-08-17 02:27:45]

https://chromiumcodereview.appspot.com/10826296/diff/12001/media/audio/shared...
File media/audio/shared_memory_util.cc (right):

https://chromiumcodereview.appspot.com/10826296/diff/12001/media/audio/shared...
media/audio/shared_memory_util.cc:17: // in bytes. Actual data starts
immediately after it.
On 2012/08/17 00:52:07, nfullagar wrote:
> Comment above: ...last word is size of data in bytes.

Done.

[DaleCurtis, 2012-08-17 03:54:03]

On 2012/08/17 02:27:45, DaleCurtis wrote:
>
https://chromiumcodereview.appspot.com/10826296/diff/12001/media/audio/shared...
> File media/audio/shared_memory_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10826296/diff/12001/media/audio/shared...
> media/audio/shared_memory_util.cc:17: // in bytes. Actual data starts
> immediately after it.
> On 2012/08/17 00:52:07, nfullagar wrote:
> > Comment above: ...last word is size of data in bytes.
> 
> Done.

Argh, I can't seem to make all the bots happy with the current approach:
- Letting each target keep duplicates of shared source files make Android
unhappy.
- Putting shared source files in a single target means everyone needs to now
depend on the new target (no transitive dependency via media :(

Any ideas? Otherwise I'll just leave media.gyp alone and move the shared sources
into their own gyp file; probably a shared_memory_support.gyp which includes
both a trusted and untrusted target if that will work.

[DaleCurtis, 2012-08-17 21:54:36]

+jam for content/*.gyp changes.

PTAL.  I had to bite the bullet and switch to explicit dependencies, I was
unable to make both android and the linux shared library build happy with
duplicate file lists.

This new patch set passes all the bots and runs the NaCl version of Bastion from
the webstore perfectly as far as I can tell.

[jam, 2012-08-17 22:47:36]

lgtm

[nfullagar, 2012-08-17 22:51:16]

lgtm

[DaleCurtis, 2012-08-17 23:08:16]

On 2012/08/17 22:51:16, nfullagar wrote:
> lgtm

Thanks for review guys. Too big for this late on a Friday, will land Monday
after scherkus gets a chance to review.

[scherkus (not reviewing), 2012-08-17 23:35:21]

lgtm w/ one nit

http://codereview.chromium.org/10826296/diff/3057/media/shared_memory_support...
File media/shared_memory_support.gypi (right):

http://codereview.chromium.org/10826296/diff/3057/media/shared_memory_support...
media/shared_memory_support.gypi:11: 'audio/audio_parameters.cc',
this block is over-indented by 2 spaces

[DaleCurtis, 2012-08-20 16:51:13]

nfullagar: looks like you aren't in the OWNERS file for ppapi. 
brettw: can you sign off?

http://codereview.chromium.org/10826296/diff/3057/media/shared_memory_support...
File media/shared_memory_support.gypi (right):

http://codereview.chromium.org/10826296/diff/3057/media/shared_memory_support...
media/shared_memory_support.gypi:11: 'audio/audio_parameters.cc',
On 2012/08/17 23:35:21, scherkus wrote:
> this block is over-indented by 2 spaces

Done.

[brettw, 2012-08-20 17:24:36]

LGTM rubberstamp

[commit-bot: I haz the power, 2012-08-20 17:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10826296/16031

[commit-bot: I haz the power, 2012-08-20 17:56:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10826296/12015

[commit-bot: I haz the power, 2012-08-20 22:19:39]

Change committed as 152406

[DaleCurtis, 2012-08-20 23:52:05]

On 2012/08/20 22:19:39, I haz the power (commit-bot) wrote:
> Change committed as 152406

Reverted due to atomicops not compiling on ARM NaCl yet :( Need to wait for
http://codereview.chromium.org/10831358/ to land first.

[bbudge, 2012-08-21 00:10:25]

On 2012/08/20 23:52:05, DaleCurtis wrote:
> On 2012/08/20 22:19:39, I haz the power (commit-bot) wrote:
> > Change committed as 152406
> 
> Reverted due to atomicops not compiling on ARM NaCl yet :( Need to wait for
> http://codereview.chromium.org/10831358/ to land first.

That CL is going to grow as we fix other issues. I will split the atomicops
change into its own issue.

[dmichael (off chromium), 2012-08-23 21:15:52]

http://codereview.chromium.org/10826296/diff/26001/media/audio/shared_memory_...
File media/audio/shared_memory_util.h (right):

http://codereview.chromium.org/10826296/diff/26001/media/audio/shared_memory_...
media/audio/shared_memory_util.h:23: MEDIA_EXPORT uint32
PacketSizeSizeInBytes(uint32 shared_memory_created_size);
drive-by...  is this really meant to have SizeSize, or should it be
PacketSizeInBytes? The name is confusing to me.

[DaleCurtis, 2012-08-23 21:55:15]

http://codereview.chromium.org/10826296/diff/26001/media/audio/shared_memory_...
File media/audio/shared_memory_util.h (right):

http://codereview.chromium.org/10826296/diff/26001/media/audio/shared_memory_...
media/audio/shared_memory_util.h:23: MEDIA_EXPORT uint32
PacketSizeSizeInBytes(uint32 shared_memory_created_size);
On 2012/08/23 21:15:53, dmichael wrote:
> drive-by...  is this really meant to have SizeSize, or should it be
> PacketSizeInBytes? The name is confusing to me.

Hah, I didn't even notice that; it's a copy paste from the old version. I'll fix
the two callers to that function. Done.

[commit-bot: I haz the power, 2012-08-24 17:17:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10826296/30001

[commit-bot: I haz the power, 2012-08-24 17:17:30]

Failed to apply patch for media/media_untrusted.gyp:
While running patch -p1 --forward --force;
patching file media/media_untrusted.gyp
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file media/media_untrusted.gyp.rej

[commit-bot: I haz the power, 2012-08-24 17:22:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10826296/25002

[commit-bot: I haz the power, 2012-08-24 19:49:19]

Change committed as 153262