Chromecast: IPC messages for browser-based media pipeline.

Chromecast: IPC messages for browser-based media pipeline.

R=xhwang@chromium.org,damienv@chromium.org,tsepez@chromium.org
BUG=408189
Committed: https://crrev.com/a2d67bde611f7b1653195298603529178860af6e
Cr-Commit-Position: refs/heads/master@{#308751}

[gunsch, 2014-12-12 01:13:40]

@damienv: PTAL at the code

@tsepez: PTAL for security review. Added the security OWNERS list for new IPC
messages.

@xhwang: added a media/base DEPS from chromecast/common. CL details are mostly
FYI

[damienv1, 2014-12-12 02:30:17]

lgtm % nits

https://codereview.chromium.org/797793004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/797793004/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:436: 'cast_shell_cma',
Would rather name it: cast_media_ipc (just like we have cast_media)

https://codereview.chromium.org/797793004/diff/1/chromecast/common/media/cma_...
File chromecast/common/media/cma_param_traits.cc (right):

https://codereview.chromium.org/797793004/diff/1/chromecast/common/media/cma_...
chromecast/common/media/cma_param_traits.cc:64: !ParamTraits<bool>::Read(m,
iter, &is_encrypted))
nit: Curly braces needed (multiple line if statement)

https://codereview.chromium.org/797793004/diff/1/chromecast/common/media/cma_...
chromecast/common/media/cma_param_traits.cc:118: !ParamTraits<bool>::Read(m,
iter, &is_encrypted))
ditto

[gunsch, 2014-12-12 22:40:53]

https://codereview.chromium.org/797793004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/797793004/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:436: 'cast_shell_cma',
On 2014/12/12 02:30:16, damienv1 wrote:
> Would rather name it: cast_media_ipc (just like we have cast_media)

Renamed to cast_shell_media --- the intent for this target is to include all the
content-side code for the pipeline (both the browser/renderer sides), not just
the IPC code.

https://codereview.chromium.org/797793004/diff/1/chromecast/common/media/cma_...
File chromecast/common/media/cma_param_traits.cc (right):

https://codereview.chromium.org/797793004/diff/1/chromecast/common/media/cma_...
chromecast/common/media/cma_param_traits.cc:118: !ParamTraits<bool>::Read(m,
iter, &is_encrypted))
On 2014/12/12 02:30:16, damienv1 wrote:
> ditto

Done.

https://codereview.chromium.org/797793004/diff/1/chromecast/common/media/cma_...
chromecast/common/media/cma_param_traits.cc:118: !ParamTraits<bool>::Read(m,
iter, &is_encrypted))
On 2014/12/12 02:30:16, damienv1 wrote:
> ditto

Done.

[Tom Sepez, 2014-12-12 22:58:42]

https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
File chromecast/common/media/cma_param_traits.cc (right):

https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
chromecast/common/media/cma_param_traits.cc:16:
IPC_ENUM_TRAITS_MIN_MAX_VALUE(media::AudioCodec,
This should  be in your messages .h file.

https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
chromecast/common/media/cma_param_traits.cc:36: void
ParamTraits<media::AudioDecoderConfig>::Write(
Your life would be a lot easier if these were POD, in which case IPC could
generate most of this for you from macros.

[Tom Sepez, 2014-12-12 23:03:47]

When adding new message files, you should also add your entry to 

  src/tools/ipc_fuzzer/message_lib/all_messages.h

and then try the following by hand:

GYP_DEFINES=enable_ipc_fuzzer=1; gclient runhooks
ninja -C out/Release ipclist ipc_fuzzer_generate

Ping me if this fails, and I'll dig into it.

[gunsch, 2014-12-16 02:39:32]

On 2014/12/12 23:03:47, Tom Sepez wrote:
> When adding new message files, you should also add your entry to 
> 
>   src/tools/ipc_fuzzer/message_lib/all_messages.h
> 
> and then try the following by hand:
> 
> GYP_DEFINES=enable_ipc_fuzzer=1; gclient runhooks
> ninja -C out/Release ipclist ipc_fuzzer_generate
> 
> Ping me if this fails, and I'll dig into it.

Not able to add to all_messages.h. Looks like enum generation for some of the
media primitives are sort of scattered right now, so we end up with duplicates.
See:

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/vie...
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

It's not appropriate for chromecast/ to be including messages from content/
(these definitions don't seem to be included from any content/public headers),
but ipc_fuzzer's all_messages.h includes content/common/all_messages.h.

I've filed crbug.com/442594 in the meantime to follow this up with a IPC header
cleanup of the media/base primitives.

[gunsch, 2014-12-16 02:39:41]

https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
File chromecast/common/media/cma_param_traits.cc (right):

https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
chromecast/common/media/cma_param_traits.cc:16:
IPC_ENUM_TRAITS_MIN_MAX_VALUE(media::AudioCodec,
On 2014/12/12 22:58:42, Tom Sepez wrote:
> This should  be in your messages .h file.

Done.

https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
chromecast/common/media/cma_param_traits.cc:36: void
ParamTraits<media::AudioDecoderConfig>::Write(
On 2014/12/12 22:58:42, Tom Sepez wrote:
> Your life would be a lot easier if these were POD, in which case IPC could
> generate most of this for you from macros.

Agreed, but as media:: primitives we don't currently have a lot of choice

xhwang: any thoughts?

[gunsch, 2014-12-16 16:17:16]

gunsch@chromium.org changed required reviewers:
+ damienv@chromium.org, tsepez@chromium.org, xhwang@chromium.org

[xhwang, 2014-12-16 19:54:15]

On 2014/12/16 02:39:41, gunsch wrote:
>
https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
> File chromecast/common/media/cma_param_traits.cc (right):
> 
>
https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
> chromecast/common/media/cma_param_traits.cc:16:
> IPC_ENUM_TRAITS_MIN_MAX_VALUE(media::AudioCodec,
> On 2014/12/12 22:58:42, Tom Sepez wrote:
> > This should  be in your messages .h file.
> 
> Done.
> 
>
https://codereview.chromium.org/797793004/diff/20001/chromecast/common/media/...
> chromecast/common/media/cma_param_traits.cc:36: void
> ParamTraits<media::AudioDecoderConfig>::Write(
> On 2014/12/12 22:58:42, Tom Sepez wrote:
> > Your life would be a lot easier if these were POD, in which case IPC could
> > generate most of this for you from macros.
> 
> Agreed, but as media:: primitives we don't currently have a lot of choice
> 
> xhwang: any thoughts?

Yeah, I don't think we should modify media C++ classes for simpler IPC macros.
The IPC layer should be transparent to media code.

FYI, hopefully we will use mojo to replace these IPCs in the future, see:
https://codereview.chromium.org/712463004/. Note that in mojo we still need to
serialize the configs, which is done here:
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

[Tom Sepez, 2014-12-16 21:18:48]

Messages LGTM

[xhwang, 2014-12-16 23:58:14]

DEPS change LGTM!

[gunsch, 2014-12-17 00:07:20]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-12-17 00:08:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797793004/60001

[commit-bot: I haz the power, 2014-12-17 00:11:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-17 00:11:34]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gunsch, 2014-12-17 05:53:31]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-12-17 05:54:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797793004/80001

[commit-bot: I haz the power, 2014-12-17 06:49:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-17 06:50:28]

Patchset 5 (id:??) landed as
https://crrev.com/a2d67bde611f7b1653195298603529178860af6e
Cr-Commit-Position: refs/heads/master@{#308751}