Use data pipe to pass DecoderBuffer contents

Use data pipe to pass DecoderBuffer contents

Create 2 pipes, one in each direction to pass the data from
DecoderBuffer to the other side.

BUG=510153
TEST=manual testing, since media mojo not currently used

Committed: https://crrev.com/b8752ec59cb8460728019f683b8ab1fbed6ddd62
Cr-Commit-Position: refs/heads/master@{#366514}

[jrummell, 2015-12-18 23:02:45]

jrummell@chromium.org changed reviewers:
+ xhwang@chromium.org

[jrummell, 2015-12-18 23:02:45]

PTAL

[xhwang, 2015-12-19 18:16:31]

I am not sure what's the cost of copying compressed buffers twice as we do
today. Maybe we should measure it and see whether it's worth to use shared
memory to avoid this copy.

This CL looks good. Just a few comments. Thanks!

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

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/interface...
media/mojo/interfaces/decryptor.mojom:23: // from the Decryptor.
Must be called before all other methods?

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/interface...
media/mojo/interfaces/decryptor.mojom:25: handle<data_pipe_producer>
transmit_pipe);
Since this is an interface, provide more details about how to setup and use
these two pipes... For example, what kind of data should be written to the
|transmit_pipe|, and what kind of data should be read from the |receive_pipe|...

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

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:75: DVLOG(1) << __FUNCTION__;
Probably use DVLOG(3) for repeating events.

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:78:
interfaces::DecoderBuffer::From(encrypted),
It might be more clear to do:

buffer = interfaces::DecoderBuffer::From(encrypted);
WriteDataIntoDataPipe(encrypted);
remote_decryptor_->DecryptAndDecodeAudio(...)

Actually, since we do this three times, maybe have a helper function to
encapsulate the first two lines?

buffer = interfaces::DecoderBuffer::From(encrypted);
WriteDataIntoDataPipe(encrypted);

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:136: DVLOG(1) << __FUNCTION__ << "(" <<
status << ")";
ditto about log level

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:147: DVLOG(1) << __FUNCTION__ << "(" <<
status << ")";
ditto about log level

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:154: DVLOG(3) << " video_frame: " <<
frame->AsHumanReadableString();
We probably don't need this by default.

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:170: mojo::DataPipe data_pipe2(options);
naming: it seems these should be write/read pipe, and the write/read_pipe_
should be some kind of handle...

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:183: if (!buffer->end_of_stream()) {
return early:

if (buffer->end_of_stream())
  return;

Or, probably we should only call this when the buffer is not EOS, in which case
we should DCHECK here.

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.cc:196: if (!buffer->end_of_stream()) {
ditto

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
File media/mojo/services/mojo_decryptor.h (right):

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor.h:66: mojo::ScopedDataPipeConsumerHandle
read_pipe_;
It's probably easier to understand if we just call it "producer_handle" and
"consumer_handle".

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

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor_service.cc:83: DVLOG(1) << __FUNCTION__;
ditto about log level.

https://chromiumcodereview.appspot.com/1538063003/diff/1/media/mojo/services/...
media/mojo/services/mojo_decryptor_service.cc:180: if (!buffer->end_of_stream())
{
ditto

[jrummell, 2015-12-21 23:20:55]

Updated. There are places where DVLOG(5) is used, so I was thinking of using it
to log the buffers. WDYT?

https://codereview.chromium.org/1538063003/diff/1/media/mojo/interfaces/decry...
File media/mojo/interfaces/decryptor.mojom (right):

https://codereview.chromium.org/1538063003/diff/1/media/mojo/interfaces/decry...
media/mojo/interfaces/decryptor.mojom:23: // from the Decryptor.
On 2015/12/19 18:16:30, xhwang wrote:
> Must be called before all other methods?

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/interfaces/decry...
media/mojo/interfaces/decryptor.mojom:25: handle<data_pipe_producer>
transmit_pipe);
On 2015/12/19 18:16:30, xhwang wrote:
> Since this is an interface, provide more details about how to setup and use
> these two pipes... For example, what kind of data should be written to the
> |transmit_pipe|, and what kind of data should be read from the
|receive_pipe|...

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
File media/mojo/services/mojo_decryptor.cc (right):

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:75: DVLOG(1) << __FUNCTION__;
On 2015/12/19 18:16:30, xhwang wrote:
> Probably use DVLOG(3) for repeating events.

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:78:
interfaces::DecoderBuffer::From(encrypted),
On 2015/12/19 18:16:30, xhwang wrote:
> It might be more clear to do:
> 
> buffer = interfaces::DecoderBuffer::From(encrypted);
> WriteDataIntoDataPipe(encrypted);
> remote_decryptor_->DecryptAndDecodeAudio(...)
> 
> Actually, since we do this three times, maybe have a helper function to
> encapsulate the first two lines?
> 
> buffer = interfaces::DecoderBuffer::From(encrypted);
> WriteDataIntoDataPipe(encrypted);

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:136: DVLOG(1) << __FUNCTION__ << "(" <<
status << ")";
On 2015/12/19 18:16:30, xhwang wrote:
> ditto about log level

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:147: DVLOG(1) << __FUNCTION__ << "(" <<
status << ")";
On 2015/12/19 18:16:30, xhwang wrote:
> ditto about log level

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:154: DVLOG(3) << " video_frame: " <<
frame->AsHumanReadableString();
On 2015/12/19 18:16:30, xhwang wrote:
> We probably don't need this by default.

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:170: mojo::DataPipe data_pipe2(options);
On 2015/12/19 18:16:30, xhwang wrote:
> naming: it seems these should be write/read pipe, and the write/read_pipe_
> should be some kind of handle...

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:183: if (!buffer->end_of_stream()) {
On 2015/12/19 18:16:30, xhwang wrote:
> return early:
> 
> if (buffer->end_of_stream())
>   return;
> 
> Or, probably we should only call this when the buffer is not EOS, in which
case
> we should DCHECK here.

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.cc:196: if (!buffer->end_of_stream()) {
On 2015/12/19 18:16:30, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
File media/mojo/services/mojo_decryptor.h (right):

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor.h:66: mojo::ScopedDataPipeConsumerHandle
read_pipe_;
On 2015/12/19 18:16:30, xhwang wrote:
> It's probably easier to understand if we just call it "producer_handle" and
> "consumer_handle".

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
File media/mojo/services/mojo_decryptor_service.cc (right):

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor_service.cc:83: DVLOG(1) << __FUNCTION__;
On 2015/12/19 18:16:31, xhwang wrote:
> ditto about log level.

Done.

https://codereview.chromium.org/1538063003/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_decryptor_service.cc:180: if (!buffer->end_of_stream())
{
On 2015/12/19 18:16:31, xhwang wrote:
> ditto

Done.

[xhwang, 2015-12-21 23:37:45]

LGTM!

We only have very limited usage of DVLOG(4) and DVLOG(5), so I am really not
sure whether we should use it...

[jrummell, 2015-12-22 01:31:31]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-12-22 01:32:53]

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

[commit-bot: I haz the power, 2015-12-22 01:57:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-22 01:58:36]

Description was changed from

==========
Use data pipe to pass DecoderBuffer contents

Create 2 pipes, one in each direction to pass the data from
DecoderBuffer to the other side.

BUG=510153
TEST=manual testing, since media mojo not currently used
==========

to

==========
Use data pipe to pass DecoderBuffer contents

Create 2 pipes, one in each direction to pass the data from
DecoderBuffer to the other side.

BUG=510153
TEST=manual testing, since media mojo not currently used

Committed: https://crrev.com/b8752ec59cb8460728019f683b8ab1fbed6ddd62
Cr-Commit-Position: refs/heads/master@{#366514}
==========

[commit-bot: I haz the power, 2015-12-22 01:58:37]

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