Audio/video data streaming over shared memory.

Audio/video data streaming over shared memory.

BUG=408189
Committed: https://crrev.com/8abccfdebebc44e9e7301da87ac99e13503f7dce
Cr-Commit-Position: refs/heads/master@{#294391}

[damienv1, 2014-09-10 15:44:30]

damienv@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org, scherkus@chromium.org,
xhwang@chromium.org

[damienv1, 2014-09-10 16:18:44]

PTAL. Thanks !

[gunsch, 2014-09-10 18:51:38]

A few largely stylistic comments, overall structure OK

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/audio_decoder_config_marshaller.h
(right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/audio_decoder_config_marshaller.h:14: class
AudioDecoderConfigMarshaller {
it looks like there's no plan for this to be expanded as a larger class of its
own---does it makes sense to have this be a class, or should these methods just
be top-level helper methods in chromecast::media? (particularly now that the cma
code isn't just in the "media" namespace)

Same comments apply to other marshaller classes.

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/audio_decoder_config_marshaller.h:18: const
::media::AudioDecoderConfig& config, MediaMessage* msg);
four-space indent

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/av_streamer_unittest.cc (right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/av_streamer_unittest.cc:203: size_t
frame_count = 100;
Chromium review picked up the diff between this and
BufferingFrameProviderTest... so why not "const" here? :)

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/coded_frame_provider_host.cc (right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/coded_frame_provider_host.cc:84: LOG(FATAL) <<
"Unknown media message";
NOTREACHED

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.h (right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.h:20: const
scoped_refptr<DecoderBufferBase>& buffer, MediaMessage* msg);
nit: four spaces

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/video_decoder_config_marshaller.h
(right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/video_decoder_config_marshaller.h:18: const
::media::VideoDecoderConfig& config, MediaMessage* msg);
four-space indent

[damienv1, 2014-09-10 21:24:55]

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/audio_decoder_config_marshaller.h
(right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/audio_decoder_config_marshaller.h:14: class
AudioDecoderConfigMarshaller {
On 2014/09/10 18:51:37, gunsch wrote:
> it looks like there's no plan for this to be expanded as a larger class of its
> own---does it makes sense to have this be a class, or should these methods
just
> be top-level helper methods in chromecast::media? (particularly now that the
cma
> code isn't just in the "media" namespace)
> 
> Same comments apply to other marshaller classes.

I wanted to group functions in the same way chrome ipc ParamTraits does (which
is also a set of static functions).
I plan to keep the same scheme.

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/audio_decoder_config_marshaller.h:18: const
::media::AudioDecoderConfig& config, MediaMessage* msg);
On 2014/09/10 18:51:37, gunsch wrote:
> four-space indent

Done.

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/av_streamer_unittest.cc (right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/av_streamer_unittest.cc:203: size_t
frame_count = 100;
On 2014/09/10 18:51:37, gunsch wrote:
> Chromium review picked up the diff between this and
> BufferingFrameProviderTest... so why not "const" here? :)

Done.

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/coded_frame_provider_host.cc (right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/coded_frame_provider_host.cc:84: LOG(FATAL) <<
"Unknown media message";
On 2014/09/10 18:51:37, gunsch wrote:
> NOTREACHED

No. Entering this use case might mean the renderer process has been compromised
(and the attacker tries to send invalid data over IPC).
In order to avoid any other potential security issue, we'd rather crash the
browser process (just like we use CHECK when we validate the audio codec in
AudioDecoderConfigMarshaller for e.g.).

I added a comment though.

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.h (right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.h:20: const
scoped_refptr<DecoderBufferBase>& buffer, MediaMessage* msg);
On 2014/09/10 18:51:38, gunsch wrote:
> nit: four spaces

Done.

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
File chromecast/media/cma/ipc_streamer/video_decoder_config_marshaller.h
(right):

https://codereview.chromium.org/557333003/diff/1/chromecast/media/cma/ipc_str...
chromecast/media/cma/ipc_streamer/video_decoder_config_marshaller.h:18: const
::media::VideoDecoderConfig& config, MediaMessage* msg);
On 2014/09/10 18:51:38, gunsch wrote:
> four-space indent

Done.

[damienv1, 2014-09-11 00:59:00]

PTAL.
Thanks !

[lcwu1, 2014-09-11 02:35:59]

lgtm

[gunsch, 2014-09-11 03:09:48]

On 2014/09/11 02:35:59, lcwu1 wrote:
> lgtm

lgtm

[gunsch, 2014-09-11 03:09:53]

https://codereview.chromium.org/557333003/diff/20001/chromecast/media/cma/ipc...
File chromecast/media/cma/ipc_streamer/av_streamer_unittest.cc (left):

https://codereview.chromium.org/557333003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc_streamer/av_streamer_unittest.cc:62:
DCHECK_GE(frame_count, 1u);
Nit: I realize it's not this CL, but I think this should be ASSERT_LE rather
than DCHECK_LE

[damienv1, 2014-09-11 15:08:11]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-09-11 15:09:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557333003/20001

[commit-bot: I haz the power, 2014-09-11 15:37:48]

Committed patchset #2 (id:20001) as 026e2c4996d40e13955bfe46eb52d1f7f4e21cd4

[commit-bot: I haz the power, 2014-09-11 15:48:23]

Patchset 2 (id:??) landed as
https://crrev.com/8abccfdebebc44e9e7301da87ac99e13503f7dce
Cr-Commit-Position: refs/heads/master@{#294391}