Use data pipe for encoded buffers in MojoAudioDecoder

Use data pipe for encoded buffers in MojoAudioDecoder

Create data pipe, connect the client and the service and
serialize/deserialize DecoderBuffers for this pipe.

The propagation of decoded AudioBuffers is not implemented.

BUG=542910
Committed: https://crrev.com/12a1c911094fea7f8b9452d096eb346faaf79714
Cr-Commit-Position: refs/heads/master@{#382438}

[Tima Vaisburd, 2016-03-19 04:27:15]

timav@chromium.org changed reviewers:
+ sandersd@chromium.org, xhwang@chromium.org

[Tima Vaisburd, 2016-03-19 04:27:16]

With this CL I observed the stream of packets in the log. PTAL.

[xhwang, 2016-03-19 04:57:25]

Mostly looking good. Thanks!

Just some comments.

With the comments you should have unencrypted playback working. Please help
check! :)

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/interface...
File media/mojo/interfaces/audio_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/interface...
media/mojo/interfaces/audio_decoder.mojom:25:
SetDataSource(handle<data_pipe_consumer> receive_pipe);
See comment below. I wonder whether it'll be better to just pass the
|receive_pipe| in Initialize().

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder.cc:68: 
Can we call CreateDataPipe() here and pass the |receiver_handle| as part of
Initialize()? I like your idea of only create DataPipe after initialization
succeeds. But given the fact most initialziation will succeed, this seems to be
optimizing the uncommon failure case at the expense (one more mojo call) of the
normal success case.

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder.cc:127: NOTIMPLEMENTED();
You can finish this now, it should just be:

output_cb_.Run(buffer.To<scoped_refptr<AudioBuffer>>())

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder.cc:200: options.capacity_num_bytes = 100
* 1024;
We use 512k for audio in mojo_demuxer_stream:
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder_service.cc:108: NOTIMPLEMENTED();
I think you can finish this implementation here in this CL, this will be as
simple as 

client_->OnBufferDecoded(interfaces::AudioBuffer::From(audio_buffer));

[Tima Vaisburd, 2016-03-19 22:04:47]

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/interface...
File media/mojo/interfaces/audio_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/interface...
media/mojo/interfaces/audio_decoder.mojom:25:
SetDataSource(handle<data_pipe_consumer> receive_pipe);
On 2016/03/19 04:57:25, xhwang wrote:
> See comment below. I wonder whether it'll be better to just pass the
> |receive_pipe| in Initialize().

I considered that, but thought that we'd like to have a separate call.
Eventually we would like to have full duplex pipe, i.e. both ends, like in
MojoDecryptor. Having the separate call would make the interface cleaner. And we
can only create a pipe if we need it, although I believe you are right and the
failures will be seldom
(although right now we use init failures as a mean to query system's
capabilities). WDYT?

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder.cc:127: NOTIMPLEMENTED();
On 2016/03/19 04:57:25, xhwang wrote:
> You can finish this now, it should just be:
> 
> output_cb_.Run(buffer.To<scoped_refptr<AudioBuffer>>())

Great, I will try that. There is a question though: from the first glance the
serialization/deserialization of AudioBuffer into the data pipe should be
performed exactly the same as with DecoderBuffer. Shall we just do that (data
pipe) instead? Or there is more to it?

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder.cc:200: options.capacity_num_bytes = 100
* 1024;
On 2016/03/19 04:57:25, xhwang wrote:
> We use 512k for audio in mojo_demuxer_stream:
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

We can calculate from the configuration if we know the frame duration in
advance. But we probably won't?
The maximal unencrypted frame size in 20 ms case is
48 samples/ms * 20 ms * 4 bytes/sample * 7 channels = 26880 bytes. Way less than
512k?

[xhwang, 2016-03-20 16:15:57]

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/interface...
File media/mojo/interfaces/audio_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/interface...
media/mojo/interfaces/audio_decoder.mojom:25:
SetDataSource(handle<data_pipe_consumer> receive_pipe);
On 2016/03/19 22:04:46, Tima Vaisburd wrote:
> On 2016/03/19 04:57:25, xhwang wrote:
> > See comment below. I wonder whether it'll be better to just pass the
> > |receive_pipe| in Initialize().
> 
> I considered that, but thought that we'd like to have a separate call.
> Eventually we would like to have full duplex pipe, i.e. both ends, like in
> MojoDecryptor. Having the separate call would make the interface cleaner. And
we
> can only create a pipe if we need it, although I believe you are right and the
> failures will be seldom
> (although right now we use init failures as a mean to query system's
> capabilities). WDYT?

You are right. I remembered that DataPipe creation actually involves an IPC to
the browser. So a separate call sgtm!

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder.cc:127: NOTIMPLEMENTED();
On 2016/03/19 22:04:46, Tima Vaisburd wrote:
> On 2016/03/19 04:57:25, xhwang wrote:
> > You can finish this now, it should just be:
> > 
> > output_cb_.Run(buffer.To<scoped_refptr<AudioBuffer>>())
> 
> Great, I will try that. There is a question though: from the first glance the
> serialization/deserialization of AudioBuffer into the data pipe should be
> performed exactly the same as with DecoderBuffer. Shall we just do that (data
> pipe) instead? Or there is more to it?

We can do that later. It's marked as a P3 in our doc I believe.

https://chromiumcodereview.appspot.com/1823443002/diff/1/media/mojo/services/...
media/mojo/services/mojo_audio_decoder.cc:200: options.capacity_num_bytes = 100
* 1024;
On 2016/03/19 22:04:46, Tima Vaisburd wrote:
> On 2016/03/19 04:57:25, xhwang wrote:
> > We use 512k for audio in mojo_demuxer_stream:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...
> 
> We can calculate from the configuration if we know the frame duration in
> advance. But we probably won't?
> The maximal unencrypted frame size in 20 ms case is
> 48 samples/ms * 20 ms * 4 bytes/sample * 7 channels = 26880 bytes. Way less
than
> 512k?

Why 48k is the limit and why 20ms? We'll have only one DataPipe per audio
decoder so I don't think the size here would affect any performance. I was
trying to be consistent with the Renderer, and try to err on the safer side.

[Tima Vaisburd, 2016-03-21 19:08:27]

https://codereview.chromium.org/1823443002/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1823443002/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:127: NOTIMPLEMENTED();
On 2016/03/20 16:15:57, xhwang wrote:
> On 2016/03/19 22:04:46, Tima Vaisburd wrote:
> > On 2016/03/19 04:57:25, xhwang wrote:
> > > You can finish this now, it should just be:
> > > 
> > > output_cb_.Run(buffer.To<scoped_refptr<AudioBuffer>>())
> > 
> > Great, I will try that. There is a question though: from the first glance
the
> > serialization/deserialization of AudioBuffer into the data pipe should be
> > performed exactly the same as with DecoderBuffer. Shall we just do that
(data
> > pipe) instead? Or there is more to it?
> 
> We can do that later. It's marked as a P3 in our doc I believe.

Done.

https://codereview.chromium.org/1823443002/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder.cc:200: options.capacity_num_bytes = 100
* 1024;
On 2016/03/20 16:15:57, xhwang wrote:
> On 2016/03/19 22:04:46, Tima Vaisburd wrote:
> > On 2016/03/19 04:57:25, xhwang wrote:
> > > We use 512k for audio in mojo_demuxer_stream:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...
> > 
> > We can calculate from the configuration if we know the frame duration in
> > advance. But we probably won't?
> > The maximal unencrypted frame size in 20 ms case is
> > 48 samples/ms * 20 ms * 4 bytes/sample * 7 channels = 26880 bytes. Way less
> than
> > 512k?
> 
> Why 48k is the limit and why 20ms? We'll have only one DataPipe per audio
> decoder so I don't think the size here would affect any performance. I was
> trying to be consistent with the Renderer, and try to err on the safer side.

Used 512k and added TODO to evaluate a smaller size.

https://codereview.chromium.org/1823443002/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1823443002/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:108: NOTIMPLEMENTED();
On 2016/03/19 04:57:25, xhwang wrote:
> I think you can finish this implementation here in this CL, this will be as
> simple as 
> 
> client_->OnBufferDecoded(interfaces::AudioBuffer::From(audio_buffer));

Done.

[xhwang, 2016-03-21 21:05:43]

LGTM % nits

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder.cc:201: // TODO(timav): consider capasity
calculation based on AudioDecoderConfig.
capacity

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder.h (right):

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder.h:75: OutputCB output_cb_;
nit: move output_cb to after init_cb to map the order they are set.

Maybe add a comment that output_cb is a permanent callback, the others are
one-time callback.

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:109:
client_->OnBufferDecoded(interfaces::AudioBuffer::From(audio_buffer));
Add a TODO to use DataPipe.

[Tima Vaisburd, 2016-03-21 22:43:02]

Patchset #4 (id:60001) has been deleted

[Tima Vaisburd, 2016-03-21 22:50:14]

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder.cc:201: // TODO(timav): consider capasity
calculation based on AudioDecoderConfig.
On 2016/03/21 21:05:43, xhwang wrote:
> capacity

Done.

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder.h (right):

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder.h:75: OutputCB output_cb_;
On 2016/03/21 21:05:43, xhwang wrote:
> nit: move output_cb to after init_cb to map the order they are set.
> 
> Maybe add a comment that output_cb is a permanent callback, the others are
> one-time callback.

Done.

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1823443002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:109:
client_->OnBufferDecoded(interfaces::AudioBuffer::From(audio_buffer));
On 2016/03/21 21:05:43, xhwang wrote:
> Add a TODO to use DataPipe.

Done.

[Tima Vaisburd, 2016-03-21 22:50:22]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2016-03-21 22:50:23]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1823443002/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-03-21 22:50:49]

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

[sandersd (OOO until July 31), 2016-03-21 23:12:38]

lgtm

https://codereview.chromium.org/1823443002/diff/80001/media/mojo/interfaces/a...
File media/mojo/interfaces/audio_decoder.mojom (right):

https://codereview.chromium.org/1823443002/diff/80001/media/mojo/interfaces/a...
media/mojo/interfaces/audio_decoder.mojom:24: // Established data connection.
Should be called before Decode().
nit: Should say 'Establish', or perhaps a different verb that is more clear.

[commit-bot: I haz the power, 2016-03-21 23:46:05]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-21 23:47:13]

Description was changed from

==========
Use data pipe for encoded buffers in MojoAudioDecoder

Create data pipe, connect the client and the service and
serialize/deserialize DecoderBuffers for this pipe.

The propagation of decoded AudioBuffers is not implemented.

BUG=542910
==========

to

==========
Use data pipe for encoded buffers in MojoAudioDecoder

Create data pipe, connect the client and the service and
serialize/deserialize DecoderBuffers for this pipe.

The propagation of decoded AudioBuffers is not implemented.

BUG=542910

Committed: https://crrev.com/12a1c911094fea7f8b9452d096eb346faaf79714
Cr-Commit-Position: refs/heads/master@{#382438}
==========

[commit-bot: I haz the power, 2016-03-21 23:47:14]

Patchset 4 (id:??) landed as
https://crrev.com/12a1c911094fea7f8b9452d096eb346faaf79714
Cr-Commit-Position: refs/heads/master@{#382438}