IPC to pass media data using a lock free circular fifo.

IPC to pass media data using a lock free circular fifo.

BUG=408189
Committed: https://crrev.com/17c61fd9779a3b83bea17b5989335130a108c269
Cr-Commit-Position: refs/heads/master@{#293451}

[damienv1, 2014-09-02 23:39:02]

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

[damienv1, 2014-09-02 23:39:16]

Please take a look. Thanks !

[xhwang, 2014-09-03 17:40:33]

It'd be nice if you could provide more big picture documentation on how the FIFO
works and typical workflow at the consumer and feeder sides.

Rubber stamp lgtm. I'd defer to the chromecast owners to finish the review.

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
File chromecast/media/cma/ipc/media_memory_chunk.h (right):

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_memory_chunk.h:13: class MediaMemoryChunk {
Add documentation.

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_memory_chunk.h:24: virtual bool valid() const =
0;
Document when this is useful.

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
File chromecast/media/cma/ipc/media_message.h (right):

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_message.h:37: // message, before allocating the
real message.
Is this the only use case? If so, why not having a function to get the size of
the message directly?

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
File chromecast/media/cma/ipc/media_message_fifo.h (right):

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_message_fifo.h:42: scoped_ptr<MediaMemoryChunk>
ReserveMemory(size_t size);
What if the memory of |size| cannot be reserved? Will NULL be returned?

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_message_fifo.h:95: base::ThreadChecker
thread_checker_;
Given the comments and w/o looking at the impl, I wonder what this checker is
checking?

[damienv1, 2014-09-03 23:59:06]

I added more comments on the media message fifo on how to use it (note that the
unit test is a good example).
There will also be a follow up CL to pass the actual DecoderBuffer
AudioDecoderConfig and VideoDecoderConfig thrugh the fifo.

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
File chromecast/media/cma/ipc/media_memory_chunk.h (right):

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_memory_chunk.h:13: class MediaMemoryChunk {
On 2014/09/03 17:40:33, xhwang wrote:
> Add documentation.

Done.

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_memory_chunk.h:24: virtual bool valid() const =
0;
On 2014/09/03 17:40:33, xhwang wrote:
> Document when this is useful.

Done.

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
File chromecast/media/cma/ipc/media_message.h (right):

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_message.h:37: // message, before allocating the
real message.
On 2014/09/03 17:40:33, xhwang wrote:
> Is this the only use case? If so, why not having a function to get the size of
> the message directly?

The size of the message is not known before all the fields of the message are
written.
A dummy message is used for that purpose: the user of MediaMessage will perform
some fake Write operations,
and once the size is known, will create a real message and replay all the Write
commands (on the real message this time).

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
File chromecast/media/cma/ipc/media_message_fifo.h (right):

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_message_fifo.h:42: scoped_ptr<MediaMemoryChunk>
ReserveMemory(size_t size);
On 2014/09/03 17:40:33, xhwang wrote:
> What if the memory of |size| cannot be reserved? Will NULL be returned?

Done.

https://codereview.chromium.org/529223003/diff/20001/chromecast/media/cma/ipc...
chromecast/media/cma/ipc/media_message_fifo.h:95: base::ThreadChecker
thread_checker_;
On 2014/09/03 17:40:33, xhwang wrote:
> Given the comments and w/o looking at the impl, I wonder what this checker is
> checking?

I updated the comment. Hope it's less confusing.

[lcwu1, 2014-09-04 01:21:52]

lgtm

https://codereview.chromium.org/529223003/diff/40001/chromecast/media/media.gyp
File chromecast/media/media.gyp (right):

https://codereview.chromium.org/529223003/diff/40001/chromecast/media/media.g...
chromecast/media/media.gyp:64: 'cma/ipc/media_message_fifo_unittest.cc',
nit: not in alphabetical order.

[damienv1, 2014-09-04 15:55:20]

https://codereview.chromium.org/529223003/diff/40001/chromecast/media/media.gyp
File chromecast/media/media.gyp (right):

https://codereview.chromium.org/529223003/diff/40001/chromecast/media/media.g...
chromecast/media/media.gyp:64: 'cma/ipc/media_message_fifo_unittest.cc',
On 2014/09/04 01:21:52, lcwu1 wrote:
> nit: not in alphabetical order.

Done.

[damienv1, 2014-09-05 02:38:25]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-09-05 02:40:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/damienv@chromium.org/529223003/100001

[commit-bot: I haz the power, 2014-09-05 05:16:14]

Committed patchset #6 (id:100001) as da3e8707ecf8b01d35daf4008ca98e7de3d8294c

[commit-bot: I haz the power, 2014-09-10 03:37:12]

Patchset 6 (id:??) landed as
https://crrev.com/17c61fd9779a3b83bea17b5989335130a108c269
Cr-Commit-Position: refs/heads/master@{#293451}